Introduce CanonicalClassNameUsage check (#881)

Error Prone checks deal with source code and type matchers, both of
which generally involve canonical type names, rather than the strings
produced by `Class#getName()`. This distinction is particularly relevant
when dealing with nested types.
This commit is contained in:
Stephan Schroevers
2023-12-18 09:19:13 +01:00
committed by GitHub
parent 22272d6059
commit 626246bcc0
20 changed files with 247 additions and 62 deletions

View File

@@ -0,0 +1,92 @@
package tech.picnic.errorprone.bugpatterns;
import static com.google.errorprone.BugPattern.LinkType.CUSTOM;
import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;
import static com.google.errorprone.BugPattern.StandardTags.FRAGILE_CODE;
import static com.google.errorprone.matchers.Matchers.allOf;
import static com.google.errorprone.matchers.Matchers.anything;
import static com.google.errorprone.matchers.Matchers.classLiteral;
import static com.google.errorprone.matchers.Matchers.instanceMethod;
import static com.google.errorprone.matchers.Matchers.receiverOfInvocation;
import static com.google.errorprone.matchers.Matchers.toType;
import static tech.picnic.errorprone.bugpatterns.util.Documentation.BUG_PATTERNS_BASE_URL;
import com.google.auto.service.AutoService;
import com.google.errorprone.BugPattern;
import com.google.errorprone.VisitorState;
import com.google.errorprone.annotations.Var;
import com.google.errorprone.bugpatterns.BugChecker;
import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher;
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.BinaryTree;
import com.sun.source.tree.ExpressionTree;
import com.sun.source.tree.MethodInvocationTree;
import com.sun.source.util.TreePath;
import com.sun.tools.javac.code.Symbol.MethodSymbol;
import java.util.regex.Pattern;
/**
* A {@link BugChecker} that flags invocations of {@link Class#getName()} where {@link
* Class#getCanonicalName()} was likely meant.
*
* <p>For top-level types these two methods generally return the same result, but for nested types
* the former separates identifiers using a dollar sign ({@code $}) rather than a dot ({@code .}).
*
* @implNote This check currently only flags {@link Class#getName()} invocations on class literals,
* and doesn't flag method references. This avoids false positives, such as suggesting use of
* {@link Class#getCanonicalName()} in contexts where the canonical name is {@code null}.
*/
@AutoService(BugChecker.class)
@BugPattern(
summary = "This code should likely use the type's canonical name",
link = BUG_PATTERNS_BASE_URL + "CanonicalClassNameUsage",
linkType = CUSTOM,
severity = WARNING,
tags = FRAGILE_CODE)
public final class CanonicalClassNameUsage extends BugChecker
implements MethodInvocationTreeMatcher {
private static final long serialVersionUID = 1L;
private static final Matcher<ExpressionTree> GET_NAME_INVOCATION =
toType(
MethodInvocationTree.class,
allOf(
receiverOfInvocation(classLiteral(anything())),
instanceMethod().onExactClass(Class.class.getCanonicalName()).named("getName")));
private static final Pattern CANONICAL_NAME_USING_TYPES =
Pattern.compile("(com\\.google\\.errorprone|tech\\.picnic\\.errorprone)\\..*");
/** Instantiates a new {@link CanonicalClassNameUsage} instance. */
public CanonicalClassNameUsage() {}
@Override
public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
if (!GET_NAME_INVOCATION.matches(tree, state) || !isPassedToCanonicalNameUsingType(state)) {
/*
* This is not a `class.getName()` invocation of which the result is passed to another method
* known to accept canonical type names.
*/
return Description.NO_MATCH;
}
return describeMatch(
tree, SuggestedFixes.renameMethodInvocation(tree, "getCanonicalName", state));
}
private static boolean isPassedToCanonicalNameUsingType(VisitorState state) {
@Var TreePath path = state.getPath().getParentPath();
while (path.getLeaf() instanceof BinaryTree) {
path = path.getParentPath();
}
return path.getLeaf() instanceof MethodInvocationTree
&& isOwnedByCanonicalNameUsingType(
ASTHelpers.getSymbol((MethodInvocationTree) path.getLeaf()));
}
private static boolean isOwnedByCanonicalNameUsingType(MethodSymbol symbol) {
return CANONICAL_NAME_USING_TYPES.matcher(symbol.owner.getQualifiedName()).matches();
}
}

