Skip to content

Commit

Permalink
PR comments - Mohamed & Rick
Browse files Browse the repository at this point in the history
  • Loading branch information
@DimaLegeza
DimaLegeza committed Dec 12, 2023
1 parent 3de47ce commit 56e9e39
Show file tree
Hide file tree
Showing 2 changed files with 64 additions and 55 deletions.
92 changes: 48 additions & 44 deletions ...rone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MonoZipOfMonoVoidUsage.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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.
*
* <p>{@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.
* <p>{@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.
*
* <p>NB: {@code Mono<?>.zipWith(Mono<Void>)} is allowed be the Reactor API, but it is an incorrect
* <p>NB: {@code Mono<?>.zipWith(Mono<Void>)} 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.
*/
Expand All @@ -54,60 +58,59 @@ public final class MonoZipOfMonoVoidUsage extends BugChecker
implements MethodInvocationTreeMatcher {
private static final long serialVersionUID = 1L;
private static final Supplier<Type> MONO = type("reactor.core.publisher.Mono");
// In fact, we use `Mono<Void>` everywhere in codebases instead of `Mono<Object>` to represent
// empty publisher

/**
* In fact, we use {@code Mono<Void>} everywhere in codebases instead of {@code Mono<Object>} to
* represent empty publisher.
*/
private static final Supplier<Type> 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<ExpressionTree> MONO_ZIP_AND_WITH =
private static final Matcher<ExpressionTree> ANY_MONO_VOID_IN_PUBLISHERS =
anyOf(
allOf(
instanceMethod().onDescendantOf(MONO).named("zipWith"),
toType(MethodInvocationTree.class, hasGenericArgumentOfExactType(MONO_VOID_TYPE))),
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<ExpressionTree> 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<ExpressionTree> onClassWithMethodName(
Supplier<Type> 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<JCTree.JCFieldAccess> 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<MethodInvocationTree> hasGenericArgumentOfExactType(
Expand All @@ -127,17 +130,18 @@ private static Matcher<MethodInvocationTree> hasGenericArgumentOfExactType(
* <p>In case of {@code Mono}, we can infer the real type out of the parameters of the invocation
* ({@link MethodInvocationTree#getArguments()}):
*
* <p>- either we have explicit variable declared and the provided type which will be inferred,
*
* <p>- or we have a method invocation, like {@code Mono.just(Object)} or {@code Mono.empty()},
* for which we can also infer type.
* <ul>
* <li>either we have explicit variable declared and the provided type which will be inferred,
* <li>or we have a method invocation, like {@code Mono.just(Object)} or {@code Mono.empty()},
* for which we can also infer type.
* </ul>
*
* <p>Similarly, we can infer the matching type
* <p>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);
}
}
27 changes: 16 additions & 11 deletions ...-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/MonoZipOfMonoVoidUsageTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<Object>}, so matcher will be too wide. Additionally, it's not expected to
* occur in the real production code.
*
Expand All @@ -29,13 +29,15 @@ void identification() {
" Mono<Integer> c = Mono.just(1);",
" Mono<Integer> d = this.publisher();",
" Mono<T> 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<Void>` 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<Void>` 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);",
Expand All @@ -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<Void>` 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<Void>` 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<Void>` parameter; please revisit the parameters used and make sure to",
" // supply correct publishers instead",
" a.zipWith(c, (first, second) -> second);",
" }",
"",
Expand Down

0 comments on commit 56e9e39

Please sign in to comment.