Bug 396575 - [compiler][resources] Incorrect Errors/Warnings check for

potential resource leak when surrounding with try-catch

Change-Id: Ic7b2b03eb15abec2943788af455020249c11a8dd
Signed-off-by: Stephan Herrmann <stephan.herrmann@berlin.de>

3 files changed, 159 insertions, 40 deletions

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 5b89044b25..055ab74668 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
@@ -5373,4 +5373,65 @@ public void testBug390064() {
 		true,
 		options);
 }
+public void testBug396575() {
+	Map options = getCompilerOptions();
+	options.put(CompilerOptions.OPTION_ReportPotentiallyUnclosedCloseable, CompilerOptions.ERROR);
+	options.put(CompilerOptions.OPTION_ReportUnclosedCloseable, CompilerOptions.ERROR);
+	options.put(CompilerOptions.OPTION_ReportSyntheticAccessEmulation, CompilerOptions.IGNORE);
+	runNegativeTest(
+		new String[] {
+			"Bug396575.java",
+			"import java.io.*;\n" + 
+			"\n" + 
+			"public class Bug396575 {\n" + 
+			"  void test1(File myFile) {\n" + 
+			"   OutputStream out = null;\n" + 
+			"   BufferedWriter bw = null;\n" + 
+			"   try {\n" + 
+			"       // code...\n" + 
+			"       out = new FileOutputStream(myFile);\n" + 
+			"       OutputStreamWriter writer = new OutputStreamWriter(out);\n" + 
+			"       bw = new BufferedWriter(writer);\n" + 
+			"       // more code...\n" + 
+			"   } catch (Exception e) {\n" + 
+			"       try {\n" + 
+			"           bw.close(); // WARN: potential null pointer access\n" + 
+			"       } catch (Exception ignored) {}\n" + 
+			"       return;  // WARN: resource leak - bw may not be closed\n" + 
+			"   }\n" + 
+			"  }\n" + 
+			"  \n" + 
+			"  void test2(File myFile) {\n" + 
+			"       BufferedWriter bw = null;\n" + 
+			"   try {\n" + 
+			"       // code...\n" + 
+			"                                                       // declare \"out\" here inside try-catch as a temp variable\n" + 
+			"       OutputStream out = new FileOutputStream(myFile); // WARN: out is never closed.\n" + 
+			"       OutputStreamWriter writer = new OutputStreamWriter(out);\n" + 
+			"       bw = new BufferedWriter(writer);\n" + 
+			"       // more code...\n" + 
+			"   } catch (Exception e) {\n" + 
+			"       try {\n" + 
+			"           bw.close(); // WARN: potential null pointer access\n" + 
+			"       } catch (Exception ignored) {}\n" + 
+			"       return;  // WARN: resource leak - bw may not be closed\n" + 
+			"   }\n" + 
+			"  }\n" + 
+			"}\n"
+		},
+		"----------\n" + 
+		"1. ERROR in Bug396575.java (at line 11)\n" + 
+		"	bw = new BufferedWriter(writer);\n" + 
+		"	^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n" + 
+		"Resource leak: \'bw\' is never closed\n" + 
+		"----------\n" + 
+		"2. ERROR in Bug396575.java (at line 28)\n" + 
+		"	bw = new BufferedWriter(writer);\n" + 
+		"	^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n" + 
+		"Resource leak: \'bw\' is never closed\n" + 
+		"----------\n",
+		null,
+		true,
+		options);
+}
 }
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 a2f372f2be..1a1f7878a5 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
@@ -13,7 +13,9 @@
 package org.eclipse.jdt.internal.compiler.ast;
 
 import java.util.HashMap;
+import java.util.HashSet;
 import java.util.Iterator;
+import java.util.List;
 import java.util.Map;
 import java.util.Map.Entry;
 import java.util.Set;
@@ -708,7 +710,7 @@ public class FakedTrackingVariable extends LocalDeclaration {
 		do {
 			flowInfo.markAsDefinitelyNonNull(current.binding);
 			current.globalClosingState |= CLOSE_SEEN;
-			flowContext.markFinallyNullStatus(this.binding, FlowInfo.NON_NULL);
+			flowContext.markFinallyNullStatus(current.binding, FlowInfo.NON_NULL);
 			current = current.innerTracker;
 		} while (current != null);
 	}
