Merge branch 'pr-2682'

[java] New Rule: AvoidReassigningCatchVariables #2682

docs/pages/release_notes.md

@@ -14,12 +14,19 @@ This is a {{ site.pmd.release_type }} release.
 
 ### New and noteworthy
 
+#### New Rules
+
+*   The new Java rule {% rule "java/bestpractices/AvoidReassigningCatchVariables" %} (`java-bestpractices`) finds
+    cases where the variable of the caught exception is reassigned. This practice is surprising and prevents
+    further evolution of the code like multi-catch.
+
 ### Fixed Issues
 
 *   core
     *   [#724](https://github.com/pmd/pmd/issues/724): \[core] Avoid parsing rulesets multiple times
     *   [#2653](https://github.com/pmd/pmd/issues/2653): \[lang-test] Upgrade kotlintest to Kotest
 *   java-bestpractices
+    *   [#2471](https://github.com/pmd/pmd/issues/2471): \[java] New Rule: AvoidReassigningCatchVariables
     *   [#2668](https://github.com/pmd/pmd/issues/2668): \[java] UnusedAssignment false positives
 *   java-errorprone
     *   [#2431](https://github.com/pmd/pmd/issues/2431): \[java] InvalidLogMessageFormatRule throws IndexOutOfBoundsException when only logging exception message
@@ -46,6 +53,7 @@ This is a {{ site.pmd.release_type }} release.
 *   [#2677](https://github.com/pmd/pmd/pull/2677): \[java] RedundantFieldInitializer can not detect a special case for char initialize: `char foo = '\0';` - [Mykhailo Palahuta](https://github.com/Drofff)
 *   [#2678](https://github.com/pmd/pmd/pull/2678): \[java] AvoidCatchingThrowable can not detect the case: catch (java.lang.Throwable t) - [Mykhailo Palahuta](https://github.com/Drofff)
 *   [#2679](https://github.com/pmd/pmd/pull/2679): \[java] InvalidLogMessageFormatRule throws IndexOutOfBoundsException when only logging exception message - [Mykhailo Palahuta](https://github.com/Drofff)
+*   [#2682](https://github.com/pmd/pmd/pull/2682): \[java] New Rule: AvoidReassigningCatchVariables - [Mykhailo Palahuta](https://github.com/Drofff)
 
 
 {% endtocmaker %}

pmd-core/src/main/resources/rulesets/releases/6270.xml

@@ -0,0 +1,13 @@
+<?xml version="1.0"?>
+
+<ruleset name="6270"
+    xmlns="http://pmd.sourceforge.net/ruleset/2.0.0"
+    xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+    xsi:schemaLocation="http://pmd.sourceforge.net/ruleset/2.0.0 https://pmd.sourceforge.io/ruleset_2_0_0.xsd">
+  <description>
+This ruleset contains links to rules that are new in PMD v6.27.0
+  </description>
+
+    <rule ref="category/java/bestpractices.xml/AvoidReassigningCatchVariables" />
+
+</ruleset>

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

@@ -0,0 +1,51 @@
+/**
+ * BSD-style license; for more info see http://pmd.sourceforge.net/license.html
+ */
+
+package net.sourceforge.pmd.lang.java.rule.bestpractices;
+
+import net.sourceforge.pmd.lang.java.ast.ASTAssignmentOperator;
+import net.sourceforge.pmd.lang.java.ast.ASTCatchStatement;
+import net.sourceforge.pmd.lang.java.ast.ASTName;
+import net.sourceforge.pmd.lang.java.ast.ASTPrimaryExpression;
+import net.sourceforge.pmd.lang.java.ast.ASTVariableDeclaratorId;
+import net.sourceforge.pmd.lang.java.ast.JavaNode;
+import net.sourceforge.pmd.lang.java.rule.AbstractJavaRule;
+import net.sourceforge.pmd.lang.symboltable.NameOccurrence;
+
+public class AvoidReassigningCatchVariablesRule extends AbstractJavaRule {
+
+    public AvoidReassigningCatchVariablesRule() {
+        addRuleChainVisit(ASTCatchStatement.class);
+    }
+
+    @Override
+    public Object visit(ASTCatchStatement catchStatement, Object data) {
+        ASTVariableDeclaratorId caughtExceptionId = catchStatement.getExceptionId();
+        String caughtExceptionVar = caughtExceptionId.getName();
+        for (NameOccurrence usage : caughtExceptionId.getUsages()) {
+            JavaNode operation = getOperationOfUsage(usage);
+            if (isAssignment(operation)) {
+                String assignedVar = getAssignedVariableName(operation);
+                if (caughtExceptionVar.equals(assignedVar)) {
+                    addViolation(data, operation, caughtExceptionVar);
+                }
+            }
+        }
+        return data;
+    }
+
+    private JavaNode getOperationOfUsage(NameOccurrence usage) {
+        return usage.getLocation()
+                .getFirstParentOfType(ASTPrimaryExpression.class)
+                .getParent();
+    }
+
+    private boolean isAssignment(JavaNode operation) {
+        return operation.hasDescendantOfType(ASTAssignmentOperator.class);
+    }
+
+    private String getAssignedVariableName(JavaNode operation) {
+        return operation.getFirstDescendantOfType(ASTName.class).getImage();
+    }
+}

pmd-java/src/main/resources/category/java/bestpractices.xml

@@ -211,6 +211,36 @@ class Foo {
         </example>
     </rule>
 
+    <rule name="AvoidReassigningCatchVariables"
+          since="6.27.0"
+          message="Avoid reassigning caught exception ''{0}''"
+          class="net.sourceforge.pmd.lang.java.rule.bestpractices.AvoidReassigningCatchVariablesRule"
+          externalInfoUrl="${pmd.website.baseurl}/pmd_rules_java_bestpractices.html#avoidreassigningcatchvariables">
+        <description>
+Reassigning exception variables caught in a catch statement should be avoided because of:
+1) If it is needed, multi catch can be easily added and code will still compile
+2) Following the principle of least surprise we want to make sure that a variable caught in a catch statement is always the one thrown in a try block
+        </description>
+        <priority>3</priority>
+        <example><![CDATA[
+public class Foo {
+    public void foo() {
+        try {
+            // do something
+        } catch (Exception e) {
+            e = new NullPointerException(); // not recommended
+        }
+
+        try {
+            // do something
+        } catch (MyException | ServerException e) {
+            e = new RuntimeException(); // won't compile
+        }
+    }
+}
+        ]]></example>
+    </rule>
+
     <rule name="AvoidReassigningLoopVariables"
           language="java"
           since="6.11.0"

pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/bestpractices/AvoidReassigningCatchVariablesTest.java

@@ -0,0 +1,11 @@
+/**
+ * BSD-style license; for more info see http://pmd.sourceforge.net/license.html
+ */
+
+package net.sourceforge.pmd.lang.java.rule.bestpractices;
+
+import net.sourceforge.pmd.testframework.PmdRuleTst;
+
+public class AvoidReassigningCatchVariablesTest extends PmdRuleTst {
+    // no additional unit tests
+}

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

@@ -0,0 +1,44 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<test-data
+        xmlns="http://pmd.sourceforge.net/rule-tests"
+        xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+        xsi:schemaLocation="http://pmd.sourceforge.net/rule-tests http://pmd.sourceforge.net/rule-tests_1_0_0.xsd">
+
+    <test-code>
+        <description>simple catch variable reassignment</description>
+        <expected-problems>1</expected-problems>
+        <expected-linenumbers>8</expected-linenumbers>
+        <code><![CDATA[
+import java.io.File;
+import java.io.IOException;
+public class Foo {
+    public void foo() {
+        try {
+            new File("/text.txt");
+        } catch (IOException e) {
+            e = new NullPointerException();
+        }
+    }
+}
+        ]]></code>
+    </test-code>
+
+    <test-code>
+        <description>new exception variable allocation is OK</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+import java.io.File;
+import java.io.IOException;
+public class Foo {
+    public void foo() {
+        try {
+            new File("/text.txt");
+        } catch (IOException e) {
+            Exception t;
+            t = new NullPointerException();
+        }
+    }
+}
+        ]]></code>
+    </test-code>
+</test-data>