Merge pull request #3930 from adangel:improve-ArrayIsStoredDirectly

[java] Improve rule ArrayIsStoredDirectly #3930
2022-04-28 14:54:15 +02:00
parent 4d78148504 f27c6453ca
commit 69c7f4b2ad
3 changed files with 100 additions and 10 deletions
--- a/docs/pages/release_notes.md
+++ b/docs/pages/release_notes.md
@@ -34,6 +34,11 @@ This is a {{ site.pmd.release_type }} release.
  * [#3068](https://github.com/pmd/pmd/issues/3068): \[java] Some tests should not depend on real rules
  * [#3889](https://github.com/pmd/pmd/pull/3889): \[java] Catch LinkageError in UselessOverridingMethodRule
  * [#3910](https://github.com/pmd/pmd/pull/3910): \[java] UnusedPrivateField - Allow the ignored fieldnames to be configurable
+* java-bestpractices
+  * [#1185](https://github.com/pmd/pmd/issues/1185): \[java] ArrayIsStoredDirectly false positive with field access
+  * [#1474](https://github.com/pmd/pmd/issues/1474): \[java] ArrayIsStoredDirectly false positive with method call
+  * [#3879](https://github.com/pmd/pmd/issues/3879) \[java] ArrayIsStoredDirectly reports duplicated violation
+  * [#3929](https://github.com/pmd/pmd/issues/3929): \[java] ArrayIsStoredDirectly should report the assignment rather than formal parameter
 * java-performance
  * [#3867](https://github.com/pmd/pmd/issues/3867): \[java] UseArraysAsList with method call
 * plsql
--- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/ArrayIsStoredDirectlyRule.java
+++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/ArrayIsStoredDirectlyRule.java
@@ -7,6 +7,7 @@ package net.sourceforge.pmd.lang.java.rule.bestpractices;
 import java.util.ArrayList;
 import java.util.List;

+import net.sourceforge.pmd.RuleContext;
 import net.sourceforge.pmd.lang.ast.Node;
 import net.sourceforge.pmd.lang.java.ast.ASTAssignmentOperator;
 import net.sourceforge.pmd.lang.java.ast.ASTBlockStatement;
@@ -19,6 +20,7 @@ import net.sourceforge.pmd.lang.java.ast.ASTMethodDeclaration;
 import net.sourceforge.pmd.lang.java.ast.ASTPrimaryExpression;
 import net.sourceforge.pmd.lang.java.ast.ASTPrimaryPrefix;
 import net.sourceforge.pmd.lang.java.ast.ASTPrimarySuffix;
+import net.sourceforge.pmd.lang.java.ast.ASTStatement;
 import net.sourceforge.pmd.lang.java.ast.ASTStatementExpression;
 import net.sourceforge.pmd.lang.java.ast.ASTVariableDeclaratorId;
 import net.sourceforge.pmd.properties.PropertyDescriptor;
@@ -79,11 +81,19 @@ public class ArrayIsStoredDirectlyRule extends AbstractSunSecureRule {

    private String getExpressionVarName(Node e) {
        String assignedVar = getFirstNameImage(e);
+        if (isMethodCall(e)) {
+            return null;
+        }
        if (assignedVar == null) {
-            ASTPrimarySuffix suffix = e.getFirstDescendantOfType(ASTPrimarySuffix.class);
+            ASTPrimaryPrefix prefix = null;
+            ASTPrimarySuffix suffix = null;
+            if (e.getNumChildren() > 0 && e.getChild(0) instanceof ASTPrimaryExpression) {
+                ASTPrimaryExpression primaryExpression = (ASTPrimaryExpression) e.getChild(0);
+                prefix = (ASTPrimaryPrefix) primaryExpression.getChild(0);
+                suffix = primaryExpression.getFirstChildOfType(ASTPrimarySuffix.class);
+            }
            if (suffix != null) {
                assignedVar = suffix.getImage();
-                ASTPrimaryPrefix prefix = e.getFirstDescendantOfType(ASTPrimaryPrefix.class);
                if (prefix != null) {
                    if (prefix.usesThisModifier()) {
                        assignedVar = "this." + assignedVar;
@@ -96,18 +106,32 @@ public class ArrayIsStoredDirectlyRule extends AbstractSunSecureRule {
        return assignedVar;
    }

+    private boolean isMethodCall(Node e) {
+        if (e.getNumChildren() == 1 && e.getChild(0) instanceof ASTPrimaryExpression) {
+            ASTPrimaryExpression primaryExpression = (ASTPrimaryExpression) e.getChild(0);
+            if (primaryExpression.getNumChildren() > 1) {
+                ASTPrimarySuffix suffix = (ASTPrimarySuffix) primaryExpression.getChild(1);
+                return suffix.isArguments();
+            }
+        }
+        return false;
+    }
+
    /**
     * Checks if the variable designed in parameter is written to a field (not
     * local variable) in the statements.
     */
-    private boolean checkForDirectAssignment(Object ctx, final ASTFormalParameter parameter,
+    private void checkForDirectAssignment(Object ctx, final ASTFormalParameter parameter,
            final List<ASTBlockStatement> bs) {
        final ASTVariableDeclaratorId vid = parameter.getFirstDescendantOfType(ASTVariableDeclaratorId.class);
-        final String varName = vid.getImage();
+        final String varName = vid.getName();
        for (ASTBlockStatement b : bs) {
-            if (b.hasDescendantOfType(ASTAssignmentOperator.class)) {
+            if (b.getChild(0) instanceof ASTStatement && b.getChild(0).getChild(0) instanceof ASTStatementExpression) {
                final ASTStatementExpression se = b.getFirstDescendantOfType(ASTStatementExpression.class);
-                if (se == null || !(se.getChild(0) instanceof ASTPrimaryExpression)) {
+                if (se == null
+                        || se.getNumChildren() < 2
+                        || !(se.getChild(0) instanceof ASTPrimaryExpression)
+                        || !(se.getChild(1) instanceof ASTAssignmentOperator)) {
                    continue;
                }
                String assignedVar = getExpressionVarName(se);
@@ -150,13 +174,13 @@ public class ArrayIsStoredDirectlyRule extends AbstractSunSecureRule {
                            md = pe.getFirstParentOfType(ASTConstructorDeclaration.class);
                        }
                        if (!isLocalVariable(varName, md)) {
-                            addViolation(ctx, parameter, varName);
+                            RuleContext ruleContext = (RuleContext) ctx;
+                            ruleContext.addViolation(e, varName);
                        }
                    }
                }
            }
        }
-        return false;
    }

    private ASTFormalParameter[] getArrays(ASTFormalParameters params) {
@@ -164,7 +188,7 @@ public class ArrayIsStoredDirectlyRule extends AbstractSunSecureRule {
        if (l != null && !l.isEmpty()) {
            List<ASTFormalParameter> l2 = new ArrayList<>();
            for (ASTFormalParameter fp : l) {
-                if (fp.isArray() || fp.isVarargs()) {
+                if (fp.getVariableDeclaratorId().hasArrayType() || fp.isVarargs()) {
                    l2.add(fp);
                }
            }
--- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/ArrayIsStoredDirectly.xml
+++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/ArrayIsStoredDirectly.xml
@@ -7,6 +7,7 @@
    <test-code>
        <description>Clear violation</description>
        <expected-problems>1</expected-problems>
+        <expected-linenumbers>3</expected-linenumbers>
        <code><![CDATA[
 public class Foo {
    String [] arr;
@@ -18,6 +19,7 @@ public class Foo {
    <test-code>
        <description>Clear violation with this.</description>
        <expected-problems>1</expected-problems>
+        <expected-linenumbers>3</expected-linenumbers>
        <code><![CDATA[
 public class Foo {
    String [] arr;
@@ -29,6 +31,7 @@ public class Foo {
    <test-code>
        <description>assignment to an internal array</description>
        <expected-problems>1</expected-problems>
+        <expected-linenumbers>3</expected-linenumbers>
        <code><![CDATA[
 public class Foo {
    String [] arr;
@@ -96,6 +99,7 @@ public class Foo {
    <test-code>
        <description>Constructor clear violation</description>
        <expected-problems>1</expected-problems>
+        <expected-linenumbers>3</expected-linenumbers>
        <code><![CDATA[
 public class Foo {
    String [] arr;
@@ -129,6 +133,7 @@ public class Foo {
    <test-code>
        <description>Trigger on Varargs</description>
        <expected-problems>1</expected-problems>
+        <expected-linenumbers>3</expected-linenumbers>
        <code><![CDATA[
 public class Foo {
    String [] arr;
@@ -222,7 +227,7 @@ public class AISD {
        <description>do not allow private methods</description>
        <rule-property name="allowPrivate">false</rule-property>
        <expected-problems>2</expected-problems>
-        <expected-linenumbers>3,6</expected-linenumbers>
+        <expected-linenumbers>4,7</expected-linenumbers>
        <code><![CDATA[
 public class AISD {
    private final byte[] buf;
@@ -269,4 +274,60 @@ public class Foo {
 }
        ]]></code>
    </test-code>
+
+    <test-code>
+        <description>[java] ArrayIsStoredDirectly reports duplicated violation #3879</description>
+        <expected-problems>1</expected-problems>
+        <expected-linenumbers>7</expected-linenumbers>
+        <code><![CDATA[
+public class ExampleAISD {
+
+    public String[] strs;
+
+    public void func(String[] strs, boolean flag) { // line 5
+        if (flag) {
+            this.strs = strs; // line 7
+        }
+    }
+}
+]]></code>
+    </test-code>
+
+    <test-code>
+        <description>[java] ArrayIsStoredDirectly false positive with method call #1474</description>
+        <rule-property name="allowPrivate">false</rule-property>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+public class TestArrayIsStoredDirectly {
+    public double energy(int x) {
+        return 0.0;
+    }
+
+    // if energy[] is replace by energy2[], then no ArrayIsStoredDirectly warning
+    private void f(double[] energy) {
+        energy[0] = energy(1);
+    }
+}
+]]></code>
+    </test-code>
+
+    <test-code>
+        <description>[java] ArrayIsStoredDirectly false positive with field access #1185</description>
+        <rule-property name="allowPrivate">false</rule-property>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+class TestArrayIsStoredDirectly {
+    private final boolean[] a;
+    private final foo b;
+
+    private TestArrayIsStoredDirectly(boolean[] a) {
+        this.a = null;
+        this.a = a.clone(); // no violation, it doesn't matter what the state of this.a is
+        this.b = new TestArrayIsStoredDirectly(a); // no violation
+        this.b = new TestArrayIsStoredDirectly(this.a); // false positive violation here
+    }
+}
+]]></code>
+    </test-code>
+
 </test-data>