Attachment 204421 Details for Bug 359334 – additional fix improved

[patch] additional fix improved

Bug_359334_4.patch (text/plain), 18.79 KB, created by Stephan Herrmann

on 2011-10-01 14:30:28 EDT

(hide)

Description:

Filename:

MIME Type:

Creator: Stephan Herrmann

Created: 2011-10-01 14:30:28 EDT

Size: 18.79 KB

patch

obsolete

>diff --git a/org.eclipse.jdt.core.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/TryWithResourcesStatementTest.java b/org.eclipse.jdt.core.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/TryWithResourcesStatementTest.java
>index 542911f..07ea7f0 100644
>--- a/org.eclipse.jdt.core.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/TryWithResourcesStatementTest.java
>+++ b/org.eclipse.jdt.core.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/TryWithResourcesStatementTest.java
>@@ -23,7 +23,7 @@
> public class TryWithResourcesStatementTest extends AbstractRegressionTest {
> 
> static {
>-//	TESTS_NAMES = new String[] { "test056zz"};
>+//	TESTS_NAMES = new String[] { "test056throw"};
> //	TESTS_NUMBERS = new int[] { 50 };
> //	TESTS_RANGE = new int[] { 11, -1 };
> }
>@@ -4944,6 +4944,7 @@
> 		options);
> }
> // Bug 359334 - Analysis for resource leak warnings does not consider exceptions as method exit points
>+// explicit throw is a true method exit here
> public void test056throw1() {
> 	Map options = getCompilerOptions();
> 	options.put(JavaCore.COMPILER_PB_UNCLOSED_CLOSEABLE, CompilerOptions.ERROR);
>@@ -4982,6 +4983,199 @@
> 		true,
> 		options);	
> }
>+// Bug 359334 - Analysis for resource leak warnings does not consider exceptions as method exit points
>+// close() within finally provides protection for throw
>+public void test056throw2() {
>+	Map options = getCompilerOptions();
>+	options.put(JavaCore.COMPILER_PB_UNCLOSED_CLOSEABLE, CompilerOptions.ERROR);
>+	options.put(JavaCore.COMPILER_PB_POTENTIALLY_UNCLOSED_CLOSEABLE, CompilerOptions.ERROR);
>+	options.put(JavaCore.COMPILER_PB_EXPLICITLY_CLOSED_AUTOCLOSEABLE, CompilerOptions.ERROR);
>+	options.put(JavaCore.COMPILER_PB_DEAD_CODE, CompilerOptions.ERROR);
>+	this.runNegativeTest(
>+		new String[] {
>+			"X.java",
>+			"import java.io.FileReader;\n" +
>+			"public class X {\n" +
>+			"    void foo1() throws Exception {\n" + 
>+			"        FileReader reader = new FileReader(\"file\"); // propose t-w-r\n" + 
>+			"        try {\n" + 
>+			"            reader.read();\n" + 
>+			"            return;\n" + 
>+			"        } catch (Exception e) {\n" + 
>+			"            throw new Exception();\n" + 
>+			"        } finally {\n" + 
>+			"            reader.close();\n" + 
>+			"        }\n" + 
>+			"    }\n" + 
>+			"\n" + 
>+			"    void foo2() throws Exception {\n" + 
>+			"        FileReader reader = new FileReader(\"file\"); // propose t-w-r\n" + 
>+			"        try {\n" + 
>+			"            reader.read();\n" + 
>+			"            throw new Exception(); // should not warn here\n" + 
>+			"        } catch (Exception e) {\n" + 
>+			"            throw new Exception();\n" + 
>+			"        } finally {\n" + 
>+			"            reader.close();\n" + 
>+			"        }\n" + 
>+			"    }\n" + 
>+			"\n" + 
>+			"    void foo3() throws Exception {\n" + 
>+			"        FileReader reader = new FileReader(\"file\"); // propose t-w-r\n" + 
>+			"        try {\n" + 
>+			"            reader.read();\n" + 
>+			"            throw new Exception();\n" + 
>+			"        } finally {\n" + 
>+			"            reader.close();\n" + 
>+			"        }\n" + 
>+			"    }\n" + 
>+			"}\n"
>+		},
>+		"----------\n" +
>+		"1. ERROR in X.java (at line 4)\n" +
>+		"	FileReader reader = new FileReader(\"file\"); // propose t-w-r\n" +
>+		"	           ^^^^^^\n" +
>+		"Resource \'reader\' should be managed by try-with-resource\n" +
>+		"----------\n" +
>+		"2. ERROR in X.java (at line 16)\n" +
>+		"	FileReader reader = new FileReader(\"file\"); // propose t-w-r\n" +
>+		"	           ^^^^^^\n" +
>+		"Resource \'reader\' should be managed by try-with-resource\n" +
>+		"----------\n" +
>+		"3. ERROR in X.java (at line 28)\n" +
>+		"	FileReader reader = new FileReader(\"file\"); // propose t-w-r\n" +
>+		"	           ^^^^^^\n" +
>+		"Resource \'reader\' should be managed by try-with-resource\n" +
>+		"----------\n",
>+		null,
>+		true,
>+		options);	
>+}
>+// Bug 359334 - Analysis for resource leak warnings does not consider exceptions as method exit points
>+// close() nested within finally provides protection for throw
>+public void test056throw3() {
>+	Map options = getCompilerOptions();
>+	options.put(JavaCore.COMPILER_PB_UNCLOSED_CLOSEABLE, CompilerOptions.ERROR);
>+	options.put(JavaCore.COMPILER_PB_POTENTIALLY_UNCLOSED_CLOSEABLE, CompilerOptions.ERROR);
>+	options.put(JavaCore.COMPILER_PB_EXPLICITLY_CLOSED_AUTOCLOSEABLE, CompilerOptions.ERROR);
>+	options.put(JavaCore.COMPILER_PB_DEAD_CODE, CompilerOptions.ERROR);
>+	this.runNegativeTest(
>+		new String[] {
>+			"X.java",
>+			"import java.io.FileReader;\n" +
>+			"public class X {\n" +
>+			"    void foo2x() throws Exception {\n" + 
>+			"        FileReader reader = new FileReader(\"file\"); // propose t-w-r\n" + 
>+			"        try {\n" + 
>+			"            reader.read();\n" + 
>+			"            throw new Exception(); // should not warn here\n" + 
>+			"        } catch (Exception e) {\n" + 
>+			"            throw new Exception();\n" + 
>+			"        } finally {\n" +
>+			"            if (reader != null)\n" +
>+			"                 try {\n" + 
>+			"                     reader.close();\n" +
>+			"                 } catch (java.io.IOException io) {}\n" + 
>+			"        }\n" + 
>+			"    }\n" + 
>+			"}\n"
>+		},
>+		"----------\n" + 
>+		"1. ERROR in X.java (at line 4)\n" + 
>+		"	FileReader reader = new FileReader(\"file\"); // propose t-w-r\n" + 
>+		"	           ^^^^^^\n" + 
>+		"Resource \'reader\' should be managed by try-with-resource\n" + 
>+		"----------\n",
>+		null,
>+		true,
>+		options);	
>+}
>+// Bug 359334 - Analysis for resource leak warnings does not consider exceptions as method exit points
>+// additional boolean should shed doubt on whether we reach the close() call
>+public void test056throw4() {
>+	Map options = getCompilerOptions();
>+	options.put(JavaCore.COMPILER_PB_UNCLOSED_CLOSEABLE, CompilerOptions.ERROR);
>+	options.put(JavaCore.COMPILER_PB_POTENTIALLY_UNCLOSED_CLOSEABLE, CompilerOptions.ERROR);
>+	options.put(JavaCore.COMPILER_PB_EXPLICITLY_CLOSED_AUTOCLOSEABLE, CompilerOptions.ERROR);
>+	options.put(JavaCore.COMPILER_PB_DEAD_CODE, CompilerOptions.ERROR);
>+	this.runNegativeTest(
>+		new String[] {
>+			"X.java",
>+			"import java.io.FileReader;\n" +
>+			"public class X {\n" +
>+			"    void foo2x(boolean b) throws Exception {\n" + 
>+			"        FileReader reader = new FileReader(\"file\");\n" + 
>+			"        try {\n" + 
>+			"            reader.read();\n" + 
>+			"            throw new Exception(); // should warn here\n" + 
>+			"        } catch (Exception e) {\n" + 
>+			"            throw new Exception(); // should warn here\n" + 
>+			"        } finally {\n" +
>+			"            if (reader != null && b)\n" + // this condition is too strong to protect reader
>+			"                 try {\n" + 
>+			"                     reader.close();\n" +
>+			"                 } catch (java.io.IOException io) {}\n" + 
>+			"        }\n" + 
>+			"    }\n" + 
>+			"}\n"
>+		},
>+		"----------\n" +
>+		"1. ERROR in X.java (at line 7)\n" +
>+		"	throw new Exception(); // should warn here\n" +
>+		"	^^^^^^^^^^^^^^^^^^^^^^\n" +
>+		"Potential resource leak: \'reader\' may not be closed at this location\n" +
>+		"----------\n" +
>+		"2. ERROR in X.java (at line 9)\n" +
>+		"	throw new Exception(); // should warn here\n" +
>+		"	^^^^^^^^^^^^^^^^^^^^^^\n" +
>+		"Potential resource leak: \'reader\' may not be closed at this location\n" +
>+		"----------\n",
>+		null,
>+		true,
>+		options);	
>+}
>+// Bug 359334 - Analysis for resource leak warnings does not consider exceptions as method exit points
>+// similar to test056throw3() but indirectly calling close(), so doubts remain.
>+public void test056throw5() {
>+	Map options = getCompilerOptions();
>+	options.put(JavaCore.COMPILER_PB_UNCLOSED_CLOSEABLE, CompilerOptions.ERROR);
>+	options.put(JavaCore.COMPILER_PB_POTENTIALLY_UNCLOSED_CLOSEABLE, CompilerOptions.ERROR);
>+	options.put(JavaCore.COMPILER_PB_EXPLICITLY_CLOSED_AUTOCLOSEABLE, CompilerOptions.ERROR);
>+	options.put(JavaCore.COMPILER_PB_DEAD_CODE, CompilerOptions.ERROR);
>+	this.runNegativeTest(
>+		new String[] {
>+			"X.java",
>+			"import java.io.FileReader;\n" +
>+			"public class X {\n" +
>+			"    void foo2x() throws Exception {\n" +
>+			"        FileReader reader = new FileReader(\"file\");\n" +
>+			"        try {\n" +
>+			"            reader.read();\n" +
>+			"            throw new Exception(); // should warn 'may not' here\n" +
>+			"        } catch (Exception e) {\n" +
>+			"            throw new Exception(); // should warn 'may not' here\n" +
>+			"        } finally {\n" +
>+			"            doClose(reader);\n" +
>+			"        }\n" +
>+			"    }\n" +
>+			"    void doClose(FileReader r) { try { r.close(); } catch (java.io.IOException ex) {}}\n" + 
>+			"}\n"
>+		},
>+		"----------\n" +
>+		"1. ERROR in X.java (at line 7)\n" +
>+		"	throw new Exception(); // should warn \'may not\' here\n" +
>+		"	^^^^^^^^^^^^^^^^^^^^^^\n" +
>+		"Potential resource leak: \'reader\' may not be closed at this location\n" +
>+		"----------\n" +
>+		"2. ERROR in X.java (at line 9)\n" +
>+		"	throw new Exception(); // should warn \'may not\' here\n" +
>+		"	^^^^^^^^^^^^^^^^^^^^^^\n" +
>+		"Potential resource leak: \'reader\' may not be closed at this location\n" +
>+		"----------\n",
>+		null,
>+		true,
>+		options);	
>+}
> public static Class testClass() {
> 	return TryWithResourcesStatementTest.class;
> }
>diff --git a/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/ast/Block.java b/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/ast/Block.java
>index 6a80298..90b2f96 100644
>--- a/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/ast/Block.java
>+++ b/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/ast/Block.java
>@@ -38,7 +38,7 @@
> 		}
> 	}
> 	if (this.explicitDeclarations > 0) // if block has its own scope analyze tracking vars now:
>-		this.scope.checkUnclosedCloseables(flowInfo, null);
>+		this.scope.checkUnclosedCloseables(flowInfo, null, null);
> 	return flowInfo;
> }
> /**
>diff --git a/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/ast/FakedTrackingVariable.java b/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/ast/FakedTrackingVariable.java
>index 10bebc4..ee13047 100644
>--- a/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/ast/FakedTrackingVariable.java
>+++ b/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/ast/FakedTrackingVariable.java
>@@ -167,7 +167,7 @@
> 		this.globalClosingState |= CLOSE_SEEN;
> //TODO(stephan): this might be useful, but I could not find a test case for it: 
> //		if (flowContext.initsOnFinally != null)
>-//			flowContext.initsOnFinally.markAsDefinitelyNonNull(this.binding);		
>+//			flowContext.initsOnFinally.markAsDefinitelyNonNull(this.binding);
> 	}
> 
> 	/** Mark that this resource is closed from a nested method (inside a local class). */
>diff --git a/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/ast/MethodDeclaration.java b/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/ast/MethodDeclaration.java
>index 5fa2895..5b60ba3 100644
>--- a/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/ast/MethodDeclaration.java
>+++ b/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/ast/MethodDeclaration.java
>@@ -135,7 +135,7 @@
> 				}
> 					
> 			}
>-			this.scope.checkUnclosedCloseables(flowInfo, null/*don't report against a specific location*/);
>+			this.scope.checkUnclosedCloseables(flowInfo, null/*don't report against a specific location*/, null);
> 		} catch (AbortMethod e) {
> 			this.ignoreFurtherInvestigation = true;
> 		}
>diff --git a/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/ast/ReturnStatement.java b/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/ast/ReturnStatement.java
>index c1f3c4d..ca1e277 100644
>--- a/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/ast/ReturnStatement.java
>+++ b/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/ast/ReturnStatement.java
>@@ -114,7 +114,7 @@
> 			this.expression.bits |= ASTNode.IsReturnedValue;
> 		}
> 	}
>-	currentScope.checkUnclosedCloseables(flowInfo, this);
>+	currentScope.checkUnclosedCloseables(flowInfo, this, currentScope);
> 	return FlowInfo.DEAD_END;
> }
> 
>diff --git a/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/ast/Statement.java b/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/ast/Statement.java
>index a6c1b75..99d0fcb 100644
>--- a/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/ast/Statement.java
>+++ b/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/ast/Statement.java
>@@ -80,14 +80,14 @@
> 			if (previousComplaintLevel < COMPLAINED_UNREACHABLE) {
> 				scope.problemReporter().unreachableCode(this);
> 				if (endOfBlock)
>-					scope.checkUnclosedCloseables(flowInfo, null);
>+					scope.checkUnclosedCloseables(flowInfo, null, null);
> 			}
> 			return COMPLAINED_UNREACHABLE;
> 		} else {
> 			if (previousComplaintLevel < COMPLAINED_FAKE_REACHABLE) {
> 				scope.problemReporter().fakeReachable(this);
> 				if (endOfBlock)
>-					scope.checkUnclosedCloseables(flowInfo, null);
>+					scope.checkUnclosedCloseables(flowInfo, null, null);
> 			}
> 			return COMPLAINED_FAKE_REACHABLE;
> 		}
>diff --git a/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/ast/ThrowStatement.java b/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/ast/ThrowStatement.java
>index 1d83852..7c06835 100644
>--- a/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/ast/ThrowStatement.java
>+++ b/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/ast/ThrowStatement.java
>@@ -36,7 +36,7 @@
> 	this.exception.checkNPE(currentScope, flowContext, flowInfo);
> 	// need to check that exception thrown is actually caught somewhere
> 	flowContext.checkExceptionHandlers(this.exceptionType, this, flowInfo, currentScope);
>-	currentScope.checkUnclosedCloseables(flowInfo, this);
>+	currentScope.checkUnclosedCloseables(flowInfo, this, currentScope);
> 	return FlowInfo.DEAD_END;
> }
> 
>diff --git a/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/ast/TryStatement.java b/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/ast/TryStatement.java
>index a23c2b7..25cc8a8 100644
>--- a/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/ast/TryStatement.java
>+++ b/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/ast/TryStatement.java
>@@ -238,6 +238,14 @@
> 		if (subInfo == FlowInfo.DEAD_END) {
> 			this.bits |= ASTNode.IsSubRoutineEscaping;
> 			this.scope.problemReporter().finallyMustCompleteNormally(this.finallyBlock);
>+		} else {
>+			// for resource analysis we need the finallyInfo in these nested scopes:
>+			FlowInfo finallyInfo = subInfo.copy();
>+			this.tryBlock.scope.finallyInfo = finallyInfo;
>+			if (this.catchBlocks != null) {
>+				for (int i = 0; i < this.catchBlocks.length; i++)
>+					this.catchBlocks[i].scope.finallyInfo = finallyInfo;
>+			}
> 		}
> 		this.subRoutineInits = subInfo;
> 		// process the try block in a context handling the local exceptions.
>diff --git a/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/lookup/BlockScope.java b/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/lookup/BlockScope.java
>index f4e2ae3..67a27e8 100644
>--- a/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/lookup/BlockScope.java
>+++ b/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/lookup/BlockScope.java
>@@ -964,6 +964,8 @@
> }
> 
> private List trackingVariables; // can be null if no resources are tracked
>+/** Used only during analyseCode and only for checking if a resource was closed in a finallyBlock. */
>+public FlowInfo finallyInfo;
> /**
>  * Register a tracking variable and compute its id.
>  */
>@@ -975,7 +977,7 @@
> 	return outerMethodScope.analysisIndex + (outerMethodScope.trackVarCount++);
> 	
> }
>-/** When no longer interested in this tracking variable remove it. */
>+/** When are no longer interested in this tracking variable - remove it. */
> public void removeTrackingVar(FakedTrackingVariable trackingVariable) {
> 	if (this.trackingVariables != null)
> 		if (this.trackingVariables.remove(trackingVariable))
>@@ -987,11 +989,11 @@
>  * At the end of a block check the closing-status of all tracked closeables that are declared in this block.
>  * Also invoked when entering unreachable code.
>  */
>-public void checkUnclosedCloseables(FlowInfo flowInfo, ASTNode location) {
>+public void checkUnclosedCloseables(FlowInfo flowInfo, ASTNode location, BlockScope locationScope) {
> 	if (this.trackingVariables == null) {
> 		// at a method return we also consider enclosing scopes
> 		if (location != null && this.parent instanceof BlockScope)
>-			((BlockScope) this.parent).checkUnclosedCloseables(flowInfo, location);
>+			((BlockScope) this.parent).checkUnclosedCloseables(flowInfo, location, locationScope);
> 		return;
> 	}
> 	if (location != null && flowInfo.reachMode() != 0) return;
>@@ -1000,14 +1002,12 @@
> 		if (location != null && trackingVar.originalBinding != null && flowInfo.isDefinitelyNull(trackingVar.originalBinding))
> 			continue; // reporting against a specific location, resource is null at this flow, don't complain
> 		int status = getNullStatusAggressively(trackingVar.binding, flowInfo);
>+		// try to improve info if a close() inside finally was observed:
>+		if (locationScope != null) // only check at method exit points
>+			status = locationScope.mergeCloseStatus(status, trackingVar.binding, this);
> 		if (status == FlowInfo.NULL) {
> 			// definitely unclosed: highest priority
> 			reportResourceLeak(trackingVar, location, status);
>-			if (location == null) {
>-				// definitely done with this trackingVar, remove it
>-				this.trackingVariables.remove(trackingVar);
>-				i--; // ... but don't disturb the enclosing loop.
>-			}
> 			continue;
> 		}
> 		if (location == null) // at end of block and not definitely unclosed
>@@ -1025,7 +1025,32 @@
> 				trackingVar.reportExplicitClosing(problemReporter());
> 		}
> 	}
>+	if (location == null) {
>+		// when leaving this block dispose off all tracking variables:
>+		for (int i=0; i<this.localIndex; i++)
>+			this.locals[i].closeTracker = null;		
>+		this.trackingVariables = null;
>+	}
> }
>+
>+private int mergeCloseStatus(int status, LocalVariableBinding binding, BlockScope outerScope) {
>+	// get the most suitable null status representing whether resource 'binding' has been closed
>+	// start at this scope and potentially travel out until 'outerScope'
>+	// at each scope consult any recorded 'finallyInfo'.
>+	if (status != FlowInfo.NON_NULL) {
>+		if (this.finallyInfo != null) {
>+			int finallyStatus = this.finallyInfo.nullStatus(binding);
>+			if (finallyStatus == FlowInfo.NON_NULL)
>+				return finallyStatus;
>+			if (finallyStatus != FlowInfo.NULL) // neither is NON_NULL, but not both are NULL => call it POTENTIALLY_NULL
>+				status = FlowInfo.POTENTIALLY_NULL;
>+		}
>+		if (this != outerScope && this.parent instanceof BlockScope)
>+			return ((BlockScope) this.parent).mergeCloseStatus(status, binding, outerScope);
>+	}
>+	return status;
>+}
>+
> private void reportResourceLeak(FakedTrackingVariable trackingVar, ASTNode location, int nullStatus) {
> 	if (location != null)
> 		trackingVar.recordErrorLocation(location, nullStatus);

Actions: View | Diff

Attachments on bug 359334: 204331 | 204333 | 204420 | 204421