diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/ReactorTemplates.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/ReactorTemplates.java index ce88aacf9c..d76ca5befb 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/ReactorTemplates.java +++ b/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 { + @BeforeTemplate + Mono before(@NotMatches(ThrowsCheckedException.class) Callable supplier) { + return Mono.fromCallable(supplier); + } + + @AfterTemplate + Mono after(Supplier 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 { @BeforeTemplate + @SuppressWarnings( + "MonoFromSupplier" /* `optional` may match a checked exception-throwing expression. */) Mono before(Optional optional) { return Refaster.anyOf( Mono.fromCallable(() -> optional.orElse(null)), diff --git a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refastertemplates/ReactorTemplatesTestInput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refastertemplates/ReactorTemplatesTestInput.java index 0fc64f7670..bd353ed35a 100644 --- a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refastertemplates/ReactorTemplatesTestInput.java +++ b/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> testMonoFromSupplier() { + return ImmutableSet.of( + Mono.fromCallable((Callable) null), + Mono.fromCallable(() -> getClass().getDeclaredConstructor()), + Mono.fromCallable(() -> toString()), + Mono.fromCallable(getClass()::getDeclaredConstructor), + Mono.fromCallable(this::toString)); + } + ImmutableSet> testMonoFromOptional() { return ImmutableSet.of( Mono.fromCallable(() -> Optional.of(1).orElse(null)), diff --git a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refastertemplates/ReactorTemplatesTestOutput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refastertemplates/ReactorTemplatesTestOutput.java index 18fd8b92b9..2116246d73 100644 --- a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refastertemplates/ReactorTemplatesTestOutput.java +++ b/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> testMonoFromSupplier() { + return ImmutableSet.of( + Mono.fromCallable((Callable) null), + Mono.fromCallable(() -> getClass().getDeclaredConstructor()), + Mono.fromSupplier(() -> toString()), + Mono.fromCallable(getClass()::getDeclaredConstructor), + Mono.fromSupplier(this::toString)); + } + ImmutableSet> testMonoFromOptional() { return ImmutableSet.of( Mono.defer(() -> Mono.justOrEmpty(Optional.of(1))), diff --git a/refaster-support/pom.xml b/refaster-support/pom.xml index 10f214e0a3..326fb8db7c 100644 --- a/refaster-support/pom.xml +++ b/refaster-support/pom.xml @@ -51,6 +51,11 @@ jsr305 provided + + com.google.guava + guava + provided + org.junit.jupiter junit-jupiter-api diff --git a/refaster-support/src/main/java/tech/picnic/errorprone/refaster/util/IsArray.java b/refaster-support/src/main/java/tech/picnic/errorprone/refaster/util/IsArray.java index e3d60e6662..5b9ac4c44b 100644 --- a/refaster-support/src/main/java/tech/picnic/errorprone/refaster/util/IsArray.java +++ b/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 { private static final long serialVersionUID = 1L; private static final Matcher DELEGATE = isArrayType(); diff --git a/refaster-support/src/main/java/tech/picnic/errorprone/refaster/util/ThrowsCheckedException.java b/refaster-support/src/main/java/tech/picnic/errorprone/refaster/util/ThrowsCheckedException.java new file mode 100644 index 0000000000..aa1e581394 --- /dev/null +++ b/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 { + private static final long serialVersionUID = 1L; + + @Override + public boolean matches(ExpressionTree tree, VisitorState state) { + return containsCheckedException(getThrownTypes(tree, state), state); + } + + private static Collection 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 types, VisitorState state) { + return !types.stream() + .allMatch( + t -> + ASTHelpers.isSubtype(t, state.getSymtab().runtimeExceptionType, state) + || ASTHelpers.isSubtype(t, state.getSymtab().errorType, state)); + } +} diff --git a/refaster-support/src/main/java/tech/picnic/errorprone/refaster/util/package-info.java b/refaster-support/src/main/java/tech/picnic/errorprone/refaster/util/package-info.java index e802b094d6..297253575f 100644 --- a/refaster-support/src/main/java/tech/picnic/errorprone/refaster/util/package-info.java +++ b/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 diff --git a/refaster-support/src/test/java/tech/picnic/errorprone/refaster/util/AbstractMatcherTestChecker.java b/refaster-support/src/test/java/tech/picnic/errorprone/refaster/util/AbstractMatcherTestChecker.java new file mode 100644 index 0000000000..f7fd67cb9e --- /dev/null +++ b/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}. + * + *

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 delegate; + + AbstractMatcherTestChecker(Matcher delegate) { + this.delegate = delegate; + } + + @Override + public Description matchCompilationUnit(CompilationUnitTree compilationUnit, VisitorState state) { + new TreeScanner() { + @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; + } +} diff --git a/refaster-support/src/test/java/tech/picnic/errorprone/refaster/util/IsArrayTest.java b/refaster-support/src/test/java/tech/picnic/errorprone/refaster/util/IsArrayTest.java index 11ad12c462..847ca0931f 100644 --- a/refaster-support/src/test/java/tech/picnic/errorprone/refaster/util/IsArrayTest.java +++ b/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()); } } } diff --git a/refaster-support/src/test/java/tech/picnic/errorprone/refaster/util/ThrowsCheckedExceptionTest.java b/refaster-support/src/test/java/tech/picnic/errorprone/refaster/util/ThrowsCheckedExceptionTest.java new file mode 100644 index 0000000000..789e16c1e3 --- /dev/null +++ b/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()); + } + } +}