7023233: False positive for -Xlint:try with nested try with resources blocks

Summary: Wrong lint warning issued about unused resource when nested try-with-resource blocks are found
Reviewed-by: jjg

src/share/classes/com/sun/tools/javac/comp/Flow.java

@@ -272,7 +272,7 @@ public class Flow extends TreeScanner {
 
     /** The list of unreferenced automatic resources.
      */
-    Map<VarSymbol, JCVariableDecl> unrefdResources;
+    Scope unrefdResources;
 
     /** Set when processing a loop body the second time for DU analysis. */
     boolean loopPassTwo = false;
@@ -992,7 +992,6 @@ public class Flow extends TreeScanner {
     public void visitTry(JCTry tree) {
         List<Type> caughtPrev = caught;
         List<Type> thrownPrev = thrown;
-        Map<VarSymbol, JCVariableDecl> unrefdResourcesPrev = unrefdResources;
         thrown = List.nil();
         for (List<JCCatch> l = tree.catchers; l.nonEmpty(); l = l.tail) {
             List<JCExpression> subClauses = TreeInfo.isMultiCatch(l.head) ?
@@ -1002,17 +1001,18 @@ public class Flow extends TreeScanner {
                 caught = chk.incl(ct.type, caught);
             }
         }
+        ListBuffer<JCVariableDecl> resourceVarDecls = ListBuffer.lb();
         Bits uninitsTryPrev = uninitsTry;
         ListBuffer<PendingExit> prevPendingExits = pendingExits;
         pendingExits = new ListBuffer<PendingExit>();
         Bits initsTry = inits.dup();
         uninitsTry = uninits.dup();
-        unrefdResources = new LinkedHashMap<VarSymbol, JCVariableDecl>();
         for (JCTree resource : tree.resources) {
             if (resource instanceof JCVariableDecl) {
                 JCVariableDecl vdecl = (JCVariableDecl) resource;
                 visitVarDef(vdecl);
-                unrefdResources.put(vdecl.sym, vdecl);
+                unrefdResources.enter(vdecl.sym);
+                resourceVarDecls.append(vdecl);
             } else if (resource instanceof JCExpression) {
                 scanExpr((JCExpression) resource);
             } else {
@@ -1049,11 +1049,14 @@ public class Flow extends TreeScanner {
         Bits uninitsEnd = uninits;
         int nextadrCatch = nextadr;
 
-        if (!unrefdResources.isEmpty() &&
+        if (!resourceVarDecls.isEmpty() &&
                 lint.isEnabled(Lint.LintCategory.TRY)) {
-            for (Map.Entry<VarSymbol, JCVariableDecl> e : unrefdResources.entrySet()) {
-                log.warning(Lint.LintCategory.TRY, e.getValue().pos(),
-                            "try.resource.not.referenced", e.getKey());
+            for (JCVariableDecl resVar : resourceVarDecls) {
+                if (unrefdResources.includes(resVar.sym)) {
+                    log.warning(Lint.LintCategory.TRY, resVar.pos(),
+                                "try.resource.not.referenced", resVar.sym);
+                    unrefdResources.remove(resVar.sym);
+                }
             }
         }
 
@@ -1143,7 +1146,6 @@ public class Flow extends TreeScanner {
             while (exits.nonEmpty()) pendingExits.append(exits.next());
         }
         uninitsTry.andSet(uninitsTryPrev).andSet(uninits);
-        unrefdResources = unrefdResourcesPrev;
     }
 
     public void visitConditional(JCConditional tree) {
@@ -1369,9 +1371,7 @@ public class Flow extends TreeScanner {
     }
 
     void referenced(Symbol sym) {
-        if (unrefdResources != null && unrefdResources.containsKey(sym)) {
-            unrefdResources.remove(sym);
-        }
+        unrefdResources.remove(sym);
     }
 
     public void visitTypeCast(JCTypeCast tree) {
@@ -1430,6 +1430,7 @@ public class Flow extends TreeScanner {
             alive = true;
             this.thrown = this.caught = null;
             this.classDef = null;
+            unrefdResources = new Scope(env.enclClass.sym);
             scan(tree);
         } finally {
             // note that recursive invocations of this method fail hard
@@ -1444,6 +1445,7 @@ public class Flow extends TreeScanner {
             this.make = null;
             this.thrown = this.caught = null;
             this.classDef = null;
+            unrefdResources = null;
         }
     }
 }

test/tools/javac/TryWithResources/UnusedResourcesTest.java

+/*
+ * Copyright (c) 2011, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+/*
+ * @test
+ * @bug 7023233
+ * @summary False positive for -Xlint:try with nested try with resources blocks
+ */
+
+import com.sun.source.util.JavacTask;
+import com.sun.tools.javac.api.JavacTool;
+import com.sun.tools.javac.util.JCDiagnostic;
+import java.net.URI;
+import java.util.Arrays;
+import javax.tools.Diagnostic;
+import javax.tools.JavaCompiler;
+import javax.tools.JavaFileObject;
+import javax.tools.SimpleJavaFileObject;
+import javax.tools.StandardJavaFileManager;
+import javax.tools.ToolProvider;
+
+public class UnusedResourcesTest {
+
+    enum XlintOption {
+        NONE("none"),
+        TRY("try");
+
+        String opt;
+
+        XlintOption(String opt) {
+            this.opt = opt;
+        }
+
+        String getXlintOption() {
+            return "-Xlint:" + opt;
+        }
+    }
+
+    enum TwrStmt {
+        TWR1("res1"),
+        TWR2("res2"),
+        TWR3("res3");
+
+        final String resourceName;
+
+        private TwrStmt(String resourceName) {
+            this.resourceName = resourceName;
+        }
+    }
+
+    enum SuppressLevel {
+        NONE,
+        SUPPRESS;
+
+        String getSuppressAnno() {
+            return this == SUPPRESS ?
+                "@SuppressWarnings(\"try\")" :
+                "";
+        }
+    }
+
+    enum ResourceUsage {
+        NONE(null),
+        USE_R1(TwrStmt.TWR1),
+        USE_R2(TwrStmt.TWR2),
+        USE_R3(TwrStmt.TWR3);
+
+        TwrStmt stmt;
+
+        private ResourceUsage(TwrStmt stmt) {
+            this.stmt = stmt;
+        }
+
+        String usedResourceName() {
+            return stmt != null ? stmt.resourceName : null;
+        }
+
+        boolean isUsedIn(TwrStmt res, TwrStmt stmt) {
+            return this.stmt == res &&
+                    stmt.ordinal() >= this.stmt.ordinal();
+        }
+
+        String getUsage(TwrStmt stmt) {
+            return this != NONE && stmt.ordinal() >= this.stmt.ordinal() ?
+                "use(" + usedResourceName() + ");" :
+                "";
+        }
+    }
+
+    static class JavaSource extends SimpleJavaFileObject {
+
+        String template = "class Resource implements AutoCloseable {\n" +
+                              "public void close() {}\n" +
+                          "}\n" +
+                          "class Test {\n" +
+                              "void use(Resource r) {}\n" +
+                              "#S void test() {\n" +
+                                 "try (Resource #R1 = new Resource()) {\n" +
+                                    "#U1_R1\n" +
+                                    "#U1_R2\n" +
+                                    "#U1_R3\n" +
+                                    "try (Resource #R2 = new Resource()) {\n" +
+                                       "#U2_R1\n" +
+                                       "#U2_R2\n" +
+                                       "#U2_R3\n" +
+                                       "try (Resource #R3 = new Resource()) {\n" +
+                                           "#U3_R1\n" +
+                                           "#U3_R2\n" +
+                                           "#U3_R3\n" +
+                                       "}\n" +
+                                    "}\n" +
+                                 "}\n" +
+                              "}\n" +
+                          "}\n";
+
+        String source;
+
+        public JavaSource(SuppressLevel suppressLevel, ResourceUsage usage1,
+                ResourceUsage usage2, ResourceUsage usage3) {
+            super(URI.create("myfo:/Test.java"), JavaFileObject.Kind.SOURCE);
+            source = template.replace("#S", suppressLevel.getSuppressAnno()).
+                    replace("#R1", TwrStmt.TWR1.resourceName).
+                    replace("#R2", TwrStmt.TWR2.resourceName).
+                    replace("#R3", TwrStmt.TWR3.resourceName).
+                    replace("#U1_R1", usage1.getUsage(TwrStmt.TWR1)).
+                    replace("#U1_R2", usage2.getUsage(TwrStmt.TWR1)).
+                    replace("#U1_R3", usage3.getUsage(TwrStmt.TWR1)).
+                    replace("#U2_R1", usage1.getUsage(TwrStmt.TWR2)).
+                    replace("#U2_R2", usage2.getUsage(TwrStmt.TWR2)).
+                    replace("#U2_R3", usage3.getUsage(TwrStmt.TWR2)).
+                    replace("#U3_R1", usage1.getUsage(TwrStmt.TWR3)).
+                    replace("#U3_R2", usage2.getUsage(TwrStmt.TWR3)).
+                    replace("#U3_R3", usage3.getUsage(TwrStmt.TWR3));
+        }
+
+        @Override
+        public CharSequence getCharContent(boolean ignoreEncodingErrors) {
+            return source;
+        }
+    }
+
+    public static void main(String... args) throws Exception {
+        for (XlintOption xlint : XlintOption.values()) {
+            for (SuppressLevel suppressLevel : SuppressLevel.values()) {
+                for (ResourceUsage usage1 : ResourceUsage.values()) {
+                    for (ResourceUsage usage2 : ResourceUsage.values()) {
+                        for (ResourceUsage usage3 : ResourceUsage.values()) {
+                                test(xlint,
+                                        suppressLevel,
+                                        usage1,
+                                        usage2,
+                                        usage3);
+                        }
+                    }
+                }
+            }
+        }
+    }
+
+    // Create a single file manager and reuse it for each compile to save time.
+    static StandardJavaFileManager fm = JavacTool.create().getStandardFileManager(null, null, null);
+
+    static void test(XlintOption xlint, SuppressLevel suppressLevel, ResourceUsage usage1,
+                ResourceUsage usage2, ResourceUsage usage3) throws Exception {
+        final JavaCompiler tool = ToolProvider.getSystemJavaCompiler();
+        JavaSource source = new JavaSource(suppressLevel, usage1, usage2, usage3);
+        DiagnosticChecker dc = new DiagnosticChecker();
+        JavacTask ct = (JavacTask)tool.getTask(null, fm, dc,
+                Arrays.asList(xlint.getXlintOption()), null, Arrays.asList(source));
+        ct.analyze();
+        check(source, xlint, suppressLevel, usage1, usage2, usage3, dc);
+    }
+
+    static void check(JavaSource source, XlintOption xlint, SuppressLevel suppressLevel,
+                ResourceUsage usage1, ResourceUsage usage2, ResourceUsage usage3, DiagnosticChecker dc) {
+
+        ResourceUsage[] usages = { usage1, usage2, usage3 };
+        boolean[] unusedFound = { dc.unused_r1, dc.unused_r2, dc.unused_r3 };
+        boolean[] usedResources = { false, false, false };
+
+        for (TwrStmt res : TwrStmt.values()) {
+            outer: for (TwrStmt stmt : TwrStmt.values()) {
+                for (ResourceUsage usage : usages) {
+                    if (usage.isUsedIn(res, stmt)) {
+                        usedResources[res.ordinal()] = true;
+                        break outer;
+                    }
+                }
+            }
+        }
+
+        for (TwrStmt stmt : TwrStmt.values()) {
+            boolean unused = !usedResources[stmt.ordinal()] &&
+                    xlint == XlintOption.TRY &&
+                    suppressLevel != SuppressLevel.SUPPRESS;
+            if (unused != unusedFound[stmt.ordinal()]) {
+                throw new Error("invalid diagnostics for source:\n" +
+                    source.getCharContent(true) +
+                    "\nOptions: " + xlint.getXlintOption() +
+                    "\nFound unused res1: " + unusedFound[0] +
+                    "\nFound unused res2: " + unusedFound[1] +
+                    "\nFound unused res3: " + unusedFound[2] +
+                    "\nExpected unused res1: " + !usedResources[0] +
+                    "\nExpected unused res2: " + !usedResources[1] +
+                    "\nExpected unused res3: " + !usedResources[2]);
+            }
+        }
+    }
+
+    static class DiagnosticChecker implements javax.tools.DiagnosticListener<JavaFileObject> {
+
+        boolean unused_r1 = false;
+        boolean unused_r2 = false;
+        boolean unused_r3 = false;
+
+        public void report(Diagnostic<? extends JavaFileObject> diagnostic) {
+            if (diagnostic.getKind() == Diagnostic.Kind.WARNING &&
+                    diagnostic.getCode().contains("try.resource.not.referenced")) {
+                String varName = ((JCDiagnostic)diagnostic).getArgs()[0].toString();
+                if (varName.equals(TwrStmt.TWR1.resourceName)) {
+                    unused_r1 = true;
+                } else if (varName.equals(TwrStmt.TWR2.resourceName)) {
+                    unused_r2 = true;
+                } else if (varName.equals(TwrStmt.TWR3.resourceName)) {
+                    unused_r3 = true;
+                }
+            }
+        }
+    }
+}