Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add CollectionsEmptyConstantsProcessor (SonarSource rule 1596) #574

Merged
merged 3 commits into from
Jun 17, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions docs/HANDLED_RULES.md
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down Expand Up @@ -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<String> collection1 = Collections.EMPTY_LIST; // Noncompliant
- Map<String, String> collection2 = Collections.EMPTY_MAP; // Noncompliant
- Set<String> collection3 = Collections.EMPTY_SET; // Noncompliant
+ List<String> collection1 = Collections.emptyList();
+ Map<String, String> collection2 = Collections.emptyMap();
+ Set<String> 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
Expand Down
3 changes: 2 additions & 1 deletion src/main/java/sorald/Processors.java
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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<? extends SoraldAbstractProcessor<?>> getProcessor(int key) {
return RULE_KEY_TO_PROCESSOR.get(key);
Expand Down
Original file line number Diff line number Diff line change
@@ -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<CtFieldAccess<?>> {

@Override
protected void repairInternal(CtFieldAccess<?> element) {
CtType<?> collectionsType = getFactory().Class().get(Collections.class);

CtMethod<?> methodToBeCalled;
if (element.getVariable().getSimpleName().equals("EMPTY_LIST")) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On a side note, this violates the rule you implemented a processor for in #562 :)

(sorald-buildbreaker does not catch it because it uses Sorald 0.3.0, which doesn't have that processor)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:X

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);
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about simplifying this a bit? The pattern is "camel case the name and make method invocation".

Suggested change
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);

StringUtils is from org.apache.common.lang (so, import org.apache.common.lang.StringUtils).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a great suggestion! Applied!


CtInvocation<?> newInvocation =
getFactory().createInvocation(element.getTarget(), methodToBeCalled.getReference());

element.replace(newInvocation);
}
}
Original file line number Diff line number Diff line change
@@ -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<String> collection1 = Collections.EMPTY_LIST; // Noncompliant
Map<String, String> collection2 = Collections.EMPTY_MAP; // Noncompliant
Set<String> collection3 = Collections.EMPTY_SET; // Noncompliant
}
}
Original file line number Diff line number Diff line change
@@ -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<String> collection1 = Collections.emptyList();
Map<String, String> collection2 = Collections.emptyMap();
Set<String> collection3 = Collections.emptySet();
}
}