View File

@@ -58,7 +58,10 @@ public final class DirectReturn extends BugChecker implements BlockTreeMatcher {
private static final Matcher<StatementTree> VARIABLE_RETURN = returnStatement(isVariable());
private static final Matcher<ExpressionTree> MOCKITO_MOCK_OR_SPY_WITH_IMPLICIT_TYPE =
allOf(
not(toType(MethodInvocationTree.class, argument(0, isSameType(Class.class.getName())))),
not(
toType(
MethodInvocationTree.class,
argument(0, isSameType(Class.class.getCanonicalName())))),
staticMethod().onClass("org.mockito.Mockito").namedAnyOf("mock", "spy"));
/** Instantiates a new {@link DirectReturn} instance. */

View File

@@ -44,7 +44,7 @@ import java.util.stream.Stream;
public final class ExplicitEnumOrdering extends BugChecker implements MethodInvocationTreeMatcher {
private static final long serialVersionUID = 1L;
private static final Matcher<ExpressionTree> EXPLICIT_ORDERING =
staticMethod().onClass(Ordering.class.getName()).named("explicit");
staticMethod().onClass(Ordering.class.getCanonicalName()).named("explicit");
/** Instantiates a new {@link ExplicitEnumOrdering} instance. */
public ExplicitEnumOrdering() {}

View File

@@ -66,7 +66,7 @@ public final class FluxFlatMapUsage extends BugChecker
instanceMethod()
.onDescendantOf(FLUX)
.namedAnyOf("flatMap", "flatMapSequential")
.withParameters(Function.class.getName());
.withParameters(Function.class.getCanonicalName());
private static final Supplier<Type> FLUX_OF_PUBLISHERS =
VisitorState.memoize(
generic(FLUX, subOf(generic(type("org.reactivestreams.Publisher"), unbound()))));

View File

@@ -48,7 +48,8 @@ public final class FluxImplicitBlock extends BugChecker implements MethodInvocat
.onDescendantOf("reactor.core.publisher.Flux")
.namedAnyOf("toIterable", "toStream")
.withNoParameters();
private static final Supplier<Type> STREAM = Suppliers.typeFromString(Stream.class.getName());
private static final Supplier<Type> STREAM =
Suppliers.typeFromString(Stream.class.getCanonicalName());
/** Instantiates a new {@link FluxImplicitBlock} instance. */
public FluxImplicitBlock() {}

View File

@@ -68,7 +68,7 @@ public final class FormatStringConcatenation extends BugChecker
anyMethod()
.anyClass()
.withAnyName()
.withParameters(String.class.getName(), Throwable.class.getName());
.withParameters(String.class.getCanonicalName(), Throwable.class.getCanonicalName());
// XXX: Drop some of these methods if we use Refaster to replace some with others.
private static final Matcher<ExpressionTree> ASSERTJ_FORMAT_METHOD =

View File

@@ -56,7 +56,7 @@ public final class IdentityConversion extends BugChecker implements MethodInvoca
.map(Class::getName)
.collect(toImmutableSet()))
.named("valueOf"),
staticMethod().onClass(String.class.getName()).named("valueOf"),
staticMethod().onClass(String.class.getCanonicalName()).named("valueOf"),
staticMethod()
.onClassAny(
"com.google.common.collect.ImmutableBiMap",

View File

@@ -99,12 +99,12 @@ public final class JUnitValueSource extends BugChecker implements MethodTreeMatc
allOf(
staticMethod()
.onClassAny(
Stream.class.getName(),
IntStream.class.getName(),
LongStream.class.getName(),
DoubleStream.class.getName(),
List.class.getName(),
Set.class.getName(),
Stream.class.getCanonicalName(),
IntStream.class.getCanonicalName(),
LongStream.class.getCanonicalName(),
DoubleStream.class.getCanonicalName(),
List.class.getCanonicalName(),
Set.class.getCanonicalName(),
"com.google.common.collect.ImmutableList",
"com.google.common.collect.ImmutableSet")
.named("of"),

View File

@@ -50,7 +50,7 @@ public final class MockitoMockClassReference extends BugChecker
private static final long serialVersionUID = 1L;
private static final Matcher<MethodInvocationTree> MOCKITO_MOCK_OR_SPY_WITH_HARDCODED_TYPE =
allOf(
argument(0, allOf(isSameType(Class.class.getName()), not(isVariable()))),
argument(0, allOf(isSameType(Class.class.getCanonicalName()), not(isVariable()))),
staticMethod().onClass("org.mockito.Mockito").namedAnyOf("mock", "spy"));
/** Instantiates a new {@link MockitoMockClassReference} instance. */

View File

@@ -72,7 +72,7 @@ public final class NonEmptyMono extends BugChecker implements MethodInvocationTr
instanceMethod()
.onDescendantOf("reactor.core.publisher.Flux")
.named("reduce")
.withParameters(Object.class.getName(), BiFunction.class.getName()),
.withParameters(Object.class.getCanonicalName(), BiFunction.class.getCanonicalName()),
instanceMethod()
.onDescendantOf("reactor.core.publisher.Mono")
.namedAnyOf("defaultIfEmpty", "hasElement", "single"));

View File

@@ -55,21 +55,21 @@ public final class PrimitiveComparison extends BugChecker implements MethodInvoc
private static final Matcher<ExpressionTree> STATIC_COMPARISON_METHOD =
anyOf(
staticMethod()
.onClass(Comparator.class.getName())
.onClass(Comparator.class.getCanonicalName())
.namedAnyOf("comparingInt", "comparingLong", "comparingDouble"),
staticMethod()
.onClass(Comparator.class.getName())
.onClass(Comparator.class.getCanonicalName())
.named("comparing")
.withParameters(Function.class.getName()));
.withParameters(Function.class.getCanonicalName()));
private static final Matcher<ExpressionTree> INSTANCE_COMPARISON_METHOD =
anyOf(
instanceMethod()
.onDescendantOf(Comparator.class.getName())
.onDescendantOf(Comparator.class.getCanonicalName())
.namedAnyOf("thenComparingInt", "thenComparingLong", "thenComparingDouble"),
instanceMethod()
.onDescendantOf(Comparator.class.getName())
.onDescendantOf(Comparator.class.getCanonicalName())
.named("thenComparing")
.withParameters(Function.class.getName()));
.withParameters(Function.class.getCanonicalName()));
/** Instantiates a new {@link PrimitiveComparison} instance. */
public PrimitiveComparison() {}
@@ -168,7 +168,7 @@ public final class PrimitiveComparison extends BugChecker implements MethodInvoc
switch (expr.getKind()) {
case IDENTIFIER:
return SuggestedFix.builder()
.addStaticImport(Comparator.class.getName() + '.' + preferredMethodName)
.addStaticImport(Comparator.class.getCanonicalName() + '.' + preferredMethodName)
.replace(expr, preferredMethodName)
.build();
case MEMBER_SELECT:

View File

@@ -86,7 +86,7 @@ public final class RedundantStringConversion extends BugChecker
private static final Matcher<MethodInvocationTree> WELL_KNOWN_STRING_CONVERSION_METHODS =
anyOf(
instanceMethod()
.onDescendantOfAny(Object.class.getName())
.onDescendantOfAny(Object.class.getCanonicalName())
.named("toString")
.withNoParameters(),
allOf(
@@ -100,7 +100,7 @@ public final class RedundantStringConversion extends BugChecker
.collect(toImmutableSet()))
.named("toString"),
allOf(
staticMethod().onClass(String.class.getName()).named("valueOf"),
staticMethod().onClass(String.class.getCanonicalName()).named("valueOf"),
not(
anyMethod()
.anyClass()
@@ -109,27 +109,29 @@ public final class RedundantStringConversion extends BugChecker
ImmutableList.of(Suppliers.arrayOf(Suppliers.CHAR_TYPE))))))));
private static final Matcher<ExpressionTree> STRINGBUILDER_APPEND_INVOCATION =
instanceMethod()
.onDescendantOf(StringBuilder.class.getName())
.onDescendantOf(StringBuilder.class.getCanonicalName())
.named("append")
.withParameters(String.class.getName());
.withParameters(String.class.getCanonicalName());
private static final Matcher<ExpressionTree> STRINGBUILDER_INSERT_INVOCATION =
instanceMethod()
.onDescendantOf(StringBuilder.class.getName())
.onDescendantOf(StringBuilder.class.getCanonicalName())
.named("insert")
.withParameters(int.class.getName(), String.class.getName());
.withParameters(int.class.getCanonicalName(), String.class.getCanonicalName());
private static final Matcher<ExpressionTree> FORMATTER_INVOCATION =
anyOf(
staticMethod().onClass(String.class.getName()).named("format"),
instanceMethod().onDescendantOf(Formatter.class.getName()).named("format"),
staticMethod().onClass(String.class.getCanonicalName()).named("format"),
instanceMethod().onDescendantOf(Formatter.class.getCanonicalName()).named("format"),
instanceMethod()
.onDescendantOfAny(PrintStream.class.getName(), PrintWriter.class.getName())
.onDescendantOfAny(
PrintStream.class.getCanonicalName(), PrintWriter.class.getCanonicalName())
.namedAnyOf("format", "printf"),
instanceMethod()
.onDescendantOfAny(PrintStream.class.getName(), PrintWriter.class.getName())
.onDescendantOfAny(
PrintStream.class.getCanonicalName(), PrintWriter.class.getCanonicalName())
.namedAnyOf("print", "println")
.withParameters(Object.class.getName()),
.withParameters(Object.class.getCanonicalName()),
staticMethod()
.onClass(Console.class.getName())
.onClass(Console.class.getCanonicalName())
.namedAnyOf("format", "printf", "readline", "readPassword"));
private static final Matcher<ExpressionTree> GUAVA_GUARD_INVOCATION =
anyOf(

View File

@@ -35,7 +35,7 @@ import tech.picnic.errorprone.bugpatterns.util.SourceCode;
public final class RefasterAnyOfUsage extends BugChecker implements MethodInvocationTreeMatcher {
private static final long serialVersionUID = 1L;
private static final Matcher<ExpressionTree> REFASTER_ANY_OF =
staticMethod().onClass(Refaster.class.getName()).named("anyOf");
staticMethod().onClass(Refaster.class.getCanonicalName()).named("anyOf");
/** Instantiates a new {@link RefasterAnyOfUsage} instance. */
public RefasterAnyOfUsage() {}

View File

@@ -49,7 +49,7 @@ public final class StringJoin extends BugChecker implements MethodInvocationTree
private static final long serialVersionUID = 1L;
private static final Splitter FORMAT_SPECIFIER_SPLITTER = Splitter.on("%s");
private static final Matcher<ExpressionTree> STRING_FORMAT_INVOCATION =
staticMethod().onClass(String.class.getName()).named("format");
staticMethod().onClass(String.class.getCanonicalName()).named("format");
private static final Supplier<Type> CHAR_SEQUENCE_TYPE =
Suppliers.typeFromClass(CharSequence.class);
private static final Supplier<Type> FORMATTABLE_TYPE = Suppliers.typeFromClass(Formattable.class);

View File

@@ -45,11 +45,11 @@ public final class TimeZoneUsage extends BugChecker implements MethodInvocationT
anyOf(
allOf(
instanceMethod()
.onDescendantOf(Clock.class.getName())
.onDescendantOf(Clock.class.getCanonicalName())
.namedAnyOf("getZone", "withZone"),
not(enclosingClass(isSubtypeOf(Clock.class)))),
staticMethod()
.onClass(Clock.class.getName())
.onClass(Clock.class.getCanonicalName())
.namedAnyOf(
"system",
"systemDefaultZone",
@@ -59,14 +59,17 @@ public final class TimeZoneUsage extends BugChecker implements MethodInvocationT
"tickSeconds"),
staticMethod()
.onClassAny(
LocalDate.class.getName(),
LocalDateTime.class.getName(),
LocalTime.class.getName(),
OffsetDateTime.class.getName(),
OffsetTime.class.getName(),
ZonedDateTime.class.getName())
LocalDate.class.getCanonicalName(),
LocalDateTime.class.getCanonicalName(),
LocalTime.class.getCanonicalName(),
OffsetDateTime.class.getCanonicalName(),
OffsetTime.class.getCanonicalName(),
ZonedDateTime.class.getCanonicalName())
.named("now"),
staticMethod().onClassAny(Instant.class.getName()).named("now").withNoParameters());
staticMethod()
.onClassAny(Instant.class.getCanonicalName())
.named("now")
.withNoParameters());
/** Instantiates a new {@link TimeZoneUsage} instance. */
public TimeZoneUsage() {}

