Community
Participate
Working Groups
RC2 In the following code, @Test is not imported, but the SimpleName 'Test' has still a binding. binding.getName() throws an NPE, see below. See also bug 139889. By design, bindings should be either complete or not there. ---- package p1; import java.util.List; public class C { /** * @category fo */ @Test private int fXoo; } -- java.lang.NullPointerException at org.eclipse.jdt.core.dom.AnnotationBinding.getName(AnnotationBinding.java:104) at org.eclipse.jdt.astview.views.Binding.getChildren(Binding.java:78) at org.eclipse.jdt.astview.views.ASTViewContentProvider.getChildren(ASTViewContentProvider.java:95) at org.eclipse.jdt.astview.views.ASTViewContentProvider.hasChildren(ASTViewContentProvider.java:245) at org.eclipse.jface.viewers.AbstractTreeViewer.isExpandable(AbstractTreeViewer.java:1860) at org.eclipse.jface.viewers.TreeViewer.isExpandable(TreeViewer.java:830) at org.eclipse.jface.viewers.AbstractTreeViewer.isExpandable(AbstractTreeViewer.java:1883) at org.eclipse.jface.viewers.AbstractTreeViewer.updatePlus(AbstractTreeViewer.java:2454) at org.eclipse.jface.viewers.AbstractTreeViewer.createTreeItem(AbstractTreeViewer.java:739)
Reproduced. The problem comes from the resolution of the annotation. We have this code in: TypeBinding typeBinding = this.type.resolveType(scope); if (typeBinding == null) { if (initializeCompilerAnnotation) this.compilerAnnotation = new AnnotationBinding(this); return null; } So even if the type binding is null, we still have a annotation binding. We can either fix this in the resolution of the annotation to return null or we fix the binding resolver in the ast world.
What do you expect in the case where Test is a class for example ? Do you expect null? I would say so.
If Test is a class, I would expect: @Test .resolveAnnotationBinding() => null but Test.resolveBinding() or resolveTypeBinding() => Type Test. Do you agree with this?
Theodora, Do you have any idea why an annotation binding is set for an annotation even if the type cannot be resolved or is not an annotation? I would expect that nothing is set in the compilerAnnotation field in these cases.
CC'ing Tim Hanson on this one for some insight.
(In reply to comment #4) > Do you have any idea why an annotation binding is set for an annotation even if > the type cannot be resolved or is not an annotation? I think this is a mistake. I believe Theodora was emulating the behaviour of our compiler which would create partial bindings. > I would expect that nothing is set in the compilerAnnotation field in these > cases. I agree.
So I will propose a fix by changing the resolution of the compiler annotation. There is no need to make any change on the DOM side.
Created attachment 40509 [details] Proposed fix
Created attachment 40510 [details] Regression test
This patch prevents incomplete bindings from being returned when resolveAnnotationBinding() is called.
Tim, Could you please test that patch in APT to be sure that you don't have any assumption made on the fact that compilerAnnotation is not null? Thanks. All existing DOM tests are passing as well as the two regression tests in the patch.
All APT tests pass.
Philippe, Do you want it as a candidate for RC4?
+1 for 3.2RC4. Martin & Tim/Jess: please cast your votes.
Created attachment 40727 [details] Better patch Tim, This patch is not setting a value in the compilerAnnotation field in case the binding cannot be resolved or the type binding is not an annotation. Could you please check that the APT tests are still ok with it? Thanks.
Olivier, the last added line in the change, is it necessary? Don't you want to just eleminate the line if (initializeCompilerAnnotation)?
In case the type cannot be resolved or the type is not an annotation, there is nothing to do even if initializeCompilerAnnotation is set to true.
I simply moved the code that initialize the initializeCompilerAnnotation temp variable down to where it is used. Right now it is set to true, but I wanted to preserve the comment behind. Unclear why this is always set to true.
Tim, Would you know why the initializeCompilerAnnotation is set to true? It looks better to use: boolean initializeCompilerAnnotation = scope.compilerOptions().storeAnnotations;, doesn't it?
All APT test pass. +1 for RC4. I don't know why the options aren't being consulted for this. I thought it was there because we didn't want to waste the space for annotations if they weren't needed.
Created attachment 40896 [details] Last patch I removed the if (true) to use the option instead. All APT, JDT/UI and JDT/Core tests passed. For JDT/UI, I got some failures, but I got the same failures without the patch. So the patch doesn't seem to be the cause of the failures.
As in RC3 we had 'boolean initializeCompilerAnnotation = true;' I would rather be conservative and not change that. So I would rather prefer the previous patch from comment 15.
I disagree. We don't want users to rely on bindings that should not be there. The option should have been used and it was not the case. Philippe, Tim, any thoughts?
The theory is that annotation bindings are an optional feature since they slow down the compiler. Why did they get added inconditionnally during RC3 ? Was this a mistake or intended ? I would also favour making the code conditional again... but for RC4, I could live with inconditional mode (still fix it for 3.2.1 or 3.3).
Fixed and released in HEAD. I released the version that does the unconditional initialization of the annotation binding to minimise the impact of the change. I opened bug 141030 to track the second issue with the usage of the compiler option. Regression tests added in org.eclipse.jdt.core.tests.dom.ASTConverter15Test.test0218/0219
+1 for 3.2 RC4
Verified with I20060511-2000 for 3.2RC4
