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

Conversation

fermadeiral
Copy link
Collaborator

This PR adds a processor for the rule "Collections.EMPTY_LIST", "EMPTY_MAP", and "EMPTY_SET" should not be used.

Example repair:

- 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();

Copy link
Collaborator

@slarse slarse left a comment

Choose a reason for hiding this comment

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

One comment on the code that I think could be simpler/less hard-coded.

Apart from my comment about simplifying the processor a bit, there's one other thing that crossed my mind. Right now, the target of the method invocation is computed by directly getting a reference to Collections. But what if Collections isn't actually imported, and it's used with a fully qualified name? That would lead to this repair, which could cause a compile error:

// original
List<Integer> list = java.util.Collections.EMPTY_LIST;
// repair
List<Integer> list = Collections.emptyList();

However, the check in Sonar is a bit lazily implemented and only checks if the full name of the target is Collections, thus it doesn't flag qualified accesses to java.util.Collections. But perhaps it will in the future, so you could perhaps add a line like so in the .java and .expected files.

List<Integer> list = java.util.Collections.EMPTY_LIST; // Compliant due to false positive

That way we catch it if they fix the problem (which we should potentially report).

Comment on lines 17 to 26
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);
}
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!

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

@fermadeiral
Copy link
Collaborator Author

@slarse, suggestions applied.

Copy link
Collaborator

@slarse slarse left a comment

Choose a reason for hiding this comment

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

Nice, short and simple processor :)

@slarse slarse merged commit 13639c8 into master Jun 17, 2021
@slarse slarse deleted the add-processor-for-s1596 branch June 17, 2021 08:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants