JUnitMethodDeclarationCheck: emit warning instead of SuggestedFix if method name clashes

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

@@ -8,6 +8,7 @@
 import static com.google.errorprone.matchers.Matchers.anyOf;
 import static com.google.errorprone.matchers.Matchers.isType;
 import static java.util.function.Predicate.not;
+import static tech.picnic.errorprone.bugpatterns.JavaKeywords.isReservedKeyword;
 
 import com.google.auto.service.AutoService;
 import com.google.common.collect.ImmutableSet;
@@ -23,10 +24,14 @@
 import com.google.errorprone.predicates.TypePredicate;
 import com.google.errorprone.util.ASTHelpers;
 import com.sun.source.tree.AnnotationTree;
+import com.sun.source.tree.ClassTree;
+import com.sun.source.tree.ImportTree;
 import com.sun.source.tree.MethodTree;
+import com.sun.source.tree.Tree;
 import com.sun.tools.javac.code.Symbol;
 import java.util.Optional;
 import javax.lang.model.element.Modifier;
+import javax.lang.model.element.Name;
 
 /** A {@link BugChecker} which flags non-canonical JUnit method declarations. */
 // XXX: Consider introducing a class-level check which enforces that test classes:
@@ -76,29 +81,101 @@ public Description matchMethod(MethodTree tree, VisitorState state) {
       return Description.NO_MATCH;
     }
 
-    SuggestedFix.Builder builder = SuggestedFix.builder();
+    SuggestedFix.Builder fixBuilder = SuggestedFix.builder();
     SuggestedFixes.removeModifiers(tree.getModifiers(), state, ILLEGAL_MODIFIERS)
-        .ifPresent(builder::merge);
+        .ifPresent(fixBuilder::merge);
 
     if (isTestMethod) {
-      // XXX: In theory this rename could clash with an existing method or static import. In that
-      // case we should emit a warning without a suggested replacement.
-      tryCanonicalizeMethodName(tree, state).ifPresent(builder::merge);
+      suggestTestMethodRenameIfApplicable(tree, fixBuilder, state);
     }
 
-    return builder.isEmpty() ? Description.NO_MATCH : describeMatch(tree, builder.build());
+    return fixBuilder.isEmpty() ? Description.NO_MATCH : describeMatch(tree, fixBuilder.build());
   }
 
