Bug 302446 - [compiler] Regression in if statement flow analysis related to null checks
Summary: [compiler] Regression in if statement flow analysis related to null checks
Status: VERIFIED FIXED
Alias: None
Product: JDT
Classification: Eclipse Project
Component: Core (show other bugs)
Version: 3.6   Edit
Hardware: PC Windows XP
: P3 normal (vote)
Target Milestone: 3.6 M6   Edit
Assignee: Ayushman Jain CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-02-10 10:39 EST by Olivier Thomann CLA
Modified: 2010-03-08 13:45 EST (History)
0 users

See Also:
Olivier_Thomann: review+

Attachments
proposed fix + regression test (21.51 KB, patch)
2010-02-10 14:25 EST, Ayushman Jain CLA 		Olivier_Thomann: iplog+
Details | Diff
Proposed fix (21.42 KB, patch)
2010-02-10 20:35 EST, Olivier Thomann CLA 		no flags Details | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Olivier Thomann CLA 2010-02-10 10:39:57 EST 
The following code should complain that i may have not been initialized after the if statement. The recent fixes around null check analysis improvement broke this as it considers that i is definitely assigned after the if statement.

public class X {
	void foo() {
		Object o = null;
		int i;
		if (o == null) // redundant check
			i = 0;
		System.out.println(i);
	}
}

This should be a negative test. Right now we compile without reporting any error or we report a redundant null check if the null warning is enabled.

We should also make sure that the try/catch/finally statement is properly handled.
Comment 1 Ayushman Jain CLA 2010-02-10 14:25:36 EST 
Created attachment 158761 [details]
proposed fix + regression test

The problem is: with our new analysis, we are sure that the comparison o==null is redundant and that the if statement will always be executed, as a result of which we concluded that i will always be initialised. But we dont want to be concluding the initializations so aggressively.

Attaching a patch which takes care of this, although it changes the null analysis code a bit. Now we do set the unreachable bit, but we dont set optimised constants. Two bits in ASTNode, Bit8 and 9, take care of code gen according to whether unreachable bit is set or not, and also report dead code warnings at appropriate places. A new method FlowInfo#mergedOptimisedBranchesIfElse() is introduced in addition to the old FlowInfo#mergedOptimisedBranches(). This new method takes care of the initializations inside if else constructs where one branch is sure to be executed.
Comment 2 Olivier Thomann CLA 2010-02-10 20:35:22 EST 
Created attachment 158805 [details]
Proposed fix

Same patch with minor changes.
Comment 3 Olivier Thomann CLA 2010-02-10 20:37:00 EST 
Released for 3.6M6.
Regression tests added in org.eclipse.jdt.core.tests.compiler.regression.NullReferenceTest. Regression tests will be released tomorrow so that the nightly build doesn't fail.
Comment 4 Olivier Thomann CLA 2010-02-11 10:01:48 EST 
Released tests:
org.eclipse.jdt.core.tests.compiler.regression.NullReferenceTest#test0569_try_catch
org.eclipse.jdt.core.tests.compiler.regression.NullReferenceTest#test0570_try_catch
org.eclipse.jdt.core.tests.compiler.regression.NullReferenceTest#test0571_try_catch_finally
org.eclipse.jdt.core.tests.compiler.regression.NullReferenceTest#test0572_if_statement
Comment 5 Olivier Thomann CLA 2010-03-08 13:45:12 EST 
Verified for 3.6M6 using I20100307-2000.