Introduce `FluxFlatMapUsageCheck` #26

rickie · 2021-12-30T12:30:09Z

There are two problems with Flux#flatMap:

It provides unbounded parallelism.
It is not guaranteed to be sequential.

Neither of these behaviors is obvious, which is problematic as this method looks a lot like the simpler Java Stream#flatMap. People coming from the non-reactive world would are much more likely to actually be looking for concatMap or flatMapSequential. People looking for particular behavior when it comes to parallelism are likely to be looking for one of the flatMap overloads which define the maximum number of threads.

For these reasons, we want to ban the non-overloaded Flux#flatMap.

werli

Nice seeing the unbounded Flux#flatMap getting attention 🚀

werli · 2021-12-31T09:14:08Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/FluxFlatMapUsageCheck.java

+    explanation =
+        "The problem with `Flux#flatMap` is that it is not clear that it provides unbounded parallelism and is"
+            + " not guaranteed to be sequential. Therefore, we disallow the use of the non-overloaded `Flux#flatMap`.",


Shorter:

Suggested change

explanation =

"The problem with `Flux#flatMap` is that it is not clear that it provides unbounded parallelism and is"

+ " not guaranteed to be sequential. Therefore, we disallow the use of the non-overloaded `Flux#flatMap`.",

explanation =

"`Flux#flatMap` provides unbounded parallelism and is not guaranteed to be sequential. " +

"Therefore, we disallow the use of the non-overloaded `Flux#flatMap`.",

werli · 2021-12-31T09:14:46Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/FluxFlatMapUsageCheck.java

+  private static final Matcher<ExpressionTree> FLUX_FLATMAP =
+      instanceMethod()
+          .onExactClass("reactor.core.publisher.Flux")
+          .named("flatMap")
+          .withParameters("java.util.function.Function");


Why not the following? 🙂

Suggested change

private static final Matcher<ExpressionTree> FLUX_FLATMAP =

instanceMethod()

.onExactClass("reactor.core.publisher.Flux")

.named("flatMap")

.withParameters("java.util.function.Function");

private static final Matcher<ExpressionTree> FLUX_FLATMAP =

instanceMethod()

.onExactClass(Flux.class.getName())

.named("flatMap")

.withParameters(Function.class.getName());

This way we have to add the imports, which in this case is not a problem. Sometimes it can be prevent us from having to specify a dependency.

Haha, reverting the Flux.class.getName() one, because indeed; the RefasterResourceCompiler says ClassNotFoundException. (Although I'm not sure why this occurs because of the RRC?) @Stephan202.

This happens because io.projectreactor:reactor-core is declared as a provided dependency. @werli's suggestion would require us to move the dependency to the compile scope, which would cause it to be pulled in everywhere that error-prone-contrib is used. If we'd do that for all dependencies for which we have one or more BugPatterns, then this checker would become a very "heavy" dependency to add to a project.

(Replacing provided with test is also not possible because there are Refaster templates that also depend on io.projectreactor:reactor-core.)

Makes sense, thanks for the explanation @Stephan202 👍

werli · 2021-12-31T09:18:05Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/FluxFlatMapUsageCheck.java

+        .addFix(
+            SuggestedFix.builder()
+                .replace(tree, Util.treeToString(tree, state).replace("flatMap", "concatMap"))
+                .build())
+        .addFix(
+            SuggestedFix.builder()
+                .replace(
+                    parentExpression,
+                    getReplacementWithConcurrencyArgument(parentExpression, state))
+                .build())


IIRC, I've seen ~two purposeful usages of Flux#flatMap(Function, int) in all this time.
Shouldn't we simply automatically apply the flatMap -> concatMap fix? Everyone using Flux#flatMap(Function, int) looks to know what they're doing :)

Makes me think then: If we apply such a "trivial" fix, wouldn't this qualify as a Refaster rule then? 👀

Nice thoughts 😄 . The reason for making it a BugPattern is that we want to have the two suggestions. However, when we run patch, the first suggestion will always be used, which means that we'll always replace flatmap with concatMap.

