From 04ae3581da34b37120f247d002f05072b099491e Mon Sep 17 00:00:00 2001 From: martinfrancois Date: Mon, 10 Aug 2026 16:02:37 +0200 Subject: [PATCH 1/4] RenameUnderscoreIdentifier: choose an unused name and rename references The recipe hard coded `__` as the replacement and never checked whether that name was already declared, so a parameter, local variable, field, method or nested class sitting next to an existing `__` was collapsed onto one spelling and the output no longer compiled. The class path renamed only the class declaration, leaving the field, return, parameter and `new` expression types spelled `_`, which Java 9 and later reject, and it renamed the explicit constructor through the method path, which overwrote the `` method type name. The replacement now grows by another underscore until the name is free: a variable or class rename skips every name used in its source file, and a method rename skips the method names of the type declaring the root of the override chain, so overriding declarations and callers in other compilation units settle on the same name. A renamed class now also renames the identifiers bound to it and its explicit constructors, whose method type keeps its `` identity. Because a class rename can also rename `_.java`, the recipe becomes a ScanningRecipe that collects every source path and skips candidates whose `.java` file is already occupied next to it, so the rename never overwrites a sibling source file. Two limits a reviewer should weigh: type references are updated only inside the compilation unit that declares the type, and a collision with a declaration the recipe cannot see, such as one in a subclass in another file, remains possible. Both are stated in the recipe description, and recipes.csv is updated to match. --- .../lang/RenameUnderscoreIdentifier.java | 257 ++++- .../resources/META-INF/rewrite/recipes.csv | 2 +- .../lang/RenameUnderscoreIdentifierTest.java | 905 ++++++++++++++++++ 3 files changed, 1137 insertions(+), 27 deletions(-) diff --git a/src/main/java/org/openrewrite/java/migrate/lang/RenameUnderscoreIdentifier.java b/src/main/java/org/openrewrite/java/migrate/lang/RenameUnderscoreIdentifier.java index be45a4b38e..3826274376 100644 --- a/src/main/java/org/openrewrite/java/migrate/lang/RenameUnderscoreIdentifier.java +++ b/src/main/java/org/openrewrite/java/migrate/lang/RenameUnderscoreIdentifier.java @@ -17,37 +17,77 @@ import lombok.EqualsAndHashCode; import lombok.Value; +import org.jspecify.annotations.Nullable; +import org.openrewrite.Cursor; import org.openrewrite.ExecutionContext; import org.openrewrite.Preconditions; -import org.openrewrite.Recipe; +import org.openrewrite.ScanningRecipe; +import org.openrewrite.SourceFile; +import org.openrewrite.Tree; import org.openrewrite.TreeVisitor; import org.openrewrite.java.JavaIsoVisitor; import org.openrewrite.java.RenameVariable; import org.openrewrite.java.search.UsesJavaVersion; 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.groovy.GroovyFileChecker; import org.openrewrite.staticanalysis.kotlin.KotlinFileChecker; +import java.nio.file.Path; +import java.util.HashSet; +import java.util.Optional; +import java.util.Set; + +import static java.util.Collections.emptySet; + @EqualsAndHashCode(callSuper = false) @Value -public class RenameUnderscoreIdentifier extends Recipe { +public class RenameUnderscoreIdentifier extends ScanningRecipe> { String displayName = "Rename `_` identifier to `__`"; String description = "Renames single-underscore identifiers to double-underscore " + "in Java source files with source compatibility of Java 8 or below. " + - "In Java 9+, `_` is a reserved keyword and causes a compile error."; + "In Java 9+, `_` is a reserved keyword and causes a compile error. " + + "Further underscores are appended when `__` is already taken, so that " + + "the rename does not collide with any declaration the recipe can see: " + + "a variable or class rename avoids every name in its source file, and " + + "a method rename avoids the method names of the type hierarchy that " + + "declares the overridden method. When a class rename also renames the " + + "source file, names whose `.java` file already exists next to it are " + + "skipped as well, so the rename never overwrites another source file. " + + "A collision with a declaration the recipe cannot see, such as one " + + "in a subclass from another source file, is still possible."; + + @Override + public Set getInitialValue(ExecutionContext ctx) { + return new HashSet<>(); + } + + @Override + public TreeVisitor getScanner(Set acc) { + return new TreeVisitor() { + @Override + public Tree visit(@Nullable Tree tree, ExecutionContext ctx) { + if (tree instanceof SourceFile) { + acc.add(((SourceFile) tree).getSourcePath()); + } + return tree; + } + }; + } @Override - public TreeVisitor getVisitor() { + public TreeVisitor getVisitor(Set acc) { return Preconditions.check( Preconditions.and( new UsesJavaVersion<>(1, 8), Preconditions.not(new KotlinFileChecker<>()), Preconditions.not(new GroovyFileChecker<>()) ), - new RenameIdentifierVisitor("_", "__") + new RenameIdentifierVisitor("_", "__", acc) ); } @@ -55,15 +95,31 @@ public TreeVisitor getVisitor() { @EqualsAndHashCode(callSuper = false) static class RenameIdentifierVisitor extends JavaIsoVisitor { + private static final String NAMES_IN_USE = "namesInUse"; + String oldName; String newName; + /// The paths of every source file in the run, so that renaming a class, which may also + /// rename its file, never picks a name whose file another source file already occupies. + Set sourcePaths; + + RenameIdentifierVisitor(String oldName, String newName) { + this(oldName, newName, emptySet()); + } + + RenameIdentifierVisitor(String oldName, String newName, Set sourcePaths) { + this.oldName = oldName; + this.newName = newName; + this.sourcePaths = sourcePaths; + } + @Override public J.VariableDeclarations visitVariableDeclarations( J.VariableDeclarations multiVariable, ExecutionContext ctx) { for (J.VariableDeclarations.NamedVariable v : multiVariable.getVariables()) { if (oldName.equals(v.getSimpleName())) { - doAfterVisit(new RenameVariable<>(v, newName)); + doAfterVisit(new RenameVariable<>(v, availableName())); } } return super.visitVariableDeclarations(multiVariable, ctx); @@ -73,12 +129,12 @@ public J.VariableDeclarations visitVariableDeclarations( public J.MethodDeclaration visitMethodDeclaration( J.MethodDeclaration method, ExecutionContext ctx) { method = super.visitMethodDeclaration(method, ctx); - if (oldName.equals(method.getSimpleName())) { - JavaType.Method type = method.getMethodType(); - if (type != null) { - type = type.withName(newName); - } - method = method.withName(method.getName().withSimpleName(newName) + // A constructor carries the name of the class it belongs to, so it is renamed along with + // the class declaration, which keeps its `` method type intact. + JavaType.Method type = method.getMethodType(); + if (oldName.equals(method.getSimpleName()) && !method.isConstructor() && type != null) { + type = type.withName(availableName(type)); + method = method.withName(method.getName().withSimpleName(type.getName()) .withType(type)) .withMethodType(type); } @@ -89,12 +145,10 @@ public J.MethodDeclaration visitMethodDeclaration( public J.MethodInvocation visitMethodInvocation( J.MethodInvocation method, ExecutionContext ctx) { method = super.visitMethodInvocation(method, ctx); - if (oldName.equals(method.getSimpleName())) { - JavaType.Method type = method.getMethodType(); - if (type != null) { - type = type.withName(newName); - } - method = method.withName(method.getName().withSimpleName(newName) + JavaType.Method type = method.getMethodType(); + if (oldName.equals(method.getSimpleName()) && type != null) { + type = type.withName(availableName(type)); + method = method.withName(method.getName().withSimpleName(type.getName()) .withType(type)) .withMethodType(type); } @@ -105,12 +159,11 @@ public J.MethodInvocation visitMethodInvocation( public J.MemberReference visitMemberReference( J.MemberReference memberRef, ExecutionContext ctx) { memberRef = super.visitMemberReference(memberRef, ctx); - if (oldName.equals(memberRef.getReference().getSimpleName())) { - JavaType.Method type = memberRef.getMethodType(); - if (type != null) { - type = type.withName(newName); - } - memberRef = memberRef.withReference(memberRef.getReference().withSimpleName(newName)) + JavaType.Method type = memberRef.getMethodType(); + if (oldName.equals(memberRef.getReference().getSimpleName()) && type != null) { + type = type.withName(availableName(type)); + memberRef = memberRef.withReference(memberRef.getReference() + .withSimpleName(type.getName())) .withMethodType(type); } return memberRef; @@ -120,10 +173,162 @@ public J.MemberReference visitMemberReference( public J.ClassDeclaration visitClassDeclaration( J.ClassDeclaration classDecl, ExecutionContext ctx) { classDecl = super.visitClassDeclaration(classDecl, ctx); - if (oldName.equals(classDecl.getSimpleName())) { - classDecl = classDecl.withName(classDecl.getName().withSimpleName(newName)); + JavaType.FullyQualified type = classDecl.getType(); + if (oldName.equals(classDecl.getSimpleName()) && type != null) { + String availableName = availableName(type); + classDecl = classDecl.withName(classDecl.getName().withSimpleName(availableName)); + doAfterVisit(new RenameTypeVisitor(type.getFullyQualifiedName(), oldName, availableName)); } return classDecl; } + + /// The first name of the form `__`, `___`, ... that is not already taken anywhere in the + /// source file, nor inherited by any of the types it declares. Picking a name that is free + /// in the whole file is stricter than picking one that is free in the declaration's own + /// scope, but it is deterministic and never merges two declarations into one. Used for + /// variable declaration renames; references to a renamed type from other compilation + /// units are deliberately not followed. + private String availableName() { + // The message has to hang off the source file rather than off the root cursor, which is + // shared by every source file in the run. + Cursor sourceFile = getCursor().dropParentUntil(JavaSourceFile.class::isInstance); + return firstAvailableName(sourceFile.computeMessageIfAbsent(NAMES_IN_USE, + k -> namesInUse(sourceFile.getValue()))); + } + + /// Like `availableName()`, but for a class declaration, whose rename may also rename the + /// source file: when it does, every candidate whose target file path is already occupied + /// by another source file is skipped as well, so the rename never moves this compilation + /// unit onto an existing one, which would silently overwrite it on write. + private String availableName(JavaType.FullyQualified type) { + Cursor cursor = getCursor().dropParentUntil(JavaSourceFile.class::isInstance); + JavaSourceFile sourceFile = cursor.getValue(); + Set namesInUse = cursor.computeMessageIfAbsent(NAMES_IN_USE, + k -> namesInUse(sourceFile)); + Path sourcePath = sourceFile.getSourcePath(); + // Mirrors the file rename condition in RenameTypeVisitor#visitCompilationUnit. + boolean renamesFile = type.getFullyQualifiedName().indexOf('$') < 0 && + (oldName + ".java").equals(sourcePath.getFileName().toString()); + StringBuilder availableName = new StringBuilder(newName); + while (namesInUse.contains(availableName.toString()) || + (renamesFile && sourcePaths.contains(sourcePath.resolveSibling(availableName + ".java")))) { + availableName.append('_'); + } + return availableName.toString(); + } + + /// The first name of the form `__`, `___`, ... that no method of the type declaring the + /// root of the override chain, or of any of its supertypes, already uses. The name is + /// derived from that root rather than from the file being visited, so that a method, every + /// declaration that overrides or implements it, and every compilation unit calling it all + /// arrive at the same name and the override links survive the rename. Only method names + /// are considered: fields occupy a separate namespace and cannot collide with a method. + private String availableName(JavaType.Method methodType) { + JavaType.Method root = methodType; + Set visited = new HashSet<>(); + while (visited.add(root.getDeclaringType().getFullyQualifiedName())) { + Optional overridden = TypeUtils.findOverriddenMethod(root); + if (!overridden.isPresent()) { + break; + } + root = overridden.get(); + } + Set namesInUse = new HashSet<>(); + addInheritedNames(root.getDeclaringType(), namesInUse, new HashSet<>(), false); + return firstAvailableName(namesInUse); + } + + private String firstAvailableName(Set namesInUse) { + StringBuilder availableName = new StringBuilder(newName); + while (namesInUse.contains(availableName.toString())) { + availableName.append('_'); + } + return availableName.toString(); + } + + private static Set namesInUse(JavaSourceFile cu) { + Set namesInUse = new HashSet<>(); + new JavaIsoVisitor>() { + @Override + public J.Identifier visitIdentifier(J.Identifier identifier, Set names) { + names.add(identifier.getSimpleName()); + return super.visitIdentifier(identifier, names); + } + + @Override + public J.ClassDeclaration visitClassDeclaration(J.ClassDeclaration classDecl, Set names) { + addInheritedNames(classDecl.getType(), names, new HashSet<>(), true); + return super.visitClassDeclaration(classDecl, names); + } + }.visit(cu, namesInUse); + return namesInUse; + } + + private static void addInheritedNames(JavaType.@Nullable FullyQualified type, + Set names, Set seen, boolean includeFields) { + if (type == null || !seen.add(type.getFullyQualifiedName())) { + return; + } + if (includeFields) { + for (JavaType.Variable member : type.getMembers()) { + names.add(member.getName()); + } + } + for (JavaType.Method method : type.getMethods()) { + names.add(method.getName()); + } + addInheritedNames(type.getSupertype(), names, seen, includeFields); + for (JavaType.FullyQualified anInterface : type.getInterfaces()) { + addInheritedNames(anInterface, names, seen, includeFields); + } + } + } + + /// Renames the references bound to a type that was just renamed: field, return, parameter and + /// local variable types, casts, `instanceof`, class literals and `new` expressions, plus the + /// explicit constructors declared on it, all within the compilation unit that declares the + /// type; references from other compilation units are not updated. Only identifiers that both + /// spell the old name and resolve to that very type are touched, so unrelated same-named + /// identifiers are left alone. + @Value + @EqualsAndHashCode(callSuper = false) + static class RenameTypeVisitor extends JavaIsoVisitor { + + String fullyQualifiedName; + String oldName; + String newName; + + @Override + public J.CompilationUnit visitCompilationUnit(J.CompilationUnit cu, ExecutionContext ctx) { + J.CompilationUnit c = super.visitCompilationUnit(cu, ctx); + // A public top level type must live in a file named after it, so when the file is + // named after the type, rename the file too. + Path sourcePath = c.getSourcePath(); + if (fullyQualifiedName.indexOf('$') < 0 && + (oldName + ".java").equals(sourcePath.getFileName().toString())) { + return c.withSourcePath(sourcePath.resolveSibling(newName + ".java")); + } + return c; + } + + @Override + public J.MethodDeclaration visitMethodDeclaration(J.MethodDeclaration method, ExecutionContext ctx) { + J.MethodDeclaration m = super.visitMethodDeclaration(method, ctx); + if (m.isConstructor() && oldName.equals(m.getSimpleName()) && m.getMethodType() != null && + TypeUtils.isOfClassType(m.getMethodType().getDeclaringType(), fullyQualifiedName)) { + // Only the printed name changes; the method type keeps its `` identity. + return m.withName(m.getName().withSimpleName(newName)); + } + return m; + } + + @Override + public J.Identifier visitIdentifier(J.Identifier identifier, ExecutionContext ctx) { + J.Identifier i = super.visitIdentifier(identifier, ctx); + if (oldName.equals(i.getSimpleName()) && TypeUtils.isOfClassType(i.getType(), fullyQualifiedName)) { + return i.withSimpleName(newName); + } + return i; + } } } diff --git a/src/main/resources/META-INF/rewrite/recipes.csv b/src/main/resources/META-INF/rewrite/recipes.csv index e51cb061b2..4fcd620581 100644 --- a/src/main/resources/META-INF/rewrite/recipes.csv +++ b/src/main/resources/META-INF/rewrite/recipes.csv @@ -380,7 +380,7 @@ maven,org.openrewrite.recipe:rewrite-migrate-java,org.openrewrite.java.migrate.l maven,org.openrewrite.recipe:rewrite-migrate-java,org.openrewrite.java.migrate.lang.MigrateSecurityManagerMulticast,Use `SecurityManager#checkMulticast(InetAddress)`,"Use `SecurityManager#checkMulticast(InetAddress)` instead of the deprecated `SecurityManager#checkMulticast(InetAddress, byte)` in Java 1.4 or higher.",1,,`java.lang` APIs,Modernize,Java,,,Modernize your code to best use the project's current JDK version. Take advantage of newly available APIs and reduce the dependency of your code on third party dependencies where there is equivalent functionality in the Java standard library.,Basic building blocks for transforming Java code.,, maven,org.openrewrite.recipe:rewrite-migrate-java,org.openrewrite.java.migrate.lang.NullCheckAsSwitchCase,Add null check to existing switch cases,"In later Java 21+, null checks are valid in switch cases. This recipe will only add null checks to existing switch cases if there are no other statements in between them or if the block in the if statement is not impacting the flow of the switch.",1,,`java.lang` APIs,Modernize,Java,,,Modernize your code to best use the project's current JDK version. Take advantage of newly available APIs and reduce the dependency of your code on third party dependencies where there is equivalent functionality in the Java standard library.,Basic building blocks for transforming Java code.,, maven,org.openrewrite.recipe:rewrite-migrate-java,org.openrewrite.java.migrate.lang.RefineSwitchCases,Use switch cases refinement when possible,Use guarded switch case labels and guards if all the statements in the switch block do if/else if/else on the guarded label.,1,,`java.lang` APIs,Modernize,Java,,,Modernize your code to best use the project's current JDK version. Take advantage of newly available APIs and reduce the dependency of your code on third party dependencies where there is equivalent functionality in the Java standard library.,Basic building blocks for transforming Java code.,, -maven,org.openrewrite.recipe:rewrite-migrate-java,org.openrewrite.java.migrate.lang.RenameUnderscoreIdentifier,Rename `_` identifier to `__`,"Renames single-underscore identifiers to double-underscore in Java source files with source compatibility of Java 8 or below. In Java 9+, `_` is a reserved keyword and causes a compile error.",1,,`java.lang` APIs,Modernize,Java,,,Modernize your code to best use the project's current JDK version. Take advantage of newly available APIs and reduce the dependency of your code on third party dependencies where there is equivalent functionality in the Java standard library.,Basic building blocks for transforming Java code.,, +maven,org.openrewrite.recipe:rewrite-migrate-java,org.openrewrite.java.migrate.lang.RenameUnderscoreIdentifier,Rename `_` identifier to `__`,"Renames single-underscore identifiers to double-underscore in Java source files with source compatibility of Java 8 or below. In Java 9+, `_` is a reserved keyword and causes a compile error. Further underscores are appended when `__` is already taken, so that the rename does not collide with any declaration the recipe can see: a variable or class rename avoids every name in its source file, and a method rename avoids the method names of the type hierarchy that declares the overridden method. When a class rename also renames the source file, names whose `.java` file already exists next to it are skipped as well, so the rename never overwrites another source file. A collision with a declaration the recipe cannot see, such as one in a subclass from another source file, is still possible.",1,,`java.lang` APIs,Modernize,Java,,,Modernize your code to best use the project's current JDK version. Take advantage of newly available APIs and reduce the dependency of your code on third party dependencies where there is equivalent functionality in the Java standard library.,Basic building blocks for transforming Java code.,, maven,org.openrewrite.recipe:rewrite-migrate-java,org.openrewrite.java.migrate.lang.ReplaceUnusedVariablesWithUnderscore,Replace unused variables with underscore,"Replace unused variable declarations with underscore (_) for Java 22+. This includes unused variables in enhanced for loops, catch blocks, and lambda parameters where the variable is never referenced.",1,,`java.lang` APIs,Modernize,Java,,,Modernize your code to best use the project's current JDK version. Take advantage of newly available APIs and reduce the dependency of your code on third party dependencies where there is equivalent functionality in the Java standard library.,Basic building blocks for transforming Java code.,, maven,org.openrewrite.recipe:rewrite-migrate-java,org.openrewrite.java.migrate.lang.StringFormatted,Prefer `String.formatted(Object...)`,"Prefer `String.formatted(Object...)` over `String.format(String, Object...)` in Java 17 or higher.",1,,`java.lang` APIs,Modernize,Java,,,Modernize your code to best use the project's current JDK version. Take advantage of newly available APIs and reduce the dependency of your code on third party dependencies where there is equivalent functionality in the Java standard library.,Basic building blocks for transforming Java code.,"[{""name"":""addParentheses"",""type"":""Boolean"",""displayName"":""Add parentheses around the first argument"",""description"":""Add parentheses around the first argument if it is not a simple expression. Default true; if false no change will be made. ""}]", maven,org.openrewrite.recipe:rewrite-migrate-java,org.openrewrite.java.migrate.lang.StringRulesRecipes$IndexOfCharRecipe,"Replace `String.indexOf(char, 0)` with `String.indexOf(char)`","Replace `String.indexOf(char ch, int fromIndex)` with `String.indexOf(char)`.",1,,`java.lang` APIs,Modernize,Java,,,Modernize your code to best use the project's current JDK version. Take advantage of newly available APIs and reduce the dependency of your code on third party dependencies where there is equivalent functionality in the Java standard library.,Basic building blocks for transforming Java code.,, diff --git a/src/test/java/org/openrewrite/java/migrate/lang/RenameUnderscoreIdentifierTest.java b/src/test/java/org/openrewrite/java/migrate/lang/RenameUnderscoreIdentifierTest.java index d29ad4f73b..415080f224 100644 --- a/src/test/java/org/openrewrite/java/migrate/lang/RenameUnderscoreIdentifierTest.java +++ b/src/test/java/org/openrewrite/java/migrate/lang/RenameUnderscoreIdentifierTest.java @@ -20,9 +20,13 @@ import org.openrewrite.ExecutionContext; import org.openrewrite.Recipe; import org.openrewrite.TreeVisitor; +import org.openrewrite.java.ChangeType; import org.openrewrite.test.RecipeSpec; import org.openrewrite.test.RewriteTest; +import java.nio.file.Paths; + +import static org.assertj.core.api.Assertions.assertThat; import static org.openrewrite.java.Assertions.java; import static org.openrewrite.java.Assertions.javaVersion; import static org.openrewrite.kotlin.Assertions.kotlin; @@ -51,6 +55,38 @@ public TreeVisitor getVisitor() { }; } + /// Setup recipe that renames the *type* `UNDERSCORE` to `_`, declaration, constructor and every + /// bound reference alike, so that the resulting LST is shaped like Java 8 source declaring a + /// type named `_`. `renameToUnderscore()` only rewrites declaration names, which is not enough + /// to exercise the type reference paths. + private static Recipe renameTypeToUnderscore() { + return new ChangeType("UNDERSCORE", "_", false); + } + + /// Runs the rename in a single pass over a clean parse, `UNDERSCORE` standing in for `_`. + /// The two-pass setup above rewrites each declaration's method type but not the method list of + /// the declaring `JavaType.Class`, so on the second pass `TypeUtils.findOverriddenMethod` can + /// no longer link an override to the method it overrides. Real Java 8 source arrives as a + /// single clean parse, which this setup preserves, so the override-sensitive tests use it. + private static Recipe renameUnderscoreWordIdentifier() { + return new Recipe() { + @Override + public String getDisplayName() { + return "Rename UNDERSCORE to __"; + } + + @Override + public String getDescription() { + return "Test setup recipe."; + } + + @Override + public TreeVisitor getVisitor() { + return new RenameUnderscoreIdentifier.RenameIdentifierVisitor("UNDERSCORE", "__"); + } + }; + } + @Override public void defaults(RecipeSpec spec) { spec.recipes(renameToUnderscore(), new RenameUnderscoreIdentifier()) @@ -293,6 +329,875 @@ fun test() { ); } + @Test + void parameterCollision() { + rewriteRun( + //language=java + java( + """ + class Test { + int sum(int UNDERSCORE, int __) { + return UNDERSCORE + __; + } + } + """, + """ + class Test { + int sum(int ___, int __) { + return ___ + __; + } + } + """ + ) + ); + } + + @Test + void localVariableCollision() { + rewriteRun( + //language=java + java( + """ + class Test { + int sum() { + int UNDERSCORE = 1; + int __ = 2; + return UNDERSCORE + __; + } + } + """, + """ + class Test { + int sum() { + int ___ = 1; + int __ = 2; + return ___ + __; + } + } + """ + ) + ); + } + + @Test + void fieldCollision() { + rewriteRun( + //language=java + java( + """ + class Test { + int UNDERSCORE = 1; + int __ = 2; + + int sum() { + return UNDERSCORE + __; + } + } + """, + """ + class Test { + int ___ = 1; + int __ = 2; + + int sum() { + return ___ + __; + } + } + """ + ) + ); + } + + @Test + void methodCollision() { + rewriteRun( + //language=java + java( + """ + class Test { + int UNDERSCORE() { + return 1; + } + + int __() { + return 2; + } + + int sum() { + return UNDERSCORE() + __(); + } + } + """, + """ + class Test { + int ___() { + return 1; + } + + int __() { + return 2; + } + + int sum() { + return ___() + __(); + } + } + """ + ) + ); + } + + @Test + void nestedClassCollision() { + rewriteRun( + //language=java + java( + """ + class Test { + class UNDERSCORE { + } + + class __ { + } + } + """, + """ + class Test { + class ___ { + } + + class __ { + } + } + """ + ) + ); + } + + @Test + void classDeclarationRenamesBoundTypeAndNewClassReferences() { + rewriteRun( + spec -> spec.recipes(renameTypeToUnderscore(), new RenameUnderscoreIdentifier()) + .allSources(s -> s.markers(javaVersion(8))), + //language=java + java( + """ + class UNDERSCORE { + UNDERSCORE() { + } + + UNDERSCORE field; + + UNDERSCORE copy(UNDERSCORE input) { + return new UNDERSCORE(); + } + } + """, + """ + class __ { + __() { + } + + __ field; + + __ copy(__ input) { + return new __(); + } + } + """ + ) + ); + } + + @Test + void classFileRenamedAlongsideTheClass() { + rewriteRun( + spec -> spec.recipes(renameTypeToUnderscore(), new RenameUnderscoreIdentifier()) + .allSources(s -> s.markers(javaVersion(8))), + //language=java + java( + """ + class UNDERSCORE { + } + """, + """ + class __ { + } + """, + spec -> spec.path("_.java") + .afterRecipe(cu -> assertThat(cu.getSourcePath()).isEqualTo(Paths.get("__.java"))) + ) + ); + } + + @Test + void classFileRenameDoesNotOverwriteAnUnrelatedSourceFile() { + rewriteRun( + spec -> spec.recipes(renameTypeToUnderscore(), new RenameUnderscoreIdentifier()) + .allSources(s -> s.markers(javaVersion(8))), + //language=java + java( + """ + class UNDERSCORE { + } + """, + """ + class ___ { + } + """, + spec -> spec.path("_.java") + .afterRecipe(cu -> assertThat(cu.getSourcePath()).isEqualTo(Paths.get("___.java"))) + ), + //language=java + java( + """ + class Helper { + int keepMe = 1; + } + """, + spec -> spec.path("__.java") + ), + //language=java + java( + """ + class User { + int use() { + return new Helper().keepMe; + } + } + """ + ) + ); + } + + @Test + void classFileRenameDoesNotOverwriteACollidingClassFile() { + rewriteRun( + spec -> spec.recipes(renameTypeToUnderscore(), new RenameUnderscoreIdentifier()) + .allSources(s -> s.markers(javaVersion(8))), + //language=java + java( + """ + class UNDERSCORE { + } + """, + """ + class ___ { + } + """, + spec -> spec.path("_.java") + .afterRecipe(cu -> assertThat(cu.getSourcePath()).isEqualTo(Paths.get("___.java"))) + ), + //language=java + java( + """ + class __ { + int keepMe = 1; + } + """, + spec -> spec.path("__.java") + ) + ); + } + + @Test + void classFileRenameAdvancesPastEveryOccupiedFile() { + rewriteRun( + spec -> spec.recipes(renameTypeToUnderscore(), new RenameUnderscoreIdentifier()) + .allSources(s -> s.markers(javaVersion(8))), + //language=java + java( + """ + class UNDERSCORE { + } + """, + """ + class ____ { + } + """, + spec -> spec.path("_.java") + .afterRecipe(cu -> assertThat(cu.getSourcePath()).isEqualTo(Paths.get("____.java"))) + ), + //language=java + java( + """ + class __ { + int keepMe = 1; + } + """, + spec -> spec.path("__.java") + ), + //language=java + java( + """ + class ___ { + int keepMeToo = 1; + } + """, + spec -> spec.path("___.java") + ) + ); + } + + @Test + void occupiedFileInAnotherPackageDoesNotAffectTheChosenName() { + rewriteRun( + spec -> spec.recipes(new ChangeType("a.UNDERSCORE", "a._", false), new RenameUnderscoreIdentifier()) + .allSources(s -> s.markers(javaVersion(8))), + //language=java + java( + """ + package a; + + class UNDERSCORE { + } + """, + """ + package a; + + class __ { + } + """, + spec -> spec.path("a/_.java") + .afterRecipe(cu -> assertThat(cu.getSourcePath()).isEqualTo(Paths.get("a/__.java"))) + ), + //language=java + java( + """ + package b; + + class __ { + int keepMe = 1; + } + """, + spec -> spec.path("b/__.java") + ) + ); + } + + @Test + void nestedTypeRenamedInEveryReferencePosition() { + rewriteRun( + //language=java + java( + """ + class Test { + static class UNDERSCORE { + } + + Object use() { + UNDERSCORE value = new UNDERSCORE(); + Class literal = UNDERSCORE.class; + Object o = value; + if (o instanceof UNDERSCORE) { + return (UNDERSCORE) o; + } + return literal; + } + } + """, + """ + class Test { + static class __ { + } + + Object use() { + __ value = new __(); + Class literal = __.class; + Object o = value; + if (o instanceof __) { + return (__) o; + } + return literal; + } + } + """ + ) + ); + } + + @Test + void selectionAdvancesPastEveryTakenUnderscoreName() { + rewriteRun( + //language=java + java( + """ + class Test { + int sum(int UNDERSCORE, int __, int ___) { + return UNDERSCORE + __ + ___; + } + } + """, + """ + class Test { + int sum(int ____, int __, int ___) { + return ____ + __ + ___; + } + } + """ + ) + ); + } + + @Test + void existingDoubleUnderscoreDeclarationIsNeverRenamed() { + rewriteRun( + //language=java + java( + """ + class Test { + int a() { + int UNDERSCORE = 1; + return UNDERSCORE; + } + + int b() { + int __ = 2; + return __; + } + } + """, + """ + class Test { + int a() { + int ___ = 1; + return ___; + } + + int b() { + int __ = 2; + return __; + } + } + """ + ) + ); + } + + @Test + void overloadsAndMethodReferencesRenameTogether() { + rewriteRun( + //language=java + java( + """ + import java.util.function.IntSupplier; + + class Test { + int UNDERSCORE() { + return 1; + } + + int UNDERSCORE(int i) { + return i; + } + + int __() { + return 2; + } + + IntSupplier supplier() { + return this::UNDERSCORE; + } + + int sum() { + return UNDERSCORE() + UNDERSCORE(1) + __(); + } + } + """, + """ + import java.util.function.IntSupplier; + + class Test { + int ___() { + return 1; + } + + int ___(int i) { + return i; + } + + int __() { + return 2; + } + + IntSupplier supplier() { + return this::___; + } + + int sum() { + return ___() + ___(1) + __(); + } + } + """ + ) + ); + } + + @Test + void localVariableShadowingFieldKeepsBindings() { + rewriteRun( + //language=java + java( + """ + class Test { + int UNDERSCORE = 1; + + int test() { + int UNDERSCORE = 2; + return UNDERSCORE + this.UNDERSCORE; + } + } + """, + """ + class Test { + int __ = 1; + + int test() { + int __ = 2; + return __ + this.__; + } + } + """ + ) + ); + } + + @Test + void inheritedMemberNameIsNotShadowed() { + rewriteRun( + //language=java + java( + """ + class Base { + int __ = 1; + } + """ + ), + //language=java + java( + """ + class Test extends Base { + int UNDERSCORE = 2; + + int sum() { + return UNDERSCORE; + } + } + """, + """ + class Test extends Base { + int ___ = 2; + + int sum() { + return ___; + } + } + """ + ) + ); + } + + @Test + void namesInUseAreComputedPerSourceFile() { + rewriteRun( + //language=java + java( + """ + class A { + int a() { + int UNDERSCORE = 1; + return UNDERSCORE; + } + } + """, + """ + class A { + int a() { + int __ = 1; + return __; + } + } + """ + ), + //language=java + java( + """ + class B { + int b() { + int UNDERSCORE = 1; + int __ = 2; + return UNDERSCORE + __; + } + } + """, + """ + class B { + int b() { + int ___ = 1; + int __ = 2; + return ___ + __; + } + } + """ + ) + ); + } + + @Test + void callersInAnotherSourceFileFollowTheDeclaration() { + rewriteRun( + //language=java + java( + """ + class Lib { + int UNDERSCORE() { + return 1; + } + + int __() { + return 2; + } + } + """, + """ + class Lib { + int ___() { + return 1; + } + + int __() { + return 2; + } + } + """ + ), + //language=java + java( + """ + class Caller { + int call(Lib lib) { + return lib.UNDERSCORE() + lib.__(); + } + } + """, + """ + class Caller { + int call(Lib lib) { + return lib.___() + lib.__(); + } + } + """ + ) + ); + } + + @Test + void overriddenMethodAndOverrideAgreeOnOneName() { + rewriteRun( + spec -> spec.recipes(renameUnderscoreWordIdentifier()), + //language=java + java( + """ + class A { + void UNDERSCORE() { + } + } + + class B extends A { + @Override + void UNDERSCORE() { + } + + void __(int i) { + } + } + """, + """ + class A { + void __() { + } + } + + class B extends A { + @Override + void __() { + } + + void __(int i) { + } + } + """ + ) + ); + } + + @Test + void interfaceMethodAndImplementationAgreeOnOneName() { + rewriteRun( + spec -> spec.recipes(renameUnderscoreWordIdentifier()), + //language=java + java( + """ + interface I { + void UNDERSCORE(); + } + """, + """ + interface I { + void __(); + } + """ + ), + //language=java + java( + """ + class Impl implements I { + @Override + public void UNDERSCORE() { + } + + void __(int i) { + } + } + """, + """ + class Impl implements I { + @Override + public void __() { + } + + void __(int i) { + } + } + """ + ) + ); + } + + @Test + void fieldNamedDoubleUnderscoreDoesNotBlockAMethodRename() { + rewriteRun( + //language=java + java( + """ + class A { + void UNDERSCORE() { + } + } + + class B extends A { + @Override + void UNDERSCORE() { + } + + int __ = 1; + } + """, + """ + class A { + void __() { + } + } + + class B extends A { + @Override + void __() { + } + + int __ = 1; + } + """ + ) + ); + } + + @Test + void overrideKeepsDynamicDispatchAcrossSourceFiles() { + rewriteRun( + spec -> spec.recipes(renameUnderscoreWordIdentifier()), + //language=java + java( + """ + class A { + int UNDERSCORE() { + return 1; + } + } + """, + """ + class A { + int __() { + return 1; + } + } + """ + ), + //language=java + java( + """ + class B extends A { + @Override + int UNDERSCORE() { + return 2; + } + + int __(int i) { + return i; + } + } + """, + """ + class B extends A { + @Override + int __() { + return 2; + } + + int __(int i) { + return i; + } + } + """ + ), + //language=java + java( + """ + class Main { + int run() { + A a = new B(); + return a.UNDERSCORE(); + } + } + """, + """ + class Main { + int run() { + A a = new B(); + return a.__(); + } + } + """ + ) + ); + } + + @Test + void alreadyRenamedSourceIsLeftAlone() { + rewriteRun( + spec -> spec.recipe(new RenameUnderscoreIdentifier()), + //language=java + java( + """ + class Test { + int sum(int ___, int __) { + return ___ + __; + } + } + """ + ) + ); + } + @Test void forEachLoopVariable() { rewriteRun( From cf5bd7f004760b4636e50ed5a915381e47b7feac Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Wed, 12 Aug 2026 00:29:56 +0200 Subject: [PATCH 2/4] Trim commentary --- .../lang/RenameUnderscoreIdentifier.java | 49 +++++++------------ 1 file changed, 17 insertions(+), 32 deletions(-) diff --git a/src/main/java/org/openrewrite/java/migrate/lang/RenameUnderscoreIdentifier.java b/src/main/java/org/openrewrite/java/migrate/lang/RenameUnderscoreIdentifier.java index 3826274376..047f783a64 100644 --- a/src/main/java/org/openrewrite/java/migrate/lang/RenameUnderscoreIdentifier.java +++ b/src/main/java/org/openrewrite/java/migrate/lang/RenameUnderscoreIdentifier.java @@ -100,8 +100,7 @@ static class RenameIdentifierVisitor extends JavaIsoVisitor { String oldName; String newName; - /// The paths of every source file in the run, so that renaming a class, which may also - /// rename its file, never picks a name whose file another source file already occupies. + /// The paths of every source file, so a class rename never picks a name whose file is taken. Set sourcePaths; RenameIdentifierVisitor(String oldName, String newName) { @@ -129,8 +128,7 @@ public J.VariableDeclarations visitVariableDeclarations( public J.MethodDeclaration visitMethodDeclaration( J.MethodDeclaration method, ExecutionContext ctx) { method = super.visitMethodDeclaration(method, ctx); - // A constructor carries the name of the class it belongs to, so it is renamed along with - // the class declaration, which keeps its `` method type intact. + // A constructor carries its class's name, so it is renamed with the class declaration JavaType.Method type = method.getMethodType(); if (oldName.equals(method.getSimpleName()) && !method.isConstructor() && type != null) { type = type.withName(availableName(type)); @@ -182,31 +180,25 @@ public J.ClassDeclaration visitClassDeclaration( return classDecl; } - /// The first name of the form `__`, `___`, ... that is not already taken anywhere in the - /// source file, nor inherited by any of the types it declares. Picking a name that is free - /// in the whole file is stricter than picking one that is free in the declaration's own - /// scope, but it is deterministic and never merges two declarations into one. Used for - /// variable declaration renames; references to a renamed type from other compilation - /// units are deliberately not followed. + /// The first name of the form `__`, `___`, ... free anywhere in the file and uninherited by any type it + /// declares. Stricter than scoping the search to the declaration, but deterministic and it never merges + /// two declarations into one. private String availableName() { - // The message has to hang off the source file rather than off the root cursor, which is - // shared by every source file in the run. + // The root cursor is shared by every source file, so the message hangs off this one Cursor sourceFile = getCursor().dropParentUntil(JavaSourceFile.class::isInstance); return firstAvailableName(sourceFile.computeMessageIfAbsent(NAMES_IN_USE, k -> namesInUse(sourceFile.getValue()))); } - /// Like `availableName()`, but for a class declaration, whose rename may also rename the - /// source file: when it does, every candidate whose target file path is already occupied - /// by another source file is skipped as well, so the rename never moves this compilation - /// unit onto an existing one, which would silently overwrite it on write. + /// Like `availableName()`, but for a class whose rename may also rename the file, so candidates whose + /// target path another source file occupies are skipped rather than silently overwriting it. private String availableName(JavaType.FullyQualified type) { Cursor cursor = getCursor().dropParentUntil(JavaSourceFile.class::isInstance); JavaSourceFile sourceFile = cursor.getValue(); Set namesInUse = cursor.computeMessageIfAbsent(NAMES_IN_USE, k -> namesInUse(sourceFile)); Path sourcePath = sourceFile.getSourcePath(); - // Mirrors the file rename condition in RenameTypeVisitor#visitCompilationUnit. + // Mirrors `RenameTypeVisitor#visitCompilationUnit` boolean renamesFile = type.getFullyQualifiedName().indexOf('$') < 0 && (oldName + ".java").equals(sourcePath.getFileName().toString()); StringBuilder availableName = new StringBuilder(newName); @@ -217,12 +209,9 @@ private String availableName(JavaType.FullyQualified type) { return availableName.toString(); } - /// The first name of the form `__`, `___`, ... that no method of the type declaring the - /// root of the override chain, or of any of its supertypes, already uses. The name is - /// derived from that root rather than from the file being visited, so that a method, every - /// declaration that overrides or implements it, and every compilation unit calling it all - /// arrive at the same name and the override links survive the rename. Only method names - /// are considered: fields occupy a separate namespace and cannot collide with a method. + /// The first name of the form `__`, `___`, ... unused by the type rooting the override chain or its + /// supertypes. Deriving it from that root rather than from the file being visited makes every override + /// and caller arrive at the same name, so the override links survive. Fields are a separate namespace. private String availableName(JavaType.Method methodType) { JavaType.Method root = methodType; Set visited = new HashSet<>(); @@ -284,12 +273,9 @@ private static void addInheritedNames(JavaType.@Nullable FullyQualified type, } } - /// Renames the references bound to a type that was just renamed: field, return, parameter and - /// local variable types, casts, `instanceof`, class literals and `new` expressions, plus the - /// explicit constructors declared on it, all within the compilation unit that declares the - /// type; references from other compilation units are not updated. Only identifiers that both - /// spell the old name and resolve to that very type are touched, so unrelated same-named - /// identifiers are left alone. + /// Renames the references bound to a just-renamed type — declared types, casts, `instanceof`, class + /// literals, `new` and its explicit constructors — within the declaring compilation unit only. An identifier + /// must both spell the old name and resolve to that type, so unrelated same-named ones are left alone. @Value @EqualsAndHashCode(callSuper = false) static class RenameTypeVisitor extends JavaIsoVisitor { @@ -301,8 +287,7 @@ static class RenameTypeVisitor extends JavaIsoVisitor { @Override public J.CompilationUnit visitCompilationUnit(J.CompilationUnit cu, ExecutionContext ctx) { J.CompilationUnit c = super.visitCompilationUnit(cu, ctx); - // A public top level type must live in a file named after it, so when the file is - // named after the type, rename the file too. + // A public top level type must live in a file named after it, so rename the file too Path sourcePath = c.getSourcePath(); if (fullyQualifiedName.indexOf('$') < 0 && (oldName + ".java").equals(sourcePath.getFileName().toString())) { @@ -316,7 +301,7 @@ public J.MethodDeclaration visitMethodDeclaration(J.MethodDeclaration method, Ex J.MethodDeclaration m = super.visitMethodDeclaration(method, ctx); if (m.isConstructor() && oldName.equals(m.getSimpleName()) && m.getMethodType() != null && TypeUtils.isOfClassType(m.getMethodType().getDeclaringType(), fullyQualifiedName)) { - // Only the printed name changes; the method type keeps its `` identity. + // Only the printed name changes; the method type keeps its `` identity return m.withName(m.getName().withSimpleName(newName)); } return m; From 8d8cf642e35189e43b413f00e1e9431fddc5b913 Mon Sep 17 00:00:00 2001 From: martinfrancois Date: Sun, 16 Aug 2026 22:17:15 +0200 Subject: [PATCH 3/4] Use descriptive names in RenameUnderscoreIdentifier --- .../lang/RenameUnderscoreIdentifier.java | 28 ++++++++++--------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/src/main/java/org/openrewrite/java/migrate/lang/RenameUnderscoreIdentifier.java b/src/main/java/org/openrewrite/java/migrate/lang/RenameUnderscoreIdentifier.java index 047f783a64..118938d0df 100644 --- a/src/main/java/org/openrewrite/java/migrate/lang/RenameUnderscoreIdentifier.java +++ b/src/main/java/org/openrewrite/java/migrate/lang/RenameUnderscoreIdentifier.java @@ -286,34 +286,36 @@ static class RenameTypeVisitor extends JavaIsoVisitor { @Override public J.CompilationUnit visitCompilationUnit(J.CompilationUnit cu, ExecutionContext ctx) { - J.CompilationUnit c = super.visitCompilationUnit(cu, ctx); + J.CompilationUnit compilationUnit = super.visitCompilationUnit(cu, ctx); // A public top level type must live in a file named after it, so rename the file too - Path sourcePath = c.getSourcePath(); + Path sourcePath = compilationUnit.getSourcePath(); if (fullyQualifiedName.indexOf('$') < 0 && (oldName + ".java").equals(sourcePath.getFileName().toString())) { - return c.withSourcePath(sourcePath.resolveSibling(newName + ".java")); + return compilationUnit.withSourcePath(sourcePath.resolveSibling(newName + ".java")); } - return c; + return compilationUnit; } @Override public J.MethodDeclaration visitMethodDeclaration(J.MethodDeclaration method, ExecutionContext ctx) { - J.MethodDeclaration m = super.visitMethodDeclaration(method, ctx); - if (m.isConstructor() && oldName.equals(m.getSimpleName()) && m.getMethodType() != null && - TypeUtils.isOfClassType(m.getMethodType().getDeclaringType(), fullyQualifiedName)) { + J.MethodDeclaration visitedMethod = super.visitMethodDeclaration(method, ctx); + if (visitedMethod.isConstructor() && oldName.equals(visitedMethod.getSimpleName()) && + visitedMethod.getMethodType() != null && + TypeUtils.isOfClassType(visitedMethod.getMethodType().getDeclaringType(), fullyQualifiedName)) { // Only the printed name changes; the method type keeps its `` identity - return m.withName(m.getName().withSimpleName(newName)); + return visitedMethod.withName(visitedMethod.getName().withSimpleName(newName)); } - return m; + return visitedMethod; } @Override public J.Identifier visitIdentifier(J.Identifier identifier, ExecutionContext ctx) { - J.Identifier i = super.visitIdentifier(identifier, ctx); - if (oldName.equals(i.getSimpleName()) && TypeUtils.isOfClassType(i.getType(), fullyQualifiedName)) { - return i.withSimpleName(newName); + J.Identifier visitedIdentifier = super.visitIdentifier(identifier, ctx); + if (oldName.equals(visitedIdentifier.getSimpleName()) && + TypeUtils.isOfClassType(visitedIdentifier.getType(), fullyQualifiedName)) { + return visitedIdentifier.withSimpleName(newName); } - return i; + return visitedIdentifier; } } } From 78d357f9013460cd71216832b202d6f8fa63c3a6 Mon Sep 17 00:00:00 2001 From: martinfrancois Date: Sun, 16 Aug 2026 23:06:51 +0200 Subject: [PATCH 4/4] Extract shared renameAlsoRenamesFile helper for the duplicated file-rename condition --- .../migrate/lang/RenameUnderscoreIdentifier.java | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/main/java/org/openrewrite/java/migrate/lang/RenameUnderscoreIdentifier.java b/src/main/java/org/openrewrite/java/migrate/lang/RenameUnderscoreIdentifier.java index 118938d0df..b611d27d13 100644 --- a/src/main/java/org/openrewrite/java/migrate/lang/RenameUnderscoreIdentifier.java +++ b/src/main/java/org/openrewrite/java/migrate/lang/RenameUnderscoreIdentifier.java @@ -91,6 +91,12 @@ public TreeVisitor getVisitor(Set acc) { ); } + /// `$` separates nested type names, so a `$`-free fully qualified name is a top-level type whose file may need renaming + static boolean renameAlsoRenamesFile(String fullyQualifiedName, String oldName, Path sourcePath) { + return fullyQualifiedName.indexOf('$') < 0 && + (oldName + ".java").equals(sourcePath.getFileName().toString()); + } + @Value @EqualsAndHashCode(callSuper = false) static class RenameIdentifierVisitor extends JavaIsoVisitor { @@ -198,9 +204,7 @@ private String availableName(JavaType.FullyQualified type) { Set namesInUse = cursor.computeMessageIfAbsent(NAMES_IN_USE, k -> namesInUse(sourceFile)); Path sourcePath = sourceFile.getSourcePath(); - // Mirrors `RenameTypeVisitor#visitCompilationUnit` - boolean renamesFile = type.getFullyQualifiedName().indexOf('$') < 0 && - (oldName + ".java").equals(sourcePath.getFileName().toString()); + boolean renamesFile = renameAlsoRenamesFile(type.getFullyQualifiedName(), oldName, sourcePath); StringBuilder availableName = new StringBuilder(newName); while (namesInUse.contains(availableName.toString()) || (renamesFile && sourcePaths.contains(sourcePath.resolveSibling(availableName + ".java")))) { @@ -289,8 +293,7 @@ public J.CompilationUnit visitCompilationUnit(J.CompilationUnit cu, ExecutionCon J.CompilationUnit compilationUnit = super.visitCompilationUnit(cu, ctx); // A public top level type must live in a file named after it, so rename the file too Path sourcePath = compilationUnit.getSourcePath(); - if (fullyQualifiedName.indexOf('$') < 0 && - (oldName + ".java").equals(sourcePath.getFileName().toString())) { + if (renameAlsoRenamesFile(fullyQualifiedName, oldName, sourcePath)) { return compilationUnit.withSourcePath(sourcePath.resolveSibling(newName + ".java")); } return compilationUnit;