From ee5e33eb9910edf8d86d7e510b38c461741444f6 Mon Sep 17 00:00:00 2001 From: Benedek Halasi Date: Fri, 20 Jan 2023 13:32:39 +0100 Subject: [PATCH 01/32] Introduce `Flux.to{Stream,Iterable}` Refaster rules fixes #468 --- .../refasterrules/ReactorRules.java | 33 +++++++++++++++++++ .../refasterrules/ReactorRulesTestInput.java | 12 ++++++- .../refasterrules/ReactorRulesTestOutput.java | 12 ++++++- 3 files changed, 55 insertions(+), 2 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java index c99c77482a..ed3700d421 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java @@ -4,6 +4,7 @@ import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; import static com.google.errorprone.refaster.ImportPolicy.STATIC_IMPORT_ALWAYS; import static java.util.function.Function.identity; +import static java.util.stream.Collectors.toList; import static org.assertj.core.api.Assertions.assertThat; import static reactor.function.TupleUtils.function; @@ -27,6 +28,7 @@ import java.util.function.Function; import java.util.function.Predicate; import java.util.function.Supplier; +import java.util.stream.Stream; import org.jspecify.annotations.Nullable; import org.reactivestreams.Publisher; import reactor.core.publisher.Flux; @@ -1303,4 +1305,35 @@ Duration after(StepVerifier.LastStep step, Duration duration) { return step.verifyTimeout(duration); } } + + /** Avoid accidental blocking with {@link Flux#toStream()} */ + // XXX: The alternative may lose some performance due to + // buffering and prefetching + static final class FluxToStream { + + @BeforeTemplate + Stream before(Flux flux) { + return flux.toStream(); + } + + @AfterTemplate + Stream after(Flux flux) { + return flux.collect(toList()).block().stream(); + } + } + + /** Avoid accidental blocking with {@link Flux#toIterable()} */ + // XXX: The alternative may lose some performance due to buffering and prefetching + static final class FluxToIterable { + + @BeforeTemplate + Iterable before(Flux flux) { + return flux.toIterable(); + } + + @AfterTemplate + Iterable after(Flux flux) { + return flux.collect(toList()).block(); + } + } } diff --git a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestInput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestInput.java index 25596c5cc3..16c3f9f198 100644 --- a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestInput.java +++ b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestInput.java @@ -12,6 +12,8 @@ import java.util.Optional; import java.util.concurrent.Callable; import java.util.function.Supplier; +import java.util.stream.Collectors; +import java.util.stream.Stream; import reactor.core.publisher.Flux; import reactor.core.publisher.Mono; import reactor.test.StepVerifier; @@ -23,7 +25,7 @@ final class ReactorRulesTest implements RefasterRuleCollectionTestCase { @Override public ImmutableSet elidedTypesAndStaticImports() { - return ImmutableSet.of(assertThat(0), HashMap.class, ImmutableMap.class); + return ImmutableSet.of(assertThat(0), Collectors.class, HashMap.class, ImmutableMap.class); } ImmutableSet> testMonoFromSupplier() { @@ -402,4 +404,12 @@ Duration testStepVerifierLastStepVerifyErrorMessage() { Duration testStepVerifierLastStepVerifyTimeout() { return StepVerifier.create(Mono.empty()).expectTimeout(Duration.ZERO).verify(); } + + Stream testFluxToStream() { + return Flux.just(1, 2, 3).toStream(); + } + + Iterable testFluxToIterable() { + return Flux.just(1, 2, 3).toIterable(); + } } diff --git a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestOutput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestOutput.java index 7c471e5314..679a27e1d7 100644 --- a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestOutput.java +++ b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestOutput.java @@ -14,6 +14,8 @@ import java.util.Optional; import java.util.concurrent.Callable; import java.util.function.Supplier; +import java.util.stream.Collectors; +import java.util.stream.Stream; import reactor.core.publisher.Flux; import reactor.core.publisher.Mono; import reactor.function.TupleUtils; @@ -26,7 +28,7 @@ final class ReactorRulesTest implements RefasterRuleCollectionTestCase { @Override public ImmutableSet elidedTypesAndStaticImports() { - return ImmutableSet.of(assertThat(0), HashMap.class, ImmutableMap.class); + return ImmutableSet.of(assertThat(0), Collectors.class, HashMap.class, ImmutableMap.class); } ImmutableSet> testMonoFromSupplier() { @@ -391,4 +393,12 @@ Duration testStepVerifierLastStepVerifyErrorMessage() { Duration testStepVerifierLastStepVerifyTimeout() { return StepVerifier.create(Mono.empty()).verifyTimeout(Duration.ZERO); } + + Stream testFluxToStream() { + return Flux.just(1, 2, 3).collect(Collectors.toList()).block().stream(); + } + + Iterable testFluxToIterable() { + return Flux.just(1, 2, 3).collect(Collectors.toList()).block(); + } } From a125e0b3eea23c9287d2e35c71c3edaa499c3748 Mon Sep 17 00:00:00 2001 From: Benedek Halasi Date: Fri, 20 Jan 2023 14:26:47 +0100 Subject: [PATCH 02/32] fix: Method return type can use a more specific type to convey more information to callers --- .../tech/picnic/errorprone/refasterrules/ReactorRules.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java index ed3700d421..b93e0a554e 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java @@ -20,6 +20,7 @@ import java.time.Duration; import java.util.Comparator; import java.util.HashMap; +import java.util.List; import java.util.Optional; import java.util.concurrent.Callable; import java.util.function.BiConsumer; @@ -1332,7 +1333,7 @@ Iterable before(Flux flux) { } @AfterTemplate - Iterable after(Flux flux) { + List after(Flux flux) { return flux.collect(toList()).block(); } } From c9d1d34efa6575de49b0d9c45fe0ea3ee361fd0a Mon Sep 17 00:00:00 2001 From: Benedek Halasi Date: Fri, 20 Jan 2023 14:40:17 +0100 Subject: [PATCH 03/32] checkstyle --- .../tech/picnic/errorprone/refasterrules/ReactorRules.java | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java index b93e0a554e..66e391cebc 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java @@ -1307,11 +1307,10 @@ Duration after(StepVerifier.LastStep step, Duration duration) { } } - /** Avoid accidental blocking with {@link Flux#toStream()} */ + /** Avoid accidental blocking with {@link Flux#toStream()}. */ // XXX: The alternative may lose some performance due to // buffering and prefetching static final class FluxToStream { - @BeforeTemplate Stream before(Flux flux) { return flux.toStream(); @@ -1323,10 +1322,9 @@ Stream after(Flux flux) { } } - /** Avoid accidental blocking with {@link Flux#toIterable()} */ + /** Avoid accidental blocking with {@link Flux#toIterable()}. */ // XXX: The alternative may lose some performance due to buffering and prefetching static final class FluxToIterable { - @BeforeTemplate Iterable before(Flux flux) { return flux.toIterable(); From d4626ade2f78a0a5523c0435bb8c20141871dddb Mon Sep 17 00:00:00 2001 From: Benedek Halasi Date: Mon, 23 Jan 2023 09:55:06 +0100 Subject: [PATCH 04/32] Revert all "Introduce `Flux.to{Stream,Iterable}` Refaster rules" connected commits --- .../refasterrules/ReactorRules.java | 32 ------------------- .../refasterrules/ReactorRulesTestInput.java | 12 +------ .../refasterrules/ReactorRulesTestOutput.java | 12 +------ 3 files changed, 2 insertions(+), 54 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java index 66e391cebc..c99c77482a 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java @@ -4,7 +4,6 @@ import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; import static com.google.errorprone.refaster.ImportPolicy.STATIC_IMPORT_ALWAYS; import static java.util.function.Function.identity; -import static java.util.stream.Collectors.toList; import static org.assertj.core.api.Assertions.assertThat; import static reactor.function.TupleUtils.function; @@ -20,7 +19,6 @@ import java.time.Duration; import java.util.Comparator; import java.util.HashMap; -import java.util.List; import java.util.Optional; import java.util.concurrent.Callable; import java.util.function.BiConsumer; @@ -29,7 +27,6 @@ import java.util.function.Function; import java.util.function.Predicate; import java.util.function.Supplier; -import java.util.stream.Stream; import org.jspecify.annotations.Nullable; import org.reactivestreams.Publisher; import reactor.core.publisher.Flux; @@ -1306,33 +1303,4 @@ Duration after(StepVerifier.LastStep step, Duration duration) { return step.verifyTimeout(duration); } } - - /** Avoid accidental blocking with {@link Flux#toStream()}. */ - // XXX: The alternative may lose some performance due to - // buffering and prefetching - static final class FluxToStream { - @BeforeTemplate - Stream before(Flux flux) { - return flux.toStream(); - } - - @AfterTemplate - Stream after(Flux flux) { - return flux.collect(toList()).block().stream(); - } - } - - /** Avoid accidental blocking with {@link Flux#toIterable()}. */ - // XXX: The alternative may lose some performance due to buffering and prefetching - static final class FluxToIterable { - @BeforeTemplate - Iterable before(Flux flux) { - return flux.toIterable(); - } - - @AfterTemplate - List after(Flux flux) { - return flux.collect(toList()).block(); - } - } } diff --git a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestInput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestInput.java index 16c3f9f198..25596c5cc3 100644 --- a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestInput.java +++ b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestInput.java @@ -12,8 +12,6 @@ import java.util.Optional; import java.util.concurrent.Callable; import java.util.function.Supplier; -import java.util.stream.Collectors; -import java.util.stream.Stream; import reactor.core.publisher.Flux; import reactor.core.publisher.Mono; import reactor.test.StepVerifier; @@ -25,7 +23,7 @@ final class ReactorRulesTest implements RefasterRuleCollectionTestCase { @Override public ImmutableSet elidedTypesAndStaticImports() { - return ImmutableSet.of(assertThat(0), Collectors.class, HashMap.class, ImmutableMap.class); + return ImmutableSet.of(assertThat(0), HashMap.class, ImmutableMap.class); } ImmutableSet> testMonoFromSupplier() { @@ -404,12 +402,4 @@ Duration testStepVerifierLastStepVerifyErrorMessage() { Duration testStepVerifierLastStepVerifyTimeout() { return StepVerifier.create(Mono.empty()).expectTimeout(Duration.ZERO).verify(); } - - Stream testFluxToStream() { - return Flux.just(1, 2, 3).toStream(); - } - - Iterable testFluxToIterable() { - return Flux.just(1, 2, 3).toIterable(); - } } diff --git a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestOutput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestOutput.java index 679a27e1d7..7c471e5314 100644 --- a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestOutput.java +++ b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestOutput.java @@ -14,8 +14,6 @@ import java.util.Optional; import java.util.concurrent.Callable; import java.util.function.Supplier; -import java.util.stream.Collectors; -import java.util.stream.Stream; import reactor.core.publisher.Flux; import reactor.core.publisher.Mono; import reactor.function.TupleUtils; @@ -28,7 +26,7 @@ final class ReactorRulesTest implements RefasterRuleCollectionTestCase { @Override public ImmutableSet elidedTypesAndStaticImports() { - return ImmutableSet.of(assertThat(0), Collectors.class, HashMap.class, ImmutableMap.class); + return ImmutableSet.of(assertThat(0), HashMap.class, ImmutableMap.class); } ImmutableSet> testMonoFromSupplier() { @@ -393,12 +391,4 @@ Duration testStepVerifierLastStepVerifyErrorMessage() { Duration testStepVerifierLastStepVerifyTimeout() { return StepVerifier.create(Mono.empty()).verifyTimeout(Duration.ZERO); } - - Stream testFluxToStream() { - return Flux.just(1, 2, 3).collect(Collectors.toList()).block().stream(); - } - - Iterable testFluxToIterable() { - return Flux.just(1, 2, 3).collect(Collectors.toList()).block(); - } } From ed4bc82a4a783d6cf6384720812500be18ad1271 Mon Sep 17 00:00:00 2001 From: Benedek Halasi Date: Mon, 23 Jan 2023 10:04:19 +0100 Subject: [PATCH 05/32] 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 0000000000..321473db79 --- /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 0000000000..745b3456e6 --- /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); + } +} From f94bd8757b109878450f621e2fd8dfd315c1f807 Mon Sep 17 00:00:00 2001 From: Benedek Halasi Date: Mon, 23 Jan 2023 10:40:09 +0100 Subject: [PATCH 06/32] checkstyle and javadocs --- .../ImplicitBlockingFluxOperation.java | 22 +++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) 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 index 321473db79..10d87bd847 100644 --- 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 @@ -24,10 +24,18 @@ import com.sun.tools.javac.code.Type; import tech.picnic.errorprone.bugpatterns.util.ThirdPartyLibrary; +/** + * A {@link BugChecker} that flags usages of such Flux methods that are documented to block, but + * that behaviour is not apparent from their signature. + * + *

