Introduce `NonEmptyMono` check #200

cernat-catalin · 2022-08-16T18:58:06Z

Avoid doing Flux#collect(...) (and similar collector methods) followed by methods that check for an empty source (e.g. Mono#single(), Mono#defaultIfEmpty(...), Mono#switchIfEmpty) as collect method on a flux always emit a value (empty collection for an empty flux)

See discussion here: https://teampicnic.slack.com/archives/C0172BMUWNL/p1660557240599619

Stephan202 · 2022-08-16T19:50:34Z

Niiice! Won't have time right now to review, but love the simplicity! 🚀 Will try to review in the coming day(s) :)

Stephan202

Rebased and added a commit. Did not yet review the Javadoc and tests; pondering whether we can generalize even a bit more... stay tuned :)

Stephan202 · 2022-08-17T18:44:29Z

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

+    tags = SIMPLIFICATION)
+public final class FluxCollect extends BugChecker implements MethodInvocationTreeMatcher {
+  private static final long serialVersionUID = 1L;
+  private static final Matcher<ExpressionTree> FLUX_EMPTY_CHECK =


Suggested change

private static final Matcher<ExpressionTree> FLUX_EMPTY_CHECK =

private static final Matcher<ExpressionTree> MONO_SIZE_CHECK =

Stephan202 · 2022-08-17T18:45:12Z

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

+      instanceMethod()
+          .onDescendantOf("reactor.core.publisher.Flux")
+          .namedAnyOf(
+              "collect", "collectList", "collectSortedList", "collectMap", "collectMultimap");


Let's sort 😄

Stephan202 · 2022-08-17T18:52:29Z

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

+    if (tree.getMethodSelect().getKind() != MEMBER_SELECT) {
+      return Description.NO_MATCH;
+    }
+
+    ExpressionTree selectExpression = ((MemberSelectTree) tree.getMethodSelect()).getExpression();


We can instead us ASTHelpers.getReceiver(tree) :)

Stephan202

Added a commit with a proposed generalization; PTAL.

The idea is not to pick up the XXX comments in this PR: before doing so I would propose to look into a kind of "calculus" for reactive streams. That said, if this sounds interesting: let's talk! 😄

Did not yet review the tests. I suspect we should condense the test cases rather than ~quadruple the current amount. Let me know what you think :)

rickie

Flushing my comments. Almost finished with a few test improvements as we often limit the amount of entries in the replacement test. Will continue tomorrow :).

Very nice check @cernat-catalin 😄:rocket:!

rickie · 2022-08-17T20:06:25Z

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

+  private static final Matcher<ExpressionTree> MONO_SIZE_CHECK =
+      instanceMethod()
+          .onDescendantOf("reactor.core.publisher.Mono")
+          .namedAnyOf("single", "defaultIfEmpty", "switchIfEmpty");


Let's sort them :).

rickie · 2022-08-17T20:15:04Z

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

+                  "last",
+                  "reduce",
+                  "reduceWith",
+                  "single"),


I have to ask, although I'm not 100% I understand the method, is there a reason for not including shareNext?

It's not included because that method might return a Mono that completes empty. This test passes:

Flux.empty().shareNext().as(StepVerifier::create).verifyComplete();

rickie · 2022-08-17T20:22:24Z

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

+      instanceMethod()
+          .onDescendantOf("reactor.core.publisher.Mono")
+          .namedAnyOf("single", "defaultIfEmpty", "switchIfEmpty");
+  private static final Matcher<ExpressionTree> SINGLETON_MONO =


Not sure if SINGLETON is the most ideal name here 🤔. Will try to come up with a different name tomorrow.

