Community
Participate
Working Groups
On 3.2M4 plus patch from bug 110030. The following case (NullReferenceTest #612): public class X { void foo(Object doubt) { Object o = null; do { if (o == null) { // complain: can only be null return; } o = doubt; } while (true); } } does not yield the expected message (silent instead). Most promising path would probably be to better exploit the return information.
The issue here is that we need to either 'guess' that we have a return downstream, which we cannot do without a second pass, or to store enough information and to carry it up to the return statement, so as to recombine the upstream information and complain if appropriate. The former is forbidden by design (we want a single pass). The latter calls for more than the flow info at the return point (else we would not know where the test occurs, only that o is - again - non null). The information held by the looping context should be specific enough, but we need a copy to avoid complaining on downstream points. Alternatively, we could check the upstream flow info against the current looping context contents whenever we reach a statement that jumps out of the loop. I'll do an experiment following that path.
Entered test cases NullReferenceTest #452 (shows that the problem happens with while loops as well) and #453 (see below). The problem with the approach of comment #1 is that conditional branches change which parts of the looping context can be used to trigger errors. In the case illustrated below, only the test on o2 should yield a message. Yet, until we consider stacking a context for the if(o1 == null) test, there is no means by which we could differentiate the test on o1 from the test on o2 at the context level. public class X { void foo(Object doubt, boolean b) { Object o1 = null, o2 = null; while (true) { if (o2 == null) { /* empty */ } // quiet: can be null or doubt if (o1 == null) { // complain: can only be null return; } o1 = o2 = doubt; } } } Many variants could be written that show that we miss some information that would have both the long lifecycle of flow contexts *and* the precision relatively to code paths of flow infos. Adding supplementary context instances for all branches may help, but at a dear cost.
Added NullReferenceTest # 614 and 615 that show that all deep exits exhibit the same problem. Philippe, I would recommand a wontfix here, since this calls for complex solutions that other static analysis tools are better equiped to tackle (especially regarding the number of passes). What do you think?
Reopening as P5 (since RESOLVED LATER is deprecated).
Here's a null issue that was forgotten a while ago. Ayush, do we want to close as WONTFIX (see comment 3)?
(In reply to comment #5) > Here's a null issue that was forgotten a while ago. > Ayush, do we want to close as WONTFIX (see comment 3)? Thanks for pointing it out Stephan. I think Maxime has already spent time working on this and as can be seen, a comprehensive solution is not possible without a second pass (which we dont want), or more contexts. Unless, you think you can investigate this, or have something in mind, I would recommend a WONTFIX.
I agree with the recommendation from comment 3 and comment 6. Static analysis of loops generally has to consider flows wrapping at the bottom of the loop. The pattern discussed in this bug would require that for particular data flows the analysis "knows" that wrapping cannot occur due to a return statement between the current statement and the end of the loop. Given the design decision to implement analysis as a single pass, there doesn't seem to exist a viable strategy that would effectively connect the two locations: the location to complain about and the return statement downstream. Reasoning about returns deeply nested within conditional structures and the option to duplicate context information for each condition involved demonstrate how such strategy would open a big can of worms that the compiler should better leave to dedicated analysis tools, which, e.g., have the option to perform any required number of passes. Closing as WONTFIX.
Verified with 3.7M4.
