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..6aa2988 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,157 @@ 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" + + "Resource leak: \'reader\' is not 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); +} 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..25e38ab 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