[java] AvoidInstantiatingObjectsInLoops - fix false positives/negatives

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/performance/AvoidInstantiatingObjectsInLoopsRule.java

@@ -9,6 +9,7 @@ import java.util.Collection;
 import net.sourceforge.pmd.lang.ast.Node;
 import net.sourceforge.pmd.lang.java.ast.ASTArgumentList;
 import net.sourceforge.pmd.lang.java.ast.ASTArrayAccess;
+import net.sourceforge.pmd.lang.java.ast.ASTArrayAllocation;
 import net.sourceforge.pmd.lang.java.ast.ASTAssignmentExpression;
 import net.sourceforge.pmd.lang.java.ast.ASTBlock;
 import net.sourceforge.pmd.lang.java.ast.ASTBreakStatement;
@@ -27,7 +28,7 @@ import net.sourceforge.pmd.lang.java.types.TypeTestUtil;
 public class AvoidInstantiatingObjectsInLoopsRule extends AbstractJavaRulechainRule {
 
     public AvoidInstantiatingObjectsInLoopsRule() {
-        super(ASTConstructorCall.class);
+        super(ASTConstructorCall.class, ASTArrayAllocation.class);
     }
 
     /**
@@ -53,22 +54,37 @@ public class AvoidInstantiatingObjectsInLoopsRule extends AbstractJavaRulechainR
         return data;
     }
 
+    @Override
+    public Object visit(ASTArrayAllocation node, Object data) {
+        if (notInsideLoop(node)) {
+            return data;
+        }
+
+        if (notCollectionAccess(node)) {
+            addViolation(data, node);
+        }
+
+        return data;
+    }
+
     private boolean notArrayAssignment(ASTConstructorCall node) {
-        if (node.getParent() instanceof ASTAssignmentExpression) {
-            if (node.getIndexInParent() == 1) {
-                Node assignee = node.getParent().getFirstChild();
-                return !(assignee instanceof ASTArrayAccess);
-            }
+        JavaNode childOfAssignment = node.ancestorsOrSelf()
+                .filter(n -> n.getParent() instanceof ASTAssignmentExpression).first();
+
+        if (childOfAssignment != null && childOfAssignment.getIndexInParent() == 1) {
+            Node assignee = childOfAssignment.getParent().getFirstChild();
+            return !(assignee instanceof ASTArrayAccess);
         }
         return true;
     }
 
-    private boolean notCollectionAccess(ASTConstructorCall node) {
-        if (node.getParent() instanceof ASTArgumentList && node.getNthParent(2) instanceof ASTMethodCall) {
-            ASTMethodCall methodCall = (ASTMethodCall) node.getNthParent(2);
-            return !TypeTestUtil.isA(Collection.class, methodCall.getQualifier());
-        }
-        return true;
+    private boolean notCollectionAccess(JavaNode node) {
+        // checks whether the given ConstructorCall/ArrayAllocation is
+        // part of a MethodCall on a Collection.
+        return node.ancestors(ASTArgumentList.class)
+            .filter(n -> n.getParent() instanceof ASTMethodCall)
+            .filter(n -> TypeTestUtil.isA(Collection.class, ((ASTMethodCall) n.getParent()).getQualifier()))
+            .isEmpty();
     }
 
     private boolean notBreakFollowing(ASTConstructorCall node) {
@@ -106,7 +122,7 @@ public class AvoidInstantiatingObjectsInLoopsRule extends AbstractJavaRulechainR
      * @param node This is the expression of part of java code to be checked.
      * @return boolean <code>false</code> if the given node is inside a loop, <code>true</code> otherwise
      */
-    private boolean notInsideLoop(ASTConstructorCall node) {
+    private boolean notInsideLoop(Node node) {
         Node n = node;
         while (n != null) {
             if (n instanceof ASTLoopStatement) {

pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/performance/xml/AvoidInstantiatingObjectsInLoops.xml

@@ -287,6 +287,94 @@ public class Sample {
 
     private static class Car {
     }
+}
+        ]]></code>
+    </test-code>
+
+    <test-code>
+        <description>False negative with array allocations within loops</description>
+        <expected-problems>1</expected-problems>
+        <expected-linenumbers>4</expected-linenumbers>
+        <code><![CDATA[
+public class AvoidInstantiatingArraysInLoops {
+    public static void main(String[] args) {
+        for (String arg : args) {
+            String[] copy = new String[] { arg };
+        }
+    }
+}
+        ]]></code>
+    </test-code>
+
+    <test-code>
+        <description>False positive with for-each loop over new array</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+public class AvoidInstantiatingArrayInLoops {
+    public static void main(String[] args) {
+        for (String configName : new String[] {"config", "test"}) {
+            System.out.println(configName);
+        }
+    }
+}
+        ]]></code>
+    </test-code>
+
+    <test-code>
+        <description>False positive with adding to a collection/array field</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+import java.util.ArrayList;
+import java.util.List;
+
+public class AvoidInstantiatingObjectsInLoops {
+    private List<String> field = new ArrayList<>();
+    private String[] arrayField = new String[10];
+
+    public static void main(String[] args) {
+        for (String arg : args) {
+            this.field.add(new String(arg));
+        }
+        for (int i = 0; i < args.length; i++) {
+            this.arrayField[i] = new String(args[i]);
+        }
+    }
+}
+        ]]></code>
+    </test-code>
+
+    <test-code>
+        <description>False positive with adding wrapped new arrays to collection</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+import java.nio.ByteBuffer;
+import java.nio.charset.StandardCharsets;
+import java.util.ArrayList;
+import java.util.List;
+
+public class AvoidInstantiatingObjectsInLoops {
+    public void testBytes(String s) {
+        byte[] bytes = s.getBytes(StandardCharsets.UTF_8);
+        List<ByteBuffer> buffers = new ArrayList<>();
+        for (byte b : bytes) {
+            buffers.add(ByteBuffer.wrap(new byte[]{b}));
+        }
+    }
+}
+        ]]></code>
+    </test-code>
+
+    <test-code>
+        <description>False positive with temporary object assigned to an array</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+public class AvoidInstantiatingObjectsInLoops {
+    public static void main(String[] args) {
+        String[] data = new String[10];
+        for (int i = 0; i < data.length; i++) {
+            data[i] = new StringBuilder().append("foo").toString();
+        }
+    }
 }
         ]]></code>
     </test-code>