From 7cda41b0b4a514e5ebdbd6e2b6a6e4358a81e1db Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Sun, 11 Sep 2022 19:52:53 +0200 Subject: [PATCH] Introduce `MoreTypes` utility class The static methods of this class allow one to construct complex types, against which expression types can subsequently be matched. --- .../bugpatterns/NestedOptionals.java | 24 +-- .../bugpatterns/util/MoreTypes.java | 132 +++++++++++++ .../bugpatterns/util/MoreTypesTest.java | 185 ++++++++++++++++++ 3 files changed, 325 insertions(+), 16 deletions(-) create mode 100644 error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/MoreTypes.java create mode 100644 error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/MoreTypesTest.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 a10ae529e42..953795266a4 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 @@ -3,9 +3,11 @@ import static com.google.errorprone.BugPattern.LinkType.NONE; import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; import static com.google.errorprone.BugPattern.StandardTags.FRAGILE_CODE; +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 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; @@ -15,9 +17,7 @@ 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 com.sun.tools.javac.util.List; import java.util.Optional; /** A {@link BugChecker} which flags nesting of {@link Optional Optionals}. */ @@ -31,21 +31,13 @@ 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)))); @Override public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { - return isOptionalOfOptional(tree, state) ? describeMatch(tree) : Description.NO_MATCH; - } - - private static boolean isOptionalOfOptional(Tree tree, VisitorState state) { - Type optionalType = OPTIONAL.get(state); - Type type = ASTHelpers.getType(tree); - if (!ASTHelpers.isSubtype(type, optionalType, state)) { - return false; - } - - List typeArguments = type.getTypeArguments(); - return !typeArguments.isEmpty() - && ASTHelpers.isSubtype(Iterables.getOnlyElement(typeArguments), optionalType, state); + return state.getTypes().isSubtype(ASTHelpers.getType(tree), OPTIONAL_OF_OPTIONAL.get(state)) + ? describeMatch(tree) + : Description.NO_MATCH; } } diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/MoreTypes.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/MoreTypes.java new file mode 100644 index 00000000000..af13208f95f --- /dev/null +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/MoreTypes.java @@ -0,0 +1,132 @@ +package tech.picnic.errorprone.bugpatterns.util; + +import static java.util.stream.Collectors.toCollection; + +import com.google.errorprone.VisitorState; +import com.google.errorprone.suppliers.Supplier; +import com.google.errorprone.suppliers.Suppliers; +import com.sun.tools.javac.code.BoundKind; +import com.sun.tools.javac.code.Type; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; +import java.util.Objects; +import java.util.Optional; +import java.util.function.BiFunction; + +/** + * A set of helper methods which together define a DSL for defining {@link Type types}. + * + *

These methods are meant to be statically imported. Example usage: + * + * 

{@code
+ * Supplier type =
+ *     VisitorState.memoize(
+ *         generic(
+ *             type("reactor.core.publisher.Flux"),
+ *             subOf(generic(type("org.reactivestreams.Publisher"), unbound()))));
+ * }
+ * + * This statement produces a memoized supplier of the type {@code Flux>}. + */ +public final class MoreTypes { + private MoreTypes() {} + + /** + * Creates a supplier of the type with the given fully qualified name. + * + *

This method should only be used when building more complex types in combination with other + * {@link MoreTypes} methods. In other cases prefer directly calling {@link + * Suppliers#typeFromString(String)}. + * + * @param typeName The type of interest. + * @return A supplier which returns the described type if available in the given state, and {@code + * null} otherwise. + */ + public static Supplier type(String typeName) { + return Suppliers.typeFromString(typeName); + } + + /** + * Creates a supplier of the described generic type. + * + * @param type The base type of interest. + * @param typeArgs The desired type arguments. + * @return A supplier which returns the described type if available in the given state, and {@code + * null} otherwise. + */ + // XXX: The given `type` should be a generic type, so perhaps `withParams` would be a better + // method name. But the DSL wouldn't look as nice that way. + @SafeVarargs + @SuppressWarnings("varargs") + public static Supplier generic(Supplier type, Supplier... typeArgs) { + return propagateNull( + type, + (state, baseType) -> { + List params = + Arrays.stream(typeArgs).map(s -> s.get(state)).collect(toCollection(ArrayList::new)); + if (params.stream().anyMatch(Objects::isNull)) { + return null; + } + + return state.getType(baseType, /* isArray= */ false, params); + }); + } + + /** + * Creates a raw (erased, non-generic) variant of the given type. + * + * @param type The base type of interest. + * @return A supplier which returns the described type if available in the given state, and {@code + * null} otherwise. + */ + public static Supplier raw(Supplier type) { + return propagateNull(type, (state, baseType) -> baseType.tsym.erasure(state.getTypes())); + } + + /** + * Creates a {@code ? super T} wildcard type, with {@code T} bound to the given type. + * + * @param type The base type of interest. + * @return A supplier which returns the described type if available in the given state, and {@code + * null} otherwise. + */ + public static Supplier supOf(Supplier type) { + return propagateNull( + type, + (state, baseType) -> + new Type.WildcardType(baseType, BoundKind.SUPER, state.getSymtab().boundClass)); + } + + /** + * Creates a {@code ? extends T} wildcard type, with {@code T} bound to the given type. + * + * @param type The base type of interest. + * @return A supplier which returns the described type if available in the given state, and {@code + * null} otherwise. + */ + public static Supplier subOf(Supplier type) { + return propagateNull( + type, + (state, baseType) -> + new Type.WildcardType( + type.get(state), BoundKind.EXTENDS, state.getSymtab().boundClass)); + } + + /** + * Creates an unbound wildcard type ({@code ?}). + * + * @return A supplier which returns the described type. + */ + public static Supplier unbound() { + return state -> + new Type.WildcardType( + state.getSymtab().objectType, BoundKind.UNBOUND, state.getSymtab().boundClass); + } + + private static Supplier propagateNull( + Supplier type, BiFunction transformer) { + return state -> + Optional.ofNullable(type.get(state)).map(t -> transformer.apply(state, t)).orElse(null); + } +} diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/MoreTypesTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/MoreTypesTest.java new file mode 100644 index 00000000000..14c4229752f --- /dev/null +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/MoreTypesTest.java @@ -0,0 +1,185 @@ +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.raw; +import static tech.picnic.errorprone.bugpatterns.util.MoreTypes.subOf; +import static tech.picnic.errorprone.bugpatterns.util.MoreTypes.supOf; +import static tech.picnic.errorprone.bugpatterns.util.MoreTypes.type; +import static tech.picnic.errorprone.bugpatterns.util.MoreTypes.unbound; + +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.MethodInvocationTreeMatcher; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.suppliers.Supplier; +import com.google.errorprone.util.ASTHelpers; +import com.google.errorprone.util.Signatures; +import com.sun.source.tree.MethodInvocationTree; +import com.sun.tools.javac.code.Type; +import java.util.ArrayList; +import java.util.List; +import org.junit.jupiter.api.Test; + +final class MoreTypesTest { + private static final ImmutableSet> TYPES = + ImmutableSet.of( + type("java.lang.Nonexistent"), + type("java.lang.String"), + type("java.lang.Number"), + supOf(type("java.lang.Number")), + subOf(type("java.lang.Number")), + type("java.lang.Integer"), + supOf(type("java.lang.Integer")), + subOf(type("java.lang.Integer")), + type("java.util.Optional"), + raw(type("java.util.Optional")), + generic(type("java.util.Optional"), unbound()), + generic(type("java.util.Optional"), type("java.lang.Number")), + type("java.util.Collection"), + raw(type("java.util.Collection")), + generic(type("java.util.Collection"), unbound()), + generic(type("java.util.Collection"), type("java.lang.Number")), + generic(type("java.util.Collection"), supOf(type("java.lang.Number"))), + generic(type("java.util.Collection"), subOf(type("java.lang.Number"))), + generic(type("java.util.Collection"), type("java.lang.Integer")), + generic(type("java.util.Collection"), supOf(type("java.lang.Integer"))), + generic(type("java.util.Collection"), subOf(type("java.lang.Integer"))), + type("java.util.List"), + raw(type("java.util.List")), + generic(type("java.util.List"), unbound()), + generic(type("java.util.List"), type("java.lang.Number")), + generic(type("java.util.List"), supOf(type("java.lang.Number"))), + generic(type("java.util.List"), subOf(type("java.lang.Number"))), + generic(type("java.util.List"), type("java.lang.Integer")), + generic(type("java.util.List"), supOf(type("java.lang.Integer"))), + generic(type("java.util.List"), subOf(type("java.lang.Integer"))), + generic( + type("java.util.Map"), + type("java.lang.String"), + subOf(generic(type("java.util.Collection"), supOf(type("java.lang.Short")))))); + + @Test + void matcher() { + CompilationTestHelper.newInstance(SubtypeFlagger.class, getClass()) + .addSourceLines( + "/A.java", + "import java.util.Collection;", + "import java.util.List;", + "import java.util.Map;", + "import java.util.Optional;", + "import java.util.Set;", + "", + "class A {", + " void m() {", + " Object object = factory();", + " A a = factory();", + "", + " // BUG: Diagnostic contains: [Number, ? super Number, Integer, ? super Integer]", + " int integer = factory();", + "", + " // BUG: Diagnostic contains: [String]", + " String string = factory();", + "", + " // BUG: Diagnostic contains: [Optional]", + " Optional rawOptional = factory();", + " // BUG: Diagnostic contains: [Optional, Optional]", + " Optional optionalOfS = factory();", + " // BUG: Diagnostic contains: [Optional, Optional]", + " Optional optionalOfT = factory();", + " // BUG: Diagnostic contains: [Optional, Optional, Optional]", + " Optional optionalOfNumber = factory();", + " // BUG: Diagnostic contains: [Optional, Optional]", + " Optional optionalOfInteger = factory();", + "", + " // BUG: Diagnostic contains: [Collection]", + " Collection rawCollection = factory();", + " // BUG: Diagnostic contains: [Collection, Collection, Collection, Collection, Collection, Collection]", + " Collection collectionOfNumber = factory();", + " // BUG: Diagnostic contains: [Collection, Collection, Collection,", + " // Collection, Collection, Collection]", + " Collection collectionOfInteger = factory();", + " // BUG: Diagnostic contains: [Collection, Collection, Collection]", + " Collection collectionOfShort = factory();", + "", + " // BUG: Diagnostic contains: [Collection, List]", + " List rawList = factory();", + " // BUG: Diagnostic contains: [Collection, Collection, Collection, Collection, Collection, Collection, List, List,", + " // List, List, List, List]", + " List listOfNumber = factory();", + " // BUG: Diagnostic contains: [Collection, Collection, Collection,", + " // Collection, Collection, Collection, List,", + " // List, List, List, List, List]", + " List listOfInteger = factory();", + " // BUG: Diagnostic contains: [Collection, Collection, Collection, List,", + " // List, List]", + " List listOfShort = factory();", + "", + " // BUG: Diagnostic contains: [Collection]", + " Set rawSet = factory();", + " // BUG: Diagnostic contains: [Collection, Collection, Collection, Collection, Collection, Collection]", + " Set setOfNumber = factory();", + " // BUG: Diagnostic contains: [Collection, Collection, Collection,", + " // Collection, Collection, Collection]", + " Set setOfInteger = factory();", + " // BUG: Diagnostic contains: [Collection, Collection, Collection]", + " Set setOfShort = factory();", + "", + " Map rawMap = factory();", + " Map> mapFromNumberToCollectionOfNumber = factory();", + " Map> mapFromNumberToCollectionOfShort = factory();", + " Map> mapFromNumberToCollectionOfInteger = factory();", + " // BUG: Diagnostic contains: [Map>]", + " Map> mapFromStringToCollectionOfNumber = factory();", + " // BUG: Diagnostic contains: [Map>]", + " Map> mapFromStringToCollectionOfShort = factory();", + " Map> mapFromStringToCollectionOfInteger = factory();", + " // BUG: Diagnostic contains: [Map>]", + " Map> mapFromStringToListOfNumber = factory();", + " // BUG: Diagnostic contains: [Map>]", + " Map> mapFromStringToListOfShort = factory();", + " Map> mapFromStringToListOfInteger = factory();", + " }", + "", + " private T factory() {", + " return null;", + " }", + "}") + .doTest(); + } + + /** + * A {@link BugChecker} which flags method invocations that are a subtype of any type contained in + * {@link #TYPES}. + */ + @BugPattern(summary = "Flags invocations of methods with select return types", severity = ERROR) + public static final class SubtypeFlagger extends BugChecker + implements MethodInvocationTreeMatcher { + private static final long serialVersionUID = 1L; + + @Override + public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { + Type treeType = ASTHelpers.getType(tree); + + List matches = new ArrayList<>(); + + for (Supplier type : TYPES) { + Type testType = type.get(state); + if (testType != null && state.getTypes().isSubtype(treeType, testType)) { + matches.add(Signatures.prettyType(testType)); + } + } + + return matches.isEmpty() + ? Description.NO_MATCH + : buildDescription(tree).setMessage(matches.toString()).build(); + } + } +}