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 d6aac82..1dfb83c 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 @@ import org.eclipse.jdt.internal.compiler.impl.CompilerOptions; public class ResourceLeakTests extends AbstractRegressionTest { static { -// TESTS_NAMES = new String[] { "test056q"}; +// TESTS_NAMES = new String[] { "test061a"}; // TESTS_NUMBERS = new int[] { 50 }; // TESTS_RANGE = new int[] { 11, -1 }; } @@ -2097,6 +2097,84 @@ public void test061f() throws IOException { null); } // Bug 358903 - Filter practically unimportant resource leak warnings +// Bug 361073 - Avoid resource leak warning when the top level resource is closed explicitly +// a resource wrapper is closed closing also the underlying resource - from a real-world example +public void test061f2() throws IOException { + Map options = getCompilerOptions(); + options.put(CompilerOptions.OPTION_ReportUnclosedCloseable, CompilerOptions.ERROR); + options.put(CompilerOptions.OPTION_ReportPotentiallyUnclosedCloseable, CompilerOptions.ERROR); + this.runConformTest( + new String[] { + "X.java", + "import java.io.OutputStream;\n" + + "import java.io.FileOutputStream;\n" + + "import java.io.BufferedOutputStream;\n" + + "import java.io.IOException;\n" + + "public class X {\n" + + " void zork() throws IOException {\n" + + " try {\n" + + " OutputStream os = null;\n" + + " try {\n" + + " os = new BufferedOutputStream(new FileOutputStream(\"somefile\"));\n" + + " String externalForm = \"externalPath\";\n" + + " } finally {\n" + + " if (os != null)\n" + + " os.close();\n" + + " }\n" + + " } catch (IOException e) {\n" + + " e.printStackTrace();\n" + + " }\n" + + " }\n" + + "}\n" + }, + "", + null, + true, + null, + options, + null); +} +// Bug 358903 - Filter practically unimportant resource leak warnings +// Bug 361073 - Avoid resource leak warning when the top level resource is closed explicitly +// a resource wrapper is sent to another method affecting also the underlying resource - from a real-world example +public void test061f3() throws IOException { + 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.FileNotFoundException;\n" + + "import java.io.InputStream;\n" + + "import java.io.BufferedInputStream;\n" + + "public class X {\n" + + " String loadProfile(File profileFile) {\n" + + " try {\n" + + " InputStream stream = new BufferedInputStream(new FileInputStream(profileFile));\n" + + " return loadProfile(stream);\n" + + " } catch (FileNotFoundException e) {\n" + + " //null\n" + + " }\n" + + " return null;\n" + + " }\n" + + " private String loadProfile(InputStream stream) {\n" + + " return null;\n" + + " }\n" + + "}\n" + }, + "----------\n" + + "1. ERROR in X.java (at line 10)\n" + + " return loadProfile(stream);\n" + + " ^^^^^^^^^^^^^^^^^^^^^^^^^^^\n" + + "Potential resource leak: \'stream\' may not be closed at this location\n" + + "----------\n", + null, + true, + options); +} +// Bug 358903 - Filter practically unimportant resource leak warnings // Bug 360908 - Avoid resource leak warning when the underlying/chained resource is closed explicitly // Different points in a resource chain are closed public void test061g() { @@ -2454,6 +2532,40 @@ public void test061o() { true, options); } +// Bug 358903 - Filter practically unimportant resource leak warnings +// a resource wrapper is conditionally allocated but not closed - from a real-world example +public void test061f4() throws IOException { + 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.FileNotFoundException;\n" + + "import java.io.InputStream;\n" + + "import java.io.BufferedInputStream;\n" + + "public class X {\n" + + " void foo(File location, String adviceFilePath) throws FileNotFoundException {\n" + + " InputStream stream = null;\n" + + " if (location.isDirectory()) {\n" + + " File adviceFile = new File(location, adviceFilePath);\n" + + " stream = new BufferedInputStream(new FileInputStream(adviceFile));\n" + + " }\n" + + " }\n" + + "}\n" + }, + "----------\n" + + "1. ERROR in X.java (at line 11)\n" + + " stream = new BufferedInputStream(new FileInputStream(adviceFile));\n" + + " ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n" + + "Potential resource leak: \'stream\' may not be closed\n" + // message could be stronger, but the enclosing if blurs the picture + "----------\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 05cc6b7..0d2cbe9 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 @@ -14,6 +14,7 @@ import java.util.HashMap; import java.util.Iterator; import java.util.Map; import java.util.Map.Entry; +import java.util.Set; import org.eclipse.jdt.internal.compiler.codegen.CodeStream; import org.eclipse.jdt.internal.compiler.flow.FlowContext; @@ -107,6 +108,7 @@ public class FakedTrackingVariable extends LocalDeclaration { scope.getJavaLangObject(), // dummy, just needs to be a reference type 0, false); + this.binding.declaringScope = scope; this.binding.setConstant(Constant.NotAConstant); this.binding.useFlag = LocalVariableBinding.USED; // use a free slot without assigning it: @@ -225,6 +227,7 @@ public class FakedTrackingVariable extends LocalDeclaration { FakedTrackingVariable currentTracker = innerTracker; while (currentTracker != null) { flowInfo.markNullStatus(currentTracker.binding, newStatus); + currentTracker.globalClosingState |= allocation.closeTracker.globalClosingState; currentTracker = currentTracker.innerTracker; } } @@ -536,46 +539,96 @@ public class FakedTrackingVariable extends LocalDeclaration { scope.removeTrackingVar(trackVar); return flowInfo; } - 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.) FlowInfo infoResourceIsClosed = flowInfo.copy(); - infoResourceIsClosed.markAsDefinitelyNonNull(trackVar.binding); + do { + trackVar.globalClosingState |= SHARED_WITH_OUTSIDE; + if (scope.methodScope() != trackVar.methodScope) + trackVar.globalClosingState |= CLOSED_IN_NESTED_METHOD; + infoResourceIsClosed.markAsDefinitelyNonNull(trackVar.binding); + } while ((trackVar = trackVar.innerTracker) != null); return FlowInfo.conditional(flowInfo, infoResourceIsClosed); } return flowInfo; } - + + /** + * Pick tracking variables from 'varsOfScope' to establish a proper order of processing: + * As much as possible pick wrapper resources before their inner resources. + * Also consider cases of wrappers and their inners being declared at different scopes. + */ + public static FakedTrackingVariable pickVarForReporting(Set varsOfScope, BlockScope scope, boolean atExit) { + if (varsOfScope.isEmpty()) return null; + FakedTrackingVariable trackingVar = (FakedTrackingVariable) varsOfScope.iterator().next(); + while (trackingVar.outerTracker != null) { + // resource is wrapped, is wrapper defined in this scope? + if (varsOfScope.contains(trackingVar.outerTracker)) { + // resource from same scope, travel up the wrapper chain + trackingVar = trackingVar.outerTracker; + } else if (atExit) { + // at an exit point we report against inner despite a wrapper that may/may not be closed later + break; + } else { + BlockScope outerTrackerScope = trackingVar.outerTracker.binding.declaringScope; + if (outerTrackerScope == scope) { + // outerTracker is from same scope and already processed -> pick trackingVar now + break; + } else { + // outer resource is from other (outer?) scope + Scope currentScope = scope; + while ((currentScope = currentScope.parent) instanceof BlockScope) { + if (outerTrackerScope == currentScope) { + // at end of block pass responsibility for inner resource to outer scope holding a wrapper + varsOfScope.remove(trackingVar); // drop this one + // pick a next candidate: + return pickVarForReporting(varsOfScope, scope, atExit); + } + } + break; // not parent owned -> pick this var + } + } + } + varsOfScope.remove(trackingVar); + return trackingVar; + } + public void recordErrorLocation(ASTNode location, int nullStatus) { if (this.recordedLocations == null) this.recordedLocations = new HashMap(); this.recordedLocations.put(location, new Integer(nullStatus)); } - public boolean reportRecordedErrors(Scope scope) { + public boolean reportRecordedErrors(Scope scope, int mergedStatus) { FakedTrackingVariable current = this; while (current.globalClosingState == 0) { current = current.innerTracker; if (current == null) { // no relevant state found -> report: - reportError(scope.problemReporter(), null, FlowInfo.NULL); + reportError(scope.problemReporter(), null, mergedStatus); return true; } } boolean hasReported = false; if (this.recordedLocations != null) { Iterator locations = this.recordedLocations.entrySet().iterator(); + int reportFlags = 0; while (locations.hasNext()) { Map.Entry entry = (Entry) locations.next(); - reportError(scope.problemReporter(), (ASTNode)entry.getKey(), ((Integer)entry.getValue()).intValue()); + reportFlags |= reportError(scope.problemReporter(), (ASTNode)entry.getKey(), ((Integer)entry.getValue()).intValue()); hasReported = true; } + if (reportFlags != 0) { + // after all locations have been reported, mark as reported to prevent duplicate report via an outer wrapper + current = this; + do { + current.globalClosingState |= reportFlags; + } while ((current = current.innerTracker) != null); + } } return hasReported; } - public void reportError(ProblemReporter problemReporter, ASTNode location, int nullStatus) { + public int reportError(ProblemReporter problemReporter, ASTNode location, int nullStatus) { // which degree of problem? boolean isPotentialProblem = false; if (nullStatus == FlowInfo.NULL) { @@ -587,22 +640,22 @@ public class FakedTrackingVariable extends LocalDeclaration { // report: if (isPotentialProblem) { if ((this.globalClosingState & (REPORTED_POTENTIAL_LEAK|REPORTED_DEFINITIVE_LEAK)) != 0) - return; + return 0; problemReporter.potentiallyUnclosedCloseable(this, location); } else { if ((this.globalClosingState & (REPORTED_DEFINITIVE_LEAK)) != 0) - return; + return 0; problemReporter.unclosedCloseable(this, location); } // propagate flag to inners: - if (location == null) { - int reportFlag = isPotentialProblem ? REPORTED_POTENTIAL_LEAK : REPORTED_DEFINITIVE_LEAK; + int reportFlag = isPotentialProblem ? REPORTED_POTENTIAL_LEAK : REPORTED_DEFINITIVE_LEAK; + if (location == null) { // if location != null flags will be set after the loop over locations FakedTrackingVariable current = this; - while (current != null) { + do { current.globalClosingState |= reportFlag; - current = current.innerTracker; - } + } while ((current = current.innerTracker) != null); } + return reportFlag; } public void reportExplicitClosing(ProblemReporter problemReporter) { @@ -611,4 +664,12 @@ public class FakedTrackingVariable extends LocalDeclaration { problemReporter.explicitlyClosedAutoCloseable(this); } } + + public void resetReportingBits() { + FakedTrackingVariable current = this; + while (current != null) { + current.globalClosingState &= ~(REPORTED_POTENTIAL_LEAK|REPORTED_DEFINITIVE_LEAK); + current = current.innerTracker; + } + } } diff --git a/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/ast/SingleNameReference.java b/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/ast/SingleNameReference.java index 41385c1..d625d5b 100644 --- a/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/ast/SingleNameReference.java +++ b/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/ast/SingleNameReference.java @@ -1,5 +1,5 @@ /******************************************************************************* - * Copyright (c) 2000, 2012 IBM Corporation and others. + * Copyright (c) 2000, 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 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 7062a9c..24ad91f 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 @@ -1006,13 +1006,10 @@ public void checkUnclosedCloseables(FlowInfo flowInfo, ASTNode location, BlockSc } if (location != null && flowInfo.reachMode() != 0) return; 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); + FakedTrackingVariable trackingVar; + // pick one outer-most variable from the set at a time + while ((trackingVar = FakedTrackingVariable.pickVarForReporting(varSet, this, location != null)) != null) { + 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 @@ -1027,7 +1024,7 @@ public void checkUnclosedCloseables(FlowInfo flowInfo, ASTNode location, BlockSc if (location == null) // at end of block and not definitely unclosed { // problems at specific locations: medium priority - if (trackingVar.reportRecordedErrors(this)) // ... report previously recorded errors + if (trackingVar.reportRecordedErrors(this, status)) // ... report previously recorded errors continue; } if (status == FlowInfo.POTENTIALLY_NULL) { @@ -1044,6 +1041,12 @@ public void checkUnclosedCloseables(FlowInfo flowInfo, ASTNode location, BlockSc for (int i=0; i