Suggestions

error-prone-contrib/pom.xml

@@ -72,6 +72,11 @@
             <artifactId>auto-service-annotations</artifactId>
             <scope>provided</scope>
         </dependency>
+        <dependency>
+            <groupId>com.google.auto.value</groupId>
+            <artifactId>auto-value-annotations</artifactId>
+            <scope>provided</scope>
+        </dependency>
         <dependency>
             <groupId>com.google.googlejavaformat</groupId>
             <artifactId>google-java-format</artifactId>

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/NonStaticImport.java

@@ -3,42 +3,49 @@
 import static com.google.errorprone.BugPattern.LinkType.CUSTOM;
 import static com.google.errorprone.BugPattern.SeverityLevel.SUGGESTION;
 import static com.google.errorprone.BugPattern.StandardTags.STYLE;
+import static tech.picnic.errorprone.bugpatterns.StaticImport.STATIC_IMPORT_CANDIDATE_MEMBERS;
 import static tech.picnic.errorprone.bugpatterns.util.Documentation.BUG_PATTERNS_BASE_URL;
 
 import com.google.auto.service.AutoService;
+import com.google.auto.value.AutoValue;
+import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.ImmutableSetMultimap;
+import com.google.common.collect.ImmutableTable;
 import com.google.errorprone.BugPattern;
 import com.google.errorprone.VisitorState;
 import com.google.errorprone.bugpatterns.BugChecker;
-import com.google.errorprone.bugpatterns.BugChecker.IdentifierTreeMatcher;
+import com.google.errorprone.bugpatterns.BugChecker.CompilationUnitTreeMatcher;
 import com.google.errorprone.fixes.SuggestedFix;
 import com.google.errorprone.fixes.SuggestedFixes;
 import com.google.errorprone.matchers.Description;
 import com.google.errorprone.util.ASTHelpers;
 import com.sun.source.tree.CompilationUnitTree;
 import com.sun.source.tree.IdentifierTree;
 import com.sun.source.tree.ImportTree;
+import com.sun.source.tree.MemberSelectTree;
 import com.sun.source.tree.Tree;
+import com.sun.source.util.TreeScanner;
 import com.sun.tools.javac.code.Symbol;
-import com.sun.tools.javac.code.Symbol.ClassSymbol;
-import com.sun.tools.javac.code.Symbol.TypeSymbol;
+import org.jspecify.annotations.Nullable;
 import tech.picnic.errorprone.bugpatterns.util.SourceCode;
 
 /**
- * A {@link BugChecker} that flags methods and constants that should *not* be statically imported.
+ * A {@link BugChecker} that flags static imports of type members that should *not* be statically
+ * imported.
  */
-// XXX: Also introduce checks that disallows the following candidates:
-// - `ZoneOffset.ofHours` and other `ofXXX`-style methods.
-// - Several other `java.time` classes.
+// XXX: Add suppression support. If qualification of one more more identifiers is suppressed, then
+// the associated static import should *not* be removed.
+// XXX: Also introduce logic that disallows statically importing `ZoneOffset.ofHours` and other
+// `ofXXX`-style methods.
 @AutoService(BugChecker.class)
 @BugPattern(
-    summary = "Identifier should not be statically imported",
+    summary = "Member should not be statically imported",
     link = BUG_PATTERNS_BASE_URL + "NonStaticImport",
     linkType = CUSTOM,
     severity = SUGGESTION,
     tags = STYLE)
