Community
Participate
Working Groups
Build ID: I20080523-0100 An initialized final field cannot be assigned null somewhere, so the null analysis should consider it as NON_NULL. class A { final ArrayList l = new ArrayList(); void dump(A a) { // non-null comparison is always true if (a.l != null) { for (Iterator it = a.l.iterator(); it.hasNext(); ) System.out.println("hey " + it.next()); } } }
Why isn't it a good feature ?
No one says this feature would not be desirable, and I apologize if I gave that impression. If you look at the NullReferenceTest class, you will see that this was considered at the beginning of the implementation, but other features got a higher priority and fields were dropped from the scope of our first implementation. As always, we only implement part of the desirable features, hopefully focusing on the most desirable ones. Furthermore, we currently have no concrete plans to push the envelope of null-analysis features in JDT, hence I downgraded the priority to P5, as I did for other null-related bugs. We may or may not open a 'null-analysis effort' at some future point, but the decision is not with me. If an effort is launched, the null-analysis related bugs will be considered and their relative priorities will be reviewed.
Thanks a lot for the explanation. I'll wait for the 'null-analysis effort' :)
You're welcome.
This was considered in bug 247564 but dropped as of bug 247564 comment 16. Ayush, do we want to a) close this as WONTFIX, or b) extend bug 247564 to final fields with an initializer? Also relevant: bug 331649.
(In reply to comment #5) > Ayush, do we want to > a) close this as WONTFIX, or > b) extend bug 247564 to final fields with an initializer? Lets keep this open for now. Once the issues in bug 247564 are tackled, we can look into this.
Ayush & Stephan, Where has the resolution of bug 247564 left us with vis-à-vis the current one ?
(In reply to comment #7) > Ayush & Stephan, > > Where has the resolution of bug 247564 left us with vis-à-vis > the current one ? This was not considered because of bug 247564 comment 16, as pointed out above. We can perhaps make a special case for final fields initialized as they are declared and treat them like static finals thereafter. Will consider for M6. Stephan, any thoughts? This shouldn't be difficult if we add this field's position also to the mask that we create for constant fields.
This may be ok for unqualified access and 'this' reference, but we should not support final fields accessed from an object. Here's one example to show why public class X { final Object o2 = null; public void someMethod(){} public void foo(X x) { if (x.o2 == null) { } x.o2.toString(); if (this.o2 == null) { } this.o2.toString(); } } Here x.o2.toString() will mark o2 as cannot be null further in the flow. This will pollute the null status of this.o2 as well, since the flowInfo knows only a single o2.
Created attachment 209904 [details] proposed fix v1.0 + regression tests This fix piggybacks on the treatment of static final fields. We record final fields initialized at the time of declaration in the constant fields mask. And then during marking of the null status, we make sure we handle both initialized finals and static finals at par. The only other challenge was to make sure access of final field using 'this' reference also works, and works only for a final field that was initialized when declared. For this, we check if the field was set in the constantFieldsMask earlier, and then return its binding.
(In reply to comment #10) > Created attachment 209904 [details] > proposed fix v1.0 + regression tests Let us defer this until we have had some more discussions.
(In reply to comment #11) > (In reply to comment #10) > > Created attachment 209904 [details] > > proposed fix v1.0 + regression tests > > Let us defer this until we have had some more discussions. Now that we have moved out some follow up items on null field analysis to future release, we can go ahead with a solution that fits well in the current scheme of things. Stephan, can you please review this for inclusion in M6. TIA.
(In reply to comment #12) > (In reply to comment #11) > > (In reply to comment #10) > > > Created attachment 209904 [details] > > > proposed fix v1.0 + regression tests > > > > Let us defer this until we have had some more discussions. > > Now that we have moved out some follow up items on null field > analysis to future release, we can go ahead with a solution > that fits well in the current scheme of things. > > Stephan, can you please review this for inclusion in M6. TIA. See https://bugs.eclipse.org/bugs/show_bug.cgi?id=370787 for a problem with static final fields. I didn't check whether the current patch would have the equivalent problem for instance final fields. If it does, we should hold on it till we have a good story on bug 370787
(In reply to comment #13) > (In reply to comment #12) > > Stephan, can you please review this for inclusion in M6. TIA. > > See https://bugs.eclipse.org/bugs/show_bug.cgi?id=370787 for a > problem with static final fields. I didn't check whether the > current patch would have the equivalent problem for instance > final fields. If it does, we should hold on it till we have > a good story on bug 370787 Ayush, have you checked whether this bug must be updated after bug 370787, or is the last patch still valid under those considerations?
*** Bug 388184 has been marked as a duplicate of this bug. ***
*** Bug 405036 has been marked as a duplicate of this bug. ***
The current analysis fails pretty bad, example: import javax.annotation.Nullable; class BadNull { private final @Nullable String x = "k"; public String theBad1() { return x == null ? "" : x.toString(); } public String theBad2() { if (x == null) { return ""; } else { return x.toString(); } } public String theUgly() { if (x != null) { return x.toString(); } return ""; } } (With Eclipse configured to use the javax annotations). Will report potential null access in all three method above. This happens even when the field is final and initialized to a non-null value.
Another analysis failure: @ParametersAreNonnullByDefault ... public void x(Long l) {} public long z() { return 0L; } public void y() { x(z()); } Null type safety: The expression of type long needs unchecked conversion to conform to '@Nonnull Long' which is bad, as autoboxing will never produce a null Long object. Likely the problem is that autoboxing is desugared to an API call which is not annotated as @Nonnull.
(In reply to Osvaldo Pinali Doederlein from comment #18) > Another analysis failure: > > @ParametersAreNonnullByDefault > ... > public void x(Long l) {} > public long z() { return 0L; } > public void y() { x(z()); } > > Null type safety: The expression of type long needs unchecked conversion to > conform to '@Nonnull Long' > > which is bad, as autoboxing will never produce a null Long object. Likely > the problem is that autoboxing is desugared to an API call which is not > annotated as @Nonnull. This looks like a duplicate of bug 407414, I'll make a note in that bug.
I guess this bug deserves a fresh assessment: (A) All final fields with an initializer can be analysed and if found to be non-null we can store this information for further analysis. Potential risks: dependencies between different fields/initializers should already be handled correctly by prohibition of forward references in initializers, but if such dependency is hidden inside (getter) methods, analysis results may be incorrect: public class T { final Object f1 = getF2(); final Object f2 = getF1(); @NonNull Object getF1() { return this.f1; } @NonNull Object getF2() { return this.f2; } public static void main(String[] args) { System.out.println(new T().getF1().toString()); } } This would be accepted by the compiler but throw NPE. This is the same risk as assuming that each access to a final field finds a validly initialized value, which Java cannot fully guarantee. Hints at this danger could be detected by checking whether any field initializers leak "this" (as a receiver or parameter in any method call). (B) Final instance fields initialized in constructors are basically equivalent to (A) given that one is translated into the other anyway. (C) Final static fields without initializer add the option that initialization is spread over multiple static initializer blocks, but I don't see a semantic difference between multiple initializers and one big merged initializer. It seems in bug 370787 we concluded that static final fields not initialized in place are specifically dangerous. From today's perspective I'd say this particular danger - arises in *any* situation where the required initialization phase calls methods - threatens not only null analysis but also the assumed guarantees of final. In one regard the mentioned leaking is more difficult to detect for static fields: *Any* method call could transitively lead to reading the still uninitialized static field, since no 'this' reference needs to be passed. Several approaches exist in research for making object initialization safe. If I understand those approaches correctly, their common key point is that any method transitively reachable from any initialization code has to obey specific constraints, viz. don't assume the object to be fully initialized. I'm mentioning this, because this would put the blame on those methods like getF1() above, *not* on the field. With these considerations, I suggest we: - apply the same treatment to all fields in (A) to (C) above - use the nullness information from field initialization if null annotations are enabled, and henceforth treat fields with a non-null initialization as @NonNull. Looking beyond, we could in some future add a warning regarding the mentioned leaks. Method calls inside instance initialization code which leak "this" are well detectable. This warning can only give an over estimation of potentially dangerous situations. If that warning is seen frequently, using one or a few more annotations for specifying object initialization will be recommended. A similar warning for static initializers would be overkill (unless we had a "@Pure" annotation...), i.e., reporting more false positives than relevant warnings. I'm tentatively marking for 4.4 M3. I think fixing this bug would make the analysis more consistent. Let me know if anyone has concerns with my proposal.
I have more urgent tasks on my plate right till the end of this year. Hopefully I'll find some time early next year.
Re-reading this bug for the umptieth time, I seem to see a roadblock: without bytecode analysis, we'll never get the same results for a class read from binary as when the source is parsed. This would imply that incremental builds get worse results than full builds, which is unacceptable. Actually, that's one of the reasons why we started work on null annotations in the first place: to make null assumptions explicit, without the need to look inside. Users wanting the effect requested here are encouraged to mark those fields as @NonNull.
Verfied (that null analysis does not address this unless @NonNull specified as per comment 22) for Eclipse 4.4M5 Version: Luna Build id: I20140120-2000
*** Bug 428228 has been marked as a duplicate of this bug. ***
Not sure if this is related to this bug or not: object.getClass() never returns null. Is it possible to add a list of "known safe methods" to the null analysis?
(In reply to Aaron Digulla from comment #25) > Not sure if this is related to this bug or not: object.getClass() never > returns null. > > Is it possible to add a list of "known safe methods" to the null analysis? This is more related to bug https://bugs.eclipse.org/bugs/show_bug.cgi?id=331651
(In reply to Stephan Herrmann from comment #22) > Re-reading this bug for the umptieth time, I seem to see a roadblock: > without bytecode analysis, we'll never get the same results for a class read > from binary as when the source is parsed. This would imply that incremental > builds get worse results than full builds, which is unacceptable. > > Actually, that's one of the reasons why we started work on null annotations > in the first place: to make null assumptions explicit, without the need to > look inside. > > Users wanting the effect requested here are encouraged to mark those fields > as @NonNull. What about adding the @NonNull annotations that are infered for final fields into the generated bytecode ?
(In reply to Ludovic Pollet from comment #27) > What about adding the @NonNull annotations that are infered for final fields > into the generated bytecode ? That breaks the contract that our compiler generates exactly those bytecodes as are specified by the specification. We had this discussion regarding NonNullByDefault which initially I materialized into the bytecode, but this was voted down by the project leads. One argument was that when consuming an existing class file it should not matter, whether it was created by our compiler or by javac.
*** Bug 509039 has been marked as a duplicate of this bug. ***
