From 6988ee39716921cd9a77953ad07e9a4dd182effa Mon Sep 17 00:00:00 2001 From: Knut Wannheden Date: Sat, 16 Dec 2023 01:30:28 +0100 Subject: [PATCH 1/6] `SemanticallyEqual`: Fix bug in `visitFieldAccess()` When comparing `J.FieldAccess` instances, the targets also needs to be compared. --- .../SimplifyBooleanExpressionVisitorTest.java | 22 +++++++++++++++++++ .../java/search/SemanticallyEqual.java | 6 +++++ 2 files changed, 28 insertions(+) diff --git a/rewrite-java-test/src/test/java/org/openrewrite/java/cleanup/SimplifyBooleanExpressionVisitorTest.java b/rewrite-java-test/src/test/java/org/openrewrite/java/cleanup/SimplifyBooleanExpressionVisitorTest.java index 1afd3f2a1b2..06886f106ae 100644 --- a/rewrite-java-test/src/test/java/org/openrewrite/java/cleanup/SimplifyBooleanExpressionVisitorTest.java +++ b/rewrite-java-test/src/test/java/org/openrewrite/java/cleanup/SimplifyBooleanExpressionVisitorTest.java @@ -405,6 +405,28 @@ public class A { ); } + @Test + void differentFieldAccesses() { + rewriteRun( + java( + """ + public class A { + Object f = null; + class B extends A { + boolean m(Object o) { + B other = (B) o; + if (this.f == null || other.f == null) { + return true; + } + return false; + } + } + } + """ + ) + ); + } + @Test void preserveComments() { rewriteRun( diff --git a/rewrite-java/src/main/java/org/openrewrite/java/search/SemanticallyEqual.java b/rewrite-java/src/main/java/org/openrewrite/java/search/SemanticallyEqual.java index 30adccf6651..58fa89ae9ba 100644 --- a/rewrite-java/src/main/java/org/openrewrite/java/search/SemanticallyEqual.java +++ b/rewrite-java/src/main/java/org/openrewrite/java/search/SemanticallyEqual.java @@ -587,6 +587,12 @@ public J.FieldAccess visitFieldAccess(J.FieldAccess fieldAccess, J j) { isEqual.set(false); return fieldAccess; } + + if (nullMissMatch(fieldAccess.getTarget(), compareTo.getTarget())) { + isEqual.set(false); + return fieldAccess; + } + visit(fieldAccess.getTarget(), compareTo.getTarget()); } return fieldAccess; } From 9aa6fc411cc75e140ca362b5265fb808eddef946 Mon Sep 17 00:00:00 2001 From: Knut Wannheden Date: Sat, 16 Dec 2023 21:16:16 +0100 Subject: [PATCH 2/6] Start adding some more `SemanticallyEqualTest` test cases --- .../java/search/SemanticallyEqualTest.java | 44 +++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/rewrite-java-test/src/test/java/org/openrewrite/java/search/SemanticallyEqualTest.java b/rewrite-java-test/src/test/java/org/openrewrite/java/search/SemanticallyEqualTest.java index 0891839f8a1..7230b768898 100644 --- a/rewrite-java-test/src/test/java/org/openrewrite/java/search/SemanticallyEqualTest.java +++ b/rewrite-java-test/src/test/java/org/openrewrite/java/search/SemanticallyEqualTest.java @@ -17,9 +17,15 @@ import org.intellij.lang.annotations.Language; import org.junit.jupiter.api.Test; +import org.openrewrite.java.JavaIsoVisitor; import org.openrewrite.java.JavaParser; +import org.openrewrite.java.tree.Expression; import org.openrewrite.java.tree.J; +import java.util.HashMap; +import java.util.Map; +import java.util.Objects; + import static org.junit.jupiter.api.Assertions.assertTrue; public class SemanticallyEqualTest { @@ -67,6 +73,24 @@ class A { ); } + @Test + void compareFieldAccess() { + assertExpressionsEqual(""" + class A { + int n = 1; + int a = A.this.n; + int b = A.this.n; + } + """); + assertExpressionsEqual(""" + class A { + int n = 1; + int a = A.this.n; + int b = this.n; + } + """); + } + private void assertEqualToSelf(@Language("java") String a) { assertEqual(a, a); } @@ -79,6 +103,26 @@ private void assertEqual(@Language("java") String a, @Language("java") String b) assertEqual(cua, cub); } + @SuppressWarnings("OptionalGetWithoutIsPresent") + private void assertExpressionsEqual(@Language(value = "java") String source) { + J.CompilationUnit cu = (J.CompilationUnit) javaParser.parse(source).findFirst().get(); + javaParser.reset(); + + JavaIsoVisitor> visitor = new JavaIsoVisitor<>() { + @Override + public J.VariableDeclarations.NamedVariable visitVariable(J.VariableDeclarations.NamedVariable variable, Map map) { + map.put(variable.getSimpleName(), variable); + return super.visitVariable(variable, map); + } + }; + + Map result = new HashMap<>(); + visitor.visit(cu, result); + Expression ea = result.get("a").getInitializer(); + Expression eb = result.get("b").getInitializer(); + assertEqual(Objects.requireNonNull(ea), Objects.requireNonNull(eb)); + } + private void assertEqual(J a, J b) { assertTrue(SemanticallyEqual.areEqual(a, b)); } From d3ff0cc95d3d0316e9fbd3f4b0762d36879d5a27 Mon Sep 17 00:00:00 2001 From: Knut Wannheden Date: Sat, 16 Dec 2023 21:18:41 +0100 Subject: [PATCH 3/6] Polish --- .../java/search/SemanticallyEqualTest.java | 43 ++++++++++--------- 1 file changed, 23 insertions(+), 20 deletions(-) diff --git a/rewrite-java-test/src/test/java/org/openrewrite/java/search/SemanticallyEqualTest.java b/rewrite-java-test/src/test/java/org/openrewrite/java/search/SemanticallyEqualTest.java index 7230b768898..bbf05ab2480 100644 --- a/rewrite-java-test/src/test/java/org/openrewrite/java/search/SemanticallyEqualTest.java +++ b/rewrite-java-test/src/test/java/org/openrewrite/java/search/SemanticallyEqualTest.java @@ -19,7 +19,6 @@ import org.junit.jupiter.api.Test; import org.openrewrite.java.JavaIsoVisitor; import org.openrewrite.java.JavaParser; -import org.openrewrite.java.tree.Expression; import org.openrewrite.java.tree.J; import java.util.HashMap; @@ -75,20 +74,24 @@ class A { @Test void compareFieldAccess() { - assertExpressionsEqual(""" - class A { - int n = 1; - int a = A.this.n; - int b = A.this.n; - } - """); - assertExpressionsEqual(""" - class A { - int n = 1; - int a = A.this.n; - int b = this.n; - } - """); + assertExpressionsEqual( + """ + class A { + int n = 1; + int a = A.this.n; + int b = A.this.n; + } + """ + ); + assertExpressionsEqual( + """ + class A { + int n = 1; + int a = A.this.n; + int b = this.n; + } + """ + ); } private void assertEqualToSelf(@Language("java") String a) { @@ -116,11 +119,11 @@ public J.VariableDeclarations.NamedVariable visitVariable(J.VariableDeclarations } }; - Map result = new HashMap<>(); - visitor.visit(cu, result); - Expression ea = result.get("a").getInitializer(); - Expression eb = result.get("b").getInitializer(); - assertEqual(Objects.requireNonNull(ea), Objects.requireNonNull(eb)); + Map result = visitor.reduce(cu, new HashMap<>()); + assertEqual( + Objects.requireNonNull(result.get("a").getInitializer()), + Objects.requireNonNull(result.get("b").getInitializer()) + ); } private void assertEqual(J a, J b) { From 552516c0885eacb480b51ca3054d8717198bf368 Mon Sep 17 00:00:00 2001 From: Knut Wannheden Date: Sat, 16 Dec 2023 21:21:34 +0100 Subject: [PATCH 4/6] Polish --- .../java/search/SemanticallyEqualTest.java | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/rewrite-java-test/src/test/java/org/openrewrite/java/search/SemanticallyEqualTest.java b/rewrite-java-test/src/test/java/org/openrewrite/java/search/SemanticallyEqualTest.java index bbf05ab2480..739db1ef362 100644 --- a/rewrite-java-test/src/test/java/org/openrewrite/java/search/SemanticallyEqualTest.java +++ b/rewrite-java-test/src/test/java/org/openrewrite/java/search/SemanticallyEqualTest.java @@ -76,22 +76,31 @@ class A { void compareFieldAccess() { assertExpressionsEqual( """ - class A { + class T { int n = 1; - int a = A.this.n; - int b = A.this.n; + int a = T.this.n; + int b = T.this.n; } """ ); assertExpressionsEqual( """ - class A { + class T { int n = 1; - int a = A.this.n; + int a = T.this.n; int b = this.n; } """ ); + assertExpressionsEqual( + """ + class T { + int n = 1; + int a = T.this.n; + int b = n; + } + """ + ); } private void assertEqualToSelf(@Language("java") String a) { From de28e8271f10f308368ec3a9c625c47d014eb755 Mon Sep 17 00:00:00 2001 From: Knut Wannheden Date: Mon, 18 Dec 2023 08:58:08 +0100 Subject: [PATCH 5/6] Add more tests --- .../java/search/SemanticallyEqualTest.java | 122 +++++++++++++++++- .../java/search/SemanticallyEqual.java | 17 ++- 2 files changed, 129 insertions(+), 10 deletions(-) diff --git a/rewrite-java-test/src/test/java/org/openrewrite/java/search/SemanticallyEqualTest.java b/rewrite-java-test/src/test/java/org/openrewrite/java/search/SemanticallyEqualTest.java index 739db1ef362..2d3bc64d4a0 100644 --- a/rewrite-java-test/src/test/java/org/openrewrite/java/search/SemanticallyEqualTest.java +++ b/rewrite-java-test/src/test/java/org/openrewrite/java/search/SemanticallyEqualTest.java @@ -17,6 +17,7 @@ import org.intellij.lang.annotations.Language; import org.junit.jupiter.api.Test; +import org.junitpioneer.jupiter.cartesian.CartesianTest; import org.openrewrite.java.JavaIsoVisitor; import org.openrewrite.java.JavaParser; import org.openrewrite.java.tree.J; @@ -25,6 +26,7 @@ import java.util.Map; import java.util.Objects; +import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.Assertions.assertTrue; public class SemanticallyEqualTest { @@ -72,6 +74,99 @@ class A { ); } + @Test + void compareClassLiterals() { + assertExpressionsEqual( + """ + import java.util.UUID; + class T { + Class a = java.util.UUID.class; + Class b = UUID.class; + } + """ + ); + assertExpressionsNotEqual( + """ + import java.util.UUID; + class T { + Class u = UUID.class; + Class a = UUID.class; + Class b = this.u; + } + """ + ); + } + + @CartesianTest + void staticFieldAccess(@CartesianTest.Values(strings = { + "java.util.regex.Pattern.CASE_INSENSITIVE", + "Pattern.CASE_INSENSITIVE", + "CASE_INSENSITIVE", + }) String a, @CartesianTest.Values(strings = { + "java.util.regex.Pattern.CASE_INSENSITIVE", + "Pattern.CASE_INSENSITIVE", + "CASE_INSENSITIVE", + }) String b) { + assertExpressionsEqual( + "import java.util.regex.Pattern; import static java.util.regex.Pattern.CASE_INSENSITIVE; class T { int a = " + a + "; int b = " + b + "; }" + ); + } + + @CartesianTest + void staticMethodAccess(@CartesianTest.Values(strings = { + "java.util.regex.Pattern.compile", + "Pattern.compile", + "compile", + }) String a, @CartesianTest.Values(strings = { + "java.util.regex.Pattern.compile", + "Pattern.compile", + "compile", + }) String b) { + assertExpressionsEqual( + "import java.util.regex.Pattern; import static java.util.regex.Pattern.compile; class T { Pattern a = " + a + "(\"\"); Pattern b = " + b + "(\"\"); }" + ); + } + + @Test + void compareTypeCasts() { + assertExpressionsEqual( + """ + class T { + Number a = (java.lang.Number) ""; + Number b = (java.lang.Number) ""; + } + """ + ); + assertExpressionsEqual( + """ + class T { + Number a = (java.lang.Number) ""; + Number b = (Number) ""; + } + """ + ); + assertExpressionsEqual( + """ + import java.util.List; + import java.util.UUID; + class T { + Number a = (List) ""; + Number b = (List) ""; + } + """ + ); + assertExpressionsEqual( + """ + import java.util.List; + import java.util.UUID; + class T { + Number a = (List) ""; + Number b = (java.util.List) ""; + } + """ + ); + } + @Test void compareFieldAccess() { assertExpressionsEqual( @@ -112,6 +207,7 @@ private void assertEqual(@Language("java") String a, @Language("java") String b) J.CompilationUnit cua = (J.CompilationUnit) javaParser.parse(a).findFirst().get(); javaParser.reset(); J.CompilationUnit cub = (J.CompilationUnit) javaParser.parse(b).findFirst().get(); + javaParser.reset(); assertEqual(cua, cub); } @@ -129,10 +225,30 @@ public J.VariableDeclarations.NamedVariable visitVariable(J.VariableDeclarations }; Map result = visitor.reduce(cu, new HashMap<>()); - assertEqual( + assertThat(SemanticallyEqual.areEqual( Objects.requireNonNull(result.get("a").getInitializer()), - Objects.requireNonNull(result.get("b").getInitializer()) - ); + Objects.requireNonNull(result.get("b").getInitializer())) + ).isTrue(); + } + + @SuppressWarnings("OptionalGetWithoutIsPresent") + private void assertExpressionsNotEqual(@Language(value = "java") String source) { + J.CompilationUnit cu = (J.CompilationUnit) javaParser.parse(source).findFirst().get(); + javaParser.reset(); + + JavaIsoVisitor> visitor = new JavaIsoVisitor<>() { + @Override + public J.VariableDeclarations.NamedVariable visitVariable(J.VariableDeclarations.NamedVariable variable, Map map) { + map.put(variable.getSimpleName(), variable); + return super.visitVariable(variable, map); + } + }; + + Map result = visitor.reduce(cu, new HashMap<>()); + assertThat(SemanticallyEqual.areEqual( + Objects.requireNonNull(result.get("a").getInitializer()), + Objects.requireNonNull(result.get("b").getInitializer())) + ).isFalse(); } private void assertEqual(J a, J b) { diff --git a/rewrite-java/src/main/java/org/openrewrite/java/search/SemanticallyEqual.java b/rewrite-java/src/main/java/org/openrewrite/java/search/SemanticallyEqual.java index 58fa89ae9ba..b1ac2f8c7de 100644 --- a/rewrite-java/src/main/java/org/openrewrite/java/search/SemanticallyEqual.java +++ b/rewrite-java/src/main/java/org/openrewrite/java/search/SemanticallyEqual.java @@ -575,23 +575,26 @@ public J.EnumValueSet visitEnumValueSet(J.EnumValueSet enums, J j) { public J.FieldAccess visitFieldAccess(J.FieldAccess fieldAccess, J j) { if (isEqual.get()) { if (!(j instanceof J.FieldAccess)) { - if (!(j instanceof J.Identifier) || !TypeUtils.isOfType(fieldAccess.getName().getFieldType(), ((J.Identifier) j).getFieldType())) { + if (!(j instanceof J.Identifier) || + !TypeUtils.isOfType(fieldAccess.getName().getFieldType(), ((J.Identifier) j).getFieldType()) || + !fieldAccess.getSimpleName().equals(((J.Identifier) j).getSimpleName())) { isEqual.set(false); } return fieldAccess; } J.FieldAccess compareTo = (J.FieldAccess) j; - if (!TypeUtils.isOfType(fieldAccess.getType(), compareTo.getType()) - || !TypeUtils.isOfType(fieldAccess.getName().getFieldType(), compareTo.getName().getFieldType())) { + if (fieldAccess.getType() instanceof JavaType.Unknown && compareTo.getType() instanceof JavaType.Unknown) { + if (!fieldAccess.getSimpleName().equals(compareTo.getSimpleName())) { + isEqual.set(false); + return fieldAccess; + } + } else if (!TypeUtils.isOfType(fieldAccess.getType(), compareTo.getType()) + || !TypeUtils.isOfType(fieldAccess.getName().getFieldType(), compareTo.getName().getFieldType())) { isEqual.set(false); return fieldAccess; } - if (nullMissMatch(fieldAccess.getTarget(), compareTo.getTarget())) { - isEqual.set(false); - return fieldAccess; - } visit(fieldAccess.getTarget(), compareTo.getTarget()); } return fieldAccess; From 7dafbbd67912455a3881abfc0c86a6d6f840c5d9 Mon Sep 17 00:00:00 2001 From: Knut Wannheden Date: Mon, 18 Dec 2023 09:14:12 +0100 Subject: [PATCH 6/6] Add more tests --- .../java/search/SemanticallyEqualTest.java | 27 +++++++++++++------ 1 file changed, 19 insertions(+), 8 deletions(-) diff --git a/rewrite-java-test/src/test/java/org/openrewrite/java/search/SemanticallyEqualTest.java b/rewrite-java-test/src/test/java/org/openrewrite/java/search/SemanticallyEqualTest.java index 2d3bc64d4a0..1f7a6250f1b 100644 --- a/rewrite-java-test/src/test/java/org/openrewrite/java/search/SemanticallyEqualTest.java +++ b/rewrite-java-test/src/test/java/org/openrewrite/java/search/SemanticallyEqualTest.java @@ -34,7 +34,7 @@ public class SemanticallyEqualTest { private final JavaParser javaParser = JavaParser.fromJavaVersion().build(); @Test - void compareAbstractMethods() { + void abstractMethods() { assertEqualToSelf( """ abstract class A { @@ -45,7 +45,7 @@ abstract class A { } @Test - void compareClassModifierLists() { + void classModifierLists() { assertEqual( """ public abstract class A { @@ -59,7 +59,7 @@ abstract public class A { } @Test - void compareLiterals() { + void literals() { assertEqual( """ class A { @@ -75,7 +75,7 @@ class A { } @Test - void compareClassLiterals() { + void classLiterals() { assertExpressionsEqual( """ import java.util.UUID; @@ -98,7 +98,7 @@ class T { } @CartesianTest - void staticFieldAccess(@CartesianTest.Values(strings = { + void staticFieldAccesses(@CartesianTest.Values(strings = { "java.util.regex.Pattern.CASE_INSENSITIVE", "Pattern.CASE_INSENSITIVE", "CASE_INSENSITIVE", @@ -113,7 +113,7 @@ void staticFieldAccess(@CartesianTest.Values(strings = { } @CartesianTest - void staticMethodAccess(@CartesianTest.Values(strings = { + void staticMethodAccesses(@CartesianTest.Values(strings = { "java.util.regex.Pattern.compile", "Pattern.compile", "compile", @@ -128,7 +128,7 @@ void staticMethodAccess(@CartesianTest.Values(strings = { } @Test - void compareTypeCasts() { + void typeCasts() { assertExpressionsEqual( """ class T { @@ -168,7 +168,7 @@ class T { } @Test - void compareFieldAccess() { + void fieldAccesses() { assertExpressionsEqual( """ class T { @@ -196,6 +196,17 @@ class T { } """ ); + assertExpressionsNotEqual( + """ + class T { + int n = 1; + void m(int n) { + int a = T.this.n; + int b = n; + } + } + """ + ); } private void assertEqualToSelf(@Language("java") String a) {