Community
Participate
Working Groups
Build ID: Eclipse 3.3RC1 Steps To Reproduce: I was running CompletionContextTests_1_5.test0006() from org.eclipse.jdt.core.tests.model. Accidentally I still had the following file in my workspace (which has been removed on Sep 6 2006): org.eclipse.jdt.core.tests.model/workspace/Completion/src3/pkgannotation/QQAnnotation.java Note that the content of this file had a wrong package declaration (with an "s" unlike the containing directory): package pkgannotations; The NPE occurs in CompletionEngine.proposeType() at this line: if (!guessedType.isValidBinding()) return; Analysis: Variable guessedType is obtained from TypeReference.resolveType(BlockScope) Resolving internally reports this problem: Pb(2) pkgannotations cannot be resolved to a type (This happens because an attempt is made to resolve "pkgannotations.QQAnnotation" to a type). Afterwards the method returns null, not a ProblemBinding. Thus the result must be checked for null. (It seems that isValidBinding() would always return true?)
To reproduce the bug outside a junit test: 1) create a project P 2) create an annotation pkgannotation/QQAnnotation.java package pkgannotations; @interface QQAnnotation { } 3) create a class p/Test.java package p; public class Test { @|ZZZ void foo() { } } 4) do ctrl+space at | location java.lang.NullPointerException at org.eclipse.jdt.internal.codeassist.CompletionEngine.proposeType(CompletionEngine.java:733) at org.eclipse.jdt.internal.codeassist.CompletionEngine.acceptTypes(CompletionEngine.java:610) at org.eclipse.jdt.internal.codeassist.CompletionEngine.findTypesAndPackages(CompletionEngine.java:6739) at org.eclipse.jdt.internal.codeassist.CompletionEngine.complete(CompletionEngine.java:1482) at org.eclipse.jdt.internal.codeassist.CompletionEngine.complete(CompletionEngine.java:2098) at org.eclipse.jdt.internal.core.Openable.codeComplete(Openable.java:123) at org.eclipse.jdt.internal.core.CompilationUnit.codeComplete(CompilationUnit.java:327) at org.eclipse.jdt.internal.core.CompilationUnit.codeComplete(CompilationUnit.java:320)
The bug has been added with the fix for bug 158985. Code assist give completion proposals in 3.2, so this is a regression.
Created attachment 70297 [details] Proposed fix 'guessedType' can be null. This patch add a null check inside proposeType().
This bug could be fixed for 3.3RC4. Jerome, Frederic, Philippe, Daniel could you review this patch ?
+1 for 3.3 RC4 (I guess as the target is not set...) Just one cosmetic point: in the test, package of first CU is test0006. I guess it's a copy/paste typo and should be test0025 instead...
To reproduce the problem the following annotation must be used instead of the annotation of comment 1. package pkgannotations; public @interface QQAnnotation { }
Is it really a show stopper ? The scenario seems like a corner case. Can't it wait until 3.3.1 ? Talking with David, it seems that the null check could protect from other bad situations as well. Now, the fix is really trivial... David - pls assess risk if we fix, risk if we don't fix.
The fix is trivial. I would approve this for 3.3 RC4 unless David's riks assessment speaks against that.
I investigated to find another test case that thrown the same exception and i didn't find one. So the only way to reproduce it is a corner case: the completion must occur inside an annotation reference and an annotation must be declared with the wrong package name. The fix is trivial but the only way to reproduce the bug is a corner case. If we don't fix this bug then there is no proposal and a window appear to signal that a completion computer did not complete normally. As we are late in the 3.3 release stream and the bug isn't a show stopper, then this bug doesn't require to be fixed in 3.3 and could be fixed in 3.3.1.
While agreeing that this is definitely no show-stopper, here are a few more details I collected plus a suggestion (for post 3.3): - A slightly different way to produce the bug is by correcting the package declaration but leaving the annotation type as package visible. In that case a NotVisible ProblemBinding will be created, everything else happens exactly as before => NPE. Still nothing dramatic. - Browsing through all implementations and clients of TypeReference.resolveType() (both overloaded versions), it is clear that this method only has two possible returns: null or a valid TypeBinding. For one implementation this policy is not easy to validate: + ParameterizedQualifiedTypeReference.internalResolveType() The data flow for the final return is non-trivial. Might be interesting to actually analyze. 3 clients actually use a (unnecessary) check isValidBinding() on the result of resolveType(): + ThrowStatement.resolve() + ASTNode.resolveDeprecatedAnnotations() + CompletionEngine.proposeType() (all classes from ...compiler.ast) The overwhelming majority if very consistent. Well done! I.e.: I couldn't find another occurrence of this bug ;-) Here's my suggestion: isn't this kind of policy (null or valid) worth documenting? IMHO that would facility maintenance and extension. The unnecessary checks don't hurt for the functionality, but they tend to confuse people trying to understand the code (like me): a good monkey will copy this kind of code and sooner or later nobody will remember the original policy. Just my 2c.
+1 for 3.3.1
Released for 3.4M1 Test added CompletionContextTests#test0025() I will enter another bug for the unnecessary call of isValidBinding() because this is not the same problem. I will prepare a patch for 3.3.1
I entered the bug 195810
Created attachment 73303 [details] Proposed patch for 3.3.1
Released for 3.3.1 Test added CompletionContextTests#test0025()
Verified for 3.4M1 using build I20070802-0800.
Verified for 3.3.1 using M20070831-2000.
