From f45959c23382137130e1852a02d40d4675532552 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Sat, 1 Nov 2025 19:34:08 +0100 Subject: [PATCH] Extend `CollectionRules` Refaster rule collection Primarily by introducing rules for APIs introduced between Java 17 and Java 21. --- .../refasterrules/CollectionRules.java | 141 +++++++++++++++++- .../CollectionRulesTestInput.java | 52 ++++++- .../CollectionRulesTestOutput.java | 43 +++++- 3 files changed, 231 insertions(+), 5 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/CollectionRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/CollectionRules.java index 6bb8ca563a..5f6633cb66 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/CollectionRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/CollectionRules.java @@ -4,6 +4,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.Iterables; import com.google.common.collect.Lists; +import com.google.common.collect.Streams; import com.google.errorprone.refaster.Refaster; import com.google.errorprone.refaster.annotation.AfterTemplate; import com.google.errorprone.refaster.annotation.AlsoNegation; @@ -17,6 +18,7 @@ import java.util.NavigableSet; import java.util.Optional; import java.util.Queue; +import java.util.SequencedCollection; import java.util.Set; import java.util.SortedSet; import java.util.function.Consumer; @@ -399,7 +401,7 @@ Optional before(Collection collection) { @BeforeTemplate Optional before(List collection) { - return collection.isEmpty() ? Optional.empty() : Optional.of(collection.get(0)); + return collection.isEmpty() ? Optional.empty() : Optional.of(collection.getFirst()); } @BeforeTemplate @@ -484,6 +486,143 @@ void after(Collection collection, Consumer consumer) { } } + /** Prefer {@code collection.iterator().next()} over more contrived alternatives. */ + static final class CollectionIteratorNext { + @BeforeTemplate + S before(Collection collection) { + return collection.stream().findFirst().orElseThrow(); + } + + @AfterTemplate + S after(Collection collection) { + return collection.iterator().next(); + } + } + + /** Prefer {@link SequencedCollection#getFirst()} over less idiomatic alternatives. */ + static final class SequencedCollectionGetFirst { + @BeforeTemplate + S before(SequencedCollection collection) { + return collection.iterator().next(); + } + + @BeforeTemplate + S before(List collection) { + return collection.get(0); + } + + @AfterTemplate + S after(SequencedCollection collection) { + return collection.getFirst(); + } + } + + /** Prefer {@link SequencedCollection#getLast()} over less idiomatic alternatives. */ + static final class SequencedCollectionGetLast { + @BeforeTemplate + S before(SequencedCollection collection) { + return Refaster.anyOf( + collection.reversed().getFirst(), Streams.findLast(collection.stream()).orElseThrow()); + } + + @BeforeTemplate + S before(List collection) { + return collection.get(collection.size() - 1); + } + + @AfterTemplate + S after(SequencedCollection collection) { + return collection.getLast(); + } + } + + /** Prefer {@link List#addFirst(Object)} over less idiomatic alternatives. */ + static final class ListAddFirst { + @BeforeTemplate + void before(List list, T element) { + list.add(0, element); + } + + @AfterTemplate + void after(List list, T element) { + list.addFirst(element); + } + } + + /** Prefer {@link List#add(Object)} over less idiomatic alternatives. */ + static final class ListAdd { + @BeforeTemplate + void before(List list, T element) { + list.addLast(element); + } + + @BeforeTemplate + void before2(List list, T element) { + list.add(list.size(), element); + } + + @AfterTemplate + void after(List list, T element) { + list.add(element); + } + } + + /** Prefer {@link List#removeFirst()}} over less idiomatic alternatives. */ + // XXX: This rule changes the exception thrown for empty lists from `IndexOutOfBoundsException` to + // `NoSuchElementException`. + static final class ListRemoveFirst { + @BeforeTemplate + S before(List list) { + return list.remove(0); + } + + @AfterTemplate + S after(List list) { + return list.removeFirst(); + } + } + + /** Prefer {@link List#removeLast()}} over less idiomatic alternatives. */ + // XXX: This rule changes the exception thrown for empty lists from `IndexOutOfBoundsException` to + // `NoSuchElementException`. + static final class ListRemoveLast { + @BeforeTemplate + S before(List list) { + return list.remove(list.size() - 1); + } + + @AfterTemplate + S after(List list) { + return list.removeLast(); + } + } + + /** Prefer {@link SortedSet#first()} over more verbose alternatives. */ + static final class SortedSetFirst { + @BeforeTemplate + S before(SortedSet set) { + return set.getFirst(); + } + + @AfterTemplate + S after(SortedSet set) { + return set.first(); + } + } + + /** Prefer {@link SortedSet#last()} over more verbose alternatives. */ + static final class SortedSetLast { + @BeforeTemplate + S before(SortedSet set) { + return set.getLast(); + } + + @AfterTemplate + S after(SortedSet set) { + return set.last(); + } + } + // XXX: collection.stream().noneMatch(e -> e.equals(other)) // ^ This is !collection.contains(other). Do we already rewrite variations on this? } diff --git a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/CollectionRulesTestInput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/CollectionRulesTestInput.java index 90dc68981c..fd685890f0 100644 --- a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/CollectionRulesTestInput.java +++ b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/CollectionRulesTestInput.java @@ -5,6 +5,7 @@ import com.google.common.collect.ImmutableSortedSet; import com.google.common.collect.Iterables; import com.google.common.collect.Lists; +import com.google.common.collect.Streams; import java.util.ArrayList; import java.util.Arrays; import java.util.HashSet; @@ -19,7 +20,7 @@ final class CollectionRulesTest implements RefasterRuleCollectionTestCase { @Override public ImmutableSet elidedTypesAndStaticImports() { - return ImmutableSet.of(Iterables.class, Lists.class); + return ImmutableSet.of(Iterables.class, Lists.class, Streams.class); } ImmutableSet testCollectionIsEmpty() { @@ -130,14 +131,18 @@ ImmutableSet> testOptionalFirstCollectionElement() { ImmutableSet.of(1).isEmpty() ? Optional.empty() : Optional.of(ImmutableSet.of(1).iterator().next()), - ImmutableList.of(2).isEmpty() ? Optional.empty() : Optional.of(ImmutableList.of(2).get(0)), + ImmutableList.of(2).isEmpty() + ? Optional.empty() + : Optional.of(ImmutableList.of(2).getFirst()), ImmutableSortedSet.of(3).isEmpty() ? Optional.empty() : Optional.of(ImmutableSortedSet.of(3).first()), !ImmutableSet.of(1).isEmpty() ? Optional.of(ImmutableSet.of(1).iterator().next()) : Optional.empty(), - !ImmutableList.of(2).isEmpty() ? Optional.of(ImmutableList.of(2).get(0)) : Optional.empty(), + !ImmutableList.of(2).isEmpty() + ? Optional.of(ImmutableList.of(2).getFirst()) + : Optional.empty(), !ImmutableSortedSet.of(3).isEmpty() ? Optional.of(ImmutableSortedSet.of(3).first()) : Optional.empty()); @@ -207,4 +212,45 @@ ImmutableSet> testRemoveOptionalFirstQueueElement() { void testCollectionForEach() { ImmutableSet.of(1).stream().forEach(String::valueOf); } + + String testCollectionIteratorNext() { + return ImmutableSet.of("foo").stream().findFirst().orElseThrow(); + } + + ImmutableSet testSequencedCollectionGetFirst() { + return ImmutableSet.of( + ImmutableList.of("foo").iterator().next(), ImmutableList.of("bar").get(0)); + } + + ImmutableSet testSequencedCollectionGetLast() { + return ImmutableSet.of( + ImmutableList.of("foo").reversed().getFirst(), + Streams.findLast(ImmutableList.of("bar").stream()).orElseThrow(), + ImmutableList.of("baz").get(ImmutableList.of("baz").size() - 1)); + } + + void testListAddFirst() { + new ArrayList().add(0, "foo"); + } + + void testListAdd() { + new ArrayList(0).addLast("bar"); + new ArrayList(1).add(new ArrayList(1).size(), "qux"); + } + + String testListRemoveFirst() { + return new ArrayList().remove(0); + } + + String testListRemoveLast() { + return new ArrayList().remove(new ArrayList().size() - 1); + } + + String testSortedSetFirst() { + return ImmutableSortedSet.of("foo").getFirst(); + } + + String testSortedSetLast() { + return ImmutableSortedSet.of("foo").getLast(); + } } diff --git a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/CollectionRulesTestOutput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/CollectionRulesTestOutput.java index b35493ba83..c574fa84c4 100644 --- a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/CollectionRulesTestOutput.java +++ b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/CollectionRulesTestOutput.java @@ -5,6 +5,7 @@ import com.google.common.collect.ImmutableSortedSet; import com.google.common.collect.Iterables; import com.google.common.collect.Lists; +import com.google.common.collect.Streams; import java.util.ArrayList; import java.util.Arrays; import java.util.HashSet; @@ -19,7 +20,7 @@ final class CollectionRulesTest implements RefasterRuleCollectionTestCase { @Override public ImmutableSet elidedTypesAndStaticImports() { - return ImmutableSet.of(Iterables.class, Lists.class); + return ImmutableSet.of(Iterables.class, Lists.class, Streams.class); } ImmutableSet testCollectionIsEmpty() { @@ -156,4 +157,44 @@ ImmutableSet> testRemoveOptionalFirstQueueElement() { void testCollectionForEach() { ImmutableSet.of(1).forEach(String::valueOf); } + + String testCollectionIteratorNext() { + return ImmutableSet.of("foo").iterator().next(); + } + + ImmutableSet testSequencedCollectionGetFirst() { + return ImmutableSet.of(ImmutableList.of("foo").getFirst(), ImmutableList.of("bar").getFirst()); + } + + ImmutableSet testSequencedCollectionGetLast() { + return ImmutableSet.of( + ImmutableList.of("foo").getLast(), + ImmutableList.of("bar").getLast(), + ImmutableList.of("baz").getLast()); + } + + void testListAddFirst() { + new ArrayList().addFirst("foo"); + } + + void testListAdd() { + new ArrayList(0).add("bar"); + new ArrayList(1).add("qux"); + } + + String testListRemoveFirst() { + return new ArrayList().removeFirst(); + } + + String testListRemoveLast() { + return new ArrayList().removeLast(); + } + + String testSortedSetFirst() { + return ImmutableSortedSet.of("foo").first(); + } + + String testSortedSetLast() { + return ImmutableSortedSet.of("foo").last(); + } }