From e8a8e3799ce0193739b251863cf1afb8bb5e0a29 Mon Sep 17 00:00:00 2001 From: mohamedsamehsalah Date: Fri, 21 Feb 2025 11:47:33 +0100 Subject: [PATCH] Introduce `EmptyReactivePublisher` bug checker --- .../bugpatterns/EmptyReactivePublisher.java | 135 ++++++++++++++++++ .../EmptyReactivePublisherTest.java | 91 ++++++++++++ 2 files changed, 226 insertions(+) create mode 100644 error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/EmptyReactivePublisher.java create mode 100644 error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/EmptyReactivePublisherTest.java diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/EmptyReactivePublisher.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/EmptyReactivePublisher.java new file mode 100644 index 00000000..9d3441dc --- /dev/null +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/EmptyReactivePublisher.java @@ -0,0 +1,135 @@ +package tech.picnic.errorprone.bugpatterns; + +import static com.google.errorprone.BugPattern.LinkType.CUSTOM; +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 static com.google.errorprone.matchers.Matchers.isSubtypeOf; +import static com.google.errorprone.matchers.Matchers.staticMethod; +import static com.google.errorprone.matchers.Matchers.typePredicateMatcher; +import static tech.picnic.errorprone.utils.Documentation.BUG_PATTERNS_BASE_URL; +import static tech.picnic.errorprone.utils.MoreTypePredicates.isSubTypeOf; +import static tech.picnic.errorprone.utils.MoreTypes.generic; +import static tech.picnic.errorprone.utils.MoreTypes.type; + +import com.google.auto.service.AutoService; +import com.google.common.collect.Iterables; +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.matchers.Description; +import com.google.errorprone.matchers.Matcher; +import com.google.errorprone.suppliers.Supplier; +import com.google.errorprone.suppliers.Suppliers; +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.source.tree.MethodInvocationTree; +import com.sun.source.tree.Tree; +import com.sun.tools.javac.code.Type; +import java.util.function.Consumer; +import org.reactivestreams.Publisher; +import reactor.core.publisher.Flux; +import reactor.core.publisher.Mono; + +/** + * A {@link BugChecker} that flags {@link Publisher} operations that are known to be vacuous, given + * that they are invoked on a {@link Mono} or {@link Flux} that do not emit next signals. + */ +@AutoService(BugChecker.class) +@BugPattern( + summary = "Avoid vacuous operations on empty `Publisher`s", + link = BUG_PATTERNS_BASE_URL + "EmptyReactivePublisher", + linkType = CUSTOM, + severity = WARNING, + tags = SIMPLIFICATION) +public final class EmptyReactivePublisher extends BugChecker + implements MethodInvocationTreeMatcher { + private static final long serialVersionUID = 1L; + + private static final Supplier MONO = + Suppliers.typeFromString("reactor.core.publisher.Mono"); + private static final Supplier FLUX = + Suppliers.typeFromString("reactor.core.publisher.Flux"); + private static final Matcher CONSUMER = + isSubtypeOf(type(Consumer.class.getCanonicalName())); + private static final Supplier VOID = type(Void.class.getCanonicalName()); + + private static final Matcher EMPTY_FLUX = + anyOf( + staticMethod().onDescendantOf(FLUX).named("empty"), + typePredicateMatcher(isSubTypeOf(generic(FLUX, VOID)))); + private static final Matcher EMPTY_MONO = + anyOf( + staticMethod().onDescendantOf(MONO).named("empty"), + typePredicateMatcher(isSubTypeOf(generic(MONO, VOID)))); + + private static final Matcher VACUOUS_EMPTY_FLUX_OPERATORS = + instanceMethod() + .onDescendantOf(FLUX) + .namedAnyOf( + "concatMap", + "doOnNext", + "filter", + "flatMap", + "flatMapIterable", + "flatMapSequential", + "handle", + "map"); + private static final Matcher VACUOUS_EMPTY_MONO_OPERATORS = + instanceMethod() + .onDescendantOf(MONO) + .namedAnyOf( + "doOnNext", "filter", "flatMap", "flatMapMany", "flatMapIterable", "handle", "map"); + + private static final Matcher SUBSCRIBE = + instanceMethod() + .onDescendantOf(Suppliers.typeFromString("org.reactivestreams.Publisher")) + .named("subscribe"); + + /** Instantiates a new {@link EmptyReactivePublisher} instance. */ + public EmptyReactivePublisher() {} + + @Override + public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { + ExpressionTree receiver = ASTHelpers.getReceiver(tree); + if (receiver == null + || (!EMPTY_FLUX.matches(receiver, state) && !EMPTY_MONO.matches(receiver, state))) { + return Description.NO_MATCH; + } + + if (SUBSCRIBE.matches(tree, state)) { + // First argument passed to `#subscribe` is always an on next signal `Consumer`. + ExpressionTree firstArgument = Iterables.getFirst(tree.getArguments(), null); + if ((firstArgument instanceof LambdaExpressionTree + || firstArgument instanceof MemberReferenceTree) + && CONSUMER.matches(firstArgument, state)) { + return buildDescription(firstArgument) + .setMessage("Passing an on next signal `Consumer` on empty `Publisher`s is a no-op") + .build(); + } + } + + if (EMPTY_FLUX.matches(receiver, state) && VACUOUS_EMPTY_FLUX_OPERATORS.matches(tree, state)) { + return buildDescription(tree) + .setMessage( + String.format( + "Operator `%s` on an empty `Flux`s is a no-op", + ASTHelpers.getSymbol(tree).getSimpleName())) + .build(); + } + + if (EMPTY_MONO.matches(receiver, state) && VACUOUS_EMPTY_MONO_OPERATORS.matches(tree, state)) { + return buildDescription(tree) + .setMessage( + String.format( + "Operator `%s` on an empty `Mono`s is a no-op", + ASTHelpers.getSymbol(tree).getSimpleName())) + .build(); + } + return Description.NO_MATCH; + } +} diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/EmptyReactivePublisherTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/EmptyReactivePublisherTest.java new file mode 100644 index 00000000..fe2b8ab7 --- /dev/null +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/EmptyReactivePublisherTest.java @@ -0,0 +1,91 @@ +package tech.picnic.errorprone.bugpatterns; + +import com.google.errorprone.CompilationTestHelper; +import org.junit.jupiter.api.Test; + +final class EmptyReactivePublisherTest { + @Test + void identification() { + CompilationTestHelper.newInstance(EmptyReactivePublisher.class, getClass()) + .addSourceLines( + "A.java", + "import com.google.common.collect.ImmutableSet;", + "import reactor.core.publisher.Flux;", + "import reactor.core.publisher.Mono;", + "import reactor.util.context.Context;", + "", + "class A {", + " void m() {", + " Mono.just(1).doOnNext(i -> {});", + " Mono.just(2).filter(i -> true);", + " Mono.just(3).flatMap(Mono::just);", + " Mono.just(4).flatMapMany(Flux::just);", + " Mono.just(5).flatMapIterable(ImmutableSet::of);", + " Mono.just(6).handle((i, j) -> {});", + " Mono.just(7).map(i -> i);", + "", + " // BUG: Diagnostic contains: doOnNext", + " Mono.empty().doOnNext(i -> {});", + " // BUG: Diagnostic contains: filter", + " Mono.empty().filter(i -> true);", + " // BUG: Diagnostic contains: flatMap", + " Mono.empty().flatMap(Mono::just);", + " // BUG: Diagnostic contains: flatMapMany", + " Mono.empty().flatMapMany(Flux::just);", + " // BUG: Diagnostic contains: flatMapIterable", + " Mono.empty().flatMapIterable(ImmutableSet::of);", + " // BUG: Diagnostic contains: handle", + " Mono.empty().handle((i, j) -> {});", + " // BUG: Diagnostic contains: map", + " Mono.empty().map(i -> i);", + "", + " // BUG: Diagnostic contains: doOnNext", + " Flux.empty().doOnNext(i -> {});", + " // BUG: Diagnostic contains: filter", + " Flux.empty().filter(i -> true);", + " // BUG: Diagnostic contains: concatMap", + " Flux.empty().concatMap(Mono::just);", + " // BUG: Diagnostic contains: flatMap", + " Flux.empty().flatMap(Mono::just);", + " // BUG: Diagnostic contains: flatMapSequential", + " Flux.empty().flatMapSequential(Flux::just);", + " // BUG: Diagnostic contains: flatMapIterable", + " Flux.empty().flatMapIterable(ImmutableSet::of);", + " // BUG: Diagnostic contains: handle", + " Flux.empty().handle((i, j) -> {});", + " // BUG: Diagnostic contains: map", + " Flux.empty().map(i -> i);", + "", + " // BUG: Diagnostic contains: doOnNext", + " Mono.just(8).then().doOnNext(i -> {});", + " // BUG: Diagnostic contains: filter", + " Mono.just(9).then().filter(i -> true);", + " // BUG: Diagnostic contains: flatMap", + " Mono.just(10).then().flatMap(Mono::just);", + " // BUG: Diagnostic contains: flatMapMany", + " Mono.just(11).then().flatMapMany(Flux::just);", + " // BUG: Diagnostic contains: flatMapIterable", + " Mono.just(12).then().flatMapIterable(ImmutableSet::of);", + " // BUG: Diagnostic contains: handle", + " Mono.just(13).then().handle((i, j) -> {});", + " // BUG: Diagnostic contains: map", + " Mono.just(14).then().map(i -> i);", + "", + " Mono.just(15).subscribe();", + " Mono.just(16).subscribe(null, t -> {});", + "", + " // BUG: Diagnostic contains:", + " Mono.just(17).then().subscribe(i -> {});", + " // BUG: Diagnostic contains:", + " Mono.just(18).then().subscribe(i -> {}, t -> {});", + " // BUG: Diagnostic contains:", + " Mono.just(19).then().subscribe(i -> {}, t -> {}, () -> {});", + " // BUG: Diagnostic contains:", + " Mono.just(20).then().subscribe(i -> {}, t -> {}, () -> {}, j -> {});", + " // BUG: Diagnostic contains:", + " Mono.just(21).then().subscribe(i -> {}, t -> {}, () -> {}, Context.empty());", + " }", + "}") + .doTest(); + } +}