Community
Participate
Working Groups
The attached snippet fails with a NPE when it tries to access the annotations of a super class. java.lang.NullPointerException at org.eclipse.jdt.internal.compiler.lookup.SourceTypeBinding.getAnnotationTagBits(SourceTypeBinding.java:677) at org.eclipse.jdt.internal.compiler.lookup.SourceTypeBinding.retrieveAnnotationHolder(SourceTypeBinding.java:1417) at org.eclipse.jdt.internal.compiler.lookup.ReferenceBinding.retrieveAnnotations(ReferenceBinding.java:1082) at org.eclipse.jdt.internal.compiler.lookup.ReferenceBinding.getAnnotations(ReferenceBinding.java:595) at org.eclipse.jdt.core.dom.TypeBinding.getAnnotations(TypeBinding.java:92) at org.eclipse.jdt.junit.tests.JUnit4TestFinderTest.testNPE(JUnit4TestFinderTest.java:323) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) ...
Created attachment 49480 [details] test case
The annotations from the superclass are not resolved before the scopes are nullified. The call getAnnotations() is trying to resolve them, but there is no more scopes to do it. The call createBindings(...) doesn't resolve the superclass of the given bindings. createAST(...) on the compilation unit that contaisn Test3 correctly initializes the annotations of the superclass and I could print: p.Test3 has 0 annotation(s). p.Test2 has 2 annotation(s). java.lang.Object has 0 annotation(s). So the problem comes from the createBindings(...) call.
Created attachment 49503 [details] New test case This test case is using one call to createBindings(...) and one call to createAST(...).
Created attachment 49504 [details] simplified test case Simplified: no JUnit container references needed and compliance correctly set to 1.5
Is it possible up the priority of this bug? The workaround only works for types in compilation units, but not in a classfile with no source.
This is a direct consequence of fix for bug 114935. If commenting out line 677 to 686 in org.eclipse.jdt.core.dom.CompilationUnitResolver, the test case doesn't fail anymore. In this case the superclass source unit is requested in order to connect the superclass, but since it is not part of the requestedSource, the superclass source unit added in the unitToProcess list is not resolved and therefore it is not possible to get the annotation bindings once the units have been cleaned up. We should investigate two fixes: 1) prevent the call from failing if annotations are not resolved => add a null check 2) resolve units required to connect the superclass. I added regression tests in org.eclipse.jdt.core.tests.dom.ASTConverter15Test#test0230/0231. 0230 is disabled since it fails.
When we use createAST(...), the scopes of the requested types are not cleaned up and then the call to get the annotations work fine. When we use createBindings(...), all scopes are cleaned up and therefore it is not possible to do any further resolution. This has been done to fix bug 114935. Jérôme, since you fixed bug 114935, you might have an idea how to fix this one as well. We might want to be able to resolve bindings inside the hierarchy. Otherwise we end up with incomplete bindings.
It looks like we want to force the resolution of annotations of types and methods that are in the queue before the cleanup. That sounds like a lot of work. Martin, since you didn't request the resolution of the super type, would returning an empty list in this case be good enough ?
In our use case we indeed need the super types and all their member's annotations. But even if we didn't, it would seem like a bug to get incomplete bindings. It shouldn't make a difference how a binding was requested (createAST or createBinding), it should always look the same.
(In reply to comment #8) > It looks like we want to force the resolution of annotations of types and > methods that are in the queue before the cleanup. We don't need to resolve if not requested. > That sounds like a lot of work. Martin, since you didn't request the resolution > of the super type, would returning an empty list in this case be good enough ? I don't think an empty list would help. We need a consistent story between createBindings(...) and createAST(...). createAST(...) doesn't clean up units in the queue. We could do the same thing for createBindings(...). However this breaks the regression test added for bug 114935. So we might want to revisit it. I don't see why in org.eclipse.jdt.core.tests.dom.BatchASTCreationTests#test070, Y should not be resolved. If the user explicitly requests bindings for Y, I expect them to be available. If not, this would need to be described in the createBindings(...) API as well as createAST(...) that is doing too much work right now. Jérôme, any comment?
(In reply to comment #10) > (In reply to comment #8) > > That sounds like a lot of work. Martin, since you didn't request the resolution > > of the super type, would returning an empty list in this case be good enough ? > I don't think an empty list would help. We already return an empty list for fields and members if their declaring type has not been resolved. > We need a consistent story between createBindings(...) and createAST(...). > createAST(...) doesn't clean up units in the queue. We could do the same thing > for createBindings(...). If we eagerly created all bindings, this would be a performance regression (see bug bug 114935). > However this breaks the regression test added for bug 114935. So we might want > to revisit it. This test ensures that we don't have a performance regression. > I don't see why in > org.eclipse.jdt.core.tests.dom.BatchASTCreationTests#test070, Y should not be > resolved. If the user explicitly requests bindings for Y, I expect them to be > available. If not, this would need to be described in the createBindings(...) > API as well as createAST(...) that is doing too much work right now. Right. In the case of BatchASTCreationTests#test070, the user doesn't ask Y to be resolved.
We definitely need consistency between the two APIs. So if we continue to clean up the queued units as we do in createBindings(...) we should also clean up queued units in createAST(...). Martin, any comment? Would this break existing code on your side?
We expect all bindings to be complete. Changing this will definitly break us. Or we have to introduce new flags on createBinding to give you hints how much should be resolved. It's not obvious to me how bug 114935 and ASTParser.createBindings are related. For the ASTRequestor it makes perfectly sense to stop reporting ASTs anymore when all user requested ASTs and bindings have been processed, but ASTParser.createBindings is a simple function, no ASTRequestor involved.
(In reply to comment #13) > We expect all bindings to be complete. Changing this will definitly break us. As said before, you are already broken for fields and methods. > Or we have to introduce new flags on createBinding to give you hints how much > should be resolved. This problem should be resolved when we reify the AST queue. In this case, the lookup environment will be kept in memory and things will be lazily resolved. > It's not obvious to me how bug 114935 and ASTParser.createBindings are related. > For the ASTRequestor it makes perfectly sense to stop reporting ASTs anymore > when all user requested ASTs and bindings have been processed, but > ASTParser.createBindings is a simple function, no ASTRequestor involved. The problem in bug 114935 was not to stop reporting ASTs, but to stop resolving ASTs that were not requested (same as stop resolving binding keys that were not requested).
Can you elaborate which fields and methods we're not getting? We need a clear understanding of this, so we know why refactorings or other stuff fails.
(In reply to comment #15) > Can you elaborate which fields and methods we're not getting? We need a clear > understanding of this, so we know why refactorings or other stuff fails. Let's say you have class A with field 'foo' and class B that extends A. If you call createBindings on B only, then you navigate to A binding (using getSuperClass() on B binding), then ask A binding for its fields using getDeclaredFields(), this will return an empty list since A was not resolved.
I verified that in the AST view and you're right. But I'm afraid this is a bug, we're really expecting bindings to be complete and to look the same, regardless if achieved from createAST or createBinding. Performance is of course an issue, but correctness is more important. If thing are left away, it needs to be decided/controled by the user of the API. What do you mean by 'not requested'. Assuming I want to know all fields from a type's superclasses, do you think I should pass in the fields as IFields to createBinding? How would we get all these IFields? Building a supertype hierarchy? That really wouldn't make sense.
I think the performance improvement of bug 114935 should be made explicit by allowing the API client to tell whether bindings should be completely resolved after the last accept*(..) callback. This could either be an additional parameter for ASTParser#createASTs(..), or another callback method in ASTRequestor, e.g. public boolean stopResolvingBindings() { return false; } ASTParser#createBindings(..) has to return complete bindings.
(In reply to comment #17) > What do you mean by 'not requested'. I meant that it is not part of the set of ICompilationUnits and IClassFiles that are the parents of the IJavaElements passed to createBindings(...). > Assuming I want to know all fields from a > type's superclasses, do you think I should pass in the fields as IFields to > createBinding? You would need to only pass in the type's superclasses. > How would we get all these IFields? Building a supertype > hierarchy? That really wouldn't make sense. Agreed that would be very inefficient.
(In reply to comment #18) > I think the performance improvement of bug 114935 should be made explicit by > allowing the API client to tell whether bindings should be completely resolved > after the last accept*(..) callback. This could either be an additional > parameter for ASTParser#createASTs(..), or another callback method in > ASTRequestor, e.g. > public boolean stopResolvingBindings() { return false; } > > ASTParser#createBindings(..) has to return complete bindings. > This proposal makes sense to me. So we have 2 issues here: - fixing the NPE (this bug) - adding a new API to control whether we should stop resolving after the last accept(...) has been done (please open a separate bug for that)
Ok, in this case we might need two new APIs depending how we do it. One for createBindings(...) and one for createAST(...). Do we agree that right now createAST(...) is doing too much work? It should also clean up all units in the queue. In order to get all the bindings complete, the new API should be used. This might break existing users.
(In reply to comment #21) > Ok, in this case we might need two new APIs depending how we do it. One for > createBindings(...) and one for createAST(...). I think you meant one for createASTs(...) and one for createAST(). Markus said that createBindings(...) should always return complete bindings, so we should fix that when we get a separate bug for it. > Do we agree that right now createAST(...) is doing too much work? It should > also clean up all units in the queue. In order to get all the bindings > complete, the new API should be used. This might break existing users. I agree that it is doing too much. However nobody complained about it yet. So when we get a complaint that createAST(...) is too slow, then we can add a new API that would cleanup the remaining CUs in the queue. Thus we would not break existing clients. So I would not change anything for createAST(...) for now.
createAST() is doing the right thing, createBindings() should do the same (complete bindings). I agree with Jerome, we don;t need to add API at this time. The new API would be for clients of 'createASTs' (like in bug 114935) so they can tell the queue that after the last callback to their ASTRequestor they are done and won't access bindings outside the queue (or is this already part of the contract of the ASTRequestor?)
(In reply to comment #23) > createAST() is doing the right thing, createBindings() should do the same > (complete bindings). I agree with Jerome, we don;t need to add API at this > time. createAST() doesn't resolve the bindings before they are requested. It simply keeps all the scopes around for the units in the queue. This allows lazy resolution to work. So in order to fix this, we would simply remove the cleaning of the units in the queue for createBindings() and we should get a consistent behavior. But this should be in a different bug report. This one is about the NPE. I open bug 164660 for the new API creation. Feel free to add your proposal there.
*** Bug 151364 has been marked as a duplicate of this bug. ***
NPE is fixed in HEAD. Released for 3.3M4. Please use bug 164660 in order to fix the problem with incomplete bindings (annotations are not resolved for the super class). Regression tests are now reenabled.
Verified for 3.3 M4 using build I20061212-0010.