@@ -746,44 +748,100 @@ public class FakedTrackingVariable extends LocalDeclaration {
 		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.
+	/**
+	 * Iterator for a set of FakedTrackingVariable, which dispenses the elements 
+	 * according to the priorities defined by enum {@link Stage}.
+	 * Resources whose outer is owned by an enclosing scope are never answered,
+	 * unless we are analysing on behalf of an exit (return/throw).
 	 */
-	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);
+	public static class IteratorForReporting implements Iterator<FakedTrackingVariable> {
+
+		private final Set<FakedTrackingVariable> varSet;
+		private final Scope scope;
+		private final boolean atExit;
+
+		private Stage stage;
+		private Iterator<FakedTrackingVariable> iterator;
+		private FakedTrackingVariable next;
+		
+		enum Stage {
+			/** 1. prio: all top-level resources, ie., resources with no outer. */
+			OuterLess,
+			/** 2. prio: resources whose outer has already been processed (element of the same varSet). */
+			InnerOfProcessed,
+			/** 3. prio: resources whose outer is not owned by any enclosing scope. */
+			InnerOfNotEnclosing,
+			/** 4. prio: when analysing on behalf of an exit point: anything not picked before. */
+			AtExit
+		}
+
+		public IteratorForReporting(List<FakedTrackingVariable> variables, Scope scope, boolean atExit) {
+			this.varSet = new HashSet<>(variables);
+			this.scope = scope;
+			this.atExit = atExit;
+			setUpForStage(Stage.OuterLess);
+		}
+		@Override
+		public boolean hasNext() {
+			FakedTrackingVariable trackingVar;
+			switch (this.stage) {
+				case OuterLess:
+					while (this.iterator.hasNext()) {
+						trackingVar = this.iterator.next();
+						if (trackingVar.outerTracker == null)
+							return found(trackingVar);
+					}
+					setUpForStage(Stage.InnerOfProcessed);
+					//$FALL-THROUGH$
+				case InnerOfProcessed:
+					while (this.iterator.hasNext()) {
+						trackingVar = this.iterator.next();
+						FakedTrackingVariable outer = trackingVar.outerTracker;
+						if (outer.binding.declaringScope == this.scope && !this.varSet.contains(outer))
+							return found(trackingVar);
+					}
+					setUpForStage(Stage.InnerOfNotEnclosing);
+					//$FALL-THROUGH$
+				case InnerOfNotEnclosing:
+					searchAlien: while (this.iterator.hasNext()) {
+						trackingVar = this.iterator.next();
+						FakedTrackingVariable outer = trackingVar.outerTracker;
+						if (!this.varSet.contains(outer)) {
+							Scope outerTrackerScope = outer.binding.declaringScope;
+							Scope currentScope = this.scope;
+							while ((currentScope = currentScope.parent) instanceof BlockScope) {
+								if (outerTrackerScope == currentScope)
+									break searchAlien;
+							}
+							return found(trackingVar);
 						}
 					}
-					break; // not parent owned -> pick this var
-				}
+					setUpForStage(Stage.AtExit);
+					//$FALL-THROUGH$
+				case AtExit:
+					if (this.atExit && this.iterator.hasNext())
+						return found(this.iterator.next());
+					return false;
+				default: throw new IllegalStateException("Unexpected Stage "+this.stage); //$NON-NLS-1$
 			}
 		}
-		varsOfScope.remove(trackingVar);
-		return trackingVar;
+		private boolean found(FakedTrackingVariable trackingVar) {
+			this.iterator.remove();
+			this.next = trackingVar;
+			return true;
+		}
+		private void setUpForStage(Stage nextStage) {
+			this.iterator = this.varSet.iterator();
+			this.stage = nextStage;
+		}
+		@Override
+		public FakedTrackingVariable next() {
+			return this.next;
+		}
+		@Override
+		public void remove() {
+			throw new UnsupportedOperationException();
+		}
 	}
 
 	/**
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 79ffaeec2a..866ca8bac5 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
@@ -21,6 +21,7 @@
  *								Bug 371614 - [compiler][resource] Wrong "resource leak" problem on return/throw inside while loop
  *								Bug 421035 - [resource] False alarm of resource leak warning when casting a closeable in its assignment
  *								Bug 444964 - [1.7+][resource] False resource leak warning (try-with-resources for ByteArrayOutputStream - return inside for loop)
+ *								Bug 396575 - [compiler][resources] Incorrect Errors/Warnings check for potential resource leak when surrounding with try-catch
  *     Jesper S Moller <jesper@selskabet.org> - Contributions for
  *								bug 378674 - "The method can be declared as static" is wrong
  *     Keigo Imai - Contribution for  bug 388903 - Cannot extend inner class as an anonymous class when it extends the outer class
@@ -28,9 +29,8 @@
 package org.eclipse.jdt.internal.compiler.lookup;
 
 import java.util.ArrayList;
-import java.util.HashSet;
+import java.util.Iterator;
 import java.util.List;
-import java.util.Set;
 
 import org.eclipse.jdt.core.compiler.CharOperation;
 import org.eclipse.jdt.internal.compiler.ast.*;
@@ -1081,10 +1081,10 @@ public void checkUnclosedCloseables(FlowInfo flowInfo, FlowContext flowContext,
 	FakedTrackingVariable returnVar = (location instanceof ReturnStatement) ?
 			FakedTrackingVariable.getCloseTrackingVariable(((ReturnStatement)location).expression, flowInfo, flowContext) : null;
 
-	Set varSet = new HashSet(this.trackingVariables);
-	FakedTrackingVariable trackingVar;
-	// pick one outer-most variable from the set at a time
-	while ((trackingVar = FakedTrackingVariable.pickVarForReporting(varSet, this, location != null)) != null) {
+	// iterate variables according to the priorities defined in FakedTrackingVariable.IteratorForReporting.Stage
+	Iterator<FakedTrackingVariable> iterator = new FakedTrackingVariable.IteratorForReporting(this.trackingVariables, this, location != null);
+	while (iterator.hasNext()) {
+		FakedTrackingVariable trackingVar = iterator.next();
 
 		if (returnVar != null && trackingVar.isResourceBeingReturned(returnVar)) {
 			continue;