From a593ef62d84d7aeac96b5f3fe13694cae6740229 Mon Sep 17 00:00:00 2001 From: Laurens Westerlaken Date: Fri, 30 Aug 2024 19:27:46 +0200 Subject: [PATCH] `ChangeType` should fully qualify type usage in the case of conflicting imports (#4458) * Add initial testcase that shows the problem * Check whether an import becomes ambiguous and if it does start fully qualifying references * Reduce warnings shown in IDE --------- Co-authored-by: Laurens Westerlaken Co-authored-by: Tim te Beek --- .../org/openrewrite/java/ChangeTypeTest.java | 320 ++++++++++-------- .../java/org/openrewrite/java/ChangeType.java | 51 ++- 2 files changed, 212 insertions(+), 159 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 5d878133206..f7deeb632b6 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 @@ -101,7 +101,7 @@ void starImport() { java( """ import java.util.logging.*; - + class Test { static void method() { LoggingMXBean loggingBean = null; @@ -111,7 +111,7 @@ static void method() { """ import java.lang.management.PlatformLoggingMXBean; import java.util.logging.*; - + class Test { static void method() { PlatformLoggingMXBean loggingBean = null; @@ -130,7 +130,7 @@ void allowJavaLangSubpackages() { java( """ import java.util.logging.LoggingMXBean; - + class Test { static void method() { LoggingMXBean loggingBean = null; @@ -139,7 +139,7 @@ static void method() { """, """ import java.lang.management.PlatformLoggingMXBean; - + class Test { static void method() { PlatformLoggingMXBean loggingBean = null; @@ -159,7 +159,7 @@ void unnecessaryImport() { java( """ import test.Outer; - + class Test { private Outer p = Outer.of(); private Outer p2 = test.Outer.of(); @@ -169,12 +169,12 @@ class Test { java( """ package test; - + public class Outer { public static Outer of() { return new Outer(); } - + public static class Inner { } } @@ -193,7 +193,7 @@ void changeInnerClassToOuterClass() { """ import java.util.Map; import java.util.Map.Entry; - + class Test { Entry p; Map.Entry p2; @@ -202,7 +202,7 @@ class Test { """, """ import java.util.List; - + class Test { List p; List p2; @@ -221,14 +221,14 @@ void changeStaticFieldAccess() { java( """ import java.io.File; - + class Test { String p = File.separator; } """, """ import my.pkg.List; - + class Test { String p = List.separator; } @@ -253,14 +253,14 @@ void replaceWithNestedType() { java( """ import java.io.File; - + class Test { File p; } """, """ import java.util.Map; - + class Test { Map.Entry p; } @@ -277,14 +277,14 @@ void replacePrivateNestedType() { java( """ package a; - + class A { private static class B1 {} } """, """ package a; - + class A { private static class B2 {} } @@ -301,7 +301,7 @@ void deeplyNestedInnerClass() { java( """ package a; - + class A { public static class B { public static class C { @@ -311,7 +311,7 @@ public static class C { """, """ package a; - + class A { public static class B { public static class C2 { @@ -329,12 +329,12 @@ void simpleName() { java( """ import a.A1; - + public class B extends A1 {} """, """ import a.A2; - + public class B extends A2 {} """ ), @@ -375,7 +375,7 @@ void array2() { java( """ package com.acme.product; - + public class Pojo { } """ @@ -383,12 +383,12 @@ public class Pojo { java( """ package com.acme.project.impl; - + import com.acme.product.Pojo; - + public class UsePojo2 { Pojo[] p; - + void run() { p[0] = null; } @@ -396,12 +396,12 @@ void run() { """, """ package com.acme.project.impl; - + import com.acme.product.v2.Pojo; - + public class UsePojo2 { Pojo[] p; - + void run() { p[0] = null; } @@ -420,14 +420,14 @@ void array() { java( """ import a.A1; - + public class B { A1[] a = new A1[0]; } """, """ import a.A2; - + public class B { A2[] a = new A2[0]; } @@ -444,14 +444,14 @@ void multiDimensionalArray() { java( """ import a.A1; - + public class A { A1[][] multiDimensionalArray; } """, """ import a.A2; - + public class A { A2[][] multiDimensionalArray; } @@ -478,12 +478,12 @@ void classDecl() { java( """ import a.A1; - + public class B extends A1 implements I1 {} """, """ import a.A2; - + public class B extends A2 implements I2 {} """ ) @@ -499,14 +499,14 @@ void method() { java( """ import a.A1; - + public class B { public A1 foo() throws A1 { return null; } } """, """ import a.A2; - + public class B { public A2 foo() throws A2 { return null; } } @@ -523,10 +523,10 @@ void methodInvocationTypeParametersAndWildcard() { java( """ import a.A1; - + public class B { public T generic(T n, java.util.List in) { - + } public void test() { A1.stat(); @@ -536,10 +536,10 @@ public void test() { """, """ import a.A2; - + public class B { public T generic(T n, java.util.List in) { - + } public void test() { A2.stat(); @@ -560,7 +560,7 @@ void multiCatch() { java( """ import a.A1; - + public class B { public void test() { try {} @@ -570,7 +570,7 @@ public void test() { """, """ import a.A2; - + public class B { public void test() { try {} @@ -590,14 +590,14 @@ void multiVariable() { java( """ import a.A1; - + public class B { A1 f1, f2; } """, """ import a.A2; - + public class B { A2 f1, f2; } @@ -614,14 +614,14 @@ void newClass() { java( """ import a.A1; - + public class B { A1 a = new A1(); } """, """ import a.A2; - + public class B { A2 a = new A2(); } @@ -641,7 +641,7 @@ void updateAssignments() { java( """ import a.A1; - + class B { void method(A1 param) { A1 a = param; @@ -650,7 +650,7 @@ void method(A1 param) { """, """ import a.A2; - + class B { void method(A2 param) { A2 a = param; @@ -669,18 +669,18 @@ void parameterizedType() { java( """ import a.A1; - + import java.util.Map; - + public class B { Map m; } """, """ import a.A2; - + import java.util.Map; - + public class B { Map m; } @@ -698,14 +698,14 @@ void typeCast() { java( """ import a.A1; - + public class B { A1 a = (A1) null; } """, """ import a.A2; - + public class B { A2 a = (A2) null; } @@ -722,14 +722,14 @@ void classReference() { java( """ import a.A1; - + public class A { Class clazz = A1.class; } """, """ import a.A2; - + public class A { Class clazz = A2.class; } @@ -746,7 +746,7 @@ void methodSelect() { java( """ import a.A1; - + public class B { A1 a = null; public void test() { a.foo(); } @@ -754,7 +754,7 @@ public class B { """, """ import a.A2; - + public class B { A2 a = null; public void test() { a.foo(); } @@ -773,7 +773,7 @@ void staticImport() { java( """ import static a.A1.stat; - + public class B { public void test() { stat(); @@ -782,7 +782,7 @@ public void test() { """, """ import static a.A2.stat; - + public class B { public void test() { stat(); @@ -799,7 +799,7 @@ void staticImports2() { java( """ package com.acme.product; - + public class RunnableFactory { public static String getString() { return "hello"; @@ -810,9 +810,9 @@ public static String getString() { java( """ package com.acme.project.impl; - + import static com.acme.product.RunnableFactory.getString; - + public class StaticImportWorker { public void work() { getString().toLowerCase(); @@ -821,9 +821,9 @@ public void work() { """, """ package com.acme.project.impl; - + import static com.acme.product.v2.RunnableFactory.getString; - + public class StaticImportWorker { public void work() { getString().toLowerCase(); @@ -841,7 +841,7 @@ void staticConstant() { java( """ package com.acme.product; - + public class RunnableFactory { public static final String CONSTANT = "hello"; } @@ -850,9 +850,9 @@ public class RunnableFactory { java( """ package com.acme.project.impl; - + import static com.acme.product.RunnableFactory.CONSTANT; - + public class StaticImportWorker { public void work() { System.out.println(CONSTANT + " fred."); @@ -861,9 +861,9 @@ public void work() { """, """ package com.acme.project.impl; - + import static com.acme.product.v2.RunnableFactory.CONSTANT; - + public class StaticImportWorker { public void work() { System.out.println(CONSTANT + " fred."); @@ -945,23 +945,23 @@ public class B {} java( """ package com.myorg; - + import java.util.ArrayList; import com.yourorg.a.A; import java.util.List; - + public class Foo { List a = new ArrayList<>(); } """, """ package com.myorg; - + import com.myorg.b.B; - + import java.util.ArrayList; import java.util.List; - + public class Foo { List a = new ArrayList<>(); } @@ -977,10 +977,10 @@ void changeTypeWithInnerClass() { java( """ package com.acme.product; - + public class OuterClass { public static class InnerClass { - + } } """ @@ -988,15 +988,15 @@ public static class InnerClass { java( """ package de; - + import com.acme.product.OuterClass; import com.acme.product.OuterClass.InnerClass; - + public class UseInnerClass { public String work() { return new InnerClass().toString(); } - + public String work2() { return new OuterClass().toString(); } @@ -1004,15 +1004,15 @@ public String work2() { """, """ package de; - + import com.acme.product.v2.OuterClass; import com.acme.product.v2.OuterClass.InnerClass; - + public class UseInnerClass { public String work() { return new InnerClass().toString(); } - + public String work2() { return new OuterClass().toString(); } @@ -1030,7 +1030,7 @@ void uppercaseInPackage() { java( """ package com.acme.product.util.accessDecision; - + public enum AccessVote { ABSTAIN } @@ -1039,9 +1039,9 @@ public enum AccessVote { java( """ package de; - + import com.acme.product.util.accessDecision.AccessVote; - + public class ProjectVoter { public AccessVote vote() { return AccessVote.ABSTAIN; @@ -1050,9 +1050,9 @@ public AccessVote vote() { """, """ package de; - + import com.acme.product.v2.util.accessDecision.AccessVote; - + public class ProjectVoter { public AccessVote vote() { return AccessVote.ABSTAIN; @@ -1079,7 +1079,7 @@ public interface Procedure { java( """ import com.acme.product.Procedure; - + public abstract class Worker { void callWorker() { worker(() -> { @@ -1090,7 +1090,7 @@ void callWorker() { """, """ import com.acme.product.Procedure2; - + public abstract class Worker { void callWorker() { worker(() -> { @@ -1111,7 +1111,7 @@ void assignment() { java( """ package com.acme.product.util.accessDecision; - + public enum AccessVote { ABSTAIN, GRANT @@ -1121,9 +1121,9 @@ public enum AccessVote { java( """ package de; - + import com.acme.product.util.accessDecision.AccessVote; - + public class ProjectVoter { public AccessVote vote(Object input) { AccessVote fred; @@ -1134,9 +1134,9 @@ public AccessVote vote(Object input) { """, """ package de; - + import com.acme.product.v2.util.accessDecision.AccessVote; - + public class ProjectVoter { public AccessVote vote(Object input) { AccessVote fred; @@ -1157,7 +1157,7 @@ void ternary() { java( """ package com.acme.product.util.accessDecision; - + public enum AccessVote { ABSTAIN, GRANT @@ -1167,9 +1167,9 @@ public enum AccessVote { java( """ package de; - + import com.acme.product.util.accessDecision.AccessVote; - + public class ProjectVoter { public AccessVote vote(Object input) { return input == null ? AccessVote.GRANT : AccessVote.ABSTAIN; @@ -1178,9 +1178,9 @@ public AccessVote vote(Object input) { """, """ package de; - + import com.acme.product.v2.util.accessDecision.AccessVote; - + public class ProjectVoter { public AccessVote vote(Object input) { return input == null ? AccessVote.GRANT : AccessVote.ABSTAIN; @@ -1230,7 +1230,7 @@ void javadocs() { java( """ import java.util.List; - + /** * {@link List} here */ @@ -1240,7 +1240,7 @@ class Test { """, """ import java.util.Collection; - + /** * {@link Collection} here */ @@ -1260,7 +1260,7 @@ void onlyUpdateApplicableImport() { java( """ package com.acme.product.factory; - + public class V1Factory { public static String getItem() { return "V1Factory"; @@ -1271,7 +1271,7 @@ public static String getItem() { java( """ package com.acme.product.factory; - + public class V2Factory { public static String getItem() { return "V2Factory"; @@ -1282,16 +1282,16 @@ public static String getItem() { java( """ import com.acme.product.factory.V1Factory; - + import static com.acme.product.factory.V2Factory.getItem; - + public class UseFactories { static class MyV1Factory extends V1Factory { static String getMyItemInherited() { return getItem(); } } - + static String getMyItemStaticImport() { return getItem(); } @@ -1299,16 +1299,16 @@ static String getMyItemStaticImport() { """, """ import com.acme.product.factory.V1FactoryA; - + import static com.acme.product.factory.V2Factory.getItem; - + public class UseFactories { static class MyV1Factory extends V1FactoryA { static String getMyItemInherited() { return getItem(); } } - + static String getMyItemStaticImport() { return getItem(); } @@ -1391,15 +1391,15 @@ void updateImportPrefixWithEmptyPackage() { java( """ package a.b; - + import java.util.List; - + class Original { } """, """ import java.util.List; - + class Target { } """ @@ -1415,7 +1415,7 @@ void updateClassPrefixWithEmptyPackage() { java( """ package a.b; - + class Original { } """, @@ -1535,9 +1535,9 @@ public class A2 { java( """ package org.foo; - + import a.A1; - + public class Example { public A1 method(A1 a1) { return a1; @@ -1546,9 +1546,9 @@ public A1 method(A1 a1) { """, """ package org.foo; - + import a.A2; - + public class Example { public A2 method(A2 a1) { return a1; @@ -1560,7 +1560,7 @@ public A2 method(A2 a1) { """ import a.A1; import org.foo.Example; - + public class Test { A1 local = new Example().method(null); } @@ -1568,7 +1568,7 @@ public class Test { """ import a.A2; import org.foo.Example; - + public class Test { A2 local = new Example().method(null); } @@ -1596,14 +1596,14 @@ void updateVariableType() { java( """ import a.A1; - + public class Test { A1 a; } """, """ import a.A2; - + public class Test { A2 a; } @@ -1626,7 +1626,7 @@ void boundedGenericType() { java( """ import a.A1; - + public class Test { T method(T t) { return t; @@ -1635,7 +1635,7 @@ T method(T t) { """, """ import a.A2; - + public class Test { T method(T t) { return t; @@ -1658,7 +1658,7 @@ void changeConstructor() { java( """ package a; - + public class A1 { public A1() { } @@ -1666,7 +1666,7 @@ public A1() { """, """ package a; - + public class A2 { public A2() { } @@ -1689,12 +1689,12 @@ void updateJavaTypeClassKindAnnotation() { java( """ package org.openrewrite; - + import java.lang.annotation.ElementType; import java.lang.annotation.Retention; import java.lang.annotation.RetentionPolicy; import java.lang.annotation.Target; - + @Target({ElementType.TYPE, ElementType.METHOD}) @Retention(RetentionPolicy.RUNTIME) public @interface Test1 {} @@ -1703,12 +1703,12 @@ void updateJavaTypeClassKindAnnotation() { java( """ package org.openrewrite; - + import java.lang.annotation.ElementType; import java.lang.annotation.Retention; import java.lang.annotation.RetentionPolicy; import java.lang.annotation.Target; - + @Target({ElementType.TYPE, ElementType.METHOD}) @Retention(RetentionPolicy.RUNTIME) public @interface Test2 {} @@ -1717,7 +1717,7 @@ void updateJavaTypeClassKindAnnotation() { java( """ import org.openrewrite.Test1; - + public class A { @Test1 void method() {} @@ -1725,7 +1725,7 @@ void method() {} """, """ import org.openrewrite.Test2; - + public class A { @Test2 void method() {} @@ -1800,14 +1800,13 @@ void doesNotModifyInnerClassesIfIgnoreDefinitionTrue() { spec -> spec.recipe(new ChangeType("Test.InnerA", "Test.InnerB", true)), java( """ - public class Test { private class InnerA { } - + private class InnerB { } - + public void test(String s) { InnerA a = new InnerA(); } @@ -1817,10 +1816,10 @@ public void test(String s) { public class Test { private class InnerA { } - + private class InnerB { } - + public void test(String s) { InnerB a = new InnerB(); } @@ -1851,9 +1850,9 @@ public class Test { java( """ package org.openrewrite; - + import org.openrewrite.Test; - + public class Sibling { public Test test() { return new Test(); @@ -1862,9 +1861,9 @@ public Test test() { """, """ package org.openrewrite; - + import org.openrewrite.subpackage.Test; - + public class Sibling { public Test test() { return new Test(); @@ -1912,9 +1911,9 @@ public ObjectMapper configure(SerializationConfig.Feature f, boolean state) { java( """ import org.codehaus.jackson.map.ObjectMapper; - + import static org.codehaus.jackson.map.SerializationConfig.Feature.WRAP_ROOT_VALUE; - + class A { void test() { ObjectMapper mapper = new ObjectMapper(); @@ -1924,9 +1923,9 @@ void test() { """, """ import com.fasterxml.jackson.databind.ObjectMapper; - + import static com.fasterxml.jackson.databind.SerializationFeature.WRAP_ROOT_VALUE; - + class A { void test() { ObjectMapper mapper = new ObjectMapper(); @@ -1937,4 +1936,39 @@ void test() { ) ); } + + @Test + @Issue("https://github.com/openrewrite/rewrite/issues/4452") + void shouldFullyQualifyWhenNewTypeIsAmbiguous() { + rewriteRun( + spec -> spec.recipe(new ChangeType( + "javax.annotation.Nonnull", + "org.checkerframework.checker.nullness.qual.NonNull", + null)), + // language=java + java( + """ + import lombok.NonNull; + import javax.annotation.Nonnull; + import org.immutables.value.Value; + + @Value.Immutable + @Value.Style(passAnnotations = Nonnull.class) + interface ConflictingImports { + void lombokMethod(@NonNull final String lombokNonNull){} + } + """, + """ + import lombok.NonNull; + import org.immutables.value.Value; + + @Value.Immutable + @Value.Style(passAnnotations = org.checkerframework.checker.nullness.qual.NonNull.class) + interface ConflictingImports { + void lombokMethod(@NonNull final String lombokNonNull){} + } + """ + ) + ); + } } 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 f6a009a5524..79c1c1277ca 100644 --- a/rewrite-java/src/main/java/org/openrewrite/java/ChangeType.java +++ b/rewrite-java/src/main/java/org/openrewrite/java/ChangeType.java @@ -205,9 +205,9 @@ private void addImport(JavaType.FullyQualified owningClass) { if (maybeType instanceof JavaType.FullyQualified) { JavaType.FullyQualified type = (JavaType.FullyQualified) maybeType; if (originalType.getFullyQualifiedName().equals(type.getFullyQualifiedName())) { - sf = (JavaSourceFile) new RemoveImport(originalType.getFullyQualifiedName()).visit(sf, ctx, getCursor().getParentOrThrow()); + sf = (JavaSourceFile) new RemoveImport(originalType.getFullyQualifiedName()).visitNonNull(sf, ctx, getCursor().getParentOrThrow()); } else if (originalType.getOwningClass() != null && originalType.getOwningClass().getFullyQualifiedName().equals(type.getFullyQualifiedName())) { - sf = (JavaSourceFile) new RemoveImport(originalType.getOwningClass().getFullyQualifiedName()).visit(sf, ctx, getCursor().getParentOrThrow()); + sf = (JavaSourceFile) new RemoveImport(originalType.getOwningClass().getFullyQualifiedName()).visitNonNull(sf, ctx, getCursor().getParentOrThrow()); } } } @@ -217,20 +217,18 @@ private void addImport(JavaType.FullyQualified owningClass) { if (fullyQualifiedTarget != null) { JavaType.FullyQualified owningClass = fullyQualifiedTarget.getOwningClass(); if (!topLevelClassnames.contains(getTopLevelClassName(fullyQualifiedTarget).getFullyQualifiedName())) { - if (owningClass != null && !"java.lang".equals(fullyQualifiedTarget.getPackageName())) { - addImport(owningClass); - } - if (!"java.lang".equals(fullyQualifiedTarget.getPackageName())) { - addImport(fullyQualifiedTarget); + if (hasNoConflictingImport(sf)) { + if (owningClass != null && !"java.lang".equals(fullyQualifiedTarget.getPackageName())) { + addImport(owningClass); + } + if (!"java.lang".equals(fullyQualifiedTarget.getPackageName())) { + addImport(fullyQualifiedTarget); + } } } } - if (sf != null) { - sf = sf.withImports(ListUtils.map(sf.getImports(), i -> visitAndCast(i, ctx, super::visitImport))); - } - - j = sf; + j = sf.withImports(ListUtils.map(sf.getImports(), i -> visitAndCast(i, ctx, super::visitImport))); } return j; @@ -301,6 +299,7 @@ public J visitIdentifier(J.Identifier ident, ExecutionContext ctx) { className = originalType.getFullyQualifiedName().substring(iType.getOwningClass().getFullyQualifiedName().length() + 1); } + JavaSourceFile sf = getCursor().firstEnclosing(JavaSourceFile.class); if (ident.getSimpleName().equals(className)) { if (targetType instanceof JavaType.FullyQualified) { if (((JavaType.FullyQualified) targetType).getOwningClass() != null) { @@ -308,7 +307,11 @@ public J visitIdentifier(J.Identifier ident, ExecutionContext ctx) { .withType(null) .withPrefix(ident.getPrefix())); } else { - ident = ident.withSimpleName(((JavaType.FullyQualified) targetType).getClassName()); + if (sf != null && hasNoConflictingImport(sf)) { + ident = ident.withSimpleName(((JavaType.FullyQualified) targetType).getClassName()); + } else { + ident = ident.withSimpleName(((JavaType.FullyQualified) targetType).getFullyQualifiedName()); + } } } else if (targetType instanceof JavaType.Primitive) { ident = ident.withSimpleName(((JavaType.Primitive) targetType).getKeyword()); @@ -316,9 +319,8 @@ public J visitIdentifier(J.Identifier ident, ExecutionContext ctx) { } // Recreate any static imports as needed - JavaSourceFile cu = getCursor().firstEnclosing(JavaSourceFile.class); - if (cu != null) { - for (J.Import anImport : cu.getImports()) { + if (sf != null) { + for (J.Import anImport : sf.getImports()) { if (anImport.isStatic() && anImport.getQualid().getTarget().getType() != null) { JavaType.FullyQualified fqn = TypeUtils.asFullyQualified(anImport.getQualid().getTarget().getType()); if (fqn != null && TypeUtils.isOfClassType(fqn, originalType.getFullyQualifiedName()) && @@ -505,6 +507,23 @@ private Expression updateOuterClassTypes(Expression typeTree) { private boolean isTargetFullyQualifiedType(JavaType.@Nullable FullyQualified fq) { return fq != null && TypeUtils.isOfClassType(fq, originalType.getFullyQualifiedName()) && targetType instanceof JavaType.FullyQualified; } + + private boolean hasNoConflictingImport(JavaSourceFile sf) { + JavaType.FullyQualified oldType = TypeUtils.asFullyQualified(originalType); + JavaType.FullyQualified newType = TypeUtils.asFullyQualified(targetType); + if (oldType == null || newType == null) { + return false; // No way to be sure + } + for (J.Import anImport : sf.getImports()) { + JavaType.FullyQualified currType = TypeUtils.asFullyQualified(anImport.getQualid().getType()); + if (currType != null && + !TypeUtils.isOfType(currType, oldType) && + currType.getClassName().equals(newType.getClassName())) { + return false; + } + } + return true; + } } private static class ChangeClassDefinition extends JavaIsoVisitor {