From ce021da8831b43ad54f630f915709387f0204848 Mon Sep 17 00:00:00 2001 From: bramli ahmed khalil Date: Sun, 15 Dec 2024 09:16:12 +0100 Subject: [PATCH 1/4] rewrite-migrate-java issue 540 : Add explicit imports to avoid conflicts with classes added to java.lang, like Record --- .../org/openrewrite/java/AddImportTest.java | 41 +++++++++++++++++++ .../java/org/openrewrite/java/AddImport.java | 3 +- 2 files changed, 43 insertions(+), 1 deletion(-) diff --git a/rewrite-java-test/src/test/java/org/openrewrite/java/AddImportTest.java b/rewrite-java-test/src/test/java/org/openrewrite/java/AddImportTest.java index d82981f8863..407790bf6a3 100644 --- a/rewrite-java-test/src/test/java/org/openrewrite/java/AddImportTest.java +++ b/rewrite-java-test/src/test/java/org/openrewrite/java/AddImportTest.java @@ -184,6 +184,47 @@ class A { ); } + @Test + void forceImportNoJavaRecord() { + rewriteRun( + spec -> spec.recipe(toRecipe(() -> new AddImport<>("com.acme.bank.Record", null, false))), + //language=java + java( + """ + package com.acme.bank; + + class Foo { + } + """, + """ + package com.acme.bank; + + import com.acme.bank.Record; + + class Foo { + } + """, + spec -> spec.markers(javaVersion(11)) + ) + ); + } + + @Test + void forceImportJavaRecord() { + rewriteRun( + spec -> spec.recipe(toRecipe(() -> new AddImport<>("java.lang.Record", null, false))), + //language=java + java( + """ + package com.acme.bank; + + class Foo { + } + """, + spec -> spec.markers(javaVersion(11)) + ) + ); + } @Test void dontImportJavaLang() { rewriteRun( diff --git a/rewrite-java/src/main/java/org/openrewrite/java/AddImport.java b/rewrite-java/src/main/java/org/openrewrite/java/AddImport.java index 9aecd66f89f..c9908e66aaa 100644 --- a/rewrite-java/src/main/java/org/openrewrite/java/AddImport.java +++ b/rewrite-java/src/main/java/org/openrewrite/java/AddImport.java @@ -106,7 +106,8 @@ public AddImport(@Nullable String packageName, String typeName, @Nullable String } // No need to add imports if the class to import is in java.lang, or if the classes are within the same package - if (("java.lang".equals(packageName) && StringUtils.isBlank(member)) || (cu.getPackageDeclaration() != null && + if (("java.lang".equals(packageName) && StringUtils.isBlank(member)) || + (!"Record".equals(typeName) && cu.getPackageDeclaration() != null && packageName.equals(cu.getPackageDeclaration().getExpression().printTrimmed(getCursor())))) { return cu; } From 897219673a9838a9b4e12ee3e1b8c0a1c6e0a66e Mon Sep 17 00:00:00 2001 From: bramli ahmed khalil Date: Sun, 15 Dec 2024 09:17:27 +0100 Subject: [PATCH 2/4] change test name --- .../src/test/java/org/openrewrite/java/AddImportTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rewrite-java-test/src/test/java/org/openrewrite/java/AddImportTest.java b/rewrite-java-test/src/test/java/org/openrewrite/java/AddImportTest.java index 407790bf6a3..68ba9620f70 100644 --- a/rewrite-java-test/src/test/java/org/openrewrite/java/AddImportTest.java +++ b/rewrite-java-test/src/test/java/org/openrewrite/java/AddImportTest.java @@ -210,7 +210,7 @@ class Foo { } @Test - void forceImportJavaRecord() { + void notForceImportJavaRecord() { rewriteRun( spec -> spec.recipe(toRecipe(() -> new AddImport<>("java.lang.Record", null, false))), //language=java From de84ed1db4726ee0d9faafb14c884ff15cd4464c Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Sun, 15 Dec 2024 12:19:37 +0100 Subject: [PATCH 3/4] Split conditional and add clarifying comments on Record handling --- .../java/org/openrewrite/java/AddImportTest.java | 2 ++ .../main/java/org/openrewrite/java/AddImport.java | 12 ++++++++---- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/rewrite-java-test/src/test/java/org/openrewrite/java/AddImportTest.java b/rewrite-java-test/src/test/java/org/openrewrite/java/AddImportTest.java index 68ba9620f70..f750d286937 100644 --- a/rewrite-java-test/src/test/java/org/openrewrite/java/AddImportTest.java +++ b/rewrite-java-test/src/test/java/org/openrewrite/java/AddImportTest.java @@ -186,6 +186,7 @@ class A { @Test void forceImportNoJavaRecord() { + // Add import for a class named `Record`, even within the same package, to avoid conflicts with java.lang.Record rewriteRun( spec -> spec.recipe(toRecipe(() -> new AddImport<>("com.acme.bank.Record", null, false))), //language=java @@ -211,6 +212,7 @@ class Foo { @Test void notForceImportJavaRecord() { + // Do not add import for java.lang.Record by default rewriteRun( spec -> spec.recipe(toRecipe(() -> new AddImport<>("java.lang.Record", null, false))), //language=java diff --git a/rewrite-java/src/main/java/org/openrewrite/java/AddImport.java b/rewrite-java/src/main/java/org/openrewrite/java/AddImport.java index c9908e66aaa..4299ef1dd08 100644 --- a/rewrite-java/src/main/java/org/openrewrite/java/AddImport.java +++ b/rewrite-java/src/main/java/org/openrewrite/java/AddImport.java @@ -105,10 +105,14 @@ public AddImport(@Nullable String packageName, String typeName, @Nullable String return cu; } - // No need to add imports if the class to import is in java.lang, or if the classes are within the same package - if (("java.lang".equals(packageName) && StringUtils.isBlank(member)) || - (!"Record".equals(typeName) && cu.getPackageDeclaration() != null && - packageName.equals(cu.getPackageDeclaration().getExpression().printTrimmed(getCursor())))) { + // No need to add imports if the class to import is in java.lang + if ("java.lang".equals(packageName) && StringUtils.isBlank(member)) { + return cu; + } + // Nor if the classes are within the same package + if (!"Record".equals(typeName) && // Record's late addition to `java.lang` might conflict with user class + cu.getPackageDeclaration() != null && + packageName.equals(cu.getPackageDeclaration().getExpression().printTrimmed(getCursor()))) { return cu; } From 4cb70f540527cd3e34e29510cb37f9b5089ac99e Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Sun, 15 Dec 2024 12:22:10 +0100 Subject: [PATCH 4/4] Add issue links to clarify the reason these tests were added --- .../src/test/java/org/openrewrite/java/AddImportTest.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/rewrite-java-test/src/test/java/org/openrewrite/java/AddImportTest.java b/rewrite-java-test/src/test/java/org/openrewrite/java/AddImportTest.java index f750d286937..ac15fd5da84 100644 --- a/rewrite-java-test/src/test/java/org/openrewrite/java/AddImportTest.java +++ b/rewrite-java-test/src/test/java/org/openrewrite/java/AddImportTest.java @@ -184,6 +184,7 @@ class A { ); } + @Issue("https://github.com/openrewrite/rewrite-migrate-java/issues/540") @Test void forceImportNoJavaRecord() { // Add import for a class named `Record`, even within the same package, to avoid conflicts with java.lang.Record @@ -210,6 +211,7 @@ class Foo { ); } + @Issue("https://github.com/openrewrite/rewrite-migrate-java/issues/540") @Test void notForceImportJavaRecord() { // Do not add import for java.lang.Record by default