The compiler error will now look like "Did you mean <FIX 1> or <FIX 2>?". So indeed, the first one will be most used, but we still wanted to give the Flux#flatMap(Function, int) as suggestion.

Makes sense 👍 Thx!

Stephan202

Added a commit. Maybe we should also flag the unary Flux#flatMapSequential?

Stephan202 · 2021-12-31T16:22:25Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/FluxFlatMapUsageCheck.java

+  private static final Matcher<ExpressionTree> FLUX_FLATMAP =
+      instanceMethod()
+          .onExactClass("reactor.core.publisher.Flux")
+          .named("flatMap")
+          .withParameters("java.util.function.Function");


This happens because io.projectreactor:reactor-core is declared as a provided dependency. @werli's suggestion would require us to move the dependency to the compile scope, which would cause it to be pulled in everywhere that error-prone-contrib is used. If we'd do that for all dependencies for which we have one or more BugPatterns, then this checker would become a very "heavy" dependency to add to a project.

(Replacing provided with test is also not possible because there are Refaster templates that also depend on io.projectreactor:reactor-core.)

Stephan202 · 2021-12-31T16:31:09Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/FluxFlatMapUsageCheck.java

+        .addFix(
+            SuggestedFix.builder()
+                .replace(tree, Util.treeToString(tree, state).replace("flatMap", "concatMap"))
+                .build())


This assumes that tree.getExpression() doesn't contain a reference to e.g. Mono.flatMap.

A more robust solution is to match on MethodInvocationTrees rather than MemberSelectTree and then use SuggestedFixes.renameMethodInvocation:

Suggested change

.addFix(

SuggestedFix.builder()

.replace(tree, Util.treeToString(tree, state).replace("flatMap", "concatMap"))

.build())

.addFix(SuggestedFixes.renameMethodInvocation(tree, "concatMap", state))

Stephan202 · 2021-12-31T16:36:02Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/FluxFlatMapUsageCheck.java

+                .replace(
+                    parentExpression,
+                    getReplacementWithConcurrencyArgument(parentExpression, state))


This approach relies on getReplacementWithConcurrencyArgument below, but if we match on MethodInvocationTrees as suggested above we can express the addition of an extra argument more straightforwardly:

Suggested change

.replace(

parentExpression,

getReplacementWithConcurrencyArgument(parentExpression, state))

.postfixWith(

Iterables.getOnlyElement(tree.getArguments()), ", " + NAME_CONCURRENCY_ARGUMENT)

Oh, that postfixWith is really useful, good one!

Stephan202 · 2021-12-31T16:57:24Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/FluxFlatMapUsageCheck.java

