Introduce `NestedPublishers` check #642

mohamedsamehsalah · 2023-05-21T19:49:18Z

Bug checker that gives a warning when nested publishers are used.

Suggested commit message:

Introduce `NestedPublishers` check (#642)

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java

github-actions · 2023-05-21T19:51:29Z

Looks good. All 5 mutations in this change were killed.

class	surviving	killed
🎉`tech.picnic.errorprone.bugpatterns.MonoOfPublishers`	0	5

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

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

github-actions · 2023-05-30T19:40:51Z

Surviving mutants in this change: 1
Killed mutants in this change: 13

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.NestedPublishers`	1	13

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

mohamedsamehsalah

Proposal from pair-programming session with @rickie

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

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java

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

github-actions · 2023-05-31T20:06:09Z

Looks good. All 18 mutations in this change were killed.

class	surviving	killed
🎉`tech.picnic.errorprone.bugpatterns.NestedPublishers`	0	18

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

github-actions · 2023-05-31T20:11:53Z

Looks good. All 18 mutations in this change were killed.

class	surviving	killed
🎉`tech.picnic.errorprone.bugpatterns.NestedPublishers`	0	18

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

Venorcis

LGTM 👍🏻

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

github-actions · 2023-06-01T11:18:41Z

Looks good. All 18 mutations in this change were killed.

class	surviving	killed
🎉`tech.picnic.errorprone.bugpatterns.NestedPublishers`	0	18

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

rickie

Rebased and added a commit.

Nice changes @mohamedsamehsalah 😄, let's get your first BugPattern merged 💪🏻!

Thanks for the review @Venorcis 💪🏻 !

Suggested commit message:

Introduce `NestedPublishers` check (#642)

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

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

rickie · 2023-06-07T10:51:49Z

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

+  private static boolean isTypeArgumentGroupedFlux(
+      VisitorState state, @Nullable Type groupedFluxType, Type treeType) {
+    return groupedFluxType != null
+        && treeType.getTypeArguments().stream()


We can assume that Publisher has only one type, so we can use something like Iterables#getONlyElement here :)

So we should either do:

isSubType(type.getTypeArguments().get(0), GROUPED_FLUX.get(state), state) // or isSubType(Iterables.getOnlyElement(type.getTypeArguments()), GROUPED_FLUX.get(state), state) // or have: List<Type> typeArguments = type.getTypeArguments(); .... isSubType(typeArguments.get(0), GROUPED_FLUX.get(state), state)

Not sure what the best option is though.... Will push a suggestion but feel free to change.

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

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java

rickie · 2023-06-07T14:01:49Z

(Okay, there was an issue with my internet, I reviewed + approved twice all at once ????, sorry for that).

github-actions · 2023-06-07T14:03:09Z

Surviving mutants in this change: 1
Killed mutants in this change: 11

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.NestedPublishers`	1	11

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

mohamedsamehsalah · 2023-07-02T13:18:29Z

Rebased ✅

github-actions · 2023-07-02T13:19:56Z

Looks good. All 18 mutations in this change were killed.

class	surviving	killed
🎉`tech.picnic.errorprone.bugpatterns.NestedPublishers`	0	18

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

mohamedsamehsalah · 2023-07-16T15:15:47Z

Error:  Failed to execute goal on project documentation-support: Could not resolve dependencies for project tech.picnic.error-prone-support:documentation-support:jar:0.12.1-SNAPSHOT: Could not find artifact com.github.PicnicSupermarket.error-prone:error_prone_annotation:jar:v2.20.0-picnic-1 in jitpack.io (https://jitpack.io/)

~~Also getting this error locally 😬~~

~~Apparently this is happenning for all of the project's PR builds.~~

~~CC: @rickie~~

Resolved ✅

github-actions · 2023-07-22T21:17:21Z

Surviving mutants in this change: 1
Killed mutants in this change: 11

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.NestedPublishers`	1	11

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

rickie

Sorry for the late re-approve but it looks goood! ✅

Thanks @mohamedsamehsalah 🚀 !

github-actions · 2023-08-11T12:53:31Z

Surviving mutants in this change: 1
Killed mutants in this change: 11

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.NestedPublishers`	1	11

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

Stephan202

Added a commit with quite some changes; consider re-reviewing :)

Suggested commit message:

Introduce `NestedPublishers` check (#642)

And introduce a few utility methods that simplify `Tree`- and `Type`-based
matching, allowing the existing (and similar) `NestedOptionals` check to be
simplified as well.

Stephan202 · 2023-08-11T18:02:59Z

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

+  }
+
+  private static boolean isSubType(Type subType, @Nullable Type type, VisitorState state) {
+    return type != null && state.getTypes().isSubtype(subType, type);


We can deduplicate the null check by pulling it up. That also prevents IDEA from warning about the type.getTypeArguments() call.

That said, in practice (see e.g. NestedOptionals) we often omit this null check, so apparently it's redundant. Omitting has the benefit that Pitest won't complain. (But then of IDEA will still complain. Trade-off.)

Another way to make sure that the null check doesn't need to be performed explicitly, is by using ASTHelpers.isSubtype. Unfortunately that method performs type erasure, which makes it unsuitable. That's actually quite a gotcha, which also impacts Matchers#isSubtypeOf(Supplier). What I'd like to do is define a corresponding MoreMatchers variant; then we can also further simplify NestedOptionals.

Stephan202 · 2023-08-11T18:08:41Z

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

+  private static final long serialVersionUID = 1L;
+  private static final Supplier<Type> PUBLISHER = type("org.reactivestreams.Publisher");
+  private static final Supplier<Type> PUBLISHER_OF_PUBLISHERS =
+      VisitorState.memoize(generic(PUBLISHER, subOf(generic(PUBLISHER, unbound()))));


I think we should also match raw inner publishers, just like in NestedOptionals. Likewise below. (And then update the tests to cover these cases.)

Stephan202 · 2023-08-11T18:11:23Z

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

+            "    // BUG: Diagnostic contains:",
+            "    Mono.just(Mono.empty());",
+            "    // BUG: Diagnostic contains:",
+            "    Mono.just(Mono.just(1));",


We can drop a few of these tests, since they produce exactly the same type of expression.

Stephan202 · 2023-08-11T18:17:56Z

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

+  public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
+    Type type = ASTHelpers.getType(tree);
+
+    if (!isSubType(type, PUBLISHER_OF_PUBLISHERS.get(state), state)


PUBLISHER_OF_PUBLISHERS.get(state) can be null, causing an NPE. See NestedOptionalsTest#identificationOptionalTypeNotLoaded() for a similar case.

Stephan202 · 2023-08-11T19:37:27Z

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

+  public NestedPublishers() {}
+
+  @Override
+  public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {


(Also holds for NestedOptionals): next to method invocations we should perhaps also flag e.g. return types and variable types. 🤔 Perhaps we can keep it out of scope for this PR by only adding an XXX comment. After all, most (but not all) such definitions would involve a method invocation of the same type.

Stephan202 · 2023-08-11T19:37:42Z

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

+import org.jspecify.annotations.Nullable;
+import org.reactivestreams.Publisher;
+
+/** A {@link BugChecker} that flags nesting of {@link Publisher Publishers}. */


Let's call out the GroupedFlux exclusion.

github-actions · 2023-08-12T09:50:57Z

Looks good. All 23 mutations in this change were killed.

class	surviving	killed
🎉`tech.picnic.errorprone.bugpatterns.util.MoreMatchers`	0	2
🎉`tech.picnic.errorprone.bugpatterns.util.MoreTypePredicates`	0	15
🎉`tech.picnic.errorprone.bugpatterns.NestedPublishers`	0	3
🎉`tech.picnic.errorprone.bugpatterns.NestedOptionals`	0	3

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

Venorcis

Still LGTM 👍🏻

github-actions · 2023-08-14T05:46:57Z

Looks good. All 23 mutations in this change were killed.

class	surviving	killed
🎉`tech.picnic.errorprone.bugpatterns.util.MoreMatchers`	0	2
🎉`tech.picnic.errorprone.bugpatterns.util.MoreTypePredicates`	0	15
🎉`tech.picnic.errorprone.bugpatterns.NestedPublishers`	0	3
🎉`tech.picnic.errorprone.bugpatterns.NestedOptionals`	0	3

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

rickie

Added a tiny Javadoc tweak. Really nice changes, we'll be able to use this in many more future cases 🚀 !

github-actions · 2023-08-14T06:05:20Z

Looks good. All 23 mutations in this change were killed.

class	surviving	killed
🎉`tech.picnic.errorprone.bugpatterns.util.MoreMatchers`	0	2
🎉`tech.picnic.errorprone.bugpatterns.util.MoreTypePredicates`	0	15
🎉`tech.picnic.errorprone.bugpatterns.NestedPublishers`	0	3
🎉`tech.picnic.errorprone.bugpatterns.NestedOptionals`	0	3

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

rickie · 2023-08-14T07:58:59Z

Rebased, will merge once 🟢.

sonarcloud · 2023-08-14T08:01:09Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

97.4% Coverage
0.0% Duplication

github-actions · 2023-08-14T08:02:01Z

Looks good. All 23 mutations in this change were killed.

class	surviving	killed
🎉`tech.picnic.errorprone.bugpatterns.util.MoreMatchers`	0	2
🎉`tech.picnic.errorprone.bugpatterns.util.MoreTypePredicates`	0	15
🎉`tech.picnic.errorprone.bugpatterns.NestedPublishers`	0	3
🎉`tech.picnic.errorprone.bugpatterns.NestedOptionals`	0	3

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

mohamedsamehsalah added this to the 0.12.0 milestone May 21, 2023

mohamedsamehsalah commented May 21, 2023

View reviewed changes

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java Outdated Show resolved Hide resolved

mohamedsamehsalah requested review from Stephan202, rickie and werli and removed request for werli May 22, 2023 06:43

Venorcis reviewed May 23, 2023

View reviewed changes

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MonoOfPublishers.java Outdated Show resolved Hide resolved

rickie added the new feature label May 26, 2023

mohamedsamehsalah force-pushed the mohamedsamehsalah/mono-of-publishers branch from d6838c0 to a27838d Compare May 30, 2023 19:38

mohamedsamehsalah commented May 30, 2023

View reviewed changes

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/NestedPublishers.java Outdated Show resolved Hide resolved

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java Show resolved Hide resolved

mohamedsamehsalah requested a review from Venorcis May 30, 2023 19:42

Venorcis reviewed May 31, 2023

View reviewed changes

mohamedsamehsalah requested a review from Venorcis May 31, 2023 20:05

Venorcis approved these changes Jun 1, 2023

View reviewed changes

error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/NestedPublishersTest.java Outdated Show resolved Hide resolved

rickie force-pushed the mohamedsamehsalah/mono-of-publishers branch from 229226e to a654657 Compare June 7, 2023 14:00

rickie reviewed Jun 7, 2023

View reviewed changes

This comment was marked as duplicate.

Sign in to view

Stephan202 modified the milestones: 0.12.0, 0.13.0 Jun 21, 2023

mohamedsamehsalah force-pushed the mohamedsamehsalah/mono-of-publishers branch from a654657 to c920892 Compare July 2, 2023 13:17

mohamedsamehsalah force-pushed the mohamedsamehsalah/mono-of-publishers branch from 23bc818 to 10d4173 Compare July 22, 2023 21:14

rickie approved these changes Aug 11, 2023

View reviewed changes

rickie force-pushed the mohamedsamehsalah/mono-of-publishers branch from 10d4173 to 4547ce9 Compare August 11, 2023 12:50

Stephan202 approved these changes Aug 12, 2023

View reviewed changes

Stephan202 mentioned this pull request Aug 12, 2023

DirectReturn bugpattern clash with lombok's @Data #728

Closed

Venorcis approved these changes Aug 13, 2023

View reviewed changes

rickie force-pushed the mohamedsamehsalah/mono-of-publishers branch from b4ce298 to 7b7987a Compare August 14, 2023 05:43

rickie approved these changes Aug 14, 2023

View reviewed changes

mohamedsamehsalah and others added 9 commits August 14, 2023 09:58

Introduce MonoOfPublishers bug check

d072cf6

Proposal

720fa6b

Address comments to improve readability

146c5d3

Add @nullable annotation

c232504

Remove unnecessary test cases

a508300

Address SC's code smell

927bf8f

Re-apply @rickie suggestions

1a656d3

Suggestions

3ca3b3c

Doc tweak

de119dd

rickie force-pushed the mohamedsamehsalah/mono-of-publishers branch from ad7debd to de119dd Compare August 14, 2023 07:58

rickie merged commit 4af7b21 into master Aug 14, 2023
17 checks passed

rickie deleted the mohamedsamehsalah/mono-of-publishers branch August 14, 2023 08:30

Stephan202 changed the title ~~Introduce NestedPublishers bug check~~ Introduce NestedPublishers check Aug 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce `NestedPublishers` check #642

Introduce `NestedPublishers` check #642

mohamedsamehsalah commented May 21, 2023 •

edited

Loading

github-actions bot commented May 21, 2023

github-actions bot commented May 30, 2023

mohamedsamehsalah left a comment

github-actions bot commented May 31, 2023

github-actions bot commented May 31, 2023

Venorcis left a comment

github-actions bot commented Jun 1, 2023

rickie left a comment

rickie Jun 7, 2023

This comment was marked as duplicate.

This comment was marked as duplicate.

rickie commented Jun 7, 2023

github-actions bot commented Jun 7, 2023

mohamedsamehsalah commented Jul 2, 2023

github-actions bot commented Jul 2, 2023

mohamedsamehsalah commented Jul 16, 2023 •

edited

Loading

github-actions bot commented Jul 22, 2023

rickie left a comment

github-actions bot commented Aug 11, 2023

Stephan202 left a comment

Stephan202 Aug 11, 2023

Stephan202 Aug 11, 2023

Stephan202 Aug 11, 2023

Stephan202 Aug 11, 2023

Stephan202 Aug 11, 2023

Stephan202 Aug 11, 2023

github-actions bot commented Aug 12, 2023

Venorcis left a comment

github-actions bot commented Aug 14, 2023

rickie left a comment

github-actions bot commented Aug 14, 2023

rickie commented Aug 14, 2023

sonarcloud bot commented Aug 14, 2023

github-actions bot commented Aug 14, 2023

Introduce NestedPublishers check #642

Introduce NestedPublishers check #642

Conversation

mohamedsamehsalah commented May 21, 2023 • edited Loading

github-actions bot commented May 21, 2023

github-actions bot commented May 30, 2023

mohamedsamehsalah left a comment

Choose a reason for hiding this comment

github-actions bot commented May 31, 2023

github-actions bot commented May 31, 2023

Venorcis left a comment

Choose a reason for hiding this comment

github-actions bot commented Jun 1, 2023

rickie left a comment

Choose a reason for hiding this comment

rickie Jun 7, 2023

Choose a reason for hiding this comment

This comment was marked as duplicate.

This comment was marked as duplicate.

rickie commented Jun 7, 2023

github-actions bot commented Jun 7, 2023

mohamedsamehsalah commented Jul 2, 2023

github-actions bot commented Jul 2, 2023

mohamedsamehsalah commented Jul 16, 2023 • edited Loading

github-actions bot commented Jul 22, 2023

rickie left a comment

Choose a reason for hiding this comment

github-actions bot commented Aug 11, 2023

Stephan202 left a comment

Choose a reason for hiding this comment

Stephan202 Aug 11, 2023

Choose a reason for hiding this comment

Stephan202 Aug 11, 2023

Choose a reason for hiding this comment

Stephan202 Aug 11, 2023

Choose a reason for hiding this comment

Stephan202 Aug 11, 2023

Choose a reason for hiding this comment

Stephan202 Aug 11, 2023

Choose a reason for hiding this comment

Stephan202 Aug 11, 2023

Choose a reason for hiding this comment

github-actions bot commented Aug 12, 2023

Venorcis left a comment

Choose a reason for hiding this comment

github-actions bot commented Aug 14, 2023

rickie left a comment

Choose a reason for hiding this comment

github-actions bot commented Aug 14, 2023

rickie commented Aug 14, 2023

sonarcloud bot commented Aug 14, 2023

github-actions bot commented Aug 14, 2023

Introduce `NestedPublishers` check #642

Introduce `NestedPublishers` check #642

mohamedsamehsalah commented May 21, 2023 •

edited

Loading

mohamedsamehsalah commented Jul 16, 2023 •

edited

Loading