Community
Participate
Working Groups
3.8 M5 // ----------------- public class X { static final Object o; static { goo(); o = new Object(); } static void goo() { if (X.o == null) { System.out.println("X.o is null"); } } public static void main(String[] args) { new X(); } } Both warnings are incorrect and should not be emitted. It looks like we should limit the special treatment accorded to the static final variables to only those that are initialized right along with declaration.
The two warnings referred to being: - Dead code - Null comparison always yields false: The field o cannot be null at this location both in line 9, i.e the line static void goo() { if (X.o == null) { // <===== HERE. System.out.println("X.o is null"); } }
How about this - we treat the constant fields as unknown in only those methods that are called from the initializer, and for all the rest keep the behaviour as it is now? Implementation won't be complicated - we can set a bit in the method's binding as soon as it is encountered in an initializer, and when we start analyzing the method, pass that bit into the flowInfo, which will then take care to treat constants as unknown. The only possible problem may be when a method is not called from the current type's initializer but from its parent type's initializer [1]. In that case, we'd still end up giving the strong warning. However, practically I don't think one would write such a twisted piece of code. Also, if a constant field is initialized to null, then we're already good, aren't we? What's your take on this? [1] public class Wrong extends PWrong { static final Object o; static { o = new Object(); System.out.println("Wrong"); } static void gooChild() { if (Wrong.o == null) { System.out.println("Wrong.o is null"); } } public static void main(String[] args) { new Wrong(); } } class PWrong { static { Wrong.gooChild(); System.out.println("PWrong"); } }
(In reply to comment #2) > How about this - we treat the constant fields as unknown in only those methods > that are called from the initializer, and for all the rest keep the behaviour > as it is now? I agree, that would significantly improve the situation. Interestingly, the example seems to show that Java itself is already broken wrt definite assignment. Look at this slight variation: public class Bug370787 { static final Object o; static { goo(); // legal !?! // System.out.println(d.toString()); illegal, d not yet initialized o = new Object(); } static void goo() { System.out.println(d.toString()); // legal !?! } public static void main(String[] args) { new Bug370787(); } } I had hoped, that Java would guarantee that 'o' is always assigned before read. The example throws NPE on a static final assigned from non-null. How about that? I could see another option: add some doubt on the nullness of all constants while analysing any static method. That, too, should avoid the warning and flagging anything as dead. It is less drastic as we could still give potential warnings in these methods, at the same time it is more drastic, if we do this for all static methods. OTOH, doing it for all static methods would rid us of the worries regarding super initializers etc. (How about boomerangs from our own initializer to a foreign method to our own static method??) I just wished Java would ... nevermind ... :)
(In reply to comment #3) > I could see another option: add some doubt on the nullness of all constants > while analysing any static method. I forgot to add: "for those fields that are not initialized at their declaration".
(In reply to comment #3) > I had hoped, that Java would guarantee that 'o' is always assigned before read. > The example throws NPE on a static final assigned from non-null. How about > that? Yup this is a discovery even I made with this bug :P > I could see another option: add some doubt on the nullness of all constants > while analysing any static method. That, too, should avoid the warning and > flagging anything as dead. Yeah, I wanted to write 'pot. null' instead of unknown infact. So I agree with this. Just changing the nullBit1 to 0 for the whole bit stream should be sufficient to achieve this I guess.
(In reply to comment #2) > How about this - we treat the constant fields as unknown in only those methods > that are called from the initializer, and for all the rest keep the behaviour > as it is now? This is problematic since these methods could call others and they yet more and so on. Also there is nothing that stops a static block of X from calling new X().foo(); Looks like the most rigorous solution is to isolate those variables which are initialized at the time of declaration and also track assignments in static blocks in the order in which they appear until the first call is encountered and freeze the state of affairs there. Since we report a dead code warning thereby encouraging users to delete some code, let us not resort to less rigor even when the test cases appear convoluted in our judgement as it is not worth it.
I believe the behavior shown by the following could be puzzling, but is actually well defined: public class X { static final Object o; static { new X().foo(); o = new Object(); } void foo() { if (o == null) { System.out.println("Dead code got executed!"); } else { System.out.println("Dead code got skipped"); } } public static void main(String[] args) { new X().foo(); } }
(In reply to comment #7) > I believe the behavior shown by the following could be puzzling, > but is actually well defined: Any fix should handle the following case correctly: class X { static final X singletonX = new X(); X() { if (singletonX == null) { System.out.println("singleton null"); } } public static void main(String[] args) { new X(); } } i.e there should not be a dead code warning as there is now.
See another weird issue. We only set the constantFieldsMask when we encounter the field declaration, but the field can actually be initialized textually before its declaration in the typeDeclaration. And if we encounter a message send in between, we'll wrongly reset its info public class Wrong { static final Object o; static final Object o1 = null; static { o3 = null; new Wrong().foo(); o = new Object(); } static final Object o2 = null; static final Object o3; void foo() { o3.toString() // no null info for o3 here } } Argh...this is complicated. :\
(In reply to comment #7) > I believe the behavior shown by the following could be puzzling, > but is actually well defined: Yup, so we need to weaken null info in all methods and not just static ones.
(In reply to comment #8) > (In reply to comment #7) > > I believe the behavior shown by the following could be puzzling, > > but is actually well defined: > > Any fix should handle the following case correctly: Doesn't this mean that whenever a message send/constructor call is encountered during initialization of fields (within/without static initializer), the fields initialized subsequently should have a weaker null info in all methods, irrespective of whether they're declared in initializer or at declaration site? Phew!
I have a patch which handles comment 9 and comment 10. Need to look at comment 8's implication too.
Created attachment 210848 [details] proposed fix v1.0 + regression tests This patch provides a fix for all above issues. Another mask needed to be used, which could record all the constant fields that have been initialized before we see a message send/constructor call. Using this mask, before we start analyzing any methods/constructors, we weaken the null info of all constants that were initialized after a message send/constructor call, EXCEPT those that are already marked as definitely null. Some nuances: 1. A preliminary loop had to be introduced in TypeDeclaration to record all constant fields and fix comment 9. 2. new Object() is excluded from the above treatment because we known it won't call any methods or refer any fields. 3. As said above, fields assigned a definite null value are considered def. null everywhere, irrespective of being assigned before/after a message send/c'tor call. Does any1 want to volunteer to review? :)
Created attachment 210849 [details] proposed fix v1.1 + regression tests This is the correct patch, last one was an older version
I like the strategy, good job. Only the bit operation in weakenNullInfoForConstantFields looks brittle to me: why is it safe to check only two bits and ignore bits 3 & 4? I tried to construct an example that breaks because one of those bits are unexpectedly set. I failed to find such an example, but I'm not sure why. Maybe it's because the markAs... methods already tweak the values for fields? In that case we'll get into trouble once we have to change that strategy. In case you figured that values 1101 - 1111 would not occur for a field initialization, try using a local var in an initializer block and then assign the local to the static final field. This should allow all possible bit combinations to flow into a static final field, no? If unsure, we should always check and update all four bits. Stylistic comments: In all those loops that search for an enclosing InitializationFlowContext: if you pull the action block right after the instanceof it would be even easier to see why the cast is safe. (Null checks can mean so many things ...) Since FieldDeclaration.analyseCode now requires an InitializationFlowContext: how about enforcing this in the signature? Expression (OL | isDefNull) looks funny :)
(In reply to comment #15) > I like the strategy, good job. Thanks for the review, Stephan! :) > Only the bit operation in weakenNullInfoForConstantFields looks brittle to me: > why is it safe to check only two bits and ignore bits 3 & 4? > In case you figured that values 1101 - 1111 would not occur for a field > initialization, try using a local var in an initializer block and then assign > the local to the static final field. This should allow all possible bit > combinations to flow into a static final field, no? Yes I think these are very much possible, and I thought I'll come to this part when i'm done with the overall patch but forgot. Ok, so lets see: 1111 prot. non null -> This can trigger a "redundant null check" warning for a field via FlowInfo#cannotBeNull(..). So this needs to be weakened. 1110 prot. null -> can be treated just like def. null. This combination can trigger an NPE warning via canBeNull(..). There's no reason why a method call should affect this. So we should not weaken this. 1101 pot. n & prot. n -> can lead to a pot. NPE warning through isPotentiallyNull(..) or a NPE warning via canBeNull(..). Again, I don't think we need to weaken this. Do you agree? > In all those loops that search for an enclosing InitializationFlowContext: if > you pull the action block right after the instanceof it would be even easier to > see why the cast is safe. (Null checks can mean so many things ...) I agree. > Since FieldDeclaration.analyseCode now requires an InitializationFlowContext: > how about enforcing this in the signature? Yes that makes sense. > Expression (OL | isDefNull) looks funny :) Yes, definitely does :D
As the implementation from https://bugs.eclipse.org/bugs/show_bug.cgi?id=247564 has been withdrawn the current bug has lost its relevance and can be closed as INVALID.
Verified for 3.8 M6.
