Prefer Mono#fromSupplier over Mono#fromCallable where possible

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

@@ -9,10 +9,12 @@
 import com.google.errorprone.refaster.annotation.AfterTemplate;
 import com.google.errorprone.refaster.annotation.BeforeTemplate;
 import com.google.errorprone.refaster.annotation.MayOptionallyUse;
+import com.google.errorprone.refaster.annotation.NotMatches;
 import com.google.errorprone.refaster.annotation.Placeholder;
 import com.google.errorprone.refaster.annotation.UseImportPolicy;
 import java.time.Duration;
 import java.util.Optional;
+import java.util.concurrent.Callable;
 import java.util.function.Consumer;
 import java.util.function.Function;
 import java.util.function.Predicate;
@@ -22,16 +24,35 @@
 import reactor.core.publisher.Mono;
 import reactor.test.StepVerifier;
 import reactor.test.publisher.PublisherProbe;
+import tech.picnic.errorprone.refaster.util.ThrowsCheckedException;
 
 /** Refaster templates related to Reactor expressions and statements. */
 final class ReactorTemplates {
   private ReactorTemplates() {}
 
+  /**
+   * Prefer {@link Mono#fromSupplier(Supplier)} over {@link Mono#fromCallable(Callable)} where
+   * feasible.
+   */
+  static final class MonoFromSupplier<T> {
+    @BeforeTemplate
+    Mono<T> before(@NotMatches(ThrowsCheckedException.class) Callable<? extends T> supplier) {
+      return Mono.fromCallable(supplier);
+    }
+
+    @AfterTemplate
+    Mono<T> after(Supplier<? extends T> supplier) {
+      return Mono.fromSupplier(supplier);
+    }
+  }
+
   /** Prefer {@link Mono#justOrEmpty(Optional)} over more verbose alternatives. */
   // XXX: If `optional` is a constant and effectively-final expression then the `Mono.defer` can be
   // dropped. Should look into Refaster support for identifying this.
   static final class MonoFromOptional<T> {
     @BeforeTemplate
+    @SuppressWarnings(
+        "MonoFromSupplier" /* `optional` may match a checked exception-throwing expression. */)
     Mono<T> before(Optional<T> optional) {
       return Refaster.anyOf(
           Mono.fromCallable(() -> optional.orElse(null)),

error-prone-contrib/src/test/resources/tech/picnic/errorprone/refastertemplates/ReactorTemplatesTestInput.java

@@ -6,6 +6,7 @@
 import com.google.common.collect.ImmutableSet;
 import java.time.Duration;
 import java.util.Optional;
+import java.util.concurrent.Callable;
 import java.util.function.Supplier;
 import reactor.core.publisher.Flux;
 import reactor.core.publisher.Mono;
@@ -19,6 +20,15 @@ public ImmutableSet<?> elidedTypesAndStaticImports() {
     return ImmutableSet.of(assertThat(0));
   }
 
+  ImmutableSet<Mono<?>> testMonoFromSupplier() {
+    return ImmutableSet.of(
+        Mono.fromCallable((Callable<?>) null),
+        Mono.fromCallable(() -> getClass().getDeclaredConstructor()),
+        Mono.fromCallable(() -> toString()),
+        Mono.fromCallable(getClass()::getDeclaredConstructor),
+        Mono.fromCallable(this::toString));
+  }
+
   ImmutableSet<Mono<Integer>> testMonoFromOptional() {
     return ImmutableSet.of(
         Mono.fromCallable(() -> Optional.of(1).orElse(null)),

error-prone-contrib/src/test/resources/tech/picnic/errorprone/refastertemplates/ReactorTemplatesTestOutput.java

@@ -7,6 +7,7 @@
 import com.google.common.collect.ImmutableSet;
 import java.time.Duration;
 import java.util.Optional;
+import java.util.concurrent.Callable;
 import java.util.function.Supplier;
 import reactor.core.publisher.Flux;
 import reactor.core.publisher.Mono;
@@ -20,6 +21,15 @@ public ImmutableSet<?> elidedTypesAndStaticImports() {
     return ImmutableSet.of(assertThat(0));
   }
 
+  ImmutableSet<Mono<?>> testMonoFromSupplier() {
+    return ImmutableSet.of(
+        Mono.fromCallable((Callable<?>) null),
+        Mono.fromCallable(() -> getClass().getDeclaredConstructor()),
+        Mono.fromSupplier(() -> toString()),
+        Mono.fromCallable(getClass()::getDeclaredConstructor),
+        Mono.fromSupplier(this::toString));
+  }
+
   ImmutableSet<Mono<Integer>> testMonoFromOptional() {
     return ImmutableSet.of(
         Mono.defer(() -> Mono.justOrEmpty(Optional.of(1))),

refaster-support/pom.xml

@@ -51,6 +51,11 @@
             <artifactId>jsr305</artifactId>
             <scope>provided</scope>
         </dependency>
+        <dependency>
+            <groupId>com.google.guava</groupId>
+            <artifactId>guava</artifactId>
+            <scope>provided</scope>
+        </dependency>
         <dependency>
             <groupId>org.junit.jupiter</groupId>
             <artifactId>junit-jupiter-api</artifactId>

refaster-support/src/main/java/tech/picnic/errorprone/refaster/util/IsArray.java

@@ -6,7 +6,7 @@
 import com.google.errorprone.matchers.Matcher;
 import com.sun.source.tree.ExpressionTree;
 
-/** A matcher of array-typed expressions, for use with Refaster's {@code @Matches} annotation. */
+/** A matcher of array-typed expressions. */
 public final class IsArray implements Matcher<ExpressionTree> {
   private static final long serialVersionUID = 1L;
   private static final Matcher<ExpressionTree> DELEGATE = isArrayType();

refaster-support/src/main/java/tech/picnic/errorprone/refaster/util/ThrowsCheckedException.java

@@ -0,0 +1,60 @@
+package tech.picnic.errorprone.refaster.util;
+
+import com.google.common.collect.ImmutableSet;
+import com.google.errorprone.VisitorState;
+import com.google.errorprone.matchers.Matcher;
+import com.google.errorprone.util.ASTHelpers;
+import com.sun.source.tree.ExpressionTree;
+import com.sun.source.tree.LambdaExpressionTree;
+import com.sun.source.tree.MemberReferenceTree;
+import com.sun.tools.javac.code.Symbol;
+import com.sun.tools.javac.code.Type;
+import com.sun.tools.javac.code.Types.FunctionDescriptorLookupError;
+import java.util.Collection;
+
+/**
+ * A matcher of functional interface expressions for which execution of the functional interface
+ * method may throw a checked exception.
+ */
+public final class ThrowsCheckedException implements Matcher<ExpressionTree> {
+  private static final long serialVersionUID = 1L;
+
+  @Override
+  public boolean matches(ExpressionTree tree, VisitorState state) {
+    return containsCheckedException(getThrownTypes(tree, state), state);
+  }
+
+  private static Collection<Type> getThrownTypes(ExpressionTree tree, VisitorState state) {
+    if (tree instanceof LambdaExpressionTree) {
+      return ASTHelpers.getThrownExceptions(((LambdaExpressionTree) tree).getBody(), state);
+    }
+
+    if (tree instanceof MemberReferenceTree) {
+      Symbol symbol = ASTHelpers.getSymbol(tree);
+      if (symbol == null) {
+        return ImmutableSet.of();
+      }
+
+      return symbol.type.getThrownTypes();
+    }
+
+    Type type = ASTHelpers.getType(tree);
+    if (type == null) {
+      return ImmutableSet.of();
+    }
+
+    try {
+      return state.getTypes().findDescriptorType(type).getThrownTypes();
+    } catch (FunctionDescriptorLookupError e) {
+      return ImmutableSet.of();
+    }
+  }
+
+  private static boolean containsCheckedException(Collection<Type> types, VisitorState state) {
+    return !types.stream()
+        .allMatch(
+            t ->
+                ASTHelpers.isSubtype(t, state.getSymtab().runtimeExceptionType, state)
+                    || ASTHelpers.isSubtype(t, state.getSymtab().errorType, state));
+  }
+}

refaster-support/src/main/java/tech/picnic/errorprone/refaster/util/package-info.java

@@ -1,7 +1,8 @@
 /**
  * A collection of zero-argument {@link com.google.errorprone.matchers.Matcher Matcher}
  * implementations for use with Refaster's {@link
- * com.google.errorprone.refaster.annotation.Matches @Matches} annotation.
+ * com.google.errorprone.refaster.annotation.Matches @Matches} and {@link
+ * com.google.errorprone.refaster.annotation.NotMatches @NotMatches} annotations.
  */
 @com.google.errorprone.annotations.CheckReturnValue
 @javax.annotation.ParametersAreNonnullByDefault

refaster-support/src/test/java/tech/picnic/errorprone/refaster/util/AbstractMatcherTestChecker.java

@@ -0,0 +1,70 @@
+package tech.picnic.errorprone.refaster.util;
+
+import com.google.errorprone.VisitorState;
+import com.google.errorprone.bugpatterns.BugChecker;
+import com.google.errorprone.bugpatterns.BugChecker.CompilationUnitTreeMatcher;
+import com.google.errorprone.matchers.Description;
+import com.google.errorprone.matchers.Matcher;
+import com.sun.source.tree.CompilationUnitTree;
+import com.sun.source.tree.ExpressionTree;
+import com.sun.source.tree.ImportTree;
+import com.sun.source.tree.MethodTree;
+import com.sun.source.tree.Tree;
+import com.sun.source.util.TreeScanner;
+import javax.annotation.Nullable;
+
+/**
+ * An abstract {@link BugChecker} that reports a match for each expression matched by the given
+ * {@link Matcher}.
+ *
+ * <p>Only {@link ExpressionTree}s that represent proper Java expressions (i.e. {@link
+ * ExpressionTree}s that may be matched by Refaster) are considered.
+ */
+abstract class AbstractMatcherTestChecker extends BugChecker implements CompilationUnitTreeMatcher {
+  private static final long serialVersionUID = 1L;
+
+  private final Matcher<ExpressionTree> delegate;
+
+  AbstractMatcherTestChecker(Matcher<ExpressionTree> delegate) {
+    this.delegate = delegate;
+  }
+
+  @Override
+  public Description matchCompilationUnit(CompilationUnitTree compilationUnit, VisitorState state) {
+    new TreeScanner<Void, Void>() {
+      @Nullable
+      @Override
+      public Void scan(Tree tree, @Nullable Void unused) {
+        if (tree instanceof ExpressionTree && delegate.matches((ExpressionTree) tree, state)) {
+          state.reportMatch(
+              Description.builder(tree, canonicalName(), null, defaultSeverity(), message())
+                  .build());
+        }
+
+        return super.scan(tree, unused);
+      }
+
+      @Nullable
+      @Override
+      public Void visitImport(ImportTree node, @Nullable Void unused) {
+        /*
+         * We're not interested in matching import statements. While components of these
+         * can be `ExpressionTree`s, they will never be matched by Refaster.
+         */
+        return null;
+      }
+
+      @Nullable
+      @Override
+      public Void visitMethod(MethodTree node, @Nullable Void unused) {
+        /*
+         * We're not interested in matching e.g. parameter and return type declarations. While these
+         * can be `ExpressionTree`s, they will never be matched by Refaster.
+         */
+        return scan(node.getBody(), unused);
+      }
+    }.scan(compilationUnit, null);
+
+    return Description.NO_MATCH;
+  }
+}

refaster-support/src/test/java/tech/picnic/errorprone/refaster/util/IsArrayTest.java

@@ -4,17 +4,13 @@
 
 import com.google.errorprone.BugPattern;
 import com.google.errorprone.CompilationTestHelper;
-import com.google.errorprone.VisitorState;
 import com.google.errorprone.bugpatterns.BugChecker;
-import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher;
-import com.google.errorprone.matchers.Description;
-import com.sun.source.tree.MethodInvocationTree;
 import org.junit.jupiter.api.Test;
 
 final class IsArrayTest {
   @Test
   void matches() {
-    CompilationTestHelper.newInstance(TestChecker.class, getClass())
+    CompilationTestHelper.newInstance(MatcherTestChecker.class, getClass())
         .addSourceLines(
             "A.java",
             "class A {",
@@ -53,13 +49,15 @@ void matches() {
   }
 
   /** A {@link BugChecker} which simply delegates to {@link IsArray}. */
-  @BugPattern(summary = "Flags array-returning method invocations", severity = ERROR)
-  public static final class TestChecker extends BugChecker implements MethodInvocationTreeMatcher {
+  @BugPattern(summary = "Flags expressions matched by `IsArray`", severity = ERROR)
+  public static final class MatcherTestChecker extends AbstractMatcherTestChecker {
     private static final long serialVersionUID = 1L;
 
-    @Override
-    public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
-      return new IsArray().matches(tree, state) ? describeMatch(tree) : Description.NO_MATCH;
+    // XXX: This is a false positive reported by Checkstyle. See
+    // https://github.com/checkstyle/checkstyle/issues/10161#issuecomment-1242732120.
+    @SuppressWarnings("RedundantModifier")
+    public MatcherTestChecker() {
+      super(new IsArray());
     }
   }
 }

refaster-support/src/test/java/tech/picnic/errorprone/refaster/util/ThrowsCheckedExceptionTest.java

@@ -0,0 +1,94 @@
+package tech.picnic.errorprone.refaster.util;
+
+import static com.google.errorprone.BugPattern.SeverityLevel.ERROR;
+
+import com.google.errorprone.BugPattern;
+import com.google.errorprone.CompilationTestHelper;
+import com.google.errorprone.bugpatterns.BugChecker;
+import org.junit.jupiter.api.Test;
+
+final class ThrowsCheckedExceptionTest {
+  @Test
+  void matches() {
+    CompilationTestHelper.newInstance(MatcherTestChecker.class, getClass())
+        .addSourceLines(
+            "A.java",
+            "import java.util.concurrent.Callable;",
+            "import java.util.function.Supplier;",
+            "",
+            "class A {",
+            "  void negative1() {",
+            "    callableSink(null);",
+            "  }",
+            "",
+            "  void negative2() {",
+            "    supplierSink(null);",
+            "  }",
+            "",
+            "  void negative3() {",
+            "    callableSink(() -> toString());",
+            "  }",
+            "",
+            "  void negative4() {",
+            "    supplierSink(() -> toString());",
+            "  }",
+            "",
+            "  void negative5() {",
+            "    callableSink(this::toString);",
+            "  }",
+            "",
+            "  void negative6() {",
+            "    supplierSink(this::toString);",
+            "  }",
+            "",
+            "  void negative7() {",
+            "    supplierSink(",
+            "        new Supplier<>() {",
+            "          @Override",
+            "          public Object get() {",
+            "            return getClass();",
+            "          }",
+            "        });",
+            "  }",
+            "",
+            "  void positive1() {",
+            "    // BUG: Diagnostic contains:",
+            "    callableSink(() -> getClass().getDeclaredConstructor());",
+            "  }",
+            "",
+            "  void positive2() {",
+            "    // BUG: Diagnostic contains:",
+            "    callableSink(getClass()::getDeclaredConstructor);",
+            "  }",
+            "",
+            "  void positive3() {",
+            "    callableSink(",
+            "        // BUG: Diagnostic contains:",
+            "        new Callable<>() {",
+            "          @Override",
+            "          public Object call() throws NoSuchMethodException {",
+            "            return getClass().getDeclaredConstructor();",
+            "          }",
+            "        });",
+            "  }",
+            "",
+            "  private static void callableSink(Callable<?> callable) {}",
+            "",
+            "  private static void supplierSink(Supplier<?> supplier) {}",
+            "}")
+        .doTest();
+  }
+
+  /** A {@link BugChecker} which simply delegates to {@link ThrowsCheckedException}. */
+  @BugPattern(summary = "Flags expressions matched by `ThrowsCheckedException`", severity = ERROR)
+  public static final class MatcherTestChecker extends AbstractMatcherTestChecker {
+    private static final long serialVersionUID = 1L;
+
+    // XXX: This is a false positive reported by Checkstyle. See
+    // https://github.com/checkstyle/checkstyle/issues/10161#issuecomment-1242732120.
+    @SuppressWarnings("RedundantModifier")
+    public MatcherTestChecker() {
+      super(new ThrowsCheckedException());
+    }
+  }
+}