diff --git a/src/main/java/org/openrewrite/staticanalysis/RenameMethodsNamedHashcodeEqualOrToString.java b/src/main/java/org/openrewrite/staticanalysis/RenameMethodsNamedHashcodeEqualOrToString.java index 466e0edbd..29e786867 100644 --- a/src/main/java/org/openrewrite/staticanalysis/RenameMethodsNamedHashcodeEqualOrToString.java +++ b/src/main/java/org/openrewrite/staticanalysis/RenameMethodsNamedHashcodeEqualOrToString.java @@ -24,13 +24,16 @@ import org.openrewrite.java.JavaIsoVisitor; import org.openrewrite.java.MethodMatcher; import org.openrewrite.java.search.DeclaresMethod; +import org.openrewrite.java.tree.Flag; import org.openrewrite.java.tree.J; +import org.openrewrite.java.tree.JavaSourceFile; import org.openrewrite.java.tree.JavaType; import org.openrewrite.java.tree.TypeUtils; import org.openrewrite.staticanalysis.java.JavaFileChecker; import java.time.Duration; import java.util.Set; +import java.util.concurrent.atomic.AtomicBoolean; import static java.util.Collections.singleton; @@ -63,17 +66,51 @@ public J.MethodDeclaration visitMethodDeclaration(J.MethodDeclaration method, Ex String sn = method.getSimpleName(); JavaType rte = method.getReturnTypeExpression().getType(); JavaType.Method t = method.getMethodType(); - if (equalsIgnoreCaseExclusive(sn, "hashCode") && JavaType.Primitive.Int == rte && NO_ARGS.matches(t)) { + if (equalsIgnoreCaseExclusive(sn, "hashCode") && JavaType.Primitive.Int == rte && NO_ARGS.matches(t) && canRenameTo(t, "hashCode", NO_ARGS)) { doAfterVisit(new ChangeMethodName(MethodMatcher.methodPattern(method), "hashCode", true, false).getVisitor()); - } else if ("equal".equalsIgnoreCase(sn) && JavaType.Primitive.Boolean == rte && OBJECT_ARG.matches(t)) { + } else if ("equal".equalsIgnoreCase(sn) && JavaType.Primitive.Boolean == rte && OBJECT_ARG.matches(t) && canRenameTo(t, "equals", OBJECT_ARG)) { doAfterVisit(new ChangeMethodName(MethodMatcher.methodPattern(method), "equals", true, false).getVisitor()); - } else if (equalsIgnoreCaseExclusive(sn, "toString") && TypeUtils.isString(rte) && NO_ARGS.matches(t)) { + } else if (equalsIgnoreCaseExclusive(sn, "toString") && TypeUtils.isString(rte) && NO_ARGS.matches(t) && canRenameTo(t, "toString", NO_ARGS)) { doAfterVisit(new ChangeMethodName(MethodMatcher.methodPattern(method), "toString", true, false).getVisitor()); } } return super.visitMethodDeclaration(method, ctx); } + /** + * Method names are case sensitive, so a type may legally declare both the near-miss and the correctly + * named method; renaming would emit a duplicate. The rename reaches overrides too, so subtypes in the + * same file must be free of the target as well, and renaming onto an inherited `final` method or onto + * one of `Object`'s public instance methods would emit an illegal override. + *

+ * Declarations come from the LST rather than the type model, which for records and Lombok `@Data` + * classes also carries generated members an explicit declaration would replace rather than collide with. + */ + private boolean canRenameTo(JavaType.Method methodType, String targetName, MethodMatcher signature) { + Set flags = methodType.getFlags(); + if (!flags.contains(Flag.Public) || flags.contains(Flag.Static)) { + return false; + } + JavaType.FullyQualified declaringType = methodType.getDeclaringType(); + AtomicBoolean targetAlreadyDeclared = new AtomicBoolean(); + new JavaIsoVisitor() { + @Override + public J.MethodDeclaration visitMethodDeclaration(J.MethodDeclaration existing, AtomicBoolean declared) { + JavaType.Method existingType = existing.getMethodType(); + if (existingType != null && targetName.equals(existing.getSimpleName()) && + signature.matches(existingType) && + TypeUtils.isAssignableTo(declaringType, existingType.getDeclaringType())) { + declared.set(true); + } + return super.visitMethodDeclaration(existing, declared); + } + }.visit(getCursor().firstEnclosingOrThrow(JavaSourceFile.class), targetAlreadyDeclared); + return !targetAlreadyDeclared.get() && + !TypeUtils.findDeclaredMethod(declaringType.getSupertype(), targetName, methodType.getParameterTypes()) + .filter(m -> m.getFlags().contains(Flag.Final)) + .isPresent(); + } + private boolean equalsIgnoreCaseExclusive(String inputToCheck, String targetToCheck) { return inputToCheck.equalsIgnoreCase(targetToCheck) && !inputToCheck.equals(targetToCheck); } diff --git a/src/test/java/org/openrewrite/staticanalysis/RenameMethodsNamedHashcodeEqualOrToStringTest.java b/src/test/java/org/openrewrite/staticanalysis/RenameMethodsNamedHashcodeEqualOrToStringTest.java index 34393129e..00a9af565 100644 --- a/src/test/java/org/openrewrite/staticanalysis/RenameMethodsNamedHashcodeEqualOrToStringTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/RenameMethodsNamedHashcodeEqualOrToStringTest.java @@ -21,8 +21,9 @@ import org.openrewrite.test.RewriteTest; import static org.openrewrite.java.Assertions.java; +import static org.openrewrite.java.Assertions.version; -@SuppressWarnings({"MethodMayBeStatic", "MisspelledEquals", "BooleanMethodNameMustStartWithQuestion"}) +@SuppressWarnings({"MethodMayBeStatic", "MisspelledEquals", "BooleanMethodNameMustStartWithQuestion", "unused"}) class RenameMethodsNamedHashcodeEqualOrToStringTest implements RewriteTest { @Override @@ -113,4 +114,310 @@ public int hashcode(int a, int b) { ) ); } + + @Test + void doNotRenameWhenTargetIsAlreadyDeclared() { + rewriteRun( + //language=java + java( + """ + class Test { + public int hashcode() { + return 1; + } + + public int hashCode() { + return 2; + } + + public boolean equal(Object value) { + return false; + } + + public boolean equals(Object value) { + return true; + } + + public String tostring() { + return "near"; + } + + public String toString() { + return "proper"; + } + } + """ + ) + ); + } + + @Test + void doNotRenameWhenInterfaceAlreadyDeclaresTarget() { + rewriteRun( + //language=java + java( + """ + interface ITest { + int hashcode(); + + int hashCode(); + + boolean equal(Object obj); + + boolean equals(Object obj); + + String tostring(); + + String toString(); + } + """ + ) + ); + } + + @Test + void doNotRenameWhenEnumAlreadyDeclaresTarget() { + rewriteRun( + //language=java + java( + """ + enum Test { + A; + + public String tostring() { + return "near"; + } + + @Override + public String toString() { + return "proper"; + } + } + """ + ) + ); + } + + @Test + void doNotRenameWhenRecordAlreadyDeclaresTarget() { + rewriteRun( + version( + //language=java + java( + """ + record Test(int value) { + public int hashcode() { + return 1; + } + + @Override + public int hashCode() { + return 2; + } + } + """ + ), 17) + ); + } + + @Test + void renameWhenRecordDoesNotExplicitlyDeclareTarget() { + rewriteRun( + version( + //language=java + java( + """ + record Test(int value) { + public int hashcode() { + return 1; + } + } + """, + """ + record Test(int value) { + public int hashCode() { + return 1; + } + } + """ + ), 17) + ); + } + + @Test + void doNotRenameWhenInheritedTargetIsFinal() { + rewriteRun( + //language=java + java( + """ + class Base { + @Override + public final int hashCode() { + return 1; + } + } + + class Test extends Base { + public int hashcode() { + return 2; + } + } + """ + ) + ); + } + + @Test + void doNotRenameWhenSubclassAlreadyDeclaresTarget() { + rewriteRun( + //language=java + java( + """ + class Base { + public int hashcode() { + return 1; + } + } + + class Sub extends Base { + @Override + public int hashcode() { + return 2; + } + + @Override + public int hashCode() { + return 3; + } + } + """ + ) + ); + } + + @Test + void doNotRenameNonPublicMethod() { + rewriteRun( + //language=java + java( + """ + class Test { + int hashcode() { + return 1; + } + + private String tostring() { + return ""; + } + + protected boolean equal(Object value) { + return false; + } + } + """ + ) + ); + } + + @Test + void doNotRenameStaticMethod() { + rewriteRun( + //language=java + java( + """ + class Test { + public static int hashcode() { + return 1; + } + } + """ + ) + ); + } + + @Test + void renameWhenExistingMethodIsAnOverloadWithDifferentParameters() { + rewriteRun( + //language=java + java( + """ + class Test { + public boolean equal(Object obj) { + return false; + } + + public boolean equals(String other) { + return true; + } + } + """, + """ + class Test { + public boolean equals(Object obj) { + return false; + } + + public boolean equals(String other) { + return true; + } + } + """ + ) + ); + } + + @Test + void renameUpdatesCallSitesWhenThereIsNoCollision() { + rewriteRun( + //language=java + java( + """ + import java.util.function.Supplier; + + class Test { + public String tostring() { + return ""; + } + + String local() { + return tostring(); + } + + Supplier reference() { + return this::tostring; + } + } + + class Caller { + String call(Test test) { + return test.tostring(); + } + } + """, + """ + import java.util.function.Supplier; + + class Test { + public String toString() { + return ""; + } + + String local() { + return toString(); + } + + Supplier reference() { + return this::toString; + } + } + + class Caller { + String call(Test test) { + return test.toString(); + } + } + """ + ) + ); + } }