From 29dc33c4d6fc8fddaa5c9c7ee333776bd10a1ea1 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Sat, 11 Nov 2023 16:07:41 +0100 Subject: [PATCH 1/8] Introduce `ErrorProneRuntimeClasspath` check Prefer "type-safe" type references were possible, but use string literals if the references type may not be available at runtime. --- .../ErrorProneRuntimeClasspath.java | 157 ++++++++++++++++++ .../bugpatterns/util/ThirdPartyLibrary.java | 9 +- .../ErrorProneRuntimeClasspathTest.java | 135 +++++++++++++++ 3 files changed, 300 insertions(+), 1 deletion(-) 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/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..cafac9cf9a --- /dev/null +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ErrorProneRuntimeClasspath.java @@ -0,0 +1,157 @@ +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 Pattern CLASSPATH_TYPES = + Pattern.compile( + "^(com\\.google\\.common\\..*|com\\.google\\.errorprone\\.([^\\.]+(? 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()); + + /** Instantiates a new {@link ErrorProneRuntimeClasspath} instance. */ + public ErrorProneRuntimeClasspath() {} + + @Override + public Description matchLiteral(LiteralTree tree, VisitorState state) { + String value = ASTHelpers.constValue(tree, String.class); + if (value == null + || !CLASSPATH_TYPES.matcher(value).matches() + || state.findEnclosing(AnnotationTree.class) != null) { + return Description.NO_MATCH; + } + + SuggestedFix fix = trySuggestClassReference(tree, value, state); + if (fix.isEmpty()) { + return Description.NO_MATCH; + } + + return buildDescription(tree) + .setMessage( + "This type will be on the runtime classpath; use `Class#getCanonicalName()` instead") + .addFix(fix) + .build(); + } + + private static SuggestedFix trySuggestClassReference( + LiteralTree tree, String value, VisitorState state) { + if (isTypeOnClasspath(value, state)) { + return suggestClassReference(tree, value, "", state); + } + + int lastDot = value.lastIndexOf('.'); + String type = value.substring(0, lastDot); + if (isTypeOnClasspath(type, state)) { + return suggestClassReference(tree, type, value.substring(lastDot), state); + } + + return SuggestedFix.emptyFix(); + } + + private static SuggestedFix suggestClassReference( + LiteralTree original, String type, String suffix, VisitorState state) { + SuggestedFix.Builder fix = SuggestedFix.builder(); + String identifier = SuggestedFixes.qualifyType(state, fix, type); + return fix.replace( + original, + identifier + + ".class.getCanonicalName()" + + (suffix.isEmpty() ? "" : " + " + Constants.format(suffix))) + .build(); + } + + @Override + public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { + if (!GET_CANONICAL_NAME_INVOCATION.matches(tree, state)) { + return Description.NO_MATCH; + } + + Symbol receiver = ASTHelpers.getSymbol(ASTHelpers.getReceiver(tree)); + if (receiver == null + || !(receiver.owner instanceof ClassSymbol) + || PRIMITIVE_TYPES.contains(receiver.owner.getQualifiedName().toString()) + || CLASSPATH_TYPES.matcher(receiver.owner.getQualifiedName()).matches()) { + return Description.NO_MATCH; + } + + /* + * This class reference may not be safe; suggest using a string literal instead. (Note that + * dropping the type reference may make the associated import statement (if any) obsolete; + * dropping such imports is left to Error Prone's `RemoveUnusedImports` check.) + */ + return buildDescription(tree) + .setMessage("This type may not be on the runtime classpath; use a string literal instead") + .addFix( + SuggestedFix.replace( + tree, Constants.format(receiver.owner.getQualifiedName().toString()))) + .build(); + } + + private static boolean isTypeOnClasspath(String type, VisitorState state) { + try { + return ThirdPartyLibrary.canIntroduceUsage(type, state); + } catch (IllegalArgumentException e) { + return false; + } + } +} 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..6eb66e9fac 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 @@ -66,7 +66,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..d8a6a17b46 --- /dev/null +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ErrorProneRuntimeClasspathTest.java @@ -0,0 +1,135 @@ +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());", + " // 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); + } +} From 17088d7acc7b922e72c565c7d1affb0e74515c58 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Mon, 13 Nov 2023 06:01:57 +0100 Subject: [PATCH 2/8] Apply automated changes --- .../BugPatternTestExtractor.java | 2 +- .../bugpatterns/CollectorMutability.java | 24 +++-- .../errorprone/bugpatterns/EmptyMethod.java | 4 +- .../bugpatterns/FluxImplicitBlock.java | 7 +- .../FormatStringConcatenation.java | 11 ++- .../bugpatterns/IdentityConversion.java | 38 +++++--- .../bugpatterns/JUnitValueSource.java | 6 +- .../bugpatterns/NonStaticImport.java | 25 +++-- .../RedundantStringConversion.java | 6 +- .../bugpatterns/RequestMappingAnnotation.java | 12 ++- .../errorprone/bugpatterns/StaticImport.java | 97 ++++++++++++------- .../bugpatterns/util/ThirdPartyLibrary.java | 3 +- .../bugpatterns/util/MoreTypesTest.java | 83 ++++++++++------ 13 files changed, 204 insertions(+), 114 deletions(-) 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/FluxImplicitBlock.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/FluxImplicitBlock.java index 7691de3e11..26694e6a7c 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/FluxImplicitBlock.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/FluxImplicitBlock.java @@ -9,6 +9,7 @@ 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.errorprone.BugPattern; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker; @@ -24,6 +25,7 @@ import com.sun.source.tree.MethodInvocationTree; import com.sun.tools.javac.code.Type; import com.sun.tools.javac.util.Position; +import java.util.stream.Collectors; import java.util.stream.Stream; import tech.picnic.errorprone.bugpatterns.util.ThirdPartyLibrary; @@ -65,10 +67,11 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState if (ThirdPartyLibrary.GUAVA.isIntroductionAllowed(state)) { description.addFix( suggestBlockingElementCollection( - tree, "com.google.common.collect.ImmutableList.toImmutableList", state)); + tree, ImmutableList.class.getCanonicalName() + ".toImmutableList", state)); } description.addFix( - suggestBlockingElementCollection(tree, "java.util.stream.Collectors.toList", state)); + suggestBlockingElementCollection( + tree, Collectors.class.getCanonicalName() + ".toList", state)); return description.build(); } diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/FormatStringConcatenation.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/FormatStringConcatenation.java index ce41913501..9f4c8f51c8 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/FormatStringConcatenation.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/FormatStringConcatenation.java @@ -13,6 +13,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.errorprone.BugPattern; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker; @@ -30,6 +32,7 @@ import com.sun.source.tree.Tree.Kind; import com.sun.source.util.SimpleTreeVisitor; import java.util.ArrayList; +import java.util.Formatter; import java.util.List; import java.util.Optional; import org.jspecify.annotations.Nullable; @@ -118,14 +121,14 @@ public final class FormatStringConcatenation extends BugChecker private static final Matcher 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..b1cea87abc 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", + Strings.class.getCanonicalName(), "com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode", - "com.google.errorprone.VisitorState", - "com.google.errorprone.util.ASTHelpers", - "java.time.Clock", - "java.time.ZoneOffset"); + VisitorState.class.getCanonicalName(), + ASTHelpers.class.getCanonicalName(), + Clock.class.getCanonicalName(), + ZoneOffset.class.getCanonicalName()); /** * Type members that should never be statically imported. @@ -83,9 +90,9 @@ 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") + .put(Predicates.class.getCanonicalName(), "contains") .putAll( - "java.util.Collections", + Collections.class.getCanonicalName(), "addAll", "copy", "fill", @@ -96,8 +103,8 @@ 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("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..f662e54f82 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(ZoneId.class.getCanonicalName()), + isSameType(Locale.class.getCanonicalName()), + isSameType(TimeZone.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..dccfe1a26e 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. @@ -61,15 +90,15 @@ public final class StaticImport extends BugChecker implements MemberSelectTreeMa "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", + Preconditions.class.getCanonicalName(), + Predicates.class.getCanonicalName(), + Verify.class.getCanonicalName(), + MoreCollectors.class.getCanonicalName(), + BugPattern.LinkType.class.getCanonicalName(), + BugPattern.SeverityLevel.class.getCanonicalName(), + BugPattern.StandardTags.class.getCanonicalName(), + Matchers.class.getCanonicalName(), + ImportPolicy.class.getCanonicalName(), "com.mongodb.client.model.Accumulators", "com.mongodb.client.model.Aggregates", "com.mongodb.client.model.Filters", @@ -77,12 +106,12 @@ 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", + StandardCharsets.class.getCanonicalName(), + Collections.class.getCanonicalName(), + Comparator.class.getCanonicalName(), + Map.Entry.class.getCanonicalName(), + Pattern.class.getCanonicalName(), + Collectors.class.getCanonicalName(), "org.assertj.core.api.Assertions", "org.assertj.core.api.InstanceOfAssertFactories", "org.assertj.core.api.SoftAssertions", @@ -122,38 +151,38 @@ public final class StaticImport extends BugChecker implements MemberSelectTreeMa static final ImmutableSetMultimap STATIC_IMPORT_CANDIDATE_MEMBERS = ImmutableSetMultimap.builder() .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(ImmutableList.class.getCanonicalName(), "toImmutableList") + .put(ImmutableMap.class.getCanonicalName(), "toImmutableMap") + .put(ImmutableMultiset.class.getCanonicalName(), "toImmutableMultiset") + .put(ImmutableRangeSet.class.getCanonicalName(), "toImmutableRangeSet") .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(ImmutableSet.class.getCanonicalName(), "toImmutableSet") + .put(ImmutableSortedMap.class.getCanonicalName(), "toImmutableSortedMap") + .put(ImmutableSortedMultiset.class.getCanonicalName(), "toImmutableSortedMultiset") + .put(ImmutableSortedSet.class.getCanonicalName(), "toImmutableSortedSet") + .put(ImmutableTable.class.getCanonicalName(), "toImmutableTable") + .put(Sets.class.getCanonicalName(), "toImmutableEnumSet") + .put(Functions.class.getCanonicalName(), "identity") + .put(ZoneOffset.class.getCanonicalName(), "UTC") + .put(Function.class.getCanonicalName(), "identity") + .put(Predicate.class.getCanonicalName(), "not") + .put(UUID.class.getCanonicalName(), "randomUUID") .put("org.junit.jupiter.params.provider.Arguments", "arguments") .putAll( - "java.util.Objects", + Objects.class.getCanonicalName(), "checkIndex", "checkFromIndexSize", "checkFromToIndex", "requireNonNull", "requireNonNullElse", "requireNonNullElseGet") - .putAll("com.google.common.collect.Comparators", "emptiesFirst", "emptiesLast") + .putAll(Comparators.class.getCanonicalName(), "emptiesFirst", "emptiesLast") .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 6eb66e9fac..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. * 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..195cdd8249 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 { @@ -157,39 +160,55 @@ private static ImmutableSet> getTestTypes() { type("java.lang.Nonexistent"), generic(type("java.util.Integer"), 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())))))); } } } From 7ffa6ac2ffdeca05b53afd57dffab9cbbe5d197d Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Mon, 13 Nov 2023 06:06:13 +0100 Subject: [PATCH 3/8] Manual tweaks --- .../bugpatterns/RequestMappingAnnotation.java | 2 +- .../errorprone/bugpatterns/StaticImport.java | 30 +++++++++---------- .../bugpatterns/util/MoreTypesTest.java | 2 +- 3 files changed, 17 insertions(+), 17 deletions(-) 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 f662e54f82..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 @@ -75,9 +75,9 @@ public final class RequestMappingAnnotation extends BugChecker implements Method isType(ANN_PACKAGE_PREFIX + "RequestParam"), isType(ANN_PACKAGE_PREFIX + "RequestPart"))), isSameType(InputStream.class.getCanonicalName()), - isSameType(ZoneId.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 dccfe1a26e..43b8853e16 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 @@ -86,19 +86,25 @@ public final class StaticImport extends BugChecker implements MemberSelectTreeMa @VisibleForTesting static final ImmutableSet STATIC_IMPORT_CANDIDATE_TYPES = ImmutableSet.of( - "com.fasterxml.jackson.annotation.JsonCreator.Mode", - "com.fasterxml.jackson.annotation.JsonFormat.Shape", - "com.fasterxml.jackson.annotation.JsonInclude.Include", - "com.fasterxml.jackson.annotation.JsonProperty.Access", - Preconditions.class.getCanonicalName(), - Predicates.class.getCanonicalName(), - Verify.class.getCanonicalName(), - MoreCollectors.class.getCanonicalName(), BugPattern.LinkType.class.getCanonicalName(), BugPattern.SeverityLevel.class.getCanonicalName(), BugPattern.StandardTags.class.getCanonicalName(), - Matchers.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.mongodb.client.model.Accumulators", "com.mongodb.client.model.Aggregates", "com.mongodb.client.model.Filters", @@ -106,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", - StandardCharsets.class.getCanonicalName(), - Collections.class.getCanonicalName(), - Comparator.class.getCanonicalName(), - Map.Entry.class.getCanonicalName(), - Pattern.class.getCanonicalName(), - Collectors.class.getCanonicalName(), "org.assertj.core.api.Assertions", "org.assertj.core.api.InstanceOfAssertFactories", "org.assertj.core.api.SoftAssertions", 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 195cdd8249..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 @@ -158,7 +158,7 @@ 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(String.class.getCanonicalName()), type(Number.class.getCanonicalName()), From 492ac0e2e9adf9089ee6f7771b8a7643b9c0c116 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Mon, 13 Nov 2023 06:33:55 +0100 Subject: [PATCH 4/8] Kill final mutant --- .../errorprone/bugpatterns/ErrorProneRuntimeClasspathTest.java | 1 + 1 file changed, 1 insertion(+) 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 index d8a6a17b46..7aff4887d0 100644 --- 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 @@ -71,6 +71,7 @@ void identification() { " 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", From f7acb3c78dded107804e8eb3c039b19d3850ab76 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Mon, 13 Nov 2023 08:25:17 +0100 Subject: [PATCH 5/8] Resolve some SonarCloud warnings --- .../bugpatterns/ErrorProneRuntimeClasspath.java | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) 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 index cafac9cf9a..205a939628 100644 --- 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 @@ -116,7 +116,7 @@ private static SuggestedFix suggestClassReference( original, identifier + ".class.getCanonicalName()" - + (suffix.isEmpty() ? "" : " + " + Constants.format(suffix))) + + (suffix.isEmpty() ? "" : (" + " + Constants.format(suffix)))) .build(); } @@ -136,8 +136,8 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState /* * This class reference may not be safe; suggest using a string literal instead. (Note that - * dropping the type reference may make the associated import statement (if any) obsolete; - * dropping such imports is left to Error Prone's `RemoveUnusedImports` check.) + * dropping the type reference may make the associated import statement (if any) obsolete. + * Dropping such imports is left to Error Prone's `RemoveUnusedImports` check.) */ return buildDescription(tree) .setMessage("This type may not be on the runtime classpath; use a string literal instead") @@ -150,7 +150,9 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState private static boolean isTypeOnClasspath(String type, VisitorState state) { try { return ThirdPartyLibrary.canIntroduceUsage(type, state); - } catch (IllegalArgumentException e) { + } catch ( + @SuppressWarnings("java:S1166" /* Not exceptional. */) + IllegalArgumentException e) { return false; } } From a8e16eeef5ba7dd669e7c3a37ee5591e21c4b76b Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Mon, 18 Dec 2023 22:02:11 +0100 Subject: [PATCH 6/8] Tweaks --- .../ErrorProneRuntimeClasspath.java | 12 ++++++++--- .../bugpatterns/NonStaticImport.java | 10 +++++----- .../errorprone/bugpatterns/StaticImport.java | 20 +++++++++---------- 3 files changed, 24 insertions(+), 18 deletions(-) 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 index 205a939628..211a24a975 100644 --- 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 @@ -59,9 +59,6 @@ public final class ErrorProneRuntimeClasspath extends BugChecker implements LiteralTreeMatcher, MethodInvocationTreeMatcher { private static final long serialVersionUID = 1L; - private static final Pattern CLASSPATH_TYPES = - Pattern.compile( - "^(com\\.google\\.common\\..*|com\\.google\\.errorprone\\.([^\\.]+(? GET_CANONICAL_NAME_INVOCATION = instanceMethod().onExactClass(Class.class.getCanonicalName()).named("getCanonicalName"); private static final ImmutableSet PRIMITIVE_TYPES = @@ -69,6 +66,15 @@ public final class ErrorProneRuntimeClasspath extends BugChecker .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\\.([^\\.]+(? NON_STATIC_IMPORT_CANDIDATE_TYPES = ImmutableSet.of( - Strings.class.getCanonicalName(), - "com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode", - VisitorState.class.getCanonicalName(), ASTHelpers.class.getCanonicalName(), Clock.class.getCanonicalName(), - ZoneOffset.class.getCanonicalName()); + Strings.class.getCanonicalName(), + VisitorState.class.getCanonicalName(), + ZoneOffset.class.getCanonicalName(), + "com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode"); /** * Type members that should never be statically imported. @@ -90,7 +90,6 @@ public final class NonStaticImport extends BugChecker implements CompilationUnit // specific context is left out. static final ImmutableSetMultimap NON_STATIC_IMPORT_CANDIDATE_MEMBERS = ImmutableSetMultimap.builder() - .put(Predicates.class.getCanonicalName(), "contains") .putAll( Collections.class.getCanonicalName(), "addAll", @@ -105,6 +104,7 @@ public final class NonStaticImport extends BugChecker implements CompilationUnit "swap") .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/StaticImport.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/StaticImport.java index 43b8853e16..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 @@ -150,30 +150,26 @@ 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( ImmutableListMultimap.class.getCanonicalName(), "flatteningToImmutableListMultimap", "toImmutableListMultimap") - .put(ImmutableList.class.getCanonicalName(), "toImmutableList") .put(ImmutableMap.class.getCanonicalName(), "toImmutableMap") .put(ImmutableMultiset.class.getCanonicalName(), "toImmutableMultiset") .put(ImmutableRangeSet.class.getCanonicalName(), "toImmutableRangeSet") + .put(ImmutableSet.class.getCanonicalName(), "toImmutableSet") .putAll( ImmutableSetMultimap.class.getCanonicalName(), "flatteningToImmutableSetMultimap", "toImmutableSetMultimap") - .put(ImmutableSet.class.getCanonicalName(), "toImmutableSet") .put(ImmutableSortedMap.class.getCanonicalName(), "toImmutableSortedMap") .put(ImmutableSortedMultiset.class.getCanonicalName(), "toImmutableSortedMultiset") .put(ImmutableSortedSet.class.getCanonicalName(), "toImmutableSortedSet") .put(ImmutableTable.class.getCanonicalName(), "toImmutableTable") - .put(Sets.class.getCanonicalName(), "toImmutableEnumSet") - .put(Functions.class.getCanonicalName(), "identity") - .put(ZoneOffset.class.getCanonicalName(), "UTC") - .put(Function.class.getCanonicalName(), "identity") - .put(Predicate.class.getCanonicalName(), "not") - .put(UUID.class.getCanonicalName(), "randomUUID") - .put("org.junit.jupiter.params.provider.Arguments", "arguments") .putAll( Objects.class.getCanonicalName(), "checkIndex", @@ -182,7 +178,11 @@ public final class StaticImport extends BugChecker implements MemberSelectTreeMa "requireNonNull", "requireNonNullElse", "requireNonNullElseGet") - .putAll(Comparators.class.getCanonicalName(), "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. */ From a1846e49b623c6b4e408bd59a74fe39ab752e4be Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Wed, 20 Dec 2023 12:55:46 +0100 Subject: [PATCH 7/8] Suggestion --- .../errorprone/bugpatterns/ErrorProneRuntimeClasspath.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 index 211a24a975..fe6bfd6431 100644 --- 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 @@ -73,7 +73,7 @@ public final class ErrorProneRuntimeClasspath extends BugChecker */ private static final Pattern CLASSPATH_TYPES = Pattern.compile( - "com\\.google\\.common\\..*|com\\.google\\.errorprone\\.([^\\.]+(? Date: Wed, 20 Dec 2023 14:44:08 +0100 Subject: [PATCH 8/8] Move the method --- .../ErrorProneRuntimeClasspath.java | 54 +++++++++---------- 1 file changed, 27 insertions(+), 27 deletions(-) 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 index fe6bfd6431..df48d49f5b 100644 --- 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 @@ -99,33 +99,6 @@ public Description matchLiteral(LiteralTree tree, VisitorState state) { .build(); } - private static SuggestedFix trySuggestClassReference( - LiteralTree tree, String value, VisitorState state) { - if (isTypeOnClasspath(value, state)) { - return suggestClassReference(tree, value, "", state); - } - - int lastDot = value.lastIndexOf('.'); - String type = value.substring(0, lastDot); - if (isTypeOnClasspath(type, state)) { - return suggestClassReference(tree, type, value.substring(lastDot), state); - } - - return SuggestedFix.emptyFix(); - } - - private static SuggestedFix suggestClassReference( - LiteralTree original, String type, String suffix, VisitorState state) { - SuggestedFix.Builder fix = SuggestedFix.builder(); - String identifier = SuggestedFixes.qualifyType(state, fix, type); - return fix.replace( - original, - identifier - + ".class.getCanonicalName()" - + (suffix.isEmpty() ? "" : (" + " + Constants.format(suffix)))) - .build(); - } - @Override public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { if (!GET_CANONICAL_NAME_INVOCATION.matches(tree, state)) { @@ -153,6 +126,33 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState .build(); } + private static SuggestedFix trySuggestClassReference( + LiteralTree tree, String value, VisitorState state) { + if (isTypeOnClasspath(value, state)) { + return suggestClassReference(tree, value, "", state); + } + + int lastDot = value.lastIndexOf('.'); + String type = value.substring(0, lastDot); + if (isTypeOnClasspath(type, state)) { + return suggestClassReference(tree, type, value.substring(lastDot), state); + } + + return SuggestedFix.emptyFix(); + } + + private static SuggestedFix suggestClassReference( + LiteralTree original, String type, String suffix, VisitorState state) { + SuggestedFix.Builder fix = SuggestedFix.builder(); + String identifier = SuggestedFixes.qualifyType(state, fix, type); + return fix.replace( + original, + identifier + + ".class.getCanonicalName()" + + (suffix.isEmpty() ? "" : (" + " + Constants.format(suffix)))) + .build(); + } + private static boolean isTypeOnClasspath(String type, VisitorState state) { try { return ThirdPartyLibrary.canIntroduceUsage(type, state);