From 3d0a14ef681c935b9d77f88bdca146bd559cdedd Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Mon, 27 Jul 2026 11:59:41 +0200 Subject: [PATCH] Do not treat missing type attribution as proof of semantic equality `SemanticallyEqual` considered `this.x` and a bare `x` equal whenever both sides lacked a `JavaType.Variable`, because `TypeUtils.isOfType(null, null)` is `true`. On LSTs with incomplete type attribution that made a constructor's `this.x = x` look like a self-assignment, and `RemoveSelfAssignment` deleted it; where the field is `private final` the result no longer compiles. `this.x` is equivalent to `x` only when `x` resolves to the field rather than to a shadowing parameter or local, which cannot be established without type information in any language. Both directions of the `J.FieldAccess` / `J.Identifier` comparison now require a field type on both sides, keeping the carve-out for type references such as `java.util.regex.Pattern` vs `Pattern`, where a null field type is expected rather than missing. Comparisons between two identifiers still fall back to name equality, which is what languages that never attribute field types rely on. See https://github.com/openrewrite/rewrite-static-analysis/issues/953 --- .../groovy/search/SemanticallyEqualTest.java | 74 +++++++++++++++++++ .../java/search/SemanticallyEqualTest.java | 28 +++++++ .../java/search/SemanticallyEqual.java | 18 ++++- 3 files changed, 118 insertions(+), 2 deletions(-) create mode 100644 rewrite-groovy/src/test/java/org/openrewrite/groovy/search/SemanticallyEqualTest.java diff --git a/rewrite-groovy/src/test/java/org/openrewrite/groovy/search/SemanticallyEqualTest.java b/rewrite-groovy/src/test/java/org/openrewrite/groovy/search/SemanticallyEqualTest.java new file mode 100644 index 00000000000..a39115aaa77 --- /dev/null +++ b/rewrite-groovy/src/test/java/org/openrewrite/groovy/search/SemanticallyEqualTest.java @@ -0,0 +1,74 @@ +/* + * Copyright 2025 the original author or authors. + *

+ * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + *

+ * https://www.apache.org/licenses/LICENSE-2.0 + *

