diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/NestedOptionals.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/NestedOptionals.java index 8fe464fd09..6bee75348a 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/NestedOptionals.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/NestedOptionals.java @@ -4,6 +4,7 @@ import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; import static com.google.errorprone.BugPattern.StandardTags.FRAGILE_CODE; import static tech.picnic.errorprone.bugpatterns.util.Documentation.BUG_PATTERNS_BASE_URL; +import static tech.picnic.errorprone.bugpatterns.util.MoreMatchers.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; @@ -14,14 +15,19 @@ 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.google.errorprone.suppliers.Suppliers; -import com.google.errorprone.util.ASTHelpers; import com.sun.source.tree.MethodInvocationTree; +import com.sun.source.tree.Tree; import com.sun.tools.javac.code.Type; import java.util.Optional; /** A {@link BugChecker} that flags nesting of {@link Optional Optionals}. */ +// XXX: Extend this checker to also flag method return types and variable/field types. +// XXX: Consider generalizing this checker and `NestedPublishers` to a single `NestedMonad` check, +// which e.g. also flags nested `Stream`s. Alternatively, combine these and other checkers into an +// even more generic `ConfusingType` checker. @AutoService(BugChecker.class) @BugPattern( summary = @@ -33,19 +39,16 @@ public final class NestedOptionals extends BugChecker implements MethodInvocationTreeMatcher { private static final long serialVersionUID = 1L; private static final Supplier OPTIONAL = Suppliers.typeFromClass(Optional.class); - private static final Supplier OPTIONAL_OF_OPTIONAL = - VisitorState.memoize(generic(OPTIONAL, subOf(raw(OPTIONAL)))); + private static final Matcher IS_OPTIONAL_OF_OPTIONAL = + isSubTypeOf(VisitorState.memoize(generic(OPTIONAL, subOf(raw(OPTIONAL))))); /** Instantiates a new {@link NestedOptionals} instance. */ public NestedOptionals() {} @Override public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { - Type type = OPTIONAL_OF_OPTIONAL.get(state); - if (type == null || !state.getTypes().isSubtype(ASTHelpers.getType(tree), type)) { - return Description.NO_MATCH; - } - - return describeMatch(tree); + return IS_OPTIONAL_OF_OPTIONAL.matches(tree, state) + ? describeMatch(tree) + : Description.NO_MATCH; } } diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/NestedPublishers.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/NestedPublishers.java new file mode 100644 index 0000000000..d4f00b99b1 --- /dev/null +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/NestedPublishers.java @@ -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} 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 PUBLISHER = type("org.reactivestreams.Publisher"); + private static final Matcher 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; + } +} diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/MoreMatchers.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/MoreMatchers.java index b62e761db0..eaa683c650 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/MoreMatchers.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/MoreMatchers.java @@ -1,11 +1,14 @@ package tech.picnic.errorprone.bugpatterns.util; +import static com.google.errorprone.matchers.Matchers.typePredicateMatcher; +import static tech.picnic.errorprone.bugpatterns.util.MoreTypePredicates.hasAnnotation; + import com.google.errorprone.matchers.Matcher; import com.google.errorprone.matchers.Matchers; -import com.google.errorprone.predicates.TypePredicate; -import com.google.errorprone.util.ASTHelpers; +import com.google.errorprone.suppliers.Supplier; import com.sun.source.tree.AnnotationTree; -import com.sun.tools.javac.code.Symbol; +import com.sun.source.tree.Tree; +import com.sun.tools.javac.code.Type; /** * A collection of general-purpose {@link Matcher}s. @@ -25,15 +28,21 @@ private MoreMatchers() {} * @return A {@link Matcher} that matches trees with the specified meta-annotation. */ public static Matcher hasMetaAnnotation(String annotationType) { - TypePredicate typePredicate = hasAnnotation(annotationType); - return (tree, state) -> { - Symbol sym = ASTHelpers.getSymbol(tree); - return sym != null && typePredicate.apply(sym.type, state); - }; + return typePredicateMatcher(hasAnnotation(annotationType)); } - // XXX: Consider moving to a `MoreTypePredicates` utility class. - private static TypePredicate hasAnnotation(String annotationClassName) { - return (type, state) -> ASTHelpers.hasAnnotation(type.tsym, annotationClassName, state); + /** + * Returns a {@link Matcher} that determines whether the type of a given {@link Tree} is a subtype + * of the type returned by the specified {@link Supplier}. + * + *

