Merge branch 'pr-2148'

[java] Fix false negative for StringInstantion with array access
2019-12-13 20:14:52 +01:00
parent 2b1e21bf11 b7d61fca94
commit da194bf427
6 changed files with 126 additions and 26 deletions
--- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/performance/StringInstantiationRule.java
+++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/performance/StringInstantiationRule.java
@ -6,12 +6,17 @@ package net.sourceforge.pmd.lang.java.rule.performance;

 import java.util.List;

+import net.sourceforge.pmd.lang.ast.Node;
 import net.sourceforge.pmd.lang.java.ast.ASTAdditiveExpression;
 import net.sourceforge.pmd.lang.java.ast.ASTAllocationExpression;
+import net.sourceforge.pmd.lang.java.ast.ASTArgumentList;
+import net.sourceforge.pmd.lang.java.ast.ASTArguments;
 import net.sourceforge.pmd.lang.java.ast.ASTArrayDimsAndInits;
 import net.sourceforge.pmd.lang.java.ast.ASTClassOrInterfaceType;
 import net.sourceforge.pmd.lang.java.ast.ASTExpression;
 import net.sourceforge.pmd.lang.java.ast.ASTName;
+import net.sourceforge.pmd.lang.java.ast.ASTPrimaryExpression;
+import net.sourceforge.pmd.lang.java.ast.ASTPrimarySuffix;
 import net.sourceforge.pmd.lang.java.rule.AbstractJavaRule;
 import net.sourceforge.pmd.lang.java.symboltable.TypedNameDeclaration;
 import net.sourceforge.pmd.lang.java.typeresolution.TypeHelper;
