Skip to content

Commit

Permalink
Introduce additional Reactor Refaster rules (#969)
Browse files Browse the repository at this point in the history 
In various contexts, suggest more efficient and/or less verbose
constructs.
  • Loading branch information
@Stephan202
Stephan202 authored Jan 21, 2024
1 parent 9e6d355 commit 0aa6120
Show file tree
Hide file tree
Showing 3 changed files with 234 additions and 25 deletions.
129 changes: 122 additions & 7 deletions error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -596,6 +601,11 @@ Flux<S> after(Flux<T> flux, Function<? super T, ? extends P> function, int prefe

/** Avoid contrived alternatives to {@link Mono#flatMapIterable(Function)}. */
static final class MonoFlatMapIterable<T, S, I extends Iterable<? extends S>> {
@BeforeTemplate
Flux<S> before(Mono<T> mono, Function<? super T, I> function) {
return mono.map(function).flatMapMany(Flux::fromIterable);
}

@BeforeTemplate
Flux<S> before(
Mono<T> mono,
Expand All @@ -608,7 +618,7 @@ Flux<S> before(
}

@AfterTemplate
Flux<S> after(Mono<T> mono, Function<? super T, ? extends Iterable<? extends S>> function) {
Flux<S> after(Mono<T> mono, Function<? super T, I> function) {
return mono.flatMapIterable(function);
}
}
Expand Down Expand Up @@ -950,11 +960,12 @@ static final class FluxThenEmpty<T> {
}
}

/** Avoid vacuous invocations of {@link Mono#ignoreElement()}. */
/** Avoid vacuous operations prior to invocation of {@link Mono#thenMany(Publisher)}. */
static final class MonoThenMany<T, S> {
@BeforeTemplate
Flux<S> before(Mono<T> mono, Publisher<S> publisher) {
return mono.ignoreElement().thenMany(publisher);
return Refaster.anyOf(
mono.ignoreElement().thenMany(publisher), mono.flux().thenMany(publisher));
}

@AfterTemplate
Expand Down Expand Up @@ -992,11 +1003,11 @@ Flux<S> after(Flux<T> flux, Publisher<S> publisher) {
}
}

/** Avoid vacuous invocations of {@link Mono#ignoreElement()}. */
/** Avoid vacuous operations prior to invocation of {@link Mono#then(Mono)}. */
static final class MonoThenMono<T, S> {
@BeforeTemplate
Mono<S> before(Mono<T> mono1, Mono<S> mono2) {
return mono1.ignoreElement().then(mono2);
return Refaster.anyOf(mono1.ignoreElement().then(mono2), mono1.flux().then(mono2));
}

@BeforeTemplate
Expand Down Expand Up @@ -1031,12 +1042,15 @@ Mono<S> after(Flux<T> flux, Mono<S> mono) {
/** Prefer {@link Mono#singleOptional()} over more contrived alternatives. */
// XXX: Consider creating a plugin that flags/discourages `Mono<Optional<T>>` method return
// types, just as we discourage nullable `Boolean`s and `Optional`s.
// XXX: The `mono.transform(Mono::singleOptional)` replacement is a special case of a more general
// rule. Consider introducing an Error Prone check for this.
static final class MonoSingleOptional<T> {
@BeforeTemplate
Mono<Optional<T>> before(Mono<T> mono) {
return Refaster.anyOf(
mono.flux().collect(toOptional()),
mono.map(Optional::of).defaultIfEmpty(Optional.empty()));
mono.map(Optional::of).defaultIfEmpty(Optional.empty()),
mono.transform(Mono::singleOptional));
}

@AfterTemplate
Expand Down Expand Up @@ -1579,6 +1593,107 @@ Mono<ImmutableSet<T>> after(Flux<T> flux) {
}
}

/** Prefer {@link Flux#sort()} over more verbose alternatives. */
static final class FluxSort<T extends Comparable<? super T>> {
@BeforeTemplate
Flux<T> before(Flux<T> flux) {
return flux.sort(naturalOrder());
}

@AfterTemplate
Flux<T> after(Flux<T> flux) {
return flux.sort();
}
}

/** Prefer {@link MathFlux#min(Publisher)} over less efficient alternatives. */
static final class FluxTransformMin<T extends Comparable<? super T>> {
@BeforeTemplate
Mono<T> before(Flux<T> flux) {
return flux.sort().next();
}

@AfterTemplate
Mono<T> after(Flux<T> flux) {
return flux.transform(MathFlux::min).singleOrEmpty();
}
}

/**
* Prefer {@link MathFlux#min(Publisher, Comparator)} over less efficient or more verbose
* alternatives.
*/
static final class FluxTransformMinWithComparator<T extends Comparable<? super T>> {
@BeforeTemplate
Mono<T> before(Flux<T> flux, Comparator<? super T> cmp) {
return Refaster.anyOf(
flux.sort(cmp).next(), flux.collect(minBy(cmp)).flatMap(Mono::justOrEmpty));
}

@AfterTemplate
Mono<T> after(Flux<T> flux, Comparator<? super T> cmp) {
return flux.transform(f -> MathFlux.min(f, cmp)).singleOrEmpty();
}
}

/** Prefer {@link MathFlux#max(Publisher)} over less efficient alternatives. */
static final class FluxTransformMax<T extends Comparable<? super T>> {
@BeforeTemplate
Mono<T> before(Flux<T> flux) {
return flux.sort().last();
}

@AfterTemplate
Mono<T> after(Flux<T> flux) {
return flux.transform(MathFlux::max).singleOrEmpty();
}
}

/**
* Prefer {@link MathFlux#max(Publisher, Comparator)} over less efficient or more verbose
* alternatives.
*/
static final class FluxTransformMaxWithComparator<T extends Comparable<? super T>> {
@BeforeTemplate
Mono<T> before(Flux<T> flux, Comparator<? super T> cmp) {
return Refaster.anyOf(
flux.sort(cmp).last(), flux.collect(maxBy(cmp)).flatMap(Mono::justOrEmpty));
}

@AfterTemplate
Mono<T> after(Flux<T> flux, Comparator<? super T> cmp) {
return flux.transform(f -> MathFlux.max(f, cmp)).singleOrEmpty();
}
}

/** Prefer {@link MathFlux#min(Publisher)} over more contrived alternatives. */
static final class MathFluxMin<T extends Comparable<? super T>> {
@BeforeTemplate
Mono<T> before(Publisher<T> publisher) {
return Refaster.anyOf(
MathFlux.min(publisher, naturalOrder()), MathFlux.max(publisher, reverseOrder()));
}

@AfterTemplate
Mono<T> after(Publisher<T> publisher) {
return MathFlux.min(publisher);
}
}

/** Prefer {@link MathFlux#max(Publisher)} over more contrived alternatives. */
static final class MathFluxMax<T extends Comparable<? super T>> {
@BeforeTemplate
Mono<T> before(Publisher<T> publisher) {
return Refaster.anyOf(
MathFlux.min(publisher, reverseOrder()), MathFlux.max(publisher, naturalOrder()));
}

@AfterTemplate
Mono<T> after(Publisher<T> 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.
Expand Down Expand Up @@ -1611,7 +1726,7 @@ PublisherProbe<T> after() {
static final class StepVerifierFromMono<T> {
@BeforeTemplate
StepVerifier.FirstStep<? extends T> before(Mono<T> mono) {
return StepVerifier.create(mono);
return Refaster.anyOf(StepVerifier.create(mono), mono.flux().as(StepVerifier::create));
}

@AfterTemplate
Expand Down
67 changes: 57 additions & 10 deletions ...ontrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestInput.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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;
Expand All @@ -38,6 +42,9 @@ public ImmutableSet<Object> elidedTypesAndStaticImports() {
ImmutableCollection.class,
ImmutableMap.class,
assertThat(0),
maxBy(null),
minBy(null),
naturalOrder(),
toCollection(null),
toImmutableList(),
toOptional());
Expand Down Expand Up @@ -218,10 +225,11 @@ ImmutableSet<Flux<Integer>> testFluxConcatMapWithPrefetch() {

ImmutableSet<Flux<Integer>> 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<Integer> testMonoFlatMapIterableIdentity() {
Expand Down Expand Up @@ -336,8 +344,10 @@ Mono<Void> testFluxThenEmpty() {
return Flux.just("foo").ignoreElements().thenEmpty(Mono.empty());
}

Flux<String> testMonoThenMany() {
return Mono.just("foo").ignoreElement().thenMany(Flux.just("bar"));
ImmutableSet<Flux<String>> testMonoThenMany() {
return ImmutableSet.of(
Mono.just("foo").ignoreElement().thenMany(Flux.just("bar")),
Mono.just("baz").ignoreElement().thenMany(Flux.just("qux")));
}

Flux<String> testMonoThenMonoFlux() {
Expand All @@ -351,7 +361,8 @@ Flux<String> testFluxThenMany() {
ImmutableSet<Mono<?>> testMonoThenMono() {
return ImmutableSet.of(
Mono.just("foo").ignoreElement().then(Mono.just("bar")),
Mono.just("baz").thenEmpty(Mono.<Void>empty()));
Mono.just("baz").flux().then(Mono.just("qux")),
Mono.just("quux").thenEmpty(Mono.<Void>empty()));
}

ImmutableSet<Mono<?>> testFluxThenMono() {
Expand All @@ -363,7 +374,8 @@ ImmutableSet<Mono<?>> testFluxThenMono() {
ImmutableSet<Mono<Optional<String>>> testMonoSingleOptional() {
return ImmutableSet.of(
Mono.just("foo").flux().collect(toOptional()),
Mono.just("bar").map(Optional::of).defaultIfEmpty(Optional.empty()));
Mono.just("bar").map(Optional::of).defaultIfEmpty(Optional.empty()),
Mono.just("baz").transform(Mono::singleOptional));
}

Mono<Number> testMonoCast() {
Expand Down Expand Up @@ -525,6 +537,40 @@ Mono<ImmutableSet<Integer>> testFluxCollectToImmutableSet() {
return Flux.just(1).collect(toImmutableList()).map(ImmutableSet::copyOf);
}

Flux<Integer> testFluxSort() {
return Flux.just(1).sort(naturalOrder());
}

Mono<Integer> testFluxTransformMin() {
return Flux.just(1).sort().next();
}

ImmutableSet<Mono<Integer>> testFluxTransformMinWithComparator() {
return ImmutableSet.of(
Flux.just(1).sort(reverseOrder()).next(),
Flux.just(2).collect(minBy(reverseOrder())).flatMap(Mono::justOrEmpty));
}

Mono<Integer> testFluxTransformMax() {
return Flux.just(1).sort().last();
}

ImmutableSet<Mono<Integer>> testFluxTransformMaxWithComparator() {
return ImmutableSet.of(
Flux.just(1).sort(reverseOrder()).last(),
Flux.just(2).collect(maxBy(reverseOrder())).flatMap(Mono::justOrEmpty));
}

ImmutableSet<Mono<Integer>> testMathFluxMin() {
return ImmutableSet.of(
MathFlux.min(Flux.just(1), naturalOrder()), MathFlux.max(Flux.just(2), reverseOrder()));
}

ImmutableSet<Mono<Integer>> testMathFluxMax() {
return ImmutableSet.of(
MathFlux.min(Flux.just(1), reverseOrder()), MathFlux.max(Flux.just(2), naturalOrder()));
}

ImmutableSet<Context> testContextEmpty() {
return ImmutableSet.of(Context.of(ImmutableMap.of()), Context.of(ImmutableMap.of(1, 2)));
}
Expand All @@ -533,8 +579,9 @@ ImmutableSet<PublisherProbe<Void>> testPublisherProbeEmpty() {
return ImmutableSet.of(PublisherProbe.of(Mono.empty()), PublisherProbe.of(Flux.empty()));
}

StepVerifier.FirstStep<Integer> testStepVerifierFromMono() {
return StepVerifier.create(Mono.just(1));
ImmutableSet<StepVerifier.FirstStep<Integer>> testStepVerifierFromMono() {
return ImmutableSet.of(
StepVerifier.create(Mono.just(1)), Mono.just(2).flux().as(StepVerifier::create));
}

StepVerifier.FirstStep<Integer> testStepVerifierFromFlux() {
Expand Down
Loading

0 comments on commit 0aa6120

Please sign in to comment.