From b4ce298e3d5d1438b4d7eb4e9bc41105d86d3b08 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Fri, 11 Aug 2023 22:23:36 +0200 Subject: [PATCH] Suggestions --- .../bugpatterns/NestedOptionals.java | 21 ++--- .../bugpatterns/NestedPublishers.java | 53 ++++++------- .../bugpatterns/util/MoreMatchers.java | 31 +++++--- .../bugpatterns/util/MoreTypePredicates.java | 63 +++++++++++++++ .../bugpatterns/NestedOptionalsTest.java | 15 ---- .../bugpatterns/NestedPublishersTest.java | 31 ++------ .../bugpatterns/util/MoreMatchersTest.java | 79 ++++++++++++++++++- 7 files changed, 203 insertions(+), 90 deletions(-) create mode 100644 error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/MoreTypePredicates.java 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 8fe464fd099..6bee75348a7 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 index 871f0e232b9..d4f00b99b13 100644 --- 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 @@ -3,32 +3,38 @@ 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 static tech.picnic.errorprone.bugpatterns.util.MoreTypes.unbound; import com.google.auto.service.AutoService; -import com.google.common.collect.Iterables; 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.google.errorprone.suppliers.Suppliers; -import com.google.errorprone.util.ASTHelpers; +import com.sun.source.tree.ExpressionTree; import com.sun.source.tree.MethodInvocationTree; import com.sun.tools.javac.code.Type; -import org.jspecify.annotations.Nullable; -import org.reactivestreams.Publisher; -/** A {@link BugChecker} that flags nesting of {@link Publisher Publishers}. */ +/** + * 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 nesting `Publisher`s inside `Publishers`s; " + "Avoid `Publisher`s that emit other `Publishers`s; " + "the resultant code is hard to reason about", link = BUG_PATTERNS_BASE_URL + "NestedPublishers", linkType = CUSTOM, @@ -37,32 +43,21 @@ 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 Supplier PUBLISHER_OF_PUBLISHERS = - VisitorState.memoize(generic(PUBLISHER, subOf(generic(PUBLISHER, unbound())))); - private static final Supplier GROUPED_FLUX = - VisitorState.memoize( - generic( - Suppliers.typeFromString("reactor.core.publisher.GroupedFlux"), - unbound(), - unbound())); + 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) { - Type type = ASTHelpers.getType(tree); - - if (!isSubType(type, PUBLISHER_OF_PUBLISHERS.get(state), state) - || isSubType( - Iterables.getOnlyElement(type.getTypeArguments()), GROUPED_FLUX.get(state), state)) { - return Description.NO_MATCH; - } - - return describeMatch(tree); - } - - private static boolean isSubType(Type subType, @Nullable Type type, VisitorState state) { - return type != null && state.getTypes().isSubtype(subType, type); + 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 b62e761db00..83963f63681 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 return 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 00000000000..3ed401d6d6a --- /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/test/java/tech/picnic/errorprone/bugpatterns/NestedOptionalsTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/NestedOptionalsTest.java index 62c5fb35fbc..8923c11b761 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 index f1ac337d1bc..846405becde 100644 --- 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 @@ -9,42 +9,27 @@ 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();", - " Mono.just(1);", + " Flux.just(1);", + " Flux.just(1, 2).groupBy(i -> i).map(groupedFlux -> (GroupedFlux) groupedFlux);", + "", " // BUG: Diagnostic contains:", " Mono.just(Mono.empty());", " // BUG: Diagnostic contains:", - " Mono.just(Mono.just(1));", - " // BUG: Diagnostic contains:", - " Mono.just(Flux.empty());", - " // BUG: Diagnostic contains:", - " Mono.just(Flux.just(1));", - " // BUG: Diagnostic contains:", - " Mono.just(1).map(Mono::just);", - " // BUG: Diagnostic contains:", - " Mono.just(1).map(Flux::just);", - "", - " Flux.empty();", - " Flux.just(1);", - " // BUG: Diagnostic contains:", " Flux.just(Flux.empty());", " // BUG: Diagnostic contains:", - " Flux.just(Flux.just(1));", - " // BUG: Diagnostic contains:", - " Flux.just(Mono.empty());", + " Mono.just((Flux) Flux.just(1));", " // BUG: Diagnostic contains:", - " Flux.just(Mono.just(1));", + " Flux.just((Publisher) Mono.just(1));", " // BUG: Diagnostic contains:", - " Flux.just(1).map(Flux::just);", - " // BUG: Diagnostic contains:", - " Flux.just(1).map(Mono::just);", - "", - " Flux.just(1, 2).groupBy(i -> i);", + " 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 842fb57b6b0..faed5900f37 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; + } + } }