From ae0b9d62239cfdb09798f9461208b74211bc482b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Fournier?= Date: Wed, 3 Apr 2024 17:43:53 +0200 Subject: [PATCH 1/6] [java] Add rule UnnecessaryVarargsArrayCreation --- .../UnnecessaryVarargsArrayCreationRule.java | 41 +++++++++++++++++++ .../resources/category/java/bestpractices.xml | 18 ++++++++ .../UnnecessaryVarargsArrayCreationTest.java | 11 +++++ .../xml/UnnecessaryVarargsArrayCreation.xml | 38 +++++++++++++++++ sandbox/rset.xml | 4 ++ 5 files changed, 112 insertions(+) create mode 100644 pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/UnnecessaryVarargsArrayCreationRule.java create mode 100644 pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/bestpractices/UnnecessaryVarargsArrayCreationTest.java create mode 100644 pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/UnnecessaryVarargsArrayCreation.xml create mode 100644 sandbox/rset.xml diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/UnnecessaryVarargsArrayCreationRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/UnnecessaryVarargsArrayCreationRule.java new file mode 100644 index 0000000000..1f26577252 --- /dev/null +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/UnnecessaryVarargsArrayCreationRule.java @@ -0,0 +1,41 @@ +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.ASTBlock; +import net.sourceforge.pmd.lang.java.ast.ASTEnumConstant; +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.JTypeMirror; +import net.sourceforge.pmd.lang.java.types.OverloadSelectionResult; + +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; + } + + asCtx(data).addViolation(array); + } + + return null; + } +} diff --git a/pmd-java/src/main/resources/category/java/bestpractices.xml b/pmd-java/src/main/resources/category/java/bestpractices.xml index 03ed6fd517..85840d7e2e 100644 --- a/pmd-java/src/main/resources/category/java/bestpractices.xml +++ b/pmd-java/src/main/resources/category/java/bestpractices.xml @@ -1435,6 +1435,24 @@ class Foo{ ]]> + + + todo + + 3 + + + + + + + + Unnecessary in asList + 2 + 6,7 + + + + + Necessary array creation + 0 + + + + diff --git a/sandbox/rset.xml b/sandbox/rset.xml new file mode 100644 index 0000000000..bf127d111a --- /dev/null +++ b/sandbox/rset.xml @@ -0,0 +1,4 @@ + + foo + + \ No newline at end of file From fe0b4a9b36397467399f57a02422d484695d4523 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Fournier?= Date: Wed, 3 Apr 2024 17:59:19 +0200 Subject: [PATCH 2/6] Improve messages when varargs call is confusing --- .../UnnecessaryVarargsArrayCreationRule.java | 20 +++++++++++++++++- .../xml/UnnecessaryVarargsArrayCreation.xml | 21 +++++++++++++++++++ sandbox/rset.xml | 4 ---- 3 files changed, 40 insertions(+), 5 deletions(-) delete mode 100644 sandbox/rset.xml diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/UnnecessaryVarargsArrayCreationRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/UnnecessaryVarargsArrayCreationRule.java index 1f26577252..aedc331bd7 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/UnnecessaryVarargsArrayCreationRule.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/UnnecessaryVarargsArrayCreationRule.java @@ -9,8 +9,10 @@ import net.sourceforge.pmd.lang.java.ast.ASTEnumConstant; 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 { @@ -33,7 +35,23 @@ public class UnnecessaryVarargsArrayCreationRule extends AbstractJavaRulechainRu return null; } - asCtx(data).addViolation(array); + List 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 { + // just regular unnecessary + asCtx(data).addViolation(array); + } } return null; diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/UnnecessaryVarargsArrayCreation.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/UnnecessaryVarargsArrayCreation.xml index 8dfcdef2f2..ffa830d272 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/UnnecessaryVarargsArrayCreation.xml +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/UnnecessaryVarargsArrayCreation.xml @@ -35,4 +35,25 @@ ]]> + + + Confusing argument + 2 + + Unnecessary explicit varargs array creation + Unclear if a varargs or non-varargs call is intended. Cast to Object or Object[], or pass varargs parameters separately to clarify intent. + + + + diff --git a/sandbox/rset.xml b/sandbox/rset.xml deleted file mode 100644 index bf127d111a..0000000000 --- a/sandbox/rset.xml +++ /dev/null @@ -1,4 +0,0 @@ - - foo - - \ No newline at end of file From 1d18209d11eb4c0077d032fbb46a3b5f676f1ca7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Fournier?= Date: Wed, 3 Apr 2024 18:14:54 +0200 Subject: [PATCH 3/6] Doc --- .../resources/category/java/bestpractices.xml | 49 ++++++++++++++++--- 1 file changed, 43 insertions(+), 6 deletions(-) diff --git a/pmd-java/src/main/resources/category/java/bestpractices.xml b/pmd-java/src/main/resources/category/java/bestpractices.xml index 85840d7e2e..ef111645a0 100644 --- a/pmd-java/src/main/resources/category/java/bestpractices.xml +++ b/pmd-java/src/main/resources/category/java/bestpractices.xml @@ -1442,15 +1442,52 @@ class Foo{ class="net.sourceforge.pmd.lang.java.rule.bestpractices.UnnecessaryVarargsArrayCreationRule" externalInfoUrl="${pmd.website.baseurl}/pmd_rules_java_bestpractices.html#unnecessaryvarargsarraycreation"> - todo + 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"}); + ``` 3 - +import java.util.Arrays; + +class C { + static { + Arrays.asList(new String[]{"foo", "bar",}); + // should be + Arrays.asList("foo", "bar"); + } +} + ]]> From e524a60d288c3a31a753d2c059024a986211183c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Fournier?= Date: Wed, 3 Apr 2024 19:28:23 +0200 Subject: [PATCH 4/6] fix fp with array creation without elements --- .../UnnecessaryVarargsArrayCreationRule.java | 2 +- .../xml/UnnecessaryVarargsArrayCreation.xml | 16 ++++++++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/UnnecessaryVarargsArrayCreationRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/UnnecessaryVarargsArrayCreationRule.java index aedc331bd7..27e641247b 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/UnnecessaryVarargsArrayCreationRule.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/UnnecessaryVarargsArrayCreationRule.java @@ -48,7 +48,7 @@ public class UnnecessaryVarargsArrayCreationRule extends AbstractJavaRulechainRu "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 { + } else if (array.getArrayInitializer() != null) { // just regular unnecessary asCtx(data).addViolation(array); } diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/UnnecessaryVarargsArrayCreation.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/UnnecessaryVarargsArrayCreation.xml index ffa830d272..7bfe784785 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/UnnecessaryVarargsArrayCreation.xml +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/UnnecessaryVarargsArrayCreation.xml @@ -56,4 +56,20 @@ ]]> + + + Array creation without elements + 0 + + + From 3502e6460c3d6abca2fd24b6e298a35a89ec11bb Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Thu, 18 Apr 2024 16:37:10 +0200 Subject: [PATCH 5/6] Fix checkstyle --- .../bestpractices/UnnecessaryVarargsArrayCreationRule.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/UnnecessaryVarargsArrayCreationRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/UnnecessaryVarargsArrayCreationRule.java index 27e641247b..9ecb6e5374 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/UnnecessaryVarargsArrayCreationRule.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/UnnecessaryVarargsArrayCreationRule.java @@ -1,11 +1,13 @@ +/* + * 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.ASTBlock; -import net.sourceforge.pmd.lang.java.ast.ASTEnumConstant; import net.sourceforge.pmd.lang.java.ast.InvocationNode; import net.sourceforge.pmd.lang.java.ast.JavaNode; import net.sourceforge.pmd.lang.java.rule.AbstractJavaRulechainRule; From 915c5aa727c510f7b4462a5f07823acf2674400c Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Thu, 18 Apr 2024 16:41:54 +0200 Subject: [PATCH 6/6] [doc] Update release notes (#3216, #4919) --- docs/pages/release_notes.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/docs/pages/release_notes.md b/docs/pages/release_notes.md index 2df014843f..fb1f6b2194 100644 --- a/docs/pages/release_notes.md +++ b/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