Bugzilla – Full Text Bug Listing
| Summary: | [null] NPE in TypeVariableBinding.evaluateNullAnnotations | ||||||
|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] JDT | Reporter: | Chris Hubick <chris> | ||||
| Component: | Core | Assignee: | Till Brychcy <register.eclipse> | ||||
| Status: | VERIFIED FIXED | QA Contact: | |||||
| Severity: | normal | ||||||
| Priority: | P3 | CC: | jarthana, register.eclipse, stephan.herrmann | ||||
| Version: | 4.6 | Flags: | stephan.herrmann:
review+
|
||||
| Target Milestone: | 4.6.1 | ||||||
| Hardware: | PC | ||||||
| OS: | Linux | ||||||
| See Also: |
https://git.eclipse.org/r/75350 https://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=e8351b891768459b45fcffb1834de23e13352047 https://git.eclipse.org/r/78665 https://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=b789afe530d50c1b1770698f09bbacfff70907bb |
||||||
| Whiteboard: | |||||||
| Bug Depends on: | 499393 | ||||||
| Bug Blocks: | |||||||
| Attachments: |
|
||||||
|
Description
Chris Hubick
(In reply to Chris Hubick from comment #0) > What steps will reproduce the problem? > 1. Press F3 on something to navigate to it's declaration. I do that all the time - without NPE :) Can you narrow it down to a small example? > eclipse.buildId=4.6.0.I20160525-2000 = 4.6RC3 > Caused by: java.lang.NullPointerException > at org.eclipse.jdt.internal.compiler.lookup.TypeVariableBinding.evaluateNullAnnotations(TypeVariableBinding.java:932) We seem to have a CaptureBinding with null superInterfaces. First guesses, why null: + some error during resolution? Chris, do you have compile errors? + some unexpected processing order? > at org.eclipse.jdt.internal.compiler.lookup.CaptureBinding.initializeBounds(CaptureBinding.java:252) This call to evaluateNullAnnotations() has been introduced via Bug 485058. (In reply to Stephan Herrmann from comment #1) > > 1. Press F3 on something to navigate to it's declaration. > > I do that all the time - without NPE :) I know, right? > Can you narrow it down to a small example? Not really, sorry... it's one of those weird things that just started happening, and I can't tell why. I initially didn't even have null annotations enabled globally, and it was doing it, so I enabled them globally... though, thanks to Maven causing each project to use it's own settings, I'm not positively sure if some projects didn't have them enabled previously. > We seem to have a CaptureBinding with null superInterfaces. > First guesses, why null: > + some error during resolution? Chris, do you have compile errors? > + some unexpected processing order? I've just finished cleaning up all compilation errors and even all the null analysis warnings, closed all other projects, and restarted Eclipse, and it's *still* doing it. Sorry, I can't really be of more help :-( >org.eclipse.jdt.internal.compiler.lookup.TypeVariableBinding.evaluateNullAnnotations(TypeVariableBinding.java:932)
at > >org.eclipse.jdt.internal.compiler.lookup.CaptureBinding.initializeBounds(CaptureBinding.java:252)
In initializeBounds, setSuperInterfaces is invoked in all code paths before the invocation of evaluateNullAnnotations. This means, that the arg of setSuperInterfaces must have been a null in the case of the bug.
A test case would be really useful. Maybe you can extract an example by a closing projects and removing code till the problem goes away. While the bug happens happens some types are being connected with super interfaces and there are type parameters involved. There must be a type variable that has a bound that contains a wildcard, e.g. interface X<T extends Y<?>>, but probably with some kind of deeper nesting or mutual recursion or nested classes. Created attachment 262325 [details]
Maven Project ZIP
Multi-Module Maven project in which I'm experiencing this bug.
Highlight "empty()" in ca.athabascau.regdata.model.student.person.StudentFilter.getStudentIDFilter() and press F3.
I'd prefer this attachment were deleted once reproduced, thanks.
Thanks, I can reproduce. As expected, type bounds like <O extends BannerPerson<K,O,?,?,?>> are involved. I'll distill a test case. Unfortunately, I don't have the necessary rights to delete the attachment. @Stephan, can you do that? Reduced test case (with a seperate .java for each interface):
public interface AURegObject {
}
--
public interface AURegKey<O extends AURegObject> {
}
--
public interface Person<O extends Person<O>> extends AURegObject, PersonKey<O> {
}
--
public interface PersonKey<O extends Person<?>> extends AURegKey<O> {
}
To reproduce, open Person.java, highlight PersonKey and press F3
I also noted the following, probably related problem:
public interface AURegType<O extends AURegObject<O>> {
}
--
public interface AURegBase<O extends AURegObject<O>> extends AURegType<O> {
}
--
public interface AURegKey<O extends AURegObject<O>> extends AURegBase<O> {
}
--
public interface AURegObject<O extends AURegObject<O>>
extends AURegBase<O>, AURegKey<O> {
}
when AugRegKey.java is open, an error "The hierarchy for type AugRegKey is inconsistent" as red underline at "AURegKey" (nothing is shown in the problems view)
The problem from comment 8 already exists in 4.4.2, 4.5.0 and 4.5.2 Analysis for the example in comment 7: While doing Connect.connectTypeHierarchy() for Person, Engine.accept(…) calls LookupEnvironment.completeTypeBindings(CompilationUnitDeclaration, boolean) which invokes CompilationUnitScope.checkParameterizedTypes(). During that, a type variable is checked against the bound Person<?> and for this the capture-operation is invoked from ReferenceBinding.isCompatibleWith0(…), but the super interfaces of type variable of Person are still null, which leads to the NPE. This does not happen during normal compilation which goes via LookupEnvironment.completeTypeBindings(), which first does connectTypeHierarchy for all involved compilation units and then invokes checkParameterizedTypes for all of them, so the super interfaces are already set in this case. Therefore I think that adding a null check in TypeVariableBinding.evaluateNullAnnotations will be enough as fix. Also, I think that the invocation of that method should only be done if annotation based null analysis is enabled. Much Thanks for looking into all this! :-) (In reply to Till Brychcy from comment #10) > Therefore I think that adding a null check in > TypeVariableBinding.evaluateNullAnnotations will be enough as fix. Is this saying: "we can skip null annotations because we're not within regular compilation"? What invocations of the compiler are considered relevant? - batch - *Builder invocations - CompilationUnitProblemFinder (part of "reconcile") > Also, I > think that the invocation of that method should only be done if annotation > based null analysis is enabled. That's always a good idea :) (and already done on the other path into evaluateNullAnnotations()) (In reply to Stephan Herrmann from comment #12) > (In reply to Till Brychcy from comment #10) > > Therefore I think that adding a null check in > > TypeVariableBinding.evaluateNullAnnotations will be enough as fix. > > Is this saying: "we can skip null annotations because we're not within > regular compilation"? Basically, yes, at least in this kind of situation. > What invocations of the compiler are considered relevant? > - batch > - *Builder invocations > - CompilationUnitProblemFinder (part of "reconcile") Yes. Or, coming from the other side: the call hierarchy of CompilationUnitScope.checkParameterizedTypes() shows that the invocations that don't go via LookupEnvironment.completeTypeBindings() but via LookupEnvironment.completeTypeBindings(CompilationUnitDeclaration, boolean) or LookupEnvironment.completeTypeBindings(CompilationUnitDeclaration[], boolean[], int) are all related to "non-compilation" topics as completion, selection, hierarchy and search. I wonder if whole invocation of CompilationUnitScope.checkParameterizedTypes is needed for any of these, or if it is just in there for symmetry reasons. LookupEnvironment.completeTypeBindings(CompilationUnitDeclaration), which *is* used by Compiler classes, does not invoke CompilationUnitScope.checkParameterizedTypes() > > > > Also, I > > think that the invocation of that method should only be done if annotation > > based null analysis is enabled. > > That's always a good idea :) > (and already done on the other path into evaluateNullAnnotations()) I've created bug 495953 for the problem in comment 8. Changing the compilation order by renaming AURegKey to AU1RegKey makes the problem appear even in the problems view. New Gerrit change created: https://git.eclipse.org/r/75350 Gerrit change https://git.eclipse.org/r/75350 was merged to [master]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=e8351b891768459b45fcffb1834de23e13352047 (In reply to Eclipse Genie from comment #16) > Gerrit change https://git.eclipse.org/r/75350 was merged to [master]. > Commit: > http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/ > ?id=e8351b891768459b45fcffb1834de23e13352047 Released for 4.7M1 @Jay, this simple fix of a NPE might be good for 4.6.1 Verified for 4.7 M1 with build I20160801-2000 Reopening the bug for 4.6.1. Stephan, the review flag is yet to be set. Do you want to set it already or use it for 4.6.1? (In reply to Jay Arthanareeswaran from comment #19) > Verified for 4.7 M1 with build I20160801-2000 > > Reopening the bug for 4.6.1. > > Stephan, the review flag is yet to be set. Do you want to set it already or > use it for 4.6.1? I think we're good for 4.7 M1, will do the review for 4.6.1. New Gerrit change created: https://git.eclipse.org/r/78665 New Gerrit change created: https://git.eclipse.org/r/78665 A few observations during review (and while waiting for repo.eclipse.org to come back online):
Indeed all this happens in the gray zone of compiler invocations for services like completion, selection, type hierarchy, search. Indeed we may not be totally consistent here as to how much error detection we actually need. Any errors found will probably be ignored by the callers anyway.
In particular we may miss a null annotation on a super type and hence raise a false error from bound checking. But I don't see how this could harm a caller like code select.
For 4.6.1 the additional null check is definitely the right measure.
For future we could think of cutting down some of this unneeded processing, with double potential benefit:
- better performance
- avoidance of null checks for which in normal operation ("real compilation") we do not have any meaningful "else" implementation. For normal compiler development work I consider such checks as undesired clutter.
This needs to be balanced against the risk of having half-processed sources, which would raise errors at bogus locations. E.g.: when avoiding null analysis for secondary types, those types will not have any nullTagBits, and when performing null analysis of the focus type with secondary types in some signatures we will see bogus errors.
For another example see where NullAnnotationMatching checks for TagBits.PassedBoundCheck which will never be set if we skip checkParameterizedTypeBounds().
The change looks good to me. In patch set #2 I made the necessary version update to 3.12.1 (this being the first real commit for 4.6.1). Gerrit change https://git.eclipse.org/r/78665 was merged to [R4_6_maintenance]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=b789afe530d50c1b1770698f09bbacfff70907bb (In reply to Eclipse Genie from comment #25) > Gerrit change https://git.eclipse.org/r/78665 was merged to > [R4_6_maintenance]. > Commit: > http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=b789afe530d50c1b1770698f09bbacfff70907bb > Released for 4.6.1 Thanks, Till. Verified for 4.6.1 with build M20160810-1300. |