Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce additional Reactor Refaster rules #969

Merged
merged 3 commits into from
Jan 21, 2024
Merged

Introduce additional Reactor Refaster rules #969

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ae10b3c
Introduce additional Reactor Refaster rules
Stephan202 Jan 13, 2024
000d9cd
Apply suggestion
Stephan202 Jan 15, 2024
1c930ba
Suggestions
Stephan202 Jan 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
129 changes: 122 additions & 7 deletions error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java
View file
Open in desktop
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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's also apply this in the other before template :).

EDIT: It doesn't work, because (correct me if I'm wrong): it doesn't make sense to go from something that is a super type of I to something that is a subtype of I, (or I), right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, it could make sense, but using I instead of ? extends Iterable<? extends S> would impose a greater restriction than necessary or desired: the rule should also work for any Iterable that is not a sub- or super-type of I.

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
Open in desktop
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