From 56e9e399e4846a8729f09be7563c7f5758bec201 Mon Sep 17 00:00:00 2001 From: Dima Legeza Date: Mon, 11 Dec 2023 13:26:55 +0100 Subject: [PATCH] PR comments - Mohamed & Rick --- .../bugpatterns/MonoZipOfMonoVoidUsage.java | 92 ++++++++++--------- .../MonoZipOfMonoVoidUsageTest.java | 27 +++--- 2 files changed, 64 insertions(+), 55 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MonoZipOfMonoVoidUsage.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MonoZipOfMonoVoidUsage.java index c747d0d0eaa..d888aeba0dc 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MonoZipOfMonoVoidUsage.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MonoZipOfMonoVoidUsage.java @@ -8,7 +8,6 @@ import static com.google.errorprone.matchers.Matchers.instanceMethod; import static com.google.errorprone.matchers.Matchers.staticMethod; import static com.google.errorprone.matchers.Matchers.toType; -import static com.google.errorprone.util.ASTHelpers.isSameType; 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.type; @@ -28,16 +27,21 @@ import com.sun.source.tree.MethodInvocationTree; import com.sun.tools.javac.code.Type; import com.sun.tools.javac.tree.JCTree; +import java.util.Optional; /** - * A {@link BugChecker} that flags usages of {@code Mono.zip(Mono, Mono)} and {@code - * Mono.zipWith(Mono)} with {@code Mono.empty()} parameters. + * A {@link BugChecker} that flags usages of {@link + * reactor.core.publisher.Mono#zip(reactor.core.publisher.Mono, reactor.core.publisher.Mono)} and + * {@link reactor.core.publisher.Mono#zipWith(reactor.core.publisher.Mono)} with {@link + * reactor.core.publisher.Mono#empty()} parameters. * - *

{@code Mono.zip(Mono, Mono)} and {@code Mono.zipWith(Mono)} perform incorrectly upon retrieval - * of the empty publisher and prematurely terminates the reactive chain from the execution. In most - * cases this is not the desired behaviour. + *

{@link reactor.core.publisher.Mono#zip(reactor.core.publisher.Mono, + * reactor.core.publisher.Mono)} and {@link + * reactor.core.publisher.Mono#zipWith(reactor.core.publisher.Mono)} perform incorrectly upon + * retrieval of the empty publisher and prematurely terminates the reactive chain from the + * execution. In most cases this is not the desired behaviour. * - *

NB: {@code Mono.zipWith(Mono)} is allowed be the Reactor API, but it is an incorrect + *

NB: {@code Mono.zipWith(Mono)} is allowed by the Reactor API, but it is an incorrect * usage of the API. It will be flagged by ErrorProne but the fix won't be supplied. The problem * with the original code should be revisited and fixed in a structural manner by the developer. */ @@ -54,13 +58,15 @@ public final class MonoZipOfMonoVoidUsage extends BugChecker implements MethodInvocationTreeMatcher { private static final long serialVersionUID = 1L; private static final Supplier MONO = type("reactor.core.publisher.Mono"); - // In fact, we use `Mono` everywhere in codebases instead of `Mono` to represent - // empty publisher + + /** + * In fact, we use {@code Mono} everywhere in codebases instead of {@code Mono} to + * represent empty publisher. + */ private static final Supplier MONO_VOID_TYPE = VisitorState.memoize(generic(MONO, type("java.lang.Void"))); - // On instance mono.zipWith, at least one element should match empty in order to proceed. - private static final Matcher MONO_ZIP_AND_WITH = + private static final Matcher ANY_MONO_VOID_IN_PUBLISHERS = anyOf( allOf( instanceMethod().onDescendantOf(MONO).named("zipWith"), @@ -68,46 +74,43 @@ public final class MonoZipOfMonoVoidUsage extends BugChecker allOf( instanceMethod().onDescendantOf(MONO).named("zipWith"), toType(MethodInvocationTree.class, staticMethod().onClass(MONO).named("empty"))), - onClassWithMethodName(MONO_VOID_TYPE, "zipWith")); - - // On class Mono.zip, at least one element should match empty in order to proceed. - private static final Matcher STATIC_MONO_ZIP = - allOf( - staticMethod().onClass(MONO).named("zip"), - toType(MethodInvocationTree.class, hasGenericArgumentOfExactType(MONO_VOID_TYPE))); + onClassWithMethodName(MONO_VOID_TYPE, "zipWith"), + allOf( + staticMethod().onClass(MONO).named("zip"), + toType(MethodInvocationTree.class, hasGenericArgumentOfExactType(MONO_VOID_TYPE)))); /** Instantiates a new {@link MonoZipOfMonoVoidUsage} instance. */ public MonoZipOfMonoVoidUsage() {} @Override public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { - boolean dynamicMono = MONO_ZIP_AND_WITH.matches(tree, state); - boolean staticMono = STATIC_MONO_ZIP.matches(tree, state); - if (!dynamicMono && !staticMono) { + boolean emptyPublisherMatches = ANY_MONO_VOID_IN_PUBLISHERS.matches(tree, state); + if (!emptyPublisherMatches) { return Description.NO_MATCH; } - return buildDescription(tree) - .setMessage( - "`Mono#zip` and `Mono#zipWith` should not be executed against empty publisher; " - + "remove it or suppress this warning and add a comment explaining its purpose") - .addFix(SuggestedFixes.addSuppressWarnings(state, canonicalName())) - .build(); + return describeMatch(tree, SuggestedFixes.addSuppressWarnings(state, canonicalName())); } private static Matcher onClassWithMethodName( Supplier genericDesiredType, String methodName) { - return (tree, state) -> { - JCTree.JCExpression methodSelect = ((JCTree.JCMethodInvocation) tree).getMethodSelect(); - if (!(methodSelect instanceof JCTree.JCFieldAccess)) { - return false; - } - JCTree.JCFieldAccess methodExecuted = (JCTree.JCFieldAccess) methodSelect; - Type invokedType = methodExecuted.selected.type; - String invokedMethodName = methodExecuted.getIdentifier().toString(); - return invokedMethodName.equals(methodName) - && hasSameGenericType(invokedType, genericDesiredType.get(state), state); - }; + return (tree, state) -> + getMethodExecuted(tree) + .filter( + methodExecuted -> { + Type invokedType = methodExecuted.getExpression().type; + String invokedMethodName = methodExecuted.getIdentifier().toString(); + return invokedMethodName.equals(methodName) + && hasSameGenericType(invokedType, genericDesiredType.get(state), state); + }) + .isPresent(); + } + + private static Optional getMethodExecuted(ExpressionTree expressionTree) { + return Optional.of((JCTree.JCMethodInvocation) expressionTree) + .map(JCTree.JCMethodInvocation::getMethodSelect) + .filter(JCTree.JCFieldAccess.class::isInstance) + .map(methodSelect -> (JCTree.JCFieldAccess) methodSelect); } private static Matcher hasGenericArgumentOfExactType( @@ -127,17 +130,18 @@ private static Matcher hasGenericArgumentOfExactType( *

In case of {@code Mono}, we can infer the real type out of the parameters of the invocation * ({@link MethodInvocationTree#getArguments()}): * - *

- either we have explicit variable declared and the provided type which will be inferred, - * - *

- or we have a method invocation, like {@code Mono.just(Object)} or {@code Mono.empty()}, - * for which we can also infer type. + *

    + *
  • either we have explicit variable declared and the provided type which will be inferred, + *
  • or we have a method invocation, like {@code Mono.just(Object)} or {@code Mono.empty()}, + * for which we can also infer type. + *
* - *

Similarly, we can infer the matching type + *

Similarly, we can infer the matching type. */ private static boolean hasSameGenericType( Type genericArgumentType, Type genericDesiredType, VisitorState state) { Type argumentType = Iterables.getFirst(genericArgumentType.allparams(), Type.noType); Type requiredType = Iterables.getOnlyElement(genericDesiredType.allparams()); - return isSameType(argumentType, requiredType, state); + return ASTHelpers.isSameType(argumentType, requiredType, state); } } diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/MonoZipOfMonoVoidUsageTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/MonoZipOfMonoVoidUsageTest.java index b2f31ab978b..d760262f18f 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/MonoZipOfMonoVoidUsageTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/MonoZipOfMonoVoidUsageTest.java @@ -6,7 +6,7 @@ final class MonoZipOfMonoVoidUsageTest { /** - * Line 42 won't be reported as a bug. It's quite hard to catch this case as {@code Mono.empty()} + * Line 44 won't be reported as a bug. It's quite hard to catch this case as {@code Mono.empty()} * yields {@code Mono}, so matcher will be too wide. Additionally, it's not expected to * occur in the real production code. * @@ -29,13 +29,15 @@ void identification() { " Mono c = Mono.just(1);", " Mono d = this.publisher();", " Mono e = Mono.just(t);", - " // BUG: Diagnostic contains: `Mono#zip` and `Mono#zipWith` should not be executed against empty", - " // publisher; remove it or suppress this warning and add a comment explaining its purpose", + " // BUG: Diagnostic contains: `Mono#zip` and `Mono#zipWith` should not be executed against", + " // `Mono#empty` or `Mono` parameter; please revisit the parameters used and make sure to", + " // supply correct publishers instead", " Mono.zip(a, a);", " Mono.zip(e, e);", " e.zipWith(e);", - " // BUG: Diagnostic contains: `Mono#zip` and `Mono#zipWith` should not be executed against empty", - " // publisher; remove it or suppress this warning and add a comment explaining its purpose", + " // BUG: Diagnostic contains: `Mono#zip` and `Mono#zipWith` should not be executed against", + " // `Mono#empty` or `Mono` parameter; please revisit the parameters used and make sure to", + " // supply correct publishers instead", " Mono.zip(d, c, b, a);", " Mono.zip(d, c, b);", " b.zipWith(b).zipWith(c).map(entry -> entry);", @@ -45,16 +47,19 @@ void identification() { " Mono.just(1).zipWith(Mono.just(1));", " Mono.zip(Mono.just(1), Mono.just(1));", " c.zipWith(c);", - " // BUG: Diagnostic contains: `Mono#zip` and `Mono#zipWith` should not be executed against empty", - " // publisher; remove it or suppress this warning and add a comment explaining its purpose", + " // BUG: Diagnostic contains: `Mono#zip` and `Mono#zipWith` should not be executed against", + " // `Mono#empty` or `Mono` parameter; please revisit the parameters used and make sure to", + " // supply correct publishers instead", " c.zipWith(a);", - " // BUG: Diagnostic contains: `Mono#zip` and `Mono#zipWith` should not be executed against empty", - " // publisher; remove it or suppress this warning and add a comment explaining its purpose", + " // BUG: Diagnostic contains: `Mono#zip` and `Mono#zipWith` should not be executed against", + " // `Mono#empty` or `Mono` parameter; please revisit the parameters used and make sure to", + " // supply correct publishers instead", " a.zipWith(c);", " instance.zipWith(a);", " c.zipWith(b, (first, second) -> first + second);", - " // BUG: Diagnostic contains: `Mono#zip` and `Mono#zipWith` should not be executed against empty", - " // publisher; remove it or suppress this warning and add a comment explaining its purpose", + " // BUG: Diagnostic contains: `Mono#zip` and `Mono#zipWith` should not be executed against", + " // `Mono#empty` or `Mono` parameter; please revisit the parameters used and make sure to", + " // supply correct publishers instead", " a.zipWith(c, (first, second) -> second);", " }", "",