From 23ce336a34e7c253ef2763c448237513621b41e8 Mon Sep 17 00:00:00 2001 From: Vincent Potucek Date: Fri, 27 Dec 2024 10:19:20 +0100 Subject: [PATCH 01/28] form --- .../EqualsAvoidsNullVisitor.java | 33 +++++++++++-------- 1 file changed, 20 insertions(+), 13 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNullVisitor.java b/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNullVisitor.java index 228217017..f443df249 100644 --- a/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNullVisitor.java +++ b/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNullVisitor.java @@ -47,11 +47,16 @@ public class EqualsAvoidsNullVisitor

extends JavaVisitor

{ private static final String JAVA_LANG_STRING = "java.lang.String "; - 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)"); + 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; @@ -59,7 +64,7 @@ public class EqualsAvoidsNullVisitor

extends JavaVisitor

{ 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) && - isStringComparisonMethod(m) && hasCompatibleArgument(m)) { + isStringComparisonMethod(m) && hasCompatibleArgument(m)) { maybeHandleParentBinary(m); @@ -91,11 +96,11 @@ private boolean hasCompatibleArgument(J.MethodInvocation 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); + !style.getIgnoreEqualsIgnoreCase() && + EQUALS_IGNORE_CASE.matches(methodInvocation) || + COMPARE_TO.matches(methodInvocation) || + COMPARE_TO_IGNORE_CASE.matches(methodInvocation) || + CONTENT_EQUALS.matches(methodInvocation); } private void maybeHandleParentBinary(J.MethodInvocation m) { @@ -103,8 +108,10 @@ private void maybeHandleParentBinary(J.MethodInvocation m) { if (parent instanceof J.Binary) { if (((J.Binary) parent).getOperator() == J.Binary.Type.And && ((J.Binary) parent).getLeft() instanceof J.Binary) { J.Binary potentialNullCheck = (J.Binary) ((J.Binary) parent).getLeft(); - if (isNullLiteral(potentialNullCheck.getLeft()) && matchesSelect(potentialNullCheck.getRight(), requireNonNull(m.getSelect())) || - isNullLiteral(potentialNullCheck.getRight()) && matchesSelect(potentialNullCheck.getLeft(), requireNonNull(m.getSelect()))) { + if (isNullLiteral(potentialNullCheck.getLeft()) && matchesSelect(potentialNullCheck.getRight(), + requireNonNull(m.getSelect())) || + isNullLiteral(potentialNullCheck.getRight()) && matchesSelect(potentialNullCheck.getLeft(), + requireNonNull(m.getSelect()))) { doAfterVisit(new RemoveUnnecessaryNullCheck<>((J.Binary) parent)); } } From 1f33b7342018654b2a3395f7e469dc20ce981125 Mon Sep 17 00:00:00 2001 From: Knut Wannheden Date: Thu, 23 Jan 2025 08:38:21 +0100 Subject: [PATCH 02/28] Use new `J.SwitchExpression` constructor --- .../EqualsAvoidsNullVisitor.java | 6 ++- .../TernaryOperatorsShouldNotBeNested.java | 7 +-- .../staticanalysis/EqualsAvoidsNullTest.java | 52 +++++++++++++++++++ 3 files changed, 61 insertions(+), 4 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNullVisitor.java b/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNullVisitor.java index d2d35fb7b..ad9941279 100644 --- a/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNullVisitor.java +++ b/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNullVisitor.java @@ -49,6 +49,8 @@ public class EqualsAvoidsNullVisitor

extends JavaVisitor

{ private static final String JAVA_LANG_STRING = "java.lang.String "; 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; @@ -90,7 +92,9 @@ private boolean hasCompatibleArgument(J.MethodInvocation m) { private boolean isStringComparisonMethod(J.MethodInvocation methodInvocation) { return EQUALS.matches(methodInvocation) || (!style.getIgnoreEqualsIgnoreCase() && EQUALS_IGNORE_CASE.matches(methodInvocation)) || - CONTENT_EQUALS.matches(methodInvocation); + CONTENT_EQUALS.matches(methodInvocation) || + COMPARE_TO.matches(methodInvocation) || + COMPARE_TO_IGNORE_CASE.matches(methodInvocation); } private void maybeHandleParentBinary(J.MethodInvocation m) { diff --git a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java index 1aa8d4979..002a8a732 100644 --- a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java +++ b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java @@ -159,7 +159,7 @@ public J visitTernary(final J.Ternary ternary, final ExecutionContext ctx) { if (nestList.size() < 2) { return null; } - return autoFormat(toSwitch(switchVar, nestList), ctx); + return autoFormat(toSwitch(switchVar, nestList, ternary.getType()), ctx); }).map(J.class::cast) .orElseGet(() -> super.visitTernary(ternary, ctx)); } @@ -194,7 +194,7 @@ private static boolean isEqualVariable(final J.Identifier switchVar, @Nullable f return Objects.equals(foundVar.getFieldType(), switchVar.getFieldType()); } - private J.SwitchExpression toSwitch(final J.Identifier switchVar, final List nestList) { + private J.SwitchExpression toSwitch(final J.Identifier switchVar, final List nestList, @Nullable JavaType type) { J.Ternary last = nestList.get(nestList.size() - 1); return new J.SwitchExpression( Tree.randomId(), @@ -210,7 +210,8 @@ private J.SwitchExpression toSwitch(final J.Identifier switchVar, final List toCase(switchVar, ternary)), Stream.of(toDefault(last)) ).collect(Collectors.toList())) - .withPrefix(Space.SINGLE_SPACE) + .withPrefix(Space.SINGLE_SPACE), + type ); } diff --git a/src/test/java/org/openrewrite/staticanalysis/EqualsAvoidsNullTest.java b/src/test/java/org/openrewrite/staticanalysis/EqualsAvoidsNullTest.java index 47e95519f..1f6ee26e6 100644 --- a/src/test/java/org/openrewrite/staticanalysis/EqualsAvoidsNullTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/EqualsAvoidsNullTest.java @@ -64,6 +64,58 @@ public class A { ); } + @DocumentExample + @Test + void invertConditional_compareTo() { + rewriteRun( + //language=java + java( + """ + public class A { + { + String s = null; + if(s.compareTo("test")) {} + } + } + """, + """ + public class A { + { + String s = null; + if("test".compareTo(s)) {} + } + } + """ + ) + ); + } + + @DocumentExample + @Test + void invertConditional_compareToIgnoreCase() { + rewriteRun( + //language=java + java( + """ + public class A { + { + String s = null; + if(s.compareToIgnoreCase("test")) {} + } + } + """, + """ + public class A { + { + String s = null; + if("test".compareToIgnoreCase(s)) {} + } + } + """ + ) + ); + } + @Test void removeUnnecessaryNullCheck() { rewriteRun( From b38ff0a0903447e85c41d36b6ed888db36b93c4a Mon Sep 17 00:00:00 2001 From: Knut Wannheden Date: Thu, 23 Jan 2025 08:38:21 +0100 Subject: [PATCH 03/28] Use new `J.SwitchExpression` constructor --- .../EqualsAvoidsNullVisitor.java | 6 ++- .../staticanalysis/EqualsAvoidsNullTest.java | 52 +++++++++++++++++++ 2 files changed, 57 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNullVisitor.java b/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNullVisitor.java index d2d35fb7b..ad9941279 100644 --- a/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNullVisitor.java +++ b/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNullVisitor.java @@ -49,6 +49,8 @@ public class EqualsAvoidsNullVisitor

extends JavaVisitor

{ private static final String JAVA_LANG_STRING = "java.lang.String "; 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; @@ -90,7 +92,9 @@ private boolean hasCompatibleArgument(J.MethodInvocation m) { private boolean isStringComparisonMethod(J.MethodInvocation methodInvocation) { return EQUALS.matches(methodInvocation) || (!style.getIgnoreEqualsIgnoreCase() && EQUALS_IGNORE_CASE.matches(methodInvocation)) || - CONTENT_EQUALS.matches(methodInvocation); + CONTENT_EQUALS.matches(methodInvocation) || + COMPARE_TO.matches(methodInvocation) || + COMPARE_TO_IGNORE_CASE.matches(methodInvocation); } private void maybeHandleParentBinary(J.MethodInvocation m) { diff --git a/src/test/java/org/openrewrite/staticanalysis/EqualsAvoidsNullTest.java b/src/test/java/org/openrewrite/staticanalysis/EqualsAvoidsNullTest.java index 47e95519f..1f6ee26e6 100644 --- a/src/test/java/org/openrewrite/staticanalysis/EqualsAvoidsNullTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/EqualsAvoidsNullTest.java @@ -64,6 +64,58 @@ public class A { ); } + @DocumentExample + @Test + void invertConditional_compareTo() { + rewriteRun( + //language=java + java( + """ + public class A { + { + String s = null; + if(s.compareTo("test")) {} + } + } + """, + """ + public class A { + { + String s = null; + if("test".compareTo(s)) {} + } + } + """ + ) + ); + } + + @DocumentExample + @Test + void invertConditional_compareToIgnoreCase() { + rewriteRun( + //language=java + java( + """ + public class A { + { + String s = null; + if(s.compareToIgnoreCase("test")) {} + } + } + """, + """ + public class A { + { + String s = null; + if("test".compareToIgnoreCase(s)) {} + } + } + """ + ) + ); + } + @Test void removeUnnecessaryNullCheck() { rewriteRun( From 52676bf969eeaf79bf6768ff436c2169c05d86f4 Mon Sep 17 00:00:00 2001 From: Vincent Potucek Date: Sat, 25 Jan 2025 17:36:22 +0100 Subject: [PATCH 04/28] tmp fix invertConditional_compareToIgnoreCase --- .../staticanalysis/EqualsAvoidsNull.java | 28 ++++++++----------- .../EqualsAvoidsNullVisitor.java | 3 +- 2 files changed, 14 insertions(+), 17 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNull.java b/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNull.java index d866a0548..6930a2a4b 100644 --- a/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNull.java +++ b/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNull.java @@ -29,6 +29,7 @@ import java.util.Set; import static java.util.Objects.requireNonNull; +import static org.apache.commons.lang3.ObjectUtils.defaultIfNull; public class EqualsAvoidsNull extends Recipe { @@ -54,24 +55,19 @@ public Duration getEstimatedEffortPerOccurrence() { @Override public TreeVisitor getVisitor() { - JavaIsoVisitor replacementVisitor = new JavaIsoVisitor() { - @Override - public J visit(@Nullable Tree tree, ExecutionContext ctx) { - if (tree instanceof JavaSourceFile) { - JavaSourceFile cu = (JavaSourceFile) requireNonNull(tree); - EqualsAvoidsNullStyle style = cu.getStyle(EqualsAvoidsNullStyle.class); - if (style == null) { - style = Checkstyle.equalsAvoidsNull(); + return Preconditions.check( + new UsesMethod<>("java.lang.String *compareTo*(..)"), + new JavaIsoVisitor() { + @Override + public J visit(@Nullable Tree tree, ExecutionContext ctx) { + if (tree instanceof JavaSourceFile) { + JavaSourceFile cu = (JavaSourceFile) requireNonNull(tree); + return new EqualsAvoidsNullVisitor<>(defaultIfNull(cu.getStyle(EqualsAvoidsNullStyle.class),Checkstyle.equalsAvoidsNull())).visitNonNull(cu, ctx); + } + //noinspection DataFlowIssue + return (J) tree; } - return new EqualsAvoidsNullVisitor<>(style).visitNonNull(cu, ctx); } - //noinspection DataFlowIssue - return (J) tree; - } - }; - return Preconditions.check( - new UsesMethod<>("java.lang.String *quals*(..)"), - replacementVisitor ); } } diff --git a/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNullVisitor.java b/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNullVisitor.java index ad9941279..46380b9c8 100644 --- a/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNullVisitor.java +++ b/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNullVisitor.java @@ -58,8 +58,9 @@ public class EqualsAvoidsNullVisitor

extends JavaVisitor

{ @Override public J visitMethodInvocation(J.MethodInvocation method, P p) { J.MethodInvocation m = (J.MethodInvocation) super.visitMethodInvocation(method, p); + boolean stringComparisonMethod = isStringComparisonMethod(m); if (m.getSelect() != null && !(m.getSelect() instanceof J.Literal) && - isStringComparisonMethod(m) && hasCompatibleArgument(m)) { + stringComparisonMethod && hasCompatibleArgument(m)) { maybeHandleParentBinary(m); From 883f9cf4d1e7297754c5dd0ded3bb5cdb413789e Mon Sep 17 00:00:00 2001 From: Vincent Potucek Date: Sat, 25 Jan 2025 17:38:23 +0100 Subject: [PATCH 05/28] wip --- .../org/openrewrite/staticanalysis/EqualsAvoidsNull.java | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNull.java b/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNull.java index 6930a2a4b..1d86aa708 100644 --- a/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNull.java +++ b/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNull.java @@ -15,6 +15,7 @@ */ package org.openrewrite.staticanalysis; +import org.apache.commons.lang3.ObjectUtils; import org.jspecify.annotations.Nullable; import org.openrewrite.*; import org.openrewrite.java.JavaIsoVisitor; @@ -29,7 +30,6 @@ import java.util.Set; import static java.util.Objects.requireNonNull; -import static org.apache.commons.lang3.ObjectUtils.defaultIfNull; public class EqualsAvoidsNull extends Recipe { @@ -62,7 +62,10 @@ public TreeVisitor getVisitor() { public J visit(@Nullable Tree tree, ExecutionContext ctx) { if (tree instanceof JavaSourceFile) { JavaSourceFile cu = (JavaSourceFile) requireNonNull(tree); - return new EqualsAvoidsNullVisitor<>(defaultIfNull(cu.getStyle(EqualsAvoidsNullStyle.class),Checkstyle.equalsAvoidsNull())).visitNonNull(cu, ctx); + return new EqualsAvoidsNullVisitor<>( + ObjectUtils.defaultIfNull(cu.getStyle(EqualsAvoidsNullStyle.class), + Checkstyle.equalsAvoidsNull())) + .visitNonNull(cu, ctx); } //noinspection DataFlowIssue return (J) tree; From 1d3c8de184ee59ed3f0844882476a4937d124f51 Mon Sep 17 00:00:00 2001 From: Vincent Potucek Date: Sat, 25 Jan 2025 17:47:52 +0100 Subject: [PATCH 06/28] tmp fix --- .../org/openrewrite/staticanalysis/EqualsAvoidsNull.java | 3 ++- .../openrewrite/staticanalysis/EqualsAvoidsNullTest.java | 9 +++++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNull.java b/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNull.java index 1d86aa708..62d15a4d9 100644 --- a/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNull.java +++ b/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNull.java @@ -56,7 +56,8 @@ public Duration getEstimatedEffortPerOccurrence() { @Override public TreeVisitor getVisitor() { return Preconditions.check( - new UsesMethod<>("java.lang.String *compareTo*(..)"), +// new UsesMethod<>("java.lang.String *quals*(..)"), + new UsesMethod<>("java.lang.String *(..)"), new JavaIsoVisitor() { @Override public J visit(@Nullable Tree tree, ExecutionContext ctx) { diff --git a/src/test/java/org/openrewrite/staticanalysis/EqualsAvoidsNullTest.java b/src/test/java/org/openrewrite/staticanalysis/EqualsAvoidsNullTest.java index 1f6ee26e6..104383f2b 100644 --- a/src/test/java/org/openrewrite/staticanalysis/EqualsAvoidsNullTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/EqualsAvoidsNullTest.java @@ -499,6 +499,15 @@ public class A { System.out.println(s.compareToIgnoreCase("test")); } } + """, + """ + public class A { + { + String s = null; + System.out.println("test".compareTo(s)); + System.out.println("test".compareToIgnoreCase(s)); + } + } """ ) ); From d33d0aaa85be2522efd033719c6b05fda8f1020c Mon Sep 17 00:00:00 2001 From: Vincent Potucek Date: Fri, 7 Feb 2025 09:02:23 +0100 Subject: [PATCH 07/28] move EqualsAvoidsNullVisitor into Recipe --- .../staticanalysis/EqualsAvoidsNull.java | 158 ++++++++++++++++- .../EqualsAvoidsNullVisitor.java | 165 ------------------ 2 files changed, 157 insertions(+), 166 deletions(-) delete mode 100644 src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNullVisitor.java diff --git a/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNull.java b/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNull.java index 62d15a4d9..686397709 100644 --- a/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNull.java +++ b/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNull.java @@ -15,20 +15,35 @@ */ package org.openrewrite.staticanalysis; +import lombok.EqualsAndHashCode; +import lombok.Value; import org.apache.commons.lang3.ObjectUtils; import org.jspecify.annotations.Nullable; -import org.openrewrite.*; +import org.openrewrite.ExecutionContext; +import org.openrewrite.Preconditions; +import org.openrewrite.Recipe; +import org.openrewrite.Tree; +import org.openrewrite.TreeVisitor; import org.openrewrite.java.JavaIsoVisitor; +import org.openrewrite.java.JavaVisitor; +import org.openrewrite.java.MethodMatcher; import org.openrewrite.java.search.UsesMethod; import org.openrewrite.java.style.Checkstyle; import org.openrewrite.java.style.EqualsAvoidsNullStyle; +import org.openrewrite.java.tree.Expression; +import org.openrewrite.java.tree.Flag; import org.openrewrite.java.tree.J; +import org.openrewrite.java.tree.JLeftPadded; import org.openrewrite.java.tree.JavaSourceFile; +import org.openrewrite.java.tree.JavaType; +import org.openrewrite.java.tree.Space; +import org.openrewrite.marker.Markers; import java.time.Duration; import java.util.Collections; import java.util.Set; +import static java.util.Collections.singletonList; import static java.util.Objects.requireNonNull; public class EqualsAvoidsNull extends Recipe { @@ -75,3 +90,144 @@ public J visit(@Nullable Tree tree, ExecutionContext ctx) { ); } } + +/** + * 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. + *

+ * 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. + *

+ * For more details, refer to the PMD best practices: + * Literals First in Comparisons + * + * @param

The type of the parent context used for visiting the AST. + */ +@Value +@EqualsAndHashCode(callSuper = false) +class EqualsAvoidsNullVisitor

extends JavaVisitor

{ + + private static final String JAVA_LANG_STRING = "java.lang.String "; + 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); + boolean stringComparisonMethod = isStringComparisonMethod(m); + if (m.getSelect() != null && !(m.getSelect() instanceof J.Literal) && + stringComparisonMethod && hasCompatibleArgument(m)) { + + maybeHandleParentBinary(m); + + Expression firstArgument = m.getArguments().get(0); + return firstArgument.getType() == JavaType.Primitive.Null ? + literalsFirstInComparisonsNull(m, firstArgument) : + literalsFirstInComparisons(m, firstArgument); + } + return m; + } + + private boolean hasCompatibleArgument(J.MethodInvocation m) { + if (m.getArguments().isEmpty()) { + return false; + } + Expression firstArgument = m.getArguments().get(0); + if (firstArgument instanceof J.Literal) { + return true; + } + if (firstArgument instanceof J.FieldAccess) { + firstArgument = ((J.FieldAccess) firstArgument).getName(); + } + if (firstArgument instanceof J.Identifier) { + JavaType.Variable fieldType = ((J.Identifier) firstArgument).getFieldType(); + return fieldType != null && fieldType.hasFlags(Flag.Static, Flag.Final); + } + return false; + } + + private boolean isStringComparisonMethod(J.MethodInvocation methodInvocation) { + return EQUALS.matches(methodInvocation) || + (!style.getIgnoreEqualsIgnoreCase() && EQUALS_IGNORE_CASE.matches(methodInvocation)) || + CONTENT_EQUALS.matches(methodInvocation) || + COMPARE_TO.matches(methodInvocation) || + COMPARE_TO_IGNORE_CASE.matches(methodInvocation); + } + + private void maybeHandleParentBinary(J.MethodInvocation m) { + P parent = getCursor().getParentTreeCursor().getValue(); + if (parent instanceof J.Binary) { + if (((J.Binary) parent).getOperator() == J.Binary.Type.And && ((J.Binary) parent).getLeft() instanceof J.Binary) { + J.Binary potentialNullCheck = (J.Binary) ((J.Binary) parent).getLeft(); + if (isNullLiteral(potentialNullCheck.getLeft()) && matchesSelect(potentialNullCheck.getRight(), + requireNonNull(m.getSelect())) || + isNullLiteral(potentialNullCheck.getRight()) && matchesSelect(potentialNullCheck.getLeft(), + requireNonNull(m.getSelect()))) { + doAfterVisit(new RemoveUnnecessaryNullCheck<>((J.Binary) parent)); + } + } + } + } + + 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 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), + 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 static class RemoveUnnecessaryNullCheck

extends JavaVisitor

{ + + 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) { + return (J) tree; + } + return super.visit(tree, p); + } + + @Override + public J visitBinary(J.Binary binary, P p) { + if (scope.isScope(binary)) { + done = true; + return binary.getRight().withPrefix(binary.getPrefix()); + } + return super.visitBinary(binary, p); + } + } +} diff --git a/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNullVisitor.java b/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNullVisitor.java deleted file mode 100644 index 46380b9c8..000000000 --- a/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNullVisitor.java +++ /dev/null @@ -1,165 +0,0 @@ -/* - * Copyright 2024 the original author or authors. - *

- * Licensed under the Moderne Source Available License (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - *

- * https://docs.moderne.io/licensing/moderne-source-available-license - *

- * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -package org.openrewrite.staticanalysis; - -import lombok.EqualsAndHashCode; -import lombok.Value; -import org.jspecify.annotations.Nullable; -import org.openrewrite.Tree; -import org.openrewrite.java.JavaVisitor; -import org.openrewrite.java.MethodMatcher; -import org.openrewrite.java.style.EqualsAvoidsNullStyle; -import org.openrewrite.java.tree.*; -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. - *

- * 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. - *

- * For more details, refer to the PMD best practices: - * Literals First in Comparisons - * - * @param

The type of the parent context used for visiting the AST. - */ -@Value -@EqualsAndHashCode(callSuper = false) -public class EqualsAvoidsNullVisitor

extends JavaVisitor

{ - - private static final String JAVA_LANG_STRING = "java.lang.String "; - 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); - boolean stringComparisonMethod = isStringComparisonMethod(m); - if (m.getSelect() != null && !(m.getSelect() instanceof J.Literal) && - stringComparisonMethod && hasCompatibleArgument(m)) { - - maybeHandleParentBinary(m); - - Expression firstArgument = m.getArguments().get(0); - return firstArgument.getType() == JavaType.Primitive.Null ? - literalsFirstInComparisonsNull(m, firstArgument) : - literalsFirstInComparisons(m, firstArgument); - } - return m; - } - - private boolean hasCompatibleArgument(J.MethodInvocation m) { - if (m.getArguments().isEmpty()) { - return false; - } - Expression firstArgument = m.getArguments().get(0); - if (firstArgument instanceof J.Literal) { - return true; - } - if (firstArgument instanceof J.FieldAccess) { - firstArgument = ((J.FieldAccess) firstArgument).getName(); - } - if (firstArgument instanceof J.Identifier) { - JavaType.Variable fieldType = ((J.Identifier) firstArgument).getFieldType(); - return fieldType != null && fieldType.hasFlags(Flag.Static, Flag.Final); - } - return false; - } - - private boolean isStringComparisonMethod(J.MethodInvocation methodInvocation) { - return EQUALS.matches(methodInvocation) || - (!style.getIgnoreEqualsIgnoreCase() && EQUALS_IGNORE_CASE.matches(methodInvocation)) || - CONTENT_EQUALS.matches(methodInvocation) || - COMPARE_TO.matches(methodInvocation) || - COMPARE_TO_IGNORE_CASE.matches(methodInvocation); - } - - private void maybeHandleParentBinary(J.MethodInvocation m) { - P parent = getCursor().getParentTreeCursor().getValue(); - if (parent instanceof J.Binary) { - if (((J.Binary) parent).getOperator() == J.Binary.Type.And && ((J.Binary) parent).getLeft() instanceof J.Binary) { - J.Binary potentialNullCheck = (J.Binary) ((J.Binary) parent).getLeft(); - if (isNullLiteral(potentialNullCheck.getLeft()) && matchesSelect(potentialNullCheck.getRight(), requireNonNull(m.getSelect())) || - isNullLiteral(potentialNullCheck.getRight()) && matchesSelect(potentialNullCheck.getLeft(), requireNonNull(m.getSelect()))) { - doAfterVisit(new RemoveUnnecessaryNullCheck<>((J.Binary) parent)); - } - } - } - } - - 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 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), - 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 static class RemoveUnnecessaryNullCheck

extends JavaVisitor

{ - - 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) { - return (J) tree; - } - return super.visit(tree, p); - } - - @Override - public J visitBinary(J.Binary binary, P p) { - if (scope.isScope(binary)) { - done = true; - return binary.getRight().withPrefix(binary.getPrefix()); - } - return super.visitBinary(binary, p); - } - } -} From a99ae408d27006508d0778b66bf4b5691dd621f9 Mon Sep 17 00:00:00 2001 From: Vincent Potucek Date: Fri, 7 Feb 2025 09:08:35 +0100 Subject: [PATCH 08/28] conduct preconditions --- .../org/openrewrite/staticanalysis/EqualsAvoidsNull.java | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNull.java b/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNull.java index 686397709..36f51bf91 100644 --- a/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNull.java +++ b/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNull.java @@ -71,8 +71,12 @@ public Duration getEstimatedEffortPerOccurrence() { @Override public TreeVisitor getVisitor() { return Preconditions.check( -// new UsesMethod<>("java.lang.String *quals*(..)"), - new UsesMethod<>("java.lang.String *(..)"), + Preconditions.or( + new UsesMethod<>("java.lang.String compareTo(..)"), + new UsesMethod<>("java.lang.String compareToIgnoreCase(..)"), + new UsesMethod<>("java.lang.String contentEquals(..)"), + new UsesMethod<>("java.lang.String equals(..)"), + new UsesMethod<>("java.lang.String equalsIgnoreCase(..)")), new JavaIsoVisitor() { @Override public J visit(@Nullable Tree tree, ExecutionContext ctx) { From b53e380754131838fe64e79cb406bbf2f487d7cb Mon Sep 17 00:00:00 2001 From: Vincent Potucek Date: Fri, 7 Feb 2025 09:09:54 +0100 Subject: [PATCH 09/28] static import --- .../org/openrewrite/staticanalysis/EqualsAvoidsNull.java | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNull.java b/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNull.java index 36f51bf91..5da98043c 100644 --- a/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNull.java +++ b/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNull.java @@ -40,9 +40,10 @@ import org.openrewrite.marker.Markers; import java.time.Duration; -import java.util.Collections; import java.util.Set; +import static java.time.Duration.ofMinutes; +import static java.util.Collections.singleton; import static java.util.Collections.singletonList; import static java.util.Objects.requireNonNull; @@ -60,12 +61,12 @@ public String getDescription() { @Override public Set getTags() { - return Collections.singleton("RSPEC-S1132"); + return singleton("RSPEC-S1132"); } @Override public Duration getEstimatedEffortPerOccurrence() { - return Duration.ofMinutes(10); + return ofMinutes(10); } @Override From 8512dbfb06e689ba300eeaffd7e993f6ea1851ac Mon Sep 17 00:00:00 2001 From: Vincent Potucek Date: Sun, 9 Feb 2025 13:14:37 +0100 Subject: [PATCH 10/28] duplicate retainCompareToAsToNotChangeOrder --- .../org/openrewrite/staticanalysis/EqualsAvoidsNullTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/java/org/openrewrite/staticanalysis/EqualsAvoidsNullTest.java b/src/test/java/org/openrewrite/staticanalysis/EqualsAvoidsNullTest.java index 104383f2b..f8b47ab96 100644 --- a/src/test/java/org/openrewrite/staticanalysis/EqualsAvoidsNullTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/EqualsAvoidsNullTest.java @@ -504,8 +504,8 @@ public class A { public class A { { String s = null; - System.out.println("test".compareTo(s)); - System.out.println("test".compareToIgnoreCase(s)); + System.out.println(s.compareTo("test")); + System.out.println(s.compareToIgnoreCase("test")); } } """ From 7d9975fbfc17e760a479d308123036fa885ad43f Mon Sep 17 00:00:00 2001 From: Vincent Potucek Date: Sun, 9 Feb 2025 14:20:48 +0100 Subject: [PATCH 11/28] undo --- .../openrewrite/staticanalysis/EqualsAvoidsNullTest.java | 9 --------- 1 file changed, 9 deletions(-) diff --git a/src/test/java/org/openrewrite/staticanalysis/EqualsAvoidsNullTest.java b/src/test/java/org/openrewrite/staticanalysis/EqualsAvoidsNullTest.java index f8b47ab96..1f6ee26e6 100644 --- a/src/test/java/org/openrewrite/staticanalysis/EqualsAvoidsNullTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/EqualsAvoidsNullTest.java @@ -491,15 +491,6 @@ void retainCompareToAsToNotChangeOrder() { rewriteRun( //language=java java( - """ - public class A { - { - String s = null; - System.out.println(s.compareTo("test")); - System.out.println(s.compareToIgnoreCase("test")); - } - } - """, """ public class A { { From 0ae11da6cad3d9b54e49c9e4280a6b41716863b4 Mon Sep 17 00:00:00 2001 From: Vincent Potucek Date: Sun, 9 Feb 2025 14:41:14 +0100 Subject: [PATCH 12/28] resolve comparison --- .../staticanalysis/EqualsAvoidsNull.java | 9 +- .../EqualsAvoidsNullVisitor.java | 172 ------------------ .../staticanalysis/EqualsAvoidsNullTest.java | 52 ------ 3 files changed, 1 insertion(+), 232 deletions(-) delete mode 100644 src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNullVisitor.java diff --git a/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNull.java b/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNull.java index 5da98043c..9aef7e7da 100644 --- a/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNull.java +++ b/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNull.java @@ -73,8 +73,6 @@ public Duration getEstimatedEffortPerOccurrence() { public TreeVisitor getVisitor() { return Preconditions.check( Preconditions.or( - new UsesMethod<>("java.lang.String compareTo(..)"), - new UsesMethod<>("java.lang.String compareToIgnoreCase(..)"), new UsesMethod<>("java.lang.String contentEquals(..)"), new UsesMethod<>("java.lang.String equals(..)"), new UsesMethod<>("java.lang.String equalsIgnoreCase(..)")), @@ -118,9 +116,6 @@ class EqualsAvoidsNullVisitor

extends JavaVisitor

{ 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)"); @@ -164,9 +159,7 @@ private boolean hasCompatibleArgument(J.MethodInvocation m) { private boolean isStringComparisonMethod(J.MethodInvocation methodInvocation) { return EQUALS.matches(methodInvocation) || (!style.getIgnoreEqualsIgnoreCase() && EQUALS_IGNORE_CASE.matches(methodInvocation)) || - CONTENT_EQUALS.matches(methodInvocation) || - COMPARE_TO.matches(methodInvocation) || - COMPARE_TO_IGNORE_CASE.matches(methodInvocation); + CONTENT_EQUALS.matches(methodInvocation); } private void maybeHandleParentBinary(J.MethodInvocation m) { diff --git a/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNullVisitor.java b/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNullVisitor.java deleted file mode 100644 index f443df249..000000000 --- a/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNullVisitor.java +++ /dev/null @@ -1,172 +0,0 @@ -/* - * Copyright 2024 the original author or authors. - *

- * Licensed under the Moderne Source Available License (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - *

- * https://docs.moderne.io/licensing/moderne-source-available-license - *

- * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -package org.openrewrite.staticanalysis; - -import lombok.EqualsAndHashCode; -import lombok.Value; -import org.jspecify.annotations.Nullable; -import org.openrewrite.Tree; -import org.openrewrite.java.JavaVisitor; -import org.openrewrite.java.MethodMatcher; -import org.openrewrite.java.style.EqualsAvoidsNullStyle; -import org.openrewrite.java.tree.*; -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. - *

- * 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. - *

- * For more details, refer to the PMD best practices: - * Literals First in Comparisons - * - * @param

The type of the parent context used for visiting the AST. - */ -@Value -@EqualsAndHashCode(callSuper = false) -public class EqualsAvoidsNullVisitor

extends JavaVisitor

{ - - private static final String JAVA_LANG_STRING = "java.lang.String "; - 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) && - isStringComparisonMethod(m) && hasCompatibleArgument(m)) { - - maybeHandleParentBinary(m); - - Expression firstArgument = m.getArguments().get(0); - return firstArgument.getType() == JavaType.Primitive.Null ? - literalsFirstInComparisonsNull(m, firstArgument) : - literalsFirstInComparisons(m, firstArgument); - } - return m; - } - - private boolean hasCompatibleArgument(J.MethodInvocation m) { - if (m.getArguments().isEmpty()) { - return false; - } - Expression firstArgument = m.getArguments().get(0); - if (firstArgument instanceof J.Literal) { - return true; - } - if (firstArgument instanceof J.FieldAccess) { - firstArgument = ((J.FieldAccess) firstArgument).getName(); - } - if (firstArgument instanceof J.Identifier) { - JavaType.Variable fieldType = ((J.Identifier) firstArgument).getFieldType(); - return fieldType != null && fieldType.hasFlags(Flag.Static, Flag.Final); - } - return false; - } - - 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 void maybeHandleParentBinary(J.MethodInvocation m) { - P parent = getCursor().getParentTreeCursor().getValue(); - if (parent instanceof J.Binary) { - if (((J.Binary) parent).getOperator() == J.Binary.Type.And && ((J.Binary) parent).getLeft() instanceof J.Binary) { - J.Binary potentialNullCheck = (J.Binary) ((J.Binary) parent).getLeft(); - if (isNullLiteral(potentialNullCheck.getLeft()) && matchesSelect(potentialNullCheck.getRight(), - requireNonNull(m.getSelect())) || - isNullLiteral(potentialNullCheck.getRight()) && matchesSelect(potentialNullCheck.getLeft(), - requireNonNull(m.getSelect()))) { - doAfterVisit(new RemoveUnnecessaryNullCheck<>((J.Binary) parent)); - } - } - } - } - - 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 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), - 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 static class RemoveUnnecessaryNullCheck

extends JavaVisitor

{ - - 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) { - return (J) tree; - } - 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); - } - } -} diff --git a/src/test/java/org/openrewrite/staticanalysis/EqualsAvoidsNullTest.java b/src/test/java/org/openrewrite/staticanalysis/EqualsAvoidsNullTest.java index 1f6ee26e6..47e95519f 100644 --- a/src/test/java/org/openrewrite/staticanalysis/EqualsAvoidsNullTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/EqualsAvoidsNullTest.java @@ -64,58 +64,6 @@ public class A { ); } - @DocumentExample - @Test - void invertConditional_compareTo() { - rewriteRun( - //language=java - java( - """ - public class A { - { - String s = null; - if(s.compareTo("test")) {} - } - } - """, - """ - public class A { - { - String s = null; - if("test".compareTo(s)) {} - } - } - """ - ) - ); - } - - @DocumentExample - @Test - void invertConditional_compareToIgnoreCase() { - rewriteRun( - //language=java - java( - """ - public class A { - { - String s = null; - if(s.compareToIgnoreCase("test")) {} - } - } - """, - """ - public class A { - { - String s = null; - if("test".compareToIgnoreCase(s)) {} - } - } - """ - ) - ); - } - @Test void removeUnnecessaryNullCheck() { rewriteRun( From d2124a5c3cb31df2d05d454d302642f216b451b5 Mon Sep 17 00:00:00 2001 From: Vincent Potucek Date: Sun, 9 Feb 2025 14:51:16 +0100 Subject: [PATCH 13/28] invertConditionalButKeepComparisonOrder --- .../staticanalysis/EqualsAvoidsNullTest.java | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/src/test/java/org/openrewrite/staticanalysis/EqualsAvoidsNullTest.java b/src/test/java/org/openrewrite/staticanalysis/EqualsAvoidsNullTest.java index 47e95519f..80d467601 100644 --- a/src/test/java/org/openrewrite/staticanalysis/EqualsAvoidsNullTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/EqualsAvoidsNullTest.java @@ -64,6 +64,35 @@ public class A { ); } + @DocumentExample + @Test + void invertConditionalButKeepComparisonOrder() { + rewriteRun( + //language=java + java( + """ + class A { + boolean bar(String x) { + x.equals("2"); // should be "2".equals(x) + x.compareTo("2"); // should be x.compareTo("2") + x.compareToIgnoreCase("2"); // should be x.compareToIgnoreCase("2") + return x.contentEquals("2"); // should be "2".contentEquals(x) + } + } + """, + """ + class A { + boolean bar(String x) { + "2".equals(x); // should be "2".equals(x) + x.compareTo("2"); // should be x.compareTo("2") + x.compareToIgnoreCase("2"); // should be x.compareToIgnoreCase("2") + return "2".contentEquals(x); // should be "2".contentEquals(x) + } + } + """) + ); + } + @Test void removeUnnecessaryNullCheck() { rewriteRun( From dac658c5edd86545b42d4d3c3fc016704c54e007 Mon Sep 17 00:00:00 2001 From: Vincent Potucek Date: Sun, 9 Feb 2025 14:52:12 +0100 Subject: [PATCH 14/28] @Issue("https://github.com/openrewrite/rewrite-static-analysis/issues/442") --- .../org/openrewrite/staticanalysis/EqualsAvoidsNullTest.java | 1 + 1 file changed, 1 insertion(+) diff --git a/src/test/java/org/openrewrite/staticanalysis/EqualsAvoidsNullTest.java b/src/test/java/org/openrewrite/staticanalysis/EqualsAvoidsNullTest.java index 80d467601..bc0a84636 100644 --- a/src/test/java/org/openrewrite/staticanalysis/EqualsAvoidsNullTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/EqualsAvoidsNullTest.java @@ -64,6 +64,7 @@ public class A { ); } + @Issue("https://github.com/openrewrite/rewrite-static-analysis/issues/442") @DocumentExample @Test void invertConditionalButKeepComparisonOrder() { From 7b1b55c4fb4dbc16d7ad61008e82bc000f8debe3 Mon Sep 17 00:00:00 2001 From: Vincent Potucek Date: Sun, 9 Feb 2025 14:52:25 +0100 Subject: [PATCH 15/28] @Issue("https://github.com/openrewrite/rewrite-static-analysis/issues/362") --- .../org/openrewrite/staticanalysis/EqualsAvoidsNullTest.java | 1 + 1 file changed, 1 insertion(+) diff --git a/src/test/java/org/openrewrite/staticanalysis/EqualsAvoidsNullTest.java b/src/test/java/org/openrewrite/staticanalysis/EqualsAvoidsNullTest.java index bc0a84636..5e2448d91 100644 --- a/src/test/java/org/openrewrite/staticanalysis/EqualsAvoidsNullTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/EqualsAvoidsNullTest.java @@ -64,6 +64,7 @@ public class A { ); } + @Issue("https://github.com/openrewrite/rewrite-static-analysis/issues/362") @Issue("https://github.com/openrewrite/rewrite-static-analysis/issues/442") @DocumentExample @Test From 21ca2f76891b28cd44cd93fe0a67df1c8ecc9f34 Mon Sep 17 00:00:00 2001 From: Vincent Potucek Date: Sun, 9 Feb 2025 14:57:34 +0100 Subject: [PATCH 16/28] extend invertConditionalButKeepComparisonOrder --- .../staticanalysis/EqualsAvoidsNullTest.java | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/test/java/org/openrewrite/staticanalysis/EqualsAvoidsNullTest.java b/src/test/java/org/openrewrite/staticanalysis/EqualsAvoidsNullTest.java index 5e2448d91..5af3dfb2c 100644 --- a/src/test/java/org/openrewrite/staticanalysis/EqualsAvoidsNullTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/EqualsAvoidsNullTest.java @@ -75,9 +75,11 @@ void invertConditionalButKeepComparisonOrder() { """ class A { boolean bar(String x) { + x.compareTo("2"); // should be x.compareTo("2") for stable compare logic + x.compareToIgnoreCase("2"); // should be x.compareToIgnoreCase("2") for stable compare logic + x.contentEquals("2"); // should be "2".contentEquals(x) x.equals("2"); // should be "2".equals(x) - x.compareTo("2"); // should be x.compareTo("2") - x.compareToIgnoreCase("2"); // should be x.compareToIgnoreCase("2") + x.equalsIgnoreCase("2"); // should be "2".equalsIgnoreCase(x) return x.contentEquals("2"); // should be "2".contentEquals(x) } } @@ -85,9 +87,11 @@ boolean bar(String x) { """ class A { boolean bar(String x) { + x.compareTo("2"); // should be x.compareTo("2") for stable compare logic + x.compareToIgnoreCase("2"); // should be x.compareToIgnoreCase("2") for stable compare logic + "2".contentEquals(x); // should be "2".contentEquals(x) "2".equals(x); // should be "2".equals(x) - x.compareTo("2"); // should be x.compareTo("2") - x.compareToIgnoreCase("2"); // should be x.compareToIgnoreCase("2") + "2".equalsIgnoreCase(x); // should be "2".equalsIgnoreCase(x) return "2".contentEquals(x); // should be "2".contentEquals(x) } } From 045ce063a8dde57660e2466a0209f09769829582 Mon Sep 17 00:00:00 2001 From: Vincent Potucek Date: Sun, 9 Feb 2025 15:04:12 +0100 Subject: [PATCH 17/28] keepOrderForSameType --- .../staticanalysis/EqualsAvoidsNullTest.java | 35 +++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/src/test/java/org/openrewrite/staticanalysis/EqualsAvoidsNullTest.java b/src/test/java/org/openrewrite/staticanalysis/EqualsAvoidsNullTest.java index 5af3dfb2c..8f2500d42 100644 --- a/src/test/java/org/openrewrite/staticanalysis/EqualsAvoidsNullTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/EqualsAvoidsNullTest.java @@ -99,6 +99,41 @@ boolean bar(String x) { ); } + @Issue("https://github.com/openrewrite/rewrite-static-analysis/issues/362") + @Issue("https://github.com/openrewrite/rewrite-static-analysis/issues/442") + @DocumentExample + @Test + void keepOrderForSameType() { + rewriteRun( + //language=java + java( + """ + class A { + void bar(String x) { + String o = null; + o.equals(x); + o.equalsIgnoreCase(x); + o.contentEquals(x); + o.compareTo(x); + o.compareToIgnoreCase(x); + } + } + """, + """ + class A { + void bar(String x) { + String o = null; + o.equals(x); + o.equalsIgnoreCase(x); + o.contentEquals(x); + o.compareTo(x); + o.compareToIgnoreCase(x); + } + } + """) + ); + } + @Test void removeUnnecessaryNullCheck() { rewriteRun( From 2e556b2bc9b8a1eaa9a9ad5466d6dce0aca6db67 Mon Sep 17 00:00:00 2001 From: Vincent Potucek Date: Sun, 9 Feb 2025 15:06:07 +0100 Subject: [PATCH 18/28] remove duplication --- .../staticanalysis/EqualsAvoidsNullTest.java | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/src/test/java/org/openrewrite/staticanalysis/EqualsAvoidsNullTest.java b/src/test/java/org/openrewrite/staticanalysis/EqualsAvoidsNullTest.java index 8f2500d42..b04cae415 100644 --- a/src/test/java/org/openrewrite/staticanalysis/EqualsAvoidsNullTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/EqualsAvoidsNullTest.java @@ -107,18 +107,6 @@ void keepOrderForSameType() { rewriteRun( //language=java java( - """ - class A { - void bar(String x) { - String o = null; - o.equals(x); - o.equalsIgnoreCase(x); - o.contentEquals(x); - o.compareTo(x); - o.compareToIgnoreCase(x); - } - } - """, """ class A { void bar(String x) { From 905573f3fd84e290e5ce495d91ca554923509c1f Mon Sep 17 00:00:00 2001 From: Vincent Potucek Date: Sun, 9 Feb 2025 15:16:47 +0100 Subject: [PATCH 19/28] inline --- .../staticanalysis/EqualsAvoidsNull.java | 253 ++++++++---------- 1 file changed, 111 insertions(+), 142 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNull.java b/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNull.java index 9aef7e7da..6149c0a8f 100644 --- a/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNull.java +++ b/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNull.java @@ -15,8 +15,6 @@ */ package org.openrewrite.staticanalysis; -import lombok.EqualsAndHashCode; -import lombok.Value; import org.apache.commons.lang3.ObjectUtils; import org.jspecify.annotations.Nullable; import org.openrewrite.ExecutionContext; @@ -81,10 +79,117 @@ public TreeVisitor getVisitor() { public J visit(@Nullable Tree tree, ExecutionContext ctx) { if (tree instanceof JavaSourceFile) { JavaSourceFile cu = (JavaSourceFile) requireNonNull(tree); - return new EqualsAvoidsNullVisitor<>( - ObjectUtils.defaultIfNull(cu.getStyle(EqualsAvoidsNullStyle.class), - Checkstyle.equalsAvoidsNull())) - .visitNonNull(cu, ctx); + return new JavaVisitor() { + @Override + public J visitMethodInvocation(J.MethodInvocation method, ExecutionContext p) { + J.MethodInvocation m = (J.MethodInvocation) super.visitMethodInvocation(method, p); + boolean stringComparisonMethod = isStringComparisonMethod(m); + if (m.getSelect() != null && !(m.getSelect() instanceof J.Literal) && + stringComparisonMethod && hasCompatibleArgument(m)) { + + maybeHandleParentBinary(m); + + Expression firstArgument = m.getArguments().get(0); + return firstArgument.getType() == JavaType.Primitive.Null ? + literalsFirstInComparisonsNull(m, firstArgument) : + literalsFirstInComparisons(m, firstArgument); + } + return m; + } + + private boolean hasCompatibleArgument(J.MethodInvocation m) { + if (m.getArguments().isEmpty()) { + return false; + } + Expression firstArgument = m.getArguments().get(0); + if (firstArgument instanceof J.Literal) { + return true; + } + if (firstArgument instanceof J.FieldAccess) { + firstArgument = ((J.FieldAccess) firstArgument).getName(); + } + if (firstArgument instanceof J.Identifier) { + JavaType.Variable fieldType = ((J.Identifier) firstArgument).getFieldType(); + return fieldType != null && fieldType.hasFlags(Flag.Static, Flag.Final); + } + return false; + } + + private boolean isStringComparisonMethod(J.MethodInvocation methodInvocation) { + return EQUALS.matches(methodInvocation) || + (!ObjectUtils.defaultIfNull(cu.getStyle(EqualsAvoidsNullStyle.class), + Checkstyle.equalsAvoidsNull()).getIgnoreEqualsIgnoreCase() && EQUALS_IGNORE_CASE.matches(methodInvocation)) || + CONTENT_EQUALS.matches(methodInvocation); + } + + private void maybeHandleParentBinary(J.MethodInvocation m) { + Tree parent = getCursor().getParentTreeCursor().getValue(); + if (parent instanceof J.Binary) { + if (((J.Binary) parent).getOperator() == J.Binary.Type.And && ((J.Binary) parent).getLeft() instanceof J.Binary) { + J.Binary potentialNullCheck = (J.Binary) ((J.Binary) parent).getLeft(); + if (isNullLiteral(potentialNullCheck.getLeft()) && matchesSelect(potentialNullCheck.getRight(), + requireNonNull(m.getSelect())) || + isNullLiteral(potentialNullCheck.getRight()) && matchesSelect(potentialNullCheck.getLeft(), + requireNonNull(m.getSelect()))) { + doAfterVisit(new JavaVisitor() { + private final J.Binary scope = (J.Binary) parent; + private boolean done; + + @Override + public @Nullable J visit(@Nullable Tree tree, ExecutionContext p) { + if (done) { + return (J) tree; + } + return super.visit(tree, p); + } + + @Override + public J visitBinary(J.Binary binary, ExecutionContext p) { + if (scope.isScope(binary)) { + done = true; + return binary.getRight().withPrefix(binary.getPrefix()); + } + return super.visitBinary(binary, p); + } + }); + } + } + } + } + + 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 final MethodMatcher EQUALS = new MethodMatcher("java.lang.String equals(java" + + ".lang.Object)"); + private final MethodMatcher EQUALS_IGNORE_CASE = new MethodMatcher("java.lang.String " + + "equalsIgnoreCase(java.lang.String)"); + private final MethodMatcher CONTENT_EQUALS = new MethodMatcher("java.lang.String " + + "contentEquals(java.lang.CharSequence)"); + + private 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), + firstArgument.withPrefix(Space.SINGLE_SPACE), + JavaType.Primitive.Boolean); + } + + private J.MethodInvocation literalsFirstInComparisons(J.MethodInvocation m, + Expression firstArgument) { + return m.withSelect(firstArgument.withPrefix(requireNonNull(m.getSelect()).getPrefix())) + .withArguments(singletonList(m.getSelect().withPrefix(Space.EMPTY))); + } + }.visitNonNull(cu, ctx); } //noinspection DataFlowIssue return (J) tree; @@ -93,139 +198,3 @@ public J visit(@Nullable Tree tree, ExecutionContext ctx) { ); } } - -/** - * 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. - *

- * 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. - *

- * For more details, refer to the PMD best practices: - * Literals First in Comparisons - * - * @param

The type of the parent context used for visiting the AST. - */ -@Value -@EqualsAndHashCode(callSuper = false) -class EqualsAvoidsNullVisitor

extends JavaVisitor

{ - - private static final String JAVA_LANG_STRING = "java.lang.String "; - 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 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); - boolean stringComparisonMethod = isStringComparisonMethod(m); - if (m.getSelect() != null && !(m.getSelect() instanceof J.Literal) && - stringComparisonMethod && hasCompatibleArgument(m)) { - - maybeHandleParentBinary(m); - - Expression firstArgument = m.getArguments().get(0); - return firstArgument.getType() == JavaType.Primitive.Null ? - literalsFirstInComparisonsNull(m, firstArgument) : - literalsFirstInComparisons(m, firstArgument); - } - return m; - } - - private boolean hasCompatibleArgument(J.MethodInvocation m) { - if (m.getArguments().isEmpty()) { - return false; - } - Expression firstArgument = m.getArguments().get(0); - if (firstArgument instanceof J.Literal) { - return true; - } - if (firstArgument instanceof J.FieldAccess) { - firstArgument = ((J.FieldAccess) firstArgument).getName(); - } - if (firstArgument instanceof J.Identifier) { - JavaType.Variable fieldType = ((J.Identifier) firstArgument).getFieldType(); - return fieldType != null && fieldType.hasFlags(Flag.Static, Flag.Final); - } - return false; - } - - private boolean isStringComparisonMethod(J.MethodInvocation methodInvocation) { - return EQUALS.matches(methodInvocation) || - (!style.getIgnoreEqualsIgnoreCase() && EQUALS_IGNORE_CASE.matches(methodInvocation)) || - CONTENT_EQUALS.matches(methodInvocation); - } - - private void maybeHandleParentBinary(J.MethodInvocation m) { - P parent = getCursor().getParentTreeCursor().getValue(); - if (parent instanceof J.Binary) { - if (((J.Binary) parent).getOperator() == J.Binary.Type.And && ((J.Binary) parent).getLeft() instanceof J.Binary) { - J.Binary potentialNullCheck = (J.Binary) ((J.Binary) parent).getLeft(); - if (isNullLiteral(potentialNullCheck.getLeft()) && matchesSelect(potentialNullCheck.getRight(), - requireNonNull(m.getSelect())) || - isNullLiteral(potentialNullCheck.getRight()) && matchesSelect(potentialNullCheck.getLeft(), - requireNonNull(m.getSelect()))) { - doAfterVisit(new RemoveUnnecessaryNullCheck<>((J.Binary) parent)); - } - } - } - } - - 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 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), - 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 static class RemoveUnnecessaryNullCheck

extends JavaVisitor

{ - - 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) { - return (J) tree; - } - return super.visit(tree, p); - } - - @Override - public J visitBinary(J.Binary binary, P p) { - if (scope.isScope(binary)) { - done = true; - return binary.getRight().withPrefix(binary.getPrefix()); - } - return super.visitBinary(binary, p); - } - } -} From ac5271092dc9711fc703e0048b5a6c283a394ee0 Mon Sep 17 00:00:00 2001 From: Vincent Potucek Date: Sun, 9 Feb 2025 15:20:57 +0100 Subject: [PATCH 20/28] inline --- .../staticanalysis/EqualsAvoidsNull.java | 52 ++++++++++--------- 1 file changed, 28 insertions(+), 24 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNull.java b/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNull.java index 6149c0a8f..0b53d110f 100644 --- a/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNull.java +++ b/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNull.java @@ -67,32 +67,42 @@ public Duration getEstimatedEffortPerOccurrence() { return ofMinutes(10); } + private static final String JAVA_LANG_STRING = "java.lang.String"; + @Override public TreeVisitor getVisitor() { return Preconditions.check( Preconditions.or( - new UsesMethod<>("java.lang.String contentEquals(..)"), - new UsesMethod<>("java.lang.String equals(..)"), - new UsesMethod<>("java.lang.String equalsIgnoreCase(..)")), + new UsesMethod<>(JAVA_LANG_STRING + " contentEquals(..)"), + new UsesMethod<>(JAVA_LANG_STRING + " equals(..)"), + new UsesMethod<>(JAVA_LANG_STRING + " equalsIgnoreCase(..)")), new JavaIsoVisitor() { + @Override public J visit(@Nullable Tree tree, ExecutionContext ctx) { if (tree instanceof JavaSourceFile) { JavaSourceFile cu = (JavaSourceFile) requireNonNull(tree); return new JavaVisitor() { + + private final MethodMatcher EQUALS = + new MethodMatcher(JAVA_LANG_STRING + " equals(java.lang.Object)"); + private final MethodMatcher EQUALS_IGNORE_CASE = + new MethodMatcher(JAVA_LANG_STRING + " equalsIgnoreCase(" + JAVA_LANG_STRING + ")"); + private final MethodMatcher CONTENT_EQUALS = new MethodMatcher(JAVA_LANG_STRING + + " contentEquals(java.lang.CharSequence)"); + @Override public J visitMethodInvocation(J.MethodInvocation method, ExecutionContext p) { J.MethodInvocation m = (J.MethodInvocation) super.visitMethodInvocation(method, p); - boolean stringComparisonMethod = isStringComparisonMethod(m); - if (m.getSelect() != null && !(m.getSelect() instanceof J.Literal) && - stringComparisonMethod && hasCompatibleArgument(m)) { - - maybeHandleParentBinary(m); - + if (m.getSelect() != null + && !(m.getSelect() instanceof J.Literal) + && isStringComparisonMethod(m) + && hasCompatibleArgument(m)) { + maybeHandleParentBinary(m, getCursor().getParentTreeCursor().getValue()); Expression firstArgument = m.getArguments().get(0); - return firstArgument.getType() == JavaType.Primitive.Null ? - literalsFirstInComparisonsNull(m, firstArgument) : - literalsFirstInComparisons(m, firstArgument); + return firstArgument.getType() == JavaType.Primitive.Null + ? literalsFirstInComparisonsNull(m, firstArgument) + : literalsFirstInComparisons(m, firstArgument); } return m; } @@ -122,14 +132,15 @@ private boolean isStringComparisonMethod(J.MethodInvocation methodInvocation) { CONTENT_EQUALS.matches(methodInvocation); } - private void maybeHandleParentBinary(J.MethodInvocation m) { - Tree parent = getCursor().getParentTreeCursor().getValue(); + private void maybeHandleParentBinary(J.MethodInvocation m, final Tree parent) { if (parent instanceof J.Binary) { if (((J.Binary) parent).getOperator() == J.Binary.Type.And && ((J.Binary) parent).getLeft() instanceof J.Binary) { J.Binary potentialNullCheck = (J.Binary) ((J.Binary) parent).getLeft(); - if (isNullLiteral(potentialNullCheck.getLeft()) && matchesSelect(potentialNullCheck.getRight(), - requireNonNull(m.getSelect())) || - isNullLiteral(potentialNullCheck.getRight()) && matchesSelect(potentialNullCheck.getLeft(), + if (isNullLiteral(potentialNullCheck.getLeft()) + && matchesSelect(potentialNullCheck.getRight(), + requireNonNull(m.getSelect())) + || isNullLiteral(potentialNullCheck.getRight()) + && matchesSelect(potentialNullCheck.getLeft(), requireNonNull(m.getSelect()))) { doAfterVisit(new JavaVisitor() { private final J.Binary scope = (J.Binary) parent; @@ -166,13 +177,6 @@ private boolean matchesSelect(Expression expression, Expression select) { .equals(select.printTrimmed(getCursor()).replaceAll("\\s", "")); } - private final MethodMatcher EQUALS = new MethodMatcher("java.lang.String equals(java" + - ".lang.Object)"); - private final MethodMatcher EQUALS_IGNORE_CASE = new MethodMatcher("java.lang.String " + - "equalsIgnoreCase(java.lang.String)"); - private final MethodMatcher CONTENT_EQUALS = new MethodMatcher("java.lang.String " + - "contentEquals(java.lang.CharSequence)"); - private J.Binary literalsFirstInComparisonsNull(J.MethodInvocation m, Expression firstArgument) { return new J.Binary(Tree.randomId(), From f6016dd5fcc8b0f5706cef1b07a099ddee500a95 Mon Sep 17 00:00:00 2001 From: Vincent Potucek Date: Sun, 9 Feb 2025 15:22:25 +0100 Subject: [PATCH 21/28] inline --- .../staticanalysis/EqualsAvoidsNull.java | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNull.java b/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNull.java index 0b53d110f..15ad7fa2f 100644 --- a/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNull.java +++ b/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNull.java @@ -126,15 +126,19 @@ private boolean hasCompatibleArgument(J.MethodInvocation m) { } private boolean isStringComparisonMethod(J.MethodInvocation methodInvocation) { - return EQUALS.matches(methodInvocation) || - (!ObjectUtils.defaultIfNull(cu.getStyle(EqualsAvoidsNullStyle.class), - Checkstyle.equalsAvoidsNull()).getIgnoreEqualsIgnoreCase() && EQUALS_IGNORE_CASE.matches(methodInvocation)) || - CONTENT_EQUALS.matches(methodInvocation); + return EQUALS.matches(methodInvocation) + || !ObjectUtils + .defaultIfNull(cu.getStyle(EqualsAvoidsNullStyle.class), + Checkstyle.equalsAvoidsNull()) + .getIgnoreEqualsIgnoreCase() + && EQUALS_IGNORE_CASE.matches(methodInvocation) + || CONTENT_EQUALS.matches(methodInvocation); } private void maybeHandleParentBinary(J.MethodInvocation m, final Tree parent) { if (parent instanceof J.Binary) { - if (((J.Binary) parent).getOperator() == J.Binary.Type.And && ((J.Binary) parent).getLeft() instanceof J.Binary) { + if (((J.Binary) parent).getOperator() == J.Binary.Type.And + && ((J.Binary) parent).getLeft() instanceof J.Binary) { J.Binary potentialNullCheck = (J.Binary) ((J.Binary) parent).getLeft(); if (isNullLiteral(potentialNullCheck.getLeft()) && matchesSelect(potentialNullCheck.getRight(), @@ -143,6 +147,7 @@ && matchesSelect(potentialNullCheck.getRight(), && matchesSelect(potentialNullCheck.getLeft(), requireNonNull(m.getSelect()))) { doAfterVisit(new JavaVisitor() { + private final J.Binary scope = (J.Binary) parent; private boolean done; From 8b8bd102711a78d95516741a36dce76849a0a439 Mon Sep 17 00:00:00 2001 From: Vincent Potucek Date: Sun, 9 Feb 2025 15:23:18 +0100 Subject: [PATCH 22/28] inline --- .../org/openrewrite/staticanalysis/EqualsAvoidsNull.java | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNull.java b/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNull.java index 15ad7fa2f..d3233013b 100644 --- a/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNull.java +++ b/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNull.java @@ -153,10 +153,9 @@ && matchesSelect(potentialNullCheck.getLeft(), @Override public @Nullable J visit(@Nullable Tree tree, ExecutionContext p) { - if (done) { - return (J) tree; - } - return super.visit(tree, p); + return done + ? (J) tree + : super.visit(tree, p); } @Override From 74cd190689e6be4326015f2e9456765139240c86 Mon Sep 17 00:00:00 2001 From: Vincent Potucek Date: Sun, 9 Feb 2025 16:09:39 +0100 Subject: [PATCH 23/28] form --- .../org/openrewrite/staticanalysis/EqualsAvoidsNull.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNull.java b/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNull.java index d3233013b..4ea346b7e 100644 --- a/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNull.java +++ b/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNull.java @@ -54,7 +54,10 @@ public String getDisplayName() { @Override public String getDescription() { - return "Checks that any combination of String literals is on the left side of an `equals()` comparison. Also checks for String literals assigned to some field (such as `someString.equals(anotherString = \"text\"))`."; + return """ + Checks that any combination of String literals is on the left side of an `equals()` comparison. + Also checks for String literals assigned to some field (such as `someString.equals(anotherString = "text"))`. + """; } @Override From 46f143bf21fa83387c4c2e378a8374a676efbf8d Mon Sep 17 00:00:00 2001 From: Vincent Potucek Date: Sun, 9 Feb 2025 16:14:59 +0100 Subject: [PATCH 24/28] Revert "form" This reverts commit 74cd190689e6be4326015f2e9456765139240c86. --- .../org/openrewrite/staticanalysis/EqualsAvoidsNull.java | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNull.java b/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNull.java index 4ea346b7e..d3233013b 100644 --- a/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNull.java +++ b/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNull.java @@ -54,10 +54,7 @@ public String getDisplayName() { @Override public String getDescription() { - return """ - Checks that any combination of String literals is on the left side of an `equals()` comparison. - Also checks for String literals assigned to some field (such as `someString.equals(anotherString = "text"))`. - """; + return "Checks that any combination of String literals is on the left side of an `equals()` comparison. Also checks for String literals assigned to some field (such as `someString.equals(anotherString = \"text\"))`."; } @Override From 5818a6e0d2f93321bbc78ae9721b51f25c2da1ec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Merlin=20B=C3=B6gershausen?= Date: Mon, 10 Feb 2025 16:01:52 +0100 Subject: [PATCH 25/28] remove duplicated tests, wrapping visitor and clean-ups --- .../staticanalysis/EqualsAvoidsNull.java | 226 ++++++++---------- .../staticanalysis/EqualsAvoidsNullTest.java | 64 +---- 2 files changed, 106 insertions(+), 184 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNull.java b/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNull.java index d3233013b..2aff357d0 100644 --- a/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNull.java +++ b/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNull.java @@ -15,6 +15,8 @@ */ package org.openrewrite.staticanalysis; +import lombok.EqualsAndHashCode; +import lombok.Value; import org.apache.commons.lang3.ObjectUtils; import org.jspecify.annotations.Nullable; import org.openrewrite.ExecutionContext; @@ -45,8 +47,16 @@ import static java.util.Collections.singletonList; import static java.util.Objects.requireNonNull; +@Value +@EqualsAndHashCode(callSuper = false) public class EqualsAvoidsNull extends Recipe { + private static final String JAVA_LANG_STRING = "java.lang.String"; + + 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 CONTENT_EQUALS = new MethodMatcher(JAVA_LANG_STRING + " contentEquals(java.lang.CharSequence)"); + @Override public String getDisplayName() { return "Equals avoids null"; @@ -67,142 +77,114 @@ public Duration getEstimatedEffortPerOccurrence() { return ofMinutes(10); } - private static final String JAVA_LANG_STRING = "java.lang.String"; - @Override public TreeVisitor getVisitor() { return Preconditions.check( - Preconditions.or( - new UsesMethod<>(JAVA_LANG_STRING + " contentEquals(..)"), - new UsesMethod<>(JAVA_LANG_STRING + " equals(..)"), - new UsesMethod<>(JAVA_LANG_STRING + " equalsIgnoreCase(..)")), - new JavaIsoVisitor() { - + Preconditions.or(new UsesMethod<>(EQUALS), new UsesMethod<>(EQUALS_IGNORE_CASE), new UsesMethod<>(CONTENT_EQUALS)), + new JavaVisitor() { @Override - public J visit(@Nullable Tree tree, ExecutionContext ctx) { - if (tree instanceof JavaSourceFile) { - JavaSourceFile cu = (JavaSourceFile) requireNonNull(tree); - return new JavaVisitor() { - - private final MethodMatcher EQUALS = - new MethodMatcher(JAVA_LANG_STRING + " equals(java.lang.Object)"); - private final MethodMatcher EQUALS_IGNORE_CASE = - new MethodMatcher(JAVA_LANG_STRING + " equalsIgnoreCase(" + JAVA_LANG_STRING + ")"); - private final MethodMatcher CONTENT_EQUALS = new MethodMatcher(JAVA_LANG_STRING - + " contentEquals(java.lang.CharSequence)"); - - @Override - public J visitMethodInvocation(J.MethodInvocation method, ExecutionContext p) { - J.MethodInvocation m = (J.MethodInvocation) super.visitMethodInvocation(method, p); - if (m.getSelect() != null - && !(m.getSelect() instanceof J.Literal) - && isStringComparisonMethod(m) - && hasCompatibleArgument(m)) { - maybeHandleParentBinary(m, getCursor().getParentTreeCursor().getValue()); - Expression firstArgument = m.getArguments().get(0); - return firstArgument.getType() == JavaType.Primitive.Null - ? literalsFirstInComparisonsNull(m, firstArgument) - : literalsFirstInComparisons(m, firstArgument); - } - return m; - } + public J visitMethodInvocation(J.MethodInvocation method, ExecutionContext ctx) { + J.MethodInvocation m = (J.MethodInvocation) super.visitMethodInvocation(method, ctx); - private boolean hasCompatibleArgument(J.MethodInvocation m) { - if (m.getArguments().isEmpty()) { - return false; - } - Expression firstArgument = m.getArguments().get(0); - if (firstArgument instanceof J.Literal) { - return true; - } - if (firstArgument instanceof J.FieldAccess) { - firstArgument = ((J.FieldAccess) firstArgument).getName(); - } - if (firstArgument instanceof J.Identifier) { - JavaType.Variable fieldType = ((J.Identifier) firstArgument).getFieldType(); - return fieldType != null && fieldType.hasFlags(Flag.Static, Flag.Final); - } - return false; - } + if (!isStringComparisonMethod(m) || !hasCompatibleArgument(m)) { + return m; + } - private boolean isStringComparisonMethod(J.MethodInvocation methodInvocation) { - return EQUALS.matches(methodInvocation) - || !ObjectUtils - .defaultIfNull(cu.getStyle(EqualsAvoidsNullStyle.class), - Checkstyle.equalsAvoidsNull()) - .getIgnoreEqualsIgnoreCase() - && EQUALS_IGNORE_CASE.matches(methodInvocation) - || CONTENT_EQUALS.matches(methodInvocation); - } + maybeHandleParentBinary(m, getCursor().getParentTreeCursor().getValue()); + Expression firstArgument = m.getArguments().get(0); + + return firstArgument.getType() == JavaType.Primitive.Null + ? literalsFirstInComparisonsNull(m, firstArgument) + : literalsFirstInComparisons(m, firstArgument); + + } - private void maybeHandleParentBinary(J.MethodInvocation m, final Tree parent) { - if (parent instanceof J.Binary) { - if (((J.Binary) parent).getOperator() == J.Binary.Type.And - && ((J.Binary) parent).getLeft() instanceof J.Binary) { - J.Binary potentialNullCheck = (J.Binary) ((J.Binary) parent).getLeft(); - if (isNullLiteral(potentialNullCheck.getLeft()) - && matchesSelect(potentialNullCheck.getRight(), - requireNonNull(m.getSelect())) - || isNullLiteral(potentialNullCheck.getRight()) - && matchesSelect(potentialNullCheck.getLeft(), - requireNonNull(m.getSelect()))) { - doAfterVisit(new JavaVisitor() { - - private final J.Binary scope = (J.Binary) parent; - private boolean done; - - @Override - public @Nullable J visit(@Nullable Tree tree, ExecutionContext p) { - return done - ? (J) tree - : super.visit(tree, p); - } - - @Override - public J visitBinary(J.Binary binary, ExecutionContext p) { - if (scope.isScope(binary)) { - done = true; - return binary.getRight().withPrefix(binary.getPrefix()); - } - return super.visitBinary(binary, p); - } - }); + private boolean hasCompatibleArgument(J.MethodInvocation m) { + if (m.getArguments().isEmpty()) { + return false; + } + Expression firstArgument = m.getArguments().get(0); + if (firstArgument instanceof J.Literal) { + return true; + } + if (firstArgument instanceof J.FieldAccess) { + firstArgument = ((J.FieldAccess) firstArgument).getName(); + } + if (firstArgument instanceof J.Identifier) { + JavaType.Variable fieldType = ((J.Identifier) firstArgument).getFieldType(); + return fieldType != null && fieldType.hasFlags(Flag.Static, Flag.Final); + } + return false; + } + + private boolean isStringComparisonMethod(J.MethodInvocation methodInvocation) { + return EQUALS.matches(methodInvocation) + || EQUALS_IGNORE_CASE.matches(methodInvocation) + || CONTENT_EQUALS.matches(methodInvocation); + } + + private void maybeHandleParentBinary(J.MethodInvocation m, final Tree parent) { + if (parent instanceof J.Binary) { + if (((J.Binary) parent).getOperator() == J.Binary.Type.And + && ((J.Binary) parent).getLeft() instanceof J.Binary) { + J.Binary potentialNullCheck = (J.Binary) ((J.Binary) parent).getLeft(); + if (isNullLiteral(potentialNullCheck.getLeft()) + && matchesSelect(potentialNullCheck.getRight(), + requireNonNull(m.getSelect())) + || isNullLiteral(potentialNullCheck.getRight()) + && matchesSelect(potentialNullCheck.getLeft(), + requireNonNull(m.getSelect()))) { + doAfterVisit(new JavaVisitor() { + + private final J.Binary scope = (J.Binary) parent; + private boolean done; + + @Override + public @Nullable J visit(@Nullable Tree tree, ExecutionContext ctx) { + return done + ? (J) tree + : super.visit(tree, ctx); + } + + @Override + public J visitBinary(J.Binary binary, ExecutionContext ctx) { + if (scope.isScope(binary)) { + done = true; + return binary.getRight().withPrefix(binary.getPrefix()); } + return super.visitBinary(binary, ctx); } - } + }); } + } + } + } - private boolean isNullLiteral(Expression expression) { - return expression instanceof J.Literal && ((J.Literal) expression).getType() == JavaType.Primitive.Null; - } + 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 boolean matchesSelect(Expression expression, Expression select) { + return expression.printTrimmed(getCursor()).replaceAll("\\s", "") + .equals(select.printTrimmed(getCursor()).replaceAll("\\s", "")); + } - private 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), - firstArgument.withPrefix(Space.SINGLE_SPACE), - JavaType.Primitive.Boolean); - } + private 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), + firstArgument.withPrefix(Space.SINGLE_SPACE), + JavaType.Primitive.Boolean); + } - private J.MethodInvocation literalsFirstInComparisons(J.MethodInvocation m, - Expression firstArgument) { - return m.withSelect(firstArgument.withPrefix(requireNonNull(m.getSelect()).getPrefix())) - .withArguments(singletonList(m.getSelect().withPrefix(Space.EMPTY))); - } - }.visitNonNull(cu, ctx); - } - //noinspection DataFlowIssue - return (J) tree; + private J.MethodInvocation literalsFirstInComparisons(J.MethodInvocation m, + Expression firstArgument) { + return m.withSelect(firstArgument.withPrefix(requireNonNull(m.getSelect()).getPrefix())) + .withArguments(singletonList(m.getSelect().withPrefix(Space.EMPTY))); } - } - ); + }); } } diff --git a/src/test/java/org/openrewrite/staticanalysis/EqualsAvoidsNullTest.java b/src/test/java/org/openrewrite/staticanalysis/EqualsAvoidsNullTest.java index b04cae415..c3f0cb6fa 100644 --- a/src/test/java/org/openrewrite/staticanalysis/EqualsAvoidsNullTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/EqualsAvoidsNullTest.java @@ -64,64 +64,6 @@ public class A { ); } - @Issue("https://github.com/openrewrite/rewrite-static-analysis/issues/362") - @Issue("https://github.com/openrewrite/rewrite-static-analysis/issues/442") - @DocumentExample - @Test - void invertConditionalButKeepComparisonOrder() { - rewriteRun( - //language=java - java( - """ - class A { - boolean bar(String x) { - x.compareTo("2"); // should be x.compareTo("2") for stable compare logic - x.compareToIgnoreCase("2"); // should be x.compareToIgnoreCase("2") for stable compare logic - x.contentEquals("2"); // should be "2".contentEquals(x) - x.equals("2"); // should be "2".equals(x) - x.equalsIgnoreCase("2"); // should be "2".equalsIgnoreCase(x) - return x.contentEquals("2"); // should be "2".contentEquals(x) - } - } - """, - """ - class A { - boolean bar(String x) { - x.compareTo("2"); // should be x.compareTo("2") for stable compare logic - x.compareToIgnoreCase("2"); // should be x.compareToIgnoreCase("2") for stable compare logic - "2".contentEquals(x); // should be "2".contentEquals(x) - "2".equals(x); // should be "2".equals(x) - "2".equalsIgnoreCase(x); // should be "2".equalsIgnoreCase(x) - return "2".contentEquals(x); // should be "2".contentEquals(x) - } - } - """) - ); - } - - @Issue("https://github.com/openrewrite/rewrite-static-analysis/issues/362") - @Issue("https://github.com/openrewrite/rewrite-static-analysis/issues/442") - @DocumentExample - @Test - void keepOrderForSameType() { - rewriteRun( - //language=java - java( - """ - class A { - void bar(String x) { - String o = null; - o.equals(x); - o.equalsIgnoreCase(x); - o.contentEquals(x); - o.compareTo(x); - o.compareToIgnoreCase(x); - } - } - """) - ); - } - @Test void removeUnnecessaryNullCheck() { rewriteRun( @@ -215,8 +157,7 @@ void chainedMethodCalls() { public class Constants { public static final String FOO = "FOO"; } - """, - SourceSpec::skip + """ ), java( """ @@ -228,8 +169,7 @@ Foo getFOO() { return this; } } - """, - SourceSpec::skip + """ ), java( """ From c20e7776a365a0270c71aac7183e11ce43d9c155 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Merlin=20B=C3=B6gershausen?= Date: Mon, 10 Feb 2025 16:07:38 +0100 Subject: [PATCH 26/28] fix style and import --- .../staticanalysis/EqualsAvoidsNull.java | 34 +++++-------------- 1 file changed, 9 insertions(+), 25 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNull.java b/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNull.java index 2aff357d0..59013d6e9 100644 --- a/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNull.java +++ b/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNull.java @@ -17,26 +17,12 @@ import lombok.EqualsAndHashCode; import lombok.Value; -import org.apache.commons.lang3.ObjectUtils; import org.jspecify.annotations.Nullable; -import org.openrewrite.ExecutionContext; -import org.openrewrite.Preconditions; -import org.openrewrite.Recipe; -import org.openrewrite.Tree; -import org.openrewrite.TreeVisitor; -import org.openrewrite.java.JavaIsoVisitor; +import org.openrewrite.*; import org.openrewrite.java.JavaVisitor; import org.openrewrite.java.MethodMatcher; import org.openrewrite.java.search.UsesMethod; -import org.openrewrite.java.style.Checkstyle; -import org.openrewrite.java.style.EqualsAvoidsNullStyle; -import org.openrewrite.java.tree.Expression; -import org.openrewrite.java.tree.Flag; -import org.openrewrite.java.tree.J; -import org.openrewrite.java.tree.JLeftPadded; -import org.openrewrite.java.tree.JavaSourceFile; -import org.openrewrite.java.tree.JavaType; -import org.openrewrite.java.tree.Space; +import org.openrewrite.java.tree.*; import org.openrewrite.marker.Markers; import java.time.Duration; @@ -93,9 +79,9 @@ public J visitMethodInvocation(J.MethodInvocation method, ExecutionContext ctx) maybeHandleParentBinary(m, getCursor().getParentTreeCursor().getValue()); Expression firstArgument = m.getArguments().get(0); - return firstArgument.getType() == JavaType.Primitive.Null - ? literalsFirstInComparisonsNull(m, firstArgument) - : literalsFirstInComparisons(m, firstArgument); + return firstArgument.getType() == JavaType.Primitive.Null ? + literalsFirstInComparisonsNull(m, firstArgument) : + literalsFirstInComparisons(m, firstArgument); } @@ -118,9 +104,9 @@ private boolean hasCompatibleArgument(J.MethodInvocation m) { } private boolean isStringComparisonMethod(J.MethodInvocation methodInvocation) { - return EQUALS.matches(methodInvocation) - || EQUALS_IGNORE_CASE.matches(methodInvocation) - || CONTENT_EQUALS.matches(methodInvocation); + return EQUALS.matches(methodInvocation) || + EQUALS_IGNORE_CASE.matches(methodInvocation) || + CONTENT_EQUALS.matches(methodInvocation); } private void maybeHandleParentBinary(J.MethodInvocation m, final Tree parent) { @@ -141,9 +127,7 @@ && matchesSelect(potentialNullCheck.getLeft(), @Override public @Nullable J visit(@Nullable Tree tree, ExecutionContext ctx) { - return done - ? (J) tree - : super.visit(tree, ctx); + return done ? (J) tree : super.visit(tree, ctx); } @Override From fe68ae144bf52706468541fd61b8bbbf6408e911 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Merlin=20B=C3=B6gershausen?= Date: Mon, 10 Feb 2025 16:10:55 +0100 Subject: [PATCH 27/28] Update src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNull.java Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> --- .../staticanalysis/EqualsAvoidsNull.java | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNull.java b/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNull.java index 59013d6e9..eaedb5e9a 100644 --- a/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNull.java +++ b/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNull.java @@ -111,13 +111,13 @@ private boolean isStringComparisonMethod(J.MethodInvocation methodInvocation) { private void maybeHandleParentBinary(J.MethodInvocation m, final Tree parent) { if (parent instanceof J.Binary) { - if (((J.Binary) parent).getOperator() == J.Binary.Type.And - && ((J.Binary) parent).getLeft() instanceof J.Binary) { - J.Binary potentialNullCheck = (J.Binary) ((J.Binary) parent).getLeft(); - if (isNullLiteral(potentialNullCheck.getLeft()) - && matchesSelect(potentialNullCheck.getRight(), - requireNonNull(m.getSelect())) - || isNullLiteral(potentialNullCheck.getRight()) + if (((J.Binary) parent).getOperator() == J.Binary.Type.And && + ((J.Binary) parent).getLeft() instanceof J.Binary) { + if (isNullLiteral(potentialNullCheck.getLeft()) && + matchesSelect(potentialNullCheck.getRight(), + requireNonNull(m.getSelect())) || + isNullLiteral(potentialNullCheck.getRight()) && + matchesSelect(potentialNullCheck.getLeft(), && matchesSelect(potentialNullCheck.getLeft(), requireNonNull(m.getSelect()))) { doAfterVisit(new JavaVisitor() { From ce96ade313ff68d525c35031f91f21ea16144c0a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Merlin=20B=C3=B6gershausen?= Date: Mon, 10 Feb 2025 16:25:59 +0100 Subject: [PATCH 28/28] reintroduced variable after GitHUb removed it.. --- .../org/openrewrite/staticanalysis/EqualsAvoidsNull.java | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNull.java b/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNull.java index eaedb5e9a..71041e2eb 100644 --- a/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNull.java +++ b/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNull.java @@ -113,13 +113,11 @@ private void maybeHandleParentBinary(J.MethodInvocation m, final Tree parent) { if (parent instanceof J.Binary) { if (((J.Binary) parent).getOperator() == J.Binary.Type.And && ((J.Binary) parent).getLeft() instanceof J.Binary) { + J.Binary potentialNullCheck = (J.Binary) ((J.Binary) parent).getLeft(); if (isNullLiteral(potentialNullCheck.getLeft()) && - matchesSelect(potentialNullCheck.getRight(), - requireNonNull(m.getSelect())) || + matchesSelect(potentialNullCheck.getRight(), requireNonNull(m.getSelect())) || isNullLiteral(potentialNullCheck.getRight()) && - matchesSelect(potentialNullCheck.getLeft(), - && matchesSelect(potentialNullCheck.getLeft(), - requireNonNull(m.getSelect()))) { + matchesSelect(potentialNullCheck.getLeft(), requireNonNull(m.getSelect()))) { doAfterVisit(new JavaVisitor() { private final J.Binary scope = (J.Binary) parent;