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.
PicnicSupermarket · Dec 18, 2023 · 626246b · 626246b
1 parent 22272d6
commit 626246b
Show file tree

Hide file tree

Showing 20 changed files with 247 additions and 62 deletions.
diff --git a/...one-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/CanonicalClassNameUsage.java b/...one-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/CanonicalClassNameUsage.java
@@ -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();
+  }
+}
diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/DirectReturn.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/DirectReturn.java
@@ -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. */

diff --git a/...-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ExplicitEnumOrdering.java b/...-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ExplicitEnumOrdering.java
@@ -44,7 +44,7 @@
 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() {}

diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/FluxFlatMapUsage.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/FluxFlatMapUsage.java
@@ -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()))));

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
@@ -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() {}

diff --git a/...e-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/FormatStringConcatenation.java b/...e-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/FormatStringConcatenation.java
@@ -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 =

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
@@ -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",

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
@@ -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"),

diff --git a/...e-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MockitoMockClassReference.java b/...e-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MockitoMockClassReference.java
@@ -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. */

diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/NonEmptyMono.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/NonEmptyMono.java
@@ -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"));

diff --git a/...r-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/PrimitiveComparison.java b/...r-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/PrimitiveComparison.java
@@ -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 @@ private static Fix suggestFix(
     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:

diff --git a/...e-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RedundantStringConversion.java b/...e-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RedundantStringConversion.java
@@ -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(

diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RefasterAnyOfUsage.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RefasterAnyOfUsage.java
@@ -35,7 +35,7 @@
 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() {}

diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/StringJoin.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/StringJoin.java
@@ -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);

diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/TimeZoneUsage.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/TimeZoneUsage.java
@@ -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() {}