diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ComparatorRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ComparatorRules.java index 965044902d1..50041013d10 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ComparatorRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ComparatorRules.java @@ -7,7 +7,6 @@ import static java.util.Comparator.comparingLong; import static java.util.Comparator.naturalOrder; import static java.util.Comparator.reverseOrder; -import static java.util.function.Function.identity; import com.google.common.collect.Comparators; import com.google.common.collect.ImmutableList; @@ -16,6 +15,7 @@ import com.google.errorprone.refaster.Refaster; import com.google.errorprone.refaster.annotation.AfterTemplate; import com.google.errorprone.refaster.annotation.BeforeTemplate; +import com.google.errorprone.refaster.annotation.Matches; import com.google.errorprone.refaster.annotation.Repeated; import com.google.errorprone.refaster.annotation.UseImportPolicy; import java.util.Arrays; @@ -28,6 +28,7 @@ import java.util.function.ToLongFunction; import java.util.stream.Stream; import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; +import tech.picnic.errorprone.refaster.matchers.IsIdentityOperation; /** Refaster rules related to expressions dealing with {@link Comparator}s. */ @OnlineDocumentation @@ -36,12 +37,12 @@ private ComparatorRules() {} /** Prefer {@link Comparator#naturalOrder()} over more complicated constructs. */ static final class NaturalOrder> { - // XXX: Drop the `Refaster.anyOf` if/when we decide to rewrite one to the other. @BeforeTemplate - Comparator before() { + Comparator before( + @Matches(IsIdentityOperation.class) Function keyExtractor) { return Refaster.anyOf( T::compareTo, - comparing(Refaster.anyOf(identity(), v -> v)), + comparing(keyExtractor), Collections.reverseOrder(reverseOrder()), Comparator.reverseOrder().reversed()); } @@ -72,10 +73,11 @@ Comparator after() { } static final class CustomComparator { - // XXX: Drop the `Refaster.anyOf` if/when we decide to rewrite one to the other. @BeforeTemplate - Comparator before(Comparator cmp) { - return comparing(Refaster.anyOf(identity(), v -> v), cmp); + Comparator before( + Comparator cmp, + @Matches(IsIdentityOperation.class) Function keyExtractor) { + return comparing(keyExtractor, cmp); } @AfterTemplate @@ -187,10 +189,11 @@ Comparator after(Comparator cmp, ToLongFunction function) { * it more clearly states intent. */ static final class ThenComparingNaturalOrder> { - // XXX: Drop the `Refaster.anyOf` if/when we decide to rewrite one to the other. @BeforeTemplate - Comparator before(Comparator cmp) { - return cmp.thenComparing(Refaster.anyOf(identity(), v -> v)); + Comparator before( + Comparator cmp, + @Matches(IsIdentityOperation.class) Function keyExtractor) { + return cmp.thenComparing(keyExtractor); } @AfterTemplate diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableListMultimapRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableListMultimapRules.java index b68a7a16018..3a943ab86c5 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableListMultimapRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableListMultimapRules.java @@ -3,7 +3,6 @@ import static com.google.common.collect.ImmutableListMultimap.flatteningToImmutableListMultimap; import static com.google.common.collect.ImmutableListMultimap.toImmutableListMultimap; import static com.google.errorprone.refaster.ImportPolicy.STATIC_IMPORT_ALWAYS; -import static java.util.function.Function.identity; import com.google.common.collect.ImmutableListMultimap; import com.google.common.collect.ImmutableMultimap; @@ -16,6 +15,7 @@ import com.google.errorprone.refaster.Refaster; import com.google.errorprone.refaster.annotation.AfterTemplate; import com.google.errorprone.refaster.annotation.BeforeTemplate; +import com.google.errorprone.refaster.annotation.Matches; import com.google.errorprone.refaster.annotation.MayOptionallyUse; import com.google.errorprone.refaster.annotation.Placeholder; import com.google.errorprone.refaster.annotation.UseImportPolicy; @@ -25,6 +25,7 @@ import java.util.function.Function; import java.util.stream.Stream; import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; +import tech.picnic.errorprone.refaster.matchers.IsIdentityOperation; /** Refaster rules related to expressions dealing with {@link ImmutableListMultimap}s. */ @OnlineDocumentation @@ -173,27 +174,29 @@ ImmutableListMultimap after(Stream stream) { * Prefer {@link Multimaps#index(Iterable, com.google.common.base.Function)} over the stream-based * alternative. */ - // XXX: Drop the `Refaster.anyOf` if/when we decide to rewrite one to the other. static final class IndexIterableToImmutableListMultimap { @BeforeTemplate ImmutableListMultimap before( - Iterator iterable, Function keyFunction) { - return Streams.stream(iterable) - .collect(toImmutableListMultimap(keyFunction, Refaster.anyOf(identity(), v -> v))); + Iterator iterable, + Function keyFunction, + @Matches(IsIdentityOperation.class) Function valueFunction) { + return Streams.stream(iterable).collect(toImmutableListMultimap(keyFunction, valueFunction)); } @BeforeTemplate ImmutableListMultimap before( - Iterable iterable, Function keyFunction) { - return Streams.stream(iterable) - .collect(toImmutableListMultimap(keyFunction, Refaster.anyOf(identity(), v -> v))); + Iterable iterable, + Function keyFunction, + @Matches(IsIdentityOperation.class) Function valueFunction) { + return Streams.stream(iterable).collect(toImmutableListMultimap(keyFunction, valueFunction)); } @BeforeTemplate ImmutableListMultimap before( - Collection iterable, Function keyFunction) { - return iterable.stream() - .collect(toImmutableListMultimap(keyFunction, Refaster.anyOf(identity(), v -> v))); + Collection iterable, + Function keyFunction, + @Matches(IsIdentityOperation.class) Function valueFunction) { + return iterable.stream().collect(toImmutableListMultimap(keyFunction, valueFunction)); } @AfterTemplate diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableMapRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableMapRules.java index 49021f1a5ad..8c0f9c62674 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableMapRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableMapRules.java @@ -4,7 +4,6 @@ import static com.google.errorprone.refaster.ImportPolicy.STATIC_IMPORT_ALWAYS; import static java.util.Collections.emptyMap; import static java.util.Collections.singletonMap; -import static java.util.function.Function.identity; import com.google.common.base.Predicate; import com.google.common.collect.ImmutableMap; @@ -13,6 +12,7 @@ import com.google.errorprone.refaster.Refaster; import com.google.errorprone.refaster.annotation.AfterTemplate; import com.google.errorprone.refaster.annotation.BeforeTemplate; +import com.google.errorprone.refaster.annotation.Matches; import com.google.errorprone.refaster.annotation.MayOptionallyUse; import com.google.errorprone.refaster.annotation.Placeholder; import com.google.errorprone.refaster.annotation.UseImportPolicy; @@ -23,6 +23,7 @@ import java.util.function.Function; import java.util.stream.Stream; import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; +import tech.picnic.errorprone.refaster.matchers.IsIdentityOperation; /** Refaster rules related to expressions dealing with {@link ImmutableMap}s. */ @OnlineDocumentation @@ -63,27 +64,29 @@ ImmutableMap after(Map.Entry entry) { * Prefer {@link Maps#toMap(Iterable, com.google.common.base.Function)} over more contrived * alternatives. */ - // XXX: Drop the `Refaster.anyOf` if/when we decide to rewrite one to the other. static final class IterableToImmutableMap { @BeforeTemplate ImmutableMap before( - Iterator iterable, Function valueFunction) { - return Streams.stream(iterable) - .collect(toImmutableMap(Refaster.anyOf(identity(), k -> k), valueFunction)); + Iterator iterable, + Function valueFunction, + @Matches(IsIdentityOperation.class) Function keyFunction) { + return Streams.stream(iterable).collect(toImmutableMap(keyFunction, valueFunction)); } @BeforeTemplate ImmutableMap before( - Iterable iterable, Function valueFunction) { - return Streams.stream(iterable) - .collect(toImmutableMap(Refaster.anyOf(identity(), k -> k), valueFunction)); + Iterable iterable, + Function valueFunction, + @Matches(IsIdentityOperation.class) Function keyFunction) { + return Streams.stream(iterable).collect(toImmutableMap(keyFunction, valueFunction)); } @BeforeTemplate ImmutableMap before( - Collection iterable, Function valueFunction) { - return iterable.stream() - .collect(toImmutableMap(Refaster.anyOf(identity(), k -> k), valueFunction)); + Collection iterable, + Function valueFunction, + @Matches(IsIdentityOperation.class) Function keyFunction) { + return iterable.stream().collect(toImmutableMap(keyFunction, valueFunction)); } @BeforeTemplate @@ -157,25 +160,29 @@ ImmutableMap after(Stream stream) { * Prefer {@link Maps#uniqueIndex(Iterable, com.google.common.base.Function)} over the * stream-based alternative. */ - // XXX: Drop the `Refaster.anyOf` if/when we decide to rewrite one to the other. static final class IndexIterableToImmutableMap { @BeforeTemplate - ImmutableMap before(Iterator iterable, Function keyFunction) { - return Streams.stream(iterable) - .collect(toImmutableMap(keyFunction, Refaster.anyOf(identity(), v -> v))); + ImmutableMap before( + Iterator iterable, + Function keyFunction, + @Matches(IsIdentityOperation.class) Function valueFunction) { + return Streams.stream(iterable).collect(toImmutableMap(keyFunction, valueFunction)); } @BeforeTemplate - ImmutableMap before(Iterable iterable, Function keyFunction) { - return Streams.stream(iterable) - .collect(toImmutableMap(keyFunction, Refaster.anyOf(identity(), v -> v))); + ImmutableMap before( + Iterable iterable, + Function keyFunction, + @Matches(IsIdentityOperation.class) Function valueFunction) { + return Streams.stream(iterable).collect(toImmutableMap(keyFunction, valueFunction)); } @BeforeTemplate ImmutableMap before( - Collection iterable, Function keyFunction) { - return iterable.stream() - .collect(toImmutableMap(keyFunction, Refaster.anyOf(identity(), v -> v))); + Collection iterable, + Function keyFunction, + @Matches(IsIdentityOperation.class) Function valueFunction) { + return iterable.stream().collect(toImmutableMap(keyFunction, valueFunction)); } @AfterTemplate diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java index 5653673bca0..2b81605b991 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java @@ -18,6 +18,7 @@ import com.google.errorprone.refaster.Refaster; import com.google.errorprone.refaster.annotation.AfterTemplate; import com.google.errorprone.refaster.annotation.BeforeTemplate; +import com.google.errorprone.refaster.annotation.Matches; import com.google.errorprone.refaster.annotation.MayOptionallyUse; import com.google.errorprone.refaster.annotation.NotMatches; import com.google.errorprone.refaster.annotation.Placeholder; @@ -48,6 +49,7 @@ import tech.picnic.errorprone.refaster.annotation.Description; import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; import tech.picnic.errorprone.refaster.annotation.Severity; +import tech.picnic.errorprone.refaster.matchers.IsIdentityOperation; import tech.picnic.errorprone.refaster.matchers.ThrowsCheckedException; /** Refaster rules related to Reactor expressions and statements. */ @@ -405,7 +407,8 @@ Mono before(Mono mono) { } // XXX: Replace this rule with an extension of the `IdentityConversion` rule, supporting - // `Stream#map`, `Mono#map` and `Flux#map`. + // `Stream#map`, `Mono#map` and `Flux#map`. Alternatively, extend the `IsIdentityOperation` + // matcher and use it to constrain the matched `map` argument. @BeforeTemplate Mono> before3(Mono> mono) { return mono.map(ImmutableList::copyOf); @@ -446,41 +449,44 @@ Flux after(Flux flux) { } /** Prefer {@link Flux#concatMap(Function)} over more contrived alternatives. */ - static final class FluxConcatMap { + static final class FluxConcatMap> { @BeforeTemplate @SuppressWarnings("NestedPublishers") - Flux before(Flux flux, Function> function) { + Flux before( + Flux flux, + Function function, + @Matches(IsIdentityOperation.class) + Function> identityOperation) { return Refaster.anyOf( flux.flatMap(function, 1), flux.flatMapSequential(function, 1), - flux.map(function).concatMap(identity())); + flux.map(function).concatMap(identityOperation)); } @AfterTemplate - Flux after(Flux flux, Function> function) { + Flux after(Flux flux, Function function) { return flux.concatMap(function); } } /** Prefer {@link Flux#concatMap(Function, int)} over more contrived alternatives. */ - static final class FluxConcatMapWithPrefetch { + static final class FluxConcatMapWithPrefetch> { @BeforeTemplate @SuppressWarnings("NestedPublishers") Flux before( Flux flux, - Function> function, - int prefetch) { + Function function, + int prefetch, + @Matches(IsIdentityOperation.class) + Function> identityOperation) { return Refaster.anyOf( flux.flatMap(function, 1, prefetch), flux.flatMapSequential(function, 1, prefetch), - flux.map(function).concatMap(identity(), prefetch)); + flux.map(function).concatMap(identityOperation, prefetch)); } @AfterTemplate - Flux after( - Flux flux, - Function> function, - int prefetch) { + Flux after(Flux flux, Function function, int prefetch) { return flux.concatMap(function, prefetch); } } @@ -795,29 +801,37 @@ Flux after(Flux flux) { } /** Prefer {@link Mono#flatMap(Function)} over more contrived alternatives. */ - static final class MonoFlatMap { + static final class MonoFlatMap> { @BeforeTemplate @SuppressWarnings("NestedPublishers") - Mono before(Mono mono, Function> function) { - return mono.map(function).flatMap(identity()); + Mono before( + Mono mono, + Function function, + @Matches(IsIdentityOperation.class) + Function> identityOperation) { + return mono.map(function).flatMap(identityOperation); } @AfterTemplate - Mono after(Mono mono, Function> function) { + Mono after(Mono mono, Function function) { return mono.flatMap(function); } } /** Prefer {@link Mono#flatMapMany(Function)} over more contrived alternatives. */ - static final class MonoFlatMapMany { + static final class MonoFlatMapMany> { @BeforeTemplate @SuppressWarnings("NestedPublishers") - Flux before(Mono mono, Function> function) { - return mono.map(function).flatMapMany(identity()); + Flux before( + Mono mono, + Function function, + @Matches(IsIdentityOperation.class) + Function> identityOperation) { + return mono.map(function).flatMapMany(identityOperation); } @AfterTemplate - Flux after(Mono mono, Function> function) { + Flux after(Mono mono, Function function) { return mono.flatMapMany(function); } } diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/StreamRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/StreamRules.java index fad5205868b..62c4afdf274 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/StreamRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/StreamRules.java @@ -3,7 +3,6 @@ import static com.google.errorprone.refaster.ImportPolicy.STATIC_IMPORT_ALWAYS; import static java.util.Comparator.naturalOrder; import static java.util.Comparator.reverseOrder; -import static java.util.function.Function.identity; import static java.util.function.Predicate.not; import static java.util.stream.Collectors.counting; import static java.util.stream.Collectors.filtering; @@ -49,6 +48,7 @@ import java.util.stream.Collectors; import java.util.stream.Stream; import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; +import tech.picnic.errorprone.refaster.matchers.IsIdentityOperation; import tech.picnic.errorprone.refaster.matchers.IsLambdaExpressionOrMethodReference; import tech.picnic.errorprone.refaster.matchers.IsRefasterAsVarargs; @@ -630,8 +630,11 @@ R after( static final class StreamsConcat { @BeforeTemplate - Stream before(@Repeated Stream stream) { - return Stream.of(Refaster.asVarargs(stream)).flatMap(Refaster.anyOf(identity(), s -> s)); + Stream before( + @Repeated Stream stream, + @Matches(IsIdentityOperation.class) + Function, ? extends Stream> mapper) { + return Stream.of(Refaster.asVarargs(stream)).flatMap(mapper); } @AfterTemplate diff --git a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ComparatorRulesTestInput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ComparatorRulesTestInput.java index 52ff1c3b6f1..b20174934f0 100644 --- a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ComparatorRulesTestInput.java +++ b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ComparatorRulesTestInput.java @@ -31,6 +31,7 @@ ImmutableSet> testNaturalOrder() { String::compareTo, Comparator.comparing(identity()), Comparator.comparing(s -> s), + Comparator.comparing(s -> 0), Collections.reverseOrder(reverseOrder()), Comparator.reverseOrder().reversed()); } @@ -45,7 +46,8 @@ ImmutableSet> testReverseOrder() { ImmutableSet> testCustomComparator() { return ImmutableSet.of( Comparator.comparing(identity(), Comparator.comparingInt(String::length)), - Comparator.comparing(s -> s, Comparator.comparingInt(String::length))); + Comparator.comparing(s -> s, Comparator.comparingInt(String::length)), + Comparator.comparing(s -> "foo", Comparator.comparingInt(String::length))); } Comparator testThenComparing() { @@ -86,7 +88,8 @@ Comparator testThenComparingLong() { ImmutableSet> testThenComparingNaturalOrder() { return ImmutableSet.of( Comparator.naturalOrder().thenComparing(identity()), - Comparator.naturalOrder().thenComparing(s -> s)); + Comparator.naturalOrder().thenComparing(s -> s), + Comparator.naturalOrder().thenComparing(s -> 0)); } ImmutableSet testCompareTo() { diff --git a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ComparatorRulesTestOutput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ComparatorRulesTestOutput.java index 7c3662bb134..088b2f01a91 100644 --- a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ComparatorRulesTestOutput.java +++ b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ComparatorRulesTestOutput.java @@ -28,7 +28,12 @@ public ImmutableSet elidedTypesAndStaticImports() { ImmutableSet> testNaturalOrder() { return ImmutableSet.of( - naturalOrder(), naturalOrder(), naturalOrder(), naturalOrder(), naturalOrder()); + naturalOrder(), + naturalOrder(), + naturalOrder(), + Comparator.comparing(s -> 0), + naturalOrder(), + naturalOrder()); } ImmutableSet> testReverseOrder() { @@ -38,7 +43,9 @@ ImmutableSet> testReverseOrder() { ImmutableSet> testCustomComparator() { return ImmutableSet.of( - Comparator.comparingInt(String::length), Comparator.comparingInt(String::length)); + Comparator.comparingInt(String::length), + Comparator.comparingInt(String::length), + Comparator.comparing(s -> "foo", Comparator.comparingInt(String::length))); } Comparator testThenComparing() { @@ -73,7 +80,8 @@ Comparator testThenComparingLong() { ImmutableSet> testThenComparingNaturalOrder() { return ImmutableSet.of( Comparator.naturalOrder().thenComparing(naturalOrder()), - Comparator.naturalOrder().thenComparing(naturalOrder())); + Comparator.naturalOrder().thenComparing(naturalOrder()), + Comparator.naturalOrder().thenComparing(s -> 0)); } ImmutableSet testCompareTo() { diff --git a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableListMultimapRulesTestInput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableListMultimapRulesTestInput.java index 6ad5befdeb9..27b231042c6 100644 --- a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableListMultimapRulesTestInput.java +++ b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableListMultimapRulesTestInput.java @@ -76,11 +76,22 @@ ImmutableListMultimap testStreamOfMapEntriesToImmutableListMult ImmutableSet> testIndexIterableToImmutableListMultimap() { return ImmutableSet.of( - ImmutableList.of(1).stream().collect(toImmutableListMultimap(n -> n * 2, identity())), - Streams.stream(ImmutableList.of(2)::iterator) - .collect(toImmutableListMultimap(Integer::valueOf, n -> n)), + Streams.stream(ImmutableList.of(1).iterator()) + .collect(toImmutableListMultimap(n -> n * 2, identity())), + Streams.stream(ImmutableList.of(2).iterator()) + .collect(toImmutableListMultimap(n -> n * 2, v -> v)), Streams.stream(ImmutableList.of(3).iterator()) - .collect(toImmutableListMultimap(n -> n.intValue(), identity()))); + .collect(toImmutableListMultimap(n -> n * 2, v -> 0)), + Streams.stream(ImmutableList.of(4)::iterator) + .collect(toImmutableListMultimap(Integer::valueOf, identity())), + Streams.stream(ImmutableList.of(5)::iterator) + .collect(toImmutableListMultimap(Integer::valueOf, v -> v)), + Streams.stream(ImmutableList.of(6)::iterator) + .collect(toImmutableListMultimap(Integer::valueOf, v -> 0)), + ImmutableList.of(7).stream() + .collect(toImmutableListMultimap(n -> n.intValue(), identity())), + ImmutableList.of(8).stream().collect(toImmutableListMultimap(n -> n.intValue(), v -> v)), + ImmutableList.of(9).stream().collect(toImmutableListMultimap(n -> n.intValue(), v -> 0))); } ImmutableListMultimap testTransformMultimapValuesToImmutableListMultimap() { diff --git a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableListMultimapRulesTestOutput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableListMultimapRulesTestOutput.java index c4f5274e96d..ca4dbdfd3be 100644 --- a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableListMultimapRulesTestOutput.java +++ b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableListMultimapRulesTestOutput.java @@ -64,9 +64,17 @@ ImmutableListMultimap testStreamOfMapEntriesToImmutableListMult ImmutableSet> testIndexIterableToImmutableListMultimap() { return ImmutableSet.of( - Multimaps.index(ImmutableList.of(1), n -> n * 2), - Multimaps.index(ImmutableList.of(2)::iterator, Integer::valueOf), - Multimaps.index(ImmutableList.of(3).iterator(), n -> n.intValue())); + Multimaps.index(ImmutableList.of(1).iterator(), n -> n * 2), + Multimaps.index(ImmutableList.of(2).iterator(), n -> n * 2), + Streams.stream(ImmutableList.of(3).iterator()) + .collect(toImmutableListMultimap(n -> n * 2, v -> 0)), + Multimaps.index(ImmutableList.of(4)::iterator, Integer::valueOf), + Multimaps.index(ImmutableList.of(5)::iterator, Integer::valueOf), + Streams.stream(ImmutableList.of(6)::iterator) + .collect(toImmutableListMultimap(Integer::valueOf, v -> 0)), + Multimaps.index(ImmutableList.of(7), n -> n.intValue()), + Multimaps.index(ImmutableList.of(8), n -> n.intValue()), + ImmutableList.of(9).stream().collect(toImmutableListMultimap(n -> n.intValue(), v -> 0))); } ImmutableListMultimap testTransformMultimapValuesToImmutableListMultimap() { diff --git a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableMapRulesTestInput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableMapRulesTestInput.java index aab420c62ca..2302bb4c9ea 100644 --- a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableMapRulesTestInput.java +++ b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableMapRulesTestInput.java @@ -34,11 +34,21 @@ ImmutableSet> testEntryToImmutableMap() { ImmutableSet> testIterableToImmutableMap() { return ImmutableSet.of( ImmutableList.of(1).stream().collect(toImmutableMap(identity(), n -> n * 2)), - Streams.stream(ImmutableList.of(2)::iterator) - .collect(toImmutableMap(n -> n, Integer::valueOf)), - Streams.stream(ImmutableList.of(3).iterator()) + ImmutableList.of(2).stream().collect(toImmutableMap(k -> k, n -> n * 2)), + ImmutableList.of(3).stream().collect(toImmutableMap(k -> 0, n -> n * 2)), + Streams.stream(ImmutableList.of(4)::iterator) + .collect(toImmutableMap(identity(), Integer::valueOf)), + Streams.stream(ImmutableList.of(5)::iterator) + .collect(toImmutableMap(k -> k, Integer::valueOf)), + Streams.stream(ImmutableList.of(6)::iterator) + .collect(toImmutableMap(k -> 0, Integer::valueOf)), + Streams.stream(ImmutableList.of(7).iterator()) .collect(toImmutableMap(identity(), n -> n.intValue())), - ImmutableMap.copyOf(Maps.asMap(ImmutableSet.of(4), Integer::valueOf))); + Streams.stream(ImmutableList.of(8).iterator()) + .collect(toImmutableMap(k -> k, n -> n.intValue())), + Streams.stream(ImmutableList.of(9).iterator()) + .collect(toImmutableMap(k -> 0, n -> n.intValue())), + ImmutableMap.copyOf(Maps.asMap(ImmutableSet.of(10), Integer::valueOf))); } ImmutableSet> testEntryIterableToImmutableMap() { @@ -63,10 +73,20 @@ ImmutableMap testStreamOfMapEntriesToImmutableMap() { ImmutableSet> testIndexIterableToImmutableMap() { return ImmutableSet.of( ImmutableList.of(1).stream().collect(toImmutableMap(n -> n * 2, identity())), - Streams.stream(ImmutableList.of(2)::iterator) - .collect(toImmutableMap(Integer::valueOf, n -> n)), - Streams.stream(ImmutableList.of(3).iterator()) - .collect(toImmutableMap(n -> n.intValue(), identity()))); + ImmutableList.of(2).stream().collect(toImmutableMap(n -> n * 2, v -> v)), + ImmutableList.of(3).stream().collect(toImmutableMap(n -> n * 2, v -> 0)), + Streams.stream(ImmutableList.of(4)::iterator) + .collect(toImmutableMap(Integer::valueOf, identity())), + Streams.stream(ImmutableList.of(5)::iterator) + .collect(toImmutableMap(Integer::valueOf, v -> v)), + Streams.stream(ImmutableList.of(6)::iterator) + .collect(toImmutableMap(Integer::valueOf, v -> 0)), + Streams.stream(ImmutableList.of(7).iterator()) + .collect(toImmutableMap(n -> n.intValue(), identity())), + Streams.stream(ImmutableList.of(8).iterator()) + .collect(toImmutableMap(n -> n.intValue(), v -> v)), + Streams.stream(ImmutableList.of(9).iterator()) + .collect(toImmutableMap(n -> n.intValue(), v -> 0))); } ImmutableSet> testTransformMapValuesToImmutableMap() { diff --git a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableMapRulesTestOutput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableMapRulesTestOutput.java index e4bfabe76c7..3365f3fd597 100644 --- a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableMapRulesTestOutput.java +++ b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableMapRulesTestOutput.java @@ -33,9 +33,17 @@ ImmutableSet> testEntryToImmutableMap() { ImmutableSet> testIterableToImmutableMap() { return ImmutableSet.of( Maps.toMap(ImmutableList.of(1), n -> n * 2), - Maps.toMap(ImmutableList.of(2)::iterator, Integer::valueOf), - Maps.toMap(ImmutableList.of(3).iterator(), n -> n.intValue()), - Maps.toMap(ImmutableSet.of(4), Integer::valueOf)); + Maps.toMap(ImmutableList.of(2), n -> n * 2), + ImmutableList.of(3).stream().collect(toImmutableMap(k -> 0, n -> n * 2)), + Maps.toMap(ImmutableList.of(4)::iterator, Integer::valueOf), + Maps.toMap(ImmutableList.of(5)::iterator, Integer::valueOf), + Streams.stream(ImmutableList.of(6)::iterator) + .collect(toImmutableMap(k -> 0, Integer::valueOf)), + Maps.toMap(ImmutableList.of(7).iterator(), n -> n.intValue()), + Maps.toMap(ImmutableList.of(8).iterator(), n -> n.intValue()), + Streams.stream(ImmutableList.of(9).iterator()) + .collect(toImmutableMap(k -> 0, n -> n.intValue())), + Maps.toMap(ImmutableSet.of(10), Integer::valueOf)); } ImmutableSet> testEntryIterableToImmutableMap() { @@ -54,8 +62,16 @@ ImmutableMap testStreamOfMapEntriesToImmutableMap() { ImmutableSet> testIndexIterableToImmutableMap() { return ImmutableSet.of( Maps.uniqueIndex(ImmutableList.of(1), n -> n * 2), - Maps.uniqueIndex(ImmutableList.of(2)::iterator, Integer::valueOf), - Maps.uniqueIndex(ImmutableList.of(3).iterator(), n -> n.intValue())); + Maps.uniqueIndex(ImmutableList.of(2), n -> n * 2), + ImmutableList.of(3).stream().collect(toImmutableMap(n -> n * 2, v -> 0)), + Maps.uniqueIndex(ImmutableList.of(4)::iterator, Integer::valueOf), + Maps.uniqueIndex(ImmutableList.of(5)::iterator, Integer::valueOf), + Streams.stream(ImmutableList.of(6)::iterator) + .collect(toImmutableMap(Integer::valueOf, v -> 0)), + Maps.uniqueIndex(ImmutableList.of(7).iterator(), n -> n.intValue()), + Maps.uniqueIndex(ImmutableList.of(8).iterator(), n -> n.intValue()), + Streams.stream(ImmutableList.of(9).iterator()) + .collect(toImmutableMap(n -> n.intValue(), v -> 0))); } ImmutableSet> testTransformMapValuesToImmutableMap() { diff --git a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestInput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestInput.java index 1317776baaa..84be06fc157 100644 --- a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestInput.java +++ b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestInput.java @@ -158,14 +158,18 @@ ImmutableSet> testFluxConcatMap() { return ImmutableSet.of( Flux.just(1).flatMap(Mono::just, 1), Flux.just(2).flatMapSequential(Mono::just, 1), - Flux.just(3).map(Mono::just).concatMap(identity())); + Flux.just(3).map(Mono::just).concatMap(identity()), + Flux.just(4).map(Mono::just).concatMap(v -> v), + Flux.just(5).map(Mono::just).concatMap(v -> Mono.empty())); } ImmutableSet> testFluxConcatMapWithPrefetch() { return ImmutableSet.of( Flux.just(1).flatMap(Mono::just, 1, 3), Flux.just(2).flatMapSequential(Mono::just, 1, 4), - Flux.just(3).map(Mono::just).concatMap(identity(), 5)); + Flux.just(3).map(Mono::just).concatMap(identity(), 5), + Flux.just(4).map(Mono::just).concatMap(v -> v, 6), + Flux.just(5).map(Mono::just).concatMap(v -> Mono.empty(), 7)); } Flux testFluxConcatMapIterable() { @@ -269,12 +273,18 @@ Flux testFluxCast() { return Flux.just(1).map(Number.class::cast); } - Mono testMonoFlatMap() { - return Mono.just("foo").map(Mono::just).flatMap(identity()); + ImmutableSet> testMonoFlatMap() { + return ImmutableSet.of( + Mono.just("foo").map(Mono::just).flatMap(identity()), + Mono.just("bar").map(Mono::just).flatMap(v -> v), + Mono.just("baz").map(Mono::just).flatMap(v -> Mono.empty())); } - Flux testMonoFlatMapMany() { - return Mono.just("foo").map(Mono::just).flatMapMany(identity()); + ImmutableSet> testMonoFlatMapMany() { + return ImmutableSet.of( + Mono.just("foo").map(Flux::just).flatMapMany(identity()), + Mono.just("bar").map(Flux::just).flatMapMany(v -> v), + Mono.just("baz").map(Flux::just).flatMapMany(v -> Flux.empty())); } ImmutableSet> testConcatMapIterableIdentity() { diff --git a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestOutput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestOutput.java index d52b8dd1806..56df255b247 100644 --- a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestOutput.java +++ b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestOutput.java @@ -162,14 +162,18 @@ ImmutableSet> testFluxConcatMap() { return ImmutableSet.of( Flux.just(1).concatMap(Mono::just), Flux.just(2).concatMap(Mono::just), - Flux.just(3).concatMap(Mono::just)); + Flux.just(3).concatMap(Mono::just), + Flux.just(4).concatMap(Mono::just), + Flux.just(5).map(Mono::just).concatMap(v -> Mono.empty())); } ImmutableSet> testFluxConcatMapWithPrefetch() { return ImmutableSet.of( Flux.just(1).concatMap(Mono::just, 3), Flux.just(2).concatMap(Mono::just, 4), - Flux.just(3).concatMap(Mono::just, 5)); + Flux.just(3).concatMap(Mono::just, 5), + Flux.just(4).concatMap(Mono::just, 6), + Flux.just(5).map(Mono::just).concatMap(v -> Mono.empty(), 7)); } Flux testFluxConcatMapIterable() { @@ -266,12 +270,18 @@ Flux testFluxCast() { return Flux.just(1).cast(Number.class); } - Mono testMonoFlatMap() { - return Mono.just("foo").flatMap(Mono::just); + ImmutableSet> testMonoFlatMap() { + return ImmutableSet.of( + Mono.just("foo").flatMap(Mono::just), + Mono.just("bar").flatMap(Mono::just), + Mono.just("baz").map(Mono::just).flatMap(v -> Mono.empty())); } - Flux testMonoFlatMapMany() { - return Mono.just("foo").flatMapMany(Mono::just); + ImmutableSet> testMonoFlatMapMany() { + return ImmutableSet.of( + Mono.just("foo").flatMapMany(Flux::just), + Mono.just("bar").flatMapMany(Flux::just), + Mono.just("baz").map(Flux::just).flatMapMany(v -> Flux.empty())); } ImmutableSet> testConcatMapIterableIdentity() { diff --git a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/StreamRulesTestInput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/StreamRulesTestInput.java index 370c76c2af6..67edc58dd91 100644 --- a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/StreamRulesTestInput.java +++ b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/StreamRulesTestInput.java @@ -256,7 +256,8 @@ ImmutableSet testStreamFlatMapCollect() { ImmutableSet> testStreamsConcat() { return ImmutableSet.of( Stream.of(Stream.of(1), Stream.of(2)).flatMap(identity()), - Stream.of(Stream.of(3), Stream.of(4)).flatMap(v -> v)); + Stream.of(Stream.of(3), Stream.of(4)).flatMap(v -> v), + Stream.of(Stream.of(5), Stream.of(6)).flatMap(v -> Stream.empty())); } Stream testStreamTakeWhile() { diff --git a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/StreamRulesTestOutput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/StreamRulesTestOutput.java index 4683df8f0d3..6766f8cb057 100644 --- a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/StreamRulesTestOutput.java +++ b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/StreamRulesTestOutput.java @@ -256,7 +256,9 @@ ImmutableSet testStreamFlatMapCollect() { ImmutableSet> testStreamsConcat() { return ImmutableSet.of( - Streams.concat(Stream.of(1), Stream.of(2)), Streams.concat(Stream.of(3), Stream.of(4))); + Streams.concat(Stream.of(1), Stream.of(2)), + Streams.concat(Stream.of(3), Stream.of(4)), + Stream.of(Stream.of(5), Stream.of(6)).flatMap(v -> Stream.empty())); } Stream testStreamTakeWhile() { diff --git a/refaster-support/src/main/java/tech/picnic/errorprone/refaster/matchers/IsIdentityOperation.java b/refaster-support/src/main/java/tech/picnic/errorprone/refaster/matchers/IsIdentityOperation.java new file mode 100644 index 00000000000..90835da4160 --- /dev/null +++ b/refaster-support/src/main/java/tech/picnic/errorprone/refaster/matchers/IsIdentityOperation.java @@ -0,0 +1,56 @@ +package tech.picnic.errorprone.refaster.matchers; + +import static com.google.errorprone.matchers.Matchers.anyOf; +import static com.google.errorprone.matchers.Matchers.staticMethod; +import static com.google.errorprone.matchers.Matchers.toType; + +import com.google.errorprone.VisitorState; +import com.google.errorprone.matchers.Matcher; +import com.google.errorprone.util.ASTHelpers; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.LambdaExpressionTree; +import com.sun.source.tree.LambdaExpressionTree.BodyKind; +import com.sun.source.tree.Tree.Kind; +import java.util.function.DoubleUnaryOperator; +import java.util.function.Function; +import java.util.function.IntUnaryOperator; +import java.util.function.LongUnaryOperator; + +/** A matcher of expressions that represent identity operations. */ +// XXX: In selected contexts many other method invocations can be considered identity operations; +// see the `IdentityConversion` check. Review whether those can/should be captured by this matcher +// as well. +public final class IsIdentityOperation implements Matcher { + private static final long serialVersionUID = 1L; + private static final Matcher DELEGATE = + anyOf( + staticMethod() + .onDescendantOfAny( + DoubleUnaryOperator.class.getName(), + Function.class.getName(), + IntUnaryOperator.class.getName(), + LongUnaryOperator.class.getName()) + .named("identity"), + isIdentityLambdaExpression()); + + /** Instantiates a new {@link IsIdentityOperation} instance. */ + public IsIdentityOperation() {} + + @Override + public boolean matches(ExpressionTree tree, VisitorState state) { + return DELEGATE.matches(tree, state); + } + + // XXX: Also support selected block expressions, including ones that perform a vacuous parameter + // transformation such as those identified by the `IdentityConversion` check. + private static Matcher isIdentityLambdaExpression() { + return toType( + LambdaExpressionTree.class, + (tree, state) -> + tree.getBodyKind() == BodyKind.EXPRESSION + && tree.getParameters().size() == 1 + && tree.getBody().getKind() == Kind.IDENTIFIER + && ASTHelpers.getSymbol(tree.getParameters().get(0)) + .equals(ASTHelpers.getSymbol(tree.getBody()))); + } +} diff --git a/refaster-support/src/test/java/tech/picnic/errorprone/refaster/matchers/IsIdentityOperationTest.java b/refaster-support/src/test/java/tech/picnic/errorprone/refaster/matchers/IsIdentityOperationTest.java new file mode 100644 index 00000000000..7536befc3b4 --- /dev/null +++ b/refaster-support/src/test/java/tech/picnic/errorprone/refaster/matchers/IsIdentityOperationTest.java @@ -0,0 +1,73 @@ +package tech.picnic.errorprone.refaster.matchers; + +import static com.google.errorprone.BugPattern.SeverityLevel.ERROR; + +import com.google.errorprone.BugPattern; +import com.google.errorprone.CompilationTestHelper; +import com.google.errorprone.bugpatterns.BugChecker; +import org.junit.jupiter.api.Test; + +final class IsIdentityOperationTest { + @Test + void matches() { + CompilationTestHelper.newInstance(MatcherTestChecker.class, getClass()) + .addSourceLines( + "A.java", + "import java.util.function.BinaryOperator;", + "import java.util.function.DoubleUnaryOperator;", + "import java.util.function.Function;", + "import java.util.function.IntUnaryOperator;", + "import java.util.function.LongUnaryOperator;", + "import java.util.function.UnaryOperator;", + "", + "class A {", + " BinaryOperator negative1() {", + " return (a, b) -> a;", + " }", + "", + " DoubleUnaryOperator positive1() {", + " // BUG: Diagnostic contains:", + " return DoubleUnaryOperator.identity();", + " }", + "", + " Function positive2() {", + " // BUG: Diagnostic contains:", + " return Function.identity();", + " }", + "", + " UnaryOperator positive3() {", + " // BUG: Diagnostic contains:", + " return UnaryOperator.identity();", + " }", + "", + " IntUnaryOperator positive4() {", + " // BUG: Diagnostic contains:", + " return IntUnaryOperator.identity();", + " }", + "", + " LongUnaryOperator positive5() {", + " // BUG: Diagnostic contains:", + " return LongUnaryOperator.identity();", + " }", + "", + " UnaryOperator positive6() {", + " // BUG: Diagnostic contains:", + " return a -> a;", + " }", + "}") + .doTest(); + } + + /** A {@link BugChecker} that simply delegates to {@link IsIdentityOperation}. */ + @BugPattern(summary = "Flags expressions matched by `IsIdentityOperation`", severity = ERROR) + public static final class MatcherTestChecker extends AbstractMatcherTestChecker { + private static final long serialVersionUID = 1L; + + // XXX: This is a false positive reported by Checkstyle. See + // https://github.com/checkstyle/checkstyle/issues/10161#issuecomment-1242732120. + @SuppressWarnings("RedundantModifier") + public MatcherTestChecker() { + super(new IsIdentityOperation()); + } + } +}