Community
Participate
Working Groups
Build ID: I20070525-1350 Steps To Reproduce: org.eclipse.jdt.core.tests.compiler.regression.NullReferenceTest.test1004() contains an error: the relevant part of the test case is: public class X { X foo(X x) { x.foo(null); // 0 if (x != null) { // 1 ... x.foo(null); // 8 } return this; } } The part of the expected output that is incorrect is: ---------- 5. ERROR in X.java (at line 12) x.foo(null); // 8 ^ Potential null pointer access: The variable x may be null at this location ---------- At point (8) in the program, x cannot be null since it is in the then part of an if with condition "x != null". More information: We found this bug while running JML4 on the JDT tests. JML4 builds on the JDT Core to provide support for the Java Modeling Language (http://www.jmlspecs.org). We will try to locate the source of the bug and identify the fix (which is already in JML4). Notes: - FYI: the incorrect expected output was added to this test via a patch associated with bug #110030. - A pre-alpha release of JML4 is available, should you be interested in trying it out (http://www.encs.concordia.ca/~dsrg/JML/).
Patrice, the error you point out is a sequel from the error reported against the line commented as // 2. Fixing the latter error would clear the one you complain about. I agree that, ideally, we should not introduce sequel errors, but the logic of the algorithm is the following: line // 2 introduces (wrongly) a doubt about the null status of x, then that doubt fires a warning on line 8. I believe that we (JDT) would not mind having the sequel error *or not*, both logics being acceptable (outer test protects x vs unneeded inner test taints x). Accordingly, I'll resolve this bug as WONTFIX, except if Philippe thinks otherwise.
Created attachment 69895 [details] patch for EqualsExpression and NullReferenceTest
Here's a patch that fixes the problem with this and a few other test cases, with only 6 lines added to EqualsExpression.
Tests for conditions that are inconsistent with the flow analysis info at that point should not affect the flow analysis, especially if this can be fixed in a straightforward manner. Reporting problems that have been allowed to pass quietly is another bonus of resolving this issue.
The null analysis is not expected to feed the constant field of expressions, that is meant to reflect a more conservative view of what is constant, and what is not. I has impacts on the code that is generated, so I'd stay on the safe side here and let the constant field unchanged. We could consider the remainder of the patch. Note that it is not the ultimate solution though, since it breaks the following: if (o != null) { if (o == null) { // reports a warning o.toString(); // should report a warning and does not with the patch } }
See also bug 129581, for which we simply decided to defer the resolution. This one is almost a duplicate, and should be worked upon simultaneously.
(In reply to comment #5) > The null analysis is not expected to feed the constant field of expressions, > that is meant to reflect a more conservative view of what is constant, and what > is not. I has impacts on the code that is generated, so I'd stay on the safe > side here and let the constant field unchanged. That is sensible. > We could consider the remainder of the patch. Note that it is not the ultimate > solution though, since it breaks the following: /*1*/ if (o != null) { /*2*/ if (o == null) { // reports a warning /*3*/ o.toString(); // should report a warning and does not with the patch /*4*/ } /*5*/ } But line 3 is actually unreachable code. Generally, nullity flow is not done for unreachable code: e.g. in FlowContext: > public void recordUsingNullReference(...) { > if ((flowInfo.tagBits & FlowInfo.UNREACHABLE) != 0 || > flowInfo.isDefinitelyUnknown(local)) { > return; > } > ... A sample program illustrating this is: Object o = null; if (false) { o.toString(); // no problem reported here } I believe that a consistent approach should be adopted for nullity flow analysis. I.e. no error should be reported at line 3 in the code snipper given at the top of this comment.
Created attachment 70061 [details] A patch using the optimizedBooleanConstant field instead of the constant field The previous patch submitted by Perry assigned to the Expression.constant field -- unfortunately, (as far as I understand) this meant we were doing constant folding (and I agree with Maxime that this should be avoided). Instead, the field BinaryExpression.optimizedBooleanConst seems to be suitable for our purpose here since it is meant to be a "constant usable for bytecode pattern optimizations, but cannot be inlined since it is not strictly equivalent to the definition of constant expressions." This new patch makes use of optimizedBooleanConst. With in the semantics of flow analysis relative to if statements becomes consistent with other occurrences of unreachable code.
A similar case where the variable gets 'tainted' on a check against null public class Try { public static void main(String[] args) { Number n= getNumber(); if (n instanceof Double) { Double d= (Double) n; if (d != null && d.isNaN()) { //"redundant null check" (good) System.out.println("outside loop"); } for (int i= 0; i < 10; i++) { if (d != null && d.isNaN()) { //"redundant null check" missing System.out.println("inside loop"); } } } } private static Number getNumber() { return new Double(Math.sqrt(-1)); } }
Patch provided in bug 293917. NullReferenceTest#testBug190623 added for this bug.
Verified for 3.6M5 using I20100125-0800
