From f8f6391eb67d69bb6c2e4151c58762e436a75e6c Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Thu, 8 Jul 2021 15:05:34 +0200 Subject: [PATCH] [java] UseTryWithResources false positive with not local vars when closeable is provided as a method argument or class field Fixes #3235 --- docs/pages/release_notes.md | 2 + .../UseTryWithResourcesRule.java | 51 ++++++++-- .../bestpractices/xml/UseTryWithResources.xml | 93 +++++++++++++++++++ 3 files changed, 140 insertions(+), 6 deletions(-) diff --git a/docs/pages/release_notes.md b/docs/pages/release_notes.md index b68134831d..bba16c8039 100644 --- a/docs/pages/release_notes.md +++ b/docs/pages/release_notes.md @@ -20,6 +20,8 @@ This is a {{ site.pmd.release_type }} release. * [#3329](https://github.com/pmd/pmd/issues/3329): \[apex] ApexCRUDViolation doesn't report SOQL for loops * core * [#3387](https://github.com/pmd/pmd/issues/3387): \[core] CPD should avoid unnecessary copies when running with --skip-lexical-errors +* java-bestpractices + * [#3235](https://github.com/pmd/pmd/issues/3235): \[java] UseTryWithResources false positive when closeable is provided as a method argument or class field ### API Changes diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/UseTryWithResourcesRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/UseTryWithResourcesRule.java index 4175f5a7cb..15ed87f05d 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/UseTryWithResourcesRule.java +++ b/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 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> 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; diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/UseTryWithResources.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/UseTryWithResources.xml index 0dfb27fad2..bc71f1e5e9 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/UseTryWithResources.xml +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/UseTryWithResources.xml @@ -215,4 +215,97 @@ public class TryWithResources { } ]]> + + + + + [java] UseTryWithResources false positive when closeable is provided as a method argument or class field #3235 before java 9 + 0 + + java 1.8 + + + + [java] UseTryWithResources false positive when closeable is provided as a method argument or class field #3235 + 2 + 7,19 + + + + + + + [java] UseTryWithResources with local var and before java 9 #3235 + 2 + 8,20 + + java 1.8 + + + + [java] UseTryWithResources with local var and latest java #3235 + 2 + 8,20 + + +