address: LiteralsFirstInComparisons PMD rule #368
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,68 +26,112 @@ | |
| import org.openrewrite.marker.Markers; | ||
|
|
||
| import static java.util.Collections.singletonList; | ||
| import static java.util.Objects.requireNonNull; | ||
|
|
||
| /** | ||
| * A visitor that identifies and addresses potential issues related to | ||
| * the use of {@code equals} methods in Java, particularly to avoid | ||
| * null pointer exceptions when comparing strings. | ||
| * <p> | ||
| * This visitor looks for method invocations of {@code equals}, | ||
| * {@code equalsIgnoreCase}, {@code compareTo}, and {@code contentEquals}, | ||
| * and performs optimizations to ensure null checks are correctly applied. | ||
| * <p> | ||
| * For more details, refer to the PMD best practices: | ||
| * <a href="https://pmd.github.io/pmd/pmd_rules_java_bestpractices.html#LiteralsFirstInComparisons">Literals First in Comparisons</a> | ||
| * | ||
| * @param <P> The type of the parent context used for visiting the AST. | ||
| */ | ||
| @Value | ||
| @EqualsAndHashCode(callSuper = false) | ||
| public class EqualsAvoidsNullVisitor<P> extends JavaVisitor<P> { | ||
|
|
||
| private static final MethodMatcher EQUALS = new MethodMatcher("java.lang.String " + "equals(java.lang.Object)"); | ||
| private static final MethodMatcher EQUALS_IGNORE_CASE = new MethodMatcher("java.lang.String " + "equalsIgnoreCase(java.lang.String)"); | ||
| private static final MethodMatcher COMPARE_TO = new MethodMatcher("java.lang.String " + "compareTo(java.lang.String)"); | ||
| private static final MethodMatcher COMPARE_TO_IGNORE_CASE = new MethodMatcher("java.lang.String " + "compareToIgnoreCase(java.lang.String)"); | ||
| private static final MethodMatcher CONTENT_EQUALS = new MethodMatcher("java.lang.String " + "contentEquals(java.lang.CharSequence)"); | ||
|
|
||
| EqualsAvoidsNullStyle style; | ||
|
|
||
| @Override | ||
| public J visitMethodInvocation(J.MethodInvocation method, P p) { | ||
| J.MethodInvocation m = (J.MethodInvocation) super.visitMethodInvocation(method, p); | ||
| if (m.getSelect() != null && | ||
| !(m.getSelect() instanceof J.Literal) && | ||
| m.getArguments().get(0) instanceof J.Literal && | ||
| isStringComparisonMethod(m)) { | ||
| return literalsFirstInComparisonsBinaryCheck(m, getCursor().getParentTreeCursor().getValue()); | ||
| } | ||
| return m; | ||
| } | ||
|
|
||
| private boolean isStringComparisonMethod(J.MethodInvocation methodInvocation) { | ||
| return EQUALS.matches(methodInvocation) || | ||
| !style.getIgnoreEqualsIgnoreCase() && | ||
| EQUALS_IGNORE_CASE.matches(methodInvocation) || | ||
| COMPARE_TO.matches(methodInvocation) || | ||
| COMPARE_TO_IGNORE_CASE.matches(methodInvocation) || | ||
| CONTENT_EQUALS.matches(methodInvocation); | ||
| } | ||
|
|
||
| private Expression literalsFirstInComparisonsBinaryCheck(J.MethodInvocation m, P parent) { | ||
| if (parent instanceof J.Binary) { | ||
| handleBinaryExpression(m, (J.Binary) parent); | ||
| } | ||
| return getExpression(m, m.getArguments().get(0)); | ||
| } | ||
|
|
||
| private static Expression getExpression(J.MethodInvocation m, Expression firstArgument) { | ||
| return firstArgument.getType() == JavaType.Primitive.Null ? | ||
| literalsFirstInComparisonsNull(m, firstArgument) : | ||
| literalsFirstInComparisons(m, firstArgument); | ||
| } | ||
|
|
||
| private static J.Binary literalsFirstInComparisonsNull(J.MethodInvocation m, Expression firstArgument) { | ||
| return new J.Binary(Tree.randomId(), | ||
| m.getPrefix(), | ||
| Markers.EMPTY, | ||
| requireNonNull(m.getSelect()), | ||
| JLeftPadded.build(J.Binary.Type.Equal).withBefore(Space.SINGLE_SPACE), | ||
|
Contributor
Author
There was a problem hiding this comment. Build maybe okay, but the Space is DRY Space. SPACE If i need unwanted context i move my mouse and not clutter the code with redundancy, but its a style where most dev´s use full qualified imports.
Member
There was a problem hiding this comment. Often classes use a mix of the two constants in Space, only one of which has
Contributor
There was a problem hiding this comment. I suspect the idea with the name was to emphasize that the object represents a single space character. The class |
||
| firstArgument.withPrefix(Space.SINGLE_SPACE), | ||
| JavaType.Primitive.Boolean); | ||
| } | ||
|
|
||
| private static J.MethodInvocation literalsFirstInComparisons(J.MethodInvocation m, Expression firstArgument) { | ||
| return m.withSelect(firstArgument.withPrefix(requireNonNull(m.getSelect()).getPrefix())) | ||
| .withArguments(singletonList(m.getSelect().withPrefix(Space.EMPTY))); | ||
| } | ||
|
|
||
| private void handleBinaryExpression(J.MethodInvocation m, J.Binary binary) { | ||
| if (binary.getOperator() == J.Binary.Type.And && binary.getLeft() instanceof J.Binary) { | ||
| J.Binary potentialNullCheck = (J.Binary) binary.getLeft(); | ||
| if (isNullLiteral(potentialNullCheck.getLeft()) && matchesSelect(potentialNullCheck.getRight(), requireNonNull(m.getSelect())) || | ||
| isNullLiteral(potentialNullCheck.getRight()) && matchesSelect(potentialNullCheck.getLeft(), requireNonNull(m.getSelect()))) { | ||
| doAfterVisit(new RemoveUnnecessaryNullCheck<>(binary)); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private boolean isNullLiteral(Expression expression) { | ||
| return expression instanceof J.Literal && ((J.Literal) expression).getType() == JavaType.Primitive.Null; | ||
| } | ||
|
|
||
| private boolean matchesSelect(Expression expression, Expression select) { | ||
| return expression.printTrimmed(getCursor()).replaceAll("\\s", "") | ||
| .equals(select.printTrimmed(getCursor()).replaceAll("\\s", "")); | ||
| } | ||
|
|
||
| private static class RemoveUnnecessaryNullCheck<P> extends JavaVisitor<P> { | ||
|
|
||
| private final J.Binary scope; | ||
|
|
||
| boolean done; | ||
|
|
||
| public RemoveUnnecessaryNullCheck(J.Binary scope) { | ||
| this.scope = scope; | ||
| } | ||
|
|
||
| @Override | ||
| public @Nullable J visit(@Nullable Tree tree, P p) { | ||
| if (done) { | ||
|
|
@@ -96,17 +140,12 @@ private static class RemoveUnnecessaryNullCheck<P> extends JavaVisitor<P> { | |
| return super.visit(tree, p); | ||
| } | ||
|
|
||
| @Override | ||
| public J visitBinary(J.Binary binary, P p) { | ||
| if (scope.isScope(binary)) { | ||
| done = true; | ||
| return binary.getRight().withPrefix(Space.EMPTY); | ||
| } | ||
| return super.visitBinary(binary, p); | ||
| } | ||
| } | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps LiteralsFirstInComparisonsVisitor would be a better fit to align with the corresponding PMD rule name already in place, as it's no longer just about equality; both equality and comparison are now merely implementation details.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd perhaps hold off on renaming this just yet. While the visitor only appears to be used in this module, the recipe name is well established as part of our Common static analysis issues recipe, which folks have created custom copies off that might break if we similarly rename the recipe. I'll think this over as I go through the review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes thanks. Its just an idea and of course would be a new PR topic.