-  private static Optional<SuggestedFix> tryCanonicalizeMethodName(
-      MethodTree tree, VisitorState state) {
+  private void suggestTestMethodRenameIfApplicable(
+      MethodTree tree, SuggestedFix.Builder fixBuilder, VisitorState state) {
+    tryCanonicalizeMethodName(tree)
+        .ifPresent(
+            newName ->
+                findMethodRenameBlocker(newName, state)
+                    .ifPresentOrElse(
+                        blocker -> reportMethodRenameBlocker(tree, blocker, state),
+                        () -> fixBuilder.merge(SuggestedFixes.renameMethod(tree, newName, state))));
+  }
+
+  private void reportMethodRenameBlocker(MethodTree tree, String reason, VisitorState state) {
+    state.reportMatch(
+        buildDescription(tree)
+            .setMessage(
+                String.format(
+                    "This method's name should not redundantly start with `%s` (but note that %s)",
+                    TEST_PREFIX, reason))
+            .build());
+  }
+
+  /**
+   * If applicable, returns a human-readable argument against assigning the given name to an
+   * existing method.
+   *
+   * <p>This method implements imperfect heuristics. Things it currently does not consider include
+   * the following:
+   *
+   * <ul>
+   *   <li>Whether the rename would merely introduce a method overload, rather than clashing with an
+   *       existing method declaration.
+   *   <li>Whether the rename would cause a method in a superclass to be overridden.
+   *   <li>Whether the rename would in fact clash with a static import. (It could be that a static
+   *       import of the same name is only referenced from lexical scopes in which the method under
+   *       consideration cannot be referenced directly.)
+   * </ul>
+   */
+  private static Optional<String> findMethodRenameBlocker(String methodName, VisitorState state) {
+    if (isMethodInEnclosingClass(methodName, state)) {
+      return Optional.of(
+          String.format("a method named `%s` already exists in this class", methodName));
+    }
+
+    if (isSimpleNameStaticallyImported(methodName, state)) {
+      return Optional.of(String.format("`%s` is already statically imported", methodName));
+    }
+
+    if (isReservedKeyword(methodName)) {
+      return Optional.of(String.format("`%s` is a reserved keyword", methodName));
+    }
+
+    return Optional.empty();
+  }
+
+  private static boolean isMethodInEnclosingClass(String methodName, VisitorState state) {
+    return state.findEnclosing(ClassTree.class).getMembers().stream()
+        .filter(MethodTree.class::isInstance)
+        .map(MethodTree.class::cast)
+        .map(MethodTree::getName)
+        .map(Name::toString)
+        .anyMatch(methodName::equals);
+  }
+
+  private static boolean isSimpleNameStaticallyImported(String simpleName, VisitorState state) {
+    return state.getPath().getCompilationUnit().getImports().stream()
+        .filter(ImportTree::isStatic)
+        .map(ImportTree::getQualifiedIdentifier)
+        .map(tree -> getStaticImportSimpleName(tree, state))
+        .anyMatch(simpleName::contentEquals);
+  }
+
+  private static CharSequence getStaticImportSimpleName(Tree tree, VisitorState state) {
+    String source = Util.treeToString(tree, state);
+    return source.subSequence(source.lastIndexOf('.') + 1, source.length());
+  }
+
+  private static Optional<String> tryCanonicalizeMethodName(MethodTree tree) {
     return Optional.ofNullable(ASTHelpers.getSymbol(tree))
         .map(sym -> sym.getQualifiedName().toString())
         .filter(name -> name.startsWith(TEST_PREFIX))
         .map(name -> name.substring(TEST_PREFIX.length()))
         .filter(not(String::isEmpty))
         .map(name -> Character.toLowerCase(name.charAt(0)) + name.substring(1))
-        .filter(name -> !Character.isDigit(name.charAt(0)))
-        .map(name -> SuggestedFixes.renameMethod(tree, name, state));
+        .filter(name -> !Character.isDigit(name.charAt(0)));
   }
 
   // XXX: Move to a `MoreMatchers` utility class.

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

@@ -0,0 +1,112 @@
+package tech.picnic.errorprone.bugpatterns;
+
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Sets;
+
+final class JavaKeywords {
+  /**
+   * List of all reserved keywords in the Java language.
+   *
+   * @see <a href="https://docs.oracle.com/javase/specs/jls/se17/html/jls-3.html#jls-3.9">JDK 17 JLS
+   *     section 3.9: Keywords</a>
+   */
+  private static final ImmutableSet<String> RESERVED_KEYWORDS =
+      ImmutableSet.of(
+          "_",
+          "abstract",
+          "assert",
+          "boolean",
+          "break",
+          "byte",
+          "case",
+          "catch",
+          "char",
+          "class",
+          "const",
+          "continue",
+          "default",
+          "do",
+          "double",
+          "else",
+          "enum",
+          "extends",
+          "final",
+          "finally",
+          "float",
+          "for",
+          "goto",
+          "if",
+          "implements",
+          "import",
+          "instanceof",
+          "int",
+          "interface",
+          "long",
+          "native",
+          "new",
+          "package",
+          "private",
+          "protected",
+          "public",
+          "return",
+          "short",
+          "static",
+          "strictfp",
+          "super",
+          "switch",
+          "synchronized",
+          "this",
+          "throw",
+          "throws",
+          "transient",
+          "try",
+          "void",
+          "volatile",
+          "while");
+
+  /**
+   * List of all contextual keywords in the Java language.
+   *
+   * @see <a href="https://docs.oracle.com/javase/specs/jls/se17/html/jls-3.html#jls-3.9">JDK 17 JLS
+   *     section 3.9: Keywords</a>
+   */
+  private static final ImmutableSet<String> CONTEXTUAL_KEYWORDS =
+      ImmutableSet.of(
+          "exports",
+          "module",
+          "non-sealed",
+          "open",
+          "opens",
+          "permits",
+          "provides",
+          "record",
+          "requires",
+          "sealed",
+          "to",
+          "transitive",
+          "uses",
+          "var",
+          "with",
+          "yield");
+
+  /** List of all keywords in the Java language. */
+  private static final ImmutableSet<String> ALL_KEYWORDS =
+      Sets.union(RESERVED_KEYWORDS, CONTEXTUAL_KEYWORDS).immutableCopy();
+
+  private JavaKeywords() {}
+
+  /** Tells whether the given string is a reserved keyword in the Java language. */
+  public static boolean isReservedKeyword(String str) {
+    return RESERVED_KEYWORDS.contains(str);
+  }
+
+  /** Tells whether the given string is a contextual keyword in the Java language. */
+  public static boolean isContextualKeyword(String str) {
+    return CONTEXTUAL_KEYWORDS.contains(str);
+  }
+
+  /** Tells whether the given string is a reserved or contextual keyword in the Java language. */
+  public static boolean isKeyword(String str) {
+    return ALL_KEYWORDS.contains(str);
+  }
+}

error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationCheckTest.java

@@ -16,6 +16,8 @@ void identification() {
     compilationTestHelper
         .addSourceLines(
             "A.java",
+            "import static org.junit.jupiter.params.provider.Arguments.arguments;",
+            "",
             "import org.junit.jupiter.api.AfterAll;",
             "import org.junit.jupiter.api.AfterEach;",
             "import org.junit.jupiter.api.BeforeAll;",
@@ -81,6 +83,14 @@ void identification() {
             "  protected void testNonTestMethod3() {}",
             "  private void testNonTestMethod4() {}",
             "  @Test void test5() {}",
+            "",
+            "  // BUG: Diagnostic contains: (but note that a method named `overload` already exists in this class)",
+            "  @Test void testOverload() {}",
+            "  void overload() {}",
+            "  // BUG: Diagnostic contains: (but note that `arguments` is already statically imported)",
+            "  @Test void testArguments() {}",
+            "  // BUG: Diagnostic contains: (but note that `public` is a reserved keyword)",
+            "  @Test void testPublic() {}",
             "}")
         .addSourceLines(
             "B.java",
@@ -122,6 +132,11 @@ void identification() {
             "  @Override public void testNonTestMethod2() {}",
             "  @Override protected void testNonTestMethod3() {}",
             "  @Override @Test void test5() {}",
+            "",
+            "  @Override @Test void testOverload() {}",
+            "  @Override void overload() {}",
+            "  @Override @Test void testArguments() {}",
+            "  @Override @Test void testPublic() {}",
             "}")
         .doTest();
   }
@@ -131,6 +146,8 @@ void replacement() {
     refactoringTestHelper
         .addInputLines(
             "in/A.java",
+            "import static org.junit.jupiter.params.provider.Arguments.arguments;",
+            "",
             "import org.junit.jupiter.api.AfterAll;",
             "import org.junit.jupiter.api.AfterEach;",
             "import org.junit.jupiter.api.BeforeAll;",
@@ -151,9 +168,16 @@ void replacement() {
             "  @Test public void baz() {}",
             "  @RepeatedTest(2) private void qux() {}",
             "  @ParameterizedTest protected void quux() {}",
+            "",
+            "  @Test public void testOverload() {}",
+            "  void overload() {}",
+            "  @Test protected void testArguments() {}",
+            "  @Test private void testClass() {}",
             "}")
         .addOutputLines(
             "out/A.java",
+            "import static org.junit.jupiter.params.provider.Arguments.arguments;",
+            "",
             "import org.junit.jupiter.api.AfterAll;",
             "import org.junit.jupiter.api.AfterEach;",
             "import org.junit.jupiter.api.BeforeAll;",
@@ -174,6 +198,11 @@ void replacement() {
             "  @Test void baz() {}",
             "  @RepeatedTest(2) void qux() {}",
             "  @ParameterizedTest void quux() {}",
+            "",
+            "  @Test void testOverload() {}",
+            "  void overload() {}",
+            "  @Test void testArguments() {}",
+            "  @Test void testClass() {}",
             "}")
         .doTest(TestMode.TEXT_MATCH);
   }