From a8447b0537f07a0fe288531a30f69fd7b216a3fa Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Sat, 13 Jan 2024 15:54:35 +0100 Subject: [PATCH] Introduce additional Reactor Refaster rules In various contexts, suggest more efficient and/or less verbose constructs. --- .../refasterrules/ReactorRules.java | 124 +++++++++++++++++- .../refasterrules/ReactorRulesTestInput.java | 64 +++++++-- .../refasterrules/ReactorRulesTestOutput.java | 58 +++++++- 3 files changed, 224 insertions(+), 22 deletions(-) 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 977248a2d85..ff036601f20 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 @@ -5,7 +5,11 @@ import static com.google.common.collect.MoreCollectors.toOptional; import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; 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.stream.Collectors.maxBy; +import static java.util.stream.Collectors.minBy; import static java.util.stream.Collectors.toCollection; import static org.assertj.core.api.Assertions.assertThat; import static reactor.function.TupleUtils.function; @@ -41,6 +45,7 @@ import org.reactivestreams.Publisher; import reactor.core.publisher.Flux; import reactor.core.publisher.Mono; +import reactor.math.MathFlux; import reactor.test.StepVerifier; import reactor.test.publisher.PublisherProbe; import reactor.util.context.Context; @@ -596,6 +601,11 @@ Flux after(Flux flux, Function function, int prefe /** Avoid contrived alternatives to {@link Mono#flatMapIterable(Function)}. */ static final class MonoFlatMapIterable> { + @BeforeTemplate + Flux before(Mono mono, Function function) { + return mono.map(function).flatMapMany(Flux::fromIterable); + } + @BeforeTemplate Flux before( Mono mono, @@ -608,7 +618,7 @@ Flux before( } @AfterTemplate - Flux after(Mono mono, Function> function) { + Flux after(Mono mono, Function function) { return mono.flatMapIterable(function); } } @@ -950,11 +960,12 @@ static final class FluxThenEmpty { } } - /** Avoid vacuous invocations of {@link Mono#ignoreElement()}. */ + /** Avoid vacuous operations prior to invocation of {@link Mono#thenMany(Publisher)}. */ static final class MonoThenMany { @BeforeTemplate Flux before(Mono mono, Publisher publisher) { - return mono.ignoreElement().thenMany(publisher); + return Refaster.anyOf( + mono.ignoreElement().thenMany(publisher), mono.flux().thenMany(publisher)); } @AfterTemplate @@ -992,11 +1003,11 @@ Flux after(Flux flux, Publisher publisher) { } } - /** Avoid vacuous invocations of {@link Mono#ignoreElement()}. */ + /** Avoid vacuous operations prior to invocation of {@link Mono#then(Mono)}. */ static final class MonoThenMono { @BeforeTemplate Mono before(Mono mono1, Mono mono2) { - return mono1.ignoreElement().then(mono2); + return Refaster.anyOf(mono1.ignoreElement().then(mono2), mono1.flux().then(mono2)); } @BeforeTemplate @@ -1579,6 +1590,107 @@ Mono> after(Flux flux) { } } + /** Prefer {@link Flux#sort()} over more verbose alternatives. */ + static final class FluxSort> { + @BeforeTemplate + Flux before(Flux flux) { + return flux.sort(naturalOrder()); + } + + @AfterTemplate + Flux after(Flux flux) { + return flux.sort(); + } + } + + /** Prefer {@link MathFlux#min(Publisher)} over less efficient alternatives. */ + static final class FluxTransformMin> { + @BeforeTemplate + Mono before(Flux flux) { + return flux.sort().next(); + } + + @AfterTemplate + Mono after(Flux flux) { + return flux.transform(MathFlux::min).next(); + } + } + + /** + * Prefer {@link MathFlux#min(Publisher, Comparator)} over less efficient or more verbose + * alternatives. + */ + static final class FluxTransformMinWithComparator> { + @BeforeTemplate + Mono before(Flux flux, Comparator cmp) { + return Refaster.anyOf( + flux.sort(cmp).next(), flux.collect(minBy(cmp)).flatMap(Mono::justOrEmpty)); + } + + @AfterTemplate + Mono after(Flux flux, Comparator cmp) { + return flux.transform(f -> MathFlux.min(f, cmp)).next(); + } + } + + /** Prefer {@link MathFlux#max(Publisher)} over less efficient alternatives. */ + static final class FluxTransformMax> { + @BeforeTemplate + Mono before(Flux flux) { + return flux.sort().last(); + } + + @AfterTemplate + Mono after(Flux flux) { + return flux.transform(MathFlux::max).next(); + } + } + + /** + * Prefer {@link MathFlux#max(Publisher, Comparator)} over less efficient or more verbose + * alternatives. + */ + static final class FluxTransformMaxWithComparator> { + @BeforeTemplate + Mono before(Flux flux, Comparator cmp) { + return Refaster.anyOf( + flux.sort(cmp).last(), flux.collect(maxBy(cmp)).flatMap(Mono::justOrEmpty)); + } + + @AfterTemplate + Mono after(Flux flux, Comparator cmp) { + return flux.transform(f -> MathFlux.max(f, cmp)).next(); + } + } + + /** Prefer {@link MathFlux#min(Publisher)} over more contrived alternatives. */ + static final class MathFluxMin> { + @BeforeTemplate + Mono before(Publisher publisher) { + return Refaster.anyOf( + MathFlux.min(publisher, naturalOrder()), MathFlux.max(publisher, reverseOrder())); + } + + @AfterTemplate + Mono after(Publisher publisher) { + return MathFlux.min(publisher); + } + } + + /** Prefer {@link MathFlux#max(Publisher)} over more contrived alternatives. */ + static final class MathFluxMax> { + @BeforeTemplate + Mono before(Publisher publisher) { + return Refaster.anyOf( + MathFlux.min(publisher, reverseOrder()), MathFlux.max(publisher, naturalOrder())); + } + + @AfterTemplate + Mono after(Publisher publisher) { + return MathFlux.max(publisher); + } + } + /** Prefer {@link reactor.util.context.Context#empty()}} over more verbose alternatives. */ // XXX: Introduce Refaster rules or a `BugChecker` that maps `(Immutable)Map.of(k, v)` to // `Context.of(k, v)` and likewise for multi-pair overloads. @@ -1611,7 +1723,7 @@ PublisherProbe after() { static final class StepVerifierFromMono { @BeforeTemplate StepVerifier.FirstStep before(Mono mono) { - return StepVerifier.create(mono); + return Refaster.anyOf(StepVerifier.create(mono), mono.flux().as(StepVerifier::create)); } @AfterTemplate 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 adcdd1243c5..5a7dd66199a 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 @@ -2,8 +2,11 @@ import static com.google.common.collect.ImmutableList.toImmutableList; import static com.google.common.collect.MoreCollectors.toOptional; +import static java.util.Comparator.naturalOrder; import static java.util.Comparator.reverseOrder; import static java.util.function.Function.identity; +import static java.util.stream.Collectors.maxBy; +import static java.util.stream.Collectors.minBy; import static java.util.stream.Collectors.toCollection; import static org.assertj.core.api.Assertions.assertThat; @@ -21,6 +24,7 @@ import java.util.function.Supplier; import reactor.core.publisher.Flux; import reactor.core.publisher.Mono; +import reactor.math.MathFlux; import reactor.test.StepVerifier; import reactor.test.publisher.PublisherProbe; import reactor.util.context.Context; @@ -38,6 +42,9 @@ public ImmutableSet elidedTypesAndStaticImports() { ImmutableCollection.class, ImmutableMap.class, assertThat(0), + maxBy(null), + minBy(null), + naturalOrder(), toCollection(null), toImmutableList(), toOptional()); @@ -218,10 +225,11 @@ ImmutableSet> testFluxConcatMapWithPrefetch() { ImmutableSet> testMonoFlatMapIterable() { return ImmutableSet.of( - Mono.just(1).map(ImmutableSet::of).flatMapIterable(identity()), - Mono.just(2).map(ImmutableSet::of).flatMapIterable(v -> v), - Mono.just(3).map(ImmutableSet::of).flatMapIterable(v -> ImmutableSet.of()), - Mono.just(4).flux().concatMapIterable(ImmutableSet::of)); + Mono.just(1).map(ImmutableSet::of).flatMapMany(Flux::fromIterable), + Mono.just(2).map(ImmutableSet::of).flatMapIterable(identity()), + Mono.just(3).map(ImmutableSet::of).flatMapIterable(v -> v), + Mono.just(4).map(ImmutableSet::of).flatMapIterable(v -> ImmutableSet.of()), + Mono.just(5).flux().concatMapIterable(ImmutableSet::of)); } Flux testMonoFlatMapIterableIdentity() { @@ -336,8 +344,10 @@ Mono testFluxThenEmpty() { return Flux.just("foo").ignoreElements().thenEmpty(Mono.empty()); } - Flux testMonoThenMany() { - return Mono.just("foo").ignoreElement().thenMany(Flux.just("bar")); + ImmutableSet> testMonoThenMany() { + return ImmutableSet.of( + Mono.just("foo").ignoreElement().thenMany(Flux.just("bar")), + Mono.just("baz").ignoreElement().thenMany(Flux.just("qux"))); } Flux testMonoThenMonoFlux() { @@ -351,7 +361,8 @@ Flux testFluxThenMany() { ImmutableSet> testMonoThenMono() { return ImmutableSet.of( Mono.just("foo").ignoreElement().then(Mono.just("bar")), - Mono.just("baz").thenEmpty(Mono.empty())); + Mono.just("baz").flux().then(Mono.just("qux")), + Mono.just("quux").thenEmpty(Mono.empty())); } ImmutableSet> testFluxThenMono() { @@ -525,6 +536,40 @@ Mono> testFluxCollectToImmutableSet() { return Flux.just(1).collect(toImmutableList()).map(ImmutableSet::copyOf); } + Flux testFluxSort() { + return Flux.just(1).sort(naturalOrder()); + } + + Mono testFluxTransformMin() { + return Flux.just(1).sort().next(); + } + + ImmutableSet> testFluxTransformMinWithComparator() { + return ImmutableSet.of( + Flux.just(1).sort(reverseOrder()).next(), + Flux.just(2).collect(minBy(reverseOrder())).flatMap(Mono::justOrEmpty)); + } + + Mono testFluxTransformMax() { + return Flux.just(1).sort().last(); + } + + ImmutableSet> testFluxTransformMaxWithComparator() { + return ImmutableSet.of( + Flux.just(1).sort(reverseOrder()).last(), + Flux.just(2).collect(maxBy(reverseOrder())).flatMap(Mono::justOrEmpty)); + } + + ImmutableSet> testMathFluxMin() { + return ImmutableSet.of( + MathFlux.min(Flux.just(1), naturalOrder()), MathFlux.max(Flux.just(2), reverseOrder())); + } + + ImmutableSet> testMathFluxMax() { + return ImmutableSet.of( + MathFlux.min(Flux.just(1), reverseOrder()), MathFlux.max(Flux.just(2), naturalOrder())); + } + ImmutableSet testContextEmpty() { return ImmutableSet.of(Context.of(ImmutableMap.of()), Context.of(ImmutableMap.of(1, 2))); } @@ -533,8 +578,9 @@ ImmutableSet> testPublisherProbeEmpty() { return ImmutableSet.of(PublisherProbe.of(Mono.empty()), PublisherProbe.of(Flux.empty())); } - StepVerifier.FirstStep testStepVerifierFromMono() { - return StepVerifier.create(Mono.just(1)); + ImmutableSet> testStepVerifierFromMono() { + return ImmutableSet.of( + StepVerifier.create(Mono.just(1)), Mono.just(2).flux().as(StepVerifier::create)); } StepVerifier.FirstStep testStepVerifierFromFlux() { 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 0a5bf6a7f8e..6e81e8e636d 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 @@ -3,8 +3,11 @@ import static com.google.common.collect.ImmutableList.toImmutableList; import static com.google.common.collect.ImmutableSet.toImmutableSet; import static com.google.common.collect.MoreCollectors.toOptional; +import static java.util.Comparator.naturalOrder; import static java.util.Comparator.reverseOrder; import static java.util.function.Function.identity; +import static java.util.stream.Collectors.maxBy; +import static java.util.stream.Collectors.minBy; import static java.util.stream.Collectors.toCollection; import static org.assertj.core.api.Assertions.assertThat; import static reactor.function.TupleUtils.function; @@ -24,6 +27,7 @@ import reactor.core.publisher.Flux; import reactor.core.publisher.Mono; import reactor.function.TupleUtils; +import reactor.math.MathFlux; import reactor.test.StepVerifier; import reactor.test.publisher.PublisherProbe; import reactor.util.context.Context; @@ -41,6 +45,9 @@ public ImmutableSet elidedTypesAndStaticImports() { ImmutableCollection.class, ImmutableMap.class, assertThat(0), + maxBy(null), + minBy(null), + naturalOrder(), toCollection(null), toImmutableList(), toOptional()); @@ -223,8 +230,9 @@ ImmutableSet> testMonoFlatMapIterable() { return ImmutableSet.of( Mono.just(1).flatMapIterable(ImmutableSet::of), Mono.just(2).flatMapIterable(ImmutableSet::of), - Mono.just(3).map(ImmutableSet::of).flatMapIterable(v -> ImmutableSet.of()), - Mono.just(4).flatMapIterable(ImmutableSet::of)); + Mono.just(3).flatMapIterable(ImmutableSet::of), + Mono.just(4).map(ImmutableSet::of).flatMapIterable(v -> ImmutableSet.of()), + Mono.just(5).flatMapIterable(ImmutableSet::of)); } Flux testMonoFlatMapIterableIdentity() { @@ -333,8 +341,9 @@ Mono testFluxThenEmpty() { return Flux.just("foo").thenEmpty(Mono.empty()); } - Flux testMonoThenMany() { - return Mono.just("foo").thenMany(Flux.just("bar")); + ImmutableSet> testMonoThenMany() { + return ImmutableSet.of( + Mono.just("foo").thenMany(Flux.just("bar")), Mono.just("baz").thenMany(Flux.just("qux"))); } Flux testMonoThenMonoFlux() { @@ -347,7 +356,9 @@ Flux testFluxThenMany() { ImmutableSet> testMonoThenMono() { return ImmutableSet.of( - Mono.just("foo").then(Mono.just("bar")), Mono.just("baz").then(Mono.empty())); + Mono.just("foo").then(Mono.just("bar")), + Mono.just("baz").then(Mono.just("qux")), + Mono.just("quux").then(Mono.empty())); } ImmutableSet> testFluxThenMono() { @@ -514,6 +525,38 @@ Mono> testFluxCollectToImmutableSet() { return Flux.just(1).collect(toImmutableSet()); } + Flux testFluxSort() { + return Flux.just(1).sort(); + } + + Mono testFluxTransformMin() { + return Flux.just(1).transform(MathFlux::min).next(); + } + + ImmutableSet> testFluxTransformMinWithComparator() { + return ImmutableSet.of( + Flux.just(1).transform(f -> MathFlux.min(f, reverseOrder())).next(), + Flux.just(2).transform(f -> MathFlux.min(f, reverseOrder())).next()); + } + + Mono testFluxTransformMax() { + return Flux.just(1).transform(MathFlux::max).next(); + } + + ImmutableSet> testFluxTransformMaxWithComparator() { + return ImmutableSet.of( + Flux.just(1).transform(f -> MathFlux.max(f, reverseOrder())).next(), + Flux.just(2).transform(f -> MathFlux.max(f, reverseOrder())).next()); + } + + ImmutableSet> testMathFluxMin() { + return ImmutableSet.of(MathFlux.min(Flux.just(1)), MathFlux.min(Flux.just(2))); + } + + ImmutableSet> testMathFluxMax() { + return ImmutableSet.of(MathFlux.max(Flux.just(1)), MathFlux.max(Flux.just(2))); + } + ImmutableSet testContextEmpty() { return ImmutableSet.of(Context.empty(), Context.of(ImmutableMap.of(1, 2))); } @@ -522,8 +565,9 @@ ImmutableSet> testPublisherProbeEmpty() { return ImmutableSet.of(PublisherProbe.empty(), PublisherProbe.empty()); } - StepVerifier.FirstStep testStepVerifierFromMono() { - return Mono.just(1).as(StepVerifier::create); + ImmutableSet> testStepVerifierFromMono() { + return ImmutableSet.of( + Mono.just(1).as(StepVerifier::create), Mono.just(2).as(StepVerifier::create)); } StepVerifier.FirstStep testStepVerifierFromFlux() {