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 563cc0b..ae4820c 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[] { "test056throw"}; +// TESTS_NAMES = new String[] { "test059"}; // TESTS_NUMBERS = new int[] { 50 }; // TESTS_RANGE = new int[] { 11, -1 }; } @@ -5226,6 +5226,214 @@ "}\n" }, ""); } +// Bug 358903 - Filter practically unimportant resource leak warnings +// Bug 360908 - Avoid resource leak warning when the underlying/chained resource is closed explicitly +// a resource wrapper is not closed but the underlying resource is +public void test059a() { + Map options = getCompilerOptions(); + options.put(CompilerOptions.OPTION_ReportUnclosedCloseable, CompilerOptions.ERROR); + options.put(CompilerOptions.OPTION_ReportPotentiallyUnclosedCloseable, CompilerOptions.WARNING); + this.runConformTest( + new String[] { + "X.java", + "import java.io.File;\n" + + "import java.io.BufferedInputStream;\n" + + "import java.io.FileInputStream;\n" + + "import java.io.IOException;\n" + + "public class X {\n" + + " void foo() throws IOException {\n" + + " File file = new File(\"somefile\");\n" + + " FileInputStream fileStream = new FileInputStream(file);\n" + + " BufferedInputStream bis = new BufferedInputStream(fileStream);\n" + + " BufferedInputStream doubleWrap = new BufferedInputStream(bis);\n" + + " System.out.println(bis.available());\n" + + " fileStream.close();\n" + + " }\n" + + " void inline() throws IOException {\n" + + " File file = new File(\"somefile\");\n" + + " FileInputStream fileStream;\n" + + " BufferedInputStream bis = new BufferedInputStream(fileStream = new FileInputStream(file));\n" + + " System.out.println(bis.available());\n" + + " fileStream.close();\n" + + " }\n" + + " public static void main(String[] args) throws IOException {\n" + + " try {\n" + + " new X().foo();\n" + + " } catch (IOException ex) {" + + " System.out.println(\"Got IO Exception\");\n" + + " }\n" + + " }\n" + + "}\n" + }, + "Got IO Exception", + null, + true, + null, + options, + null); +} +// Bug 358903 - Filter practically unimportant resource leak warnings +// a closeable without OS resource is not closed +public void test059b() { + Map options = getCompilerOptions(); + options.put(CompilerOptions.OPTION_ReportUnclosedCloseable, CompilerOptions.ERROR); + options.put(CompilerOptions.OPTION_ReportPotentiallyUnclosedCloseable, CompilerOptions.WARNING); + this.runConformTest( + new String[] { + "X.java", + "import java.io.StringReader;\n" + + "import java.io.IOException;\n" + + "public class X {\n" + + " void foo() throws IOException {\n" + + " StringReader string = new StringReader(\"content\");\n" + + " System.out.println(string.read());\n" + + " }\n" + + " public static void main(String[] args) throws IOException {\n" + + " new X().foo();\n" + + " }\n" + + "}\n" + }, + "99", // character 'c' + null, + true, + null, + options, + null); +} +// Bug 358903 - Filter practically unimportant resource leak warnings +// a resource wrapper is not closed but the underlying closeable is resource-free +public void test059c() { + Map options = getCompilerOptions(); + options.put(CompilerOptions.OPTION_ReportUnclosedCloseable, CompilerOptions.ERROR); + options.put(CompilerOptions.OPTION_ReportPotentiallyUnclosedCloseable, CompilerOptions.WARNING); + this.runConformTest( + new String[] { + "X.java", + "import java.io.BufferedReader;\n" + + "import java.io.StringReader;\n" + + "import java.io.IOException;\n" + + "public class X {\n" + + " void foo() throws IOException {\n" + + " StringReader input = new StringReader(\"content\");\n" + + " BufferedReader br = new BufferedReader(input);\n" + + " BufferedReader doubleWrap = new BufferedReader(br);\n" + + " System.out.println(br.read());\n" + + " }\n" + + " void inline() throws IOException {\n" + + " BufferedReader br = new BufferedReader(new StringReader(\"content\"));\n" + + " System.out.println(br.read());\n" + + " }\n" + + " public static void main(String[] args) throws IOException {\n" + + " new X().foo();\n" + + " }\n" + + "}\n" + }, + "99", + null, + true, + null, + options, + null); +} +// Bug 358903 - Filter practically unimportant resource leak warnings +// a resource wrapper is not closed neither is the underlying resource +public void test059d() { + Map options = getCompilerOptions(); + options.put(CompilerOptions.OPTION_ReportUnclosedCloseable, CompilerOptions.ERROR); + options.put(CompilerOptions.OPTION_ReportPotentiallyUnclosedCloseable, CompilerOptions.WARNING); + this.runNegativeTest( + new String[] { + "X.java", + "import java.io.File;\n" + + "import java.io.BufferedInputStream;\n" + + "import java.io.FileInputStream;\n" + + "import java.io.IOException;\n" + + "public class X {\n" + + " void foo() throws IOException {\n" + + " File file = new File(\"somefile\");\n" + + " FileInputStream fileStream = new FileInputStream(file);\n" + + " BufferedInputStream bis = new BufferedInputStream(fileStream);\n" + + " BufferedInputStream doubleWrap = new BufferedInputStream(bis);\n" + + " System.out.println(bis.available());\n" + + " }\n" + + " void inline() throws IOException {\n" + + " File file = new File(\"somefile\");\n" + + " BufferedInputStream bis2 = new BufferedInputStream(new FileInputStream(file));\n" + + " System.out.println(bis2.available());\n" + + " }\n" + + " public static void main(String[] args) throws IOException {\n" + + " try {\n" + + " new X().foo();\n" + + " } catch (IOException ex) {" + + " System.out.println(\"Got IO Exception\");\n" + + " }\n" + + " }\n" + + "}\n" + }, + "----------\n" + + "1. ERROR in X.java (at line 9)\n" + + " BufferedInputStream bis = new BufferedInputStream(fileStream);\n" + + " ^^^\n" + + "Resource leak: \'bis\' is never closed\n" + + "----------\n" + + "2. ERROR in X.java (at line 15)\n" + + " BufferedInputStream bis2 = new BufferedInputStream(new FileInputStream(file));\n" + + " ^^^^\n" + + "Resource leak: \'bis2\' is never closed\n" + + "----------\n", + null, + true, + options); +} +// 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 +public void test059e() { + Map options = getCompilerOptions(); + options.put(CompilerOptions.OPTION_ReportUnclosedCloseable, CompilerOptions.ERROR); + options.put(CompilerOptions.OPTION_ReportPotentiallyUnclosedCloseable, CompilerOptions.WARNING); + this.runConformTest( + new String[] { + "X.java", + "import java.io.File;\n" + + "import java.io.BufferedInputStream;\n" + + "import java.io.FileInputStream;\n" + + "import java.io.IOException;\n" + + "public class X {\n" + + " void foo() throws IOException {\n" + + " File file = new File(\"somefile\");\n" + + " FileInputStream fileStream = new FileInputStream(file);\n" + + " BufferedInputStream bis = new BufferedInputStream(fileStream);\n" + + " BufferedInputStream doubleWrap = new BufferedInputStream(bis);\n" + + " System.out.println(bis.available());\n" + + " bis.close();\n" + + " }\n" + + " void inline() throws IOException {\n" + + " File file = new File(\"somefile\");\n" + + " BufferedInputStream bis2 = new BufferedInputStream(new FileInputStream(file));\n" + + " System.out.println(bis2.available());\n" + + " bis2.close();\n" + + " FileInputStream fileStream = null;\n" + + " BufferedInputStream bis3 = new BufferedInputStream(fileStream = new FileInputStream(file));\n" + + " System.out.println(bis3.available());\n" + + " bis3.close();\n" + + " }\n" + + " public static void main(String[] args) throws IOException {\n" + + " try {\n" + + " new X().foo();\n" + + " } catch (IOException ex) {" + + " System.out.println(\"Got IO Exception\");\n" + + " }\n" + + " }\n" + + "}\n" + }, + "Got IO Exception", + null, + true, + null, + options, + null); +} public static Class testClass() { return TryWithResourcesStatementTest.class; } 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 ee13047..2621041 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 @@ -48,6 +48,8 @@ private static final int CLOSED_IN_NESTED_METHOD = 4; // a location independent issue has been reported already against this resource: private static final int REPORTED = 8; + // a resource is wrapped in another resource: + private static final int WRAPPED = 16; /** * Bitset of {@link #CLOSE_SEEN}, {@link #PASSED_TO_OUTSIDE}, {@link #CLOSED_IN_NESTED_METHOD} and {@link #REPORTED}. @@ -58,10 +60,12 @@ public LocalVariableBinding originalBinding; // the real local being tracked - HashMap recordedLocations; // initially null, ASTNode -> Integer + HashMap recordedLocations; // initially null, ASTNode -> Integer + + public FakedTrackingVariable innerTracker; // chained tracking variable of a chained (wrapped) resource - public FakedTrackingVariable(LocalVariableBinding original, Statement location) { + public FakedTrackingVariable(LocalVariableBinding original, ASTNode location) { super(original.name, location.sourceStart, location.sourceEnd); this.type = new SingleTypeReference( TypeConstants.OBJECT, @@ -145,7 +149,10 @@ // re-assigning from a fresh, mark as not-closed again: flowInfo.markAsDefinitelyNull(previousTrackerBinding); } else { // 3. no re-use, create a fresh tracking variable: - local.closeTracker = new FakedTrackingVariable(local, location); + rhsTrackVar = analyseCloseableExpression(local, location, rhs); + if (rhsTrackVar == null) + return; + local.closeTracker = rhsTrackVar; // a fresh resource, mark as not-closed: flowInfo.markAsDefinitelyNull(local.closeTracker.binding); // TODO(stephan): this might be useful, but I could not find a test case for it: @@ -153,6 +160,76 @@ // flowContext.initsOnFinally.markAsDefinitelyNonNull(trackerBinding); } } + } + /* analyse structure of a closeable expression, matching (chained) resources against our white lists. */ + private static FakedTrackingVariable analyseCloseableExpression(LocalVariableBinding local, + ASTNode location, Expression expression) + { + if (expression.resolvedType instanceof ReferenceBinding) { + ReferenceBinding resourceType = (ReferenceBinding) expression.resolvedType; + + if (resourceType.hasTypeBit(TypeIds.BitResourceFreeCloseable)) { + // (a) resource-free closeable: -> null + return null; + } + + if (resourceType.hasTypeBit(TypeIds.BitWrapperCloseable)) { + if (expression instanceof Assignment) + expression = ((Assignment)expression).expression; + if (expression instanceof AllocationExpression) { + Expression[] args = ((AllocationExpression) expression).arguments; + if (args != null && args.length == 1) { + Expression arg = args[0]; + if (arg instanceof Assignment) { + Expression lhs = ((Assignment)arg).lhs; + if (lhs instanceof SingleNameReference) { + SingleNameReference lhsRef = (SingleNameReference) lhs; + if (lhsRef.binding instanceof LocalVariableBinding) { + FakedTrackingVariable innerTracker = analyseCloseableExpression(((LocalVariableBinding)lhsRef.binding), arg, ((Assignment) arg).expression); + if (innerTracker != null) { + // (b.1) wrapper with relevant inner: -> tracking var, linked to inner tracking var + FakedTrackingVariable tracker = new FakedTrackingVariable(local, location); + tracker.innerTracker = innerTracker; + innerTracker.globalClosingState |= WRAPPED; + return tracker; + } + } + } + // FIXME: if we ever get here, what should we do? + } + if (arg instanceof SingleNameReference) { + SingleNameReference ref = (SingleNameReference) arg; + if (ref.binding instanceof LocalVariableBinding) { + LocalVariableBinding innerLocal = (LocalVariableBinding)ref.binding; + FakedTrackingVariable innerTracker = innerLocal.closeTracker; + if (innerTracker != null) + innerTracker = analyseCloseableExpression(innerLocal, expression, arg); + if (innerTracker != null) { + // (b.2) wrapper with relevant inner: -> tracking var, linked to inner tracking var + FakedTrackingVariable tracker = new FakedTrackingVariable(local, location); + tracker.innerTracker = innerTracker; + innerTracker.globalClosingState |= WRAPPED; + return tracker; + } + } + } else if (arg instanceof AllocationExpression) { + ReferenceBinding innerType = (ReferenceBinding)arg.resolvedType; + if (!innerType.hasTypeBit(TypeIds.BitResourceFreeCloseable|TypeIds.BitWrapperCloseable)) { + // (c) wrapper alloc with direct nested alloc of regular: -> normal track var (no local represents inner) + return new FakedTrackingVariable(local, location); + } + } + } + } + // (d) wrapper with irrelevant inner: -> null + return null; + } + } + if (local.closeTracker != null) + // (e): inner has already been analysed: -> re-use track var + return local.closeTracker; + // (f): normal resource: -> normal tracking var + return new FakedTrackingVariable(local, location); } /** Answer wither the given type binding is a subtype of java.lang.AutoCloseable. */ @@ -182,7 +259,7 @@ */ public static FlowInfo markPassedToOutside(BlockScope scope, Expression expression, FlowInfo flowInfo) { FakedTrackingVariable trackVar = getCloseTrackingVariable(expression); - if (trackVar != null) { + if (trackVar != null && trackVar.innerTracker == null) { // ignore when resource is passed to the ctor of a resource wrapper trackVar.globalClosingState |= PASSED_TO_OUTSIDE; if (scope.methodScope() != trackVar.methodScope) trackVar.globalClosingState |= CLOSED_IN_NESTED_METHOD; @@ -200,9 +277,9 @@ this.recordedLocations.put(location, new Integer(nullStatus)); } - public boolean reportRecordedErrors(Scope scope) { + public boolean reportRecordedErrors(Scope scope, int innerStatus) { if (this.globalClosingState == 0) { - reportError(scope.problemReporter(), null, FlowInfo.NULL); + reportError(scope.problemReporter(), null, FlowInfo.NULL, innerStatus); return true; } boolean hasReported = false; @@ -210,14 +287,19 @@ Iterator locations = this.recordedLocations.entrySet().iterator(); while (locations.hasNext()) { Map.Entry entry = (Entry) locations.next(); - reportError(scope.problemReporter(), (ASTNode)entry.getKey(), ((Integer)entry.getValue()).intValue()); + reportError(scope.problemReporter(), (ASTNode)entry.getKey(), ((Integer)entry.getValue()).intValue(), 0); // FIXME: innerStatus hasReported = true; } } return hasReported; } - public void reportError(ProblemReporter problemReporter, ASTNode location, int nullStatus) { + public void reportError(ProblemReporter problemReporter, ASTNode location, int nullStatus, int innerStatus) { + if (this.innerTracker != null && innerStatus == FlowInfo.NON_NULL) { + return; // inner resource is closed, don't complain against the wrapper + } + if ((this.globalClosingState & WRAPPED) != 0) + return; if (nullStatus == FlowInfo.NULL) { if ((this.globalClosingState & CLOSED_IN_NESTED_METHOD) != 0) problemReporter.potentiallyUnclosedCloseable(this, location); @@ -225,7 +307,7 @@ problemReporter.unclosedCloseable(this, location); } else if (nullStatus == FlowInfo.POTENTIALLY_NULL) { problemReporter.potentiallyUnclosedCloseable(this, location); - } + } } public void reportExplicitClosing(ProblemReporter problemReporter) { diff --git a/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/lookup/BinaryTypeBinding.java b/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/lookup/BinaryTypeBinding.java index 9294726..ec2fe2c 100644 --- a/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/lookup/BinaryTypeBinding.java +++ b/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/lookup/BinaryTypeBinding.java @@ -1276,6 +1276,8 @@ } } this.typeBits |= this.superclass.typeBits; + if (this.superclass.hasTypeBit(TypeIds.BitAutoCloseable|TypeIds.BitCloseable)) + this.typeBits |= applyCloseableWhitelists(); return this.superclass; } // NOTE: superInterfaces of binary types are resolved when needed 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 c93c9d9..9d058cd 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 @@ -1007,20 +1007,28 @@ // 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); + // consider wrappers (per white list) encapsulating an inner resource: + int innerStatus = 0; + if (trackingVar.innerTracker != null) { + LocalVariableBinding innerVar = trackingVar.innerTracker.binding; + innerStatus = getNullStatusAggressively(innerVar, flowInfo); + if (locationScope != null) // only check at method exit points + innerStatus = locationScope.mergeCloseStatus(innerStatus, innerVar, this); + } if (status == FlowInfo.NULL) { // definitely unclosed: highest priority - reportResourceLeak(trackingVar, location, status); + reportResourceLeak(trackingVar, location, status, innerStatus); continue; } 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, innerStatus)) // ... report previously recorded errors continue; } if (status == FlowInfo.POTENTIALLY_NULL) { // potentially unclosed: lower priority - reportResourceLeak(trackingVar, location, status); + reportResourceLeak(trackingVar, location, status, innerStatus); } else if (status == FlowInfo.NON_NULL) { // properly closed but not managed by t-w-r: lowest priority if (environment().globalOptions.complianceLevel >= ClassFileConstants.JDK1_7) @@ -1053,11 +1061,11 @@ return status; } -private void reportResourceLeak(FakedTrackingVariable trackingVar, ASTNode location, int nullStatus) { +private void reportResourceLeak(FakedTrackingVariable trackingVar, ASTNode location, int nullStatus, int innerStatus) { if (location != null) trackingVar.recordErrorLocation(location, nullStatus); else - trackingVar.reportError(problemReporter(), null, nullStatus); + trackingVar.reportError(problemReporter(), null, nullStatus, innerStatus); } /** diff --git a/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/lookup/ReferenceBinding.java b/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/lookup/ReferenceBinding.java index 75de854..057fb4f 100644 --- a/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/lookup/ReferenceBinding.java +++ b/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/lookup/ReferenceBinding.java @@ -1469,4 +1469,31 @@ public FieldBinding[] unResolvedFields() { return Binding.NO_FIELDS; } + +/* + * If a type - known to be a Closeable - is mentioned in one of our white lists + * answer the typeBit for the white list (BitWrapperCloseable or BitResourceFreeCloseable). + */ +protected int applyCloseableWhitelists() { + switch (this.compoundName.length) { + case 3: + if (CharOperation.equals(TypeConstants.JAVA, this.compoundName[0])) { + if (CharOperation.equals(TypeConstants.IO, this.compoundName[1])) { + char[] simpleName = this.compoundName[2]; + int l = TypeConstants.JAVA_IO_WRAPPER_CLOSEABLES.length; + for (int i = 0; i < l; i++) { + if (CharOperation.equals(simpleName, TypeConstants.JAVA_IO_WRAPPER_CLOSEABLES[i])) + return TypeIds.BitWrapperCloseable; + } + l = TypeConstants.JAVA_IO_RESOURCE_FREE_CLOSEABLES.length; + for (int i = 0; i < l; i++) { + if (CharOperation.equals(simpleName, TypeConstants.JAVA_IO_RESOURCE_FREE_CLOSEABLES[i])) + return TypeIds.BitResourceFreeCloseable; + } + } + } + break; + } + return 0; +} } diff --git a/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/lookup/TypeConstants.java b/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/lookup/TypeConstants.java index 0bbef47..2dd53b6 100644 --- a/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/lookup/TypeConstants.java +++ b/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/lookup/TypeConstants.java @@ -154,6 +154,22 @@ }; char[][] JAVA_LANG_AUTOCLOSEABLE = {JAVA, LANG, "AutoCloseable".toCharArray()}; //$NON-NLS-1$ char[] CLOSE = "close".toCharArray(); //$NON-NLS-1$ + // white lists of closeables: + char[][] JAVA_IO_WRAPPER_CLOSEABLES = new char[][] { + "BufferedInputStream".toCharArray(), //$NON-NLS-1$ + "BufferedOutputStream".toCharArray(), //$NON-NLS-1$ + "BufferedReader".toCharArray(), //$NON-NLS-1$ + "BufferedWriter".toCharArray(), //$NON-NLS-1$ + "PrintWriter".toCharArray(), //$NON-NLS-1$ + }; + char[][] JAVA_IO_RESOURCE_FREE_CLOSEABLES = new char[][] { + "StringReader".toCharArray(), //$NON-NLS-1$ + "StringWriter".toCharArray(), //$NON-NLS-1$ + "ByteArrayInputStream".toCharArray(), //$NON-NLS-1$ + "ByteArrayOutputStream".toCharArray(), //$NON-NLS-1$ + "CharArrayReader".toCharArray(), //$NON-NLS-1$ + "CharArrayWriter".toCharArray(), //$NON-NLS-1$ + }; // Constraints for generic type argument inference int CONSTRAINT_EQUAL = 0; // Actual = Formal diff --git a/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/lookup/TypeIds.java b/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/lookup/TypeIds.java index 7fff434..4a627bb 100644 --- a/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/lookup/TypeIds.java +++ b/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/lookup/TypeIds.java @@ -207,4 +207,14 @@ * @see ReferenceBinding#hasTypeBit(int) */ final int BitCloseable = 2; + /** + * Bit for members of a white list: + * Subtypes of Closeable that wrap another resource without directly holding any OS resources. + */ + final int BitWrapperCloseable = 4; + /** + * Bit for members of a white list: + * Subtypes of Closeable that do not hold an OS resource that needs to be released. + */ + final int BitResourceFreeCloseable = 8; }