NON_EMPTY_MONO would match the class name. (Would actually prefer NONEMPTY_MONO, but that doesn't quite match the type name and some other usages.)

Stephan202

Rebased and added a commit. Looking forward to the test tweaks 😄

Stephan202 · 2022-08-18T05:57:13Z

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

+      instanceMethod()
+          .onDescendantOf("reactor.core.publisher.Mono")
+          .namedAnyOf("single", "defaultIfEmpty", "switchIfEmpty");
+  private static final Matcher<ExpressionTree> SINGLETON_MONO =


NON_EMPTY_MONO would match the class name. (Would actually prefer NONEMPTY_MONO, but that doesn't quite match the type name and some other usages.)

Stephan202 · 2022-08-18T05:59:15Z

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

+                  "last",
+                  "reduce",
+                  "reduceWith",
+                  "single"),


It's not included because that method might return a Mono that completes empty. This test passes:

Flux.empty().shareNext().as(StepVerifier::create).verifyComplete();

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

cernat-catalin · 2022-08-18T07:15:25Z

Very nice additions @Stephan202 ! It is starting to become a very encompassing BugPattern

rickie · 2022-08-18T08:13:11Z

Pushed a tweak for the replacement test. I think it is enough to test one Flux and one Mono case w.r.t. the replacement.

I was working on a proposal for the identification part as well. However, I wasn't too sure how to proceed here. It's a bit too much to add tests for all variants + the three methods we flag.
We can also leave it at this for now. WDYT?

TBH, I think it'd be okay to remove some of the entries in the identification method. WDYT?

Stephan202 · 2022-08-18T08:54:25Z

TBH, I think it'd be okay to remove some of the entries in the identification method. WDYT?

I would be in favour of that. Would I'd probably have one line for each method in NON_EMPTY_MONO, and then cycle through MONO_SIZE_CHECK. So in total that'd be NON_EMPTY_MONO.size() cases.

cernat-catalin · 2022-08-18T11:53:16Z

Thanks both!

I've pushed a commit which rewrites the entries in the identification method. Now there are NON_EMPTY_MONO.size() entries.

However, I've removed from the NON_EMPTY_MONO set the methods defaultIfEmpty (as it produced a Flux not a Mono) and reduce (as there is an overload of this method which does not take an initial value thus the operation might indeed complete empty). There reduce method could be treated as a special case after the standard checks. The defaultIfEmpty one will need more generalization (I don't have time today or tomorrow though). WDYT?

rickie · 2022-08-19T14:26:17Z

as there is an overload of this method which does not take an initial value thus the operation might indeed complete empty). There reduce method could be treated as a special case after the standard checks.

Good catch, added a more specific part to the Matcher to only flag the correct one.

