From 38935bff9732d55ff39f8934ebb2d0567e82c37b Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Fri, 31 Dec 2021 18:04:51 +0100 Subject: [PATCH] Suggestions --- .../bugpatterns/FluxFlatMapUsageCheck.java | 64 +++++++++++-------- .../refastertemplates/ReactorTemplates.java | 13 ++++ .../FluxFlatMapUsageCheckTest.java | 42 +++++++----- .../ReactorTemplatesTestInput.java | 5 ++ .../ReactorTemplatesTestOutput.java | 4 ++ 5 files changed, 86 insertions(+), 42 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/FluxFlatMapUsageCheck.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/FluxFlatMapUsageCheck.java index 196499d33a5..0a79d40cc92 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/FluxFlatMapUsageCheck.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/FluxFlatMapUsageCheck.java @@ -3,37 +3,51 @@ import static com.google.errorprone.matchers.method.MethodMatchers.instanceMethod; import com.google.auto.service.AutoService; +import com.google.common.collect.Iterables; import com.google.errorprone.BugPattern; import com.google.errorprone.BugPattern.LinkType; import com.google.errorprone.BugPattern.SeverityLevel; import com.google.errorprone.BugPattern.StandardTags; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker; -import com.google.errorprone.bugpatterns.BugChecker.MemberSelectTreeMatcher; +import com.google.errorprone.bugpatterns.BugChecker.MemberReferenceTreeMatcher; +import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher; import com.google.errorprone.fixes.SuggestedFix; +import com.google.errorprone.fixes.SuggestedFixes; import com.google.errorprone.matchers.Description; import com.google.errorprone.matchers.Matcher; import com.sun.source.tree.ExpressionTree; -import com.sun.source.tree.MemberSelectTree; -import com.sun.source.tree.Tree; +import com.sun.source.tree.MemberReferenceTree; +import com.sun.source.tree.MethodInvocationTree; import java.util.function.Function; +import java.util.function.Supplier; import reactor.core.publisher.Flux; -/** A {@link BugChecker} which flags usages of {@link Flux#flatMap(Function)}s. */ +/** + * A {@link BugChecker} which flags usages of {@link Flux#flatMap(Function)}. + * + *

{@link Flux#flatMap(Function)} eagerly performs up to {@link + * reactor.util.concurrent.Queues#SMALL_BUFFER_SIZE} subscriptions, interleaving the results as they + * are emitted. In most cases {@link Flux#concatMap(Function)} should be preferred, as it produces + * consistent results and avoids potentially saturating the thread pool on which subscription + * happens. If {@code concatMap}'s single-subscription semantics are undesirable one should invoke a + * {@code flatMap} or {@code flatMapSequential} overload with an explicit concurrency level. + * + *

NB: The rarely-used overload {@link Flux#flatMap(Function, Function, Supplier)} is not flagged + * by this check because there is no clear alternative to point to. + */ @AutoService(BugChecker.class) @BugPattern( name = "FluxFlatMapUsage", summary = - "`Flux#flatMap` is not allowed, please use `Flux#concatMap` or specify an argument for the concurrency.", - explanation = - "`Flux#flatMap` provides unbounded parallelism and is not guaranteed to be sequential. " - + "Therefore, we disallow the use of the non-overloaded `Flux#flatMap`.", + "`Flux#flatMap` has subtle semantics; please use `Flux#concatMap` or explicitly specify the desired amount of concurrency", linkType = LinkType.NONE, severity = SeverityLevel.ERROR, tags = StandardTags.LIKELY_ERROR) -public final class FluxFlatMapUsageCheck extends BugChecker implements MemberSelectTreeMatcher { +public final class FluxFlatMapUsageCheck extends BugChecker + implements MethodInvocationTreeMatcher, MemberReferenceTreeMatcher { private static final long serialVersionUID = 1L; - private static final String NAME_CONCURRENCY_ARGUMENT = "MAX_CONCURRENCY"; + private static final String MAX_CONCURRENCY_ARG_NAME = "MAX_CONCURRENCY"; private static final Matcher FLUX_FLATMAP = instanceMethod() .onDescendantOf("reactor.core.publisher.Flux") @@ -41,33 +55,29 @@ public final class FluxFlatMapUsageCheck extends BugChecker implements MemberSel .withParameters(Function.class.getName()); @Override - public Description matchMemberSelect(MemberSelectTree tree, VisitorState state) { + public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { if (!FLUX_FLATMAP.matches(tree, state)) { return Description.NO_MATCH; } - Tree parentExpression = state.getPath().getParentPath().getLeaf(); - return buildDescription(tree) - .setMessage(message()) + .addFix(SuggestedFixes.renameMethodInvocation(tree, "concatMap", state)) .addFix( SuggestedFix.builder() - .replace(tree, Util.treeToString(tree, state).replace("flatMap", "concatMap")) - .build()) - .addFix( - SuggestedFix.builder() - .replace( - parentExpression, - getReplacementWithConcurrencyArgument(parentExpression, state)) + .postfixWith( + Iterables.getOnlyElement(tree.getArguments()), ", " + MAX_CONCURRENCY_ARG_NAME) .build()) .build(); } - private static String getReplacementWithConcurrencyArgument( - Tree parentExpression, VisitorState state) { - String parentString = Util.treeToString(parentExpression, state); - return String.format( - "%s, %s)", - parentString.substring(0, parentString.lastIndexOf(')')), NAME_CONCURRENCY_ARGUMENT); + @Override + public Description matchMemberReference(MemberReferenceTree tree, VisitorState state) { + if (!FLUX_FLATMAP.matches(tree, state)) { + return Description.NO_MATCH; + } + + // Method references are expected to occur very infrequently; generating both variants of + // suggested fixes is not worth the trouble. + return describeMatch(tree); } } diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/ReactorTemplates.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/ReactorTemplates.java index 2ec67549e87..27e7f6fa5bc 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/ReactorTemplates.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/ReactorTemplates.java @@ -142,6 +142,19 @@ Flux after(Flux flux) { } } + /** Prefer {@link Flux#concatMap(Function)} over more contrived alternatives. */ + static final class FluxConcatMap { + @BeforeTemplate + Flux before(Flux flux, Function> function) { + return Refaster.anyOf(flux.flatMap(function, 1), flux.flatMapSequential(function, 1)); + } + + @AfterTemplate + Flux after(Flux flux, Function> function) { + return flux.concatMap(function); + } + } + /** * Don't use {@link Mono#flatMapMany(Function)} to implicitly convert a {@link Mono} to a {@link * Flux}. diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/FluxFlatMapUsageCheckTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/FluxFlatMapUsageCheckTest.java index e570cb31fb3..227863b3d93 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/FluxFlatMapUsageCheckTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/FluxFlatMapUsageCheckTest.java @@ -18,20 +18,32 @@ void identification() { compilationTestHelper .addSourceLines( "A.java", + "import java.util.function.BiFunction;", + "import java.util.function.Function;", + "import reactor.core.publisher.Mono;", "import reactor.core.publisher.Flux;", "", "class A {", - " void positive() {", - " // BUG: Diagnostic contains:", - " Flux.just(1).flatMap(Flux::just);", - " }", + " void m() {", + " // BUG: Diagnostic contains:", + " Flux.just(1).flatMap(Flux::just);", + " // BUG: Diagnostic contains:", + " Flux.just(1).flatMap(i -> Flux.just(String.valueOf(i)));", "", - " void negative() {", " Flux.just(1).concatMap(Flux::just);", " Flux.just(1).flatMap(Flux::just, 1);", " Flux.just(1).flatMap(Flux::just, 1, 1);", " Flux.just(1).flatMap(Flux::just, throwable -> Flux.empty(), Flux::empty);", + "", + " // BUG: Diagnostic contains:", + " this.>sink(Flux::flatMap);", + " // BUG: Diagnostic contains:", + " this.>sink(Flux::flatMap);", + "", + " this.>sink(Mono::flatMap);", " }", + "", + " private void sink(BiFunction, P> fun) {}", "}") .doTest(); } @@ -45,8 +57,8 @@ void replacementFirstSuggestedFix() { "import reactor.core.publisher.Flux;", "", "class A {", - " void positive() {", - " Flux.just(1).flatMap(Flux::just);", + " void m() {", + " Flux.just(1).flatMap(Flux::just);", " }", "}") .addOutputLines( @@ -54,8 +66,8 @@ void replacementFirstSuggestedFix() { "import reactor.core.publisher.Flux;", "", "class A {", - " void positive() {", - " Flux.just(1).concatMap(Flux::just);", + " void m() {", + " Flux.just(1).concatMap(Flux::just);", " }", "}") .doTest(); @@ -70,10 +82,10 @@ void replacementSecondSuggestedFix() { "import reactor.core.publisher.Flux;", "", "class A {", - " private static final int MAX_CONCURRENCY = 10;", + " private static final int MAX_CONCURRENCY = 8;", "", - " void positive() {", - " Flux.just(1).flatMap(Flux::just);", + " void m() {", + " Flux.just(1).flatMap(Flux::just);", " }", "}") .addOutputLines( @@ -81,10 +93,10 @@ void replacementSecondSuggestedFix() { "import reactor.core.publisher.Flux;", "", "class A {", - " private static final int MAX_CONCURRENCY = 10;", + " private static final int MAX_CONCURRENCY = 8;", "", - " void positive() {", - " Flux.just(1).flatMap(Flux::just, MAX_CONCURRENCY);", + " void m() {", + " Flux.just(1).flatMap(Flux::just, MAX_CONCURRENCY);", " }", "}") .doTest(); diff --git a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/bugpatterns/ReactorTemplatesTestInput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/bugpatterns/ReactorTemplatesTestInput.java index 5a22d191e06..26dd52a1be0 100644 --- a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/bugpatterns/ReactorTemplatesTestInput.java +++ b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/bugpatterns/ReactorTemplatesTestInput.java @@ -45,6 +45,11 @@ ImmutableSet> testFluxSwitchIfEmptyOfEmptyPublisher() { Flux.just(1).switchIfEmpty(Mono.empty()), Flux.just(2).switchIfEmpty(Flux.empty())); } + ImmutableSet> testFluxConcatMap() { + return ImmutableSet.of( + Flux.just(1).flatMap(Mono::just, 1), Flux.just(2).flatMapSequential(Mono::just, 1)); + } + Flux testMonoFlatMapToFlux() { return Mono.just("foo").flatMapMany(s -> Mono.just(s + s)); } diff --git a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/bugpatterns/ReactorTemplatesTestOutput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/bugpatterns/ReactorTemplatesTestOutput.java index c300a31d788..cfc97c29778 100644 --- a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/bugpatterns/ReactorTemplatesTestOutput.java +++ b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/bugpatterns/ReactorTemplatesTestOutput.java @@ -46,6 +46,10 @@ ImmutableSet> testFluxSwitchIfEmptyOfEmptyPublisher() { return ImmutableSet.of(Flux.just(1), Flux.just(2)); } + ImmutableSet> testFluxConcatMap() { + return ImmutableSet.of(Flux.just(1).concatMap(Mono::just), Flux.just(2).concatMap(Mono::just)); + } + Flux testMonoFlatMapToFlux() { return Mono.just("foo").flatMap(s -> Mono.just(s + s)).flux(); }