Skip to content

Fix ChangeType inner class import handling when outer class import is present #7032

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
MBoegers wants to merge 3 commits into
base: main
Choose a base branch
from
+164 −11
Open

Fix ChangeType inner class import handling when outer class import is present #7032

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5910a8f
Fix ChangeType inner class import handling for outer class imports
MBoegers Mar 18, 2026
f0b4f3d
Refactor ChangeType inner class test cases for clarity
MBoegers Mar 18, 2026
5c96c5c
Merge remote-tracking branch 'upstream/main' into fix/change-type-inn…
MBoegers Apr 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
132 changes: 131 additions & 1 deletion rewrite-java-test/src/test/java/org/openrewrite/java/ChangeTypeTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2505,7 +2505,8 @@ A test() {
}
}
""",
"""
"""
import bar.A.Builder;
import foo.A;

class Test {
Expand All @@ -2519,6 +2520,135 @@ A test() {
);
}

@Test
void renameInnerClassWithinSameOuterClass() {
rewriteRun(
spec -> spec.recipe(new ChangeType("foo.A$Builder", "foo.A$Creator", null))
Comment on lines +2524 to +2526
Copy link
Copy Markdown
Member

@timtebeek timtebeek Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now we're testing the fairly common case of A.B, but should we also test the variants for A.B.C ? I wonder if those are handled correctly for all the permutations of imported as A, A.B or A.B.C, and then changed as A, A$B or A$B$C.

.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 changeTypeOfInnerCompletely() {
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;
Comment on lines +2584 to +2585
Copy link
Copy Markdown
Member

@timtebeek timtebeek Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we have both imports here in the test? I'd think we only need the first one right?


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 changeTypeOfInnerClassImplicitly() {
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 {}
}
"""
Comment on lines +2618 to +2624
Copy link
Copy Markdown
Member

@timtebeek timtebeek Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't need this second depends on block right? As ChangeType synthesizes types for what it changes, and we only need to provide the correct types to compile the imports?

)
),
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 inheritedTypesUpdated() {
rewriteRun(
Expand Down
43 changes: 33 additions & 10 deletions rewrite-java/src/main/java/org/openrewrite/java/ChangeType.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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()) {
Expand All @@ -242,7 +243,10 @@ private void addImport(JavaType.FullyQualified owningClass) {
if (originalType.getFullyQualifiedName().equals(type.getFullyQualifiedName())) {
sf = (JavaSourceFile) new RemoveImport<ExecutionContext>(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<ExecutionContext>(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);
}
}
}
Expand All @@ -253,13 +257,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.
Copy link
Copy Markdown
Member

@timtebeek timtebeek Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In our comments we first use B, then switch to Y here; that seems needlessly confusing, or was that intentional?

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);
}
}
}
Expand All @@ -278,6 +290,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())) {
Expand Down Expand Up @@ -400,7 +422,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;
}
Expand Down