WIP: Integrate Spotbugs

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

@@ -0,0 +1,91 @@
+package tech.picnic.errorprone.bugpatterns;
+
+import static com.google.errorprone.BugPattern.LinkType.NONE;
+import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;
+import static com.google.errorprone.BugPattern.StandardTags.SIMPLIFICATION;
+import static com.google.errorprone.matchers.Matchers.anyOf;
+import static com.google.errorprone.matchers.Matchers.instanceMethod;
+
+import com.google.auto.service.AutoService;
+import com.google.errorprone.BugPattern;
+import com.google.errorprone.VisitorState;
+import com.google.errorprone.bugpatterns.BugChecker;
+import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher;
+import com.google.errorprone.fixes.SuggestedFix;
+import com.google.errorprone.matchers.Description;
+import com.google.errorprone.matchers.Matcher;
+import com.google.errorprone.util.ASTHelpers;
+import com.sun.source.tree.ExpressionTree;
+import com.sun.source.tree.MethodInvocationTree;
+import java.util.function.BiFunction;
+import reactor.core.publisher.Mono;
+import tech.picnic.errorprone.bugpatterns.util.SourceCode;
+
+/**
+ * A {@link BugChecker} which flags {@link Mono} operations that are known to be vacuous, given that
+ * they are invoked on a {@link Mono} that is known not to complete empty.
+ */
+@AutoService(BugChecker.class)
+@BugPattern(
+    summary = "Avoid vacuous operations on known non-empty `Mono`s",
+    linkType = NONE,
+    severity = WARNING,
+    tags = SIMPLIFICATION)
+// XXX: This check does not simplify `someFlux.defaultIfEmpty(T).{defaultIfEmpty(T),hasElements()}`,
+// as `someFlux.defaultIfEmpty(T)` yields a `Flux` rather than a `Mono`. Consider adding support for
+// these cases.
+// XXX: Given more advanced analysis many more expressions could be flagged. Consider
+// `Mono.just(someValue)`, `Flux.just(someNonEmptySequence)`,
+// `someMono.switchIfEmpty(someProvablyNonEmptyMono)` and many other variants.
+// XXX: Consider implementing a similar check for `Publisher`s that are known to complete without
+// emitting a value (e.g. `Mono.empty()`, `someFlux.then()`, ...), or known not to complete normally
+// (`Mono.never()`, `someFlux.repeat()`, `Mono.error(...)`, ...). The latter category could
+// potentially be split out further.
+public final class NonEmptyMono extends BugChecker implements MethodInvocationTreeMatcher {
+  private static final long serialVersionUID = 1L;
+  private static final Matcher<ExpressionTree> MONO_SIZE_CHECK =
+      instanceMethod()
+          .onDescendantOf("reactor.core.publisher.Mono")
+          .namedAnyOf("defaultIfEmpty", "single", "switchIfEmpty");
+  private static final Matcher<ExpressionTree> NON_EMPTY_MONO =
+      anyOf(
+          instanceMethod()
+              .onDescendantOf("reactor.core.publisher.Flux")
+              .namedAnyOf(
+                  "all",
+                  "any",
+                  "collect",
+                  "collectList",
+                  "collectMap",
+                  "collectMultimap",
+                  "collectSortedList",
+                  "count",
+                  "elementAt",
+                  "hasElement",
+                  "hasElements",
+                  "last",
+                  "reduceWith",
+                  "single"),
+          instanceMethod()
+              .onDescendantOf("reactor.core.publisher.Flux")
+              .named("reduce")
+              .withParameters(Object.class.getName(), BiFunction.class.getName()),
+          instanceMethod()
+              .onDescendantOf("reactor.core.publisher.Mono")
+              .namedAnyOf("defaultIfEmpty", "hasElement", "single"));
+
+  @Override
+  public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
+    if (!MONO_SIZE_CHECK.matches(tree, state)) {
+      return Description.NO_MATCH;
+    }
+
+    ExpressionTree receiver = ASTHelpers.getReceiver(tree);
+    if (!NON_EMPTY_MONO.matches(receiver, state)) {
+      return Description.NO_MATCH;
+    }
+
+    return describeMatch(
+        tree, SuggestedFix.replace(tree, SourceCode.treeToString(receiver, state)));
+  }
+}

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