+    tags = StandardTags.LIKELY_ERROR)
+public final class FluxFlatMapUsageCheck extends BugChecker implements MemberSelectTreeMatcher {
+  private static final long serialVersionUID = 1L;
+  private static final String NAME_CONCURRENCY_ARGUMENT = "MAX_CONCURRENCY";


Suggested change

private static final String NAME_CONCURRENCY_ARGUMENT = "MAX_CONCURRENCY";

private static final String MAX_CONCURRENCY_ARG_NAME = "MAX_CONCURRENCY";

Stephan202 · 2021-12-31T16:58:08Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/FluxFlatMapUsageCheck.java

+import java.util.function.Function;
+import reactor.core.publisher.Flux;
+
+/** A {@link BugChecker} which flags usages of {@link Flux#flatMap(Function)}s. */


Suggested change

/** A {@link BugChecker} which flags usages of {@link Flux#flatMap(Function)}s. */

/** A {@link BugChecker} which flags usages of {@link Flux#flatMap(Function)}. */

Stephan202 · 2022-01-01T10:38:52Z

...rone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/FluxFlatMapUsageCheckTest.java

+            "import reactor.core.publisher.Flux;",
+            "",
+            "class A {",
+            "  void positive() {",


While I like the positive/negative split, we currently don't do this most other tests, instead relying on grouping with and without // BUG: Diagnostic contains:. The alternative can make it a bit easier to see that all permutations are tested. For now I'd stick with the status quo.

Stephan202 · 2022-01-01T10:43:45Z

...rone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/FluxFlatMapUsageCheckTest.java

+            "    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);",


This overload has the same implicit reliance on the default concurrency level as the unary variant. However, there is no more elaborate overload we can refer to. So perhaps we should just call out this observation in the main code, without further flagging this method.

Stephan202 · 2022-01-01T10:52:18Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/FluxFlatMapUsageCheck.java

+@BugPattern(
+    name = "FluxFlatMapUsage",
+    summary =
+        "`Flux#flatMap` is not allowed, please use `Flux#concatMap` or specify an argument for the concurrency.",


"Not allowed" is a bit too strong. How about:

Suggested change

"`Flux#flatMap` is not allowed, please use `Flux#concatMap` or specify an argument for the concurrency.",

"`Flux#flatMap` has subtle semantics; please use `Flux#concatMap` or explicitly specify the desired amount of concurrency",

(Other summaries currently don't end with a dot.)

Stephan202 · 2022-01-01T11:31:58Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/FluxFlatMapUsageCheck.java

+        "`Flux#flatMap` provides unbounded parallelism and is not guaranteed to be sequential. "
+            + "Therefore, we disallow the use of the non-overloaded `Flux#flatMap`.",


The parallelism isn't unbounded (it's Queues.SMALL_BUFFER_SIZE by default). Since currently we don't generate documentation, let's move this explanation to the class Javadoc, where we don't need to deal with string concatenation.

(We should of course look into generating a website with documentation; later 🙃.)

Stephan202 · 2022-01-01T12:02:14Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/FluxFlatMapUsageCheck.java

+    String parentString = Util.treeToString(parentExpression, state);
+    return String.format(
+        "%s, %s)",
+        parentString.substring(0, parentString.lastIndexOf(')')), NAME_CONCURRENCY_ARGUMENT);


In case the developer chooses a concurrency value of 1 then the result is equivalent to a concatMap; let's add a Refaster template for that.

rickie · 2022-01-02T14:15:03Z

Maybe we should also flag the unary Flux#flatMapSequential?

Based on the implementation of flatMapSequential, that might be a good idea. I'm curious why it hasn't been flagged as "potentially dangerous" yet? I think I saw it listed as alternative to flatMap because the naming was a lot clearer?

Btw: nice improvements!! 🚀

Stephan202 · 2022-01-02T14:21:37Z

I'm curious why it hasn't been flagged as "potentially dangerous" yet? I think I saw it listed as alternative to flatMap because the naming was a lot clearer?

I guess because flatMapSequential is uses less, and has one less caveat (because ordering is consistent).

rickie · 2022-01-02T15:06:51Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/FluxFlatMapUsageCheck.java

@@ -40,7 +40,7 @@
 @BugPattern(
    name = "FluxFlatMapUsage",
    summary =
-        "`Flux#flatMap` has subtle semantics; please use `Flux#concatMap` or explicitly specify the desired amount of concurrency",
+        "`Flux#flatMap` and `Flux#flatMapSequential` have subtle semantics; please use `Flux#concatMap` or explicitly specify the desired amount of concurrency",


Or: Flux#flatMap{,Sequential} ?

I think this is fine, but let's split the line.

rickie · 2022-01-02T15:17:53Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/FluxFlatMapUsageCheck.java

+ * A {@link BugChecker} which flags usages of {@link Flux#flatMap(Function)} and {@link
+ * Flux#flatMapSequential(Function)}.
+ *
+ * <p>{@link Flux#flatMap(Function)} and {@link Flux#flatMapSequential(Function)} eagerly perform up


Not entirely sure about this first two sentences, are they still correct 😬 .

Not quite; will push something :)

EnricSala

Very cool 👍 Will be nice not having to worry about this one anymore, when will it be released? 😄

EnricSala · 2022-01-02T17:55:42Z

...rone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/FluxFlatMapUsageCheckTest.java

+            "import reactor.core.publisher.Flux;",
+            "",
+            "class A {",
+            "  private static final int MAX_CONCURRENCY = 8;",


Just checking, is this constant necessary in the before case?

Yep. The suggested fix does not introduce this constant, so strictly speaking it yields non-compilable code, which refactoringTestHelper doesn't like. So we introduce this constant to work around that.

(In theory we could update the code to suggest a MAX_CONCURRENCY constant, but that's not worth the hassle.)

Stephan202

Added one more commit :)

Suggested commit message:

Introduce `FluxFlatMapUsageCheck` (#26)

@werli @nathankooij anything (else) from your side?

when will it be released?

I estimate we're ~1-2 months away from officially enabling Error Prone Support in PSM... 🤞

Stephan202 · 2022-01-02T21:10:12Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/FluxFlatMapUsageCheck.java

+ * A {@link BugChecker} which flags usages of {@link Flux#flatMap(Function)} and {@link
+ * Flux#flatMapSequential(Function)}.
+ *
+ * <p>{@link Flux#flatMap(Function)} and {@link Flux#flatMapSequential(Function)} eagerly perform up


Not quite; will push something :)

Stephan202 · 2022-01-02T21:11:06Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/FluxFlatMapUsageCheck.java

@@ -40,7 +40,7 @@
 @BugPattern(
    name = "FluxFlatMapUsage",
    summary =
-        "`Flux#flatMap` has subtle semantics; please use `Flux#concatMap` or explicitly specify the desired amount of concurrency",
+        "`Flux#flatMap` and `Flux#flatMapSequential` have subtle semantics; please use `Flux#concatMap` or explicitly specify the desired amount of concurrency",


I think this is fine, but let's split the line.

Stephan202 · 2022-01-02T21:15:43Z

...rone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/FluxFlatMapUsageCheckTest.java

+            "import reactor.core.publisher.Flux;",
+            "",
+            "class A {",
+            "  private static final int MAX_CONCURRENCY = 8;",


Yep. The suggested fix does not introduce this constant, so strictly speaking it yields non-compilable code, which refactoringTestHelper doesn't like. So we introduce this constant to work around that.

(In theory we could update the code to suggest a MAX_CONCURRENCY constant, but that's not worth the hassle.)

werli

Nothing more from my side 👍 Nice to see flatMapSequential also being covered 🚀

nathankooij

Just one question but already approving :) Nice work!

nathankooij · 2022-01-14T09:12:05Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/FluxFlatMapUsageCheck.java

+ * <p>{@link Flux#flatMap(Function)} and {@link Flux#flatMapSequential(Function)} eagerly perform up
+ * to {@link reactor.util.concurrent.Queues#SMALL_BUFFER_SIZE} subscriptions. Additionally, the
+ * former interleaves values as they are emitted, yielding nondeterministic results. In most cases
+ * {@link Flux#concatMap(Function)} should be preferred, as it produces consistent results and


Should we then also not include {concat,flat}MapIterable for that reason? Or is this one problematic because there's no concurrency overload?

Note that unlike #flatMap(Function) and #concatMap(Function), with Iterable there is no notion of eager vs lazy inner subscription. The content of the Iterables are all played sequentially. Thus flatMapIterable and concatMapIterable are equivalent offered as a discoverability improvement for users that explore the API with the concat vs flatMap expectation.

Good one, I think it would make sense to just pick one (concat?) to keep usages uniform 👍

So IIUC:

{concat,flat}MapIterable do not suffer from the issue with eager subscription.

{concat,flat}MapIterable both emit values in a deterministic order.

Based on this I guess all we need is a Refaster check to replace flatMapIterable with concatMapIterable. I guess strictly speaking that's out of scope for this PR, but let's just add it for completeness 😄

Pushed a commit 😄 !

Stephan202

Rebased and added a small commit.

Stephan202 · 2022-01-15T11:31:10Z

...r-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/ReactorTemplates.java

+   * Prefer {@link Flux#concatMapIterable(Function)} over {@link Flux#concatMapIterable(Function)}
+   * to be consistent with {@link FluxFlatMapUsageCheck}.
+   *
+   * <p>NB: Both implementations emit values in a deterministic order and there is no difference
+   * with eager or lazy inner subscriptions. This means that both implementations are *equivalent*.


We can express this more concisely:

Suggested change

* Prefer {@link Flux#concatMapIterable(Function)} over {@link Flux#concatMapIterable(Function)}

* to be consistent with {@link FluxFlatMapUsageCheck}.

*

* <p>NB: Both implementations emit values in a deterministic order and there is no difference

* with eager or lazy inner subscriptions. This means that both implementations are *equivalent*.

* Prefer {@link Flux#concatMapIterable(Function)} over {@link Flux#concatMapIterable(Function)},

* as the former has equivalent semantics but a clearer name.

rickie requested review from Stephan202, EnricSala and nathankooij December 30, 2021 12:30

werli reviewed Dec 31, 2021

View reviewed changes

rickie force-pushed the rossendrijver/disallow_flux_flatmap branch from 27b57a5 to e89e5db Compare December 31, 2021 10:46

Stephan202 reviewed Jan 1, 2022

View reviewed changes

rickie force-pushed the rossendrijver/disallow_flux_flatmap branch 2 times, most recently from 57d559e to edf932c Compare January 2, 2022 15:16

rickie commented Jan 2, 2022

View reviewed changes

EnricSala approved these changes Jan 2, 2022

View reviewed changes

Stephan202 approved these changes Jan 2, 2022

View reviewed changes

werli approved these changes Jan 3, 2022

View reviewed changes

nathankooij approved these changes Jan 14, 2022

View reviewed changes

rickie and others added 8 commits January 15, 2022 12:27

Introduce FluxFlatMapUsageCheck

bb8b8dc

Apply suggestions

e29539c

Suggestion

5486c77

Suggestions

516d3f9

Also flag Flux#flatMapSequential

a30eb0b

Suggestions

ba13b73

Introduce template to rewrite flatMapIterable to concatMapIterable

5d362c2

Suggestion

f68492e

Stephan202 force-pushed the rossendrijver/disallow_flux_flatmap branch from 96af19b to f68492e Compare January 15, 2022 11:32

Stephan202 approved these changes Jan 15, 2022

View reviewed changes

Stephan202 merged commit 63b4fae into master Jan 15, 2022

Stephan202 deleted the rossendrijver/disallow_flux_flatmap branch January 15, 2022 11:39

Stephan202 added this to the 0.1.0 milestone Apr 10, 2022

Stephan202 mentioned this pull request Jul 31, 2022

Introduce {Mono,Flux}Map{,NotNull} Refaster rules #142

Merged

	private static final String NAME_CONCURRENCY_ARGUMENT = "MAX_CONCURRENCY";
	private static final String MAX_CONCURRENCY_ARG_NAME = "MAX_CONCURRENCY";

	/** A {@link BugChecker} which flags usages of {@link Flux#flatMap(Function)}s. */
	/** A {@link BugChecker} which flags usages of {@link Flux#flatMap(Function)}. */

	"`Flux#flatMap` is not allowed, please use `Flux#concatMap` or specify an argument for the concurrency.",
	"`Flux#flatMap` has subtle semantics; please use `Flux#concatMap` or explicitly specify the desired amount of concurrency",

		"`Flux#flatMap` provides unbounded parallelism and is not guaranteed to be sequential. "
		+ "Therefore, we disallow the use of the non-overloaded `Flux#flatMap`.",

Introduce FluxFlatMapUsageCheck #26

Introduce FluxFlatMapUsageCheck #26

Conversation

rickie commented Dec 30, 2021

werli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rickie Dec 31, 2021 • edited Loading

Choose a reason for hiding this comment

werli Dec 31, 2021 • edited Loading

Choose a reason for hiding this comment

Stephan202 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rickie commented Jan 2, 2022 • edited Loading

Stephan202 commented Jan 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EnricSala left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stephan202 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

werli left a comment

Choose a reason for hiding this comment

nathankooij left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stephan202 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Introduce `FluxFlatMapUsageCheck` #26

Introduce `FluxFlatMapUsageCheck` #26

rickie Dec 31, 2021 •

edited

Loading

werli Dec 31, 2021 •

edited

Loading

rickie commented Jan 2, 2022 •

edited

Loading