diff --git a/src/main/java/org/openrewrite/java/migrate/util/UseListOf.java b/src/main/java/org/openrewrite/java/migrate/util/UseListOf.java index 0d47ab02cf..8d935f537d 100644 --- a/src/main/java/org/openrewrite/java/migrate/util/UseListOf.java +++ b/src/main/java/org/openrewrite/java/migrate/util/UseListOf.java @@ -16,6 +16,7 @@ package org.openrewrite.java.migrate.util; import lombok.Getter; +import org.jspecify.annotations.Nullable; import org.openrewrite.ExecutionContext; import org.openrewrite.Preconditions; import org.openrewrite.Recipe; @@ -29,6 +30,7 @@ import org.openrewrite.java.search.UsesMethod; import org.openrewrite.java.tree.Expression; import org.openrewrite.java.tree.J; +import org.openrewrite.java.tree.JavaType; import org.openrewrite.java.tree.Statement; import org.openrewrite.java.tree.TypeUtils; @@ -39,7 +41,15 @@ public class UseListOf extends Recipe { private static final MethodMatcher NEW_ARRAY_LIST = new MethodMatcher("java.util.ArrayList ()", true); + private static final MethodMatcher NEW_LINKED_HASH_SET = new MethodMatcher("java.util.LinkedHashSet ()", true); private static final MethodMatcher LIST_ADD = new MethodMatcher("java.util.List add(..)", true); + private static final MethodMatcher COLLECTION_ADD = new MethodMatcher("java.util.Collection add(..)", true); + + /** + * Concrete collection types the prose pattern may keep: each preserves insertion order and has a + * {@code Collection} constructor, so wrapping an ordered {@code List.of(..)} is behavior preserving. + */ + private static final List ORDERED_COLLECTIONS = Arrays.asList("java.util.ArrayList", "java.util.LinkedHashSet"); private static final String PROSE_REWRITES_KEY = "use-list-of.prose-rewrites"; @@ -51,36 +61,42 @@ public class UseListOf extends Recipe { "- Anonymous-class initialization (`new ArrayList<>() {{ add(\"a\"); add(\"b\"); }}`), " + "which is replaced wholesale with `List.of(\"a\", \"b\")` (immutable result, matching the " + "anonymous-class idiom's typical intent).\n" + - "- A `new ArrayList<>()` declaration followed by a chain of `target.add(..)` statements, " + - "which is collapsed to `new ArrayList<>(List.of(..))` (preserving the mutable `ArrayList`)."; + "- A `new ArrayList<>()` or `new LinkedHashSet<>()` declaration followed by a chain of " + + "`target.add(..)` statements, which is collapsed to `new ArrayList<>(List.of(..))` or " + + "`new LinkedHashSet<>(List.of(..))` (preserving both the mutable collection and its iteration order)."; @Override public TreeVisitor getVisitor() { return Preconditions.check( Preconditions.and( new UsesJavaVersion<>(10), - new UsesMethod<>(NEW_ARRAY_LIST)), + Preconditions.or( + new UsesMethod<>(NEW_ARRAY_LIST), + new UsesMethod<>(NEW_LINKED_HASH_SET))), new JavaVisitor() { @Override public J visitNewClass(J.NewClass newClass, ExecutionContext ctx) { J.NewClass n = (J.NewClass) super.visitNewClass(newClass, ctx); // Prose-pattern: see if visitBlock (above us on the cursor) decided this - // initializer should be wrapped with `new ArrayList<>(List.of(..))`. + // initializer should keep its type and wrap a `List.of(..)`. Map> rewrites = getCursor().getNearestMessage(PROSE_REWRITES_KEY); - if (rewrites != null) { + String orderedCollection = orderedCollectionType(n); + if (rewrites != null && orderedCollection != null) { List adds = rewrites.get(n.getId()); if (adds != null) { + String simpleName = orderedCollection.substring(orderedCollection.lastIndexOf('.') + 1); List args = new ArrayList<>(); - StringJoiner joiner = new StringJoiner(", ", "new ArrayList<>(List.of(", "))"); + StringJoiner joiner = new StringJoiner(", ", "new " + simpleName + "<>(List.of(", "))"); for (J.MethodInvocation add : adds) { args.add(add.getArguments().get(0)); joiner.add("#{any()}"); } + maybeAddImport(orderedCollection); maybeAddImport("java.util.List"); J applied = JavaTemplate.builder(joiner.toString()) .contextSensitive() - .imports("java.util.ArrayList", "java.util.List") + .imports(orderedCollection, "java.util.List") .build() .apply(updateCursor(n), n.getCoordinates().replace(), args.toArray()); // Reattach each add's prefix so the elements land one-per-line and any @@ -124,7 +140,7 @@ public J visitNewClass(J.NewClass newClass, ExecutionContext ctx) { /** * Re-applies the absorbed add statements' prefixes to the generated - * {@code new ArrayList<>(List.of(..))} so each element keeps its own line and any + * {@code new ArrayList<>(List.of(..))} constructor call so each element keeps its own line and any * leading comments. {@code adds} holds one invocation per element, in order. */ private J reattachElementPrefixes(J applied, List adds) { @@ -223,14 +239,14 @@ private void identifyProseRewrites( /** * Returns the variable name if {@code decl} is a single-variable, parameterized - * {@code List} declaration whose initializer is a no-arg {@code new ArrayList<>()} - * with no anonymous-class body. Returns {@code null} otherwise. + * declaration whose initializer is a no-arg {@code new ArrayList<>()} or + * {@code new LinkedHashSet<>()} with no anonymous-class body. Returns {@code null} otherwise. */ private String matchingTargetName(J.VariableDeclarations decl) { if (decl.getVariables().size() != 1) { return null; } - // Require parameterized LHS; for raw `List` we'd be guessing at a type argument. + // Require parameterized LHS; for a raw `List` we'd be guessing at a type argument. if (!(decl.getTypeExpression() instanceof J.ParameterizedType)) { return null; } @@ -239,10 +255,10 @@ private String matchingTargetName(J.VariableDeclarations decl) { return null; } J.NewClass nc = (J.NewClass) nv.getInitializer(); - if (!NEW_ARRAY_LIST.matches(nc)) { + if (!NEW_ARRAY_LIST.matches(nc) && !NEW_LINKED_HASH_SET.matches(nc)) { return null; } - if (!isExactlyArrayList(nc)) { + if (orderedCollectionType(nc) == null) { return null; } // A body would put us in the anonymous-class case handled by visitNewClass directly. @@ -253,16 +269,31 @@ private String matchingTargetName(J.VariableDeclarations decl) { } /** - * Skip `ArrayList` subclasses: `new ArrayList<>(..)` is not assignable to a subclass - * declared type, and the subclass may carry behavior beyond a plain `ArrayList` - * (issue #1181, matching #1113). + * Returns the constructed type if it is exactly one of {@link #ORDERED_COLLECTIONS}, which the + * prose rewrite retains rather than replacing. Subclasses return {@code null}: they may carry + * behavior beyond the collection they extend, and the constructor we'd generate might not exist + * on them (issue #1181, matching #1113). + */ + private @Nullable String orderedCollectionType(J.NewClass nc) { + JavaType type = nc.getClazz() != null ? nc.getClazz().getType() : null; + for (String fqn : ORDERED_COLLECTIONS) { + if (TypeUtils.isOfClassType(type, fqn)) { + return fqn; + } + } + return null; + } + + /** + * Skip `ArrayList` subclasses: the subclass may carry behavior beyond a plain `ArrayList` + * that `List.of(..)` would drop (issue #1181, matching #1113). */ private boolean isExactlyArrayList(J.NewClass nc) { return TypeUtils.isOfClassType(nc.getClazz() != null ? nc.getClazz().getType() : null, "java.util.ArrayList"); } /** - * If {@code stmt} is {@code targetName.add(arg)} matching {@link #LIST_ADD}, + * If {@code stmt} is {@code targetName.add(arg)} matching {@link #COLLECTION_ADD}, * returns the single argument expression; otherwise {@code null}. Also returns * {@code null} when the argument is the {@code null} literal, since * {@code List.of(..)} rejects nulls. @@ -272,7 +303,7 @@ private Expression matchAddCallOn(Statement stmt, String targetName) { return null; } J.MethodInvocation mi = (J.MethodInvocation) stmt; - if (!LIST_ADD.matches(mi)) { + if (!COLLECTION_ADD.matches(mi)) { return null; } if (mi.getArguments().size() != 1) { diff --git a/src/main/resources/META-INF/rewrite/recipes.csv b/src/main/resources/META-INF/rewrite/recipes.csv index ab4ac3c0b3..a25d96d967 100644 --- a/src/main/resources/META-INF/rewrite/recipes.csv +++ b/src/main/resources/META-INF/rewrite/recipes.csv @@ -497,7 +497,7 @@ maven,org.openrewrite.recipe:rewrite-migrate-java,org.openrewrite.java.migrate.u maven,org.openrewrite.recipe:rewrite-migrate-java,org.openrewrite.java.migrate.util.UseListOf,Prefer `List.of(..)`,"Prefer `List.of(..)` in Java 10 or higher. Two input shapes are recognised: - Anonymous-class initialization (`new ArrayList<>() {{ add(""a""); add(""b""); }}`), which is replaced wholesale with `List.of(""a"", ""b"")` (immutable result, matching the anonymous-class idiom's typical intent). -- A `new ArrayList<>()` declaration followed by a chain of `target.add(..)` statements, which is collapsed to `new ArrayList<>(List.of(..))` (preserving the mutable `ArrayList`).",1,,`java.util` 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.,, +- A `new ArrayList<>()` or `new LinkedHashSet<>()` declaration followed by a chain of `target.add(..)` statements, which is collapsed to `new ArrayList<>(List.of(..))` or `new LinkedHashSet<>(List.of(..))` (preserving both the mutable collection and its iteration order).",1,,`java.util` 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.util.UseLocaleOf,Prefer `Locale.of(..)` over `new Locale(..)`,Prefer `Locale.of(..)` over `new Locale(..)` in Java 19 or higher.,1,,`java.util` 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.util.UseMapOf,Prefer `Map.of(..)`,"Prefer `Map.of(..)` instead of using `java.util.Map#put(..)` in Java 10 or higher. Two input shapes are recognised: diff --git a/src/test/java/org/openrewrite/java/migrate/util/UseListOfTest.java b/src/test/java/org/openrewrite/java/migrate/util/UseListOfTest.java index 69ba279a4f..67b1c5ff12 100644 --- a/src/test/java/org/openrewrite/java/migrate/util/UseListOfTest.java +++ b/src/test/java/org/openrewrite/java/migrate/util/UseListOfTest.java @@ -329,6 +329,105 @@ void m() { ); } + @Issue("https://github.com/openrewrite/rewrite-migrate-java/issues/1181") + @Test + void convertLinkedHashSetToUseListOf() { + //language=java + rewriteRun( + java( + """ + import java.util.LinkedHashSet; + import java.util.Set; + + class Test { + void m() { + Set tags = new LinkedHashSet<>(); + tags.add("alpha"); + tags.add("beta"); + tags.add("gamma"); + } + } + """, + """ + import java.util.LinkedHashSet; + import java.util.List; + import java.util.Set; + + class Test { + void m() { + Set tags = new LinkedHashSet<>(List.of( + "alpha", + "beta", + "gamma")); + } + } + """ + ) + ); + } + + @Test + void proseAddChainOnCollectionDeclaredType() { + //language=java + rewriteRun( + java( + """ + import java.util.ArrayList; + import java.util.Collection; + + class Test { + void m() { + Collection names = new ArrayList<>(); + names.add("Bob"); + names.add("alice"); + } + } + """, + """ + import java.util.ArrayList; + import java.util.Collection; + import java.util.List; + + class Test { + void m() { + Collection names = new ArrayList<>(List.of( + "Bob", + "alice")); + } + } + """ + ) + ); + } + + @Issue("https://github.com/openrewrite/rewrite-migrate-java/issues/1181") + @Test + void doNotChangeLinkedHashSetSubclass() { + //language=java + rewriteRun( + java( + """ + import java.util.LinkedHashSet; + + class Tags extends LinkedHashSet {} + """ + ), + java( + """ + import java.util.Set; + + class Test { + void m() { + Set tags = new Tags(); + tags.add("alpha"); + tags.add("beta"); + } + } + """ + ) + ); + } + @Test void proseSingleAddBelowThresholdLeftAlone() { // A single add is below the threshold — the rewrite would be more noise than benefit.