[java] UseTryWithResources false positive with not local vars

when closeable is provided as a method argument or class field
Fixes #3235

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/UseTryWithResourcesRule.java

@@ -8,15 +8,20 @@ import static net.sourceforge.pmd.properties.PropertyFactory.stringListProperty;
 
 import java.util.ArrayList;
 import java.util.List;
+import java.util.Map;
 
+import net.sourceforge.pmd.RuleContext;
 import net.sourceforge.pmd.lang.java.ast.ASTArguments;
-import net.sourceforge.pmd.lang.java.ast.ASTExpression;
 import net.sourceforge.pmd.lang.java.ast.ASTFinallyStatement;
 import net.sourceforge.pmd.lang.java.ast.ASTName;
 import net.sourceforge.pmd.lang.java.ast.ASTTryStatement;
-import net.sourceforge.pmd.lang.java.ast.TypeNode;
+import net.sourceforge.pmd.lang.java.ast.ASTVariableDeclaratorId;
 import net.sourceforge.pmd.lang.java.rule.AbstractJavaRule;
+import net.sourceforge.pmd.lang.java.symboltable.MethodScope;
+import net.sourceforge.pmd.lang.java.symboltable.VariableNameDeclaration;
 import net.sourceforge.pmd.lang.java.types.TypeTestUtil;
+import net.sourceforge.pmd.lang.symboltable.NameDeclaration;
+import net.sourceforge.pmd.lang.symboltable.NameOccurrence;
 import net.sourceforge.pmd.properties.PropertyDescriptor;
 
 public final class UseTryWithResourcesRule extends AbstractJavaRule {
@@ -35,12 +40,15 @@ public final class UseTryWithResourcesRule extends AbstractJavaRule {
 
     @Override
     public Object visit(ASTTryStatement node, Object data) {
+        boolean isJava9OrLater = isJava9OrLater((RuleContext) data);
+
         ASTFinallyStatement finallyClause = node.getFinallyClause();
         if (finallyClause != null) {
             List<ASTName> methods = findCloseMethods(finallyClause.findDescendantsOfType(ASTName.class));
             for (ASTName method : methods) {
-                TypeNode typeNode = getType(method);
-                if (TypeTestUtil.isA(AutoCloseable.class, typeNode)) {
+                ASTName closeTarget = getCloseTarget(method);
+                if (TypeTestUtil.isA(AutoCloseable.class, closeTarget)
+                        && (isJava9OrLater || isLocalVar(closeTarget))) {
                     addViolation(data, node);
                     break; // only report the first closeable
                 }
@@ -49,10 +57,41 @@ public final class UseTryWithResourcesRule extends AbstractJavaRule {
         return data;
     }
 
-    private TypeNode getType(ASTName method) {
+    private boolean isJava9OrLater(RuleContext ruleContext) {
+        String currentVersion = ruleContext.getLanguageVersion().getVersion();
+        currentVersion = currentVersion.replace("-preview", "");
+        return Double.parseDouble(currentVersion) >= 9;
+    }
+
+    private boolean isLocalVar(ASTName closeTarget) {
+        NameDeclaration nameDeclaration = closeTarget.getNameDeclaration();
+        if (nameDeclaration instanceof VariableNameDeclaration) {
+            ASTVariableDeclaratorId id = ((VariableNameDeclaration) nameDeclaration).getDeclaratorId();
+            return id.isLocalVariable();
+        } else if (closeTarget.getImage().contains(".")) {
+            // this is a workaround for a bug in the symbol table:
+            // the name might be resolved to a wrong method
+            int lastDot = closeTarget.getImage().lastIndexOf('.');
+            String varName = closeTarget.getImage().substring(0, lastDot);
+            Map<VariableNameDeclaration, List<NameOccurrence>> vars = closeTarget.getScope()
+                    .getEnclosingScope(MethodScope.class)
+                    .getDeclarations(VariableNameDeclaration.class);
+            for (VariableNameDeclaration varDecl : vars.keySet()) {
+                if (varDecl.getName().equals(varName)) {
+                    return varDecl.getDeclaratorId().isLocalVariable();
+                }
+            }
+        }
+        return false;
+    }
+
+    private ASTName getCloseTarget(ASTName method) {
         ASTArguments arguments = method.getNthParent(2).getFirstDescendantOfType(ASTArguments.class);
         if (arguments.size() > 0) {
-            return (ASTExpression) arguments.getChild(0).getChild(0);
+            ASTName firstArgument = arguments.getChild(0).getChild(0).getFirstDescendantOfType(ASTName.class);
+            if (firstArgument != null) {
+                return firstArgument;
+            }
         }
 
         return method;

pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/UseTryWithResources.xml

@@ -215,4 +215,97 @@ public class TryWithResources {
 }
         ]]></code>
     </test-code>
+
+    <code-fragment id="issue-3235"><![CDATA[
+import java.io.IOException;
+import java.io.Reader;
+
+class Scratch {
+    public static int count(AutoCloseable iterator) {
+        int count = 0;
+        try {
+            count++;
+        } finally {
+            iterator.close();
+        }
+        return count;
+    }
+}
+
+class Holder implements AutoCloseable {
+   private Reader reader;
+   public void close() throws IOException {
+      try {
+         someOtherActivity();
+      } finally {
+         reader.close();
+      }
+   }
+   private void someOtherActivity() throws IOException {
+     // do stuff
+   }
+}
+    ]]></code-fragment>
+
+    <test-code>
+        <description>[java] UseTryWithResources false positive when closeable is provided as a method argument or class field #3235 before java 9</description>
+        <expected-problems>0</expected-problems>
+        <code-ref id="issue-3235"/>
+        <source-type>java 1.8</source-type>
+    </test-code>
+
+    <test-code>
+        <description>[java] UseTryWithResources false positive when closeable is provided as a method argument or class field #3235</description>
+        <expected-problems>2</expected-problems>
+        <expected-linenumbers>7,19</expected-linenumbers>
+        <code-ref id="issue-3235"/>
+    </test-code>
+
+    <code-fragment id="issue-3235-with-local-var"><![CDATA[
+import java.io.IOException;
+import java.io.Reader;
+
+class Scratch {
+    public static int count() {
+        AutoCloseable iterator;
+        int count = 0;
+        try {
+            count++;
+        } finally {
+            iterator.close();
+        }
+        return count;
+    }
+}
+
+class Holder implements AutoCloseable {
+   public void close() throws IOException {
+      Reader reader;
+      try {
+         someOtherActivity();
+      } finally {
+         reader.close();
+      }
+   }
+   private void someOtherActivity() throws IOException {
+     // do stuff
+   }
+}
+    ]]></code-fragment>
+
+    <test-code>
+        <description>[java] UseTryWithResources with local var and before java 9 #3235</description>
+        <expected-problems>2</expected-problems>
+        <expected-linenumbers>8,20</expected-linenumbers>
+        <code-ref id="issue-3235-with-local-var"/>
+        <source-type>java 1.8</source-type>
+    </test-code>
+
+    <test-code>
+        <description>[java] UseTryWithResources with local var and latest java #3235</description>
+        <expected-problems>2</expected-problems>
+        <expected-linenumbers>8,20</expected-linenumbers>
+        <code-ref id="issue-3235-with-local-var"/>
+    </test-code>
+
 </test-data>