This method differs from {@link Matchers#isSubtypeOf(Supplier)} in that it does not perform + * type erasure. + * + * @param The type of tree to match against. + * @param type The {@link Supplier} that returns the type to match against. + * @return A {@link Matcher} that matches trees with the specified type. + */ + public static Matcher isSubTypeOf(Supplier type) { + return typePredicateMatcher(MoreTypePredicates.isSubTypeOf(type)); } } diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/MoreTypePredicates.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/MoreTypePredicates.java new file mode 100644 index 0000000000..3ed401d6d6 --- /dev/null +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/MoreTypePredicates.java @@ -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. + * + *

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 bound) { + Supplier 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 typeArguments = type.getTypeArguments(); + return typeArguments.size() > index && predicate.apply(typeArguments.get(index), state); + }; + } +} 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 e41d180890..5653673bca 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 @@ -448,6 +448,7 @@ Flux after(Flux flux) { /** Prefer {@link Flux#concatMap(Function)} over more contrived alternatives. */ static final class FluxConcatMap { @BeforeTemplate + @SuppressWarnings("NestedPublishers") Flux before(Flux flux, Function> function) { return Refaster.anyOf( flux.flatMap(function, 1), @@ -464,6 +465,7 @@ Flux after(Flux flux, Function /** Prefer {@link Flux#concatMap(Function, int)} over more contrived alternatives. */ static final class FluxConcatMapWithPrefetch { @BeforeTemplate + @SuppressWarnings("NestedPublishers") Flux before( Flux flux, Function> function, @@ -795,6 +797,7 @@ Flux after(Flux flux) { /** Prefer {@link Mono#flatMap(Function)} over more contrived alternatives. */ static final class MonoFlatMap { @BeforeTemplate + @SuppressWarnings("NestedPublishers") Mono before(Mono mono, Function> function) { return mono.map(function).flatMap(identity()); } @@ -808,6 +811,7 @@ Mono after(Mono mono, Function> fun /** Prefer {@link Mono#flatMapMany(Function)} over more contrived alternatives. */ static final class MonoFlatMapMany { @BeforeTemplate + @SuppressWarnings("NestedPublishers") Flux before(Mono mono, Function> function) { return mono.map(function).flatMapMany(identity()); } diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/NestedOptionalsTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/NestedOptionalsTest.java index 62c5fb35fb..8923c11b76 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/NestedOptionalsTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/NestedOptionalsTest.java @@ -36,19 +36,4 @@ void identification() { "}") .doTest(); } - - @Test - void identificationOptionalTypeNotLoaded() { - CompilationTestHelper.newInstance(NestedOptionals.class, getClass()) - .addSourceLines( - "A.java", - "import java.time.Duration;", - "", - "class A {", - " void m() {", - " Duration.ofSeconds(1);", - " }", - "}") - .doTest(); - } } diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/NestedPublishersTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/NestedPublishersTest.java new file mode 100644 index 0000000000..846405becd --- /dev/null +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/NestedPublishersTest.java @@ -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(); + } +} diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/MoreMatchersTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/MoreMatchersTest.java index 842fb57b6b..faed5900f3 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/MoreMatchersTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/MoreMatchersTest.java @@ -1,21 +1,29 @@ package tech.picnic.errorprone.bugpatterns.util; import static com.google.errorprone.BugPattern.SeverityLevel.ERROR; +import static tech.picnic.errorprone.bugpatterns.util.MoreTypes.generic; +import static tech.picnic.errorprone.bugpatterns.util.MoreTypes.subOf; +import static tech.picnic.errorprone.bugpatterns.util.MoreTypes.type; +import com.google.common.collect.ImmutableSet; import com.google.errorprone.BugPattern; import com.google.errorprone.CompilationTestHelper; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker; import com.google.errorprone.bugpatterns.BugChecker.AnnotationTreeMatcher; +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.AnnotationTree; +import com.sun.source.tree.MethodInvocationTree; +import com.sun.source.tree.Tree; import org.junit.jupiter.api.Test; final class MoreMatchersTest { @Test void hasMetaAnnotation() { - CompilationTestHelper.newInstance(TestMatcher.class, getClass()) + CompilationTestHelper.newInstance(HasMetaAnnotationTestChecker.class, getClass()) .addSourceLines( "A.java", "import org.junit.jupiter.api.AfterAll;", @@ -47,9 +55,59 @@ void hasMetaAnnotation() { .doTest(); } - /** A {@link BugChecker} that delegates to {@link MoreMatchers#hasMetaAnnotation(String)} . */ + @Test + void isSubTypeOf() { + CompilationTestHelper.newInstance(IsSubTypeOfTestChecker.class, getClass()) + .addSourceLines( + "A.java", + "import com.google.common.collect.ImmutableList;", + "import com.google.common.collect.ImmutableSet;", + "import com.google.common.collect.ImmutableSortedSet;", + "", + "class A {", + " void m() {", + " ImmutableSet.of(\"foo\");", + " ImmutableSortedSet.of(\"foo\");", + " ImmutableList.of(\"foo\");", + " ImmutableList.of(1);", + " ImmutableList.of(1.0);", + " ImmutableList.of((Number) 1);", + "", + " // BUG: Diagnostic contains:", + " ImmutableSet.of(1);", + " // BUG: Diagnostic contains:", + " ImmutableSet.of(1.0);", + " // BUG: Diagnostic contains:", + " ImmutableSet.of((Number) 1);", + " // BUG: Diagnostic contains:", + " ImmutableSortedSet.of(1);", + " // BUG: Diagnostic contains:", + " ImmutableSortedSet.of(1.0);", + " // BUG: Diagnostic contains:", + " ImmutableSortedSet.of((Number) 1);", + " }", + "}") + .doTest(); + } + + @Test + void isSubTypeOfBoundTypeUnknown() { + CompilationTestHelper.newInstance(IsSubTypeOfTestChecker.class, getClass()) + .withClasspath() + .addSourceLines( + "A.java", + "class A {", + " void m() {", + " System.out.println(toString());", + " }", + "}") + .doTest(); + } + + /** A {@link BugChecker} that delegates to {@link MoreMatchers#hasMetaAnnotation(String)}. */ @BugPattern(summary = "Interacts with `MoreMatchers` for testing purposes", severity = ERROR) - public static final class TestMatcher extends BugChecker implements AnnotationTreeMatcher { + public static final class HasMetaAnnotationTestChecker extends BugChecker + implements AnnotationTreeMatcher { private static final long serialVersionUID = 1L; private static final Matcher DELEGATE = MoreMatchers.hasMetaAnnotation("org.junit.jupiter.api.TestTemplate"); @@ -59,4 +117,19 @@ public Description matchAnnotation(AnnotationTree tree, VisitorState state) { return DELEGATE.matches(tree, state) ? describeMatch(tree) : Description.NO_MATCH; } } + + /** A {@link BugChecker} that delegates to {@link MoreMatchers#isSubTypeOf(Supplier)}. */ + @BugPattern(summary = "Interacts with `MoreMatchers` for testing purposes", severity = ERROR) + public static final class IsSubTypeOfTestChecker extends BugChecker + implements MethodInvocationTreeMatcher { + private static final long serialVersionUID = 1L; + private static final Matcher DELEGATE = + MoreMatchers.isSubTypeOf( + generic(type(ImmutableSet.class.getName()), subOf(type(Number.class.getName())))); + + @Override + public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { + return DELEGATE.matches(tree, state) ? describeMatch(tree) : Description.NO_MATCH; + } + } }