Proposal

PicnicSupermarket · May 30, 2023 · a27838d · a27838d
1 parent a9584b8
commit a27838d
Show file tree

Hide file tree

Showing 3 changed files with 48 additions and 17 deletions.
diff --git a/...orprone/bugpatterns/MonoOfPublishers.java → ...orprone/bugpatterns/NestedPublishers.java b/...orprone/bugpatterns/MonoOfPublishers.java → ...orprone/bugpatterns/NestedPublishers.java
@@ -30,29 +30,45 @@
 @AutoService(BugChecker.class)
 @BugPattern(
     summary =
-        "Avoid nesting `Publisher`s inside `Mono`s; the resultant code is hard to reason about",
-    link = BUG_PATTERNS_BASE_URL + "MonoOfPublishers",
+        "Avoid nesting `Publisher`s inside `Publishers`s; the resultant code is hard to reason about",
+    link = BUG_PATTERNS_BASE_URL + "NestedPublishers",
     linkType = CUSTOM,
     severity = WARNING,
     tags = FRAGILE_CODE)
-public final class MonoOfPublishers extends BugChecker implements MethodInvocationTreeMatcher {
+public final class NestedPublishers extends BugChecker implements MethodInvocationTreeMatcher {
   private static final long serialVersionUID = 1L;
-  private static final Supplier<Type> MONO =
-      Suppliers.typeFromString("reactor.core.publisher.Mono");
-  private static final Supplier<Type> MONO_OF_PUBLISHERS =
+  private static final Supplier<Type> PUBLISHER = type("org.reactivestreams.Publisher");
+  private static final Supplier<Type> PUBLISHER_OF_PUBLISHERS =
       VisitorState.memoize(
-          generic(MONO, subOf(generic(type("org.reactivestreams.Publisher"), unbound()))));
+          generic(PUBLISHER, subOf(generic(type("org.reactivestreams.Publisher"), unbound()))));
+  private static final Supplier<Type> GROUPED_FLUX =
+      VisitorState.memoize(
+          generic(
+              Suppliers.typeFromString("reactor.core.publisher.GroupedFlux"),
+              unbound(),
+              unbound()));
 
-  /** Instantiates a new {@link MonoOfPublishers} instance. */
-  public MonoOfPublishers() {}
+  /** Instantiates a new {@link NestedPublishers} instance. */
+  public NestedPublishers() {}
 
   @Override
   public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
-    Type type = MONO_OF_PUBLISHERS.get(state);
-    if (type == null || !state.getTypes().isSubtype(ASTHelpers.getType(tree), type)) {
+    Type publisherOfPublisherType = PUBLISHER_OF_PUBLISHERS.get(state);
+    Type groupedFluxType = GROUPED_FLUX.get(state);
+    Type treeType = ASTHelpers.getType(tree);
+    if ((publisherOfPublisherType == null
+            || !state.getTypes().isSubtype(treeType, publisherOfPublisherType))
+        || (groupedFluxType != null
+            && isTypeArgumentGroupedFlux(state, groupedFluxType, treeType))) {
       return Description.NO_MATCH;
     }
 
     return describeMatch(tree);
   }
+
+  private static boolean isTypeArgumentGroupedFlux(
+      VisitorState state, Type groupedFluxType, Type treeType) {
+    return treeType.getTypeArguments().stream()
+        .anyMatch(typez -> ASTHelpers.isSameType(typez, groupedFluxType, state));
+  }
 }
diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java
@@ -435,6 +435,7 @@ Flux<T> after(Flux<T> flux) {
   /** Prefer {@link Flux#concatMap(Function)} over more contrived alternatives. */
   static final class FluxConcatMap<T, S> {
     @BeforeTemplate
+    @SuppressWarnings("NestedPublishers")
     Flux<S> before(Flux<T> flux, Function<? super T, ? extends Publisher<? extends S>> function) {
       return Refaster.anyOf(
           flux.flatMap(function, 1),
@@ -451,6 +452,7 @@ Flux<S> after(Flux<T> flux, Function<? super T, ? extends Publisher<? extends S>
   /** Prefer {@link Flux#concatMap(Function, int)} over more contrived alternatives. */
   static final class FluxConcatMapWithPrefetch<T, S> {
     @BeforeTemplate
+    @SuppressWarnings("NestedPublishers")
     Flux<S> before(
         Flux<T> flux,
         Function<? super T, ? extends Publisher<? extends S>> function,
@@ -782,7 +784,7 @@ Flux<S> after(Flux<T> flux) {
   /** Prefer {@link Mono#flatMap(Function)} over more contrived alternatives. */
   static final class MonoFlatMap<S, T> {
     @BeforeTemplate
-    @SuppressWarnings("MonoOfPublishers")
+    @SuppressWarnings("NestedPublishers")
     Mono<T> before(Mono<S> mono, Function<? super S, ? extends Mono<? extends T>> function) {
       return mono.map(function).flatMap(identity());
     }
@@ -796,7 +798,7 @@ Mono<T> after(Mono<S> mono, Function<? super S, ? extends Mono<? extends T>> fun
   /** Prefer {@link Mono#flatMapMany(Function)} over more contrived alternatives. */
   static final class MonoFlatMapMany<S, T> {
     @BeforeTemplate
-    @SuppressWarnings("MonoOfPublishers")
+    @SuppressWarnings("NestedPublishers")
     Flux<T> before(Mono<S> mono, Function<? super S, ? extends Publisher<? extends T>> function) {
       return mono.map(function).flatMapMany(identity());
     }

diff --git a/...one/bugpatterns/MonoOfPublishersTest.java → ...one/bugpatterns/NestedPublishersTest.java b/...one/bugpatterns/MonoOfPublishersTest.java → ...one/bugpatterns/NestedPublishersTest.java
@@ -3,19 +3,17 @@
 import com.google.errorprone.CompilationTestHelper;
 import org.junit.jupiter.api.Test;
 
-final class MonoOfPublishersTest {
+final class NestedPublishersTest {
   @Test
   void identification() {
-    CompilationTestHelper.newInstance(MonoOfPublishers.class, getClass())
+    CompilationTestHelper.newInstance(NestedPublishers.class, getClass())
         .addSourceLines(
             "A.java",
             "import reactor.core.publisher.Flux;",
             "import reactor.core.publisher.Mono;",
             "",
             "class A {",
             "  void m() {",
-            "    Mono.empty();",
-            "    Mono.just(1);",
             "    // BUG: Diagnostic contains:",
             "    Mono.just(Mono.empty());",
             "    // BUG: Diagnostic contains:",
@@ -38,6 +36,21 @@ void identification() {
             "    Mono.justOrEmpty(1).map(Mono::just);",
             "    // BUG: Diagnostic contains:",
             "    Mono.justOrEmpty(1).map(Flux::just);",
+            "",
+            "    // BUG: Diagnostic contains:",
+            "    Flux.just(Flux.empty());",
+            "    // BUG: Diagnostic contains:",
+            "    Flux.just(Flux.just(1));",
+            "    // BUG: Diagnostic contains:",
+            "    Flux.just(Mono.empty());",
+            "    // BUG: Diagnostic contains:",
+            "    Flux.just(Mono.just(1));",
+            "    // BUG: Diagnostic contains:",
+            "    Flux.just(1).map(Flux::just);",
+            "    // BUG: Diagnostic contains:",
+            "    Flux.just(1).map(Mono::just);",
+            "",
+            "    Flux.just(1, 2).groupBy(i -> i);",
             "  }",
             "}")
         .doTest();