The defaultIfEmpty one will need more generalization (I don't have time today or tomorrow though). WDYT?

I think it's fair to leave this one out for now. This one requires some extra thought.

I added three extra test cases for the Monos as well. Also added a number to the Flux.just() occurrences. Currently the preferred way is to use Flux.just(<number>), although the existing suggestions weren't wrong. Just to be consistent I updated the tests.

Stephan202

However, I've removed from the NON_EMPTY_MONO set the methods defaultIfEmpty (as it produced a Flux not a Mono)

Check, that would be a type change. I rebased and added a comment about this, because ideally we introduce a generalization which does cover this case.

Next up: reviewing the tests.

rickie

Really cool check @cernat-catalin, I'm curious what the downstream impact will be! 🚀

Suggested commit message:

Introduce `NonEmptyMono` check (#200)

P.s: Notice the nice PR number 😄.

Stephan202

Rebased and added a commit with test cases for other overloads.

Very nice check!

Stephan202 · 2022-08-20T08:54:07Z

error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/NonEmptyMonoTest.java

+            "import reactor.core.publisher.Mono;",
+            "",
+            "class A {",
+            "  void m() {",


We should also add a few more expressions that aren't flagged.

cernat-catalin · 2022-08-20T09:13:23Z

Cool stuff! Thanks @rickie and @Stephan202 for the latest additions.
I'm really happy with how this one turned out (looking forward to the generalization).

P.s: Notice the nice PR number 😄.

Nice round number!

rickie · 2022-08-20T09:19:23Z

Latest changes LGTM!

I'm really happy with how this one turned out

Me too! 😄

cernat-catalin requested review from Stephan202 and rickie August 16, 2022 18:58

cernat-catalin force-pushed the cernat-catalin/flux_collect branch 3 times, most recently from 08de097 to bc832c4 Compare August 16, 2022 21:05

rickie changed the title ~~FluxCollect BugPattern~~ Introduce FluxCollect BugPattern Aug 17, 2022

Stephan202 force-pushed the cernat-catalin/flux_collect branch from bc832c4 to d81a6c4 Compare August 17, 2022 19:01

Stephan202 reviewed Aug 17, 2022

View reviewed changes

rickie reviewed Aug 17, 2022

View reviewed changes

Stephan202 force-pushed the cernat-catalin/flux_collect branch from 39e4023 to 2319bfe Compare August 18, 2022 06:01

Stephan202 reviewed Aug 18, 2022

View reviewed changes

rickie force-pushed the cernat-catalin/flux_collect branch from 57bf773 to ad67fe9 Compare August 19, 2022 13:42

Stephan202 force-pushed the cernat-catalin/flux_collect branch from d858ec2 to f75bd23 Compare August 19, 2022 17:28

Stephan202 reviewed Aug 19, 2022

View reviewed changes

rickie approved these changes Aug 20, 2022

View reviewed changes

rickie added this to the 0.1.1 milestone Aug 20, 2022

cernat-catalin and others added 7 commits August 20, 2022 10:54

FluxCollect BugPattern

b0295a8

Format code in test file

08e2a89

Suggestions

60667b3

Generalize

a4f30b8

Tiny tweak

c771a0d

Rename constant

076ef41

Suggestions for replacement test

9dd7eb5

cernat-catalin and others added 4 commits August 20, 2022 10:54

Rewerite identification tests

a3d3410

Suggestions for identification test

d50858e

Add another XXX comment

d6e16f7

Suggestions

1c832a8

Stephan202 force-pushed the cernat-catalin/flux_collect branch from f75bd23 to 1c832a8 Compare August 20, 2022 08:54

Stephan202 approved these changes Aug 20, 2022

View reviewed changes

Stephan202 merged commit b8e22ff into master Aug 20, 2022

Stephan202 deleted the cernat-catalin/flux_collect branch August 20, 2022 10:47

rickie changed the title ~~Introduce FluxCollect BugPattern~~ Introduce NonEmptyMono check Aug 23, 2022

rickie added the enhancement label Aug 24, 2022

rickie mentioned this pull request Aug 24, 2022

Introduce release.yml to improve GitHub release notes generation #213

Merged

rickie added the new feature label Aug 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce `NonEmptyMono` check #200

Introduce `NonEmptyMono` check #200

cernat-catalin commented Aug 16, 2022

Stephan202 commented Aug 16, 2022

Stephan202 left a comment

Stephan202 Aug 17, 2022

Stephan202 Aug 17, 2022

Stephan202 Aug 17, 2022

Stephan202 left a comment

rickie left a comment

rickie Aug 17, 2022

rickie Aug 17, 2022

Stephan202 Aug 18, 2022

rickie Aug 17, 2022

Stephan202 Aug 18, 2022

Stephan202 left a comment

Stephan202 Aug 18, 2022

Stephan202 Aug 18, 2022

cernat-catalin commented Aug 18, 2022

rickie commented Aug 18, 2022 •

edited

Loading

Stephan202 commented Aug 18, 2022

cernat-catalin commented Aug 18, 2022

rickie commented Aug 19, 2022

Stephan202 left a comment

rickie left a comment

Stephan202 left a comment

Stephan202 Aug 20, 2022

cernat-catalin commented Aug 20, 2022

rickie commented Aug 20, 2022 •

edited

Loading

	private static final Matcher<ExpressionTree> FLUX_EMPTY_CHECK =
	private static final Matcher<ExpressionTree> MONO_SIZE_CHECK =

Introduce NonEmptyMono check #200

Introduce NonEmptyMono check #200

Conversation

cernat-catalin commented Aug 16, 2022

Stephan202 commented Aug 16, 2022

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

Stephan202 left a comment

Choose a reason for hiding this comment

rickie 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

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

cernat-catalin commented Aug 18, 2022

rickie commented Aug 18, 2022 • edited Loading

Stephan202 commented Aug 18, 2022

cernat-catalin commented Aug 18, 2022

rickie commented Aug 19, 2022

Stephan202 left a comment

Choose a reason for hiding this comment

rickie left a 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

cernat-catalin commented Aug 20, 2022

rickie commented Aug 20, 2022 • edited Loading

Introduce `NonEmptyMono` check #200

Introduce `NonEmptyMono` check #200

rickie commented Aug 18, 2022 •

edited

Loading

rickie commented Aug 20, 2022 •

edited

Loading