The alternatives suggested are explicitly blocking operators, highlighting actual behavior. + */ @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.", + "`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, @@ -46,6 +54,9 @@ public class ImplicitBlockingFluxOperation extends BugChecker private static final Matcher FLUX_IMPLICIT_BLOCKING_METHOD = anyOf(FLUX_TO_ITERABLE, FLUX_TO_STREAM); + /** Instantiates a new {@link ImplicitBlockingFluxOperation} instance. */ + public ImplicitBlockingFluxOperation() {} + @Override public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { if (!FLUX_IMPLICIT_BLOCKING_METHOD.matches(tree, state)) { @@ -86,8 +97,11 @@ private static SuggestedFix getUnmodifiableListFix( } /** + * merges `flux.collect(...).block()...` fix into given fix with specified collector and postfix + * to match the original expression tree. + * * @param collector expression - * @return `collect(...).block()...` fix with specified collector and postfix to match the + * @return `flux.collect(...).block()...` fix with specified collector and postfix to match the * original expression tree. */ private static SuggestedFix.Builder getCollectAndBlockFix( @@ -97,13 +111,13 @@ private static SuggestedFix.Builder getCollectAndBlockFix( 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. + * finds the extension of `Flux.collect(...).block()` expression to match the original expression + * tree. */ private static String getCollectAndBlockFixPostfix( MethodInvocationTree tree, VisitorState state) { From 030f0919a014648b92d1210cda73f01ba7ac30ae Mon Sep 17 00:00:00 2001 From: Benedek Halasi Date: Mon, 23 Jan 2023 12:12:46 +0100 Subject: [PATCH 07/32] checkstyle: new-line at the start of a block Co-authored-by: Rick Ossendrijver --- .../bugpatterns/ImplicitBlockingFluxOperationTest.java | 1 - 1 file changed, 1 deletion(-) 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 index 745b3456e6..076e26d3d8 100644 --- 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 @@ -6,7 +6,6 @@ import org.junit.jupiter.api.Test; final class ImplicitBlockingFluxOperationTest { - @Test void identification() { CompilationTestHelper.newInstance(ImplicitBlockingFluxOperation.class, getClass()) From 14165d6eb3c84beb84939576426c99634d65f0d4 Mon Sep 17 00:00:00 2001 From: Benedek Halasi Date: Mon, 23 Jan 2023 12:17:13 +0100 Subject: [PATCH 08/32] convention: refering `BugCheckerRefactoringTestHelper.TestMode` through `BugCheckerRefactoringTestHelper` Co-authored-by: Rick Ossendrijver --- .../bugpatterns/ImplicitBlockingFluxOperationTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 index 076e26d3d8..fd858a37ea 100644 --- 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 @@ -135,6 +135,6 @@ void replacementThirdSuggestedFix() { " return Flux.just(1, 2, 3);", " }", "}") - .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + .doTest(TestMode.TEXT_MATCH); } } From bfcc395d0063096ec429a6417235b7804d045c7e Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Mon, 23 Jan 2023 11:16:12 +0100 Subject: [PATCH 09/32] Suggestions --- .../ImplicitBlockingFluxOperation.java | 18 ++++++++---------- .../ImplicitBlockingFluxOperationTest.java | 2 +- 2 files changed, 9 insertions(+), 11 deletions(-) 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 index 10d87bd847..a157d5e639 100644 --- 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 @@ -17,25 +17,25 @@ 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; /** - * A {@link BugChecker} that flags usages of such Flux methods that are documented to block, but - * that behaviour is not apparent from their signature. + * A {@link BugChecker} that flags usages of Flux methods that are documented to block, but that + * behaviour is not apparent from their signature. * - *

The alternatives suggested are explicitly blocking operators, highlighting actual behavior. + *

The alternatives suggested are explicitly blocking operators, highlighting actual behavior, + * not adequate for automatic replacements, they need manual review. */ @AutoService(BugChecker.class) @BugPattern( - summary = + explanation = "`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.", + summary = "Accidental blocking of `Flux` with convenience method.", link = BUG_PATTERNS_BASE_URL + "ImplicitBlockingFluxOperation", linkType = CUSTOM, severity = ERROR, @@ -43,10 +43,8 @@ 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 MethodMatchers.MethodClassMatcher FLUX_METHOD = + instanceMethod().onDescendantOf(Suppliers.typeFromString("reactor.core.publisher.Flux")); private static final Matcher FLUX_TO_ITERABLE = FLUX_METHOD.named("toIterable").withNoParameters(); private static final Matcher FLUX_TO_STREAM = 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 index fd858a37ea..076e26d3d8 100644 --- 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 @@ -135,6 +135,6 @@ void replacementThirdSuggestedFix() { " return Flux.just(1, 2, 3);", " }", "}") - .doTest(TestMode.TEXT_MATCH); + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); } } From b160ab347042c717f2697c2bcfc7abdbbc13588a Mon Sep 17 00:00:00 2001 From: Benedek Halasi Date: Mon, 23 Jan 2023 12:58:29 +0100 Subject: [PATCH 10/32] Resolve MethodMatchers.MethodClassMatcher deprecation --- .../bugpatterns/ImplicitBlockingFluxOperation.java | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) 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 index a157d5e639..425b61d441 100644 --- 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 @@ -16,7 +16,6 @@ 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.Suppliers; import com.sun.source.tree.ExpressionTree; import com.sun.source.tree.MethodInvocationTree; @@ -43,12 +42,16 @@ public class ImplicitBlockingFluxOperation extends BugChecker implements MethodInvocationTreeMatcher { private static final long serialVersionUID = 1L; - private static final MethodMatchers.MethodClassMatcher FLUX_METHOD = - instanceMethod().onDescendantOf(Suppliers.typeFromString("reactor.core.publisher.Flux")); private static final Matcher FLUX_TO_ITERABLE = - FLUX_METHOD.named("toIterable").withNoParameters(); + instanceMethod() + .onDescendantOf(Suppliers.typeFromString("reactor.core.publisher.Flux")) + .named("toIterable") + .withNoParameters(); private static final Matcher FLUX_TO_STREAM = - FLUX_METHOD.named("toStream").withNoParameters(); + instanceMethod() + .onDescendantOf(Suppliers.typeFromString("reactor.core.publisher.Flux")) + .named("toStream") + .withNoParameters(); private static final Matcher FLUX_IMPLICIT_BLOCKING_METHOD = anyOf(FLUX_TO_ITERABLE, FLUX_TO_STREAM); From 408a96893ac894899d4709440bde02e7b03ccd6b Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Mon, 23 Jan 2023 13:34:29 +0100 Subject: [PATCH 11/32] Suggestions 2 --- .../ImplicitBlockingFluxOperation.java | 57 ++++++++----------- .../ImplicitBlockingFluxOperationTest.java | 23 +++----- 2 files changed, 31 insertions(+), 49 deletions(-) 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 index 425b61d441..ff57ad3522 100644 --- 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 @@ -1,9 +1,8 @@ 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.BugPattern.SeverityLevel.SUGGESTION; +import static com.google.errorprone.BugPattern.StandardTags.STYLE; import static com.google.errorprone.matchers.method.MethodMatchers.instanceMethod; import static tech.picnic.errorprone.bugpatterns.util.Documentation.BUG_PATTERNS_BASE_URL; @@ -19,11 +18,12 @@ import com.google.errorprone.suppliers.Suppliers; import com.sun.source.tree.ExpressionTree; import com.sun.source.tree.MethodInvocationTree; +import tech.picnic.errorprone.bugpatterns.util.SourceCode; import tech.picnic.errorprone.bugpatterns.util.ThirdPartyLibrary; /** - * A {@link BugChecker} that flags usages of Flux methods that are documented to block, but that - * behaviour is not apparent from their signature. + * A {@link BugChecker} that flags usages of {@link reactor.core.publisher.Flux} methods that are + * documented to block, but that behaviour is not apparent from their signature. * *

