Community
Participate
Working Groups
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.9.1.4) Gecko/20091016 Firefox/3.5.4 (.NET CLR 3.5.30729) Build Identifier: 20090920-1017 Potential null access warning seems to contain bug in its logic. At first, it gives warnings when there is no chance of null. In the follwing code: String x = null, y = null; if (x == null) x = "foo"; if (x != null) y = "bar"; x.length(); y.length(); ... the warning is issued on both x.length() and y.length(). Both are wrong. Guess what happens when you comment 4th line: String x = null, y = null; if (x == null) x = "foo"; //if (x != null) y = "bar"; x.length(); y.length(); y.length() still gives the warning which is ok at this time, but x.length() disappears! Obviously this is not the proper behavior. Reproducible: Always Steps to Reproduce: 1. Enable 'Potential null pointer access' warning in compiler configuration. 2. Create a class and a method with code described above. 3. Check compiler errors/warnings.
Ayush, in the context of bug 291418, you had found several problems, is what is described in this report one of them ? Please investigate, Thanks.
I think this is a communication problem: users seem to expect a perfect flow analysis, OTOH users don's see what actually triggers a warning. From test cases I learned that after if (foo == null) { } the analysis assumes that nullity of foo is unknown at this point. (Otherwise the if would not make sense). Applying this observation to the example confirms that the if (x != null) statement marks x as unknown and the else branch may mark y as unknown. @Marcin: it should become clearer if you investigate warnings top to bottom: both if statements have warnings (at least in 3.6 M3, didn't try 3.5.x): if (x == null) x = "foo"; ^ Redundant null check: The variable x can only be null at this location if (x != null) y = "bar"; ^ Redundant null check: The variable x cannot be null at this location Once you correct the program so that these warnings go, the other ones should go, too.
(In reply to comment #1) > Ayush, in the context of bug 291418, you had found several problems, > is what is described in this report one of them ? No not really. All those problems had to do with looping statements only. Will have to investigate this one. (In reply to comment #2) >From test cases I learned that after > if (foo == null) { } > the analysis assumes that nullity of foo is unknown at this point. Your'e correct. But this is only in the case of variables such as arguments to methods whose nullness info isn't really confirmed from the very beginning in a particular context. > Applying this observation to the example confirms that the > if (x != null) statement marks x as unknown and the else > branch may mark y as unknown. In the given case where we're sure of the path that the program will follow at runtime, the static analysis should have no doubt on the nullness of x. Ideally, the else statement here should be marked as UNREACHABLE, and so when merging the nullness info from the if and else streams, we'll reject that of the else stream (this is the current behaviour as well). So even if the else stream considers y as still being null, it shouldnt influence our analysis following the if-else. I think it makes sense to look into why x and y are still being flagged off as potentially null.
Created attachment 154955 [details] proposed fix v0.5 + regression test Here's the strategy followed: Wherever a redundant null check is done => we'll surely execute the 'then' condition. So we set the unreachable bit for the else flow stream. Wherever a check always yields false => we'll surely execute the else condition. So we set the unreachable bit for the then flow stream. The idea is to avoid tainting of variables whose null status we know for sure. The above strategy basically short circuits the analysis of the parts of code that'll never be executed and thus prevents any false tainting of variables. These changes are included in FinallyFlowContext.java, FlowContext.java, and LoopingFlowContext.java. The changes in EqualExpression.java and IfStatement.java optimize the bytecode generated by the compiler ie. when we know for sure what branch in an if else condition will be taken, we set the optimised boolean constant accordingly and thus avoid creating bytecode corresponding to an if statement. We simply generate the bytecodes for the statements which are going to be executed. The above changes also expose dead code warnings since the unreachable bit gets set. So the modifications in existing regression tests have two reasons: 1. To include dead code warnings at the appropriate places. 2. To reflect the change in bytecode generated in some cases. Added regression test NullReferenceTest#testBug293917(), NullReferenceTest#testBug190623(). Activated regression test NullReferenceTest#test0335_if_else(). This patch also fixes bug 190623, and bug 129581.
Created attachment 156262 [details] Slightly modified patch for regression tests I slighlty modified the patch for the regression tests update. I disabled dead code detection for assignment tests and I updated the StackMapAttributeTests to keep some stack map table otherwise the two modified tests are not meaningfull anymore. Let me know what you think.
Both changes make sense. The changes in the StackMapAttributeTests also dont seem to be defeating the purpose for which those tests were written, so i guess everything's in shape here.
Created attachment 156644 [details] same patch with two additional tests Just some consolidating changes after srikanth's feedback. Added a couple more regression tests to NullReferenceTest.java to verify that the fix works fine for loops and finally blocks. Corrected an indentation in EqualExpression.java Changed copyrights to 2010.
Released for 3.6M5. Updated existing regression tests.
*** Bug 300239 has been marked as a duplicate of this bug. ***
Verified for 3.6M5 using I20100125-0800