@ -19,6 +24,10 @@ import net.sourceforge.pmd.lang.symboltable.NameDeclaration;

 public class StringInstantiationRule extends AbstractJavaRule {

+    public StringInstantiationRule() {
+        addRuleChainVisit(ASTAllocationExpression.class);
+    }
+
    @Override
    public Object visit(ASTAllocationExpression node, Object data) {
        if (!(node.jjtGetChild(0) instanceof ASTClassOrInterfaceType)) {
@ -29,6 +38,11 @@ public class StringInstantiationRule extends AbstractJavaRule {
            return data;
        }

+        if (isArrayAccess(node)) {
+            addViolation(data, node);
+            return data;
+        }
+
        List<ASTExpression> exp = node.findDescendantsOfType(ASTExpression.class);
        if (exp.size() >= 2) {
            return data;
@ -55,4 +69,20 @@ public class StringInstantiationRule extends AbstractJavaRule {
        }
        return data;
    }
+
+    private boolean isArrayAccess(ASTAllocationExpression node) {
+        ASTArguments arguments = node.getFirstChildOfType(ASTArguments.class);
+        if (arguments == null || arguments.getArgumentCount() != 1) {
+            return false;
+        }
+
+        Node firstArg = arguments.getFirstChildOfType(ASTArgumentList.class).jjtGetChild(0);
+        ASTPrimaryExpression primary = firstArg.getFirstChildOfType(ASTPrimaryExpression.class);
+        if (primary == null || primary.getType() != String.class) {
+            return false;
+        }
+
+        ASTPrimarySuffix suffix = primary.getFirstChildOfType(ASTPrimarySuffix.class);
+        return suffix != null && suffix.isArrayDereference();
+    }
 }
--- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/typeresolution/ClassTypeResolver.java
+++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/typeresolution/ClassTypeResolver.java
@ -69,6 +69,7 @@ import net.sourceforge.pmd.lang.java.ast.ASTPreDecrementExpression;
 import net.sourceforge.pmd.lang.java.ast.ASTPreIncrementExpression;
 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.ASTPrimitiveType;
 import net.sourceforge.pmd.lang.java.ast.ASTReferenceType;
 import net.sourceforge.pmd.lang.java.ast.ASTRelationalExpression;
@ -951,8 +952,10 @@ public class ClassTypeResolver extends JavaParserVisitorAdapter {


            if (currentChild.getType() != null) {
-                // rollup type from the child: PrimaryPrefix -> PrimaryExpression
-                primaryNodeType = currentChild.getTypeDefinition();
+                // rollup type from the child: PrimaryPrefix/PrimarySuffx -> PrimaryExpression
+                if (primaryNodeType == null || !primaryNodeType.isArrayType()) {
+                    primaryNodeType = currentChild.getTypeDefinition();
+                }

                // if this expression is a method call, then make sure, PrimaryPrefix has the type
                // on which the method is executed (type of the target reference)
@ -969,6 +972,13 @@ public class ClassTypeResolver extends JavaParserVisitorAdapter {
                        name.setTypeDefinition(primaryNodeType);
                    }
                }
+
+                // maybe array access?
+                if (primaryNodeType != null && primaryNodeType.isArrayType()) {
+                    if (currentChild instanceof ASTPrimarySuffix && ((ASTPrimarySuffix) currentChild).isArrayDereference()) {
+                        primaryNodeType = JavaTypeDefinition.forClass(primaryNodeType.getType().getComponentType());
+                    }
+                }
            } else {
                // avoid falsely passing tests
                primaryNodeType = null;
@ -1066,6 +1076,14 @@ public class ClassTypeResolver extends JavaParserVisitorAdapter {
        return data;
    }

+    @Override
+    public Object visit(ASTPrimarySuffix node, Object data) {
+        super.visit(node, data);
+        rollupTypeUnary(node);
+
+        return data;
+    }
+
    @Override
    public Object visit(ASTTypeArgument node, Object data) {
        if (node.jjtGetNumChildren() == 0) { // if type argument is '?'
--- a/pmd-java/src/test/java/net/sourceforge/pmd/typeresolution/ClassTypeResolverTest.java
+++ b/pmd-java/src/test/java/net/sourceforge/pmd/typeresolution/ClassTypeResolverTest.java
@ -77,6 +77,7 @@ import net.sourceforge.pmd.typeresolution.testdata.AbstractReturnTypeUseCase;
 import net.sourceforge.pmd.typeresolution.testdata.AnonymousClassFromInterface;
 import net.sourceforge.pmd.typeresolution.testdata.AnonymousInnerClass;
 import net.sourceforge.pmd.typeresolution.testdata.AnoymousExtendingObject;
+import net.sourceforge.pmd.typeresolution.testdata.ArrayAccess;
 import net.sourceforge.pmd.typeresolution.testdata.ArrayListFound;
 import net.sourceforge.pmd.typeresolution.testdata.ArrayTypes;
 import net.sourceforge.pmd.typeresolution.testdata.ArrayVariableDeclaration;
@ -797,6 +798,22 @@ public class ClassTypeResolverTest {
        assertEquals("All expressions not tested", index, expressions.size());
    }

+    @Test
+    public void testArrayAccess() throws JaxenException {
+        List<ASTExpression> expressions = selectNodes(ArrayAccess.class, ASTExpression.class, "//VariableInitializer/Expression");
+
+        int index = 1;
+        // int aElement = a[0];
+        assertEquals(int.class, expressions.get(index).getType());
+
+        index += 2;
+        // Object bElement = b[0][0];
+        assertEquals(Object.class, expressions.get(index).getType());
+
+        index += 3;
+        // ArrayAccess cElement = c[0][0][0];
+        assertEquals(ArrayAccess.class, expressions.get(index).getType());
+    }

    @Test
    public void testReferenceType() {
--- a/pmd-java/src/test/java/net/sourceforge/pmd/typeresolution/testdata/ArrayAccess.java
+++ b/pmd-java/src/test/java/net/sourceforge/pmd/typeresolution/testdata/ArrayAccess.java
@ -0,0 +1,21 @@
+/**
+ * BSD-style license; for more info see http://pmd.sourceforge.net/license.html
+ */
+
+package net.sourceforge.pmd.typeresolution.testdata;
+
+public class ArrayAccess {
+
+    @SuppressWarnings("unused")
+    public void test() {
+        int[] a = new int[1];
+        int aElement = a[0];
+
+        Object[][] b = new Object[1][0];
+        Object bElement = b[0][0];
+
+        ArrayAccess[][][] c = new ArrayAccess[][][] { new ArrayAccess[1][2] };
+        ArrayAccess cElement = c[0][0][0];
+    }
+
+}
--- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/performance/xml/StringInstantiation.xml
+++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/performance/xml/StringInstantiation.xml
@ -3,10 +3,9 @@
    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><![CDATA[
-new 'new String's
-     ]]></description>
+        <description>new 'new String's</description>
        <expected-problems>2</expected-problems>
        <code><![CDATA[
 public class Foo {
@ -15,10 +14,9 @@ public class Foo {
 }
     ]]></code>
    </test-code>
+
    <test-code>
-        <description><![CDATA[
-new String array
-     ]]></description>
+        <description>new String array</description>
        <expected-problems>0</expected-problems>
        <code><![CDATA[
 public class Foo {
@ -26,10 +24,9 @@ public class Foo {
 }
     ]]></code>
    </test-code>
+
    <test-code>
-        <description><![CDATA[
-using multiple parameter constructor
-     ]]></description>
+        <description>using multiple parameter constructor</description>
        <expected-problems>0</expected-problems>
        <code><![CDATA[
 public class Foo {
@ -40,10 +37,9 @@ public class Foo {
 }
     ]]></code>
    </test-code>
+
    <test-code>
-        <description><![CDATA[
-using 4 parameter constructor
-     ]]></description>
+        <description>using 4 parameter constructor</description>
        <expected-problems>0</expected-problems>
        <code><![CDATA[
 public class Foo {
@ -54,10 +50,9 @@ public class Foo {
 }
     ]]></code>
    </test-code>
+
    <test-code>
-        <description><![CDATA[
-byte array constructor is ok
-     ]]></description>
+        <description>byte array constructor is ok</description>
        <expected-problems>0</expected-problems>
        <code><![CDATA[
 public class Foo {
@ -68,10 +63,9 @@ public class Foo {
 }
     ]]></code>
    </test-code>
+
    <test-code>
-        <description><![CDATA[
-Method returning new String
-     ]]></description>
+        <description>Method returning new String</description>
        <expected-problems>1</expected-problems>
        <code><![CDATA[
 public class Foo {
@ -81,10 +75,9 @@ public class Foo {
 }
     ]]></code>
    </test-code>
+
    <test-code>
-        <description><![CDATA[
-Not a new String
-     ]]></description>
+        <description>Not a new String</description>
        <expected-problems>0</expected-problems>
        <code><![CDATA[
 public class Foo {
@ -94,10 +87,9 @@ public class Foo {
 }
     ]]></code>
    </test-code>
+
    <test-code>
-        <description><![CDATA[
-Returns new String(str)
-     ]]></description>
+        <description>Returns new String(str)</description>
        <expected-problems>1</expected-problems>
        <code><![CDATA[
 public class Foo {
@ -107,4 +99,24 @@ public class Foo {
 }
     ]]></code>
    </test-code>
+
+    <test-code>
+        <description>#2141 [java] StringInstatiation: False negative with String-array access</description>
+        <expected-problems>2</expected-problems>
+        <expected-linenumbers>4,10</expected-linenumbers>
+        <code><![CDATA[
+public class Foo {
+    public void bar() {
+        String[] arr = getArray();
+        String s = new String(arr[0]);
+        // better
+        String s2 = arr[0];
+    }
+    public void bar2() {
+        String[][] arr = getArray2();
+        String s = new String(arr[0][0]);
+    }
+}
+        ]]></code>
+    </test-code>
 </test-data>