@@ -0,0 +1,155 @@
+package tech.picnic.errorprone.bugpatterns;
+
+import static com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH;
+
+import com.google.errorprone.BugCheckerRefactoringTestHelper;
+import com.google.errorprone.CompilationTestHelper;
+import org.junit.jupiter.api.Test;
+
+final class NonEmptyMonoTest {
+  private final CompilationTestHelper compilationTestHelper =
+      CompilationTestHelper.newInstance(NonEmptyMono.class, getClass());
+  private final BugCheckerRefactoringTestHelper refactoringTestHelper =
+      BugCheckerRefactoringTestHelper.newInstance(NonEmptyMono.class, getClass());
+
+  @Test
+  void identification() {
+    compilationTestHelper
+        .addSourceLines(
+            "A.java",
+            "import static com.google.common.collect.ImmutableList.toImmutableList;",
+            "import static java.util.function.Function.identity;",
+            "",
+            "import com.google.common.collect.ImmutableList;",
+            "import com.google.common.collect.ImmutableMap;",
+            "import java.util.ArrayList;",
+            "import java.util.HashMap;",
+            "import java.util.List;",
+            "import reactor.core.publisher.Flux;",
+            "import reactor.core.publisher.Mono;",
+            "",
+            "class A {",
+            "  void m() {",
+            "    Mono.just(1).defaultIfEmpty(2);",
+            "    Mono.just(1).single();",
+            "    Mono.just(1).switchIfEmpty(Mono.just(2));",
+            "",
+            "    // BUG: Diagnostic contains:",
+            "    Flux.just(1).all(x -> true).defaultIfEmpty(true);",
+            "",
+            "    // BUG: Diagnostic contains:",
+            "    Flux.just(1).any(x -> true).single();",
+            "",
+            "    // BUG: Diagnostic contains:",
+            "    Flux.just(1).collect(toImmutableList()).switchIfEmpty(Mono.just(ImmutableList.of()));",
+            "    // BUG: Diagnostic contains:",
+            "    Flux.just(1).collect(ArrayList::new, List::add).defaultIfEmpty(new ArrayList<>());",
+            "",
+            "    // BUG: Diagnostic contains:",
+            "    Flux.just(1).collectList().single();",
+            "",
+            "    // BUG: Diagnostic contains:",
+            "    Flux.just(1).collectMap(identity()).switchIfEmpty(Mono.just(ImmutableMap.of()));",
+            "    // BUG: Diagnostic contains:",
+            "    Flux.just(1).collectMap(identity(), identity()).defaultIfEmpty(ImmutableMap.of());",
+            "    // BUG: Diagnostic contains:",
+            "    Flux.just(1).collectMap(identity(), identity(), HashMap::new).single();",
+            "",
+            "    // BUG: Diagnostic contains:",
+            "    Flux.just(1).collectMultimap(identity()).switchIfEmpty(Mono.just(ImmutableMap.of()));",
+            "    // BUG: Diagnostic contains:",
+            "    Flux.just(1).collectMultimap(identity(), identity()).defaultIfEmpty(ImmutableMap.of());",
+            "    // BUG: Diagnostic contains:",
+            "    Flux.just(1).collectMultimap(identity(), identity(), HashMap::new).single();",
+            "",
+            "    // BUG: Diagnostic contains:",
+            "    Flux.just(1).collectSortedList().defaultIfEmpty(ImmutableList.of());",
+            "    // BUG: Diagnostic contains:",
+            "    Flux.just(1).collectSortedList((o1, o2) -> 0).single();",
+            "",
+            "    // BUG: Diagnostic contains:",
+            "    Flux.just(1).count().switchIfEmpty(Mono.just(2L));",
+            "",
+            "    // BUG: Diagnostic contains:",
+            "    Flux.just(1).elementAt(0).defaultIfEmpty(1);",
+            "    // BUG: Diagnostic contains:",
+            "    Flux.just(1).elementAt(0, 2).single();",
+            "",
+            "    // BUG: Diagnostic contains:",
+            "    Flux.just(1).hasElement(2).switchIfEmpty(Mono.just(true));",
+            "",
+            "    // BUG: Diagnostic contains:",
+            "    Flux.just(1).hasElements().defaultIfEmpty(true);",
+            "",
+            "    // BUG: Diagnostic contains:",
+            "    Flux.just(1).last().single();",
+            "    // BUG: Diagnostic contains:",
+            "    Flux.just(1).last(2).switchIfEmpty(Mono.just(3));",
+            "",
+            "    // BUG: Diagnostic contains:",
+            "    Flux.just(1).reduceWith(() -> 0, Integer::sum).defaultIfEmpty(2);",
+            "",
+            "    // BUG: Diagnostic contains:",
+            "    Flux.just(1).single().single();",
+            "    // BUG: Diagnostic contains:",
+            "    Flux.just(1).single(2).switchIfEmpty(Mono.just(3));",
+            "",
+            "    Flux.just(1).reduce(Integer::sum).defaultIfEmpty(2);",
+            "    // BUG: Diagnostic contains:",
+            "    Flux.just(1).reduce(2, Integer::sum).single();",
+            "",
+            "    // BUG: Diagnostic contains:",
+            "    Mono.just(1).defaultIfEmpty(1).switchIfEmpty(Mono.just(2));",
+            "    // BUG: Diagnostic contains:",
+            "    Mono.just(1).hasElement().defaultIfEmpty(true);",
+            "    // BUG: Diagnostic contains:",
+            "    Mono.just(1).single().single();",
+            "  }",
+            "}")
+        .doTest();
+  }
+
+  @Test
+  void replacement() {
+    refactoringTestHelper
+        .addInputLines(
+            "A.java",
+            "import static com.google.common.collect.ImmutableList.toImmutableList;",
+            "",
+            "import com.google.common.collect.ImmutableList;",
+            "import reactor.core.publisher.Flux;",
+            "import reactor.core.publisher.Mono;",
+            "",
+            "class A {",
+            "  void m() {",
+            "    Flux.just(1).collect(toImmutableList()).single();",
+            "    Flux.just(1).collect(toImmutableList()).defaultIfEmpty(ImmutableList.of());",
+            "    Flux.just(1).collect(toImmutableList()).switchIfEmpty(Mono.just(ImmutableList.of()));",
+            "",
+            "    Mono.just(2).hasElement().single();",
+            "    Mono.just(2).hasElement().defaultIfEmpty(true);",
+            "    Mono.just(2).hasElement().switchIfEmpty(Mono.just(true));",
+            "  }",
+            "}")
+        .addOutputLines(
+            "A.java",
+            "import static com.google.common.collect.ImmutableList.toImmutableList;",
+            "",
+            "import com.google.common.collect.ImmutableList;",
+            "import reactor.core.publisher.Flux;",
+            "import reactor.core.publisher.Mono;",
+            "",
+            "class A {",
+            "  void m() {",
+            "    Flux.just(1).collect(toImmutableList());",
+            "    Flux.just(1).collect(toImmutableList());",
+            "    Flux.just(1).collect(toImmutableList());",
+            "",
+            "    Mono.just(2).hasElement();",
+            "    Mono.just(2).hasElement();",
+            "    Mono.just(2).hasElement();",
+            "  }",
+            "}")
+        .doTest(TEXT_MATCH);
+  }
+}

