Attachment 209409 Details for Bug 358903 – incremental change 4

[patch] incremental change 4

Bug_358903_v0.9.6-update4.patch (text/plain), 15.90 KB, created by Stephan Herrmann

on 2012-01-12 16:00:44 EST

(hide)

Description:

Filename:

MIME Type:

Creator: Stephan Herrmann

Created: 2012-01-12 16:00:44 EST

Size: 15.90 KB

patch

obsolete

>diff --git a/org.eclipse.jdt.core.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/ResourceLeakTests.java b/org.eclipse.jdt.core.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/ResourceLeakTests.java
>index 9ed8c39..d6aac82 100644
>--- a/org.eclipse.jdt.core.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/ResourceLeakTests.java
>+++ b/org.eclipse.jdt.core.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/ResourceLeakTests.java
>@@ -26,7 +26,7 @@
> public class ResourceLeakTests extends AbstractRegressionTest {
> 
> static {
>-//	TESTS_NAMES = new String[] { "test063e"};
>+//	TESTS_NAMES = new String[] { "test056q"};
> //	TESTS_NUMBERS = new int[] { 50 };
> //	TESTS_RANGE = new int[] { 11, -1 };
> }
>@@ -2392,7 +2392,7 @@
> 		options);
> }
> // Bug 358903 - Filter practically unimportant resource leak warnings
>-// a resource wrapper does not wrap a provided resource
>+// a resource wrapper does not wrap any provided resource
> public void test061n() {
> 	Map options = getCompilerOptions();
> 	options.put(CompilerOptions.OPTION_ReportUnclosedCloseable, CompilerOptions.ERROR);
>@@ -2419,6 +2419,41 @@
> 		true,
> 		options);
> }
>+// Bug 358903 - Filter practically unimportant resource leak warnings
>+// a resource wrapper is closed only in its local block, underlying resource may leak
>+public void test061o() {
>+	Map options = getCompilerOptions();
>+	options.put(CompilerOptions.OPTION_ReportUnclosedCloseable, CompilerOptions.ERROR);
>+	options.put(CompilerOptions.OPTION_ReportPotentiallyUnclosedCloseable, CompilerOptions.ERROR);
>+	this.runNegativeTest(
>+		new String[] {
>+			"X.java",
>+			"import java.io.File;\n" +
>+			"import java.io.FileInputStream;\n" +
>+			"import java.io.BufferedInputStream;\n" +
>+			"import java.io.IOException;\n" +
>+			"public class X {\n" +
>+			"    void foo(boolean bar) throws IOException {\n" + 
>+			"        File file = new File(\"somefil\");\n" + 
>+			"        FileInputStream fileStream  = new FileInputStream(file);\n" + 
>+			"        BufferedInputStream bis = new BufferedInputStream(fileStream);   \n" + 
>+			"        if (bar) {\n" + 
>+			"            BufferedInputStream doubleWrap = new BufferedInputStream(bis);\n" + 
>+			"            doubleWrap.close();\n" + 
>+			"        }\n" + 
>+			"    }\n" + 
>+			"}\n"
>+		},
>+		"----------\n" +
>+		"1. ERROR in X.java (at line 9)\n" +
>+		"	BufferedInputStream bis = new BufferedInputStream(fileStream);   \n" +
>+		"	                    ^^^\n" +
>+		"Potential resource leak: \'bis\' may not be closed\n" +
>+		"----------\n",
>+		null,
>+		true,
>+		options);
>+}
> // Bug 362331 - Resource leak not detected when closeable not assigned to variable
> // a resource is never assigned
> public void test062a() throws IOException {
>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 4ec7620..05cc6b7 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
>@@ -41,27 +41,33 @@
> 
> 	// a call to close() was seen at least on one path:
> 	private static final int CLOSE_SEEN = 1;
>-	// the resource was passed to outside code (arg in method/ctor call or as a return value from this method):
>-	private static final int PASSED_TO_OUTSIDE = 2;
>-	// the resource was obtained from the outside (argument of the current method, or via a field read)
>-	private static final int OBTAINED_FROM_OUTSIDE = 4;
>+	// the resource is shared with outside code either by
>+	// - passing as an arg in a method call or
>+	// - obtaining this from a method call or array reference
>+	// Interpret that we may or may not be responsible for closing
>+	private static final int SHARED_WITH_OUTSIDE = 2;
>+	// the resource is likely owned by outside code (owner has responsibility to close): 
>+	// - obtained as argument of the current method, or via a field read
>+	// - returned as the result of this method 
>+	private static final int OWNED_BY_OUTSIDE = 4;
> 	// If close() is invoked from a nested method (inside a local type) report remaining problems only as potential:
> 	private static final int CLOSED_IN_NESTED_METHOD = 8;
>-	// a location independent issue has been reported already against this resource:
>-	private static final int REPORTED = 16;
>-	// a resource is wrapped in another resource:
>-	private static final int WRAPPED = 32;
>-
>-	private static final int DOUBT_MASK = CLOSE_SEEN | PASSED_TO_OUTSIDE | OBTAINED_FROM_OUTSIDE | CLOSED_IN_NESTED_METHOD | REPORTED; // not WRAPPED
>+	// explicit closing has been reported already against this resource:
>+	private static final int REPORTED_EXPLICIT_CLOSE = 16;
>+	// a location independent potential problem has been reported against this resource:
>+	private static final int REPORTED_POTENTIAL_LEAK = 32;
>+	// a location independent definitive problem has been reported against this resource:
>+	private static final int REPORTED_DEFINITIVE_LEAK = 64;
> 
> 	/**
>-	 * Bitset of {@link #CLOSE_SEEN}, {@link #PASSED_TO_OUTSIDE}, {@link #OBTAINED_FROM_OUTSIDE}, {@link #CLOSED_IN_NESTED_METHOD}, {@link #REPORTED} and {@link #WRAPPED}.
>+	 * Bitset of {@link #CLOSE_SEEN}, {@link #SHARED_WITH_OUTSIDE}, {@link #OWNED_BY_OUTSIDE}, {@link #CLOSED_IN_NESTED_METHOD}, {@link #REPORTED_EXPLICIT_CLOSE}, {@link #REPORTED_POTENTIAL_LEAK} and {@link #REPORTED_DEFINITIVE_LEAK}.
> 	 */
> 	private int globalClosingState = 0;
> 
> 	public LocalVariableBinding originalBinding; // the real local being tracked, can be null for preliminary track vars for allocation expressions
> 	
> 	public FakedTrackingVariable innerTracker; // chained tracking variable of a chained (wrapped) resource
>+	public FakedTrackingVariable outerTracker; // inverse of 'innerTracker'
> 
> 	MethodScope methodScope; // designates the method declaring this variable
> 
>@@ -139,7 +145,7 @@
> 				Statement location = local.declaration;
> 				local.closeTracker = new FakedTrackingVariable(local, location);
> 				if (local.isParameter()) {
>-					local.closeTracker.globalClosingState |= OBTAINED_FROM_OUTSIDE;
>+					local.closeTracker.globalClosingState |= OWNED_BY_OUTSIDE;
> 					// status of this tracker is now UNKNOWN
> 				}
> 				return local.closeTracker;
>@@ -169,6 +175,9 @@
> 			if (closeTracker == null) {
> 				if (rhs.resolvedType != TypeBinding.NULL) { // not NULL means valid closeable as per method precondition
> 					closeTracker = new FakedTrackingVariable(local, location);
>+					if (local.isParameter()) {
>+						closeTracker.globalClosingState |= OWNED_BY_OUTSIDE;
>+					}
> 				}					
> 			}
> 			if (closeTracker != null) {
>@@ -197,12 +206,28 @@
> 				if (innerTracker != null) {
> 					if (innerTracker == allocation.closeTracker)
> 						return; // self wrap (res = new Res(res)) -> neither change (here) nor remove (below)
>+					int newStatus = FlowInfo.NULL;
> 					if (allocation.closeTracker == null) {
> 						allocation.closeTracker = new FakedTrackingVariable(scope, allocation); // no local available, closeable is unassigned
>+					} else {
>+						if (scope.finallyInfo != null) {
>+							// inject results from analysing a finally block onto the newly connected wrapper
>+							int finallyStatus = scope.finallyInfo.nullStatus(allocation.closeTracker.binding);
>+							if (finallyStatus != FlowInfo.UNKNOWN)
>+								newStatus = finallyStatus;
>+						}
> 					}
> 					allocation.closeTracker.innerTracker = innerTracker;
>-					innerTracker.globalClosingState |= WRAPPED;
>-					flowInfo.markAsDefinitelyNull(allocation.closeTracker.binding);
>+					innerTracker.outerTracker = allocation.closeTracker;
>+					flowInfo.markNullStatus(allocation.closeTracker.binding, newStatus);
>+					if (newStatus != FlowInfo.NULL) {
>+						// propagate results from a finally block also into nested resources:
>+						FakedTrackingVariable currentTracker = innerTracker;
>+						while (currentTracker != null) {
>+							flowInfo.markNullStatus(currentTracker.binding, newStatus);
>+							currentTracker = currentTracker.innerTracker;
>+						}
>+					}
> 					return; // keep chaining wrapper
> 				}
> 			}
>@@ -216,6 +241,7 @@
> 			if (presetTracker != null && presetTracker.originalBinding != null) {
> 				int closeStatus = flowInfo.nullStatus(presetTracker.binding);
> 				if (closeStatus != FlowInfo.NON_NULL
>+						&& closeStatus != FlowInfo.UNKNOWN
> 						&& !flowInfo.isDefinitelyNull(presetTracker.originalBinding)
> 						&& !(presetTracker.currentAssignment instanceof LocalDeclaration))
> 					allocation.closeTracker.recordErrorLocation(presetTracker.currentAssignment, closeStatus);
>@@ -291,7 +317,7 @@
> 				// keep close-status of RHS unchanged across this assignment
> 			} else if (previousTracker != null) {					// 2. re-use tracking variable from the LHS?
> 				// re-assigning from a fresh value, mark as not-closed again:
>-				if ((previousTracker.globalClosingState & (PASSED_TO_OUTSIDE|OBTAINED_FROM_OUTSIDE)) == 0)
>+				if ((previousTracker.globalClosingState & (SHARED_WITH_OUTSIDE|OWNED_BY_OUTSIDE)) == 0)
> 					flowInfo.markAsDefinitelyNull(previousTracker.binding);
> 				local.closeTracker = analyseCloseableExpression(flowInfo, local, location, rhs, previousTracker);
> 			} else {												// 3. no re-use, create a fresh tracking variable:
>@@ -299,7 +325,7 @@
> 				if (rhsTrackVar != null) {
> 					local.closeTracker = rhsTrackVar;
> 					// a fresh resource, mark as not-closed:
>-					if ((rhsTrackVar.globalClosingState & (PASSED_TO_OUTSIDE|OBTAINED_FROM_OUTSIDE)) == 0)
>+					if ((rhsTrackVar.globalClosingState & (SHARED_WITH_OUTSIDE|OWNED_BY_OUTSIDE)) == 0)
> 						flowInfo.markAsDefinitelyNull(rhsTrackVar.binding);
> // TODO(stephan): this might be useful, but I could not find a test case for it: 
> //					if (flowContext.initsOnFinally != null)
>@@ -308,8 +334,11 @@
> 			}
> 		}
> 
>-		if (disconnectedTracker != null)
>-			disconnectedTracker.recordErrorLocation(location, upstreamInfo.nullStatus(disconnectedTracker.binding));
>+		if (disconnectedTracker != null) {
>+			int upstreamStatus = upstreamInfo.nullStatus(disconnectedTracker.binding);
>+			if (upstreamStatus != FlowInfo.NON_NULL)
>+				disconnectedTracker.recordErrorLocation(location, upstreamStatus);
>+		}
> 	}
> 	/**
> 	 * Analyze structure of a closeable expression, matching (chained) resources against our white lists.
>@@ -348,14 +377,14 @@
> 		{
> 			// we *might* be responsible for the resource obtained
> 			FakedTrackingVariable tracker = new FakedTrackingVariable(local, location);
>-			tracker.globalClosingState |= PASSED_TO_OUTSIDE;
>+			tracker.globalClosingState |= SHARED_WITH_OUTSIDE;
> 			flowInfo.markPotentiallyNullBit(tracker.binding); // shed some doubt
> 			return tracker;			
> 		} else if ((expression.bits & RestrictiveFlagMASK) == Binding.FIELD) 
> 		{
> 			// responsibility for this resource probably lies at a higher level
> 			FakedTrackingVariable tracker = new FakedTrackingVariable(local, location);
>-			tracker.globalClosingState |= OBTAINED_FROM_OUTSIDE;
>+			tracker.globalClosingState |= OWNED_BY_OUTSIDE;
> 			// leave state as UNKNOWN, the bit OBTAINED_FROM_OUTSIDE will prevent spurious warnings
> 			return tracker;			
> 		}
>@@ -477,11 +506,15 @@
> 
> 	/** Mark that this resource is closed locally. */
> 	public void markClose(FlowInfo flowInfo, FlowContext flowContext) {
>-		flowInfo.markAsDefinitelyNonNull(this.binding);
>-		this.globalClosingState |= CLOSE_SEEN;
>+		FakedTrackingVariable current = this;
>+		while (current != null) {
>+			flowInfo.markAsDefinitelyNonNull(current.binding);
>+			current.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);
>+//			if (flowContext.initsOnFinally != null)
>+//				flowContext.initsOnFinally.markAsDefinitelyNonNull(this.binding);
>+			current = current.innerTracker;
>+		}
> 	}
> 
> 	/** Mark that this resource is closed from a nested method (inside a local class). */
>@@ -503,7 +536,7 @@
> 				scope.removeTrackingVar(trackVar);
> 				return flowInfo;
> 			}
>-			trackVar.globalClosingState |= PASSED_TO_OUTSIDE;
>+			trackVar.globalClosingState |= SHARED_WITH_OUTSIDE;
> 			if (scope.methodScope() != trackVar.methodScope)
> 				trackVar.globalClosingState |= CLOSED_IN_NESTED_METHOD;
> 			// insert info that the tracked resource *may* be closed (by the target method, i.e.)
>@@ -522,7 +555,7 @@
> 
> 	public boolean reportRecordedErrors(Scope scope) {
> 		FakedTrackingVariable current = this;
>-		while ((current.globalClosingState & DOUBT_MASK) == 0) {
>+		while (current.globalClosingState == 0) {
> 			current = current.innerTracker;
> 			if (current == null) {
> 				// no relevant state found -> report:
>@@ -543,21 +576,38 @@
> 	}
> 	
> 	public void reportError(ProblemReporter problemReporter, ASTNode location, int nullStatus) {
>-		if ((this.globalClosingState & WRAPPED) != 0)
>-			return;
>+		// which degree of problem?
>+		boolean isPotentialProblem = false;
> 		if (nullStatus == FlowInfo.NULL) {
> 			if ((this.globalClosingState & CLOSED_IN_NESTED_METHOD) != 0)
>-				problemReporter.potentiallyUnclosedCloseable(this, location);
>-			else
>-				problemReporter.unclosedCloseable(this, location);
>+				isPotentialProblem = true;
> 		} else if (nullStatus == FlowInfo.POTENTIALLY_NULL) {
>-			problemReporter.potentiallyUnclosedCloseable(this, location);
>+			isPotentialProblem = true;
>+		}
>+		// report:
>+		if (isPotentialProblem) {
>+			if ((this.globalClosingState & (REPORTED_POTENTIAL_LEAK|REPORTED_DEFINITIVE_LEAK)) != 0)
>+				return;
>+			problemReporter.potentiallyUnclosedCloseable(this, location);	
>+		} else {
>+			if ((this.globalClosingState & (REPORTED_DEFINITIVE_LEAK)) != 0)
>+				return;
>+			problemReporter.unclosedCloseable(this, location);			
>+		}
>+		// propagate flag to inners:
>+		if (location == null) {
>+			int reportFlag = isPotentialProblem ? REPORTED_POTENTIAL_LEAK : REPORTED_DEFINITIVE_LEAK;
>+			FakedTrackingVariable current = this;
>+			while (current != null) {
>+				current.globalClosingState |= reportFlag;
>+				current = current.innerTracker;
>+			}
> 		}
> 	}
> 
> 	public void reportExplicitClosing(ProblemReporter problemReporter) {
>-		if ((this.globalClosingState & (OBTAINED_FROM_OUTSIDE|REPORTED)) == 0) { // can't use t-w-r for OBTAINED_FROM_OUTSIDE
>-			this.globalClosingState |= REPORTED;
>+		if ((this.globalClosingState & (OWNED_BY_OUTSIDE|REPORTED_EXPLICIT_CLOSE)) == 0) { // can't use t-w-r for OWNED_BY_OUTSIDE
>+			this.globalClosingState |= REPORTED_EXPLICIT_CLOSE;
> 			problemReporter.explicitlyClosedAutoCloseable(this);
> 		}
> 	}
>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 6804f89..7062a9c 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
>@@ -15,7 +15,9 @@
> package org.eclipse.jdt.internal.compiler.lookup;
> 
> import java.util.ArrayList;
>+import java.util.HashSet;
> import java.util.List;
>+import java.util.Set;
> 
> import org.eclipse.jdt.core.compiler.CharOperation;
> import org.eclipse.jdt.internal.compiler.ast.*;
>@@ -1003,9 +1005,14 @@
> 		return;
> 	}
> 	if (location != null && flowInfo.reachMode() != 0) return;
>-	int trackVarsLen = this.trackingVariables.size();
>-	for (int i=0; i<trackVarsLen; i++) {
>-		FakedTrackingVariable trackingVar = (FakedTrackingVariable) this.trackingVariables.get(i);
>+	Set varSet = new HashSet(this.trackingVariables);
>+	while (!varSet.isEmpty()) {
>+		// pick one outer-most variable from the set
>+		FakedTrackingVariable trackingVar = (FakedTrackingVariable) varSet.iterator().next();
>+		while (trackingVar.outerTracker != null && varSet.contains(trackingVar.outerTracker)) {
>+			trackingVar = trackingVar.outerTracker;
>+		}
>+		varSet.remove(trackingVar);
> 		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
>

Actions: View | Diff

Attachments on bug 358903: 208899 | 209082 | 209107 | 209169 | 209179 | 209232 | 209234 | 209276 | 209373 | 209374 | 209384 | 209409 | 209457 | 209460 | 209464 | 209472 | 209482