Bug 338370

Comment 1 Philippe Marschall 2011-02-28 03:48:49 EST

Created attachment 189915 [details]
patch that throws FileNotFoundException

Comment 2 Walter Harley 2011-02-28 13:17:27 EST

Thanks for finding that and for providing the patch!

Filer.getResource is documented to throw an IOException if the file cannot be found, so I agree with the proposed change.

Olivier, I wonder if instead of catching the null and converting it to a FileNotFoundException in the dispatcher, we should just throw FileNotFoundException directly from EclipseFileManager.getFileForInput()?  I don't have the source code handy right now but I'll try to look at this late tonight or tomorrow unless you get a chance sooner.

Comment 3 Olivier Thomann 2011-02-28 13:25:51 EST

A test case would be welcome.
Walter, you also need to set the iplog property and the copyrights/contribution of this file need to be updated.

We should indeed investigate if the exception should not be thrown by the file manager directly. It would be good to check what javac is doing in this case.

Comment 4 Olivier Thomann 2011-03-01 10:27:59 EST

Walter,

Can you take care of this for M6 (next week) or should I do it ?

Comment 5 Walter Harley 2011-03-01 14:40:24 EST

I think I can get it in for M6.  I want to do a little research first to see if we should throw an exception from getFileForInput().

Comment 6 Walter Harley 2011-03-03 04:26:06 EST

The file manager that is used by BatchFilerImpl may be passed in by the compiler and may even come from an outside implementation via the Tools (JSR199) API.  The JavaFileManager.getFileForInput() method is documented by Sun/Oracle as "might return null if the file does not exist" - how's that for precise API documentation?  "Might"??

So it seems we cannot rely on the file manager to throw an exception, and we need to throw it ourselves inside BatchFilerImpl, as in Philippe's original suggestion.

Comment 7 Olivier Thomann 2011-03-03 12:25:30 EST

This should be released with a regression test.

Comment 8 Walter Harley 2011-03-03 14:26:51 EST

Yep, I agree.  I didn't do it last night because I didn't have time to write the test... will try again tonight.

Comment 9 Philippe Marschall 2011-03-04 08:52:32 EST

Created attachment 190382 [details]
sample processor

A really simple sample processor that triggers the bug.

Comment 10 Philippe Marschall 2011-03-04 08:52:59 EST

Created attachment 190383 [details]
sample processor user

a really simple sample processor user

Comment 11 Philippe Marschall 2011-03-04 08:55:25 EST

I added two files. A really simple processor and user that should trigger the issue. They work fine when running in Eclipse JDT but should throw a NullPointerException when running the batch compiler. I hope this helps you write a regression test.

Comment 12 Olivier Thomann 2011-03-04 10:01:20 EST

Ok, I'll prepare a patch with a regression test and I'll request a review from Walter.

Comment 13 Olivier Thomann 2011-03-04 11:06:44 EST

(In reply to comment #0)
> javac throws a FileNotFoundException in this case so this patch introduces the
> same behavior.
Philippe, what version of javac have you tried ?
When I try it with a jdk6 version, the file not found exception is not thrown. If I try with a JDK7 (b131), then it works.

Comment 14 Olivier Thomann 2011-03-04 11:22:01 EST

Created attachment 190406 [details]
Proposed fix + regression test

Patch that contains the fix and a regression test. The system compiler is not tested as I could not get the test to pass on a JDK6 VM. It works on a JDK7, but this is not the VM used for the Eclipse tests.

Walter, please review.
Philippe, I would like to understand how you got this to work with javac.

Comment 15 Olivier Thomann 2011-03-04 11:30:22 EST

Created attachment 190408 [details]
Proposed fix + regression test

Same patch removing an unused import

Comment 16 Walter Harley 2011-03-04 12:46:32 EST

I will review tonight & release for M6 warmup build if it looks good.

Comment 17 Walter Harley 2011-03-05 14:46:05 EST

Committed to HEAD; will be released in M6.

Comment 18 Philippe Marschall 2011-03-16 09:49:41 EDT

Oliver you're right. I went over the code with jdk6 again and the the following line

messenger.printMessage(Diagnostic.Kind.ERROR, "no exception raised");

is definitely reached. Sorry I didn't test the sample code more throughly.

Comment 19 Olivier Thomann 2011-03-16 09:54:43 EDT

(In reply to comment #18)
> Oliver you're right. I went over the code with jdk6 again and the the following
> line
> messenger.printMessage(Diagnostic.Kind.ERROR, "no exception raised");
> is definitely reached. Sorry I didn't test the sample code more throughly.
Ok, thanks for confirming. This means that the fix is right (we are fixing a bug), but we might end up being out-of-sync with the way javac behaves. In this case, I think it is perfectly valid.