The alternatives suggested are explicitly blocking operators, highlighting actual behavior, * not adequate for automatic replacements, they need manual review. @@ -33,40 +33,33 @@ explanation = "`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.", - summary = "Accidental blocking of `Flux` with convenience method.", + + "please make sure that they are used with this in mind", + summary = "Accidental blocking of `Flux` with convenience method", link = BUG_PATTERNS_BASE_URL + "ImplicitBlockingFluxOperation", linkType = CUSTOM, - severity = ERROR, - tags = LIKELY_ERROR) -public class ImplicitBlockingFluxOperation extends BugChecker + severity = SUGGESTION, + tags = STYLE) +public final class ImplicitBlockingFluxOperation extends BugChecker implements MethodInvocationTreeMatcher { private static final long serialVersionUID = 1L; - private static final Matcher FLUX_TO_ITERABLE = + private static final Matcher FLUX_WITH_IMPLICIT_BLOCK = instanceMethod() .onDescendantOf(Suppliers.typeFromString("reactor.core.publisher.Flux")) - .named("toIterable") + .namedAnyOf("toIterable", "toStream") .withNoParameters(); - private static final Matcher FLUX_TO_STREAM = - instanceMethod() - .onDescendantOf(Suppliers.typeFromString("reactor.core.publisher.Flux")) - .named("toStream") - .withNoParameters(); - private static final Matcher FLUX_IMPLICIT_BLOCKING_METHOD = - anyOf(FLUX_TO_ITERABLE, FLUX_TO_STREAM); /** Instantiates a new {@link ImplicitBlockingFluxOperation} instance. */ public ImplicitBlockingFluxOperation() {} @Override public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { - if (!FLUX_IMPLICIT_BLOCKING_METHOD.matches(tree, state)) { + if (!FLUX_WITH_IMPLICIT_BLOCK.matches(tree, state)) { return Description.NO_MATCH; } Description.Builder description = buildDescription(tree); - description.addFix(getSuppressWarningsFix(state)); + description.addFix(SuggestedFixes.addSuppressWarnings(state, "ImplicitBlockingFluxOperation")); if (ThirdPartyLibrary.GUAVA.isIntroductionAllowed(state)) { description.addFix(getGuavaFix(tree, state)); } @@ -75,10 +68,6 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState 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 = @@ -98,33 +87,33 @@ private static SuggestedFix getUnmodifiableListFix( } /** - * merges `flux.collect(...).block()...` fix into given fix with specified collector and postfix + * Merges `flux.collect(...).block()...` fix into given fix with specified collector and postfix * to match the original expression tree. * - * @param collector expression + * @param collector expression. * @return `flux.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 source = state.getSourceForNode(tree); + String flux = source.substring(0, source.indexOf(".")); String replacement = String.format("%s.collect(%s).block()%s", flux, collector, postfix); fix.merge(SuggestedFix.replace(tree, replacement)); return fix; } /** - * finds the extension of `Flux.collect(...).block()` expression to match the original expression + * Finds the extension of `Flux.collect(...).block()` expression to match the original expression * tree. */ private static String getCollectAndBlockFixPostfix( MethodInvocationTree tree, VisitorState state) { - if (FLUX_TO_STREAM.matches(tree, state)) { - return ".stream()"; - } - return ""; + return SourceCode.treeToString(tree.getMethodSelect(), state).endsWith("toIterable") + ? "" + : ".stream()"; } } 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 index 076e26d3d8..a984454741 100644 --- 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 @@ -2,6 +2,7 @@ import com.google.errorprone.BugCheckerRefactoringTestHelper; import com.google.errorprone.BugCheckerRefactoringTestHelper.FixChoosers; +import com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode; import com.google.errorprone.CompilationTestHelper; import org.junit.jupiter.api.Test; @@ -61,7 +62,7 @@ void replacementFirstSuggestedFix() { " return Flux.just(1, 2, 3);", " }", "}") - .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + .doTest(TestMode.TEXT_MATCH); } @Test @@ -98,7 +99,7 @@ void replacementSecondSuggestedFix() { " return Flux.just(1, 2, 3);", " }", "}") - .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + .doTest(TestMode.TEXT_MATCH); } @Test @@ -111,12 +112,8 @@ void replacementThirdSuggestedFix() { "", "class A {", " void m() {", - " flux().toIterable();", - " flux().toStream();", - " }", - "", - " Flux flux() {", - " return Flux.just(1, 2, 3);", + " Flux.just(1).toIterable();", + " Flux.just(2).toStream();", " }", "}") .addOutputLines( @@ -127,14 +124,10 @@ void replacementThirdSuggestedFix() { "", "class A {", " void m() {", - " flux().collect(toUnmodifiableList()).block();", - " flux().collect(toUnmodifiableList()).block().stream();", - " }", - "", - " Flux flux() {", - " return Flux.just(1, 2, 3);", + " Flux.just(1).collect(toUnmodifiableList()).block();", + " Flux.just(2).collect(toUnmodifiableList()).block().stream();", " }", "}") - .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + .doTest(TestMode.TEXT_MATCH); } } From 73ff4189e6ff2c95b051cd1cf9f8195ec56166ce Mon Sep 17 00:00:00 2001 From: Benedek Halasi Date: Tue, 24 Jan 2023 10:28:30 +0100 Subject: [PATCH 12/32] Apply suggested changes. matcher: - improve temp solution to handle cases when the matched methodSelect has multiple '.' chars in it. Like `Flux.just(...).toStream()` tests: - Add test `identificationWithoutGuavaOnClasspath` - Apply code-style suggestions/conventions - Can we get the type of the matched expression --- .../ImplicitBlockingFluxOperation.java | 4 +- .../ImplicitBlockingFluxOperationTest.java | 65 ++++++++++--------- 2 files changed, 35 insertions(+), 34 deletions(-) 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 index ff57ad3522..349b03407c 100644 --- 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 @@ -99,8 +99,8 @@ private static SuggestedFix.Builder getCollectAndBlockFix( String postfix = getCollectAndBlockFixPostfix(tree, state); // XXX: replace DIY string replace fix with something more resilient - String source = state.getSourceForNode(tree); - String flux = source.substring(0, source.indexOf(".")); + String methodSelectSource = state.getSourceForNode(tree.getMethodSelect()); + String flux = methodSelectSource.substring(0, methodSelectSource.lastIndexOf(".")); String replacement = String.format("%s.collect(%s).block()%s", flux, collector, postfix); fix.merge(SuggestedFix.replace(tree, replacement)); return fix; 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 index a984454741..c2a08f90b5 100644 --- 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 @@ -2,9 +2,11 @@ import com.google.errorprone.BugCheckerRefactoringTestHelper; import com.google.errorprone.BugCheckerRefactoringTestHelper.FixChoosers; -import com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode; import com.google.errorprone.CompilationTestHelper; import org.junit.jupiter.api.Test; +import org.reactivestreams.Publisher; +import reactor.core.CorePublisher; +import reactor.core.publisher.Flux; final class ImplicitBlockingFluxOperationTest { @Test @@ -17,13 +19,28 @@ void identification() { "class A {", " void m() {", " // BUG: Diagnostic contains:", - " flux().toIterable();", + " Flux.just(1).toIterable();", " // BUG: Diagnostic contains:", - " flux().toStream();", + " Flux.just(2).toStream();", " }", + "}") + .doTest(); + } + + @Test + void identificationWithoutGuavaOnClasspath() { + CompilationTestHelper.newInstance(ImplicitBlockingFluxOperation.class, getClass()) + .withClasspath(Publisher.class, CorePublisher.class, Flux.class) + .addSourceLines( + "A.java", + "import reactor.core.publisher.Flux;", "", - " Flux flux() {", - " return Flux.just(1, 2, 3);", + "class A {", + " void m() {", + " // BUG: Diagnostic contains:", + " Flux.just(1).toIterable();", + " // BUG: Diagnostic contains:", + " Flux.just(2).toStream();", " }", "}") .doTest(); @@ -39,12 +56,8 @@ void replacementFirstSuggestedFix() { "", "class A {", " void m() {", - " flux().toIterable();", - " flux().toStream();", - " }", - "", - " Flux flux() {", - " return Flux.just(1, 2, 3);", + " Flux.just(1).toIterable();", + " Flux.just(2).toStream();", " }", "}") .addOutputLines( @@ -54,15 +67,11 @@ void replacementFirstSuggestedFix() { "class A {", " @SuppressWarnings(\"ImplicitBlockingFluxOperation\")", " void m() {", - " flux().toIterable();", - " flux().toStream();", - " }", - "", - " Flux flux() {", - " return Flux.just(1, 2, 3);", + " Flux.just(1).toIterable();", + " Flux.just(2).toStream();", " }", "}") - .doTest(TestMode.TEXT_MATCH); + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); } @Test @@ -75,12 +84,8 @@ void replacementSecondSuggestedFix() { "", "class A {", " void m() {", - " flux().toIterable();", - " flux().toStream();", - " }", - "", - " Flux flux() {", - " return Flux.just(1, 2, 3);", + " Flux.just(1).toIterable();", + " Flux.just(2).toStream();", " }", "}") .addOutputLines( @@ -91,15 +96,11 @@ void replacementSecondSuggestedFix() { "", "class A {", " void m() {", - " flux().collect(toImmutableList()).block();", - " flux().collect(toImmutableList()).block().stream();", - " }", - "", - " Flux flux() {", - " return Flux.just(1, 2, 3);", + " Flux.just(1).collect(toImmutableList()).block();", + " Flux.just(2).collect(toImmutableList()).block().stream();", " }", "}") - .doTest(TestMode.TEXT_MATCH); + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); } @Test @@ -128,6 +129,6 @@ void replacementThirdSuggestedFix() { " Flux.just(2).collect(toUnmodifiableList()).block().stream();", " }", "}") - .doTest(TestMode.TEXT_MATCH); + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); } } From 0343892c8a60c2a9232184d89dea926e40cfbab4 Mon Sep 17 00:00:00 2001 From: Benedek Halasi Date: Wed, 25 Jan 2023 09:15:23 +0100 Subject: [PATCH 13/32] Refactor implementation for readability and resiliency - implement test for identification without guava on classpath --- .../ImplicitBlockingFluxOperation.java | 141 +++++++++++++----- .../ImplicitBlockingFluxOperationTest.java | 4 + 2 files changed, 111 insertions(+), 34 deletions(-) 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 index 349b03407c..3b886ef086 100644 --- 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 @@ -4,9 +4,12 @@ import static com.google.errorprone.BugPattern.SeverityLevel.SUGGESTION; import static com.google.errorprone.BugPattern.StandardTags.STYLE; import static com.google.errorprone.matchers.method.MethodMatchers.instanceMethod; +import static com.google.errorprone.util.ASTHelpers.*; +import static java.util.Collections.emptySet; import static tech.picnic.errorprone.bugpatterns.util.Documentation.BUG_PATTERNS_BASE_URL; import com.google.auto.service.AutoService; +import com.google.common.collect.ImmutableList; import com.google.errorprone.BugPattern; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker; @@ -16,8 +19,19 @@ import com.google.errorprone.matchers.Description; import com.google.errorprone.matchers.Matcher; import com.google.errorprone.suppliers.Suppliers; +import com.google.errorprone.util.ErrorProneToken; +import com.google.errorprone.util.ErrorProneTokens; import com.sun.source.tree.ExpressionTree; import com.sun.source.tree.MethodInvocationTree; +import com.sun.tools.javac.code.Symbol.TypeSymbol; +import com.sun.tools.javac.code.Type; +import com.sun.tools.javac.util.Position; +import java.util.HashSet; +import java.util.Objects; +import java.util.Optional; +import java.util.Set; +import java.util.stream.Stream; +import javax.annotation.Nullable; import tech.picnic.errorprone.bugpatterns.util.SourceCode; import tech.picnic.errorprone.bugpatterns.util.ThirdPartyLibrary; @@ -69,51 +83,110 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState } private static SuggestedFix getGuavaFix(MethodInvocationTree tree, VisitorState state) { - SuggestedFix.Builder guavaFix = SuggestedFix.builder(); + SuggestedFix.Builder imports = SuggestedFix.builder(); String toImmutableList = SuggestedFixes.qualifyStaticImport( - "com.google.common.collect.ImmutableList.toImmutableList", guavaFix, state); - return getCollectAndBlockFix(tree, state, guavaFix, toImmutableList + "()").build(); + "com.google.common.collect.ImmutableList.toImmutableList", imports, state); + String collector = toImmutableList + "()"; + + return replaceMethodInvocationWithCollect(tree, collector, imports.build(), state); } private static SuggestedFix getUnmodifiableListFix( MethodInvocationTree tree, VisitorState state) { - SuggestedFix.Builder unmodifiableListFix = SuggestedFix.builder(); + SuggestedFix.Builder imports = SuggestedFix.builder(); String toUnmodifiableList = SuggestedFixes.qualifyStaticImport( - "java.util.stream.Collectors.toUnmodifiableList", unmodifiableListFix, state); - return getCollectAndBlockFix(tree, state, unmodifiableListFix, toUnmodifiableList + "()") - .build(); + "java.util.stream.Collectors.toUnmodifiableList", imports, state); + String collector = toUnmodifiableList + "()"; + + return replaceMethodInvocationWithCollect(tree, collector, imports.build(), state); } - /** - * Merges `flux.collect(...).block()...` fix into given fix with specified collector and postfix - * to match the original expression tree. - * - * @param collector expression. - * @return `flux.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 methodSelectSource = state.getSourceForNode(tree.getMethodSelect()); - String flux = methodSelectSource.substring(0, methodSelectSource.lastIndexOf(".")); - String replacement = String.format("%s.collect(%s).block()%s", flux, collector, postfix); - fix.merge(SuggestedFix.replace(tree, replacement)); - return fix; + // XXX: Assumes that the generated `collect(...)` expression will evaluate to + // `Mono>` + // XXX: May return `emptyFix`. + private static SuggestedFix replaceMethodInvocationWithCollect( + MethodInvocationTree tree, + String collectArgument, + SuggestedFix additionalFixes, + VisitorState state) { + String collectMethodInvocation = String.format("collect(%s)", collectArgument); + SuggestedFix.Builder fix = replaceMethodInvocation(tree, collectMethodInvocation, state); + fix.merge(additionalFixes); + + TypeSymbol resultTypeSymbol = + Optional.ofNullable(getResultType(tree)).map(Type::asElement).orElse(null); + + if (Objects.isNull(resultTypeSymbol)) { + return SuggestedFix.emptyFix(); + } + + if (isClassValidSubstituteFor(resultTypeSymbol, Stream.class)) { + fix.postfixWith(tree, ".block().stream()"); + return fix.build(); + } + + if (isClassValidSubstituteFor(resultTypeSymbol, Iterable.class)) { + fix.postfixWith(tree, ".block()"); + return fix.build(); + } + + // XXX: Expected a replacement that evaluates to a type that we don't handle. Should be + // impossible to get here, as we only match `toStream` and `toIterable`. + return SuggestedFix.emptyFix(); } - /** - * Finds the extension of `Flux.collect(...).block()` expression to match the original expression - * tree. - */ - private static String getCollectAndBlockFixPostfix( - MethodInvocationTree tree, VisitorState state) { - return SourceCode.treeToString(tree.getMethodSelect(), state).endsWith("toIterable") - ? "" - : ".stream()"; + // XXX: Assumes that the specified tree is valid, has starting position and contains the matched + // method invocation. + // XXX: Assumes that the specified tree's end is the matched method invocation's end. + private static SuggestedFix.Builder replaceMethodInvocation( + MethodInvocationTree tree, String replacement, VisitorState state) { + ImmutableList tokens = + ErrorProneTokens.getTokens(SourceCode.treeToString(tree, state), state.context); + + int treeStartPosition = getStartPosition(tree); + int methodInvocationStartPosition = + tokens.stream() + .filter(token -> isTokenTheInvokedMethod(tree, token)) + .findFirst() + .map(token -> treeStartPosition + token.pos()) + .orElse(Position.NOPOS); + int methodInvocationEndPosition = treeStartPosition + tokens.get(tokens.size() - 1).endPos(); + + return SuggestedFix.builder() + .replace(methodInvocationStartPosition, methodInvocationEndPosition, replacement); + } + + // XXX: Replace with prewritten solution (?) or rewrite it in a more resilient way. + private static boolean isTokenTheInvokedMethod(MethodInvocationTree tree, ErrorProneToken token) { + return token.hasName() + && Objects.equals(token.name().toString(), getSymbol(tree).getQualifiedName().toString()); + } + + // XXX: Replace with prewritten solution. (?) + private static boolean isClassValidSubstituteFor(TypeSymbol symbol, Class replacement) { + return getAllSuperOf(replacement).stream() + .anyMatch( + clazz -> + Objects.equals( + replacement.getCanonicalName(), symbol.getQualifiedName().toString())); + } + + // XXX: Replace with prewritten solution. (?) + private static Set> getAllSuperOf(@Nullable Class clazz) { + if (Objects.isNull(clazz)) { + return emptySet(); + } + + Set> superTypes = new HashSet<>(); + + superTypes.add(clazz); + for (Class superInterface : clazz.getInterfaces()) { + superTypes.addAll(getAllSuperOf(superInterface)); + } + superTypes.addAll(getAllSuperOf(clazz.getSuperclass())); + + return superTypes; } } 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 index c2a08f90b5..7374d7c0b7 100644 --- 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 @@ -22,6 +22,8 @@ void identification() { " Flux.just(1).toIterable();", " // BUG: Diagnostic contains:", " Flux.just(2).toStream();", + "", + " Flux.just(4).toStream(16);", " }", "}") .doTest(); @@ -115,6 +117,7 @@ void replacementThirdSuggestedFix() { " void m() {", " Flux.just(1).toIterable();", " Flux.just(2).toStream();", + " Flux.just(3).toStream().findAny();", " }", "}") .addOutputLines( @@ -127,6 +130,7 @@ void replacementThirdSuggestedFix() { " void m() {", " Flux.just(1).collect(toUnmodifiableList()).block();", " Flux.just(2).collect(toUnmodifiableList()).block().stream();", + " Flux.just(3).collect(toUnmodifiableList()).block().stream().findAny();", " }", "}") .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); From c244042b2d83b2308f087fb2805802d020b8307e Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Wed, 25 Jan 2023 12:49:57 +0100 Subject: [PATCH 14/32] Suggestions --- .../ImplicitBlockingFluxOperation.java | 74 ++++++++----------- .../ImplicitBlockingFluxOperationTest.java | 16 ++-- 2 files changed, 41 insertions(+), 49 deletions(-) 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 index 3b886ef086..4ae111a40b 100644 --- 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 @@ -4,14 +4,14 @@ import static com.google.errorprone.BugPattern.SeverityLevel.SUGGESTION; import static com.google.errorprone.BugPattern.StandardTags.STYLE; import static com.google.errorprone.matchers.method.MethodMatchers.instanceMethod; -import static com.google.errorprone.util.ASTHelpers.*; -import static java.util.Collections.emptySet; import static tech.picnic.errorprone.bugpatterns.util.Documentation.BUG_PATTERNS_BASE_URL; import com.google.auto.service.AutoService; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableSet; import com.google.errorprone.BugPattern; import com.google.errorprone.VisitorState; +import com.google.errorprone.annotations.Var; import com.google.errorprone.bugpatterns.BugChecker; import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher; import com.google.errorprone.fixes.SuggestedFix; @@ -19,6 +19,7 @@ import com.google.errorprone.matchers.Description; import com.google.errorprone.matchers.Matcher; import com.google.errorprone.suppliers.Suppliers; +import com.google.errorprone.util.ASTHelpers; import com.google.errorprone.util.ErrorProneToken; import com.google.errorprone.util.ErrorProneTokens; import com.sun.source.tree.ExpressionTree; @@ -31,7 +32,7 @@ import java.util.Optional; import java.util.Set; import java.util.stream.Stream; -import javax.annotation.Nullable; +import org.jspecify.annotations.Nullable; import tech.picnic.errorprone.bugpatterns.util.SourceCode; import tech.picnic.errorprone.bugpatterns.util.ThirdPartyLibrary; @@ -44,11 +45,11 @@ */ @AutoService(BugChecker.class) @BugPattern( + summary = "Accidental blocking of `Flux` with convenience method", explanation = "`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", - summary = "Accidental blocking of `Flux` with convenience method", link = BUG_PATTERNS_BASE_URL + "ImplicitBlockingFluxOperation", linkType = CUSTOM, severity = SUGGESTION, @@ -75,66 +76,56 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState description.addFix(SuggestedFixes.addSuppressWarnings(state, "ImplicitBlockingFluxOperation")); if (ThirdPartyLibrary.GUAVA.isIntroductionAllowed(state)) { - description.addFix(getGuavaFix(tree, state)); + description.addFix(trySuggestGuava(tree, state)); } - description.addFix(getUnmodifiableListFix(tree, state)); + description.addFix(trySuggestImmutableList(tree, state)); return description.build(); } - private static SuggestedFix getGuavaFix(MethodInvocationTree tree, VisitorState state) { - SuggestedFix.Builder imports = SuggestedFix.builder(); + private static SuggestedFix trySuggestGuava(MethodInvocationTree tree, VisitorState state) { + SuggestedFix.Builder fix = SuggestedFix.builder(); String toImmutableList = SuggestedFixes.qualifyStaticImport( - "com.google.common.collect.ImmutableList.toImmutableList", imports, state); - String collector = toImmutableList + "()"; + "com.google.common.collect.ImmutableList.toImmutableList", fix, state); - return replaceMethodInvocationWithCollect(tree, collector, imports.build(), state); + return replaceMethodInvocationWithCollect(tree, toImmutableList + "()", fix.build(), state); } - private static SuggestedFix getUnmodifiableListFix( + private static SuggestedFix trySuggestImmutableList( MethodInvocationTree tree, VisitorState state) { - SuggestedFix.Builder imports = SuggestedFix.builder(); - String toUnmodifiableList = + SuggestedFix.Builder fix = SuggestedFix.builder(); + String toUnmodifiableListWithFqcn = SuggestedFixes.qualifyStaticImport( - "java.util.stream.Collectors.toUnmodifiableList", imports, state); - String collector = toUnmodifiableList + "()"; + "java.util.stream.Collectors.toUnmodifiableList", fix, state); - return replaceMethodInvocationWithCollect(tree, collector, imports.build(), state); + return replaceMethodInvocationWithCollect( + tree, toUnmodifiableListWithFqcn + "()", fix.build(), state); } // XXX: Assumes that the generated `collect(...)` expression will evaluate to // `Mono>` - // XXX: May return `emptyFix`. private static SuggestedFix replaceMethodInvocationWithCollect( MethodInvocationTree tree, String collectArgument, - SuggestedFix additionalFixes, + SuggestedFix additionalFix, VisitorState state) { String collectMethodInvocation = String.format("collect(%s)", collectArgument); SuggestedFix.Builder fix = replaceMethodInvocation(tree, collectMethodInvocation, state); - fix.merge(additionalFixes); - - TypeSymbol resultTypeSymbol = - Optional.ofNullable(getResultType(tree)).map(Type::asElement).orElse(null); + fix.merge(additionalFix); - if (Objects.isNull(resultTypeSymbol)) { - return SuggestedFix.emptyFix(); - } - - if (isClassValidSubstituteFor(resultTypeSymbol, Stream.class)) { - fix.postfixWith(tree, ".block().stream()"); - return fix.build(); - } + Optional resultTypeSymbol = + Optional.ofNullable(ASTHelpers.getResultType(tree)).map(Type::asElement); - if (isClassValidSubstituteFor(resultTypeSymbol, Iterable.class)) { - fix.postfixWith(tree, ".block()"); - return fix.build(); + if (resultTypeSymbol.isEmpty()) { + // XXX: Check if this can actually happen... I think it should go well in all cases actually. + throw new IllegalStateException(); } - // XXX: Expected a replacement that evaluates to a type that we don't handle. Should be - // impossible to get here, as we only match `toStream` and `toIterable`. - return SuggestedFix.emptyFix(); + @Var String postfix = ".block()"; + postfix += + isClassValidSubstituteFor(resultTypeSymbol.orElseThrow(), Stream.class) ? ".stream()" : ""; + return fix.postfixWith(tree, postfix).build(); } // XXX: Assumes that the specified tree is valid, has starting position and contains the matched @@ -145,7 +136,7 @@ private static SuggestedFix.Builder replaceMethodInvocation( ImmutableList tokens = ErrorProneTokens.getTokens(SourceCode.treeToString(tree, state), state.context); - int treeStartPosition = getStartPosition(tree); + int treeStartPosition = ASTHelpers.getStartPosition(tree); int methodInvocationStartPosition = tokens.stream() .filter(token -> isTokenTheInvokedMethod(tree, token)) @@ -160,8 +151,7 @@ private static SuggestedFix.Builder replaceMethodInvocation( // XXX: Replace with prewritten solution (?) or rewrite it in a more resilient way. private static boolean isTokenTheInvokedMethod(MethodInvocationTree tree, ErrorProneToken token) { - return token.hasName() - && Objects.equals(token.name().toString(), getSymbol(tree).getQualifiedName().toString()); + return token.hasName() && token.name().equals(ASTHelpers.getSymbol(tree).getQualifiedName()); } // XXX: Replace with prewritten solution. (?) @@ -175,8 +165,8 @@ private static boolean isClassValidSubstituteFor(TypeSymbol symbol, Class rep // XXX: Replace with prewritten solution. (?) private static Set> getAllSuperOf(@Nullable Class clazz) { - if (Objects.isNull(clazz)) { - return emptySet(); + if (clazz == null) { + return ImmutableSet.of(); } Set> superTypes = new HashSet<>(); 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 index 7374d7c0b7..ce7b03efc6 100644 --- 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 @@ -2,6 +2,7 @@ import com.google.errorprone.BugCheckerRefactoringTestHelper; import com.google.errorprone.BugCheckerRefactoringTestHelper.FixChoosers; +import com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode; import com.google.errorprone.CompilationTestHelper; import org.junit.jupiter.api.Test; import org.reactivestreams.Publisher; @@ -22,8 +23,10 @@ void identification() { " Flux.just(1).toIterable();", " // BUG: Diagnostic contains:", " Flux.just(2).toStream();", + " // BUG: Diagnostic contains:", + " long count = Flux.just(3).toStream().count();", "", - " Flux.just(4).toStream(16);", + " Flux.just(3).toStream(16);", " }", "}") .doTest(); @@ -51,7 +54,6 @@ void identificationWithoutGuavaOnClasspath() { @Test void replacementFirstSuggestedFix() { BugCheckerRefactoringTestHelper.newInstance(ImplicitBlockingFluxOperation.class, getClass()) - .setFixChooser(FixChoosers.FIRST) .addInputLines( "A.java", "import reactor.core.publisher.Flux;", @@ -73,7 +75,7 @@ void replacementFirstSuggestedFix() { " Flux.just(2).toStream();", " }", "}") - .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + .doTest(TestMode.TEXT_MATCH); } @Test @@ -87,7 +89,7 @@ void replacementSecondSuggestedFix() { "class A {", " void m() {", " Flux.just(1).toIterable();", - " Flux.just(2).toStream();", + " Flux.just(2).toStream().count();", " }", "}") .addOutputLines( @@ -99,10 +101,10 @@ void replacementSecondSuggestedFix() { "class A {", " void m() {", " Flux.just(1).collect(toImmutableList()).block();", - " Flux.just(2).collect(toImmutableList()).block().stream();", + " Flux.just(2).collect(toImmutableList()).block().stream().count();", " }", "}") - .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + .doTest(TestMode.TEXT_MATCH); } @Test @@ -133,6 +135,6 @@ void replacementThirdSuggestedFix() { " Flux.just(3).collect(toUnmodifiableList()).block().stream().findAny();", " }", "}") - .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + .doTest(TestMode.TEXT_MATCH); } } From 7fc2527f013632a682bdda32a09edeb32a2503ca Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Wed, 25 Jan 2023 12:59:55 +0100 Subject: [PATCH 15/32] Drop semi-duplicate method and inline a method --- .../ImplicitBlockingFluxOperation.java | 37 +++++++------------ 1 file changed, 13 insertions(+), 24 deletions(-) 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 index 4ae111a40b..977334cf75 100644 --- 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 @@ -76,31 +76,22 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState description.addFix(SuggestedFixes.addSuppressWarnings(state, "ImplicitBlockingFluxOperation")); if (ThirdPartyLibrary.GUAVA.isIntroductionAllowed(state)) { - description.addFix(trySuggestGuava(tree, state)); + description.addFix( + trySuggestFix("com.google.common.collect.ImmutableList.toImmutableList", tree, state)); } - description.addFix(trySuggestImmutableList(tree, state)); + description.addFix( + trySuggestFix("java.util.stream.Collectors.toUnmodifiableList", tree, state)); return description.build(); } - private static SuggestedFix trySuggestGuava(MethodInvocationTree tree, VisitorState state) { + private static SuggestedFix trySuggestFix( + String fullyQualifiedMethodInvocation, MethodInvocationTree tree, VisitorState state) { SuggestedFix.Builder fix = SuggestedFix.builder(); - String toImmutableList = - SuggestedFixes.qualifyStaticImport( - "com.google.common.collect.ImmutableList.toImmutableList", fix, state); + String replacement = + SuggestedFixes.qualifyStaticImport(fullyQualifiedMethodInvocation, fix, state); - return replaceMethodInvocationWithCollect(tree, toImmutableList + "()", fix.build(), state); - } - - private static SuggestedFix trySuggestImmutableList( - MethodInvocationTree tree, VisitorState state) { - SuggestedFix.Builder fix = SuggestedFix.builder(); - String toUnmodifiableListWithFqcn = - SuggestedFixes.qualifyStaticImport( - "java.util.stream.Collectors.toUnmodifiableList", fix, state); - - return replaceMethodInvocationWithCollect( - tree, toUnmodifiableListWithFqcn + "()", fix.build(), state); + return replaceMethodInvocationWithCollect(tree, replacement + "()", fix.build(), state); } // XXX: Assumes that the generated `collect(...)` expression will evaluate to @@ -139,7 +130,10 @@ private static SuggestedFix.Builder replaceMethodInvocation( int treeStartPosition = ASTHelpers.getStartPosition(tree); int methodInvocationStartPosition = tokens.stream() - .filter(token -> isTokenTheInvokedMethod(tree, token)) + .filter( + token -> + token.hasName() + && token.name().equals(ASTHelpers.getSymbol(tree).getQualifiedName())) .findFirst() .map(token -> treeStartPosition + token.pos()) .orElse(Position.NOPOS); @@ -149,11 +143,6 @@ private static SuggestedFix.Builder replaceMethodInvocation( .replace(methodInvocationStartPosition, methodInvocationEndPosition, replacement); } - // XXX: Replace with prewritten solution (?) or rewrite it in a more resilient way. - private static boolean isTokenTheInvokedMethod(MethodInvocationTree tree, ErrorProneToken token) { - return token.hasName() && token.name().equals(ASTHelpers.getSymbol(tree).getQualifiedName()); - } - // XXX: Replace with prewritten solution. (?) private static boolean isClassValidSubstituteFor(TypeSymbol symbol, Class replacement) { return getAllSuperOf(replacement).stream() From ac931e4276d75c0a503885782feba9c5008cfc53 Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Wed, 25 Jan 2023 13:36:28 +0100 Subject: [PATCH 16/32] Use `Types#isSubType` in combination with `MoreTypes` DSL --- .../ImplicitBlockingFluxOperation.java | 54 ++++--------------- 1 file changed, 9 insertions(+), 45 deletions(-) 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 index 977334cf75..17389d7396 100644 --- 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 @@ -5,34 +5,29 @@ import static com.google.errorprone.BugPattern.StandardTags.STYLE; import static com.google.errorprone.matchers.method.MethodMatchers.instanceMethod; import static tech.picnic.errorprone.bugpatterns.util.Documentation.BUG_PATTERNS_BASE_URL; +import static tech.picnic.errorprone.bugpatterns.util.MoreTypes.generic; +import static tech.picnic.errorprone.bugpatterns.util.MoreTypes.unbound; import com.google.auto.service.AutoService; import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableSet; import com.google.errorprone.BugPattern; import com.google.errorprone.VisitorState; -import com.google.errorprone.annotations.Var; 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.suppliers.Supplier; import com.google.errorprone.suppliers.Suppliers; import com.google.errorprone.util.ASTHelpers; import com.google.errorprone.util.ErrorProneToken; import com.google.errorprone.util.ErrorProneTokens; import com.sun.source.tree.ExpressionTree; import com.sun.source.tree.MethodInvocationTree; -import com.sun.tools.javac.code.Symbol.TypeSymbol; import com.sun.tools.javac.code.Type; import com.sun.tools.javac.util.Position; -import java.util.HashSet; -import java.util.Objects; -import java.util.Optional; -import java.util.Set; import java.util.stream.Stream; -import org.jspecify.annotations.Nullable; import tech.picnic.errorprone.bugpatterns.util.SourceCode; import tech.picnic.errorprone.bugpatterns.util.ThirdPartyLibrary; @@ -62,6 +57,8 @@ public final class ImplicitBlockingFluxOperation extends BugChecker .onDescendantOf(Suppliers.typeFromString("reactor.core.publisher.Flux")) .namedAnyOf("toIterable", "toStream") .withNoParameters(); + private static final Supplier STREAM = + VisitorState.memoize(generic(Suppliers.typeFromClass(Stream.class), unbound())); /** Instantiates a new {@link ImplicitBlockingFluxOperation} instance. */ public ImplicitBlockingFluxOperation() {} @@ -105,17 +102,10 @@ private static SuggestedFix replaceMethodInvocationWithCollect( SuggestedFix.Builder fix = replaceMethodInvocation(tree, collectMethodInvocation, state); fix.merge(additionalFix); - Optional resultTypeSymbol = - Optional.ofNullable(ASTHelpers.getResultType(tree)).map(Type::asElement); - - if (resultTypeSymbol.isEmpty()) { - // XXX: Check if this can actually happen... I think it should go well in all cases actually. - throw new IllegalStateException(); - } - - @Var String postfix = ".block()"; - postfix += - isClassValidSubstituteFor(resultTypeSymbol.orElseThrow(), Stream.class) ? ".stream()" : ""; + String postfix = + state.getTypes().isSubtype(ASTHelpers.getResultType(tree), STREAM.get(state)) + ? ".block().stream()" + : ".block()"; return fix.postfixWith(tree, postfix).build(); } @@ -142,30 +132,4 @@ private static SuggestedFix.Builder replaceMethodInvocation( return SuggestedFix.builder() .replace(methodInvocationStartPosition, methodInvocationEndPosition, replacement); } - - // XXX: Replace with prewritten solution. (?) - private static boolean isClassValidSubstituteFor(TypeSymbol symbol, Class replacement) { - return getAllSuperOf(replacement).stream() - .anyMatch( - clazz -> - Objects.equals( - replacement.getCanonicalName(), symbol.getQualifiedName().toString())); - } - - // XXX: Replace with prewritten solution. (?) - private static Set> getAllSuperOf(@Nullable Class clazz) { - if (clazz == null) { - return ImmutableSet.of(); - } - - Set> superTypes = new HashSet<>(); - - superTypes.add(clazz); - for (Class superInterface : clazz.getInterfaces()) { - superTypes.addAll(getAllSuperOf(superInterface)); - } - superTypes.addAll(getAllSuperOf(clazz.getSuperclass())); - - return superTypes; - } } From 2f1e6da6aad0de6d1dce8d740f7fc06b0933a09e Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Wed, 25 Jan 2023 14:41:19 +0100 Subject: [PATCH 17/32] Discussed this with @Stephan202 :) --- .../bugpatterns/ImplicitBlockingFluxOperation.java | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) 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 index 17389d7396..0288b4fa2f 100644 --- 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 @@ -5,8 +5,6 @@ import static com.google.errorprone.BugPattern.StandardTags.STYLE; import static com.google.errorprone.matchers.method.MethodMatchers.instanceMethod; import static tech.picnic.errorprone.bugpatterns.util.Documentation.BUG_PATTERNS_BASE_URL; -import static tech.picnic.errorprone.bugpatterns.util.MoreTypes.generic; -import static tech.picnic.errorprone.bugpatterns.util.MoreTypes.unbound; import com.google.auto.service.AutoService; import com.google.common.collect.ImmutableList; @@ -26,8 +24,8 @@ import com.sun.source.tree.ExpressionTree; import com.sun.source.tree.MethodInvocationTree; import com.sun.tools.javac.code.Type; +import com.sun.tools.javac.code.Types; import com.sun.tools.javac.util.Position; -import java.util.stream.Stream; import tech.picnic.errorprone.bugpatterns.util.SourceCode; import tech.picnic.errorprone.bugpatterns.util.ThirdPartyLibrary; @@ -57,8 +55,7 @@ public final class ImplicitBlockingFluxOperation extends BugChecker .onDescendantOf(Suppliers.typeFromString("reactor.core.publisher.Flux")) .namedAnyOf("toIterable", "toStream") .withNoParameters(); - private static final Supplier STREAM = - VisitorState.memoize(generic(Suppliers.typeFromClass(Stream.class), unbound())); + private static final Supplier STREAM = Suppliers.typeFromString("java.util.stream.Stream"); /** Instantiates a new {@link ImplicitBlockingFluxOperation} instance. */ public ImplicitBlockingFluxOperation() {} @@ -102,8 +99,9 @@ private static SuggestedFix replaceMethodInvocationWithCollect( SuggestedFix.Builder fix = replaceMethodInvocation(tree, collectMethodInvocation, state); fix.merge(additionalFix); + Types types = state.getTypes(); String postfix = - state.getTypes().isSubtype(ASTHelpers.getResultType(tree), STREAM.get(state)) + types.isSubtype(ASTHelpers.getResultType(tree), types.erasure(STREAM.get(state))) ? ".block().stream()" : ".block()"; return fix.postfixWith(tree, postfix).build(); From e97dde7a37f7facbab1a9fa249853036ea63c9bb Mon Sep 17 00:00:00 2001 From: Benedek Halasi Date: Sun, 29 Jan 2023 18:37:01 +0100 Subject: [PATCH 18/32] resolve `replaceMethodInvocation` method's assumptions with `Optional` --- .../ImplicitBlockingFluxOperation.java | 49 +++++++++++-------- 1 file changed, 28 insertions(+), 21 deletions(-) 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 index 0288b4fa2f..d4c9d9bfd8 100644 --- 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 @@ -26,6 +26,7 @@ import com.sun.tools.javac.code.Type; import com.sun.tools.javac.code.Types; import com.sun.tools.javac.util.Position; +import java.util.Optional; import tech.picnic.errorprone.bugpatterns.util.SourceCode; import tech.picnic.errorprone.bugpatterns.util.ThirdPartyLibrary; @@ -70,16 +71,16 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState description.addFix(SuggestedFixes.addSuppressWarnings(state, "ImplicitBlockingFluxOperation")); if (ThirdPartyLibrary.GUAVA.isIntroductionAllowed(state)) { - description.addFix( - trySuggestFix("com.google.common.collect.ImmutableList.toImmutableList", tree, state)); + trySuggestFix("com.google.common.collect.ImmutableList.toImmutableList", tree, state) + .ifPresent(description::addFix); } - description.addFix( - trySuggestFix("java.util.stream.Collectors.toUnmodifiableList", tree, state)); + trySuggestFix("java.util.stream.Collectors.toUnmodifiableList", tree, state) + .ifPresent(description::addFix); return description.build(); } - private static SuggestedFix trySuggestFix( + private static Optional trySuggestFix( String fullyQualifiedMethodInvocation, MethodInvocationTree tree, VisitorState state) { SuggestedFix.Builder fix = SuggestedFix.builder(); String replacement = @@ -90,27 +91,27 @@ private static SuggestedFix trySuggestFix( // XXX: Assumes that the generated `collect(...)` expression will evaluate to // `Mono>` - private static SuggestedFix replaceMethodInvocationWithCollect( + private static Optional replaceMethodInvocationWithCollect( MethodInvocationTree tree, String collectArgument, SuggestedFix additionalFix, VisitorState state) { String collectMethodInvocation = String.format("collect(%s)", collectArgument); - SuggestedFix.Builder fix = replaceMethodInvocation(tree, collectMethodInvocation, state); - fix.merge(additionalFix); - - Types types = state.getTypes(); - String postfix = - types.isSubtype(ASTHelpers.getResultType(tree), types.erasure(STREAM.get(state))) - ? ".block().stream()" - : ".block()"; - return fix.postfixWith(tree, postfix).build(); + return replaceMethodInvocation(tree, collectMethodInvocation, state) + .map(fix -> fix.merge(additionalFix)) + .map( + fix -> { + Types types = state.getTypes(); + String postfix = + types.isSubtype(ASTHelpers.getResultType(tree), types.erasure(STREAM.get(state))) + ? ".block().stream()" + : ".block()"; + return fix.postfixWith(tree, postfix); + }) + .map(SuggestedFix.Builder::build); } - // XXX: Assumes that the specified tree is valid, has starting position and contains the matched - // method invocation. - // XXX: Assumes that the specified tree's end is the matched method invocation's end. - private static SuggestedFix.Builder replaceMethodInvocation( + private static Optional replaceMethodInvocation( MethodInvocationTree tree, String replacement, VisitorState state) { ImmutableList tokens = ErrorProneTokens.getTokens(SourceCode.treeToString(tree, state), state.context); @@ -125,9 +126,15 @@ private static SuggestedFix.Builder replaceMethodInvocation( .findFirst() .map(token -> treeStartPosition + token.pos()) .orElse(Position.NOPOS); + + if (treeStartPosition == Position.NOPOS || methodInvocationStartPosition == Position.NOPOS) { + return Optional.empty(); + } + int methodInvocationEndPosition = treeStartPosition + tokens.get(tokens.size() - 1).endPos(); - return SuggestedFix.builder() - .replace(methodInvocationStartPosition, methodInvocationEndPosition, replacement); + return Optional.of( + SuggestedFix.builder() + .replace(methodInvocationStartPosition, methodInvocationEndPosition, replacement)); } } From 54aa877e0c0f8d562b55b7be4e839fbe359e4de5 Mon Sep 17 00:00:00 2001 From: Benedek Halasi Date: Sun, 29 Jan 2023 22:14:06 +0100 Subject: [PATCH 19/32] implement helper class and verify that matcher only picks up Flux methods with it --- .../errorprone/bugpatterns/util/NotFlux.java | 34 +++++++++++++++++++ .../ImplicitBlockingFluxOperationTest.java | 3 ++ .../bugpatterns/util/NotFluxTest.java | 13 +++++++ 3 files changed, 50 insertions(+) create mode 100644 error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/NotFlux.java create mode 100644 error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/NotFluxTest.java diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/NotFlux.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/NotFlux.java new file mode 100644 index 0000000000..86a91eb1c4 --- /dev/null +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/NotFlux.java @@ -0,0 +1,34 @@ +package tech.picnic.errorprone.bugpatterns.util; + +import com.google.common.collect.ImmutableList; +import java.util.stream.Stream; +import reactor.core.publisher.Flux; + +/** + * Class that has members like {@link Flux} but is not related to it. + * + *

This class meant to help verify that matchers only pick up {@link Flux} members instead of + * everything that has the same signature. + * + * @param has no purpose other than to match the signature of {@link Flux}. + */ +public final class NotFlux { + /** + * mocks {@link Flux#toIterable()}. + * + * @return empty {@link Iterable}. + */ + @SuppressWarnings("PreferredInterfaceType") + public Iterable toIterable() { + return ImmutableList.of(); + } + + /** + * mocks {@link Flux#toStream()}. + * + * @return empty {@link Stream}. + */ + public Stream toStream() { + return Stream.empty(); + } +} 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 index ce7b03efc6..465cef9072 100644 --- 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 @@ -16,6 +16,7 @@ void identification() { .addSourceLines( "A.java", "import reactor.core.publisher.Flux;", + "import tech.picnic.errorprone.bugpatterns.util.NotFlux;", "", "class A {", " void m() {", @@ -27,6 +28,8 @@ void identification() { " long count = Flux.just(3).toStream().count();", "", " Flux.just(3).toStream(16);", + " new NotFlux().toIterable();", + " new NotFlux().toStream();", " }", "}") .doTest(); diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/NotFluxTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/NotFluxTest.java new file mode 100644 index 0000000000..9ba08a2f7f --- /dev/null +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/NotFluxTest.java @@ -0,0 +1,13 @@ +package tech.picnic.errorprone.bugpatterns.util; + +import org.junit.jupiter.api.Test; + +final class NotFluxTest { + @SuppressWarnings("UnusedVariable") + @Test + void signature() { + NotFlux notFlux = new NotFlux<>(); + var unusedIterable = notFlux.toIterable(); + var unusedStream = notFlux.toStream(); + } +} From 8ff5b12121a1ea6e5819b51a58360d1c376743d6 Mon Sep 17 00:00:00 2001 From: Benedek Halasi Date: Mon, 30 Jan 2023 00:30:20 +0100 Subject: [PATCH 20/32] javadoc for default constructor --- .../java/tech/picnic/errorprone/bugpatterns/util/NotFlux.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/NotFlux.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/NotFlux.java index 86a91eb1c4..5e786a9c61 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/NotFlux.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/NotFlux.java @@ -13,6 +13,9 @@ * @param has no purpose other than to match the signature of {@link Flux}. */ public final class NotFlux { + /** Instantiates a new {@link NotFlux} instance. */ + public NotFlux() {} + /** * mocks {@link Flux#toIterable()}. * From cf15383e4d4654579e74fb34ac9bbc63daacf51c Mon Sep 17 00:00:00 2001 From: Benedek Halasi Date: Tue, 31 Jan 2023 06:26:35 +0100 Subject: [PATCH 21/32] kill `NULL_RETURNS` mutant --- .../picnic/errorprone/bugpatterns/util/NotFluxTest.java | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/NotFluxTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/NotFluxTest.java index 9ba08a2f7f..857379e1d1 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/NotFluxTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/NotFluxTest.java @@ -1,13 +1,14 @@ package tech.picnic.errorprone.bugpatterns.util; +import static org.assertj.core.api.Assertions.assertThat; + import org.junit.jupiter.api.Test; final class NotFluxTest { - @SuppressWarnings("UnusedVariable") @Test void signature() { NotFlux notFlux = new NotFlux<>(); - var unusedIterable = notFlux.toIterable(); - var unusedStream = notFlux.toStream(); + assertThat(notFlux.toIterable()).isNotNull(); + assertThat(notFlux.toStream()).isNotNull(); } } From 22fb2dd12982be9dade61234c2babda0dceeb1e6 Mon Sep 17 00:00:00 2001 From: Benedek Halasi Date: Tue, 31 Jan 2023 08:23:18 +0100 Subject: [PATCH 22/32] making tests fail when guava is introduced despite that's not allowed --- .../bugpatterns/ImplicitBlockingFluxOperationTest.java | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) 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 index 465cef9072..e3f6b3ed07 100644 --- 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 @@ -1,5 +1,8 @@ package tech.picnic.errorprone.bugpatterns; +import static com.google.common.base.Predicates.containsPattern; +import static com.google.common.base.Predicates.not; + import com.google.errorprone.BugCheckerRefactoringTestHelper; import com.google.errorprone.BugCheckerRefactoringTestHelper.FixChoosers; import com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode; @@ -39,15 +42,17 @@ void identification() { void identificationWithoutGuavaOnClasspath() { CompilationTestHelper.newInstance(ImplicitBlockingFluxOperation.class, getClass()) .withClasspath(Publisher.class, CorePublisher.class, Flux.class) + .expectErrorMessage( + "X", not(containsPattern("toImmutableList"))) .addSourceLines( "A.java", "import reactor.core.publisher.Flux;", "", "class A {", " void m() {", - " // BUG: Diagnostic contains:", + " // BUG: Diagnostic matches: X", " Flux.just(1).toIterable();", - " // BUG: Diagnostic contains:", + " // BUG: Diagnostic matches: X", " Flux.just(2).toStream();", " }", "}") From 0273b3fab0695409fbe826961c21c5862a804d4f Mon Sep 17 00:00:00 2001 From: Benedek Halasi Date: Tue, 31 Jan 2023 08:32:44 +0100 Subject: [PATCH 23/32] formatting --- .../bugpatterns/ImplicitBlockingFluxOperationTest.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) 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 index e3f6b3ed07..8b736780d0 100644 --- 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 @@ -42,8 +42,7 @@ void identification() { void identificationWithoutGuavaOnClasspath() { CompilationTestHelper.newInstance(ImplicitBlockingFluxOperation.class, getClass()) .withClasspath(Publisher.class, CorePublisher.class, Flux.class) - .expectErrorMessage( - "X", not(containsPattern("toImmutableList"))) + .expectErrorMessage("X", not(containsPattern("toImmutableList"))) .addSourceLines( "A.java", "import reactor.core.publisher.Flux;", From 0ae778b764e9caa6aee8bf25132a8a183460d703 Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Wed, 1 Feb 2023 09:53:21 +0100 Subject: [PATCH 24/32] Suggestions --- .../ImplicitBlockingFluxOperation.java | 17 +++------ .../errorprone/bugpatterns/util/NotFlux.java | 37 ------------------- .../ImplicitBlockingFluxOperationTest.java | 15 ++++++-- .../bugpatterns/util/NotFluxTest.java | 14 ------- 4 files changed, 18 insertions(+), 65 deletions(-) delete mode 100644 error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/NotFlux.java delete mode 100644 error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/NotFluxTest.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 index d4c9d9bfd8..ab6a92c00f 100644 --- 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 @@ -89,25 +89,20 @@ private static Optional trySuggestFix( return replaceMethodInvocationWithCollect(tree, replacement + "()", fix.build(), state); } - // XXX: Assumes that the generated `collect(...)` expression will evaluate to - // `Mono>` private static Optional replaceMethodInvocationWithCollect( MethodInvocationTree tree, String collectArgument, SuggestedFix additionalFix, VisitorState state) { String collectMethodInvocation = String.format("collect(%s)", collectArgument); + Types types = state.getTypes(); + String postFix = + types.isSubtype(ASTHelpers.getResultType(tree), types.erasure(STREAM.get(state))) + ? ".block().stream()" + : ".block()"; return replaceMethodInvocation(tree, collectMethodInvocation, state) .map(fix -> fix.merge(additionalFix)) - .map( - fix -> { - Types types = state.getTypes(); - String postfix = - types.isSubtype(ASTHelpers.getResultType(tree), types.erasure(STREAM.get(state))) - ? ".block().stream()" - : ".block()"; - return fix.postfixWith(tree, postfix); - }) + .map(fix -> fix.postfixWith(tree, postFix)) .map(SuggestedFix.Builder::build); } diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/NotFlux.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/NotFlux.java deleted file mode 100644 index 5e786a9c61..0000000000 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/NotFlux.java +++ /dev/null @@ -1,37 +0,0 @@ -package tech.picnic.errorprone.bugpatterns.util; - -import com.google.common.collect.ImmutableList; -import java.util.stream.Stream; -import reactor.core.publisher.Flux; - -/** - * Class that has members like {@link Flux} but is not related to it. - * - *

This class meant to help verify that matchers only pick up {@link Flux} members instead of - * everything that has the same signature. - * - * @param has no purpose other than to match the signature of {@link Flux}. - */ -public final class NotFlux { - /** Instantiates a new {@link NotFlux} instance. */ - public NotFlux() {} - - /** - * mocks {@link Flux#toIterable()}. - * - * @return empty {@link Iterable}. - */ - @SuppressWarnings("PreferredInterfaceType") - public Iterable toIterable() { - return ImmutableList.of(); - } - - /** - * mocks {@link Flux#toStream()}. - * - * @return empty {@link Stream}. - */ - public Stream toStream() { - return Stream.empty(); - } -} 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 index 8b736780d0..2fe917bd78 100644 --- 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 @@ -19,7 +19,6 @@ void identification() { .addSourceLines( "A.java", "import reactor.core.publisher.Flux;", - "import tech.picnic.errorprone.bugpatterns.util.NotFlux;", "", "class A {", " void m() {", @@ -31,8 +30,18 @@ void identification() { " long count = Flux.just(3).toStream().count();", "", " Flux.just(3).toStream(16);", - " new NotFlux().toIterable();", - " new NotFlux().toStream();", + " new Foo().toIterable();", + " new Foo().toStream();", + " }", + "", + " public final class Foo {", + " public Iterable toIterable() {", + " return ImmutableList.of();", + " }", + "", + " public Stream toStream() {", + " return Stream.empty();", + " }", " }", "}") .doTest(); diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/NotFluxTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/NotFluxTest.java deleted file mode 100644 index 857379e1d1..0000000000 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/NotFluxTest.java +++ /dev/null @@ -1,14 +0,0 @@ -package tech.picnic.errorprone.bugpatterns.util; - -import static org.assertj.core.api.Assertions.assertThat; - -import org.junit.jupiter.api.Test; - -final class NotFluxTest { - @Test - void signature() { - NotFlux notFlux = new NotFlux<>(); - assertThat(notFlux.toIterable()).isNotNull(); - assertThat(notFlux.toStream()).isNotNull(); - } -} From 3e07247c1902fe690fdbffd824f7c707620e0f61 Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Wed, 1 Feb 2023 10:28:22 +0100 Subject: [PATCH 25/32] Add missing imports --- .../bugpatterns/ImplicitBlockingFluxOperationTest.java | 2 ++ 1 file changed, 2 insertions(+) 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 index 2fe917bd78..961047a203 100644 --- 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 @@ -18,6 +18,8 @@ void identification() { CompilationTestHelper.newInstance(ImplicitBlockingFluxOperation.class, getClass()) .addSourceLines( "A.java", + "import com.google.common.collect.ImmutableList;", + "import java.util.stream.Stream;", "import reactor.core.publisher.Flux;", "", "class A {", From b242c7837ebdb9dc2ea407cccfda05e43f5b8387 Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Wed, 8 Feb 2023 17:15:49 +0100 Subject: [PATCH 26/32] Suggestions and rename --- ...eration.java => ImplicitBlockingFlux.java} | 25 ++++++++----------- ...est.java => ImplicitBlockingFluxTest.java} | 24 +++++++++--------- 2 files changed, 23 insertions(+), 26 deletions(-) rename error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/{ImplicitBlockingFluxOperation.java => ImplicitBlockingFlux.java} (85%) rename error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/{ImplicitBlockingFluxOperationTest.java => ImplicitBlockingFluxTest.java} (85%) 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/ImplicitBlockingFlux.java similarity index 85% rename from error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ImplicitBlockingFluxOperation.java rename to error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ImplicitBlockingFlux.java index ab6a92c00f..b19c52f7ac 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ImplicitBlockingFluxOperation.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ImplicitBlockingFlux.java @@ -31,25 +31,23 @@ import tech.picnic.errorprone.bugpatterns.util.ThirdPartyLibrary; /** - * A {@link BugChecker} that flags usages of {@link reactor.core.publisher.Flux} methods that are - * documented to block, but that behaviour is not apparent from their signature. + * A {@link BugChecker} that flags {@link reactor.core.publisher.Flux} operators that are implicitly + * blocking. * *

The alternatives suggested are explicitly blocking operators, highlighting actual behavior, * not adequate for automatic replacements, they need manual review. + * + *

`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. */ @AutoService(BugChecker.class) @BugPattern( summary = "Accidental blocking of `Flux` with convenience method", - explanation = - "`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", + link = BUG_PATTERNS_BASE_URL + "ImplicitBlockingFlux", linkType = CUSTOM, severity = SUGGESTION, tags = STYLE) -public final class ImplicitBlockingFluxOperation extends BugChecker - implements MethodInvocationTreeMatcher { +public final class ImplicitBlockingFlux extends BugChecker implements MethodInvocationTreeMatcher { private static final long serialVersionUID = 1L; private static final Matcher FLUX_WITH_IMPLICIT_BLOCK = instanceMethod() @@ -58,8 +56,8 @@ public final class ImplicitBlockingFluxOperation extends BugChecker .withNoParameters(); private static final Supplier STREAM = Suppliers.typeFromString("java.util.stream.Stream"); - /** Instantiates a new {@link ImplicitBlockingFluxOperation} instance. */ - public ImplicitBlockingFluxOperation() {} + /** Instantiates a new {@link ImplicitBlockingFlux} instance. */ + public ImplicitBlockingFlux() {} @Override public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { @@ -69,13 +67,12 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState Description.Builder description = buildDescription(tree); - description.addFix(SuggestedFixes.addSuppressWarnings(state, "ImplicitBlockingFluxOperation")); + description.addFix(SuggestedFixes.addSuppressWarnings(state, canonicalName())); if (ThirdPartyLibrary.GUAVA.isIntroductionAllowed(state)) { trySuggestFix("com.google.common.collect.ImmutableList.toImmutableList", tree, state) .ifPresent(description::addFix); } - trySuggestFix("java.util.stream.Collectors.toUnmodifiableList", tree, state) - .ifPresent(description::addFix); + trySuggestFix("java.util.stream.Collectors.toList", tree, state).ifPresent(description::addFix); return description.build(); } 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/ImplicitBlockingFluxTest.java similarity index 85% rename from error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ImplicitBlockingFluxOperationTest.java rename to error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ImplicitBlockingFluxTest.java index 961047a203..69989c176c 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ImplicitBlockingFluxOperationTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ImplicitBlockingFluxTest.java @@ -12,10 +12,10 @@ import reactor.core.CorePublisher; import reactor.core.publisher.Flux; -final class ImplicitBlockingFluxOperationTest { +final class ImplicitBlockingFluxTest { @Test void identification() { - CompilationTestHelper.newInstance(ImplicitBlockingFluxOperation.class, getClass()) + CompilationTestHelper.newInstance(ImplicitBlockingFlux.class, getClass()) .addSourceLines( "A.java", "import com.google.common.collect.ImmutableList;", @@ -51,8 +51,8 @@ void identification() { @Test void identificationWithoutGuavaOnClasspath() { - CompilationTestHelper.newInstance(ImplicitBlockingFluxOperation.class, getClass()) - .withClasspath(Publisher.class, CorePublisher.class, Flux.class) + CompilationTestHelper.newInstance(ImplicitBlockingFlux.class, getClass()) + .withClasspath(CorePublisher.class, Flux.class, Publisher.class) .expectErrorMessage("X", not(containsPattern("toImmutableList"))) .addSourceLines( "A.java", @@ -71,7 +71,7 @@ void identificationWithoutGuavaOnClasspath() { @Test void replacementFirstSuggestedFix() { - BugCheckerRefactoringTestHelper.newInstance(ImplicitBlockingFluxOperation.class, getClass()) + BugCheckerRefactoringTestHelper.newInstance(ImplicitBlockingFlux.class, getClass()) .addInputLines( "A.java", "import reactor.core.publisher.Flux;", @@ -87,7 +87,7 @@ void replacementFirstSuggestedFix() { "import reactor.core.publisher.Flux;", "", "class A {", - " @SuppressWarnings(\"ImplicitBlockingFluxOperation\")", + " @SuppressWarnings(\"ImplicitBlockingFlux\")", " void m() {", " Flux.just(1).toIterable();", " Flux.just(2).toStream();", @@ -98,7 +98,7 @@ void replacementFirstSuggestedFix() { @Test void replacementSecondSuggestedFix() { - BugCheckerRefactoringTestHelper.newInstance(ImplicitBlockingFluxOperation.class, getClass()) + BugCheckerRefactoringTestHelper.newInstance(ImplicitBlockingFlux.class, getClass()) .setFixChooser(FixChoosers.SECOND) .addInputLines( "A.java", @@ -127,7 +127,7 @@ void replacementSecondSuggestedFix() { @Test void replacementThirdSuggestedFix() { - BugCheckerRefactoringTestHelper.newInstance(ImplicitBlockingFluxOperation.class, getClass()) + BugCheckerRefactoringTestHelper.newInstance(ImplicitBlockingFlux.class, getClass()) .setFixChooser(FixChoosers.THIRD) .addInputLines( "A.java", @@ -142,15 +142,15 @@ void replacementThirdSuggestedFix() { "}") .addOutputLines( "A.java", - "import static java.util.stream.Collectors.toUnmodifiableList;", + "import static java.util.stream.Collectors.toList;", "", "import reactor.core.publisher.Flux;", "", "class A {", " void m() {", - " Flux.just(1).collect(toUnmodifiableList()).block();", - " Flux.just(2).collect(toUnmodifiableList()).block().stream();", - " Flux.just(3).collect(toUnmodifiableList()).block().stream().findAny();", + " Flux.just(1).collect(toList()).block();", + " Flux.just(2).collect(toList()).block().stream();", + " Flux.just(3).collect(toList()).block().stream().findAny();", " }", "}") .doTest(TestMode.TEXT_MATCH); From c90c588fa548d4fbff7e67308e5738363b3ad777 Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Thu, 9 Feb 2023 11:11:54 +0100 Subject: [PATCH 27/32] Suggestions --- .../bugpatterns/ImplicitBlockingFlux.java | 60 ++++++------------- .../bugpatterns/ImplicitBlockingFluxTest.java | 7 ++- 2 files changed, 21 insertions(+), 46 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ImplicitBlockingFlux.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ImplicitBlockingFlux.java index b19c52f7ac..54e35aa0c9 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ImplicitBlockingFlux.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ImplicitBlockingFlux.java @@ -1,5 +1,6 @@ package tech.picnic.errorprone.bugpatterns; +import static com.google.common.base.Preconditions.checkState; import static com.google.errorprone.BugPattern.LinkType.CUSTOM; import static com.google.errorprone.BugPattern.SeverityLevel.SUGGESTION; import static com.google.errorprone.BugPattern.StandardTags.STYLE; @@ -7,7 +8,6 @@ import static tech.picnic.errorprone.bugpatterns.util.Documentation.BUG_PATTERNS_BASE_URL; import com.google.auto.service.AutoService; -import com.google.common.collect.ImmutableList; import com.google.errorprone.BugPattern; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker; @@ -19,30 +19,20 @@ import com.google.errorprone.suppliers.Supplier; import com.google.errorprone.suppliers.Suppliers; import com.google.errorprone.util.ASTHelpers; -import com.google.errorprone.util.ErrorProneToken; -import com.google.errorprone.util.ErrorProneTokens; import com.sun.source.tree.ExpressionTree; import com.sun.source.tree.MethodInvocationTree; import com.sun.tools.javac.code.Type; import com.sun.tools.javac.code.Types; import com.sun.tools.javac.util.Position; -import java.util.Optional; -import tech.picnic.errorprone.bugpatterns.util.SourceCode; import tech.picnic.errorprone.bugpatterns.util.ThirdPartyLibrary; /** * A {@link BugChecker} that flags {@link reactor.core.publisher.Flux} operators that are implicitly * blocking. - * - *

The alternatives suggested are explicitly blocking operators, highlighting actual behavior, - * not adequate for automatic replacements, they need manual review. - * - *

`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. */ @AutoService(BugChecker.class) @BugPattern( - summary = "Accidental blocking of `Flux` with convenience method", + summary = "Avoid using `Flux` operators that implicitly block", link = BUG_PATTERNS_BASE_URL + "ImplicitBlockingFlux", linkType = CUSTOM, severity = SUGGESTION, @@ -69,15 +59,15 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState description.addFix(SuggestedFixes.addSuppressWarnings(state, canonicalName())); if (ThirdPartyLibrary.GUAVA.isIntroductionAllowed(state)) { - trySuggestFix("com.google.common.collect.ImmutableList.toImmutableList", tree, state) - .ifPresent(description::addFix); + description.addFix( + trySuggestFix("com.google.common.collect.ImmutableList.toImmutableList", tree, state)); } - trySuggestFix("java.util.stream.Collectors.toList", tree, state).ifPresent(description::addFix); + description.addFix(trySuggestFix("java.util.stream.Collectors.toList", tree, state)); return description.build(); } - private static Optional trySuggestFix( + private static SuggestedFix trySuggestFix( String fullyQualifiedMethodInvocation, MethodInvocationTree tree, VisitorState state) { SuggestedFix.Builder fix = SuggestedFix.builder(); String replacement = @@ -86,47 +76,31 @@ private static Optional trySuggestFix( return replaceMethodInvocationWithCollect(tree, replacement + "()", fix.build(), state); } - private static Optional replaceMethodInvocationWithCollect( + private static SuggestedFix replaceMethodInvocationWithCollect( MethodInvocationTree tree, String collectArgument, SuggestedFix additionalFix, VisitorState state) { String collectMethodInvocation = String.format("collect(%s)", collectArgument); Types types = state.getTypes(); - String postFix = + String explicitBlock = types.isSubtype(ASTHelpers.getResultType(tree), types.erasure(STREAM.get(state))) ? ".block().stream()" : ".block()"; return replaceMethodInvocation(tree, collectMethodInvocation, state) - .map(fix -> fix.merge(additionalFix)) - .map(fix -> fix.postfixWith(tree, postFix)) - .map(SuggestedFix.Builder::build); + .merge(additionalFix) + .postfixWith(tree, explicitBlock) + .build(); } - private static Optional replaceMethodInvocation( + private static SuggestedFix.Builder replaceMethodInvocation( MethodInvocationTree tree, String replacement, VisitorState state) { - ImmutableList tokens = - ErrorProneTokens.getTokens(SourceCode.treeToString(tree, state), state.context); + String methodName = ASTHelpers.getSymbol(tree).getQualifiedName().toString(); - int treeStartPosition = ASTHelpers.getStartPosition(tree); - int methodInvocationStartPosition = - tokens.stream() - .filter( - token -> - token.hasName() - && token.name().equals(ASTHelpers.getSymbol(tree).getQualifiedName())) - .findFirst() - .map(token -> treeStartPosition + token.pos()) - .orElse(Position.NOPOS); + int endPosition = state.getEndPosition(tree); + checkState(endPosition != Position.NOPOS, "Cannot determine location of method in source code"); + int startPosition = endPosition - methodName.length() - 2; - if (treeStartPosition == Position.NOPOS || methodInvocationStartPosition == Position.NOPOS) { - return Optional.empty(); - } - - int methodInvocationEndPosition = treeStartPosition + tokens.get(tokens.size() - 1).endPos(); - - return Optional.of( - SuggestedFix.builder() - .replace(methodInvocationStartPosition, methodInvocationEndPosition, replacement)); + return SuggestedFix.builder().replace(startPosition, endPosition, replacement); } } diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ImplicitBlockingFluxTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ImplicitBlockingFluxTest.java index 69989c176c..382bff1057 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ImplicitBlockingFluxTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ImplicitBlockingFluxTest.java @@ -2,9 +2,10 @@ import static com.google.common.base.Predicates.containsPattern; import static com.google.common.base.Predicates.not; +import static com.google.errorprone.BugCheckerRefactoringTestHelper.FixChoosers.SECOND; +import static com.google.errorprone.BugCheckerRefactoringTestHelper.FixChoosers.THIRD; import com.google.errorprone.BugCheckerRefactoringTestHelper; -import com.google.errorprone.BugCheckerRefactoringTestHelper.FixChoosers; import com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode; import com.google.errorprone.CompilationTestHelper; import org.junit.jupiter.api.Test; @@ -99,7 +100,7 @@ void replacementFirstSuggestedFix() { @Test void replacementSecondSuggestedFix() { BugCheckerRefactoringTestHelper.newInstance(ImplicitBlockingFlux.class, getClass()) - .setFixChooser(FixChoosers.SECOND) + .setFixChooser(SECOND) .addInputLines( "A.java", "import reactor.core.publisher.Flux;", @@ -128,7 +129,7 @@ void replacementSecondSuggestedFix() { @Test void replacementThirdSuggestedFix() { BugCheckerRefactoringTestHelper.newInstance(ImplicitBlockingFlux.class, getClass()) - .setFixChooser(FixChoosers.THIRD) + .setFixChooser(THIRD) .addInputLines( "A.java", "import reactor.core.publisher.Flux;", From 4a3eb9307f9b4b543291ab55e4f0640c510b8cac Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Thu, 9 Feb 2023 11:18:39 +0100 Subject: [PATCH 28/32] Naming tweaks --- .../bugpatterns/ImplicitBlockingFlux.java | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ImplicitBlockingFlux.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ImplicitBlockingFlux.java index 54e35aa0c9..a3bba29888 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ImplicitBlockingFlux.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ImplicitBlockingFlux.java @@ -69,26 +69,26 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState private static SuggestedFix trySuggestFix( String fullyQualifiedMethodInvocation, MethodInvocationTree tree, VisitorState state) { - SuggestedFix.Builder fix = SuggestedFix.builder(); + SuggestedFix.Builder importFix = SuggestedFix.builder(); String replacement = - SuggestedFixes.qualifyStaticImport(fullyQualifiedMethodInvocation, fix, state); + SuggestedFixes.qualifyStaticImport(fullyQualifiedMethodInvocation, importFix, state); - return replaceMethodInvocationWithCollect(tree, replacement + "()", fix.build(), state); + return replaceWithExplicitBlock(tree, replacement + "()", importFix.build(), state); } - private static SuggestedFix replaceMethodInvocationWithCollect( + private static SuggestedFix replaceWithExplicitBlock( MethodInvocationTree tree, String collectArgument, - SuggestedFix additionalFix, + SuggestedFix importFix, VisitorState state) { - String collectMethodInvocation = String.format("collect(%s)", collectArgument); + String collect = String.format("collect(%s)", collectArgument); Types types = state.getTypes(); String explicitBlock = types.isSubtype(ASTHelpers.getResultType(tree), types.erasure(STREAM.get(state))) ? ".block().stream()" : ".block()"; - return replaceMethodInvocation(tree, collectMethodInvocation, state) - .merge(additionalFix) + return replaceMethodInvocation(tree, collect, state) + .merge(importFix) .postfixWith(tree, explicitBlock) .build(); } From c8afb35003f48f39a0ce6e7ca87c548336338ba9 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Sat, 18 Feb 2023 18:03:55 +0100 Subject: [PATCH 29/32] Suggestions --- .../bugpatterns/ImplicitBlockingFlux.java | 69 ++++++++----------- .../bugpatterns/ImplicitBlockingFluxTest.java | 39 ++++++++--- 2 files changed, 58 insertions(+), 50 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ImplicitBlockingFlux.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ImplicitBlockingFlux.java index a3bba29888..9461ee1775 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ImplicitBlockingFlux.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ImplicitBlockingFlux.java @@ -2,8 +2,9 @@ import static com.google.common.base.Preconditions.checkState; import static com.google.errorprone.BugPattern.LinkType.CUSTOM; -import static com.google.errorprone.BugPattern.SeverityLevel.SUGGESTION; -import static com.google.errorprone.BugPattern.StandardTags.STYLE; +import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; +import static com.google.errorprone.BugPattern.StandardTags.CONCURRENCY; +import static com.google.errorprone.BugPattern.StandardTags.PERFORMANCE; import static com.google.errorprone.matchers.method.MethodMatchers.instanceMethod; import static tech.picnic.errorprone.bugpatterns.util.Documentation.BUG_PATTERNS_BASE_URL; @@ -22,8 +23,8 @@ import com.sun.source.tree.ExpressionTree; import com.sun.source.tree.MethodInvocationTree; import com.sun.tools.javac.code.Type; -import com.sun.tools.javac.code.Types; import com.sun.tools.javac.util.Position; +import java.util.stream.Stream; import tech.picnic.errorprone.bugpatterns.util.ThirdPartyLibrary; /** @@ -35,16 +36,16 @@ summary = "Avoid using `Flux` operators that implicitly block", link = BUG_PATTERNS_BASE_URL + "ImplicitBlockingFlux", linkType = CUSTOM, - severity = SUGGESTION, - tags = STYLE) + severity = WARNING, + tags = {CONCURRENCY, PERFORMANCE}) public final class ImplicitBlockingFlux extends BugChecker implements MethodInvocationTreeMatcher { private static final long serialVersionUID = 1L; private static final Matcher FLUX_WITH_IMPLICIT_BLOCK = instanceMethod() - .onDescendantOf(Suppliers.typeFromString("reactor.core.publisher.Flux")) + .onDescendantOf("reactor.core.publisher.Flux") .namedAnyOf("toIterable", "toStream") .withNoParameters(); - private static final Supplier STREAM = Suppliers.typeFromString("java.util.stream.Stream"); + private static final Supplier STREAM = Suppliers.typeFromString(Stream.class.getName()); /** Instantiates a new {@link ImplicitBlockingFlux} instance. */ public ImplicitBlockingFlux() {} @@ -55,51 +56,41 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState return Description.NO_MATCH; } - Description.Builder description = buildDescription(tree); - - description.addFix(SuggestedFixes.addSuppressWarnings(state, canonicalName())); + Description.Builder description = + buildDescription(tree).addFix(SuggestedFixes.addSuppressWarnings(state, canonicalName())); if (ThirdPartyLibrary.GUAVA.isIntroductionAllowed(state)) { description.addFix( - trySuggestFix("com.google.common.collect.ImmutableList.toImmutableList", tree, state)); + suggestBlockingElementCollection( + tree, "com.google.common.collect.ImmutableList.toImmutableList", state)); } - description.addFix(trySuggestFix("java.util.stream.Collectors.toList", tree, state)); + description.addFix( + suggestBlockingElementCollection(tree, "java.util.stream.Collectors.toList", state)); return description.build(); } - private static SuggestedFix trySuggestFix( - String fullyQualifiedMethodInvocation, MethodInvocationTree tree, VisitorState state) { - SuggestedFix.Builder importFix = SuggestedFix.builder(); - String replacement = - SuggestedFixes.qualifyStaticImport(fullyQualifiedMethodInvocation, importFix, state); - - return replaceWithExplicitBlock(tree, replacement + "()", importFix.build(), state); - } + private static SuggestedFix suggestBlockingElementCollection( + MethodInvocationTree tree, String fullyQualifiedCollectorMethod, VisitorState state) { + SuggestedFix.Builder importSuggestion = SuggestedFix.builder(); + String replacementMethodInvocation = + SuggestedFixes.qualifyStaticImport(fullyQualifiedCollectorMethod, importSuggestion, state); - private static SuggestedFix replaceWithExplicitBlock( - MethodInvocationTree tree, - String collectArgument, - SuggestedFix importFix, - VisitorState state) { - String collect = String.format("collect(%s)", collectArgument); - Types types = state.getTypes(); - String explicitBlock = - types.isSubtype(ASTHelpers.getResultType(tree), types.erasure(STREAM.get(state))) - ? ".block().stream()" - : ".block()"; - return replaceMethodInvocation(tree, collect, state) - .merge(importFix) - .postfixWith(tree, explicitBlock) - .build(); + boolean isStream = + ASTHelpers.isSubtype(ASTHelpers.getResultType(tree), STREAM.get(state), state); + String replacement = + String.format( + ".collect(%s()).block()%s", replacementMethodInvocation, isStream ? ".stream()" : ""); + return importSuggestion.merge(replaceMethodInvocation(tree, replacement, state)).build(); } private static SuggestedFix.Builder replaceMethodInvocation( MethodInvocationTree tree, String replacement, VisitorState state) { - String methodName = ASTHelpers.getSymbol(tree).getQualifiedName().toString(); - + int startPosition = state.getEndPosition(ASTHelpers.getReceiver(tree)); int endPosition = state.getEndPosition(tree); - checkState(endPosition != Position.NOPOS, "Cannot determine location of method in source code"); - int startPosition = endPosition - methodName.length() - 2; + + checkState( + startPosition != Position.NOPOS && endPosition != Position.NOPOS, + "Cannot locate method to be replaced in source code"); return SuggestedFix.builder().replace(startPosition, endPosition, replacement); } diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ImplicitBlockingFluxTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ImplicitBlockingFluxTest.java index 382bff1057..3722c0be82 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ImplicitBlockingFluxTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ImplicitBlockingFluxTest.java @@ -17,6 +17,7 @@ final class ImplicitBlockingFluxTest { @Test void identification() { CompilationTestHelper.newInstance(ImplicitBlockingFlux.class, getClass()) + .expectErrorMessage("X", containsPattern("SuppressWarnings.*toImmutableList.*toList")) .addSourceLines( "A.java", "import com.google.common.collect.ImmutableList;", @@ -25,24 +26,26 @@ void identification() { "", "class A {", " void m() {", - " // BUG: Diagnostic contains:", + " // BUG: Diagnostic matches: X", " Flux.just(1).toIterable();", - " // BUG: Diagnostic contains:", + " // BUG: Diagnostic matches: X", " Flux.just(2).toStream();", - " // BUG: Diagnostic contains:", + " // BUG: Diagnostic matches: X", " long count = Flux.just(3).toStream().count();", "", - " Flux.just(3).toStream(16);", + " Flux.just(4).toIterable(1);", + " Flux.just(5).toIterable(2, null);", + " Flux.just(6).toStream(3);", " new Foo().toIterable();", " new Foo().toStream();", " }", "", - " public final class Foo {", - " public Iterable toIterable() {", + " class Foo {", + " Iterable toIterable() {", " return ImmutableList.of();", " }", "", - " public Stream toStream() {", + " Stream toStream() {", " return Stream.empty();", " }", " }", @@ -108,7 +111,11 @@ void replacementSecondSuggestedFix() { "class A {", " void m() {", " Flux.just(1).toIterable();", - " Flux.just(2).toStream().count();", + " Flux.just(2).toStream();", + " Flux.just(3).toIterable().iterator();", + " Flux.just(4).toStream().count();", + " Flux.just(5) /* a */./* b */ toIterable /* c */(/* d */ ) /* e */;", + " Flux.just(6) /* a */./* b */ toStream /* c */(/* d */ ) /* e */;", " }", "}") .addOutputLines( @@ -120,7 +127,11 @@ void replacementSecondSuggestedFix() { "class A {", " void m() {", " Flux.just(1).collect(toImmutableList()).block();", - " Flux.just(2).collect(toImmutableList()).block().stream().count();", + " Flux.just(2).collect(toImmutableList()).block().stream();", + " Flux.just(3).collect(toImmutableList()).block().iterator();", + " Flux.just(4).collect(toImmutableList()).block().stream().count();", + " Flux.just(5).collect(toImmutableList()).block() /* e */;", + " Flux.just(6).collect(toImmutableList()).block().stream() /* e */;", " }", "}") .doTest(TestMode.TEXT_MATCH); @@ -138,7 +149,10 @@ void replacementThirdSuggestedFix() { " void m() {", " Flux.just(1).toIterable();", " Flux.just(2).toStream();", - " Flux.just(3).toStream().findAny();", + " Flux.just(3).toIterable().iterator();", + " Flux.just(4).toStream().count();", + " Flux.just(5) /* a */./* b */ toIterable /* c */(/* d */ ) /* e */;", + " Flux.just(6) /* a */./* b */ toStream /* c */(/* d */ ) /* e */;", " }", "}") .addOutputLines( @@ -151,7 +165,10 @@ void replacementThirdSuggestedFix() { " void m() {", " Flux.just(1).collect(toList()).block();", " Flux.just(2).collect(toList()).block().stream();", - " Flux.just(3).collect(toList()).block().stream().findAny();", + " Flux.just(3).collect(toList()).block().iterator();", + " Flux.just(4).collect(toList()).block().stream().count();", + " Flux.just(5).collect(toList()).block() /* e */;", + " Flux.just(6).collect(toList()).block().stream() /* e */;", " }", "}") .doTest(TestMode.TEXT_MATCH); From db3be54bd5a07e81077f902a8ec650fa1720f392 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Sun, 19 Feb 2023 10:13:56 +0100 Subject: [PATCH 30/32] More accurate phrasing --- .../errorprone/bugpatterns/ImplicitBlockingFlux.java | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ImplicitBlockingFlux.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ImplicitBlockingFlux.java index 9461ee1775..8f16f6f8f8 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ImplicitBlockingFlux.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ImplicitBlockingFlux.java @@ -28,12 +28,15 @@ import tech.picnic.errorprone.bugpatterns.util.ThirdPartyLibrary; /** - * A {@link BugChecker} that flags {@link reactor.core.publisher.Flux} operators that are implicitly - * blocking. + * A {@link BugChecker} that flags {@link reactor.core.publisher.Flux} operator usages that may + * implicitly cause the calling thread to be blocked. + * + *

Note that the methods flagged here are not themselves blocking, but iterating over the + * resulting {@link Iterable} or {@link Stream} may be. */ @AutoService(BugChecker.class) @BugPattern( - summary = "Avoid using `Flux` operators that implicitly block", + summary = "Avoid iterating over `Flux`es in an implicitly blocking manner", link = BUG_PATTERNS_BASE_URL + "ImplicitBlockingFlux", linkType = CUSTOM, severity = WARNING, From 4de090ebc6e7644efaf0f43e7f429d5d70a33e4d Mon Sep 17 00:00:00 2001 From: Benedek Halasi Date: Sun, 19 Feb 2023 12:12:24 +0100 Subject: [PATCH 31/32] Allow suggestion order changes in `identification` test, since the order is enforced by other tests. --- .../errorprone/bugpatterns/ImplicitBlockingFluxTest.java | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ImplicitBlockingFluxTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ImplicitBlockingFluxTest.java index 3722c0be82..e3a1cd9bee 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ImplicitBlockingFluxTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ImplicitBlockingFluxTest.java @@ -1,5 +1,6 @@ package tech.picnic.errorprone.bugpatterns; +import static com.google.common.base.Predicates.and; import static com.google.common.base.Predicates.containsPattern; import static com.google.common.base.Predicates.not; import static com.google.errorprone.BugCheckerRefactoringTestHelper.FixChoosers.SECOND; @@ -17,7 +18,12 @@ final class ImplicitBlockingFluxTest { @Test void identification() { CompilationTestHelper.newInstance(ImplicitBlockingFlux.class, getClass()) - .expectErrorMessage("X", containsPattern("SuppressWarnings.*toImmutableList.*toList")) + .expectErrorMessage( + "X", + and( + containsPattern("SuppressWarnings"), + containsPattern("toImmutableList"), + containsPattern("toList"))) .addSourceLines( "A.java", "import com.google.common.collect.ImmutableList;", From 56067cbb69a4f640e36f4a6b1b99bcc9ac97c194 Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Mon, 20 Feb 2023 09:15:26 +0100 Subject: [PATCH 32/32] `s/ImplicitBlockingFlux/FluxImplicitBlock/` --- ...citBlockingFlux.java => FluxImplicitBlock.java} | 8 ++++---- ...ingFluxTest.java => FluxImplicitBlockTest.java} | 14 +++++++------- 2 files changed, 11 insertions(+), 11 deletions(-) rename error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/{ImplicitBlockingFlux.java => FluxImplicitBlock.java} (94%) rename error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/{ImplicitBlockingFluxTest.java => FluxImplicitBlockTest.java} (92%) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ImplicitBlockingFlux.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/FluxImplicitBlock.java similarity index 94% rename from error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ImplicitBlockingFlux.java rename to error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/FluxImplicitBlock.java index 8f16f6f8f8..17c8098c09 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ImplicitBlockingFlux.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/FluxImplicitBlock.java @@ -37,11 +37,11 @@ @AutoService(BugChecker.class) @BugPattern( summary = "Avoid iterating over `Flux`es in an implicitly blocking manner", - link = BUG_PATTERNS_BASE_URL + "ImplicitBlockingFlux", + link = BUG_PATTERNS_BASE_URL + "FluxImplicitBlock", linkType = CUSTOM, severity = WARNING, tags = {CONCURRENCY, PERFORMANCE}) -public final class ImplicitBlockingFlux extends BugChecker implements MethodInvocationTreeMatcher { +public final class FluxImplicitBlock extends BugChecker implements MethodInvocationTreeMatcher { private static final long serialVersionUID = 1L; private static final Matcher FLUX_WITH_IMPLICIT_BLOCK = instanceMethod() @@ -50,8 +50,8 @@ public final class ImplicitBlockingFlux extends BugChecker implements MethodInvo .withNoParameters(); private static final Supplier STREAM = Suppliers.typeFromString(Stream.class.getName()); - /** Instantiates a new {@link ImplicitBlockingFlux} instance. */ - public ImplicitBlockingFlux() {} + /** Instantiates a new {@link FluxImplicitBlock} instance. */ + public FluxImplicitBlock() {} @Override public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ImplicitBlockingFluxTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/FluxImplicitBlockTest.java similarity index 92% rename from error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ImplicitBlockingFluxTest.java rename to error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/FluxImplicitBlockTest.java index e3a1cd9bee..6390b2d041 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ImplicitBlockingFluxTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/FluxImplicitBlockTest.java @@ -14,10 +14,10 @@ import reactor.core.CorePublisher; import reactor.core.publisher.Flux; -final class ImplicitBlockingFluxTest { +final class FluxImplicitBlockTest { @Test void identification() { - CompilationTestHelper.newInstance(ImplicitBlockingFlux.class, getClass()) + CompilationTestHelper.newInstance(FluxImplicitBlock.class, getClass()) .expectErrorMessage( "X", and( @@ -61,7 +61,7 @@ void identification() { @Test void identificationWithoutGuavaOnClasspath() { - CompilationTestHelper.newInstance(ImplicitBlockingFlux.class, getClass()) + CompilationTestHelper.newInstance(FluxImplicitBlock.class, getClass()) .withClasspath(CorePublisher.class, Flux.class, Publisher.class) .expectErrorMessage("X", not(containsPattern("toImmutableList"))) .addSourceLines( @@ -81,7 +81,7 @@ void identificationWithoutGuavaOnClasspath() { @Test void replacementFirstSuggestedFix() { - BugCheckerRefactoringTestHelper.newInstance(ImplicitBlockingFlux.class, getClass()) + BugCheckerRefactoringTestHelper.newInstance(FluxImplicitBlock.class, getClass()) .addInputLines( "A.java", "import reactor.core.publisher.Flux;", @@ -97,7 +97,7 @@ void replacementFirstSuggestedFix() { "import reactor.core.publisher.Flux;", "", "class A {", - " @SuppressWarnings(\"ImplicitBlockingFlux\")", + " @SuppressWarnings(\"FluxImplicitBlock\")", " void m() {", " Flux.just(1).toIterable();", " Flux.just(2).toStream();", @@ -108,7 +108,7 @@ void replacementFirstSuggestedFix() { @Test void replacementSecondSuggestedFix() { - BugCheckerRefactoringTestHelper.newInstance(ImplicitBlockingFlux.class, getClass()) + BugCheckerRefactoringTestHelper.newInstance(FluxImplicitBlock.class, getClass()) .setFixChooser(SECOND) .addInputLines( "A.java", @@ -145,7 +145,7 @@ void replacementSecondSuggestedFix() { @Test void replacementThirdSuggestedFix() { - BugCheckerRefactoringTestHelper.newInstance(ImplicitBlockingFlux.class, getClass()) + BugCheckerRefactoringTestHelper.newInstance(FluxImplicitBlock.class, getClass()) .setFixChooser(THIRD) .addInputLines( "A.java",