From 1f7e605be0caf0f7d512790d022af03831ee19e0 Mon Sep 17 00:00:00 2001 From: Benedek Halasi Date: Mon, 23 Jan 2023 10:04:19 +0100 Subject: [PATCH] Introduce `ImplicitBlockingFluxOperation` check (#468) --- .../ImplicitBlockingFluxOperation.java | 115 ++++++++++++++ .../ImplicitBlockingFluxOperationTest.java | 141 ++++++++++++++++++ 2 files changed, 256 insertions(+) create mode 100644 error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ImplicitBlockingFluxOperation.java create mode 100644 error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ImplicitBlockingFluxOperationTest.java diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ImplicitBlockingFluxOperation.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ImplicitBlockingFluxOperation.java new file mode 100644 index 00000000000..321473db792 --- /dev/null +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ImplicitBlockingFluxOperation.java @@ -0,0 +1,115 @@ +package tech.picnic.errorprone.bugpatterns; + +import static com.google.errorprone.BugPattern.LinkType.CUSTOM; +import static com.google.errorprone.BugPattern.SeverityLevel.ERROR; +import static com.google.errorprone.BugPattern.StandardTags.LIKELY_ERROR; +import static com.google.errorprone.matchers.Matchers.anyOf; +import static com.google.errorprone.matchers.method.MethodMatchers.instanceMethod; +import static tech.picnic.errorprone.bugpatterns.util.Documentation.BUG_PATTERNS_BASE_URL; + +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.fixes.SuggestedFixes; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.matchers.Matcher; +import com.google.errorprone.matchers.method.MethodMatchers; +import com.google.errorprone.suppliers.Supplier; +import com.google.errorprone.suppliers.Suppliers; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.MethodInvocationTree; +import com.sun.tools.javac.code.Type; +import tech.picnic.errorprone.bugpatterns.util.ThirdPartyLibrary; + +@AutoService(BugChecker.class) +@BugPattern( + summary = + "`Flux#toStream` and `Flux#toIterable` are documented to block, but this is not apparent from the method signature; please make sure that they are used with this in mind.", + link = BUG_PATTERNS_BASE_URL + "ImplicitBlockingFluxOperation", + linkType = CUSTOM, + severity = ERROR, + tags = LIKELY_ERROR) +public class ImplicitBlockingFluxOperation extends BugChecker + implements MethodInvocationTreeMatcher { + private static final long serialVersionUID = 1L; + private static final Supplier FLUX = + Suppliers.typeFromString("reactor.core.publisher.Flux"); + public static final MethodMatchers.MethodClassMatcher FLUX_METHOD = + instanceMethod().onDescendantOf(FLUX); + private static final Matcher FLUX_TO_ITERABLE = + FLUX_METHOD.named("toIterable").withNoParameters(); + private static final Matcher FLUX_TO_STREAM = + FLUX_METHOD.named("toStream").withNoParameters(); + private static final Matcher FLUX_IMPLICIT_BLOCKING_METHOD = + anyOf(FLUX_TO_ITERABLE, FLUX_TO_STREAM); + + @Override + public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { + if (!FLUX_IMPLICIT_BLOCKING_METHOD.matches(tree, state)) { + return Description.NO_MATCH; + } + + Description.Builder description = buildDescription(tree); + + description.addFix(getSuppressWarningsFix(state)); + if (ThirdPartyLibrary.GUAVA.isIntroductionAllowed(state)) { + description.addFix(getGuavaFix(tree, state)); + } + description.addFix(getUnmodifiableListFix(tree, state)); + + return description.build(); + } + + private static SuggestedFix getSuppressWarningsFix(VisitorState state) { + return SuggestedFixes.addSuppressWarnings(state, "ImplicitBlockingFluxOperation"); + } + + private static SuggestedFix getGuavaFix(MethodInvocationTree tree, VisitorState state) { + SuggestedFix.Builder guavaFix = SuggestedFix.builder(); + String toImmutableList = + SuggestedFixes.qualifyStaticImport( + "com.google.common.collect.ImmutableList.toImmutableList", guavaFix, state); + return getCollectAndBlockFix(tree, state, guavaFix, toImmutableList + "()").build(); + } + + private static SuggestedFix getUnmodifiableListFix( + MethodInvocationTree tree, VisitorState state) { + SuggestedFix.Builder unmodifiableListFix = SuggestedFix.builder(); + String toUnmodifiableList = + SuggestedFixes.qualifyStaticImport( + "java.util.stream.Collectors.toUnmodifiableList", unmodifiableListFix, state); + return getCollectAndBlockFix(tree, state, unmodifiableListFix, toUnmodifiableList + "()") + .build(); + } + + /** + * @param collector expression + * @return `collect(...).block()...` fix with specified collector and postfix to match the + * original expression tree. + */ + private static SuggestedFix.Builder getCollectAndBlockFix( + MethodInvocationTree tree, VisitorState state, SuggestedFix.Builder fix, String collector) { + String postfix = getCollectAndBlockFixPostfix(tree, state); + // XXX: replace DIY string replace fix with something more resilient + String flux = + state.getSourceForNode(tree).substring(0, state.getSourceForNode(tree).indexOf(".")); + String replacement = String.format("%s.collect(%s).block()%s", flux, collector, postfix); + // fix.merge(SuggestedFix.replace(startPos, endPos, replacement)); + fix.merge(SuggestedFix.replace(tree, replacement)); + return fix; + } + + /** + * @return postfix for `Flux.collect(...).block()` fix to match the original expression tree. + */ + private static String getCollectAndBlockFixPostfix( + MethodInvocationTree tree, VisitorState state) { + if (FLUX_TO_STREAM.matches(tree, state)) { + return ".stream()"; + } + return ""; + } +} diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ImplicitBlockingFluxOperationTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ImplicitBlockingFluxOperationTest.java new file mode 100644 index 00000000000..745b3456e68 --- /dev/null +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ImplicitBlockingFluxOperationTest.java @@ -0,0 +1,141 @@ +package tech.picnic.errorprone.bugpatterns; + +import com.google.errorprone.BugCheckerRefactoringTestHelper; +import com.google.errorprone.BugCheckerRefactoringTestHelper.FixChoosers; +import com.google.errorprone.CompilationTestHelper; +import org.junit.jupiter.api.Test; + +final class ImplicitBlockingFluxOperationTest { + + @Test + void identification() { + CompilationTestHelper.newInstance(ImplicitBlockingFluxOperation.class, getClass()) + .addSourceLines( + "A.java", + "import reactor.core.publisher.Flux;", + "", + "class A {", + " void m() {", + " // BUG: Diagnostic contains:", + " flux().toIterable();", + " // BUG: Diagnostic contains:", + " flux().toStream();", + " }", + "", + " Flux flux() {", + " return Flux.just(1, 2, 3);", + " }", + "}") + .doTest(); + } + + @Test + void replacementFirstSuggestedFix() { + BugCheckerRefactoringTestHelper.newInstance(ImplicitBlockingFluxOperation.class, getClass()) + .setFixChooser(FixChoosers.FIRST) + .addInputLines( + "A.java", + "import reactor.core.publisher.Flux;", + "", + "class A {", + " void m() {", + " flux().toIterable();", + " flux().toStream();", + " }", + "", + " Flux flux() {", + " return Flux.just(1, 2, 3);", + " }", + "}") + .addOutputLines( + "A.java", + "import reactor.core.publisher.Flux;", + "", + "class A {", + " @SuppressWarnings(\"ImplicitBlockingFluxOperation\")", + " void m() {", + " flux().toIterable();", + " flux().toStream();", + " }", + "", + " Flux flux() {", + " return Flux.just(1, 2, 3);", + " }", + "}") + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + } + + @Test + void replacementSecondSuggestedFix() { + BugCheckerRefactoringTestHelper.newInstance(ImplicitBlockingFluxOperation.class, getClass()) + .setFixChooser(FixChoosers.SECOND) + .addInputLines( + "A.java", + "import reactor.core.publisher.Flux;", + "", + "class A {", + " void m() {", + " flux().toIterable();", + " flux().toStream();", + " }", + "", + " Flux flux() {", + " return Flux.just(1, 2, 3);", + " }", + "}") + .addOutputLines( + "A.java", + "import static com.google.common.collect.ImmutableList.toImmutableList;", + "", + "import reactor.core.publisher.Flux;", + "", + "class A {", + " void m() {", + " flux().collect(toImmutableList()).block();", + " flux().collect(toImmutableList()).block().stream();", + " }", + "", + " Flux flux() {", + " return Flux.just(1, 2, 3);", + " }", + "}") + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + } + + @Test + void replacementThirdSuggestedFix() { + BugCheckerRefactoringTestHelper.newInstance(ImplicitBlockingFluxOperation.class, getClass()) + .setFixChooser(FixChoosers.THIRD) + .addInputLines( + "A.java", + "import reactor.core.publisher.Flux;", + "", + "class A {", + " void m() {", + " flux().toIterable();", + " flux().toStream();", + " }", + "", + " Flux flux() {", + " return Flux.just(1, 2, 3);", + " }", + "}") + .addOutputLines( + "A.java", + "import static java.util.stream.Collectors.toUnmodifiableList;", + "", + "import reactor.core.publisher.Flux;", + "", + "class A {", + " void m() {", + " flux().collect(toUnmodifiableList()).block();", + " flux().collect(toUnmodifiableList()).block().stream();", + " }", + "", + " Flux flux() {", + " return Flux.just(1, 2, 3);", + " }", + "}") + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + } +}