Community
Participate
Working Groups
The null check on LINE 5 should be warned as always false. class Bug { { int[]array={4}; for(int x:array) if(null==array); // LINE 5 } }
Can reproduce in I20090929-0800.
Ayush, Please investigate -- Thanks.
Created attachment 150909 [details] test highlighting all problems My findings from the analysis: The problem as reported by Brian illustrates the most trivial ramification of this bug - not reporting redundant null check for the collection variable in a loop. There are a couple of other side effects as well: 1) The warning is not just missed in a foreach loop, but in every other loop as well, including for, while, and do. 2) The warning is correctly reported if the code in the given description is changed to class Bug { { int[]array={4}; for(int x:array) if(null==array) return; // LINE 5 } } 3) The problem is not just in case of collection variable, but also in case of other local variables that have been declared outside the loop. -> The root cause of the warning is the fact that we dont carry the nullness info from outside the loop while doing the static analysis of the loop's action statements. Line 95 in ForEachStatement#analyseCode() is the cause of this: UnconditionalFlowInfo actionInfo = condInfo.nullInfoLessUnconditionalCopy(); Due to this we dont know if a local variable being compared to null is definitely null or not inside the loop, unless its actually declared in the loop. Then the intuitive fix is to change the above line to UnconditionalFlowInfo actionInfo = condInfo.unconditionalCopy(); But if we do this, then the following case will fail: class Bug { public void doNothing(){ int[]array={4}; int[] collectionVar = {1}; for(int x:collectionVar) { if(null==array) ; // should not report the warning but it does .... array = null; } } } This is because we report the warning as soon as we encounter if(null==array), as we have also carried over the nullness info due to which we know array is definitely non null. The warnings should ideally be reported on line 145 of ForEachStatement#analyseCode() : loopingContext.complainOnDeferredNullChecks(currentScope, actionInfo); This method call takes place after nullness info from inside and outside the loop have been merged together into actionInfo. In the present scenario, this merging results in a maybe null status for the variable we want to report a redundant null check for. The question, then, is- how do we defer the warnings if we want to carry the nullness info into the loop's analysis? Or, is there some alternate way to report the warning without actually carrying the nullness info? The fix in case of a collection variable is still trivial ie. marking the collection variable as definitely non null for the static analysis of the loop statements. The above patch illustrates the missing warnings in the 4 kinds of loops.In place of 9 warnings, it reports none.
Created attachment 151153 [details] proposed fix v0.5 + regression test The strategy followed in the fix is: For the variable being compared to null - we check the nullness status in the null info upstream of the loop, and if it is properly initialised AND if it hasnt been possibly nulled in any loop statement as yet, mark it as definitely non null in the loop's nullness info. However, we dont go and report the warning at this point but defer it until the end of the loop. The check !flowInfo.isPotentiallyNull(local) tackles cases such as: void myMethod(boolean b){ int[] myArray = {1,2}; for(int x: collection) { if(b) myArray = null; ..... if(myArray == null); //We keep quiet because myArray's nullness depends on runtime now. } }
Created attachment 151789 [details] proposed fix v1.0+ regression tests Same fix with a bit of modification in regression tests. Added NullReferenceTest#testBug291418b Activated NullReferenceTest#test0470_while() which was released as part of bug 220788. Please note that this fixes the problem as reported, along with additional problems found later during the analysis. Problems reported in bug 123399, bug 190623, bug 129581 and others related to sequel null errors (ie. null errors reported due to a redundant null check introduced somewhere upstream)are not fixed here, and will still be reproducible.
As far as I can say, this looks good. I am not familiar with the flow analysis code at all, but the code change makes sense. The code improves the case described in comment 0. class Bug { void foo(boolean b) { int[] array = null; if (b) { array = new int[] {1 , 2}; } for (int x : array) { if (null == array); } } } reports two issues. Potential null access for array in the foreach declaration and a redundant null check inside the for loop. This can look weird, but it makes sense. If we end up inside the for loop, this means the array local is not null as it is used to initialize the 'x' local. So +1. It might be a good idea for you to spend some time on other null check issues.
*** This bug has been marked as a duplicate of bug 190737 ***
