diff --git a/src/main/java/org/openrewrite/java/migrate/util/ListFirstAndLast.java b/src/main/java/org/openrewrite/java/migrate/util/ListFirstAndLast.java index c848f93e0d..90d86184e2 100644 --- a/src/main/java/org/openrewrite/java/migrate/util/ListFirstAndLast.java +++ b/src/main/java/org/openrewrite/java/migrate/util/ListFirstAndLast.java @@ -34,6 +34,7 @@ public class ListFirstAndLast extends Recipe { + // While more SequencedCollections have `*First` and `*Last` methods, only list has `get`, `add`, and `remove` methods that take an index private static final MethodMatcher ADD_MATCHER = new MethodMatcher("java.util.List add(int, ..)", true); // , * fails private static final MethodMatcher GET_MATCHER = new MethodMatcher("java.util.List get(int)", true); private static final MethodMatcher REMOVE_MATCHER = new MethodMatcher("java.util.List remove(int)", true); @@ -41,7 +42,7 @@ public class ListFirstAndLast extends Recipe { @Override public String getDisplayName() { - return "Replace `List` `get`, `add`, and `remove` with `SequencedCollection` `*First` and `*Last` methods"; + return "Replace `List.get(int)`, `add(int, Object)`, and `remove(int)` with `SequencedCollection` `*First` and `*Last` methods"; } @Override @@ -67,11 +68,6 @@ private static class FirstLastVisitor extends JavaIsoVisitor { @Override public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, ExecutionContext ctx) { J.MethodInvocation mi = super.visitMethodInvocation(method, ctx); - Expression select = mi.getSelect(); - if (!(select instanceof J.Identifier)) { - return mi; - } - J.Identifier sequencedCollection = (J.Identifier) select; final String operation; if (ADD_MATCHER.matches(mi)) { @@ -84,6 +80,22 @@ public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, Execu return mi; } + // Limit *Last to identifiers for now, as x.get(x.size() - 1) requires the same reference for x + if (mi.getSelect() instanceof J.Identifier) { + return handleSelectIdentifier((J.Identifier) mi.getSelect(), mi, operation); + } + + // XXX Maybe handle J.FieldAccess explicitly as well to support *Last on fields too + + // For anything else support limited cases, as we can't guarantee the same reference for the collection + if (J.Literal.isLiteralValue(mi.getArguments().get(0), 0)) { + return getMethodInvocation(mi, operation, "First"); + } + + return mi; + } + + private static J.MethodInvocation handleSelectIdentifier(J.Identifier sequencedCollection, J.MethodInvocation mi, String operation) { final String firstOrLast; Expression expression = mi.getArguments().get(0); if (J.Literal.isLiteralValue(expression, 0)) { @@ -93,7 +105,10 @@ public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, Execu } else { return mi; } + return getMethodInvocation(mi, operation, firstOrLast); + } + private static J.MethodInvocation getMethodInvocation(J.MethodInvocation mi, String operation, String firstOrLast) { List arguments = new ArrayList<>(); final JavaType.Method newMethodType; JavaType.Method originalMethodType = mi.getMethodType(); @@ -123,8 +138,8 @@ private static boolean lastElementOfSequencedCollection(J.Identifier sequencedCo if (expression instanceof J.Binary) { J.Binary binary = (J.Binary) expression; if (binary.getOperator() == J.Binary.Type.Subtraction - && J.Literal.isLiteralValue(binary.getRight(), 1) - && SIZE_MATCHER.matches(binary.getLeft())) { + && J.Literal.isLiteralValue(binary.getRight(), 1) + && SIZE_MATCHER.matches(binary.getLeft())) { Expression sizeSelect = ((J.MethodInvocation) binary.getLeft()).getSelect(); if (sizeSelect instanceof J.Identifier) { return sequencedCollection.getSimpleName().equals(((J.Identifier) sizeSelect).getSimpleName()); diff --git a/src/test/java/org/openrewrite/java/migrate/util/ListFirstAndLastTest.java b/src/test/java/org/openrewrite/java/migrate/util/ListFirstAndLastTest.java index dc005c5348..a5d531ceb5 100644 --- a/src/test/java/org/openrewrite/java/migrate/util/ListFirstAndLastTest.java +++ b/src/test/java/org/openrewrite/java/migrate/util/ListFirstAndLastTest.java @@ -64,6 +64,42 @@ String bar(List collection) { ); } + @Test + @Issue("https://github.com/openrewrite/rewrite-migrate-java/issues/423") + void getFirstFromMethodInvocation() { + rewriteRun( + //language=java + java( + """ + import java.util.*; + + class Foo { + List collection() { + return new ArrayList<>(); + } + + String bar() { + return collection().get(0); + } + } + """, + """ + import java.util.*; + + class Foo { + List collection() { + return new ArrayList<>(); + } + + String bar() { + return collection().getFirst(); + } + } + """ + ) + ); + } + @Test void addFirst() { rewriteRun( @@ -91,6 +127,40 @@ void bar(List collection) { ); } + @Test + @Issue("https://github.com/openrewrite/rewrite-migrate-java/issues/423") + void addFirstFromMethodInvocation() { + rewriteRun( + //language=java + java( + """ + import java.util.*; + + class Foo { + List collection() { + return new ArrayList<>(); + } + void bar() { + collection().add(0, "first"); + } + } + """, + """ + import java.util.*; + + class Foo { + List collection() { + return new ArrayList<>(); + } + void bar() { + collection().addFirst("first"); + } + } + """ + ) + ); + } + @Test void removeFirst() { rewriteRun( @@ -117,6 +187,42 @@ String bar(List collection) { ) ); } + + @Test + @Issue("https://github.com/openrewrite/rewrite-migrate-java/issues/423") + void removeFirstFromMethodInvocation() { + rewriteRun( + //language=java + java( + """ + import java.util.*; + + class Foo { + List collection() { + return new ArrayList<>(); + } + + String bar() { + return collection().remove(0); + } + } + """, + """ + import java.util.*; + + class Foo { + List collection() { + return new ArrayList<>(); + } + + String bar() { + return collection().removeFirst(); + } + } + """ + ) + ); + } } @Nested