View File

@@ -0,0 +1,82 @@
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 CanonicalClassNameUsageTest {
@Test
void identification() {
CompilationTestHelper.newInstance(CanonicalClassNameUsage.class, getClass())
.setArgs(
"--add-exports=jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED",
"--add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED")
.addSourceLines(
"A.java",
"import static com.google.errorprone.matchers.Matchers.instanceMethod;",
"",
"import com.google.errorprone.VisitorState;",
"import tech.picnic.errorprone.bugpatterns.util.MoreTypes;",
"",
"class A {",
" void m(VisitorState state) {",
" String a = A.class.getName();",
" String b = getClass().getName();",
" A.class.getName().toString();",
" System.out.println(A.class.getName());",
" methodInUnnamedPackage(A.class.getName());",
" instanceMethod().onExactClass(A.class.getCanonicalName());",
" MoreTypes.type(A.class.getCanonicalName());",
" MoreTypes.type(A.class.getCanonicalName() + \".SubType\");",
" instanceMethod().onExactClass(new Object() {}.getClass().getName());",
" instanceMethod().onExactClass(methodInUnnamedPackage(A.class.getName()));",
" // BUG: Diagnostic contains:",
" instanceMethod().onExactClass(A.class.getName());",
" // BUG: Diagnostic contains:",
" MoreTypes.type(A.class.getName());",
" // BUG: Diagnostic contains:",
" state.binaryNameFromClassname(A.class.getName() + \".SubType\");",
" }",
"",
" String methodInUnnamedPackage(String str) {",
" return str;",
" }",
"}")
.doTest();
}
@Test
void replacement() {
BugCheckerRefactoringTestHelper.newInstance(CanonicalClassNameUsage.class, getClass())
.addInputLines(
"A.java",
"import static com.google.errorprone.matchers.Matchers.instanceMethod;",
"",
"import com.google.errorprone.BugPattern;",
"import tech.picnic.errorprone.bugpatterns.util.MoreTypes;",
"",
"class A {",
" void m() {",
" instanceMethod().onDescendantOfAny(A.class.getName(), BugPattern.LinkType.class.getName());",
" MoreTypes.type(String.class.getName());",
" }",
"}")
.addOutputLines(
"A.java",
"import static com.google.errorprone.matchers.Matchers.instanceMethod;",
"",
"import com.google.errorprone.BugPattern;",
"import tech.picnic.errorprone.bugpatterns.util.MoreTypes;",
"",
"class A {",
" void m() {",
" instanceMethod()",
" .onDescendantOfAny(",
" A.class.getCanonicalName(), BugPattern.LinkType.class.getCanonicalName());",
" MoreTypes.type(String.class.getCanonicalName());",
" }",
"}")
.doTest(TestMode.TEXT_MATCH);
}
}

