[java] New rule UnnecessaryVarargsArrayCreation (#4919, #3216)

Merge pull request #4919 from oowekyala:new-rule-UnnecessaryVarargsArrayCreation
Fixes #3216

docs/pages/release_notes.md

@@ -14,6 +14,11 @@ This is a {{ site.pmd.release_type }} release.
 
 ### 🚀 New and noteworthy
 
+### ✨ New rules
+
+- The new Java rule {%rule java/bestpractices/UnnecessaryVarargsArrayCreation %} reports explicit array creation
+  when a varargs is expected. This is more heavy to read and could be simplified.
+
 ### 🌟 Rule Changes
 
 * {%rule java/bestpractices/JUnitTestsShouldIncludeAssert %} and {% rule java/bestpractices/JUnitTestContainsTooManyAsserts %}
@@ -38,6 +43,7 @@ This is a {{ site.pmd.release_type }} release.
   * [#4947](https://github.com/pmd/pmd/issues/4947): \[java] Broken TextBlock parser
 * java-bestpractices
   * [#1084](https://github.com/pmd/pmd/issues/1084): \[java] Allow JUnitTestsShouldIncludeAssert to configure verification methods
+  * [#3216](https://github.com/pmd/pmd/issues/3216): \[java] New rule: UnnecessaryVarargsArrayCreation
   * [#4435](https://github.com/pmd/pmd/issues/4435): \[java] \[7.0-rc1] UnusedAssignment for used field
   * [#4569](https://github.com/pmd/pmd/issues/4569): \[java] ForLoopCanBeForeach reports on loop `for (int i = 0; i < list.size(); i += 2)`
   * [#4618](https://github.com/pmd/pmd/issues/4618): \[java] UnusedAssignment false positive with conditional assignments of fields

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

@@ -0,0 +1,61 @@
+/*
+ * BSD-style license; for more info see http://pmd.sourceforge.net/license.html
+ */
+
+package net.sourceforge.pmd.lang.java.rule.bestpractices;
+
+import java.util.List;
+
+import net.sourceforge.pmd.lang.java.ast.ASTArgumentList;
+import net.sourceforge.pmd.lang.java.ast.ASTArrayAllocation;
+import net.sourceforge.pmd.lang.java.ast.InvocationNode;
+import net.sourceforge.pmd.lang.java.ast.JavaNode;
+import net.sourceforge.pmd.lang.java.rule.AbstractJavaRulechainRule;
+import net.sourceforge.pmd.lang.java.types.JArrayType;
+import net.sourceforge.pmd.lang.java.types.JTypeMirror;
+import net.sourceforge.pmd.lang.java.types.OverloadSelectionResult;
+import net.sourceforge.pmd.lang.java.types.TypePrettyPrint;
+
+public class UnnecessaryVarargsArrayCreationRule extends AbstractJavaRulechainRule {
+
+    // we visit array allocations because they are less frequent than
+    // method calls
+    public UnnecessaryVarargsArrayCreationRule() {
+        super(ASTArrayAllocation.class);
+    }
+
+    @Override
+    public Object visit(ASTArrayAllocation array, Object data) {
+
+        JavaNode parent = array.getParent();
+        if (parent instanceof ASTArgumentList && array.getIndexInParent() == parent.getNumChildren() - 1) {
+            // node is the last param in an arguments list
+            InvocationNode call = (InvocationNode) parent.getParent();
+            OverloadSelectionResult info = call.getOverloadSelectionInfo();
+            if (info.isFailed() || info.isVarargsCall()
+                || !info.getMethodType().isVarargs()) {
+                return null;
+            }
+
+            List<JTypeMirror> formals = info.getMethodType().getFormalParameters();
+            JTypeMirror lastFormal = formals.get(formals.size() - 1);
+            JTypeMirror expectedComponent = ((JArrayType) lastFormal).getComponentType();
+
+            if (array.getTypeMirror().isSubtypeOf(expectedComponent)
+                && !array.getTypeMirror().equals(lastFormal)) {
+                // confusing
+                asCtx(data)
+                    .addViolationWithMessage(
+                        array,
+                        "Unclear if a varargs or non-varargs call is intended. Cast to {0} or {0}[], or pass varargs parameters separately to clarify intent.",
+                        TypePrettyPrint.prettyPrintWithSimpleNames(expectedComponent)
+                    );
+            } else if (array.getArrayInitializer() != null) {
+                // just regular unnecessary
+                asCtx(data).addViolation(array);
+            }
+        }
+
+        return null;
+    }
+}

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

@@ -1435,6 +1435,61 @@ class Foo{
 ]]>
         </example>
     </rule>
+    <rule name="UnnecessaryVarargsArrayCreation"
+          language="java"
+          since="7.1.0"
+          message="Unnecessary explicit varargs array creation"
+          class="net.sourceforge.pmd.lang.java.rule.bestpractices.UnnecessaryVarargsArrayCreationRule"
+          externalInfoUrl="${pmd.website.baseurl}/pmd_rules_java_bestpractices.html#unnecessaryvarargsarraycreation">
+        <description>
+            Reports explicit array creation when a varargs is expected.
+            For instance:
+            ```java
+            Arrays.asList(new String[] { "foo", "bar", });
+            ```
+            can be replaced by:
+            ```java
+            Arrays.asList("foo", "bar");
+            ```
+
+            This rule also reports such array creations when they are confusing, because the array is a subtype of the component type of the expected array type.
+            For instance if you have
+            ```java
+            void varargs(Object... parm);
+            ```
+            and call it like so
+            ```java
+            varargs(new String[]{"a"});
+            ```
+            it is not clear whether you intended the method to receive the value `new Object[]{ new String[] {"a"} }` or just `new String[] {"a"}` (the latter happens). This confusion occurs because `String[]` is both a subtype of `Object[]` and of `Object`. To clarify your intent in this case, use a cast or pass individual elements like so:
+            ```java
+            // varargs call
+            // parm will be `new Object[] { "a" }`
+            varargs("a");
+
+            // non-varargs call
+            // parm will be `new String[] { "a" }`
+            varargs((Object[]) new String[]{"a"});
+
+            // varargs call
+            // parm will be `new Object[] { new String[] { "a" } }`
+            varargs((Object) new String[]{"a"});
+            ```
+        </description>
+        <priority>3</priority>
+        <example><![CDATA[
+import java.util.Arrays;
+
+class C {
+    static {
+        Arrays.asList(new String[]{"foo", "bar",});
+        // should be
+        Arrays.asList("foo", "bar");
+    }
+}
+            ]]></example>
+    </rule>
+
 
     <rule name="UnusedAssignment"
           language="java"

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

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

@@ -0,0 +1,75 @@
+<?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>Unnecessary in asList</description>
+        <expected-problems>2</expected-problems>
+        <expected-linenumbers>6,7</expected-linenumbers>
+        <code><![CDATA[
+            import java.util.Arrays;
+
+            public class Foo {
+                static {
+                    Arrays.asList("a" , "b");
+                    Arrays.asList(new String[] { "a", "b" });
+                    Arrays.asList(new Object[] { "a", "b" });
+                }
+            }
+            ]]></code>
+    </test-code>
+
+    <test-code>
+        <description>Necessary array creation</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+            import java.util.Arrays;
+
+            public class Foo {
+                static {
+                    Arrays.asList(new String[]{""}, new String[] { "a", "b" });
+                }
+            }
+            ]]></code>
+    </test-code>
+
+
+    <test-code>
+        <description>Confusing argument</description>
+        <expected-problems>2</expected-problems>
+        <expected-messages>
+            <message>Unnecessary explicit varargs array creation</message>
+            <message>Unclear if a varargs or non-varargs call is intended. Cast to Object or Object[], or pass varargs parameters separately to clarify intent.</message>
+        </expected-messages>
+        <code><![CDATA[
+            import java.util.Arrays;
+
+            public class Foo {
+                static {
+                    foo(new Object[]{ "a" }); // regular unnecessary
+                    foo(new String[]{ "a" }); // confusing bc String[] <: Object
+                }
+                static void foo(Object... args) {}
+            }
+            ]]></code>
+    </test-code>
+
+
+    <test-code>
+        <description>Array creation without elements</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+            import java.util.Arrays;
+
+            public class Foo {
+                static {
+                    foo(new Object[4]);
+                }
+                static void foo(Object... args) {}
+            }
+            ]]></code>
+    </test-code>
+
+</test-data>