Summary: | [compiler][null][tests] org.eclipse.jdt.core.tests.compiler.regression.NullReferenceTest.test1004 contains an error | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | [Eclipse Project] JDT | Reporter: | Patrice Chalin <chalin> | ||||||
Component: | Core | Assignee: | Ayushman Jain <amj87.iitr> | ||||||
Status: | VERIFIED FIXED | QA Contact: | |||||||
Severity: | normal | ||||||||
Priority: | P5 | CC: | amj87.iitr, chalin, karabot, markus.kell.r, Olivier_Thomann, perry, philippe_mulet | ||||||
Version: | 3.3 | ||||||||
Target Milestone: | 3.6 M5 | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Whiteboard: | |||||||||
Attachments: |
|
Description
Patrice Chalin
2007-06-01 23:28:23 EDT
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 |