View File

@@ -125,7 +125,9 @@ final class MoreMatchersTest {
private static final long serialVersionUID = 1L;
private static final Matcher<Tree> DELEGATE =
MoreMatchers.isSubTypeOf(
generic(type(ImmutableSet.class.getName()), subOf(type(Number.class.getName()))));
generic(
type(ImmutableSet.class.getCanonicalName()),
subOf(type(Number.class.getCanonicalName()))));
@Override
public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {

View File

@@ -74,30 +74,30 @@ public final class IsEmpty implements Matcher<ExpressionTree> {
isSameType(Vector.class));
private static final Matcher<ExpressionTree> EMPTY_INSTANCE_FACTORY =
anyOf(
staticField(Collections.class.getName(), "EMPTY_LIST"),
staticField(Collections.class.getName(), "EMPTY_MAP"),
staticField(Collections.class.getName(), "EMPTY_SET"),
staticField(Collections.class.getCanonicalName(), "EMPTY_LIST"),
staticField(Collections.class.getCanonicalName(), "EMPTY_MAP"),
staticField(Collections.class.getCanonicalName(), "EMPTY_SET"),
toType(
MethodInvocationTree.class,
allOf(
argumentCount(0),
anyOf(
staticMethod()
.onClass(Collections.class.getName())
.onClass(Collections.class.getCanonicalName())
.withNameMatching(EMPTY_INSTANCE_FACTORY_METHOD_PATTERN),
staticMethod()
.onDescendantOfAny(
ImmutableCollection.class.getName(),
ImmutableMap.class.getName(),
ImmutableMultimap.class.getName(),
List.class.getName(),
Map.class.getName(),
Set.class.getName(),
Stream.class.getName())
ImmutableCollection.class.getCanonicalName(),
ImmutableMap.class.getCanonicalName(),
ImmutableMultimap.class.getCanonicalName(),
List.class.getCanonicalName(),
Map.class.getCanonicalName(),
Set.class.getCanonicalName(),
Stream.class.getCanonicalName())
.named("of"),
staticMethod()
.onClassAny(
Stream.class.getName(),
Stream.class.getCanonicalName(),
"reactor.core.publisher.Flux",
"reactor.core.publisher.Mono",
"reactor.util.context.Context")

View File

@@ -25,10 +25,10 @@ public final class IsIdentityOperation implements Matcher<ExpressionTree> {
anyOf(
staticMethod()
.onDescendantOfAny(
DoubleUnaryOperator.class.getName(),
Function.class.getName(),
IntUnaryOperator.class.getName(),
LongUnaryOperator.class.getName())
DoubleUnaryOperator.class.getCanonicalName(),
Function.class.getCanonicalName(),
IntUnaryOperator.class.getCanonicalName(),
LongUnaryOperator.class.getCanonicalName())
.named("identity"),
isIdentityLambdaExpression());

View File

@@ -11,7 +11,7 @@ import com.sun.source.tree.ExpressionTree;
public final class IsRefasterAsVarargs implements Matcher<ExpressionTree> {
private static final long serialVersionUID = 1L;
private static final Matcher<ExpressionTree> DELEGATE =
staticMethod().onClass(Refaster.class.getName()).named("asVarargs");
staticMethod().onClass(Refaster.class.getCanonicalName()).named("asVarargs");
/** Instantiates a new {@link IsRefasterAsVarargs} instance. */
public IsRefasterAsVarargs() {}