From ff1da3f228fd7a882a82bbd87a5f8b7b66a4a760 Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Mon, 6 Nov 2023 12:33:11 +0100 Subject: [PATCH] Adopt SequencedCollection First and Last methods (#339) * Adopt SequencedCollection First and Last methods * Add license header * First passing tests when running on Java 21 * Add negative tests and simplify condition * Add IteratorNext recipe that only transforms sequenced collections * Limit recipes to running on Java 21 * Do not use `addLast` as the index is off by one * Use correct method type on getFirst * Use correct return type and declaring type * Drop JavaTemplate as template was context sensitive * Drop afterTypeValidationOptions by adding rewrite-java-21 * More descriptive test names * Document improbable comment loss --- build.gradle.kts | 1 + .../java/migrate/util/IteratorNext.java | 74 +++++ .../java/migrate/util/ListFirstAndLast.java | 137 +++++++++ .../META-INF/rewrite/java-version-21.yml | 11 +- .../java/migrate/util/IteratorNextTest.java | 144 +++++++++ .../migrate/util/ListFirstAndLastTest.java | 291 ++++++++++++++++++ .../migrate/util/SequencedCollectionTest.java | 33 +- 7 files changed, 650 insertions(+), 41 deletions(-) create mode 100644 src/main/java/org/openrewrite/java/migrate/util/IteratorNext.java create mode 100644 src/main/java/org/openrewrite/java/migrate/util/ListFirstAndLast.java create mode 100644 src/test/java/org/openrewrite/java/migrate/util/IteratorNextTest.java create mode 100644 src/test/java/org/openrewrite/java/migrate/util/ListFirstAndLastTest.java diff --git a/build.gradle.kts b/build.gradle.kts index 423705441d..91d7d64a5e 100644 --- a/build.gradle.kts +++ b/build.gradle.kts @@ -38,6 +38,7 @@ dependencies { runtimeOnly("org.openrewrite:rewrite-java-8") runtimeOnly("org.openrewrite:rewrite-java-11") runtimeOnly("org.openrewrite:rewrite-java-17") + runtimeOnly("org.openrewrite:rewrite-java-21") testImplementation("org.junit.jupiter:junit-jupiter-api:latest.release") testImplementation("org.junit.jupiter:junit-jupiter-params:latest.release") diff --git a/src/main/java/org/openrewrite/java/migrate/util/IteratorNext.java b/src/main/java/org/openrewrite/java/migrate/util/IteratorNext.java new file mode 100644 index 0000000000..a6ea1ee945 --- /dev/null +++ b/src/main/java/org/openrewrite/java/migrate/util/IteratorNext.java @@ -0,0 +1,74 @@ +/* + * Copyright 2023 the original author or authors. + *

+ * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + *

+ * https://www.apache.org/licenses/LICENSE-2.0 + *

+ * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.openrewrite.java.migrate.util; + +import org.openrewrite.ExecutionContext; +import org.openrewrite.Preconditions; +import org.openrewrite.Recipe; +import org.openrewrite.TreeVisitor; +import org.openrewrite.java.JavaIsoVisitor; +import org.openrewrite.java.MethodMatcher; +import org.openrewrite.java.search.UsesJavaVersion; +import org.openrewrite.java.search.UsesMethod; +import org.openrewrite.java.tree.Expression; +import org.openrewrite.java.tree.J; +import org.openrewrite.java.tree.TypeUtils; + +public class IteratorNext extends Recipe { + private static final MethodMatcher ITERATOR_MATCHER = new MethodMatcher("java.util.Collection iterator()", true); + private static final MethodMatcher NEXT_MATCHER = new MethodMatcher("java.util.Iterator next()", true); + + @Override + public String getDisplayName() { + return "Replace `iterator().next()` with `getFirst()`"; + } + + @Override + public String getDescription() { + return "Replace `SequencedCollection.iterator().next()` with `getFirst()`."; + } + + @Override + public TreeVisitor getVisitor() { + return Preconditions.check( + Preconditions.and( + new UsesJavaVersion<>(21), + Preconditions.and( + new UsesMethod<>(ITERATOR_MATCHER), + new UsesMethod<>(NEXT_MATCHER) + ) + ), + new JavaIsoVisitor() { + @Override + public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, ExecutionContext executionContext) { + J.MethodInvocation nextInvocation = super.visitMethodInvocation(method, executionContext); + if (NEXT_MATCHER.matches(nextInvocation) && ITERATOR_MATCHER.matches(nextInvocation.getSelect())) { + J.MethodInvocation iteratorInvocation = (J.MethodInvocation) nextInvocation.getSelect(); + if (TypeUtils.isAssignableTo("java.util.SequencedCollection", iteratorInvocation.getSelect().getType())) { + J.MethodInvocation getFirstInvocation = nextInvocation.withSelect(iteratorInvocation.getSelect()) + .withName(nextInvocation.getName().withSimpleName("getFirst")) + .withMethodType(nextInvocation.getMethodType() + .withName("getFirst") + .withDeclaringType(iteratorInvocation.getMethodType().getDeclaringType())); + return getFirstInvocation; + } + } + return nextInvocation; + } + } + ); + } +} diff --git a/src/main/java/org/openrewrite/java/migrate/util/ListFirstAndLast.java b/src/main/java/org/openrewrite/java/migrate/util/ListFirstAndLast.java new file mode 100644 index 0000000000..15ad7cd029 --- /dev/null +++ b/src/main/java/org/openrewrite/java/migrate/util/ListFirstAndLast.java @@ -0,0 +1,137 @@ +/* + * Copyright 2023 the original author or authors. + *

+ * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + *

+ * https://www.apache.org/licenses/LICENSE-2.0 + *

+ * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.openrewrite.java.migrate.util; + +import org.openrewrite.ExecutionContext; +import org.openrewrite.Preconditions; +import org.openrewrite.Recipe; +import org.openrewrite.TreeVisitor; +import org.openrewrite.java.JavaIsoVisitor; +import org.openrewrite.java.MethodMatcher; +import org.openrewrite.java.search.UsesJavaVersion; +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.Space; + +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; + +public class ListFirstAndLast extends Recipe { + + 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); + private static final MethodMatcher SIZE_MATCHER = new MethodMatcher("java.util.List size()", true); + + @Override + public String getDisplayName() { + return "Replace `List` `get`, `add`, and `remove` with `SequencedCollection` `*First` and `*Last` methods"; + } + + @Override + public String getDescription() { + return "Replace `list.get(0)` with `list.getFirst()`, `list.get(list.size() - 1)` with `list.getLast()`, and similar for `add(int, E)` and `remove(int)`."; + } + + @Override + public TreeVisitor getVisitor() { + return Preconditions.check( + Preconditions.and( + new UsesJavaVersion<>(21), + Preconditions.or( + new UsesMethod<>(ADD_MATCHER), + new UsesMethod<>(GET_MATCHER), + new UsesMethod<>(REMOVE_MATCHER) + ) + ), + new FirstLastVisitor()); + } + + private static class FirstLastVisitor extends JavaIsoVisitor { + @Override + public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, ExecutionContext executionContext) { + J.MethodInvocation mi = super.visitMethodInvocation(method, executionContext); + 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)) { + operation = "add"; + } else if (GET_MATCHER.matches(mi)) { + operation = "get"; + } else if (REMOVE_MATCHER.matches(mi)) { + operation = "remove"; + } else { + return mi; + } + + final String firstOrLast; + Expression expression = mi.getArguments().get(0); + if (J.Literal.isLiteralValue(expression, 0)) { + firstOrLast = "First"; + } else if (!"add".equals(operation) && lastElementOfSequencedCollection(sequencedCollection, expression)) { + firstOrLast = "Last"; + } else { + return mi; + } + + List arguments = new ArrayList<>(); + final JavaType.Method newMethodType; + JavaType.Method originalMethodType = mi.getMethodType(); + if ("add".equals(operation)) { + arguments.add(mi.getArguments().get(1).withPrefix(Space.EMPTY)); + newMethodType = originalMethodType + .withName(operation + firstOrLast) + .withParameterNames(Collections.singletonList(originalMethodType.getParameterNames().get(1))) + .withParameterTypes(Collections.singletonList(originalMethodType.getParameterTypes().get(1))); + } else { + newMethodType = originalMethodType + .withName(operation + firstOrLast) + .withParameterNames(null) + .withParameterTypes(null); + } + return mi.withName(mi.getName().withSimpleName(operation + firstOrLast)) + .withArguments(arguments) + .withMethodType(newMethodType); + } + + /** + * @param sequencedCollection the identifier of the collection we're calling `get` on + * @param expression the expression we're passing to `get` + * @return true, if we're calling `sequencedCollection.size() - 1` in expression on the same collection + */ + private static boolean lastElementOfSequencedCollection(J.Identifier sequencedCollection, Expression expression) { + 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())) { + Expression sizeSelect = ((J.MethodInvocation) binary.getLeft()).getSelect(); + if (sizeSelect instanceof J.Identifier) { + return sequencedCollection.getSimpleName().equals(((J.Identifier) sizeSelect).getSimpleName()); + } + } + } + return false; + } + } +} diff --git a/src/main/resources/META-INF/rewrite/java-version-21.yml b/src/main/resources/META-INF/rewrite/java-version-21.yml index fc294dd412..7f8c6b81f6 100644 --- a/src/main/resources/META-INF/rewrite/java-version-21.yml +++ b/src/main/resources/META-INF/rewrite/java-version-21.yml @@ -54,10 +54,9 @@ tags: - java21 - collections recipeList: + - org.openrewrite.java.migrate.util.FirstAndLast + - org.openrewrite.java.migrate.util.IteratorNext # Iterator - - org.openrewrite.java.SimplifyMethodChain: - methodPatternChain: [ 'java.util.SequencedCollection iterator()', 'java.util.Iterator next()' ] - newMethodName: getFirst - org.openrewrite.java.ChangeMethodName: methodPattern: java.util.SortedSet first() newMethodName: getFirst @@ -68,11 +67,5 @@ recipeList: methodPattern: java.util.NavigableSet descendingSet() newMethodName: reversed # TODO convert additional patterns to sequenced collections - # SequencedCollection.get(0) -> SequencedCollection.getFirst() - # SequencedCollection.get(SequencedCollection.size() - 1) -> SequencedCollection.getLast() - # list.add(0, E) -> SequencedCollection.addFirst(E) - # list.addLast(list.size(), E) -> SequencedCollection.addLast(E) - # list.remove(0) -> SequencedCollection.removeFirst() - # list.remove(list.size() - 1) -> SequencedCollection.removeLast() # list.listIterator() -> ??? # list.listIterator(int) -> ??? diff --git a/src/test/java/org/openrewrite/java/migrate/util/IteratorNextTest.java b/src/test/java/org/openrewrite/java/migrate/util/IteratorNextTest.java new file mode 100644 index 0000000000..ad9a517ab0 --- /dev/null +++ b/src/test/java/org/openrewrite/java/migrate/util/IteratorNextTest.java @@ -0,0 +1,144 @@ +/* + * Copyright 2023 the original author or authors. + *

+ * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + *

+ * https://www.apache.org/licenses/LICENSE-2.0 + *

+ * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.openrewrite.java.migrate.util; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.condition.EnabledForJreRange; +import org.junit.jupiter.api.condition.JRE; +import org.openrewrite.Issue; +import org.openrewrite.test.RecipeSpec; +import org.openrewrite.test.RewriteTest; + +import static org.openrewrite.java.Assertions.java; +import static org.openrewrite.java.Assertions.javaVersion; + +@Issue("https://github.com/openrewrite/rewrite-migrate-java/issues/243") +@EnabledForJreRange(min = JRE.JAVA_21) +class IteratorNextTest implements RewriteTest { + @Override + public void defaults(RecipeSpec spec) { + spec.recipe(new IteratorNext()) + .allSources(src -> src.markers(javaVersion(21))); + } + + @Test + void listIteratorNextToGetFirst() { + rewriteRun( + //language=java + java( + """ + import java.util.*; + + class Foo { + String bar(List collection) { + return collection.iterator().next(); + } + } + """, + """ + import java.util.*; + + class Foo { + String bar(List collection) { + return collection.getFirst(); + } + } + """ + ) + ); + } + + @Test + void nonSequencedCollectionUnchanged() { + rewriteRun( + //language=java + java( + """ + import java.util.*; + + class Foo { + void bar(Collection collection) { + String first = collection.iterator().next(); + } + } + """ + ) + ); + } + + @Test + void nextCommentRetained() { + rewriteRun( + //language=java + java( + """ + import java.util.*; + + class Foo { + void bar(List collection) { + String first = collection + .iterator() + // Next comment + .next(); + } + } + """, + """ + import java.util.*; + + class Foo { + void bar(List collection) { + String first = collection + // Next comment + .getFirst(); + } + } + """ + ) + ); + } + + @Test + void iteratorCommentLost() { + rewriteRun( + //language=java + java( + """ + import java.util.*; + + class Foo { + void bar(List collection) { + String first = collection + // Iterator comment + .iterator() + .next(); + } + } + """, + """ + import java.util.List; + + class Foo { + void bar(List collection) { + String first = collection + .getFirst(); + } + } + """ + ) + ); + } +} diff --git a/src/test/java/org/openrewrite/java/migrate/util/ListFirstAndLastTest.java b/src/test/java/org/openrewrite/java/migrate/util/ListFirstAndLastTest.java new file mode 100644 index 0000000000..94bd5cd37e --- /dev/null +++ b/src/test/java/org/openrewrite/java/migrate/util/ListFirstAndLastTest.java @@ -0,0 +1,291 @@ +/* + * Copyright 2023 the original author or authors. + *

+ * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + *

+ * https://www.apache.org/licenses/LICENSE-2.0 + *

+ * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.openrewrite.java.migrate.util; + +import org.junit.jupiter.api.Nested; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.condition.EnabledForJreRange; +import org.junit.jupiter.api.condition.JRE; +import org.openrewrite.Issue; +import org.openrewrite.test.RecipeSpec; +import org.openrewrite.test.RewriteTest; + +import static org.openrewrite.java.Assertions.java; +import static org.openrewrite.java.Assertions.javaVersion; + + +@Issue("https://github.com/openrewrite/rewrite-migrate-java/issues/243") +@EnabledForJreRange(min = JRE.JAVA_21) +class ListFirstAndLastTest implements RewriteTest { + + @Override + public void defaults(RecipeSpec spec) { + spec.recipe(new ListFirstAndLast()) + .allSources(src -> src.markers(javaVersion(21))); + } + + @Nested + class First { + @Test + void getFirst() { + rewriteRun( + //language=java + java( + """ + import java.util.*; + + class Foo { + String bar(List collection) { + return collection.get(0); + } + } + """, + """ + import java.util.*; + + class Foo { + String bar(List collection) { + return collection.getFirst(); + } + } + """ + ) + ); + } + + @Test + void addFirst() { + rewriteRun( + //language=java + java( + """ + import java.util.*; + + class Foo { + void bar(List collection) { + collection.add(0, "first"); + } + } + """, + """ + import java.util.*; + + class Foo { + void bar(List collection) { + collection.addFirst("first"); + } + } + """ + ) + ); + } + + @Test + void removeFirst() { + rewriteRun( + //language=java + java( + """ + import java.util.*; + + class Foo { + String bar(List collection) { + return collection.remove(0); + } + } + """, + """ + import java.util.*; + + class Foo { + String bar(List collection) { + return collection.removeFirst(); + } + } + """ + ) + ); + } + } + + @Nested + class Last { + @Test + void getLast() { + rewriteRun( + //language=java + java( + """ + import java.util.*; + + class Foo { + String bar(List collection) { + return collection.get(collection.size() - 1); + } + } + """, + """ + import java.util.*; + + class Foo { + String bar(List collection) { + return collection.getLast(); + } + } + """ + ) + ); + } + + @Test + void removeLast() { + rewriteRun( + //language=java + java( + """ + import java.util.*; + + class Foo { + String bar(List collection) { + return collection.remove(collection.size() - 1); + } + } + """, + """ + import java.util.*; + + class Foo { + String bar(List collection) { + return collection.removeLast(); + } + } + """ + ) + ); + } + } + + @Nested + class NoChange { + @Test + void getSecond() { + rewriteRun( + //language=java + java( + """ + import java.util.*; + + class Foo { + String bar(List collection) { + return collection.get(1); + } + } + """ + ) + ); + } + + @Test + void getSecondToLast() { + rewriteRun( + //language=java + java( + """ + import java.util.List; + + class Foo { + String bar(List collection) { + return collection.get(collection.size() - 2); + } + } + """ + ) + ); + } + + @Test + void getLastOfDifferentCollection() { + rewriteRun( + //language=java + java( + """ + import java.util.*; + + class Foo { + String bar(List a, List b) { + return a.get(b.size() - 1); + } + } + """ + ) + ); + } + + @Test + void addInteger() { + rewriteRun( + //language=java + java( + """ + import java.util.*; + + class Foo { + String bar(List collection) { + return collection.add(0); + } + } + """ + ) + ); + } + + @Test + void addAtSize() { + rewriteRun( + //language=java + java( + """ + import java.util.*; + + class Foo { + void bar(List collection) { + collection.add(collection.size(), "last"); + } + } + """ + ) + ); + } + + @Test + void addAtSizeMinusOne() { + rewriteRun( + //language=java + java( + """ + import java.util.*; + + class Foo { + void bar(List collection) { + collection.add(collection.size() - 1, "last"); + } + } + """ + ) + ); + } + } +} diff --git a/src/test/java/org/openrewrite/java/migrate/util/SequencedCollectionTest.java b/src/test/java/org/openrewrite/java/migrate/util/SequencedCollectionTest.java index c81ab6e434..1d677d17b6 100644 --- a/src/test/java/org/openrewrite/java/migrate/util/SequencedCollectionTest.java +++ b/src/test/java/org/openrewrite/java/migrate/util/SequencedCollectionTest.java @@ -27,44 +27,13 @@ @Issue("https://github.com/openrewrite/rewrite-migrate-java/issues/243") +@EnabledForJreRange(min = JRE.JAVA_21) class SequencedCollectionTest implements RewriteTest { - @Override public void defaults(RecipeSpec spec) { spec.recipeFromResource("/META-INF/rewrite/java-version-21.yml", "org.openrewrite.java.migrate.util.SequencedCollection"); } - @Nested - class IteratorNext { - @Test - @EnabledForJreRange(min = JRE.JAVA_21) - void sequencedCollectionIteratorNextToGetFirst() { - rewriteRun( - //language=java - java( - """ - import java.util.*; - - class Foo { - void bar(SequencedCollection collection) { - String first = collection.iterator().next(); - } - } - """, - """ - import java.util.*; - - class Foo { - void bar(SequencedCollection collection) { - String first = collection.getFirst(); - } - } - """ - ) - ); - } - } - @Nested class SortedSetFirstLast { @Test