+ * 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.groovy.search; + +import org.junit.jupiter.api.Test; +import org.openrewrite.InMemoryExecutionContext; +import org.openrewrite.Issue; +import org.openrewrite.groovy.GroovyParser; +import org.openrewrite.groovy.tree.G; +import org.openrewrite.java.search.SemanticallyEqual; +import org.openrewrite.java.tree.J; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * The Groovy parser does not attribute {@link J.Identifier#getFieldType()} on references, so these cases exercise + * what {@code SemanticallyEqual} may conclude when type information is systemically absent rather than merely missing. + */ +class SemanticallyEqualTest { + + @Issue("https://github.com/openrewrite/rewrite-static-analysis/issues/953") + @Test + void fieldNotEqualToShadowedNameVariable() { + J.Assignment assignment = firstAssignment(""" + class A { + private final boolean readOnly + A(boolean readOnly) { + this.readOnly = readOnly + } + } + """); + assertThat(SemanticallyEqual.areEqual(assignment.getVariable(), assignment.getAssignment())).isFalse(); + assertThat(SemanticallyEqual.areEqual(assignment.getAssignment(), assignment.getVariable())).isFalse(); + } + + @Test + void identifiersWithSameNameRemainEqual() { + J.Assignment assignment = firstAssignment(""" + class A { + void m(boolean readOnly) { + readOnly = readOnly + } + } + """); + assertThat(SemanticallyEqual.areEqual(assignment.getVariable(), assignment.getAssignment())).isTrue(); + } + + private J.Assignment firstAssignment(String source) { + G.CompilationUnit cu = GroovyParser.builder().build() + .parse(new InMemoryExecutionContext(), source) + .findFirst() + .map(G.CompilationUnit.class::cast) + .orElseThrow(); + J.ClassDeclaration clazz = (J.ClassDeclaration) cu.getStatements().getFirst(); + J.MethodDeclaration method = (J.MethodDeclaration) clazz.getBody().getStatements().stream() + .filter(J.MethodDeclaration.class::isInstance) + .findFirst() + .orElseThrow(); + return (J.Assignment) method.getBody().getStatements().getFirst(); + } +} 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 7ddb5177497..ad6ba7e0cf4 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 @@ -20,6 +20,7 @@ import org.junit.jupiter.api.Test; import org.junitpioneer.jupiter.ExpectedToFail; import org.junitpioneer.jupiter.cartesian.CartesianTest; +import org.openrewrite.Issue; import org.openrewrite.java.JavaIsoVisitor; import org.openrewrite.java.JavaParser; import org.openrewrite.java.tree.J; @@ -436,6 +437,33 @@ void m(int a) { assertFalse(SemanticallyEqual.areEqual(((J.FieldAccess) a.getVariable()).getName(), a.getAssignment())); } + @Issue("https://github.com/openrewrite/rewrite-static-analysis/issues/953") + @Test + void fieldNotEqualToShadowedNameVariableWithoutTypeAttribution() { + J.CompilationUnit cu = javaParser.parse( + """ + class T { + int a = 1; + void m(int a) { + this.a = a; + } + } + """ + ).findFirst() + .map(J.CompilationUnit.class::cast) + .get(); + J.CompilationUnit unattributed = (J.CompilationUnit) new JavaIsoVisitor() { + @Override + public J.Identifier visitIdentifier(J.Identifier identifier, Integer p) { + return identifier.withFieldType(null).withType(null); + } + }.visitNonNull(cu, 0); + J.MethodDeclaration m = (J.MethodDeclaration) unattributed.getClasses().getFirst().getBody().getStatements().get(1); + J.Assignment a = (J.Assignment) m.getBody().getStatements().getFirst(); + assertFalse(SemanticallyEqual.areEqual(a.getVariable(), a.getAssignment())); + assertFalse(SemanticallyEqual.areEqual(a.getAssignment(), a.getVariable())); + } + @Test void fieldsFromDifferentInstances() { J.CompilationUnit cu = javaParser.parse( 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 27452c2a556..53adc031663 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 @@ -608,10 +608,14 @@ public J.FieldAccess visitFieldAccess(J.FieldAccess fieldAccess, J j) { !isOfType(fieldType, ((J.Identifier) j).getFieldType()) || !fieldAccess.getSimpleName().equals(((J.Identifier) j).getSimpleName())) { isEqual.set(false); - } else { - if (fieldType != null && !fieldType.hasFlags(Flag.Static)) { + } else if (fieldType == null) { + // A null field type is either a type reference (`java.util.regex.Pattern` vs `Pattern`) + // or a variable whose type attribution is missing; only the former is safe to compare by name. + if (!isTypeReference(fieldAccess.getName()) || !isTypeReference((J.Identifier) j)) { isEqual.set(false); } + } else if (!fieldType.hasFlags(Flag.Static)) { + isEqual.set(false); } return fieldAccess; } @@ -710,6 +714,10 @@ public J.Identifier visitIdentifier(J.Identifier identifier, J j) { // Consider implicit-this "a" equivalent to "this.a" // Definitely does not account for every possible edge case around "this", but handles the common case if (fieldTarget instanceof J.Identifier && "this".equals(((J.Identifier) fieldTarget).getSimpleName())) { + if (identifier.getFieldType() == null || field.getName().getFieldType() == null) { + isEqual.set(false); + return identifier; + } visit(identifier, field.getName()); return identifier; } @@ -1561,6 +1569,12 @@ public N visitTypeName(N firstTypeName, J j) { return firstTypeName; } + private static boolean isTypeReference(J.Identifier identifier) { + return identifier.getFieldType() == null && + identifier.getType() instanceof JavaType.FullyQualified && + !(identifier.getType() instanceof JavaType.Unknown); + } + protected boolean isOfType(JavaType target, JavaType source) { return TypeUtils.isOfType(target, source); }