From 25ecf938cb47926f73cba801499c3c7a9aabaca7 Mon Sep 17 00:00:00 2001 From: Fer Madeiral Date: Wed, 16 Jun 2021 13:58:34 +0200 Subject: [PATCH 1/3] Add CollectionsEmptyConstantsProcessor, tests, and documentation --- docs/HANDLED_RULES.md | 17 ++++++++++ src/main/java/sorald/Processors.java | 3 +- .../CollectionsEmptyConstantsProcessor.java | 33 +++++++++++++++++++ .../CollectionsEmptyConstants.java | 12 +++++++ .../CollectionsEmptyConstants.java.expected | 12 +++++++ 5 files changed, 76 insertions(+), 1 deletion(-) create mode 100644 src/main/java/sorald/processor/CollectionsEmptyConstantsProcessor.java create mode 100644 src/test/resources/processor_test_files/1596_CollectionsEmptyConstants/CollectionsEmptyConstants.java create mode 100644 src/test/resources/processor_test_files/1596_CollectionsEmptyConstants/CollectionsEmptyConstants.java.expected diff --git a/docs/HANDLED_RULES.md b/docs/HANDLED_RULES.md index 0bcc96bd1..beb6e0e49 100644 --- a/docs/HANDLED_RULES.md +++ b/docs/HANDLED_RULES.md @@ -24,6 +24,7 @@ Sorald can currently repair violations of the following rules: * ["toString()" and "clone()" methods should not return null](#tostring-and-clone-methods-should-not-return-null-sonar-rule-2225) ([Sonar Rule 2225](https://rules.sonarsource.com/java/tag/multi-threading/RSPEC-2225)) * [Code Smell](#code-smell) * [Collection.isEmpty() should be used to test for emptiness](#collectionisempty-should-be-used-to-test-for-emptiness-sonar-rule-1155httpsrulessonarsourcecomjavarspec-1155) ([Sonar Rule 1155](https://rules.sonarsource.com/java/RSPEC-1155)) + * ["Collections.EMPTY_LIST", "EMPTY_MAP", and "EMPTY_SET" should not be used](#collectionsempty_list-empty_map-and-empty_set-should-not-be-used-sonar-rule-1596httpsrulessonarsourcecomjavarspec-1596) ([Sonar Rule 1596](https://rules.sonarsource.com/java/RSPEC-1596)) * [Fields in a "Serializable" class should either be transient or serializable](#fields-in-a-serializable-class-should-either-be-transient-or-serializable-sonar-rule-1948) ([Sonar Rule 1948](https://rules.sonarsource.com/java/RSPEC-1948)) * [Unused assignments should be removed](#unused-assignments-should-be-removed-sonar-rule-1854) ([Sonar Rule 1854](https://rules.sonarsource.com/java/RSPEC-1854)) * [Unused local variables should be removed](#unused-local-variables-should-be-removed-sonar-rule-1481) ([Sonar Rule 1481](https://rules.sonarsource.com/java/RSPEC-1481)) @@ -448,6 +449,22 @@ Example: ------ +#### "Collections.EMPTY_LIST", "EMPTY_MAP", and "EMPTY_SET" should not be used ([Sonar Rule 1596](https://rules.sonarsource.com/java/RSPEC-1596)) + +The `EMPTY_...` fields from `Collections` return raw types, so they are replaced by the `empty...()` methods that return generic ones. + +Example: +```diff +- List collection1 = Collections.EMPTY_LIST; // Noncompliant +- Map collection2 = Collections.EMPTY_MAP; // Noncompliant +- Set collection3 = Collections.EMPTY_SET; // Noncompliant ++ List collection1 = Collections.emptyList(); ++ Map collection2 = Collections.emptyMap(); ++ Set collection3 = Collections.emptySet(); +``` + +------ + #### Fields in a "Serializable" class should either be transient or serializable ([Sonar Rule 1948](https://rules.sonarsource.com/java/RSPEC-1948)) The repair adds the modifier `transient` to all non-serializable diff --git a/src/main/java/sorald/Processors.java b/src/main/java/sorald/Processors.java index b55a65a0f..5481e831e 100644 --- a/src/main/java/sorald/Processors.java +++ b/src/main/java/sorald/Processors.java @@ -21,6 +21,7 @@ private Processors() {} put(1217, ThreadRunProcessor.class); put(1444, PublicStaticFieldShouldBeFinalProcessor.class); put(1481, UnusedLocalVariableProcessor.class); + put(1596, CollectionsEmptyConstantsProcessor.class); put(1656, SelfAssignementProcessor.class); put(1854, DeadStoreProcessor.class); put(1860, SynchronizationOnStringOrBoxedProcessor.class); @@ -47,7 +48,7 @@ private Processors() {} // GENERATED FIELD public static final String RULE_DESCRIPTIONS = - "1118: Utility classes should not have public constructors\n\t(incomplete: Only handles implicit public constructor)\n1132: Strings literals should be placed on the left side when checking for equality\n1155: Collection.isEmpty() should be used to test for emptiness\n1217: \"Thread.run()\" should not be called directly\n1444: \"public static\" fields should be constant\n\t(incomplete: does not fix variable naming)\n1481: Unused local variables should be removed\n1656: Variables should not be self-assigned\n1854: Unused assignments should be removed\n1860: Synchronization should not be based on Strings or boxed primitives\n1948: Fields in a \"Serializable\" class should either be transient or serializable\n2057: Every class implementing Serializable should declare a static final serialVersionUID.\n\t(incomplete: This processor does not address the case where the class already has a serialVersionUID with a non long type.)\n2095: Resources should be closed\n2097: \"equals(Object obj)\" should test argument type\n2111: \"BigDecimal(double)\" should not be used\n2116: \"hashCode\" and \"toString\" should not be called on array instances\n2142: \"InterruptedException\" should not be ignored\n2164: Math should not be performed on floats\n2167: \"compareTo\" should not return \"Integer.MIN_VALUE\"\n2184: Math operands should be cast before assignment\n2204: \".equals()\" should not be used to test the values of \"Atomic\" classes\n2225: \"toString()\" and \"clone()\" methods should not return null\n\t(incomplete: does not fix null returning clone())\n2272: \"Iterator.next()\" methods should throw \"NoSuchElementException\"\n2755: XML parsers should not be vulnerable to XXE attacks\n\t(incomplete: This processor is a WIP and currently supports a subset of rule 2755. See Sorald\'s documentation for details.)\n3032: JEE applications should not \"getClassLoader\"\n3067: \"getClass\" should not be used for synchronization\n3984: Exception should not be created without being thrown\n4973: Strings and Boxed types should be compared using \"equals()\""; + "1118: Utility classes should not have public constructors\n\t(incomplete: Only handles implicit public constructor)\n1132: Strings literals should be placed on the left side when checking for equality\n1155: Collection.isEmpty() should be used to test for emptiness\n1217: \"Thread.run()\" should not be called directly\n1444: \"public static\" fields should be constant\n\t(incomplete: does not fix variable naming)\n1481: Unused local variables should be removed\n1596: \"Collections.EMPTY_LIST\", \"EMPTY_MAP\", and \"EMPTY_SET\" should not be used\n1656: Variables should not be self-assigned\n1854: Unused assignments should be removed\n1860: Synchronization should not be based on Strings or boxed primitives\n1948: Fields in a \"Serializable\" class should either be transient or serializable\n2057: Every class implementing Serializable should declare a static final serialVersionUID.\n\t(incomplete: This processor does not address the case where the class already has a serialVersionUID with a non long type.)\n2095: Resources should be closed\n2097: \"equals(Object obj)\" should test argument type\n2111: \"BigDecimal(double)\" should not be used\n2116: \"hashCode\" and \"toString\" should not be called on array instances\n2142: \"InterruptedException\" should not be ignored\n2164: Math should not be performed on floats\n2167: \"compareTo\" should not return \"Integer.MIN_VALUE\"\n2184: Math operands should be cast before assignment\n2204: \".equals()\" should not be used to test the values of \"Atomic\" classes\n2225: \"toString()\" and \"clone()\" methods should not return null\n\t(incomplete: does not fix null returning clone())\n2272: \"Iterator.next()\" methods should throw \"NoSuchElementException\"\n2755: XML parsers should not be vulnerable to XXE attacks\n\t(incomplete: This processor is a WIP and currently supports a subset of rule 2755. See Sorald\'s documentation for details.)\n3032: JEE applications should not \"getClassLoader\"\n3067: \"getClass\" should not be used for synchronization\n3984: Exception should not be created without being thrown\n4973: Strings and Boxed types should be compared using \"equals()\""; public static Class> getProcessor(int key) { return RULE_KEY_TO_PROCESSOR.get(key); diff --git a/src/main/java/sorald/processor/CollectionsEmptyConstantsProcessor.java b/src/main/java/sorald/processor/CollectionsEmptyConstantsProcessor.java new file mode 100644 index 000000000..197f03e2b --- /dev/null +++ b/src/main/java/sorald/processor/CollectionsEmptyConstantsProcessor.java @@ -0,0 +1,33 @@ +package sorald.processor; + +import java.util.Collections; +import sorald.annotations.ProcessorAnnotation; +import spoon.reflect.code.*; +import spoon.reflect.declaration.CtMethod; +import spoon.reflect.declaration.CtType; + +@ProcessorAnnotation( + key = 1596, + description = + "\"Collections.EMPTY_LIST\", \"EMPTY_MAP\", and \"EMPTY_SET\" should not be used") +public class CollectionsEmptyConstantsProcessor extends SoraldAbstractProcessor> { + + @Override + protected void repairInternal(CtFieldAccess element) { + CtType collectionsType = getFactory().Class().get(Collections.class); + + CtMethod methodToBeCalled; + if (element.getVariable().getSimpleName().equals("EMPTY_LIST")) { + methodToBeCalled = collectionsType.getMethodsByName("emptyList").get(0); + } else if (element.getVariable().getSimpleName().equals("EMPTY_MAP")) { + methodToBeCalled = collectionsType.getMethodsByName("emptyMap").get(0); + } else { + methodToBeCalled = collectionsType.getMethodsByName("emptySet").get(0); + } + + CtInvocation newInvocation = + getFactory().createInvocation(element.getTarget(), methodToBeCalled.getReference()); + + element.replace(newInvocation); + } +} diff --git a/src/test/resources/processor_test_files/1596_CollectionsEmptyConstants/CollectionsEmptyConstants.java b/src/test/resources/processor_test_files/1596_CollectionsEmptyConstants/CollectionsEmptyConstants.java new file mode 100644 index 000000000..2ec302a58 --- /dev/null +++ b/src/test/resources/processor_test_files/1596_CollectionsEmptyConstants/CollectionsEmptyConstants.java @@ -0,0 +1,12 @@ +import java.util.Collections; +import java.util.List; +import java.util.Map; +import java.util.Set; + +public class CollectionsEmptyConstants { + public static void main(String[] args) { + List collection1 = Collections.EMPTY_LIST; // Noncompliant + Map collection2 = Collections.EMPTY_MAP; // Noncompliant + Set collection3 = Collections.EMPTY_SET; // Noncompliant + } +} diff --git a/src/test/resources/processor_test_files/1596_CollectionsEmptyConstants/CollectionsEmptyConstants.java.expected b/src/test/resources/processor_test_files/1596_CollectionsEmptyConstants/CollectionsEmptyConstants.java.expected new file mode 100644 index 000000000..68869a3f1 --- /dev/null +++ b/src/test/resources/processor_test_files/1596_CollectionsEmptyConstants/CollectionsEmptyConstants.java.expected @@ -0,0 +1,12 @@ +import java.util.Collections; +import java.util.List; +import java.util.Map; +import java.util.Set; + +public class CollectionsEmptyConstants { + public static void main(String[] args) { + List collection1 = Collections.emptyList(); + Map collection2 = Collections.emptyMap(); + Set collection3 = Collections.emptySet(); + } +} From a9540e5049e43d6cc4acb94d02e02d0c5b4a8111 Mon Sep 17 00:00:00 2001 From: Fer Madeiral Date: Thu, 17 Jun 2021 10:12:45 +0200 Subject: [PATCH 2/3] Simplify the processor as suggested by @slarse --- .../CollectionsEmptyConstantsProcessor.java | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/src/main/java/sorald/processor/CollectionsEmptyConstantsProcessor.java b/src/main/java/sorald/processor/CollectionsEmptyConstantsProcessor.java index 197f03e2b..875ebe3a2 100644 --- a/src/main/java/sorald/processor/CollectionsEmptyConstantsProcessor.java +++ b/src/main/java/sorald/processor/CollectionsEmptyConstantsProcessor.java @@ -1,10 +1,10 @@ package sorald.processor; import java.util.Collections; +import org.apache.commons.lang.StringUtils; import sorald.annotations.ProcessorAnnotation; import spoon.reflect.code.*; import spoon.reflect.declaration.CtMethod; -import spoon.reflect.declaration.CtType; @ProcessorAnnotation( key = 1596, @@ -14,16 +14,10 @@ public class CollectionsEmptyConstantsProcessor extends SoraldAbstractProcessor< @Override protected void repairInternal(CtFieldAccess element) { - CtType collectionsType = getFactory().Class().get(Collections.class); - - CtMethod methodToBeCalled; - if (element.getVariable().getSimpleName().equals("EMPTY_LIST")) { - methodToBeCalled = collectionsType.getMethodsByName("emptyList").get(0); - } else if (element.getVariable().getSimpleName().equals("EMPTY_MAP")) { - methodToBeCalled = collectionsType.getMethodsByName("emptyMap").get(0); - } else { - methodToBeCalled = collectionsType.getMethodsByName("emptySet").get(0); - } + String[] loweredNameParts = element.getVariable().getSimpleName().toLowerCase().split("_"); + String camelCasedName = loweredNameParts[0] + StringUtils.capitalize(loweredNameParts[1]); + CtMethod methodToBeCalled = + getFactory().Class().get(Collections.class).getMethod(camelCasedName); CtInvocation newInvocation = getFactory().createInvocation(element.getTarget(), methodToBeCalled.getReference()); From 9757b2592bae0fa6cc778ce6d8eb5bbd08d796cd Mon Sep 17 00:00:00 2001 From: Fer Madeiral Date: Thu, 17 Jun 2021 10:13:39 +0200 Subject: [PATCH 3/3] Add special case in the test files as suggested by @slarse --- .../CollectionsEmptyConstants.java | 1 + .../CollectionsEmptyConstants.java.expected | 1 + 2 files changed, 2 insertions(+) diff --git a/src/test/resources/processor_test_files/1596_CollectionsEmptyConstants/CollectionsEmptyConstants.java b/src/test/resources/processor_test_files/1596_CollectionsEmptyConstants/CollectionsEmptyConstants.java index 2ec302a58..52f00a668 100644 --- a/src/test/resources/processor_test_files/1596_CollectionsEmptyConstants/CollectionsEmptyConstants.java +++ b/src/test/resources/processor_test_files/1596_CollectionsEmptyConstants/CollectionsEmptyConstants.java @@ -8,5 +8,6 @@ public static void main(String[] args) { List collection1 = Collections.EMPTY_LIST; // Noncompliant Map collection2 = Collections.EMPTY_MAP; // Noncompliant Set collection3 = Collections.EMPTY_SET; // Noncompliant + List list = java.util.Collections.EMPTY_LIST; // Compliant due to false positive } } diff --git a/src/test/resources/processor_test_files/1596_CollectionsEmptyConstants/CollectionsEmptyConstants.java.expected b/src/test/resources/processor_test_files/1596_CollectionsEmptyConstants/CollectionsEmptyConstants.java.expected index 68869a3f1..1fc610e36 100644 --- a/src/test/resources/processor_test_files/1596_CollectionsEmptyConstants/CollectionsEmptyConstants.java.expected +++ b/src/test/resources/processor_test_files/1596_CollectionsEmptyConstants/CollectionsEmptyConstants.java.expected @@ -8,5 +8,6 @@ public class CollectionsEmptyConstants { List collection1 = Collections.emptyList(); Map collection2 = Collections.emptyMap(); Set collection3 = Collections.emptySet(); + List list = java.util.Collections.EMPTY_LIST; // Compliant due to false positive } }