Community
Participate
Working Groups
This program: public class C { public static void main(String[] args) { Integer i = null; int j = i; System.out.println(j); } } compiles with no warning and throws NPE at runtime.
Created attachment 173727 [details] proposed patch with a few tests Wow, so far I found and fixed three locations lacking a call to checkNPE if unboxing is involved: Assignment, LocalDeclaration, MessageSend. And this isn't complete yet. Before we walk through all expression types, do you think inserting these calls at the receiving AST node is good? I first tried inserting the check at the providing node (e.g., a SingleNameReference) but that would create duplicate warnings in two tests involving an EqualExpression which already checks for null of its operands.
Isn't int j = i below short for int j = i.intValue(); ??? Integer i = null; int j = i.intValue(); // Would give a NPE here System.out.println(j); and int j = null; // would give "Type missmatch"
(In reply to comment #1) > Before we walk through all expression types, do you think inserting > these calls at the receiving AST node is good? I first tried inserting > the check at the providing node (e.g., a SingleNameReference) > but that would create duplicate warnings in two tests involving > an EqualExpression which already checks for null of its operands. Thanks for the patch Stephen!I dont see how you get a duplicate warning though, if you insert the check at SingleNameReference#analyseCode()? In Expression#checkNPE():511, we mark the expression as being compared to non null, so any subsequent checks in equal expression wont flag the NPE again. Am i missing something here?
(In reply to comment #3) > Thanks for the patch Stephen!I dont see how you get a duplicate warning though, > if you insert the check at SingleNameReference#analyseCode()? In > Expression#checkNPE():511, we mark the expression as being compared to non > null, so any subsequent checks in equal expression wont flag the NPE again. Am > i missing something here? Good question. It occurred at testBug253896c and testBug253896d. I will try later to see why setting NULL_FLAG_MASK doesn't prevent double reporting. Inserting the check only in SingleNameReference would feel much better.
Forget my comment re NULL_FLAG_MASK. The duplicate warnings are caused by calls to recordNullReference(local, reference, checkType); from either LoopingFlowContext (testBug253896c) in line 580 or from FinallyFlowContext (testBug253896d) in line 406. Both branches are reached although flowInfo.isDefinitelyNonNull(local) would answer true. I must admit that I don't quickly see, who's to blame for the duplicate warning. Do both flow contexts (loop & finally) generally assume that null can *always* occur?
Excuse my piece-meal comments here, I'm not too familiar with the null check logic but I'm learning: so recordNullReference(..) is used to collect references for deferred analysis and complainOnDeferredNullChecks(..) performs this deferred analysis. If I add the call to checkNPE into SingleNameReference.analyseCode the flow context records the same reference twice: 1. with nullCheckType[i] == MAY_NULL (from SingleNameReference) 2. with nullCheckType[i] == CAN_ONLY_NULL | IN_COMPARISON_NON_NULL (from EqualExpression) flow context has no means to collate those entries and it seems non-trivial to figure out how those nullCheckTypes should be merged, right? While it would be nice if complainOnDeferredNullChecks(..) would report only the most severe issue, I'd currently say it's easier to insert checkNPE at the "consuming" sides as I started in my patch. Am I missing anything.
(In reply to comment #6) > Excuse my piece-meal comments here, I'm not too familiar with > the null check logic but I'm learning: > > so recordNullReference(..) is used to collect references for deferred > analysis and complainOnDeferredNullChecks(..) performs this deferred > analysis. > > If I add the call to checkNPE into SingleNameReference.analyseCode > the flow context records the same reference twice: > 1. with nullCheckType[i] == MAY_NULL (from SingleNameReference) > 2. with nullCheckType[i] == CAN_ONLY_NULL | IN_COMPARISON_NON_NULL > (from EqualExpression) > flow context has no means to collate those entries and it seems non-trivial > to figure out how those nullCheckTypes should be merged, right? > > While it would be nice if complainOnDeferredNullChecks(..) would > report only the most severe issue, I'd currently say it's easier > to insert checkNPE at the "consuming" sides as I started in my patch. > > Am I missing anything. I think testBug253896c makes it clear that its not prudent to raise the warning on encountering a singleNameReference straightaway. For looping and finally contexts, where the check is deferred because the null state may be modified somewhere down the loop, we can't afford to raise a premature warning, as well as setting nullCheckType to MAY_NULL. So i think your initial approach of putting the checkNPE at consuming sides will do more good, since the current context best knows how to handle a null warning. I think this is why we never had the checkNPE in SingleNameReference in the first place! :)
(In reply to comment #7) > So i think your initial approach of > putting the checkNPE at consuming sides will do more good, since the current > context best knows how to handle a null warning. Would have been nice to avoid that, but, OK, lets collect a list of all AST nodes that consume a primitive value (that could require unboxing): From my initial patch: Assignment, LocalDeclaration, MessageSend. More to cover (only listing those which don't yet checkNPE): AllocationExpression (arguments) AND_AND_Expression (left & right) ArrayAllocation (dimensions) AssertStatement (assertExpression) DoStatement (condition) ExplicitConstructorCall (arguments) ForeachStatement (elementVariable) ForStatement (condition) IfStatement (condition) OR_OR_Expression (left & right) QualifiedAllocationExpression (arguments) ReturnExpression (expression) SwitchStatement (expression) WhileStatement (condition) Of these, ForeachStatement is special: Integer[] is = new Integer{ null }; for(int i : is) System.out.print(i+1); Would we ever have a chance to know that unboxing will *not* fail due to null? Null-analysis cannot look into the array, right? So we might skip ForachStatement to avoid 100% false positives. If you agree with this list I'll go ahead and prepare a more complete patch.
Looking at AutoBoxingTest the following are relevant, too: CastExpression (expression, when casting to primitive type0 FieldDeclaration (initialization)
(In reply to comment #8) > Of these, ForeachStatement is special: > Integer[] is = new Integer{ null }; > for(int i : is) > System.out.print(i+1); > Would we ever have a chance to know that unboxing will *not* fail due > to null? Null-analysis cannot look into the array, right? > So we might skip ForachStatement to avoid 100% false positives. Yes, we can't look inside the array and unboxing may/may not fail, so we should omit foreachStatatement. Also, why do we need a check for the condition in ForStatement, While, doStatement, ifStatement, AND_AND, OR_OR? I believe we're already reporting the correct warning in these cases. I also tried a few examples and could see the null reference warning. Do you have cases in which you think there should be wa warning and there isn't any? I think these should be omitted. Again, for ASSERT statement, we report the null reference warning correctly because it also calls condition.analyseCode. So ASSERT also doest require any change.
(In reply to comment #10) > Also, why do we need a check for the condition in ForStatement, While, > doStatement, ifStatement, AND_AND, OR_OR? I believe we're already reporting the > correct warning in these cases. I also tried a few examples and could see the > null reference warning. Really? What did you try? > Do you have cases in which you think there should be wa > warning and there isn't any? The following test class produces no null-warnings: class Y { public Y(boolean b) {} } public class X extends Y { public X(boolean b, Boolean b2) { super(b2); } class Z { public Z(boolean b) {} } boolean fB = (Boolean)null; public boolean foo(boolean inB) { Boolean b1 = null; X x = new X(b1, null); boolean dontcare = b1 && inB; dontcare = inB || b1; Integer dims = null; char[] cs = new char[dims]; do { for (int i=0;b1; i++); } while (b1); if (b1) { } Z z = this.new Z(b1); Integer sel = null; switch(sel) { case 1: break; default: break; } while (b1) {} dontcare = (boolean)b1; return b1; } } I will prepare a patch that fixes these. > Again, for ASSERT statement, we report the null reference warning correctly > because it also calls condition.analyseCode. So ASSERT also doest require any > change. I forgot AssertStatement, but I disagree with your analysis: just as with the other nodes, invoking analyseCode on a sub-node doesn't help, we have to call condition.checkNPE(..) explicitly (if unboxing is involved).
(In reply to comment #11) > (In reply to comment #10) > > Also, why do we need a check for the condition in ForStatement, While, > > doStatement, ifStatement, AND_AND, OR_OR? I believe we're already reporting the > > correct warning in these cases. I also tried a few examples and could see the > > null reference warning. > > Really? What did you try? Sorry, I got a bit off track. I was trying cases such as Integer i = null, j = 1; if( i == 1&& j == 1) {} These are not SingleNameReference cases. My bad. > I forgot AssertStatement, but I disagree with your analysis: just as with > the other nodes, invoking analyseCode on a sub-node doesn't help, we have > to call condition.checkNPE(..) explicitly (if unboxing is involved). Yes i agree there are cases such as when the sub-node is a SingleNameReference that invoking analyseCode() doesnt help, as shown in the testcase you provided. Go ahead with the fix then. We'll have to carefully test the fix - to make sure there are no false positives.
Created attachment 174412 [details] tests & fixes This patch fixes the issue for 17 kinds of AST nodes. Each individual fix is simple (all use the same pattern). All these are covered by new tests. At least within the NullReferenceTest no existing tests are affected.
*** Bug 320216 has been marked as a duplicate of this bug. ***
(In reply to comment #13) > Created an attachment (id=174412) [details] > tests & fixes > Thanks Stephen. I have a few points regarding the fix: 1) For the do{..}while case, the fix gives false positives. CASE1: Boolean b = null; do { for(int i = 1; i<10; i++); } while (b) // NPE: OK CASE2: Boolean b = null do { b = true; } while (b) // NPE: not ok This happens because for doStatement, there are two infos - flowInfo and actionInfo(corresponding to the action statements) . Just using flowInfo in the checkNPE doesnt incorporate the null related changes that get affected in actionInfo. I tried this on line 94 after condition.analyseCode(..) has been called: if ((this.condition.implicitConversion & TypeIds.UNBOXING) != 0) { this.condition.checkNPE( currentScope, flowContext, ((this.action == null)? flowInfo : flowInfo.unconditionalCopy().addPotentialNullInfoFrom( actionInfo.unconditionalCopy()))); } With this CASE2 gives Potential NPE, which looks better ( potential because there can be a break in the do actions itself). What do you think? 2) You need to put a space after // in line comments. Please also mention as a comment over each test what all nodes it tests for this warning. I'll run the tests on your patch and see if there's any regression due to this. Everything else looks fine.
(In reply to comment #15) > I'll run the tests on your patch and see if there's any regression due to this. > Everything else looks fine. All tests pass. Stephen, please take care of the 2 points in comment 15 and i'll release the patch. Thanks!
Created attachment 174950 [details] improved patch (In reply to comment #16) > 1) For the do{..}while case, the fix gives false positives. > CASE1: Boolean b = null; > do { > for(int i = 1; i<10; i++); > } while (b) // NPE: OK > CASE2: Boolean b = null > do { > b = true; > } while (b) // NPE: not ok Good point. > I tried this on line 94 after condition.analyseCode(..) has been called: > [...] > > With this CASE2 gives Potential NPE, which looks better ( potential because > there can be a break in the do actions itself). What do you think? I think we can do even better :) I don't think breaks are relevant for the condition, but continues are. Also real analysis of the action can be leveraged for more precise results. I've added a test for a few variants of DoStatements like this: public void foo(boolean cond) { Boolean b = null; do { b = false; if (cond) continue; // shouldn't make a difference } while (b); // don't complain, loop body has already assigned b Boolean b2 = null; do { if (cond) continue; // now assignment is reached only potentially b2 = false; } while (b2); // complain here: potentially null Boolean b3 = null; do { } while (b3); // complain here: definitely null do { if (cond) { b4 = true; if (cond2) continue; } b4 = false; } while (b4); // don't complain here: definitely non-null } > 2) You need to put a space after // in line comments. Please also mention as a > comment over each test what all nodes it tests for this warning. done.
(In reply to comment #17) > Created an attachment (id=174950) [details] > improved patch With this the following cases give spurious warnings: Boolean b1 = null; do { b1 = true; } while (b1); // warns potential NPE Boolean b2 = null; do { b2 = true; continue; } while (b2); // warns NPE
Created attachment 174994 [details] patch version 3 (In reply to comment #18) > (In reply to comment #17) > > Created an attachment (id=174950) [details] [details] > > improved patch > > With this the following cases give spurious warnings: > Boolean b1 = null; > do { > b1 = true; > } while (b1); // warns potential NPE > Boolean b2 = null; > do { > b2 = true; > continue; > } while (b2); // warns NPE You're right, combining the various flow infos in DoStatement needs to be smarter: if either flow is a dead end adopt the other one as uncondintional. Test & fix have been updated.
(In reply to comment #19) > if either flow is a dead end adopt the other one as > uncondintional. I didnt quite understand what you meant by this statement. Also with this, the following case doesnt give an NPE: boolean bar() { return true; } void foo() { Boolean b1 = null, b2 = null; do { if(bar() ){ b1 = true; continue; } else { b2 = true; } }while (b1); // No NPE here. Removing continue makes NPE appear. Its ok if you think this is a separate issue. We can open another bug for it. I just want to make sure the fix covers all cases it intends to.
Created attachment 175518 [details] patch version 4 (In reply to comment #20) > (In reply to comment #19) > > if either flow is a dead end adopt the other one as > > uncondintional. > > I didnt quite understand what you meant by this statement. Never mind, that solution wasn't good enough as your next example shows. > Also with this, the following case doesnt give an NPE: > boolean bar() { > return true; > } > void foo() { > Boolean b1 = null, b2 = null; > do { > if(bar() ){ > b1 = true; > continue; > } else { > b2 = true; > } > }while (b1); // No NPE here. Removing continue makes NPE appear. > > Its ok if you think this is a separate issue. We can open another bug for it. I > just want to make sure the fix covers all cases it intends to. I added this to testBug319201d() and I agree with your observation. My solution was drawing from incorrect analogies. Tests show that the info from the action and loopingContext.initsOnContinue need to be combined still differently. By just looking a few source lines ahead I seem to find the solution: actionInfo.mergedWith(loopingContext.initsOnContinue) this expression is used in the argument to this.condition.analyseCode(..) and it also gives the expected null info for all tests we have by now. This is also plausible by the following reasoning: I previously used addPotentialInitializationsFrom and addInitializationsFrom. However, these functions are intended for sequention flows, whereas the state at the condition joins different branches: those flows that completed through action and those flows that jumped from a continue to the condition. As other uses show, merging two flows must be done using the mergedWith(..) function. I've updated the patch incl. copyright headers.
(In reply to comment #21) > Created an attachment (id=175518) [details] > patch version 4 Thanks Stephen. This patch looks good. I'll release it for 3.7M2.
(In reply to comment #22) > Thanks Stephen. This patch looks good. I'll release it for 3.7M2. Lovely. Actually my reason for diving into null-analysis was on the lines of bug 292478. Do you see a chance to look into that patch any time soon? That would make me happy :) Note, that bug 292478 has a dependency on bug 320170 but I think we have sufficiently discussed that one, right?
(In reply to comment #23) {..} > Actually my reason for diving into null-analysis was on the lines of bug > 292478. > Do you see a chance to look into that patch any time soon? > That would make me happy :) > > Note, that bug 292478 has a dependency on bug 320170 but I think we > have sufficiently discussed that one, right? Sure Stephen, I'll try and review the patch as soon as I can. I was deferring it because of some more important items. My apologies. Bug 320170 is anyways ready for release, so we're good on that front. :)
Released in HEAD for 3.7M2. Added tests NullReferenceTests#testBug319201(), NullReferenceTests#testBug319201a()....testBug319201d().
Verified for 3.7M2 using I20100914-0100
