Community
Participate
Working Groups
The following test if (new CompilerOptions(getCompilerOptions()).complianceLevel < ClassFileConstants.JDK1_5) return; this.runNegativeTest( new String[] { "X.java", "public class X<p> {\n" + " void foo(p.O[] longs) {\n" + " }\n" + "}\n", }, "----------\n" + "1. ERROR in X.java (at line 2)\n" + " void foo(p.O[] longs) {\n" + " ^^^^^\n" + "Illegal qualified access from the type parameter p\n" + "----------\n"); currently throws java.lang.NullPointerException at org.eclipse.jdt.internal.compiler.lookup.Scope.createArrayType(Scope.java:701) I admit the example is stupid, but NPE still isn't a good answer.
Here's what happens: Inside ArrayQualifiedTypeReference.getTypeBinding(Scope) it is assumed that "super.getTypeBinding(scope)" will return @NonNull TypeBinding. From scanning through implementations of TypeReference.getTypeBinding(Scope) I see three exits with "return null", all others take care to return a problem binding on error. From those three exits two *could* even be safe: SingleTypeReference and QualifiedTypeReference both have s.t. like if (..detectHierarchyCycle(..)) return null; One could argue that hierarchy cycles can probably only occur during ClassScope.findSupertype(TypeReference) the result of which is indeed checked for null, hm.. BUT, the third occurrence is in QualifiedTypeReference just after problemReporter().illegalAccessFromTypeVariable(..) Instead of adding one more null-check I'd prefer to make @NonNull the contract and make everybody adhere to that.
Created attachment 184557 [details] proposed patch This patch fixes the issue by returning a ProblemReferenceBinding. This is safe and avoids the NPE. A tiny question remains: what should be the appropriate ProblemReason? By saying NotFound we get the following errors: public class X<p> { void foo(p.O[] elems) { } void bar() { foo(new Object[0]); } } ---------- 1. ERROR in X.java (at line 2) void foo(p.O[] elems) { ^^^^^ Illegal qualified access from the type parameter p ---------- 2. ERROR in X.java (at line 2) void foo(p.O[] elems) { ^^^ p.O cannot be resolved to a type ---------- 3. ERROR in X.java (at line 5) foo(new Object[0]); ^^^ The method foo(O[]) from the type X<p> refers to the missing type O ---------- the 2. error is useless, but it's probably better to live with it rather than, e.g., creating a ProblemReferenceBinding(..ProblemReasons.NoError), which might cause confusion downstream, or creating a new constant just for this special case.
Srikanth, please review. Thanks!
I wonder if we should not rather handle the null value inside org.eclipse.jdt.internal.compiler.ast.ArrayQualifiedTypeReference.getTypeBinding(Scope). In this case, I don't think reporting the second error helps. If the type is really a missing type, it is already handled as such. So we can only get null when trying to use a type variable inside a qualified type reference. Stephan, I'll propose another way to take care of this. I don't think we can simply use the missing type support for this as I don't see a good way to describe this missing type. It is not really a missing type.
Created attachment 184614 [details] Proposed fix + regression test I kept the regression test with a small modification about the expected errors. Stephan, what do you think ?
(In reply to comment #5) > Created an attachment (id=184614) [details] > Proposed fix + regression test > > I kept the regression test with a small modification about the expected errors. > Stephan, what do you think ? I agree that the secondary error is not ideal. How sure are we that ArrayQualifiedTypeReference.getTypeBinding(Scope) is the only client that has to handle the null result from QualifiedTypeReference.getTypeBinding(Scope) ? Anyway, I agree with releasing your patch for M4. I might challenge it afterwards to see if more clients are potentially affected. If you want this released for the next I-build could you please do it? I don't have the workspace configured on this machine.
I only see: org.eclipse.jdt.internal.compiler.ast.JavadocArgumentExpression.internalResolveType(Scope) that might require more work. I wrote a small test case that produced: java.lang.NullPointerException at org.eclipse.jdt.internal.compiler.ast.JavadocArgumentExpression.internalResolveType(JavadocArgumentExpression.java:53) at org.eclipse.jdt.internal.compiler.ast.JavadocArgumentExpression.resolveType(JavadocArgumentExpression.java:97) at org.eclipse.jdt.internal.compiler.ast.JavadocMessageSend.internalResolveType(JavadocMessageSend.java:64) at org.eclipse.jdt.internal.compiler.ast.JavadocMessageSend.resolveType(JavadocMessageSend.java:200) at org.eclipse.jdt.internal.compiler.ast.Javadoc.resolveReference(Javadoc.java:367) at org.eclipse.jdt.internal.compiler.ast.Javadoc.resolve(Javadoc.java:269) at org.eclipse.jdt.internal.compiler.ast.AbstractMethodDeclaration.resolveJavadoc(AbstractMethodDeclaration.java:440) at org.eclipse.jdt.internal.compiler.ast.AbstractMethodDeclaration.resolve(AbstractMethodDeclaration.java:420) at org.eclipse.jdt.internal.compiler.ast.TypeDeclaration.resolve(TypeDeclaration.java:1147) at org.eclipse.jdt.internal.compiler.ast.TypeDeclaration.resolve(TypeDeclaration.java:1257) at org.eclipse.jdt.internal.compiler.ast.CompilationUnitDeclaration.resolve(CompilationUnitDeclaration.java:540) at org.eclipse.jdt.internal.compiler.Compiler.resolve(Compiler.java:899) at org.eclipse.jdt.internal.compiler.Compiler.resolve(Compiler.java:944) at org.eclipse.jdt.internal.core.CompilationUnitProblemFinder.process(CompilationUnitProblemFinder.java:202) at org.eclipse.jdt.internal.core.CompilationUnitProblemFinder.process(CompilationUnitProblemFinder.java:268) at org.eclipse.jdt.internal.core.ReconcileWorkingCopyOperation.makeConsistent(ReconcileWorkingCopyOperation.java:190) at org.eclipse.jdt.internal.core.ReconcileWorkingCopyOperation.executeOperation(ReconcileWorkingCopyOperation.java:89) at org.eclipse.jdt.internal.core.JavaModelOperation.run(JavaModelOperation.java:728) at org.eclipse.jdt.internal.core.JavaModelOperation.runOperation(JavaModelOperation.java:788) at org.eclipse.jdt.internal.core.CompilationUnit.reconcile(CompilationUnit.java:1244) at org.eclipse.jdt.internal.ui.text.java.JavaReconcilingStrategy.reconcile(JavaReconcilingStrategy.java:126) at org.eclipse.jdt.internal.ui.text.java.JavaReconcilingStrategy.access$0(JavaReconcilingStrategy.java:108) at org.eclipse.jdt.internal.ui.text.java.JavaReconcilingStrategy$1.run(JavaReconcilingStrategy.java:89) at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:42) at org.eclipse.jdt.internal.ui.text.java.JavaReconcilingStrategy.reconcile(JavaReconcilingStrategy.java:87) at org.eclipse.jdt.internal.ui.text.java.JavaReconcilingStrategy.initialReconcile(JavaReconcilingStrategy.java:178) at org.eclipse.jdt.internal.ui.text.CompositeReconcilingStrategy.initialReconcile(CompositeReconcilingStrategy.java:114) at org.eclipse.jdt.internal.ui.text.JavaCompositeReconcilingStrategy.initialReconcile(JavaCompositeReconcilingStrategy.java:133) at org.eclipse.jface.text.reconciler.MonoReconciler.initialProcess(MonoReconciler.java:105) at org.eclipse.jdt.internal.ui.text.JavaReconciler.initialProcess(JavaReconciler.java:398) at org.eclipse.jface.text.reconciler.AbstractReconciler$BackgroundThread.run(AbstractReconciler.java:173) So I'll target 6pm build and not 1pm.
Created attachment 184635 [details] Proposed fix + regression tests This is fixing the javadoc case as well.
I'll run all tests and if ok, I'll release for M4.
Created attachment 184639 [details] Proposed fix + regression tests More regression tests.
I could see that there are quite a few calls to Scope.createArrayType(..) from other places as well, and all of them are not protected with a null check. Shouldn't we guard every call (or atleast those where we cannot ascertain nullity of the first argument) to prevent an NPE ?
(In reply to comment #11) > I could see that there are quite a few calls to Scope.createArrayType(..) from > other places as well, and all of them are not protected with a null check. > Shouldn't we guard every call (or atleast those where we cannot ascertain > nullity of the first argument) to prevent an NPE ? My impression is that those callers are right in not expecting null. I suggest to release Olivier's patch to M4 and during M5 I will propose a solution that avoids the null within createArrayType, which might include introducing a new ProblemReason and defer reporting until ProblemReport.invalidType(..) (I might have a more qualified statement within 2 hours or so ...)
I opened bug 331936 to follow up post M4.
Released last patch to HEAD. Regression tests added in: org.eclipse.jdt.core.tests.compiler.regression.ArrayTest#test018 org.eclipse.jdt.core.tests.compiler.regression.JavadocTest_1_5#testBug331872 org.eclipse.jdt.core.tests.compiler.regression.JavadocTest_1_5#testBug331872b org.eclipse.jdt.core.tests.compiler.regression.JavadocTest_1_5#testBug331872c org.eclipse.jdt.core.tests.compiler.regression.JavadocTest_1_5#testBug331872d
I agree with the latest patch and the approach planned.
Verified using I20101207-0250 (4.1 I-build)
