From d569156a6b3c28c958b2d435b7be57052a4449f1 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Wed, 20 Dec 2023 22:14:22 +0100 Subject: [PATCH] Introduce `ErrorProneRuntimeClasspath` check (#882) Prefer "type-safe" type references were possible, but use string literals if the references type may not be available at runtime. --- .../BugPatternTestExtractor.java | 2 +- .../bugpatterns/CollectorMutability.java | 24 ++- .../errorprone/bugpatterns/EmptyMethod.java | 4 +- .../ErrorProneRuntimeClasspath.java | 165 ++++++++++++++++++ .../bugpatterns/FluxImplicitBlock.java | 7 +- .../FormatStringConcatenation.java | 11 +- .../bugpatterns/IdentityConversion.java | 38 ++-- .../bugpatterns/JUnitValueSource.java | 6 +- .../bugpatterns/NonStaticImport.java | 27 +-- .../RedundantStringConversion.java | 6 +- .../bugpatterns/RequestMappingAnnotation.java | 12 +- .../errorprone/bugpatterns/StaticImport.java | 99 +++++++---- .../bugpatterns/util/ThirdPartyLibrary.java | 12 +- .../ErrorProneRuntimeClasspathTest.java | 136 +++++++++++++++ .../bugpatterns/util/MoreTypesTest.java | 85 +++++---- 15 files changed, 516 insertions(+), 118 deletions(-) create mode 100644 error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ErrorProneRuntimeClasspath.java create mode 100644 error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ErrorProneRuntimeClasspathTest.java diff --git a/documentation-support/src/main/java/tech/picnic/errorprone/documentation/BugPatternTestExtractor.java b/documentation-support/src/main/java/tech/picnic/errorprone/documentation/BugPatternTestExtractor.java index cf7f7a0b0e..e24a23c865 100644 --- a/documentation-support/src/main/java/tech/picnic/errorprone/documentation/BugPatternTestExtractor.java +++ b/documentation-support/src/main/java/tech/picnic/errorprone/documentation/BugPatternTestExtractor.java @@ -67,7 +67,7 @@ private static final class BugPatternTestCollector "com.google.errorprone.CompilationTestHelper", "com.google.errorprone.BugCheckerRefactoringTestHelper") .named("newInstance") - .withParameters("java.lang.Class", "java.lang.Class"); + .withParameters(Class.class.getCanonicalName(), Class.class.getCanonicalName()); private static final Matcher IDENTIFICATION_SOURCE_LINES = instanceMethod() .onDescendantOf("com.google.errorprone.CompilationTestHelper") diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/CollectorMutability.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/CollectorMutability.java index e3b2c76776..08376ab358 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/CollectorMutability.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/CollectorMutability.java @@ -7,6 +7,9 @@ import static tech.picnic.errorprone.bugpatterns.util.Documentation.BUG_PATTERNS_BASE_URL; import com.google.auto.service.AutoService; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; import com.google.errorprone.BugPattern; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker; @@ -17,7 +20,11 @@ import com.google.errorprone.matchers.Matcher; import com.sun.source.tree.ExpressionTree; import com.sun.source.tree.MethodInvocationTree; +import java.util.ArrayList; +import java.util.HashMap; +import java.util.HashSet; import java.util.stream.Collector; +import java.util.stream.Collectors; import tech.picnic.errorprone.bugpatterns.util.ThirdPartyLibrary; /** @@ -38,7 +45,7 @@ public final class CollectorMutability extends BugChecker implements MethodInvocationTreeMatcher { private static final long serialVersionUID = 1L; private static final Matcher COLLECTOR_METHOD = - staticMethod().onClass("java.util.stream.Collectors"); + staticMethod().onClass(Collectors.class.getCanonicalName()); private static final Matcher LIST_COLLECTOR = staticMethod().anyClass().named("toList"); private static final Matcher MAP_COLLECTOR = @@ -59,8 +66,8 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState if (LIST_COLLECTOR.matches(tree, state)) { return suggestToCollectionAlternatives( tree, - "com.google.common.collect.ImmutableList.toImmutableList", - "java.util.ArrayList", + ImmutableList.class.getCanonicalName() + ".toImmutableList", + ArrayList.class.getCanonicalName(), state); } @@ -71,8 +78,8 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState if (SET_COLLECTOR.matches(tree, state)) { return suggestToCollectionAlternatives( tree, - "com.google.common.collect.ImmutableSet.toImmutableSet", - "java.util.HashSet", + ImmutableSet.class.getCanonicalName() + ".toImmutableSet", + HashSet.class.getCanonicalName(), state); } @@ -87,7 +94,7 @@ private Description suggestToCollectionAlternatives( SuggestedFix.Builder mutableFix = SuggestedFix.builder(); String toCollectionSelect = SuggestedFixes.qualifyStaticImport( - "java.util.stream.Collectors.toCollection", mutableFix, state); + Collectors.class.getCanonicalName() + ".toCollection", mutableFix, state); String mutableCollection = SuggestedFixes.qualifyType(state, mutableFix, mutableReplacement); return buildDescription(tree) @@ -106,12 +113,13 @@ private Description suggestToMapAlternatives(MethodInvocationTree tree, VisitorS } SuggestedFix.Builder mutableFix = SuggestedFix.builder(); - String hashMap = SuggestedFixes.qualifyType(state, mutableFix, "java.util.HashMap"); + String hashMap = + SuggestedFixes.qualifyType(state, mutableFix, HashMap.class.getCanonicalName()); return buildDescription(tree) .addFix( replaceMethodInvocation( - tree, "com.google.common.collect.ImmutableMap.toImmutableMap", state)) + tree, ImmutableMap.class.getCanonicalName() + ".toImmutableMap", state)) .addFix( mutableFix .postfixWith( diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/EmptyMethod.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/EmptyMethod.java index e1b4173c71..d2b53cdb98 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/EmptyMethod.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/EmptyMethod.java @@ -36,7 +36,9 @@ public final class EmptyMethod extends BugChecker implements MethodTreeMatcher { private static final Matcher PERMITTED_ANNOTATION = annotations( AT_LEAST_ONE, - anyOf(isType("java.lang.Override"), isType("org.aspectj.lang.annotation.Pointcut"))); + anyOf( + isType(Override.class.getCanonicalName()), + isType("org.aspectj.lang.annotation.Pointcut"))); /** Instantiates a new {@link EmptyMethod} instance. */ public EmptyMethod() {} diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ErrorProneRuntimeClasspath.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ErrorProneRuntimeClasspath.java new file mode 100644 index 0000000000..df48d49f5b --- /dev/null +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ErrorProneRuntimeClasspath.java @@ -0,0 +1,165 @@ +package tech.picnic.errorprone.bugpatterns; + +import static com.google.common.collect.ImmutableSet.toImmutableSet; +import static com.google.errorprone.BugPattern.LinkType.CUSTOM; +import static com.google.errorprone.BugPattern.SeverityLevel.SUGGESTION; +import static com.google.errorprone.BugPattern.StandardTags.FRAGILE_CODE; +import static com.google.errorprone.matchers.Matchers.instanceMethod; +import static tech.picnic.errorprone.bugpatterns.util.Documentation.BUG_PATTERNS_BASE_URL; + +import com.google.auto.service.AutoService; +import com.google.common.collect.ImmutableSet; +import com.google.common.primitives.Primitives; +import com.google.errorprone.BugPattern; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker; +import com.google.errorprone.bugpatterns.BugChecker.LiteralTreeMatcher; +import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher; +import com.google.errorprone.fixes.SuggestedFix; +import com.google.errorprone.fixes.SuggestedFixes; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.matchers.Matcher; +import com.google.errorprone.util.ASTHelpers; +import com.sun.source.tree.AnnotationTree; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.LiteralTree; +import com.sun.source.tree.MethodInvocationTree; +import com.sun.tools.javac.code.Symbol; +import com.sun.tools.javac.code.Symbol.ClassSymbol; +import com.sun.tools.javac.util.Constants; +import java.util.regex.Pattern; +import tech.picnic.errorprone.bugpatterns.util.ThirdPartyLibrary; + +/** + * A {@link BugChecker} that flags literal strings in Error Prone Support code that represent the + * fully qualified class name of a type that's on Error Prone's classpath. + * + *

Deriving such strings from the associated {@link Class} instance makes for more maintainable + * code. + */ +// XXX: Consider generalizing this to a `RuntimeClasspath` check with a configurable +// `CLASSPATH_TYPES` regex. Such a check would likely be a no-op by default, with Error Prone +// Support's parent specifying the regex that is currently hard-coded. +// XXX: As-is this check may suggest usage of a string literal even if the relevant type will be on +// the runtime classpath. Review how we can further reduce false-positives. +// XXX: Slightly "out there", but we could try to derive the subset of classpath types likely to be +// available at runtime from the `compile`-scoped Maven dependencies in the `pom.xml` file of +// the module being compiled. +// XXX: Right now this check does not rewrite primitive string references such as `"int"` to +// `int.class.getCanonicalName()`. Review whether that's worth it. +@AutoService(BugChecker.class) +@BugPattern( + summary = + "Prefer `Class#getCanonicalName()` over an equivalent string literal if and only if the " + + "type will be on the runtime classpath", + link = BUG_PATTERNS_BASE_URL + "ErrorProneRuntimeClasspath", + linkType = CUSTOM, + severity = SUGGESTION, + tags = FRAGILE_CODE) +public final class ErrorProneRuntimeClasspath extends BugChecker + implements LiteralTreeMatcher, MethodInvocationTreeMatcher { + private static final long serialVersionUID = 1L; + private static final Matcher GET_CANONICAL_NAME_INVOCATION = + instanceMethod().onExactClass(Class.class.getCanonicalName()).named("getCanonicalName"); + private static final ImmutableSet PRIMITIVE_TYPES = + Primitives.allPrimitiveTypes().stream() + .map(Class::getCanonicalName) + .collect(toImmutableSet()); + + /** + * A pattern that matches fully qualified type names that are expected to be runtime dependencies + * of Error Prone, and that are thus presumed to be unconditionally present on a bug checker's + * runtime classpath. + */ + private static final Pattern CLASSPATH_TYPES = + Pattern.compile( + "com\\.google\\.common\\..*|com\\.google\\.errorprone\\.([^.]+(? GUAVA_FORMAT_METHOD = anyOf( staticMethod() - .onClass("com.google.common.base.Preconditions") + .onClass(Preconditions.class.getCanonicalName()) .namedAnyOf("checkArgument", "checkNotNull", "checkState"), - staticMethod().onClass("com.google.common.base.Verify").named("verify")); + staticMethod().onClass(Verify.class.getCanonicalName()).named("verify")); // XXX: Add `PrintWriter`, maybe others. private static final Matcher JDK_FORMAT_METHOD = anyOf( - staticMethod().onClass("java.lang.String").named("format"), - instanceMethod().onExactClass("java.util.Formatter").named("format")); + staticMethod().onClass(String.class.getCanonicalName()).named("format"), + instanceMethod().onExactClass(Formatter.class.getCanonicalName()).named("format")); private static final Matcher SLF4J_FORMAT_METHOD = instanceMethod() .onDescendantOf("org.slf4j.Logger") diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/IdentityConversion.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/IdentityConversion.java index 6389279366..f4b2a980b5 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/IdentityConversion.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/IdentityConversion.java @@ -10,6 +10,17 @@ import static tech.picnic.errorprone.bugpatterns.util.Documentation.BUG_PATTERNS_BASE_URL; import com.google.auto.service.AutoService; +import com.google.common.collect.ImmutableBiMap; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableListMultimap; +import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableMultimap; +import com.google.common.collect.ImmutableMultiset; +import com.google.common.collect.ImmutableRangeMap; +import com.google.common.collect.ImmutableRangeSet; +import com.google.common.collect.ImmutableSet; +import com.google.common.collect.ImmutableSetMultimap; +import com.google.common.collect.ImmutableTable; import com.google.common.primitives.Primitives; import com.google.errorprone.BugPattern; import com.google.errorprone.VisitorState; @@ -20,6 +31,7 @@ import com.google.errorprone.fixes.SuggestedFixes; import com.google.errorprone.matchers.Description; import com.google.errorprone.matchers.Matcher; +import com.google.errorprone.matchers.Matchers; import com.google.errorprone.util.ASTHelpers; import com.google.errorprone.util.ASTHelpers.TargetType; import com.sun.source.tree.ExpressionTree; @@ -59,21 +71,19 @@ public final class IdentityConversion extends BugChecker implements MethodInvoca staticMethod().onClass(String.class.getCanonicalName()).named("valueOf"), staticMethod() .onClassAny( - "com.google.common.collect.ImmutableBiMap", - "com.google.common.collect.ImmutableList", - "com.google.common.collect.ImmutableListMultimap", - "com.google.common.collect.ImmutableMap", - "com.google.common.collect.ImmutableMultimap", - "com.google.common.collect.ImmutableMultiset", - "com.google.common.collect.ImmutableRangeMap", - "com.google.common.collect.ImmutableRangeSet", - "com.google.common.collect.ImmutableSet", - "com.google.common.collect.ImmutableSetMultimap", - "com.google.common.collect.ImmutableTable") + ImmutableBiMap.class.getCanonicalName(), + ImmutableList.class.getCanonicalName(), + ImmutableListMultimap.class.getCanonicalName(), + ImmutableMap.class.getCanonicalName(), + ImmutableMultimap.class.getCanonicalName(), + ImmutableMultiset.class.getCanonicalName(), + ImmutableRangeMap.class.getCanonicalName(), + ImmutableRangeSet.class.getCanonicalName(), + ImmutableSet.class.getCanonicalName(), + ImmutableSetMultimap.class.getCanonicalName(), + ImmutableTable.class.getCanonicalName()) .named("copyOf"), - staticMethod() - .onClass("com.google.errorprone.matchers.Matchers") - .namedAnyOf("allOf", "anyOf"), + staticMethod().onClass(Matchers.class.getCanonicalName()).namedAnyOf("allOf", "anyOf"), staticMethod().onClass("reactor.adapter.rxjava.RxJava2Adapter"), staticMethod() .onClass("reactor.core.publisher.Flux") diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitValueSource.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitValueSource.java index 4834267e94..e6dc139f1d 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitValueSource.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitValueSource.java @@ -24,6 +24,8 @@ import static tech.picnic.errorprone.bugpatterns.util.MoreJUnitMatchers.getMethodSourceFactoryNames; import com.google.auto.service.AutoService; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; import com.google.errorprone.BugPattern; import com.google.errorprone.VisitorState; @@ -106,8 +108,8 @@ public final class JUnitValueSource extends BugChecker implements MethodTreeMatc DoubleStream.class.getCanonicalName(), List.class.getCanonicalName(), Set.class.getCanonicalName(), - "com.google.common.collect.ImmutableList", - "com.google.common.collect.ImmutableSet") + ImmutableList.class.getCanonicalName(), + ImmutableSet.class.getCanonicalName()) .named("of"), hasArguments(AT_LEAST_ONE, anything()), hasArguments(ALL, SUPPORTED_VALUE_FACTORY_VALUES))); diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/NonStaticImport.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/NonStaticImport.java index 8c5e2fa43a..bacf57fbe5 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/NonStaticImport.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/NonStaticImport.java @@ -9,6 +9,8 @@ import com.google.auto.service.AutoService; import com.google.auto.value.AutoValue; import com.google.common.annotations.VisibleForTesting; +import com.google.common.base.Predicates; +import com.google.common.base.Strings; import com.google.common.collect.ImmutableSet; import com.google.common.collect.ImmutableSetMultimap; import com.google.common.collect.ImmutableTable; @@ -27,6 +29,11 @@ import com.sun.source.tree.Tree; import com.sun.source.util.TreeScanner; import com.sun.tools.javac.code.Symbol; +import java.time.Clock; +import java.time.ZoneOffset; +import java.util.Collections; +import java.util.Locale; +import java.util.regex.Pattern; import org.jspecify.annotations.Nullable; import tech.picnic.errorprone.bugpatterns.util.SourceCode; @@ -59,12 +66,12 @@ public final class NonStaticImport extends BugChecker implements CompilationUnit @VisibleForTesting static final ImmutableSet NON_STATIC_IMPORT_CANDIDATE_TYPES = ImmutableSet.of( - "com.google.common.base.Strings", - "com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode", - "com.google.errorprone.VisitorState", - "com.google.errorprone.util.ASTHelpers", - "java.time.Clock", - "java.time.ZoneOffset"); + ASTHelpers.class.getCanonicalName(), + Clock.class.getCanonicalName(), + Strings.class.getCanonicalName(), + VisitorState.class.getCanonicalName(), + ZoneOffset.class.getCanonicalName(), + "com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode"); /** * Type members that should never be statically imported. @@ -83,9 +90,8 @@ public final class NonStaticImport extends BugChecker implements CompilationUnit // specific context is left out. static final ImmutableSetMultimap NON_STATIC_IMPORT_CANDIDATE_MEMBERS = ImmutableSetMultimap.builder() - .put("com.google.common.base.Predicates", "contains") .putAll( - "java.util.Collections", + Collections.class.getCanonicalName(), "addAll", "copy", "fill", @@ -96,8 +102,9 @@ public final class NonStaticImport extends BugChecker implements CompilationUnit "rotate", "sort", "swap") - .put("java.util.Locale", "ROOT") - .putAll("java.util.regex.Pattern", "compile", "matches", "quote") + .put(Locale.class.getCanonicalName(), "ROOT") + .putAll(Pattern.class.getCanonicalName(), "compile", "matches", "quote") + .put(Predicates.class.getCanonicalName(), "contains") .put("org.springframework.http.MediaType", "ALL") .build(); diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RedundantStringConversion.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RedundantStringConversion.java index 201b43eeef..4b6ba6ce3d 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RedundantStringConversion.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RedundantStringConversion.java @@ -18,6 +18,8 @@ import static tech.picnic.errorprone.bugpatterns.util.Documentation.BUG_PATTERNS_BASE_URL; import com.google.auto.service.AutoService; +import com.google.common.base.Preconditions; +import com.google.common.base.Verify; import com.google.common.collect.ImmutableList; import com.google.common.collect.Iterables; import com.google.common.primitives.Primitives; @@ -136,10 +138,10 @@ public final class RedundantStringConversion extends BugChecker private static final Matcher GUAVA_GUARD_INVOCATION = anyOf( staticMethod() - .onClass("com.google.common.base.Preconditions") + .onClass(Preconditions.class.getCanonicalName()) .namedAnyOf("checkArgument", "checkState", "checkNotNull"), staticMethod() - .onClass("com.google.common.base.Verify") + .onClass(Verify.class.getCanonicalName()) .namedAnyOf("verify", "verifyNotNull")); private static final Matcher SLF4J_LOGGER_INVOCATION = instanceMethod() diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RequestMappingAnnotation.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RequestMappingAnnotation.java index d8d6df2f80..7c1a48c01f 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RequestMappingAnnotation.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RequestMappingAnnotation.java @@ -22,6 +22,10 @@ import com.google.errorprone.matchers.Matcher; import com.sun.source.tree.MethodTree; import com.sun.source.tree.Tree; +import java.io.InputStream; +import java.time.ZoneId; +import java.util.Locale; +import java.util.TimeZone; /** * A {@link BugChecker} that flags {@code @RequestMapping} methods that have one or more parameters @@ -70,10 +74,10 @@ public final class RequestMappingAnnotation extends BugChecker implements Method isType(ANN_PACKAGE_PREFIX + "RequestHeader"), isType(ANN_PACKAGE_PREFIX + "RequestParam"), isType(ANN_PACKAGE_PREFIX + "RequestPart"))), - isSameType("java.io.InputStream"), - isSameType("java.time.ZoneId"), - isSameType("java.util.Locale"), - isSameType("java.util.TimeZone"), + isSameType(InputStream.class.getCanonicalName()), + isSameType(Locale.class.getCanonicalName()), + isSameType(TimeZone.class.getCanonicalName()), + isSameType(ZoneId.class.getCanonicalName()), isSameType("jakarta.servlet.http.HttpServletRequest"), isSameType("jakarta.servlet.http.HttpServletResponse"), isSameType("javax.servlet.http.HttpServletRequest"), diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/StaticImport.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/StaticImport.java index 8eb703f76d..f7fa3fe26b 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/StaticImport.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/StaticImport.java @@ -10,8 +10,24 @@ import com.google.auto.service.AutoService; import com.google.common.annotations.VisibleForTesting; +import com.google.common.base.Functions; +import com.google.common.base.Preconditions; +import com.google.common.base.Predicates; +import com.google.common.base.Verify; +import com.google.common.collect.Comparators; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableListMultimap; +import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableMultiset; +import com.google.common.collect.ImmutableRangeSet; import com.google.common.collect.ImmutableSet; import com.google.common.collect.ImmutableSetMultimap; +import com.google.common.collect.ImmutableSortedMap; +import com.google.common.collect.ImmutableSortedMultiset; +import com.google.common.collect.ImmutableSortedSet; +import com.google.common.collect.ImmutableTable; +import com.google.common.collect.MoreCollectors; +import com.google.common.collect.Sets; import com.google.errorprone.BugPattern; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker; @@ -22,12 +38,25 @@ import com.google.errorprone.fixes.SuggestedFix; import com.google.errorprone.fixes.SuggestedFixes; import com.google.errorprone.matchers.Description; +import com.google.errorprone.matchers.Matchers; +import com.google.errorprone.refaster.ImportPolicy; import com.google.errorprone.util.ASTHelpers; import com.sun.source.tree.MemberSelectTree; import com.sun.source.tree.MethodInvocationTree; import com.sun.source.tree.Tree; import com.sun.tools.javac.code.Type; +import java.nio.charset.StandardCharsets; +import java.time.ZoneOffset; +import java.util.Collections; +import java.util.Comparator; +import java.util.Map; +import java.util.Objects; import java.util.Optional; +import java.util.UUID; +import java.util.function.Function; +import java.util.function.Predicate; +import java.util.regex.Pattern; +import java.util.stream.Collectors; /** A {@link BugChecker} that flags type members that can and should be statically imported. */ // XXX: This check is closely linked to `NonStaticImport`. Consider merging the two. @@ -57,19 +86,25 @@ public final class StaticImport extends BugChecker implements MemberSelectTreeMa @VisibleForTesting static final ImmutableSet STATIC_IMPORT_CANDIDATE_TYPES = ImmutableSet.of( + BugPattern.LinkType.class.getCanonicalName(), + BugPattern.SeverityLevel.class.getCanonicalName(), + BugPattern.StandardTags.class.getCanonicalName(), + Collections.class.getCanonicalName(), + Collectors.class.getCanonicalName(), + Comparator.class.getCanonicalName(), + ImportPolicy.class.getCanonicalName(), + Map.Entry.class.getCanonicalName(), + Matchers.class.getCanonicalName(), + MoreCollectors.class.getCanonicalName(), + Pattern.class.getCanonicalName(), + Preconditions.class.getCanonicalName(), + Predicates.class.getCanonicalName(), + StandardCharsets.class.getCanonicalName(), + Verify.class.getCanonicalName(), "com.fasterxml.jackson.annotation.JsonCreator.Mode", "com.fasterxml.jackson.annotation.JsonFormat.Shape", "com.fasterxml.jackson.annotation.JsonInclude.Include", "com.fasterxml.jackson.annotation.JsonProperty.Access", - "com.google.common.base.Preconditions", - "com.google.common.base.Predicates", - "com.google.common.base.Verify", - "com.google.common.collect.MoreCollectors", - "com.google.errorprone.BugPattern.LinkType", - "com.google.errorprone.BugPattern.SeverityLevel", - "com.google.errorprone.BugPattern.StandardTags", - "com.google.errorprone.matchers.Matchers", - "com.google.errorprone.refaster.ImportPolicy", "com.mongodb.client.model.Accumulators", "com.mongodb.client.model.Aggregates", "com.mongodb.client.model.Filters", @@ -77,12 +112,6 @@ public final class StaticImport extends BugChecker implements MemberSelectTreeMa "com.mongodb.client.model.Projections", "com.mongodb.client.model.Sorts", "com.mongodb.client.model.Updates", - "java.nio.charset.StandardCharsets", - "java.util.Collections", - "java.util.Comparator", - "java.util.Map.Entry", - "java.util.regex.Pattern", - "java.util.stream.Collectors", "org.assertj.core.api.Assertions", "org.assertj.core.api.InstanceOfAssertFactories", "org.assertj.core.api.SoftAssertions", @@ -121,39 +150,39 @@ public final class StaticImport extends BugChecker implements MemberSelectTreeMa */ static final ImmutableSetMultimap STATIC_IMPORT_CANDIDATE_MEMBERS = ImmutableSetMultimap.builder() + .putAll(Comparators.class.getCanonicalName(), "emptiesFirst", "emptiesLast") + .put(Function.class.getCanonicalName(), "identity") + .put(Functions.class.getCanonicalName(), "identity") + .put(ImmutableList.class.getCanonicalName(), "toImmutableList") .putAll( - "com.google.common.collect.ImmutableListMultimap", + ImmutableListMultimap.class.getCanonicalName(), "flatteningToImmutableListMultimap", "toImmutableListMultimap") - .put("com.google.common.collect.ImmutableList", "toImmutableList") - .put("com.google.common.collect.ImmutableMap", "toImmutableMap") - .put("com.google.common.collect.ImmutableMultiset", "toImmutableMultiset") - .put("com.google.common.collect.ImmutableRangeSet", "toImmutableRangeSet") + .put(ImmutableMap.class.getCanonicalName(), "toImmutableMap") + .put(ImmutableMultiset.class.getCanonicalName(), "toImmutableMultiset") + .put(ImmutableRangeSet.class.getCanonicalName(), "toImmutableRangeSet") + .put(ImmutableSet.class.getCanonicalName(), "toImmutableSet") .putAll( - "com.google.common.collect.ImmutableSetMultimap", + ImmutableSetMultimap.class.getCanonicalName(), "flatteningToImmutableSetMultimap", "toImmutableSetMultimap") - .put("com.google.common.collect.ImmutableSet", "toImmutableSet") - .put("com.google.common.collect.ImmutableSortedMap", "toImmutableSortedMap") - .put("com.google.common.collect.ImmutableSortedMultiset", "toImmutableSortedMultiset") - .put("com.google.common.collect.ImmutableSortedSet", "toImmutableSortedSet") - .put("com.google.common.collect.ImmutableTable", "toImmutableTable") - .put("com.google.common.collect.Sets", "toImmutableEnumSet") - .put("com.google.common.base.Functions", "identity") - .put("java.time.ZoneOffset", "UTC") - .put("java.util.function.Function", "identity") - .put("java.util.function.Predicate", "not") - .put("java.util.UUID", "randomUUID") - .put("org.junit.jupiter.params.provider.Arguments", "arguments") + .put(ImmutableSortedMap.class.getCanonicalName(), "toImmutableSortedMap") + .put(ImmutableSortedMultiset.class.getCanonicalName(), "toImmutableSortedMultiset") + .put(ImmutableSortedSet.class.getCanonicalName(), "toImmutableSortedSet") + .put(ImmutableTable.class.getCanonicalName(), "toImmutableTable") .putAll( - "java.util.Objects", + Objects.class.getCanonicalName(), "checkIndex", "checkFromIndexSize", "checkFromToIndex", "requireNonNull", "requireNonNullElse", "requireNonNullElseGet") - .putAll("com.google.common.collect.Comparators", "emptiesFirst", "emptiesLast") + .put(Predicate.class.getCanonicalName(), "not") + .put(Sets.class.getCanonicalName(), "toImmutableEnumSet") + .put(UUID.class.getCanonicalName(), "randomUUID") + .put(ZoneOffset.class.getCanonicalName(), "UTC") + .put("org.junit.jupiter.params.provider.Arguments", "arguments") .build(); /** Instantiates a new {@link StaticImport} instance. */ diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/ThirdPartyLibrary.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/ThirdPartyLibrary.java index 51678cd335..04c994a915 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/ThirdPartyLibrary.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/ThirdPartyLibrary.java @@ -1,5 +1,6 @@ package tech.picnic.errorprone.bugpatterns.util; +import com.google.common.collect.ImmutableList; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker; import com.google.errorprone.suppliers.Supplier; @@ -31,7 +32,7 @@ public enum ThirdPartyLibrary { * * @see Guava on GitHub */ - GUAVA("com.google.common.collect.ImmutableList"), + GUAVA(ImmutableList.class.getCanonicalName()), /** * VMWare's Project Reactor. * @@ -66,7 +67,14 @@ public boolean isIntroductionAllowed(VisitorState state) { return canUse.get(state); } - private static boolean canIntroduceUsage(String className, VisitorState state) { + /** + * Tells whether the given fully qualified type is available on the current class path. + * + * @param className The type of interest. + * @param state The context under consideration. + * @return {@code true} iff it is okay to assume or create a dependency on this type. + */ + public static boolean canIntroduceUsage(String className, VisitorState state) { return shouldIgnoreClasspath(state) || isKnownClass(className, state); } diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ErrorProneRuntimeClasspathTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ErrorProneRuntimeClasspathTest.java new file mode 100644 index 0000000000..7aff4887d0 --- /dev/null +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ErrorProneRuntimeClasspathTest.java @@ -0,0 +1,136 @@ +package tech.picnic.errorprone.bugpatterns; + +import com.google.errorprone.BugCheckerRefactoringTestHelper; +import com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode; +import com.google.errorprone.CompilationTestHelper; +import org.junit.jupiter.api.Test; + +final class ErrorProneRuntimeClasspathTest { + @Test + void identification() { + CompilationTestHelper.newInstance(ErrorProneRuntimeClasspath.class, getClass()) + .expectErrorMessage( + "USE_CLASS_REFERENCE", + m -> + m.contains( + "This type will be on the runtime classpath; use `Class#getCanonicalName()` instead")) + .expectErrorMessage( + "USE_STRING_LITERAL", + m -> + m.contains( + "This type may not be on the runtime classpath; use a string literal instead")) + .addSourceLines( + "A.java", + "import com.google.common.collect.ImmutableList;", + "import com.google.errorprone.BugCheckerRefactoringTestHelper;", + "import com.google.errorprone.BugPattern;", + "import com.google.errorprone.CompilationTestHelper;", + "import org.junit.jupiter.api.Test;", + "", + "class A {", + " @SuppressWarnings(\"java.lang.String\")", + " void m(Object o) {", + " m(null);", + " m(0);", + " m(getClass().getName());", + " m(getClass().getCanonicalName());", + " m(\"\");", + " m(\"foo\");", + " m(\"java.util.\");", + "", + " m(\"org.junit.jupiter.api.Test\");", + " m(\"org.junit.jupiter.api.Test.toString\");", + " m(\"com.google.errorprone.CompilationTestHelper\");", + " m(\"com.google.errorprone.CompilationTestHelper.toString\");", + " m(\"com.google.errorprone.BugCheckerRefactoringTestHelper.ExpectOutput\");", + " m(\"com.google.errorprone.BugCheckerRefactoringTestHelper.ExpectOutput.toString\");", + " m(\"com.google.errorprone.NonExistent\");", + " m(\"com.google.common.NonExistent.toString\");", + " m(\"java.lang.NonExistent\");", + " // BUG: Diagnostic matches: USE_CLASS_REFERENCE", + " m(\"com.google.errorprone.BugPattern\");", + " // BUG: Diagnostic matches: USE_CLASS_REFERENCE", + " m(\"com.google.errorprone.util.ErrorProneToken\");", + " // BUG: Diagnostic matches: USE_CLASS_REFERENCE", + " m(\"com.google.common.collect.ImmutableList\");", + " // BUG: Diagnostic matches: USE_CLASS_REFERENCE", + " m(\"java.lang.String\");", + " // BUG: Diagnostic matches: USE_CLASS_REFERENCE", + " m(\"java.lang.String.toString\");", + "", + " m(BugPattern.class.getCanonicalName());", + " m(ImmutableList.class.getCanonicalName());", + " m(String.class.getCanonicalName());", + " m(void.class.getCanonicalName());", + " m(boolean.class.getCanonicalName());", + " m(byte.class.getCanonicalName());", + " m(char.class.getCanonicalName());", + " m(short.class.getCanonicalName());", + " m(int.class.getCanonicalName());", + " m(long.class.getCanonicalName());", + " m(float.class.getCanonicalName());", + " m(double.class.getCanonicalName());", + " m(java.lang.Iterable.class.getCanonicalName());", + " m(CompilationTestHelper.class.toString());", + " // BUG: Diagnostic matches: USE_STRING_LITERAL", + " m(CompilationTestHelper.class.getCanonicalName());", + " // BUG: Diagnostic matches: USE_STRING_LITERAL", + " m(BugCheckerRefactoringTestHelper.ExpectOutput.class.getCanonicalName());", + " // BUG: Diagnostic matches: USE_STRING_LITERAL", + " m(Test.class.getCanonicalName());", + " // BUG: Diagnostic matches: USE_STRING_LITERAL", + " m(org.junit.jupiter.api.Nested.class.getCanonicalName());", + " }", + "}") + .doTest(); + } + + @Test + void replacement() { + BugCheckerRefactoringTestHelper.newInstance(ErrorProneRuntimeClasspath.class, getClass()) + .addInputLines( + "A.java", + "import com.google.errorprone.BugCheckerRefactoringTestHelper;", + "import com.google.errorprone.CompilationTestHelper;", + "import org.junit.jupiter.api.Test;", + "", + "class A {", + " void m(Object o) {", + " m(\"com.google.errorprone.BugPattern\");", + " m(\"com.google.errorprone.util.ErrorProneToken\");", + " m(\"com.google.common.collect.ImmutableList\");", + " m(\"java.lang.String\");", + " m(\"java.lang.String.toString\");", + "", + " m(CompilationTestHelper.class.getCanonicalName());", + " m(BugCheckerRefactoringTestHelper.ExpectOutput.class.getCanonicalName());", + " m(Test.class.getCanonicalName());", + " m(org.junit.jupiter.api.Nested.class.getCanonicalName());", + " }", + "}") + .addOutputLines( + "A.java", + "import com.google.common.collect.ImmutableList;", + "import com.google.errorprone.BugCheckerRefactoringTestHelper;", + "import com.google.errorprone.BugPattern;", + "import com.google.errorprone.CompilationTestHelper;", + "import com.google.errorprone.util.ErrorProneToken;", + "import org.junit.jupiter.api.Test;", + "", + "class A {", + " void m(Object o) {", + " m(BugPattern.class.getCanonicalName());", + " m(ErrorProneToken.class.getCanonicalName());", + " m(ImmutableList.class.getCanonicalName());", + " m(String.class.getCanonicalName());", + " m(String.class.getCanonicalName() + \".toString\");", + "", + " m(\"com.google.errorprone.CompilationTestHelper\");", + " m(\"com.google.errorprone.BugCheckerRefactoringTestHelper.ExpectOutput\");", + " m(\"org.junit.jupiter.api.Test\");", + " m(\"org.junit.jupiter.api.Nested\");", + " }", + "}") + .doTest(TestMode.TEXT_MATCH); + } +} 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 index 29b6fed550..0a30ba4bc6 100644 --- 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 @@ -21,7 +21,10 @@ import com.sun.source.tree.MethodInvocationTree; import com.sun.tools.javac.code.Type; import java.util.ArrayList; +import java.util.Collection; import java.util.List; +import java.util.Map; +import java.util.Optional; import org.junit.jupiter.api.Test; final class MoreTypesTest { @@ -155,41 +158,57 @@ private static ImmutableSet> getTestTypes() { return ImmutableSet.of( // Invalid types. type("java.lang.Nonexistent"), - generic(type("java.util.Integer"), unbound()), + generic(type("java.util.Nonexistent"), unbound()), // Valid types. - type("java.lang.String"), - type("java.lang.Number"), - superOf(type("java.lang.Number")), - subOf(type("java.lang.Number")), - type("java.lang.Integer"), - superOf(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"), superOf(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"), superOf(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"), superOf(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"), superOf(type("java.lang.Integer"))), - generic(type("java.util.List"), subOf(type("java.lang.Integer"))), + type(String.class.getCanonicalName()), + type(Number.class.getCanonicalName()), + superOf(type(Number.class.getCanonicalName())), + subOf(type(Number.class.getCanonicalName())), + type(Integer.class.getCanonicalName()), + superOf(type(Integer.class.getCanonicalName())), + subOf(type(Integer.class.getCanonicalName())), + type(Optional.class.getCanonicalName()), + raw(type(Optional.class.getCanonicalName())), + generic(type(Optional.class.getCanonicalName()), unbound()), + generic(type(Optional.class.getCanonicalName()), type(Number.class.getCanonicalName())), + type(Collection.class.getCanonicalName()), + raw(type(Collection.class.getCanonicalName())), + generic(type(Collection.class.getCanonicalName()), unbound()), + generic(type(Collection.class.getCanonicalName()), type(Number.class.getCanonicalName())), generic( - type("java.util.Map"), - type("java.lang.String"), - subOf(generic(type("java.util.Collection"), superOf(type("java.lang.Short")))))); + type(Collection.class.getCanonicalName()), + superOf(type(Number.class.getCanonicalName()))), + generic( + type(Collection.class.getCanonicalName()), + subOf(type(Number.class.getCanonicalName()))), + generic( + type(Collection.class.getCanonicalName()), type(Integer.class.getCanonicalName())), + generic( + type(Collection.class.getCanonicalName()), + superOf(type(Integer.class.getCanonicalName()))), + generic( + type(Collection.class.getCanonicalName()), + subOf(type(Integer.class.getCanonicalName()))), + type(List.class.getCanonicalName()), + raw(type(List.class.getCanonicalName())), + generic(type(List.class.getCanonicalName()), unbound()), + generic(type(List.class.getCanonicalName()), type(Number.class.getCanonicalName())), + generic( + type(List.class.getCanonicalName()), superOf(type(Number.class.getCanonicalName()))), + generic( + type(List.class.getCanonicalName()), subOf(type(Number.class.getCanonicalName()))), + generic(type(List.class.getCanonicalName()), type(Integer.class.getCanonicalName())), + generic( + type(List.class.getCanonicalName()), superOf(type(Integer.class.getCanonicalName()))), + generic( + type(List.class.getCanonicalName()), subOf(type(Integer.class.getCanonicalName()))), + generic( + type(Map.class.getCanonicalName()), + type(String.class.getCanonicalName()), + subOf( + generic( + type(Collection.class.getCanonicalName()), + superOf(type(Short.class.getCanonicalName())))))); } } }