Community
Participate
Working Groups
Build ID: M20080911-1700 Steps To Reproduce: 1. Create a clean Java project with two source folders (e.g. "src" and "test"). Both folders should use the same output folder. 2. Create a package in the "src" folder (e.g. "my.foo"), and the same package in the "test" folder. 3. Add a "package-info.java" file to that package in both folders. It is important that there is also a javadoc comment in the file. 4. Clean the project. 5. Look at the list of problems. An example "package-info.java" file is: ------------------ 8< --------------- /** * A demo package for foo. */ package my.foo; ------------------ 8< --------------- More information: In the list of Problems you'll find a report line with the description "The type package-info is already defined". This line is expected. But you'll also find a related NPE report: Internal compiler error java.lang.NullPointerException at org.eclipse.jdt.internal.compiler.ast.Javadoc.resolve(Javadoc.java:247) at org.eclipse.jdt.internal.compiler.ast.CompilationUnitDeclaration.resolve(CompilationUnitDeclaration.java:520) at org.eclipse.jdt.internal.compiler.Compiler.process(Compiler.java:743) at org.eclipse.jdt.internal.compiler.ProcessTaskManager.run(ProcessTaskManager.java:137) at java.lang.Thread.run(Thread.java:619)
Created attachment 117877 [details] Build test patch to test for bug 252555 Patch to PackageInfoTest.Java. Test to reproduce problem scenario
Created attachment 118036 [details] Cumulative patch with fixes and tests Basically the problem here was several folds: 1. There was really no support for handling Javadoc comments occuring at the compilation unit level. A manifestation of this is the fact that even with Window + preferences + Java + compiler + javadoc + ((report error on malformed tags)/(validate tags)) enabled, we don't issue any diagnostics about completely bogus tags occurring inside javadoc comments at the CU level, e.g: /** * @param blah blah * */ package pack.age; /** * * @param blah blah */ public class agepack { /** * * @param args junk * @param ghost missing */ public static void main(String args[]) { System.out.println("Hello!"); } } the @param blah blah at the top level comment doesn't elicit any message from the compiler. 2. There is really no consistent treatment of javadoc comments at the CU level. (a) From a validation/resolve level we were not any checking at all (per above), But from a completion/assit p.o.v, we were filtering the permissible tags to be the same as class scope tags and finally if the CU happened to be package-info.java, we were resolving the javadoc using a method scope - i.e, as though the Javadoc comment was applied to a method. 3. This "resolution" of package level javadoc comments using method scope is the triggering point for the NPE since in the given test scenario, there is an error which causes the types from being fully initialized, resulting in the method scope field being null. The fix was to NOT use the erroneous (method) scope at all and instead add full support for validating the set of tags that can figure at the CU level.
See that annotations at the package level get validated similarly (erroneously) using MethodScope. I haven't yet verified if the code handling annotations is any more resilient to the method scope being null. If that code also runs into problem (and likely even otherwise), I'll be raising a separate defect to track that problem.
(In reply to comment #2) > Created an attachment (id=118036) [details] > Cumulative patch with fixes and tests > > > Basically the problem here was several folds: > > 1. There was really no support for handling Javadoc comments occuring at the > compilation unit level. A manifestation of this is the fact that even with > Window + preferences + Java + compiler + javadoc + ((report error on malformed > tags)/(validate tags)) enabled, we don't issue any diagnostics about completely > bogus tags occurring inside javadoc comments at the CU level, e.g: ... > the @param blah blah at the top level comment doesn't elicit any message from > the compiler. > This was intentional to mimic the javadoc tool behavior: on your example, it won't raise any warning for this tag while generating the documentation... One of the rule for our compiler javadoc warnings is not to raise any warning if the javadoc tool does not or have a specific preference for this kind of warning. The other important rule is: always report a warning when the javadoc tool does... > 2. There is really no consistent treatment of javadoc comments at the CU level. > > (a) From a validation/resolve level we were not any checking at all (per > above), But from a completion/assit p.o.v, we were filtering the permissible > tags to be the same as class scope tags and finally if the CU happened to be > package-info.java, we were resolving the javadoc using a method scope - i.e, as > though the Javadoc comment was applied to a method. > I agree that code assist should only propose tags for the package-info javadoc comment... I think a separate bug can be opened for this. > 3. This "resolution" of package level javadoc comments using method scope is > the triggering point for the NPE since in the given test scenario, there is an > error which causes the types from being fully initialized, resulting in the > method scope field being null. > > The fix was to NOT use the erroneous (method) scope at all and instead add full > support for validating the set of tags that can figure at the CU level. > As per my answer to point 1), I think that it would be enough to check that the static initializer scope was not null before resolving the javadoc... Note that when this scope is not null we definitely need it to be able to resolve bindings in the javadoc comments.
(In reply to comment #4) > (In reply to comment #2) > > Created an attachment (id=118036) [details] [details] > > Cumulative patch with fixes and tests [...] > This was intentional to mimic the javadoc tool behavior: on your example, it > won't raise any warning for this tag while generating the documentation... > One of the rule for our compiler javadoc warnings is not to raise any warning > if the javadoc tool does not or have a specific preference for this kind of > warning. The other important rule is: always report a warning when the javadoc > tool does... Thanks for shedding light on this. It is a bit surprising that we would - lose up an opportunity to do better than javadoc - be totally silent even with the validate tags check box enabled. That said, I can see the value in NOT being too chatty and drowning the user with too many messages that bring down the signal to noise ratio. > I agree that code assist should only propose tags for the package-info javadoc > comment... I think a separate bug can be opened for this. Are you suggesting we offer no proposal at all for the CU level ? Or are you suggesting we offer the same set of tags for CU as we do for package level ? (currently for completion at CU level javadoc, we offer the same proposal as we would for class javadoc comment) > As per my answer to point 1), I think that it would be enough to check that the > static initializer scope was not null before resolving the javadoc... OK, I am comfortable with that since the MethodScope being used should come out as null only if there is an error already. In that event, short circuiting the javadoc resolution should be alright I think. > Note that when this scope is not null we definitely need it to be able to > resolve bindings in the javadoc comments. Just for my edification: Are you saying there is some reason why these bindings MUST be resolved using MethodScope ? The proposed patch augments the capability to validate the CU level tags using CompilationUnitScope. Other than the (justified) concern you have raised that this would have the side effect of rejecting certain malformed javadoc constructs that do pass through SDK's javadoc, is there some reason why that treatment would be inadequate ?
(In reply to comment #5) > (In reply to comment #4) > > (In reply to comment #2) > > > Created an attachment (id=118036) [details] [details] [details] > > > Cumulative patch with fixes and tests > > [...] > > Thanks for shedding light on this. It is a bit surprising that we would > > - lose up an opportunity to do better than javadoc > - be totally silent even with the validate tags check box enabled. > > That said, I can see the value in NOT being too chatty and drowning the user > with too many messages that bring down the signal to noise ratio. > I've opened bug 255626 for this. Instead of parsing/resolving the javadoc comment, we may want to warn the user that it won't be generated by the javadoc tool... We can do better than the javadoc tool, but this must be always controlled by a preference set by default to have the same behavior than the javadoc tool. Then, users will not have any unexpected new warnings when they get the build including this new compiler diagnosis. > > I agree that code assist should only propose tags for the package-info javadoc > > comment... I think a separate bug can be opened for this. > > Are you suggesting we offer no proposal at all for the CU level ? Or are you > suggesting we offer the same set of tags for CU as we do for package level ? > (currently for completion at CU level javadoc, we offer the same proposal as we > would for class javadoc comment) > I would suggest not to propose any tags as our compiler won't parse this kind of comment... I agree that the current behavior is buggy as only the tags of JavadocTagConstants.PACKAGE_TAGS array should be proposed at least. So, we definitely need a separate bug for this issue! > > As per my answer to point 1), I think that it would be enough to check that the > > static initializer scope was not null before resolving the javadoc... > > OK, I am comfortable with that since the MethodScope being used should come out > as null only if there is an error already. In that event, short circuiting the > javadoc resolution should be alright I think. > :-) > > Note that when this scope is not null we definitely need it to be able to > > resolve bindings in the javadoc comments. > > Just for my edification: Are you saying there is some reason why these bindings > MUST be resolved using MethodScope ? The proposed patch augments the capability > to validate the CU level tags using CompilationUnitScope. Other than the > (justified) concern you have raised that this would have the side effect of > rejecting certain malformed javadoc constructs that do pass through SDK's > javadoc, is there some reason why that treatment would be inadequate ? > No, I wanted to say that we need a scope to resolve bindings. If CUScope is better, then it's ok for me to use it instead...
Created attachment 118224 [details] Patch with review comments/suggestions incorporated
(In reply to comment #6) > (In reply to comment #5) > > (In reply to comment #4) > > > (In reply to comment #2) > > > > Created an attachment (id=118036) [details] [details] [details] [details] [...] > > Are you suggesting we offer no proposal at all for the CU level ? Or are you > > suggesting we offer the same set of tags for CU as we do for package level ? > > (currently for completion at CU level javadoc, we offer the same proposal as we > > would for class javadoc comment) > > > I would suggest not to propose any tags as our compiler won't parse this kind > of comment... I agree that the current behavior is buggy as only the tags of > JavadocTagConstants.PACKAGE_TAGS array should be proposed at least. > So, we definitely need a separate bug for this issue! I have opened bug 255752 for this. > > > As per my answer to point 1), I think that it would be enough to check that the > > > static initializer scope was not null before resolving the javadoc... > > > > OK, I am comfortable with that since the MethodScope being used should come out > > as null only if there is an error already. In that event, short circuiting the > > javadoc resolution should be alright I think. > > > :-) OK, this fix in place. Apart from the problem exposed by the submitter's test case, I also fixed a few other places where this null reference could bite us: in CompilationUnitDeclaration.resolve and CompilationUnitDeclaration.traverse. Philippe and Frederic, please review and help take this to commit. Thanks
(In reply to comment #8) > > I have opened bug 255752 for this. > Thanks > > OK, this fix in place. Apart from the problem exposed by the submitter's test > case, I also fixed a few other places where this null reference could bite us: > in CompilationUnitDeclaration.resolve and CompilationUnitDeclaration.traverse. > > Philippe and Frederic, please review and help take this to commit. Thanks > Patch looks good to me
Comment on attachment 118224 [details] Patch with review comments/suggestions incorporated Unfortunately, I made my previous review just by reading the patch file... While applying it in a workspace, I saw that there's was a warning in the added test: unused local variable. I also think that it's a good inhabit to put a reference to the bug in the test method comment (I like a javadoc comment to allow useful links, e.g. in JavadocBugsTest; but teammates also use a simple line comment). Also, in CompilationUintDeclaration.resolve() method, I think the 2 line comments should be put in the block comment above or at least no empty line between block and line comments... Finally, in the block comment mentioned above, I think it would be better to change the phrase: * we do it now and the javadoc in the fake type won't be resolved as: * we need to do it now as the javadoc in the fake type won't be resolved... Sorry, for having missed these remarks on the first pass...
Created attachment 118563 [details] Patch with cleanups. Clean up : eliminated unused variable, mentioned defect number in test, merged comments ..
Comment on attachment 118563 [details] Patch with cleanups. OK for me
(In reply to comment #12) > (From update of attachment 118563 [details]) > OK for me > Released for 3.5M4 in HEAD stream.
Jerome, even if this is not a regression, the fix is so simple and safe that I think it could be a candidate for 3.4.2. What's your mind about it?
+1 for backporting to 3.4.2
Created attachment 118867 [details] Patch for backport to 3.4.2 HEAD patch unsuitable for 3.4.2, regenerated for the R_3.4_maintenence branch and attached.
Released for 3.4.2 in R3_4_maintenance stream.
When I paste the package-info.java in the second source folder, I get: Java Model Exception: Java Model Status [package-info [in package-info.java [in my.foo [in src [in 252555]]]] does not exist] at org.eclipse.jdt.internal.core.JavaElement.newNotPresentException(JavaElement.java:492) at org.eclipse.jdt.internal.core.JavaElement.openWhenClosed(JavaElement.java:526) at org.eclipse.jdt.internal.core.JavaElement.getElementInfo(JavaElement.java:252) at org.eclipse.jdt.internal.core.JavaElement.getElementInfo(JavaElement.java:238) at org.eclipse.jdt.internal.core.Member.getNameRange(Member.java:316) at org.eclipse.jdt.internal.core.builder.AbstractImageBuilder.createProblemFor(AbstractImageBuilder.java:393) at org.eclipse.jdt.internal.core.builder.AbstractImageBuilder.acceptResult(AbstractImageBuilder.java:178) at org.eclipse.jdt.internal.compiler.Compiler.compile(Compiler.java:484) at org.eclipse.jdt.internal.core.builder.AbstractImageBuilder.compile(AbstractImageBuilder.java:363) at org.eclipse.jdt.internal.core.builder.IncrementalImageBuilder.compile(IncrementalImageBuilder.java:313) at org.eclipse.jdt.internal.core.builder.AbstractImageBuilder.compile(AbstractImageBuilder.java:300) at org.eclipse.jdt.internal.core.builder.IncrementalImageBuilder.build(IncrementalImageBuilder.java:133) at org.eclipse.jdt.internal.core.builder.JavaBuilder.buildDeltas(JavaBuilder.java:265) at org.eclipse.jdt.internal.core.builder.JavaBuilder.build(JavaBuilder.java:193) at org.eclipse.core.internal.events.BuildManager$2.run(BuildManager.java:633) at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:37) at org.eclipse.core.internal.events.BuildManager.basicBuild(BuildManager.java:170) at org.eclipse.core.internal.events.BuildManager.basicBuild(BuildManager.java:201) at org.eclipse.core.internal.events.BuildManager$1.run(BuildManager.java:253) at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:37) at org.eclipse.core.internal.events.BuildManager.basicBuild(BuildManager.java:256) at org.eclipse.core.internal.events.BuildManager.basicBuildLoop(BuildManager.java:309) at org.eclipse.core.internal.events.BuildManager.build(BuildManager.java:341) at org.eclipse.core.internal.events.AutoBuildJob.doBuild(AutoBuildJob.java:140) at org.eclipse.core.internal.events.AutoBuildJob.run(AutoBuildJob.java:238) at org.eclipse.core.internal.jobs.Worker.run(Worker.java:55)
Ok, closing as RESOLVED/FIXED. I'll open a new bug for the new stacktrace. It is different from the original statcktrace.
Verified for 3.5M4 using I20081208-1800
Open bug 258145 for issue reported in comment 18.
Verified for 3.4.2 using M20090127-1710.
