Bugzilla – Full Text Bug Listing
| Summary: | [1.8][null] null annotations completely broken for classes that are resolved in lambda | ||
|---|---|---|---|
| Product: | [Eclipse Project] JDT | Reporter: | Till Brychcy <register.eclipse> |
| Component: | Core | Assignee: | Till Brychcy <register.eclipse> |
| Status: | VERIFIED FIXED | QA Contact: | Stephan Herrmann <stephan.herrmann> |
| Severity: | normal | ||
| Priority: | P3 | CC: | dusisarath, manoj.palat, stephan.herrmann, tsichevski |
| Version: | 4.6 | ||
| Target Milestone: | 4.6 M6 | ||
| Hardware: | PC | ||
| OS: | Mac OS X | ||
| See Also: |
https://git.eclipse.org/r/63904 https://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=5959bbf385fc1f797d392576c0a27049452fc9d1 |
||
| Whiteboard: | |||
New Gerrit change created: https://git.eclipse.org/r/63904 (In reply to Till Brychcy from comment #0) > ANALYSIS: > In LambdaExpression, isAnnotationBasedNullAnalysisEnabled is temporarily set > to false. If during that time URBs are resolved, > the null bits are not correctly set when > UnresolvedReferenceBinding.swapUnresolved calls > TypeBinding.setTypeAnnotations. > > Because the annotated types are cached, this may lead to all kinds of > problems in the null analysis (outside the lambda expression) > > In the given example, a NonNull annotation is added to "@Nullable Registry" > in FieldBinding.fillInDefaultNonNullness because of the incorrect tag bits > when compiling ZClassWithBug.java. > > PATCH (via gerrit): > It is unclear, why "isAnnotationBasedNullAnalysisEnabled" is set to false. > There is no comment and the tests (testBug429430*) that came with the commit > that added this code don’t seem to need it. (I ran them as variation with > Null Type Annotations on). Maybe it was some leftover from debugging or > maybe some later changes have made this unnecessary? The Patch simply > removes that code. Very good catch, thanks!! As you can see from the comments in bug 429430, this code area has a colorful history, the core problem being: lambda shape analysis needs a lot of compiler work to be performed ahead of time, like flow analysis of a lambda before resolve has completed. I can well imagine that Srikanth carefully turned off everything that doesn't look pertinent to lambda shape analysis. For the AST part this is made to work by working from throw-away copies of the lambda AST, not accounting for any side effects caused along the way. Setting target to M6 simply because of other reviews in the pipe line - will pull into M5 if time permits. Result of software archaeology via several indirections: the test in question has been discussed in Bug 422515 comment 4 f. (The check was originally implemented in isCompatibleWith(), later copied to getResolvedCopyForInferenceTargeting(), renamed later to resolveExpressionExpecting() from there extracted into cachedResolvedCopy()). While implementing a "trial evaluation", Srikanth apparently reduced this analysis to the minimum to figure out just value compatible vs. void compatible lambdas. In the light of side effects performed (or not performed) during this analysis, such optimization is bogus. Gerrit change https://git.eclipse.org/r/63904 was merged to [master]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=5959bbf385fc1f797d392576c0a27049452fc9d1 (In reply to Eclipse Genie from comment #4) > Gerrit change https://git.eclipse.org/r/63904 was merged to [master]. > Commit: > http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=5959bbf385fc1f797d392576c0a27049452fc9d1 > Rebased and released for 4.6 M6 Thanks, Till! . Verified for 4.6M6 using Version: Neon (4.6) Build id: I20160314-200 *** Bug 471048 has been marked as a duplicate of this bug. *** |
DESCRIPTION: The following problem is very dependent of the compilation order. I noted it when the reconciler ran while editing code. Luckily I was able to narrow it down to a reproducible test case. project 1 - - - package test2; import java.rmi.registry.Registry; import org.eclipse.jdt.annotation.Nullable; public class ClassWithRegistry { @Nullable public Registry registry; } project 2 - - - package test1; import test2.ClassWithRegistry; // must be compiled before ZClassWithBug (but in the same project) public class ClassWithLambda { interface Lambda { void f(); } public static void invoke(Lambda lambda) { lambda.f(); } public void f() { new ClassWithRegistry(); // must be accessed as class file invoke(() -> java.rmi.registry.Registry.class.hashCode()); } } package test1; import java.rmi.registry.Registry; import org.eclipse.jdt.annotation.NonNullByDefault; import org.eclipse.jdt.annotation.Nullable; @NonNullByDefault public abstract class ZClassWithBug { @Nullable public Registry rmiregistry; } 1. ERROR in test1\ZClassWithBug.java (at line 12) public Registry rmiregistry; ^^^^^^^^^^^\n The @NonNull field rmiregistry may not have been initialized ANALYSIS: In LambdaExpression, isAnnotationBasedNullAnalysisEnabled is temporarily set to false. If during that time URBs are resolved, the null bits are not correctly set when UnresolvedReferenceBinding.swapUnresolved calls TypeBinding.setTypeAnnotations. Because the annotated types are cached, this may lead to all kinds of problems in the null analysis (outside the lambda expression) In the given example, a NonNull annotation is added to "@Nullable Registry" in FieldBinding.fillInDefaultNonNullness because of the incorrect tag bits when compiling ZClassWithBug.java. PATCH (via gerrit): It is unclear, why "isAnnotationBasedNullAnalysisEnabled" is set to false. There is no comment and the tests (testBug429430*) that came with the commit that added this code don’t seem to need it. (I ran them as variation with Null Type Annotations on). Maybe it was some leftover from debugging or maybe some later changes have made this unnecessary? The Patch simply removes that code.