Merge branch 'pr-2618'

[java] New rule: UnusedAssignment #2618

docs/pages/release_notes.md

@@ -14,6 +14,12 @@ This is a {{ site.pmd.release_type }} release.
 
 ### New and noteworthy
 
+#### New Rules
+
+*   The new Java rule {% rule "java/bestpractices/UnusedAssignment" %} (`java-bestpractices`) finds assignments
+    to variables, that are never used and are useless. The new rule is supposed to entirely replace
+    {% rule "java/errorprone/DataflowAnomalyAnalysis" %}.
+
 ### Fixed Issues
 *   apex
     *   [#2610](https://github.com/pmd/pmd/pull/2610): \[apex] Support top-level enums in rules

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

@@ -0,0 +1,13 @@
+<?xml version="1.0"?>
+
+<ruleset name="6260"
+    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.26.0
+  </description>
+
+    <rule ref="category/java/bestpractices.xml/UnusedAssignment" />
+
+</ruleset>

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTAnnotation.java

@@ -26,7 +26,7 @@ import net.sourceforge.pmd.annotation.InternalApi;
 public class ASTAnnotation extends AbstractJavaTypeNode {
 
     private static final List<String> UNUSED_RULES
-        = Arrays.asList("UnusedPrivateField", "UnusedLocalVariable", "UnusedPrivateMethod", "UnusedFormalParameter");
+        = Arrays.asList("UnusedPrivateField", "UnusedLocalVariable", "UnusedPrivateMethod", "UnusedFormalParameter", "UnusedAssignment");
 
     private static final List<String> SERIAL_RULES = Arrays.asList("BeanMembersShouldSerialize", "MissingSerialVersionUID");
 

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTCatchStatement.java

@@ -101,4 +101,10 @@ public class ASTCatchStatement extends AbstractJavaNode {
         return getFirstDescendantOfType(ASTVariableDeclaratorId.class).getImage();
     }
 
+    /**
+     * Returns the declarator id for the exception parameter.
+     */
+    public ASTVariableDeclaratorId getExceptionId() {
+        return getFirstChildOfType(ASTFormalParameter.class).getVariableDeclaratorId();
+    }
 }

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTConditionalExpression.java

@@ -74,7 +74,7 @@ public class ASTConditionalExpression extends AbstractJavaTypeNode {
      * Returns the node that represents the guard of this conditional.
      * That is the expression before the '?'.
      */
-    public Node getCondition() {
+    public JavaNode getCondition() {
         return getChild(0);
     }
 

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTTryStatement.java

@@ -49,7 +49,7 @@ public class ASTTryStatement extends AbstractJavaNode {
      * Returns the body of this try statement.
      */
     public ASTBlock getBody() {
-        return (ASTBlock) getChild(1);
+        return (ASTBlock) getFirstChildOfType(ASTBlock.class);
     }
 
     /**

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/ConfusingTernaryRule.java

@@ -16,6 +16,7 @@ import net.sourceforge.pmd.lang.java.ast.ASTIfStatement;
 import net.sourceforge.pmd.lang.java.ast.ASTPrimaryExpression;
 import net.sourceforge.pmd.lang.java.ast.ASTPrimaryPrefix;
 import net.sourceforge.pmd.lang.java.ast.ASTUnaryExpressionNotPlusMinus;
+import net.sourceforge.pmd.lang.java.ast.JavaNode;
 import net.sourceforge.pmd.lang.java.rule.AbstractJavaRule;
 import net.sourceforge.pmd.properties.PropertyDescriptor;
 
@@ -65,9 +66,9 @@ public class ConfusingTernaryRule extends AbstractJavaRule {
     public Object visit(ASTIfStatement node, Object data) {
         // look for "if (match) ..; else .."
         if (node.getNumChildren() == 3) {
-            Node inode = node.getChild(0);
+            JavaNode inode = node.getChild(0);
             if (inode instanceof ASTExpression && inode.getNumChildren() == 1) {
-                Node jnode = inode.getChild(0);
+                JavaNode jnode = inode.getChild(0);
                 if (isMatch(jnode)) {
 
                     if (!getProperty(ignoreElseIfProperty)
@@ -85,7 +86,7 @@ public class ConfusingTernaryRule extends AbstractJavaRule {
     public Object visit(ASTConditionalExpression node, Object data) {
         // look for "match ? .. : .."
         if (node.getNumChildren() > 0) {
-            Node inode = node.getChild(0);
+            JavaNode inode = node.getChild(0);
             if (isMatch(inode)) {
                 addViolation(data, node);
             }
@@ -94,9 +95,9 @@ public class ConfusingTernaryRule extends AbstractJavaRule {
     }
 
     // recursive!
-    private static boolean isMatch(Node node) {
-        return isUnaryNot(node) || isNotEquals(node) || isConditionalWithAllMatches(node)
-                || isParenthesisAroundMatch(node);
+    private static boolean isMatch(JavaNode node) {
+        node = unwrapParentheses(node);
+        return isUnaryNot(node) || isNotEquals(node) || isConditionalWithAllMatches(node);
     }
 
     private static boolean isUnaryNot(Node node) {
@@ -109,7 +110,7 @@ public class ConfusingTernaryRule extends AbstractJavaRule {
         return node instanceof ASTEqualityExpression && "!=".equals(node.getImage());
     }
 
-    private static boolean isConditionalWithAllMatches(Node node) {
+    private static boolean isConditionalWithAllMatches(JavaNode node) {
         // look for "match && match" or "match || match"
         if (!(node instanceof ASTConditionalAndExpression) && !(node instanceof ASTConditionalOrExpression)) {
             return false;
@@ -119,7 +120,7 @@ public class ConfusingTernaryRule extends AbstractJavaRule {
             return false;
         }
         for (int i = 0; i < n; i++) {
-            Node inode = node.getChild(i);
+            JavaNode inode = node.getChild(i);
             // recurse!
             if (!isMatch(inode)) {
                 return false;
@@ -129,21 +130,31 @@ public class ConfusingTernaryRule extends AbstractJavaRule {
         return true;
     }
 
-    private static boolean isParenthesisAroundMatch(Node node) {
+    /**
+     * Extracts the outermost node that is not a parenthesized
+     * expression.
+     *
+     * @deprecated This is internal API, because it will be removed in PMD 7.
+     *     In PMD 7 there are no additional layers for parentheses in the Java tree.
+     */
+    @Deprecated
+    public static JavaNode unwrapParentheses(final JavaNode top) {
+        JavaNode node = top;
         // look for "(match)"
         if (!(node instanceof ASTPrimaryExpression) || node.getNumChildren() != 1) {
-            return false;
+            return top;
         }
-        Node inode = node.getChild(0);
-        if (!(inode instanceof ASTPrimaryPrefix) || inode.getNumChildren() != 1) {
-            return false;
+        node = node.getChild(0);
+        if (!(node instanceof ASTPrimaryPrefix) || node.getNumChildren() != 1) {
+            return top;
         }
-        Node jnode = inode.getChild(0);
-        if (!(jnode instanceof ASTExpression) || jnode.getNumChildren() != 1) {
-            return false;
+        node = node.getChild(0);
+        if (!(node instanceof ASTExpression) || node.getNumChildren() != 1) {
+            return top;
         }
-        Node knode = jnode.getChild(0);
-        // recurse!
-        return isMatch(knode);
+        node = node.getChild(0);
+
+        // recurse to unwrap another layer if possible
+        return unwrapParentheses(node);
     }
 }

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

@@ -1298,6 +1298,121 @@ class Foo{
         </example>
     </rule>
 
+    <rule name="UnusedAssignment"
+          language="java"
+          since="6.26.0"
+          message="The value assigned to this variable is never used or always overwritten"
+          class="net.sourceforge.pmd.lang.java.rule.bestpractices.UnusedAssignmentRule"
+          externalInfoUrl="${pmd.website.baseurl}/pmd_rules_java_bestpractices.html#unusedassignment">
+        <description>
+            Reports assignments to variables that are never used before the variable is overwritten,
+            or goes out of scope. Unused assignments are those for which
+            1. The variable is never read after the assignment, or
+            2. The assigned value is always overwritten by other assignments before the next read of
+            the variable.
+
+            The rule doesn't consider assignments to fields except for those of `this` in a constructor,
+            or static fields of the current class in static initializers.
+
+            The rule may be suppressed with the standard `@SuppressWarnings("unused")` tag.
+
+            The rule subsumes {% rule "UnusedLocalVariable" %}, and {% rule "UnusedFormalParameter" %}.
+            Those violations are filtered
+            out by default, in case you already have enabled those rules, but may be enabled with the property
+            `reportUnusedVariables`. Variables whose name starts with `ignored` are filtered out, as
+            is standard practice for exceptions.
+        </description>
+        <priority>3</priority>
+        <example>
+            <![CDATA[
+            class A {
+                // this field initializer is redundant,
+                // it is always overwritten in the constructor
+                int f = 1;
+
+                A(int f) {
+                    this.f = f;
+                }
+            }
+        ]]>
+        </example>
+        <example><![CDATA[
+class B {
+
+    int method(int i, int j) {
+        // this initializer is redundant,
+        // it is overwritten in all branches of the `if`
+        int k = 0;
+
+        // Both the assignments to k are unused, because k is
+        // not read after the if/else
+        // This may hide a bug: the programmer probably wanted to return k
+        if (i < j)
+            k = i;
+        else
+            k = j;
+
+        return j;
+    }
+
+}
+        ]]>
+
+        </example>
+        <example><![CDATA[
+class C {
+
+    int method() {
+        int i = 0;
+
+        checkSomething(++i);
+        checkSomething(++i);
+        checkSomething(++i);
+        checkSomething(++i);
+
+        // That last increment is not reported unless
+        // the property `checkUnusedPrefixIncrement` is
+        // set to `true`
+        // Technically it could be written (i+1), but it
+        // is not very important
+    }
+
+}
+        ]]>
+
+        </example>
+        <example><![CDATA[
+class C {
+
+    // variables that are truly unused (at most assigned to, but never accessed)
+    // are only reported if property `reportUnusedVariables` is true
+
+    void method(int param) { } // for example this method parameter
+
+    // even then, you can suppress the violation with an annotation:
+
+    void method(@SuppressWarning("unused") int param) { } // no violation, even if `reportUnusedVariables` is true
+
+    // For catch parameters, or for resources which don't need to be used explicitly,
+    // you can give a name that starts with "ignored" to ignore such warnings
+
+    {
+        try (Something ignored = Something.create()) {
+            // even if ignored is unused, it won't be flagged
+            // its purpose might be to side-effect in the create/close routines
+
+        } catch (Exception e) { // this is unused and will cause a warning if `reportUnusedVariables` is true
+            // you should choose a name that starts with "ignored"
+            return;
+        }
+    }
+
+}
+        ]]>
+
+        </example>
+    </rule>
+
     <rule name="UnusedFormalParameter"
           language="java"
           since="0.8"

pmd-java/src/main/resources/rulesets/java/quickstart.xml

@@ -41,6 +41,7 @@
     <!-- <rule ref="category/java/bestpractices.xml/ReplaceVectorWithList" /> -->
     <rule ref="category/java/bestpractices.xml/SwitchStmtsShouldHaveDefault"/>
     <!-- <rule ref="category/java/bestpractices.xml/SystemPrintln" /> -->
+    <!-- <rule ref="category/java/bestpractices.xml/UnusedAssignment"/> -->
     <rule ref="category/java/bestpractices.xml/UnusedFormalParameter"/>
     <rule ref="category/java/bestpractices.xml/UnusedImports"/>
     <rule ref="category/java/bestpractices.xml/UnusedLocalVariable"/>

pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/bestpractices/UnusedAssignmentTest.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 UnusedAssignmentTest extends PmdRuleTst {
+    // no additional unit tests
+}