Download
Getting Started
Members
Projects
Community
Marketplace
Events
Planet Eclipse
Newsletter
Videos
Participate
Report a Bug
Forums
Mailing Lists
Wiki
IRC
How to Contribute
Working Groups
Automotive
Internet of Things
LocationTech
Long-Term Support
PolarSys
Science
OpenMDM
More
Community
Marketplace
Events
Planet Eclipse
Newsletter
Videos
Participate
Report a Bug
Forums
Mailing Lists
Wiki
IRC
How to Contribute
Working Groups
Automotive
Internet of Things
LocationTech
Long-Term Support
PolarSys
Science
OpenMDM
Toggle navigation
Bugzilla – Attachment 204420 Details for
Bug 359334
Analysis for resource leak warnings does not consider exceptions as method exit points
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Requests
|
Help
|
Log In
[x]
|
Terms of Use
|
Copyright Agent
[patch]
additional fix
Bug_359334_3.patch (text/plain), 17.01 KB, created by
Stephan Herrmann
on 2011-10-01 13:41:38 EDT
(
hide
)
Description:
additional fix
Filename:
MIME Type:
Creator:
Stephan Herrmann
Created:
2011-10-01 13:41:38 EDT
Size:
17.01 KB
patch
obsolete
>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<this.localIndex; i++) >+ this.locals[i].closeTracker = null; >+ this.trackingVariables = null; >+ } > } >+ >+private int mergeCloseStatus(int status, LocalVariableBinding binding, BlockScope outerScope) { >+ // get the most suitable null status representing whether resource 'binding' has been closed >+ // start at this scope and potentially travel out until 'outerScope' >+ // at each scope consult any recorded 'finallyInfo'. >+ if (status != FlowInfo.NON_NULL) { >+ if (this.finallyInfo != null) { >+ int finallyStatus = this.finallyInfo.nullStatus(binding); >+ if (finallyStatus == FlowInfo.NON_NULL) >+ return finallyStatus; >+ if (status != FlowInfo.NULL && finallyStatus != FlowInfo.NULL) // neither definitely null nor non-null >+ status = FlowInfo.POTENTIALLY_NULL; >+ } >+ if (this != outerScope && this.parent instanceof BlockScope) >+ return ((BlockScope) this.parent).mergeCloseStatus(status, binding, outerScope); >+ } >+ return status; >+} >+ > private void reportResourceLeak(FakedTrackingVariable trackingVar, ASTNode location, int nullStatus) { > if (location != null) > trackingVar.recordErrorLocation(location, nullStatus);
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
Attachments on
bug 359334
:
204331
|
204333
|
204420
|
204421