Throw FileNotFoundException in Source.fromResource

[nogurenn]

Fixes scala/bug#11743

Commits this comment quip. The quip makes sense to me. Just two questions.

1. Would IllegalArgumentException be okay over FileNotFoundException?
2. Is there a common workflow for running tests for this? I haven't really touched that.

[nogurenn]

I think the CLA submission is manually approved. Signed it just now. The CLA check is not yet verified.

[lrytz]

This is a potentially breaking change - should it go to 2.14? Or do we consider it a bugfix?

[eed3si9n]

@lrytz Currently it NPEs. This just explicitly throws an exception containing the argument, so I think it's bug improvement.

[eed3si9n]

@nogurenn Thanks for the contribution!
I added a docs a while back on various tests used in scala/scala here - https://github.com/scala/scala/blob/2.13.x/CONTRIBUTING.md#tests. I haven't checked, but I'd guess that JUnit might be the best fit for this kind of testing. See https://github.com/scala/scala/blob/2.13.x/test/junit/scala/collection/ArrayOpsTest.scala etc.

[som-snytt]

At the office, we were joking about an improvement to throw NullPointerException with an explanatory message. For this case, I'm not sure IllegalArgument is correct or more precise. (The arg is OK, it just happens to not represent an existing resource.) I guess there is no ResourceNotFoundException, that's just something we add to every project ever? My point (or null point, if you will) is that the fix could also throw NPE with the message. It's not a big deal either way, but I wanted to record this note for future reference.

[eed3si9n]

@som-snytt https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/lang/ClassLoader.html#getResourceAsStream(java.lang.String) says that it will return "null if the resource could not be found."
So the onus is on the caller of ClassLoader#getResourceAsStream to check for nullness before proceeding further.

If the original method threw an exception, I agree that the original exception should be preserved as a cause, but in this case, ClassLoader#getResourceAsStream returned null as a valid Java response.

In general, it might be an interesting analysis to check all Java call sites and see if we're doing proper null checking, especially in scala-library.

[lrytz]

https://docs.oracle.com/javase/8/docs/api/java/util/MissingResourceException.html maybe?

[som-snytt]

If I had a blog, I would already have posted something about all the stuff @lrytz does. I was thinking how he's been doing reviews and also the recent super call PR.

The previous comment is a typical @lrytz contribution on the order of, I'm not above helping you figure out some stuff.

This is my new favorite "javaism":

Gets parameter passed by constructor.

[lrytz]

recent super call PR

it got me soooo close to a destructive gesture against my laptop... thankfully @retronym's status on our slack reminded me that i'm not alone:

[Jasper-M]

This is my new favorite "javaism":

Gets parameter passed by constructor.

That should be automatically added to the scaladoc of getters for constructor arguments annotated with @BeanProperty.

[nogurenn]

IllegalArgumentException is different from what this context needs. MissingResourceException sounds good, but I think we should go with FileNotFoundException as it has greater specificity over a generic "resource". Is this okay?

[lrytz]

FileNotFoundException sounds good to me 👍

If we were doing this API from scratch we'd probably return an Option, but changing this method in isolation will probably be inconsistent. Also not binary compatible.

[SethTisue]

resources aren't files; FileNotFoundException seems inappropriate to me here. whereas MissingResourceException seems exactly appropriate. am I missing something?

[som-snytt]

Resource as in ResourceBundle. Apples and oranges.

Why is it "source from resource". You'd think the re-source would come from a source.

Maybe Source.fromClassLoader. Then throw MissingSourceException that extends ReflectiveOperationException like ClassNotFound does.

[SethTisue]

Resource as in ResourceBundle. Apples and oranges.

Ah, I see.

(FileNotFoundException still rubs me the wrong way a little, but whatevs)

[nogurenn]

I take it we're still going for FileNotFoundException? Either way, do we need a better error message when this gets thrown? resource not found could potentially be confusing, though maybe that's just me as a novice. If someone uses this API, they should probably have an idea what things like Source, ResourceBundles are anyway.

[SethTisue]

I take it we're still going for FileNotFoundException?

yes, that seems to be the consensus-except-for-me :-)

[SethTisue]

we usually ask for test coverage, but this seems like such a straightforward change I wouldn't insist on it. I assume you've tested it manually?

[ashawley]

Not to be nihlistic, but the method doesn't return a File nor a Resource.  It retrieves a Source object, an interface to string/char data from a file, or an "iterable representation of a source file".    The suggestion ofMissingResourceException is accurate, but the FileNotFoundException seems less surprising to me, IMHO.    The decision here may come down to style and to what user's expect, and not what's semantically correct.

[ashawley]

I agree. A test could probably be added to test/junit/scala/io/SourceTest.scala. Something along the lines of:

  @Test(expected = classOf[java.io.FileNotFoundException])
  def loadFromMissingResource(): Unit = {
    Source.fromResource("missing.txt")
  }

The commits need to be squashed to one and given an appropriate commit message.

The only procedural question I have is: If this is a bug, maybe this should be fixed in 2.12, as well?

[SethTisue]

The only procedural question I have is: If this is a bug, maybe this should be fixed in 2.12, as well?

Meh, it's so late in the 2.12.x series, I think it's better to err on the side of stability, leaving existing behaviors unchanged unless they are blatantly incorrect.

[som-snytt]

There is a facility:

import scala.tools.testkit.AssertUtil.assertThrows
@Test def loadFromMissingResource(): Unit = assertThrows[FileNotFoundException] {
  Source.fromResource("missing.txt")
}

Could also get a tmp file name, to ensure that it really doesn't exist.

[nogurenn]

Here's what I tried. I looked for a random file inside src and tried it within the project sbt scala console. Is everything as expected here?

scala> Source.fromResource("rootdoc.txt")
res2: scala.io.BufferedSource = <iterator>

scala> Source.fromResource("rootdoc.tx")
java.io.FileNotFoundException: resource 'rootdoc.tx' was not found
  at scala.io.Source$.fromResource(Source.scala:181)
  ... 29 elided

[nogurenn]

Would it help to adjust the error message to
resource '$resource' not found in resource bundles.?