Attachment 190428 Details for Bug 324178 – simplified patch

[patch] simplified patch

Bug_324178_v4.patch (text/plain), 14.98 KB, created by Stephan Herrmann

on 2011-03-04 14:10:12 EST

(hide)

Description:

Filename:

MIME Type:

Creator: Stephan Herrmann

Created: 2011-03-04 14:10:12 EST

Size: 14.98 KB

patch

obsolete

>### Eclipse Workspace Patch 1.0
>#P org.eclipse.jdt.core
>Index: compiler/org/eclipse/jdt/internal/compiler/ast/ConditionalExpression.java
>===================================================================
>RCS file: /cvsroot/eclipse/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/ast/ConditionalExpression.java,v
>retrieving revision 1.98
>diff -u -r1.98 ConditionalExpression.java
>--- compiler/org/eclipse/jdt/internal/compiler/ast/ConditionalExpression.java	14 Jan 2011 17:02:24 -0000	1.98
>+++ compiler/org/eclipse/jdt/internal/compiler/ast/ConditionalExpression.java	4 Mar 2011 18:55:46 -0000
>@@ -7,7 +7,10 @@
>  *
>  * Contributors:
>  *     IBM Corporation - initial API and implementation
>- *     Stephen Herrmann <stephan@cs.tu-berlin.de> -  Contributions for bugs 133125, 292478
>+ *     Stephen Herrmann <stephan@cs.tu-berlin.de> -  Contributions for 
>+ *     						bug 133125 - [compiler][null] need to report the null status of expressions and analyze them simultaneously
>+ *     						bug 292478 - Report potentially null across variable assignment
>+ * 							bug 324178 - [null] ConditionalExpression.nullStatus(..) doesn't take into account the analysis of condition itself
>  *******************************************************************************/
> package org.eclipse.jdt.internal.compiler.ast;
> 
>@@ -29,6 +32,9 @@
> 	int trueInitStateIndex = -1;
> 	int falseInitStateIndex = -1;
> 	int mergedInitStateIndex = -1;
>+	
>+	// we compute and store the null status during analyseCode (https://bugs.eclipse.org/324178):
>+	private int nullStatus = FlowInfo.UNKNOWN;
> 
> 	public ConditionalExpression(
> 		Expression condition,
>@@ -81,10 +87,30 @@
> 		FlowInfo mergedInfo;
> 		if (isConditionOptimizedTrue){
> 			mergedInfo = trueFlowInfo.addPotentialInitializationsFrom(falseFlowInfo);
>+			this.nullStatus = this.valueIfTrue.nullStatus(trueFlowInfo);
> 		} else if (isConditionOptimizedFalse) {
> 			mergedInfo = falseFlowInfo.addPotentialInitializationsFrom(trueFlowInfo);
>+			this.nullStatus = this.valueIfFalse.nullStatus(falseFlowInfo);
> 		} else {
>-			// if ((t && (v = t)) ? t : t && (v = f)) r = v;  -- ok
>+			// this block must meet two conflicting requirements (see https://bugs.eclipse.org/324178):
>+			// (1) For null analysis of "Object o2 = (o1 != null) ? o1 : new Object();" we need to distinguish
>+			//     the paths *originating* from the evaluation of the condition to true/false respectively.
>+			//     This is used to determine the possible null status of the entire conditional expression.
>+			// (2) For definite assignment analysis (JLS 16.1.5) of boolean conditional expressions of the form
>+			//     "if (c1 ? expr1 : expr2) use(v);" we need to check whether any variable v will be definitely
>+			//     assigned whenever the entire conditional expression evaluates to true (to reach the then branch).
>+			//     I.e., we need to collect flowInfo *towards* the overall outcome true/false 
>+			//     (regardless of the evaluation of the condition).
>+			
>+			// to support (1) save the branches originating from the condition (for use by nullStatus):
>+			computeNullInfo(trueFlowInfo, falseFlowInfo);
>+			
>+			// to support (2) we split the true/false branches according to their inner structure. Consider this:
>+			// if (b ? false : (true && (v = false))) return v; -- ok
>+			// - expr1 ("false") has no path towards true (mark as unreachable)
>+			// - expr2 ("(true && (v = false))") has a branch towards true on which v is assigned.
>+			//   -> merging these two branches yields: v is assigned
>+			// - the paths towards false are irrelevant since the enclosing if has no else.
> 			cst = this.optimizedIfTrueConstant;
> 			boolean isValueIfTrueOptimizedTrue = cst != null && cst != Constant.NotAConstant && cst.booleanValue() == true;
> 			boolean isValueIfTrueOptimizedFalse = cst != null && cst != Constant.NotAConstant && cst.booleanValue() == false;
>@@ -93,26 +119,26 @@
> 			boolean isValueIfFalseOptimizedTrue = cst != null && cst != Constant.NotAConstant && cst.booleanValue() == true;
> 			boolean isValueIfFalseOptimizedFalse = cst != null && cst != Constant.NotAConstant && cst.booleanValue() == false;
> 
>-			UnconditionalFlowInfo trueInfoWhenTrue = trueFlowInfo.initsWhenTrue().unconditionalCopy();
>-			UnconditionalFlowInfo falseInfoWhenTrue = falseFlowInfo.initsWhenTrue().unconditionalCopy();
>-			UnconditionalFlowInfo trueInfoWhenFalse = trueFlowInfo.initsWhenFalse().unconditionalInits();
>-			UnconditionalFlowInfo falseInfoWhenFalse = falseFlowInfo.initsWhenFalse().unconditionalInits();
>+			UnconditionalFlowInfo trueFlowTowardsTrue = trueFlowInfo.initsWhenTrue().unconditionalCopy();
>+			UnconditionalFlowInfo falseFlowTowardsTrue = falseFlowInfo.initsWhenTrue().unconditionalCopy();
>+			UnconditionalFlowInfo trueFlowTowardsFalse = trueFlowInfo.initsWhenFalse().unconditionalInits();
>+			UnconditionalFlowInfo falseFlowTowardsFalse = falseFlowInfo.initsWhenFalse().unconditionalInits();
> 			if (isValueIfTrueOptimizedFalse) {
>-				trueInfoWhenTrue.setReachMode(FlowInfo.UNREACHABLE);				
>+				trueFlowTowardsTrue.setReachMode(FlowInfo.UNREACHABLE);				
> 			}
> 			if (isValueIfFalseOptimizedFalse) {
>-				falseInfoWhenTrue.setReachMode(FlowInfo.UNREACHABLE);	
>+				falseFlowTowardsTrue.setReachMode(FlowInfo.UNREACHABLE);	
> 			}
> 			if (isValueIfTrueOptimizedTrue) {
>-				trueInfoWhenFalse.setReachMode(FlowInfo.UNREACHABLE);	
>+				trueFlowTowardsFalse.setReachMode(FlowInfo.UNREACHABLE);	
> 			}
> 			if (isValueIfFalseOptimizedTrue) {
>-				falseInfoWhenFalse.setReachMode(FlowInfo.UNREACHABLE);	
>+				falseFlowTowardsFalse.setReachMode(FlowInfo.UNREACHABLE);	
> 			}
> 			mergedInfo =
> 				FlowInfo.conditional(
>-					trueInfoWhenTrue.mergedWith(falseInfoWhenTrue),
>-					trueInfoWhenFalse.mergedWith(falseInfoWhenFalse));
>+					trueFlowTowardsTrue.mergedWith(falseFlowTowardsTrue),
>+					trueFlowTowardsFalse.mergedWith(falseFlowTowardsFalse));
> 		}
> 		this.mergedInitStateIndex =
> 			currentScope.methodScope().recordInitializationStates(mergedInfo);
>@@ -120,6 +146,30 @@
> 		return mergedInfo;
> 	}
> 
>+	private void computeNullInfo(FlowInfo trueBranchInfo, FlowInfo falseBranchInfo) {
>+		// given that the condition cannot be optimized to a constant 
>+		// we now merge the nullStatus from both branches:
>+		int ifTrueNullStatus = this.valueIfTrue.nullStatus(trueBranchInfo);
>+		int ifFalseNullStatus = this.valueIfFalse.nullStatus(falseBranchInfo);
>+
>+		if (ifTrueNullStatus == ifFalseNullStatus) {
>+			this.nullStatus = ifTrueNullStatus;
>+			return;
>+		}
>+		// is there a chance of null (or non-null)? -> potentially null etc.
>+		// https://bugs.eclipse.org/bugs/show_bug.cgi?id=133125
>+		int status = 0;
>+		int combinedStatus = ifTrueNullStatus|ifFalseNullStatus;
>+		if ((combinedStatus & (FlowInfo.NULL|FlowInfo.POTENTIALLY_NULL)) != 0)
>+			status |= FlowInfo.POTENTIALLY_NULL;
>+		if ((combinedStatus & (FlowInfo.NON_NULL|FlowInfo.POTENTIALLY_NON_NULL)) != 0)
>+			status |= FlowInfo.POTENTIALLY_NON_NULL;
>+		if ((combinedStatus & (FlowInfo.UNKNOWN|FlowInfo.POTENTIALLY_UNKNOWN)) != 0)
>+			status |= FlowInfo.POTENTIALLY_UNKNOWN;
>+		if (status > 0)
>+			this.nullStatus = status;
>+	}
>+
> 	/**
> 	 * Code generation for the conditional operator ?:
> 	 *
>@@ -306,33 +356,9 @@
> 		codeStream.updateLastRecordedEndPC(currentScope, codeStream.position);
> 	}
> 
>-public int nullStatus(FlowInfo flowInfo) {
>-	Constant cst = this.condition.optimizedBooleanConstant();
>-	if (cst != Constant.NotAConstant) {
>-		if (cst.booleanValue()) {
>-			return this.valueIfTrue.nullStatus(flowInfo);
>-		}
>-		return this.valueIfFalse.nullStatus(flowInfo);
>+	public int nullStatus(FlowInfo flowInfo) {
>+		return this.nullStatus;
> 	}
>-	int ifTrueNullStatus = this.valueIfTrue.nullStatus(flowInfo),
>-	    ifFalseNullStatus = this.valueIfFalse.nullStatus(flowInfo);
>-	if (ifTrueNullStatus == ifFalseNullStatus) {
>-		return ifTrueNullStatus;
>-	}
>-	// is there a chance of null (or non-null)? -> potentially null etc.
>-	// https://bugs.eclipse.org/bugs/show_bug.cgi?id=133125
>-	int status = 0;
>-	int combinedStatus = ifTrueNullStatus|ifFalseNullStatus;
>-	if ((combinedStatus & (FlowInfo.NULL|FlowInfo.POTENTIALLY_NULL)) != 0)
>-		status |= FlowInfo.POTENTIALLY_NULL;
>-	if ((combinedStatus & (FlowInfo.NON_NULL|FlowInfo.POTENTIALLY_NON_NULL)) != 0)
>-		status |= FlowInfo.POTENTIALLY_NON_NULL;
>-	if ((combinedStatus & (FlowInfo.UNKNOWN|FlowInfo.POTENTIALLY_UNKNOWN)) != 0)
>-		status |= FlowInfo.POTENTIALLY_UNKNOWN;
>-	if (status > 0)
>-		return status;
>-	return FlowInfo.UNKNOWN;
>-}
> 
> 	public Constant optimizedBooleanConstant() {
> 
>#P org.eclipse.jdt.core.tests.compiler
>Index: src/org/eclipse/jdt/core/tests/compiler/regression/InitializationTests.java
>===================================================================
>RCS file: /cvsroot/eclipse/org.eclipse.jdt.core.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/InitializationTests.java,v
>retrieving revision 1.4
>diff -u -r1.4 InitializationTests.java
>--- src/org/eclipse/jdt/core/tests/compiler/regression/InitializationTests.java	21 Sep 2010 14:02:56 -0000	1.4
>+++ src/org/eclipse/jdt/core/tests/compiler/regression/InitializationTests.java	4 Mar 2011 18:55:49 -0000
>@@ -1,5 +1,5 @@
> /*******************************************************************************
>- * Copyright (c) 2010 IBM Corporation and others.
>+ * Copyright (c) 2010, 2011 IBM Corporation and others.
>  * All rights reserved. This program and the accompanying materials
>  * are made available under the terms of the Eclipse Public License v1.0
>  * which accompanies this distribution, and is available at
>@@ -7,6 +7,7 @@
>  *
>  * Contributors:
>  *     IBM Corporation - initial API and implementation
>+ *     Stephan Herrmann - contribution for bug 324178 - [null] ConditionalExpression.nullStatus(..) doesn't take into account the analysis of condition itself
>  *******************************************************************************/
> package org.eclipse.jdt.core.tests.compiler.regression;
> 
>@@ -14,6 +15,7 @@
> 
> import junit.framework.Test;
> 
>+import org.eclipse.jdt.internal.compiler.classfmt.ClassFileConstants;
> import org.eclipse.jdt.internal.compiler.impl.CompilerOptions;
> 
> public class InitializationTests extends AbstractRegressionTest {
>@@ -393,6 +395,75 @@
> 			"----------\n",
> 			null, false, options);
> }
>+
>+// Bug 324178 - [null] ConditionalExpression.nullStatus(..) doesn't take into account the analysis of condition itself
>+// definite assignment along all true-yielding paths is sufficient
>+public void testBug324178b() {
>+	this.runConformTest(
>+		new String[] {
>+			"Bug324178.java",
>+			"public class Bug324178 {\n" +
>+			"	 boolean foo(boolean b) {\n" +
>+			"        boolean v;\n" +
>+			"        if (b ? false : (true && (v = true)))\n" +
>+			"            return v;\n" + // OK to read v!
>+			"        return false;\n" +
>+			"    }\n" +
>+			"    public static void main(String[] args) {\n" +
>+			"        System.out.print(new Bug324178().foo(false));\n" +
>+			"    }\n" +
>+			"}\n"
>+		},
>+		"true");
>+}
>+
>+// Bug 324178 - [null] ConditionalExpression.nullStatus(..) doesn't take into account the analysis of condition itself
>+// definite assignment along all true-yielding paths is sufficient
>+public void testBug324178c() {
>+	this.runConformTest(
>+		new String[] {
>+			"Bug324178.java",
>+			"public class Bug324178 {\n" +
>+			"	 boolean foo() {\n" +
>+			"        boolean r=false;" +
>+			"        boolean v;\n" +
>+			"        if ((true && (v = true)) ? true : true && (v = false)) r = v;\n" +
>+			"        return r;\n" +
>+			"    }\n" +
>+			"    public static void main(String[] args) {\n" +
>+			"        System.out.print(new Bug324178().foo());\n" +
>+			"    }\n" +
>+			"}\n"
>+		},
>+		"true");
>+}
>+// Bug 324178 - [null] ConditionalExpression.nullStatus(..) doesn't take into account the analysis of condition itself
>+// must detect that b2 may be uninitialized, no special semantics for Boolean
>+public void testBug324178d() {
>+	if (this.complianceLevel < ClassFileConstants.JDK1_5)
>+		return;
>+	this.runNegativeTest(
>+		new String[] {
>+			"Bug324178.java",
>+			"public class Bug324178 {\n" +
>+			"	 boolean foo(boolean b1) {\n" +
>+			"  		 Boolean b2;\n" + 
>+			"        if (b1 ? (b2 = Boolean.TRUE) : null)\n" + 
>+			"          return b2;\n" +
>+			"        return false;\n" +
>+			"    }\n" +
>+			"    public static void main(String[] args) {\n" +
>+			"        System.out.print(new Bug324178().foo(true));\n" +
>+			"    }\n" +
>+			"}\n"
>+		},
>+		"----------\n" + 
>+		"1. ERROR in Bug324178.java (at line 5)\n" + 
>+		"	return b2;\n" + 
>+		"	       ^^\n" + 
>+		"The local variable b2 may not have been initialized\n" + 
>+		"----------\n");
>+}
> public static Class testClass() {
> 	return InitializationTests.class;
> }
>Index: src/org/eclipse/jdt/core/tests/compiler/regression/NullReferenceTest.java
>===================================================================
>RCS file: /cvsroot/eclipse/org.eclipse.jdt.core.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/NullReferenceTest.java,v
>retrieving revision 1.114
>diff -u -r1.114 NullReferenceTest.java
>--- src/org/eclipse/jdt/core/tests/compiler/regression/NullReferenceTest.java	3 Mar 2011 13:00:33 -0000	1.114
>+++ src/org/eclipse/jdt/core/tests/compiler/regression/NullReferenceTest.java	4 Mar 2011 18:56:05 -0000
>@@ -16,6 +16,7 @@
>  *     						bug 332637 - Dead Code detection removing code that isn't dead
>  *     						bug 338303 - Warning about Redundant assignment conflicts with definite assignment
>  *     						bug 336428 - [compiler][null] bogus warning "redundant null check" in condition of do {} while() loop
>+ * 							bug 324178 - [null] ConditionalExpression.nullStatus(..) doesn't take into account the analysis of condition itself
>  *******************************************************************************/
> package org.eclipse.jdt.core.tests.compiler.regression;
> 
>@@ -43,7 +44,7 @@
> // Only the highest compliance level is run; add the VM argument
> // -Dcompliance=1.4 (for example) to lower it if needed
> static {
>-//		TESTS_NAMES = new String[] { "testBug336428e" };
>+//		TESTS_NAMES = new String[] { "testBug324178" };
> //		TESTS_NUMBERS = new int[] { 561 };
> //		TESTS_RANGE = new int[] { 1, 2049 };
> }
>@@ -14204,4 +14205,39 @@
> 		},
> 		"");
> }
>+// Bug 324178 - [null] ConditionalExpression.nullStatus(..) doesn't take into account the analysis of condition itself
>+public void testBug324178() {
>+	this.runConformTest(
>+		new String[] {
>+			"Bug324178.java",
>+			"public class Bug324178 {\n" +
>+			"    boolean b;\n" + 
>+			"    void foo(Object u) {\n" + 
>+			"    if (u == null) {}\n" + 
>+			"        Object o = (u == null) ? new Object() : u;\n" + 
>+			"        o.toString();   // Incorrect potential NPE\n" + 
>+			"    }\n" + 
>+			"}\n"			
>+		},
>+		"");
>+}
>+
>+// Bug 324178 - [null] ConditionalExpression.nullStatus(..) doesn't take into account the analysis of condition itself
>+public void testBug324178a() {
>+	this.runConformTest(
>+		new String[] {
>+			"Bug324178.java",
>+			"public class Bug324178 {\n" +
>+			"    boolean b;\n" + 
>+			"    void foo(Boolean u) {\n" + 
>+			"    if (u == null) {}\n" + 
>+			"        Boolean o;\n" +
>+			"        o = (u == null) ? Boolean.TRUE : u;\n" + 
>+			"        o.toString();   // Incorrect potential NPE\n" + 
>+			"    }\n" + 
>+			"}\n"			
>+		},
>+		"");
>+}
>+
> }
>\ No newline at end of file

Actions: View | Diff

Attachments on bug 324178: 188486 | 189907 | 190328 | 190428 | 190447