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