-public final class NonStaticImport extends BugChecker implements IdentifierTreeMatcher {
+public final class NonStaticImport extends BugChecker implements CompilationUnitTreeMatcher {
   private static final long serialVersionUID = 1L;
 
   /**
@@ -48,16 +55,21 @@ public final class NonStaticImport extends BugChecker implements IdentifierTreeM
    * <p>Types listed here should be mutually exclusive with {@link
    * StaticImport#STATIC_IMPORT_CANDIDATE_TYPES}.
    */
+  @VisibleForTesting
   static final ImmutableSet<String> NON_STATIC_IMPORT_CANDIDATE_TYPES =
       ImmutableSet.of("com.google.common.base.Strings", "java.time.Clock", "java.time.ZoneOffset");
 
   /**
    * Type members that should never be statically imported.
    *
-   * <p>Identifiers listed by {@link #NON_STATIC_IMPORT_CANDIDATE_IDENTIFIERS} should be omitted
-   * from this collection.
+   * <p>Please note that:
    *
-   * <p>This should be mutually exclusive with {@link StaticImport#STATIC_IMPORT_CANDIDATE_MEMBERS}.
+   * <ul>
+   *   <li>Types listed by {@link #NON_STATIC_IMPORT_CANDIDATE_TYPES} and members listed by {@link
+   *       #NON_STATIC_IMPORT_CANDIDATE_IDENTIFIERS} should be omitted from this collection.
+   *   <li>This collection should be mutually exclusive with {@link
+   *       StaticImport#STATIC_IMPORT_CANDIDATE_MEMBERS}.
+   * </ul>
    */
   // XXX: Perhaps the set of exempted `java.util.Collections` methods is too strict. For now any
   // method name that could be considered "too vague" or could conceivably mean something else in a
@@ -87,11 +99,14 @@ public final class NonStaticImport extends BugChecker implements IdentifierTreeM
   /**
    * Identifiers that should never be statically imported.
    *
-   * <p>Identifiers listed by {@link StaticImport#STATIC_IMPORT_CANDIDATE_MEMBERS} should be
-   * mutually exclusive with identifiers listed here.
+   * <p>Please note that:
    *
-   * <p>This should be a superset of the identifiers flagged by {@link
-   * com.google.errorprone.bugpatterns.BadImport}.
+   * <ul>
+   *   <li>Identifiers listed by {@link StaticImport#STATIC_IMPORT_CANDIDATE_MEMBERS} should be
+   *       mutually exclusive with identifiers listed here.
+   *   <li>This list should contain a superset of the identifiers flagged by {@link
+   *       com.google.errorprone.bugpatterns.BadImport}.
+   * </ul>
    */
   static final ImmutableSet<String> NON_STATIC_IMPORT_CANDIDATE_IDENTIFIERS =
       ImmutableSet.of(
@@ -117,68 +132,80 @@ public final class NonStaticImport extends BugChecker implements IdentifierTreeM
   public NonStaticImport() {}
 
   @Override
-  public Description matchIdentifier(IdentifierTree tree, VisitorState state) {
-    Symbol symbol = ASTHelpers.getSymbol(tree);
-    if (symbol == null || !shouldAddQualifier(symbol, state)) {
-      return Description.NO_MATCH;
-    }
-
-    SuggestedFix.Builder builder = SuggestedFix.builder();
-    String replacement = SuggestedFixes.qualifyType(state, builder, symbol.owner) + ".";
+  public Description matchCompilationUnit(CompilationUnitTree tree, VisitorState state) {
+    ImmutableTable<String, String, UndesiredStaticImport> undesiredStaticImports =
+        getUndesiredStaticImports(tree, state);
 
-    return describeMatch(tree, builder.prefixWith(tree, replacement).build());
-  }
+    if (!undesiredStaticImports.isEmpty()) {
+      replaceUndesiredStaticImportUsages(tree, state, undesiredStaticImports);
 
-  private static boolean shouldAddQualifier(Symbol symbol, VisitorState state) {
-    if (symbol instanceof TypeSymbol) {
-      return false;
-    }
-    String identifierName = symbol.getSimpleName().toString();
-    if (!isIdentifierStaticallyImported(identifierName, state)) {
-      return false;
-    }
-    if (isIdentifierDefinedInFile(symbol, state.getPath().getCompilationUnit())) {
-      return false;
+      for (UndesiredStaticImport staticImport : undesiredStaticImports.values()) {
+        state.reportMatch(
+            describeMatch(staticImport.importTree(), staticImport.fixBuilder().build()));
+      }
     }
 
-    String qualifiedTypeName = symbol.owner.getQualifiedName().toString();
-    return !isStaticImportCandidateMember(qualifiedTypeName, identifierName)
-        && isNonStaticImportCandidate(qualifiedTypeName, identifierName);
+    /* Any violations have been flagged against the offending static import statement. */
+    return Description.NO_MATCH;
   }
 
-  private static boolean isIdentifierDefinedInFile(Symbol symbol, CompilationUnitTree tree) {
-    return tree.getTypeDecls().stream()
-        .anyMatch(
-            t -> {
-              Symbol topLevelClass = ASTHelpers.getSymbol(t);
-              return topLevelClass instanceof ClassSymbol
-                  && symbol.isEnclosedBy((ClassSymbol) topLevelClass);
-            });
-  }
+  private static ImmutableTable<String, String, UndesiredStaticImport> getUndesiredStaticImports(
+      CompilationUnitTree tree, VisitorState state) {
+    ImmutableTable.Builder<String, String, UndesiredStaticImport> imports =
+        ImmutableTable.builder();
+    for (ImportTree importTree : tree.getImports()) {
+      Tree qualifiedIdentifier = importTree.getQualifiedIdentifier();
+      if (importTree.isStatic() && qualifiedIdentifier instanceof MemberSelectTree) {
+        MemberSelectTree memberSelectTree = (MemberSelectTree) qualifiedIdentifier;
+        String type = SourceCode.treeToString(memberSelectTree.getExpression(), state);
+        String member = memberSelectTree.getIdentifier().toString();
+        if (shouldNotBeStaticallyImported(type, member)) {
+          imports.put(
+              type,
+              member,
+              new AutoValue_NonStaticImport_UndesiredStaticImport(
+                  importTree, SuggestedFix.builder().removeStaticImport(type + '.' + member)));
+        }
+      }
+    }
 
-  private static boolean isNonStaticImportCandidate(
-      String qualifiedTypeName, String identifierName) {
-    return NON_STATIC_IMPORT_CANDIDATE_TYPES.contains(qualifiedTypeName)
-        || NON_STATIC_IMPORT_CANDIDATE_MEMBERS.containsEntry(qualifiedTypeName, identifierName)
-        || NON_STATIC_IMPORT_CANDIDATE_IDENTIFIERS.contains(identifierName);
+    return imports.build();
   }
 
-  private static boolean isStaticImportCandidateMember(
-      String qualifiedTypeName, String identifierName) {
-    return StaticImport.STATIC_IMPORT_CANDIDATE_MEMBERS.containsEntry(
-        qualifiedTypeName, identifierName);
+  private static boolean shouldNotBeStaticallyImported(String type, String member) {
+    return (NON_STATIC_IMPORT_CANDIDATE_TYPES.contains(type)
+            && !STATIC_IMPORT_CANDIDATE_MEMBERS.containsEntry(type, member))
+        || NON_STATIC_IMPORT_CANDIDATE_MEMBERS.containsEntry(type, member)
+        || NON_STATIC_IMPORT_CANDIDATE_IDENTIFIERS.contains(member);
   }
 
-  private static boolean isIdentifierStaticallyImported(String identifierName, VisitorState state) {
-    return state.getPath().getCompilationUnit().getImports().stream()
-        .filter(ImportTree::isStatic)
-        .map(ImportTree::getQualifiedIdentifier)
-        .map(tree -> getStaticImportIdentifier(tree, state))
-        .anyMatch(identifierName::contentEquals);
+  private static void replaceUndesiredStaticImportUsages(
+      CompilationUnitTree tree,
+      VisitorState state,
+      ImmutableTable<String, String, UndesiredStaticImport> undesiredStaticImports) {
+    new TreeScanner<@Nullable Void, @Nullable Void>() {
+      @Override
+      public @Nullable Void visitIdentifier(IdentifierTree node, @Nullable Void unused) {
+        Symbol symbol = ASTHelpers.getSymbol(node);
+        if (symbol != null) {
+          UndesiredStaticImport staticImport =
+              undesiredStaticImports.get(
+                  symbol.owner.getQualifiedName().toString(), symbol.name.toString());
+          if (staticImport != null) {
+            SuggestedFix.Builder fix = staticImport.fixBuilder();
+            fix.prefixWith(node, SuggestedFixes.qualifyType(state, fix, symbol.owner) + '.');
+          }
+        }
+
+        return super.visitIdentifier(node, unused);
+      }
+    }.scan(tree, null);
   }
 
-  private static CharSequence getStaticImportIdentifier(Tree tree, VisitorState state) {
-    String source = SourceCode.treeToString(tree, state);
-    return source.subSequence(source.lastIndexOf('.') + 1, source.length());
+  @AutoValue
+  abstract static class UndesiredStaticImport {
+    abstract ImportTree importTree();
+
+    abstract SuggestedFix.Builder fixBuilder();
   }
 }

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/StaticImport.java

@@ -2,13 +2,14 @@
 
 import static com.google.errorprone.BugPattern.LinkType.CUSTOM;
 import static com.google.errorprone.BugPattern.SeverityLevel.SUGGESTION;
-import static com.google.errorprone.BugPattern.StandardTags.SIMPLIFICATION;
+import static com.google.errorprone.BugPattern.StandardTags.STYLE;
 import static java.util.Objects.requireNonNull;
 import static tech.picnic.errorprone.bugpatterns.NonStaticImport.NON_STATIC_IMPORT_CANDIDATE_IDENTIFIERS;
 import static tech.picnic.errorprone.bugpatterns.NonStaticImport.NON_STATIC_IMPORT_CANDIDATE_MEMBERS;
 import static tech.picnic.errorprone.bugpatterns.util.Documentation.BUG_PATTERNS_BASE_URL;
 
 import com.google.auto.service.AutoService;
+import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.ImmutableSetMultimap;
 import com.google.errorprone.BugPattern;
@@ -28,21 +29,19 @@
 import com.sun.tools.javac.code.Type;
 import java.util.Optional;
 
-/**
- * A {@link BugChecker} that flags methods and constants that can and should be statically imported.
- */
+/** A {@link BugChecker} that flags type members that can and should be statically imported. */
 // XXX: Tricky cases:
 // - `org.springframework.http.HttpStatus` (not always an improvement, and `valueOf` must
 //    certainly be excluded)
 // - `com.google.common.collect.Tables`
-// - `ch.qos.logback.classic.Level.{DEBUG, ERROR, INFO, TRACE, WARN"}`
+// - `ch.qos.logback.classic.Level.{DEBUG, ERROR, INFO, TRACE, WARN}`
 @AutoService(BugChecker.class)
 @BugPattern(
     summary = "Identifier should be statically imported",
     link = BUG_PATTERNS_BASE_URL + "StaticImport",
     linkType = CUSTOM,
     severity = SUGGESTION,
-    tags = SIMPLIFICATION)
+    tags = STYLE)
 public final class StaticImport extends BugChecker implements MemberSelectTreeMatcher {
   private static final long serialVersionUID = 1L;
 
@@ -54,6 +53,7 @@ public final class StaticImport extends BugChecker implements MemberSelectTreeMa
    * <p>Types listed here should be mutually exclusive with {@link
    * NonStaticImport#NON_STATIC_IMPORT_CANDIDATE_TYPES}.
    */
+  @VisibleForTesting
   static final ImmutableSet<String> STATIC_IMPORT_CANDIDATE_TYPES =
       ImmutableSet.of(
           "com.google.common.base.Preconditions",
@@ -103,11 +103,16 @@ public final class StaticImport extends BugChecker implements MemberSelectTreeMa
   /**
    * Type members that should be statically imported.
    *
-   * <p>This should be mutually exclusive with {@link
-   * NonStaticImport#NON_STATIC_IMPORT_CANDIDATE_MEMBERS}.
+   * <p>Please note that:
    *
-   * <p>Identifiers listed by {@link NonStaticImport#NON_STATIC_IMPORT_CANDIDATE_IDENTIFIERS} should
-   * be mutually exclusive with identifiers listed here.
+   * <ul>
+   *   <li>Types listed by {@link #STATIC_IMPORT_CANDIDATE_TYPES} should be omitted from this
+   *       collection.
+   *   <li>This collection should be mutually exclusive with {@link
+   *       NonStaticImport#NON_STATIC_IMPORT_CANDIDATE_MEMBERS}.
+   *   <li>This collection should not list members contained in {@link
+   *       NonStaticImport#NON_STATIC_IMPORT_CANDIDATE_IDENTIFIERS}.
+   * </ul>
    */
   static final ImmutableSetMultimap<String, String> STATIC_IMPORT_CANDIDATE_MEMBERS =
       ImmutableSetMultimap.<String, String>builder()