-
Notifications
You must be signed in to change notification settings - Fork 39
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Introduce
NestedPublishers check (#642)
And introduce a few utility methods that simplify `Tree`- and `Type`-based matching, allowing the existing (and similar) `NestedOptionals` check to be simplified as well.
- Loading branch information
1 parent
fc5f3bd
commit 4af7b21
Showing
8 changed files
with
275 additions
and
38 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
63 changes: 63 additions & 0 deletions
63
error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/NestedPublishers.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,63 @@ | ||
| package tech.picnic.errorprone.bugpatterns; | ||
|
|
||
| import static com.google.errorprone.BugPattern.LinkType.CUSTOM; | ||
| import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; | ||
| import static com.google.errorprone.BugPattern.StandardTags.FRAGILE_CODE; | ||
| import static com.google.errorprone.matchers.Matchers.typePredicateMatcher; | ||
| import static com.google.errorprone.predicates.TypePredicates.allOf; | ||
| import static com.google.errorprone.predicates.TypePredicates.not; | ||
| import static tech.picnic.errorprone.bugpatterns.util.Documentation.BUG_PATTERNS_BASE_URL; | ||
| import static tech.picnic.errorprone.bugpatterns.util.MoreTypePredicates.hasTypeParameter; | ||
| import static tech.picnic.errorprone.bugpatterns.util.MoreTypePredicates.isSubTypeOf; | ||
| import static tech.picnic.errorprone.bugpatterns.util.MoreTypes.generic; | ||
| import static tech.picnic.errorprone.bugpatterns.util.MoreTypes.raw; | ||
| import static tech.picnic.errorprone.bugpatterns.util.MoreTypes.subOf; | ||
| import static tech.picnic.errorprone.bugpatterns.util.MoreTypes.type; | ||
|
|
||
| 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.matchers.Description; | ||
| import com.google.errorprone.matchers.Matcher; | ||
| import com.google.errorprone.suppliers.Supplier; | ||
| import com.sun.source.tree.ExpressionTree; | ||
| import com.sun.source.tree.MethodInvocationTree; | ||
| import com.sun.tools.javac.code.Type; | ||
|
|
||
| /** | ||
| * A {@link BugChecker} that flags {@code Publisher<? extends Publisher>} instances, unless the | ||
| * nested {@link org.reactivestreams.Publisher} is a {@link reactor.core.publisher.GroupedFlux}. | ||
| */ | ||
| // XXX: See the `NestedOptionals` check for some ideas on how to generalize this kind of checker. | ||
| @AutoService(BugChecker.class) | ||
| @BugPattern( | ||
| summary = | ||
| "Avoid `Publisher`s that emit other `Publishers`s; " | ||
| + "the resultant code is hard to reason about", | ||
| link = BUG_PATTERNS_BASE_URL + "NestedPublishers", | ||
| linkType = CUSTOM, | ||
| severity = WARNING, | ||
| tags = FRAGILE_CODE) | ||
| public final class NestedPublishers extends BugChecker implements MethodInvocationTreeMatcher { | ||
| private static final long serialVersionUID = 1L; | ||
| private static final Supplier<Type> PUBLISHER = type("org.reactivestreams.Publisher"); | ||
| private static final Matcher<ExpressionTree> IS_NON_GROUPED_PUBLISHER_OF_PUBLISHERS = | ||
| typePredicateMatcher( | ||
| allOf( | ||
| isSubTypeOf(generic(PUBLISHER, subOf(raw(PUBLISHER)))), | ||
| not( | ||
| hasTypeParameter( | ||
| 0, isSubTypeOf(raw(type("reactor.core.publisher.GroupedFlux"))))))); | ||
|
|
||
| /** Instantiates a new {@link NestedPublishers} instance. */ | ||
| public NestedPublishers() {} | ||
|
|
||
| @Override | ||
| public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { | ||
| return IS_NON_GROUPED_PUBLISHER_OF_PUBLISHERS.matches(tree, state) | ||
| ? describeMatch(tree) | ||
| : Description.NO_MATCH; | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
63 changes: 63 additions & 0 deletions
63
...one-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/MoreTypePredicates.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,63 @@ | ||
| package tech.picnic.errorprone.bugpatterns.util; | ||
|
|
||
| import com.google.errorprone.VisitorState; | ||
| import com.google.errorprone.predicates.TypePredicate; | ||
| import com.google.errorprone.predicates.TypePredicates; | ||
| import com.google.errorprone.suppliers.Supplier; | ||
| import com.google.errorprone.util.ASTHelpers; | ||
| import com.sun.tools.javac.code.Type; | ||
| import com.sun.tools.javac.util.List; | ||
|
|
||
| /** | ||
| * A collection of general-purpose {@link TypePredicate}s. | ||
| * | ||
| * <p>These methods are additions to the ones found in {@link TypePredicates}. | ||
| */ | ||
| // XXX: The methods in this class are tested only indirectly. Consider adding a dedicated test | ||
| // class, or make sure that each method is explicitly covered by a tested analog in `MoreMatchers`. | ||
| public final class MoreTypePredicates { | ||
| private MoreTypePredicates() {} | ||
|
|
||
| /** | ||
| * Returns a {@link TypePredicate} that matches types that are annotated with the indicated | ||
| * annotation type. | ||
| * | ||
| * @param annotationType The fully-qualified name of the annotation type. | ||
| * @return A {@link TypePredicate} that matches appropriate types. | ||
| */ | ||
| public static TypePredicate hasAnnotation(String annotationType) { | ||
| return (type, state) -> ASTHelpers.hasAnnotation(type.tsym, annotationType, state); | ||
| } | ||
|
|
||
| /** | ||
| * Returns a {@link TypePredicate} that matches subtypes of the type returned by the specified | ||
| * {@link Supplier}. | ||
| * | ||
| * @implNote This method does not use {@link ASTHelpers#isSubtype(Type, Type, VisitorState)}, as | ||
| * that method performs type erasure. | ||
| * @param bound The {@link Supplier} that returns the type to match against. | ||
| * @return A {@link TypePredicate} that matches appropriate subtypes. | ||
| */ | ||
| public static TypePredicate isSubTypeOf(Supplier<Type> bound) { | ||
| Supplier<Type> memoizedType = VisitorState.memoize(bound); | ||
| return (type, state) -> { | ||
| Type boundType = memoizedType.get(state); | ||
| return boundType != null && state.getTypes().isSubtype(type, boundType); | ||
| }; | ||
| } | ||
|
|
||
| /** | ||
| * Returns a {@link TypePredicate} that matches generic types with a type parameter that matches | ||
| * the specified {@link TypePredicate} at the indicated index. | ||
| * | ||
| * @param index The index of the type parameter to match against. | ||
| * @param predicate The {@link TypePredicate} to match against the type parameter. | ||
| * @return A {@link TypePredicate} that matches appropriate generic types. | ||
| */ | ||
| public static TypePredicate hasTypeParameter(int index, TypePredicate predicate) { | ||
| return (type, state) -> { | ||
| List<Type> typeArguments = type.getTypeArguments(); | ||
| return typeArguments.size() > index && predicate.apply(typeArguments.get(index), state); | ||
| }; | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
37 changes: 37 additions & 0 deletions
37
...-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/NestedPublishersTest.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| package tech.picnic.errorprone.bugpatterns; | ||
|
|
||
| import com.google.errorprone.CompilationTestHelper; | ||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| final class NestedPublishersTest { | ||
| @Test | ||
| void identification() { | ||
| CompilationTestHelper.newInstance(NestedPublishers.class, getClass()) | ||
| .addSourceLines( | ||
| "A.java", | ||
| "import org.reactivestreams.Publisher;", | ||
| "import reactor.core.publisher.Flux;", | ||
| "import reactor.core.publisher.GroupedFlux;", | ||
| "import reactor.core.publisher.Mono;", | ||
| "", | ||
| "class A {", | ||
| " void m() {", | ||
| " Mono.empty();", | ||
| " Flux.just(1);", | ||
| " Flux.just(1, 2).groupBy(i -> i).map(groupedFlux -> (GroupedFlux) groupedFlux);", | ||
| "", | ||
| " // BUG: Diagnostic contains:", | ||
| " Mono.just(Mono.empty());", | ||
| " // BUG: Diagnostic contains:", | ||
| " Flux.just(Flux.empty());", | ||
| " // BUG: Diagnostic contains:", | ||
| " Mono.just((Flux) Flux.just(1));", | ||
| " // BUG: Diagnostic contains:", | ||
| " Flux.just((Publisher) Mono.just(1));", | ||
| " // BUG: Diagnostic contains:", | ||
| " Mono.just(1).map(Mono::just);", | ||
| " }", | ||
| "}") | ||
| .doTest(); | ||
| } | ||
| } |
Oops, something went wrong.