pom.xml

@@ -465,6 +465,23 @@
                         </execution>
                     </executions>
                 </plugin>
+                <plugin>
+                    <groupId>com.github.spotbugs</groupId>
+                    <artifactId>spotbugs-maven-plugin</artifactId>
+                    <version>4.7.1.1</version>
+                    <configuration>
+                        <failOnError>false</failOnError>
+                        <fork>false</fork>
+                    </configuration>
+                    <executions>
+                        <execution>
+                            <id>run-spotbugs</id>
+                            <goals>
+                                <goal>check</goal>
+                            </goals>
+                        </execution>
+                    </executions>
+                </plugin>
                 <plugin>
                     <groupId>com.spotify.fmt</groupId>
                     <artifactId>fmt-maven-plugin</artifactId>
@@ -1401,6 +1418,10 @@
                         <groupId>com.github.ekryd.sortpom</groupId>
                         <artifactId>sortpom-maven-plugin</artifactId>
                     </plugin>
+                    <plugin>
+                        <groupId>com.github.spotbugs</groupId>
+                        <artifactId>spotbugs-maven-plugin</artifactId>
+                    </plugin>
                     <plugin>
                         <groupId>com.spotify.fmt</groupId>
                         <artifactId>fmt-maven-plugin</artifactId>
@@ -1602,6 +1623,13 @@
                                 <verifyFail>stop</verifyFail>
                             </configuration>
                         </plugin>
+                        <plugin>
+                            <groupId>com.github.spotbugs</groupId>
+                            <artifactId>spotbugs-maven-plugin</artifactId>
+                            <configuration>
+                                <failOnError>true</failOnError>
+                            </configuration>
+                        </plugin>
                         <plugin>
                             <groupId>com.spotify.fmt</groupId>
                             <artifactId>fmt-maven-plugin</artifactId>