From 5910a8f82732c43f011f9e8966a1be34419bb01b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Merlin=20B=C3=B6gershausen?= Date: Wed, 18 Mar 2026 17:32:27 +0100 Subject: [PATCH 1/2] Fix ChangeType inner class import handling for outer class imports MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When renaming an inner class (e.g. foo.A\$Builder → bar.B\$Builder) where the source file imports the outer class (import foo.A), ChangeType now correctly: - Force-adds the new outer class import (import bar.B) when the old outer class import was removed, since the code accesses the inner class via qualified name (B.Builder) - Omits the redundant inner class import (import bar.B.Builder) in this case, since import bar.B already makes B.Builder accessible Also adds a test for renaming an inner class within the same outer class. --- .../org/openrewrite/java/ChangeTypeTest.java | 134 +++++++++++++++++- .../java/org/openrewrite/java/ChangeType.java | 43 ++++-- 2 files changed, 164 insertions(+), 13 deletions(-) diff --git a/rewrite-java-test/src/test/java/org/openrewrite/java/ChangeTypeTest.java b/rewrite-java-test/src/test/java/org/openrewrite/java/ChangeTypeTest.java index 8462e5820e7..dbf4fe366b8 100644 --- a/rewrite-java-test/src/test/java/org/openrewrite/java/ChangeTypeTest.java +++ b/rewrite-java-test/src/test/java/org/openrewrite/java/ChangeTypeTest.java @@ -2356,7 +2356,6 @@ void newBar() {} ); } - @Issue("https://github.com/openrewrite/rewrite/issues/4773") @Test void noRenameOfTypeWithMatchingPrefix() { rewriteRun( @@ -2405,7 +2404,97 @@ public boolean isDirty() { ); } - @Issue("https://github.com/openrewrite/rewrite/issues/4764") + @Test + void changeTypeOfInnerClassConstructor() { + rewriteRun( + spec -> spec.recipe(new ChangeType("foo.A$Builder", "bar.B$Builder", null)) + .parser(JavaParser.fromJavaVersion().dependsOn( + """ + package foo; + + public class A { + public static class Builder {} + } + """, + """ + package bar; + + public class B { + public static class Builder {} + } + """ + ) + ), + java( + """ + import foo.A; + import foo.A.Builder; + + class Test { + void test() { + A.Builder x = new A.Builder(); + } + } + """, + """ + import bar.B; + + class Test { + void test() { + B.Builder x = new B.Builder(); + } + } + """ + ) + ); + } + + @Test + void changeTypeOfInnerClassContractorFromOuter() { + rewriteRun( + spec -> spec.recipe(new ChangeType("foo.A", "bar.B", null)) + .parser(JavaParser.fromJavaVersion().dependsOn( + """ + package foo; + + public class A { + public static class Builder {} + } + """, + """ + package bar; + + public class B { + public static class Builder {} + } + """ + ) + ), + java( + """ + import foo.A; + import foo.A.Builder; + + class Test { + void test() { + A.Builder x = new A.Builder(); + } + } + """, + """ + import bar.B; + import bar.B.Builder; + + class Test { + void test() { + B.Builder x = new B.Builder(); + } + } + """ + ) + ); + } + @Test void changeTypeOfInnerClass() { rewriteRun( @@ -2455,7 +2544,8 @@ A test() { } } """, - """ + """ + import bar.A.Builder; import foo.A; class Test { @@ -2469,6 +2559,44 @@ A test() { ); } + @Test + void renameInnerClassWithinSameOuterClass() { + rewriteRun( + spec -> spec.recipe(new ChangeType("foo.A$Builder", "foo.A$Creator", null)) + .parser(JavaParser.fromJavaVersion().dependsOn( + """ + package foo; + + public class A { + public static class Builder {} + public static class Creator {} + } + """ + ) + ), + java( + """ + import foo.A; + + class Test { + void test() { + A.Builder x = new A.Builder(); + } + } + """, + """ + import foo.A; + + class Test { + void test() { + A.Creator x = new A.Creator(); + } + } + """ + ) + ); + } + @Test void inheritedTypesUpdated() { rewriteRun( diff --git a/rewrite-java/src/main/java/org/openrewrite/java/ChangeType.java b/rewrite-java/src/main/java/org/openrewrite/java/ChangeType.java index 4d4887e4faf..347812d8906 100644 --- a/rewrite-java/src/main/java/org/openrewrite/java/ChangeType.java +++ b/rewrite-java/src/main/java/org/openrewrite/java/ChangeType.java @@ -193,12 +193,12 @@ public J visitImport(J.Import import_, ExecutionContext ctx) { return updateType(javaType); } - private void addImport(JavaType.FullyQualified owningClass) { + private void addImport(JavaType.FullyQualified owningClass, boolean onlyIfUsed) { if (importAlias != null) { - maybeAddImport(owningClass.getPackageName(), owningClass.getClassName(), null, importAlias.getSimpleName(), true); + maybeAddImport(owningClass.getPackageName(), owningClass.getClassName(), null, importAlias.getSimpleName(), onlyIfUsed); } - maybeAddImport(owningClass.getPackageName(), owningClass.getClassName(), null, null, true); + maybeAddImport(owningClass.getPackageName(), owningClass.getClassName(), null, null, onlyIfUsed); } @Override @@ -225,6 +225,7 @@ private void addImport(JavaType.FullyQualified owningClass) { j = ((TypedTree) tree).withType(updateType(((TypedTree) tree).getType())); } else if (tree instanceof JavaSourceFile) { JavaSourceFile sf = (JavaSourceFile) tree; + boolean outerClassImportRemoved = false; if (targetType instanceof JavaType.FullyQualified) { for (J.Import anImport : sf.getImports()) { if (anImport.isStatic()) { @@ -237,7 +238,10 @@ private void addImport(JavaType.FullyQualified owningClass) { if (originalType.getFullyQualifiedName().equals(type.getFullyQualifiedName())) { sf = (JavaSourceFile) new RemoveImport(originalType.getFullyQualifiedName()).visitNonNull(sf, ctx, getCursor().getParentOrThrow()); } else if (originalType.getOwningClass() != null && originalType.getOwningClass().getFullyQualifiedName().equals(type.getFullyQualifiedName())) { + JavaSourceFile sfBefore = sf; sf = (JavaSourceFile) new RemoveImport(originalType.getOwningClass().getFullyQualifiedName()).visitNonNull(sf, ctx, getCursor().getParentOrThrow()); + // Track whether the outer class import was actually removed (not retained because still in use) + outerClassImportRemoved |= (sf != sfBefore); } } } @@ -247,13 +251,21 @@ private void addImport(JavaType.FullyQualified owningClass) { if (fullyQualifiedTarget != null) { JavaType.FullyQualified owningClass = fullyQualifiedTarget.getOwningClass(); if (!topLevelClassnames.contains(getTopLevelClassName(fullyQualifiedTarget).getFullyQualifiedName())) { - if (hasNoConflictingImport(sf)) { - if (owningClass != null && !"java.lang".equals(fullyQualifiedTarget.getPackageName())) { - addImport(owningClass); - } - if (!"java.lang".equals(fullyQualifiedTarget.getPackageName())) { - addImport(fullyQualifiedTarget); + if (!"java.lang".equals(fullyQualifiedTarget.getPackageName()) && hasNoConflictingImport(sf)) { + if (owningClass != null) { + // Force-add the outer class import when the original outer class import was + // explicitly removed (meaning code used the outer class by name, e.g. "B" in "B.Builder"). + // When the outer class import was absent, the code uses only the simple inner class + // name (e.g. just "Y"), so AddImport can decide based on actual references. + addImport(owningClass, !outerClassImportRemoved); } + // Force-add the inner class import only when the outer class was NOT removed: + // if the outer class was removed and is being re-added, `import bar.B` already + // makes `B.Builder` accessible — adding `import bar.B.Builder` would be redundant. + // When only the inner class import existed (e.g. code uses simple name "Y"), + // force-add when the owning class changed so FindTypes can locate the new type. + boolean forceAddInnerImport = !outerClassImportRemoved && !owningClassSame(owningClass); + addImport(fullyQualifiedTarget, !forceAddInnerImport); } } } @@ -272,6 +284,16 @@ private void addImport(JavaType.FullyQualified owningClass) { return j; } + private boolean owningClassSame(JavaType.@Nullable FullyQualified owningClass) { + JavaType.FullyQualified originalOwningClass = originalType.getOwningClass(); + if (originalOwningClass == null || owningClass == null) { + return true; + } + + return originalOwningClass.getClassName().equals(owningClass.getClassName()) && + originalOwningClass.getPackageName().equals(owningClass.getPackageName()); + } + @Override public J visitFieldAccess(J.FieldAccess fieldAccess, ExecutionContext ctx) { if (fieldAccess.isFullyQualifiedClassReference(originalType.getFullyQualifiedName())) { @@ -394,7 +416,8 @@ public J visitMethodInvocation(J.MethodInvocation method, ExecutionContext ctx) method.getSimpleName().equals(anImport.getQualid().getSimpleName())) { JavaType.FullyQualified targetFqn = (JavaType.FullyQualified) targetType; - addImport(targetFqn); + // onlyIfUsed=true: the static import already proves the type is referenced. + addImport(targetFqn, true); maybeAddImport((targetFqn).getFullyQualifiedName(), method.getName().getSimpleName()); break; } From f0b4f3d7dfcf61d4847cab7cc3c83a41a1344f3a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Merlin=20B=C3=B6gershausen?= Date: Wed, 18 Mar 2026 17:50:00 +0100 Subject: [PATCH 2/2] Refactor ChangeType inner class test cases for clarity MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Rename changeTypeOfInnerClassConstructor → changeTypeOfInnerClass (scenario: import foo.A.Builder only, rename to bar.A$Builder within same outer name) - Rename changeTypeOfInnerClass → changeTypeOfInnerCompletely (scenario: rename to a completely different outer class bar.B$Builder) - Rename renameInnerClassWithinSameOuterClass → changeTypeOfInnerClassImplicitly (scenario: outer class renamed, inner class implicitly follows via import) - Add @Issue annotations linking tests to the GitHub issues they cover - Adjust test content to better represent the intent of each scenario --- .../org/openrewrite/java/ChangeTypeTest.java | 126 +++++++++--------- 1 file changed, 64 insertions(+), 62 deletions(-) diff --git a/rewrite-java-test/src/test/java/org/openrewrite/java/ChangeTypeTest.java b/rewrite-java-test/src/test/java/org/openrewrite/java/ChangeTypeTest.java index dbf4fe366b8..ff1bab65f38 100644 --- a/rewrite-java-test/src/test/java/org/openrewrite/java/ChangeTypeTest.java +++ b/rewrite-java-test/src/test/java/org/openrewrite/java/ChangeTypeTest.java @@ -2356,6 +2356,7 @@ void newBar() {} ); } + @Issue("https://github.com/openrewrite/rewrite/issues/4773") @Test void noRenameOfTypeWithMatchingPrefix() { rewriteRun( @@ -2404,23 +2405,40 @@ public boolean isDirty() { ); } + @Issue("https://github.com/openrewrite/rewrite/issues/4764") @Test - void changeTypeOfInnerClassConstructor() { + void changeTypeOfInnerClass() { rewriteRun( - spec -> spec.recipe(new ChangeType("foo.A$Builder", "bar.B$Builder", null)) + spec -> spec.recipe(new ChangeType("foo.A$Builder", "bar.A$Builder", true)) .parser(JavaParser.fromJavaVersion().dependsOn( """ package foo; public class A { - public static class Builder {} + public A.Builder builder() { + return new A.Builder(); + } + + public static class Builder { + public A build() { + return new A(); + } + } } """, """ package bar; - public class B { - public static class Builder {} + public class A { + public A.Builder builder() { + return new A.Builder(); + } + + public static class Builder { + public A build() { + return new A(); + } + } } """ ) @@ -2431,17 +2449,20 @@ public static class Builder {} import foo.A.Builder; class Test { - void test() { - A.Builder x = new A.Builder(); + A test() { + A.Builder b = A.builder(); + return b.build(); } } """, - """ - import bar.B; + """ + import bar.A.Builder; + import foo.A; class Test { - void test() { - B.Builder x = new B.Builder(); + A test() { + bar.A.Builder b = A.builder(); + return b.build(); } } """ @@ -2450,22 +2471,16 @@ void test() { } @Test - void changeTypeOfInnerClassContractorFromOuter() { + void renameInnerClassWithinSameOuterClass() { rewriteRun( - spec -> spec.recipe(new ChangeType("foo.A", "bar.B", null)) + spec -> spec.recipe(new ChangeType("foo.A$Builder", "foo.A$Creator", null)) .parser(JavaParser.fromJavaVersion().dependsOn( """ package foo; public class A { public static class Builder {} - } - """, - """ - package bar; - - public class B { - public static class Builder {} + public static class Creator {} } """ ) @@ -2473,7 +2488,6 @@ public static class Builder {} java( """ import foo.A; - import foo.A.Builder; class Test { void test() { @@ -2482,12 +2496,11 @@ void test() { } """, """ - import bar.B; - import bar.B.Builder; + import foo.A; class Test { void test() { - B.Builder x = new B.Builder(); + A.Creator x = new A.Creator(); } } """ @@ -2496,38 +2509,22 @@ void test() { } @Test - void changeTypeOfInnerClass() { + void changeTypeOfInnerCompletely() { rewriteRun( - spec -> spec.recipe(new ChangeType("foo.A$Builder", "bar.A$Builder", true)) + spec -> spec.recipe(new ChangeType("foo.A$Builder", "bar.B$Builder", null)) .parser(JavaParser.fromJavaVersion().dependsOn( """ package foo; public class A { - public A.Builder builder() { - return new A.Builder(); - } - - public static class Builder { - public A build() { - return new A(); - } - } + public static class Builder {} } """, """ package bar; - public class A { - public A.Builder builder() { - return new A.Builder(); - } - - public static class Builder { - public A build() { - return new A(); - } - } + public class B { + public static class Builder {} } """ ) @@ -2538,38 +2535,41 @@ public A build() { import foo.A.Builder; class Test { - A test() { - A.Builder b = A.builder(); - return b.build(); + void test() { + A.Builder x = new A.Builder(); } } """, """ - import bar.A.Builder; - import foo.A; + import bar.B; - class Test { - A test() { - bar.A.Builder b = A.builder(); - return b.build(); - } - } - """ + class Test { + void test() { + B.Builder x = new B.Builder(); + } + } + """ ) ); } @Test - void renameInnerClassWithinSameOuterClass() { + void changeTypeOfInnerClassImplicitly() { rewriteRun( - spec -> spec.recipe(new ChangeType("foo.A$Builder", "foo.A$Creator", null)) + spec -> spec.recipe(new ChangeType("foo.A", "bar.B", null)) .parser(JavaParser.fromJavaVersion().dependsOn( """ package foo; public class A { public static class Builder {} - public static class Creator {} + } + """, + """ + package bar; + + public class B { + public static class Builder {} } """ ) @@ -2577,6 +2577,7 @@ public static class Creator {} java( """ import foo.A; + import foo.A.Builder; class Test { void test() { @@ -2585,11 +2586,12 @@ void test() { } """, """ - import foo.A; + import bar.B; + import bar.B.Builder; class Test { void test() { - A.Creator x = new A.Creator(); + B.Builder x = new B.Builder(); } } """