Improve JUnitMethodDeclaration check (#406)

Implemented changes:
- Ignore method overrides even if not annotated with `@Override`.
- Don't rename methods to `true`, `false` or `null`.
- Don't rename methods to a name declared in a super type. This
  prevents e.g. renaming `testToString` to `toString`.

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

@@ -3,21 +3,19 @@
 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.matchers.ChildMultiMatcher.MatchType.AT_LEAST_ONE;
 import static com.google.errorprone.matchers.Matchers.allOf;
-import static com.google.errorprone.matchers.Matchers.annotations;
-import static com.google.errorprone.matchers.Matchers.anyOf;
 import static com.google.errorprone.matchers.Matchers.enclosingClass;
 import static com.google.errorprone.matchers.Matchers.hasModifier;
-import static com.google.errorprone.matchers.Matchers.isType;
+import static com.google.errorprone.matchers.Matchers.not;
 import static java.util.function.Predicate.not;
 import static tech.picnic.errorprone.bugpatterns.util.Documentation.BUG_PATTERNS_BASE_URL;
-import static tech.picnic.errorprone.bugpatterns.util.JavaKeywords.isReservedKeyword;
+import static tech.picnic.errorprone.bugpatterns.util.JavaKeywords.isValidIdentifier;
 import static tech.picnic.errorprone.bugpatterns.util.MoreJUnitMatchers.SETUP_OR_TEARDOWN_METHOD;
 import static tech.picnic.errorprone.bugpatterns.util.MoreJUnitMatchers.TEST_METHOD;
 
 import com.google.auto.service.AutoService;
 import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Sets;
 import com.google.errorprone.BugPattern;
 import com.google.errorprone.VisitorState;
 import com.google.errorprone.bugpatterns.BugChecker;
@@ -26,14 +24,15 @@
 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.sun.source.tree.ImportTree;
 import com.sun.source.tree.MethodTree;
 import com.sun.source.tree.Tree;
+import com.sun.tools.javac.code.Symbol;
+import com.sun.tools.javac.code.Symbol.MethodSymbol;
+import com.sun.tools.javac.code.Type;
 import java.util.Optional;
 import javax.lang.model.element.Modifier;
-import tech.picnic.errorprone.bugpatterns.util.MoreASTHelpers;
 import tech.picnic.errorprone.bugpatterns.util.SourceCode;
 
 /** A {@link BugChecker} that flags non-canonical JUnit method declarations. */
@@ -53,21 +52,19 @@ public final class JUnitMethodDeclaration extends BugChecker implements MethodTr
   private static final long serialVersionUID = 1L;
   private static final String TEST_PREFIX = "test";
   private static final ImmutableSet<Modifier> ILLEGAL_MODIFIERS =
-      ImmutableSet.of(Modifier.PRIVATE, Modifier.PROTECTED, Modifier.PUBLIC);
-  private static final Matcher<MethodTree> HAS_UNMODIFIABLE_SIGNATURE =
-      anyOf(
-          annotations(AT_LEAST_ONE, isType("java.lang.Override")),
-          allOf(
-              Matchers.not(hasModifier(Modifier.FINAL)),
-              Matchers.not(hasModifier(Modifier.PRIVATE)),
-              enclosingClass(hasModifier(Modifier.ABSTRACT))));
+      Sets.immutableEnumSet(Modifier.PRIVATE, Modifier.PROTECTED, Modifier.PUBLIC);
+  private static final Matcher<MethodTree> IS_LIKELY_OVERRIDDEN =
+      allOf(
+          not(hasModifier(Modifier.FINAL)),
+          not(hasModifier(Modifier.PRIVATE)),
+          enclosingClass(hasModifier(Modifier.ABSTRACT)));
 
   /** Instantiates a new {@link JUnitMethodDeclaration} instance. */
   public JUnitMethodDeclaration() {}
 
   @Override
   public Description matchMethod(MethodTree tree, VisitorState state) {
-    if (HAS_UNMODIFIABLE_SIGNATURE.matches(tree, state)) {
+    if (IS_LIKELY_OVERRIDDEN.matches(tree, state) || isOverride(tree, state)) {
       return Description.NO_MATCH;
     }
 
@@ -89,10 +86,11 @@ public Description matchMethod(MethodTree tree, VisitorState state) {
 
   private void suggestTestMethodRenameIfApplicable(
       MethodTree tree, SuggestedFix.Builder fixBuilder, VisitorState state) {
-    tryCanonicalizeMethodName(tree)
+    MethodSymbol symbol = ASTHelpers.getSymbol(tree);
+    tryCanonicalizeMethodName(symbol)
         .ifPresent(
             newName ->
-                findMethodRenameBlocker(newName, state)
+                findMethodRenameBlocker(symbol, newName, state)
                     .ifPresentOrElse(
                         blocker -> reportMethodRenameBlocker(tree, blocker, state),
                         () -> fixBuilder.merge(SuggestedFixes.renameMethod(tree, newName, state))));
@@ -124,23 +122,31 @@ private void reportMethodRenameBlocker(MethodTree tree, String reason, VisitorSt
    *       consideration cannot be referenced directly.)
    * </ul>
    */
-  private static Optional<String> findMethodRenameBlocker(String methodName, VisitorState state) {
-    if (MoreASTHelpers.methodExistsInEnclosingClass(methodName, state)) {
+  private static Optional<String> findMethodRenameBlocker(
+      MethodSymbol method, String newName, VisitorState state) {
+    if (isExistingMethodName(method.owner.type, newName, state)) {
       return Optional.of(
-          String.format("a method named `%s` already exists in this class", methodName));
+          String.format(
+              "a method named `%s` is already defined in this class or a supertype", newName));
     }
 
-    if (isSimpleNameStaticallyImported(methodName, state)) {
-      return Optional.of(String.format("`%s` is already statically imported", methodName));
+    if (isSimpleNameStaticallyImported(newName, state)) {
+      return Optional.of(String.format("`%s` is already statically imported", newName));
     }
 
-    if (isReservedKeyword(methodName)) {
-      return Optional.of(String.format("`%s` is a reserved keyword", methodName));
+    if (!isValidIdentifier(newName)) {
+      return Optional.of(String.format("`%s` is not a valid identifier", newName));
     }
 
     return Optional.empty();
   }
 
+  private static boolean isExistingMethodName(Type clazz, String name, VisitorState state) {
+    return ASTHelpers.matchingMethods(state.getName(name), method -> true, clazz, state.getTypes())
+        .findAny()
+        .isPresent();
+  }
+
   private static boolean isSimpleNameStaticallyImported(String simpleName, VisitorState state) {
     return state.getPath().getCompilationUnit().getImports().stream()
         .filter(ImportTree::isStatic)
@@ -154,12 +160,18 @@ private static CharSequence getStaticImportSimpleName(Tree tree, VisitorState st
     return source.subSequence(source.lastIndexOf('.') + 1, source.length());
   }
 
-  private static Optional<String> tryCanonicalizeMethodName(MethodTree tree) {
-    return Optional.of(ASTHelpers.getSymbol(tree).getQualifiedName().toString())
+  private static Optional<String> tryCanonicalizeMethodName(Symbol symbol) {
+    return Optional.of(symbol.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)));
   }
+
+  private static boolean isOverride(MethodTree tree, VisitorState state) {
+    return ASTHelpers.streamSuperMethods(ASTHelpers.getSymbol(tree), state.getTypes())
+        .findAny()
+        .isPresent();
+  }
 }

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

@@ -4,7 +4,19 @@
 import com.google.common.collect.Sets;
 
 /** Utility class that can be used to identify reserved keywords of the Java language. */
+// XXX: This class is no longer only about keywords. Consider changing its name and class-level
+// documentation.
 public final class JavaKeywords {
+  /**
+   * Enumeration of boolean and null literals.
+   *
+   * @see <a href="https://docs.oracle.com/javase/specs/jls/se17/html/jls-3.html#jls-3.10.3">JDK 17
+   *     JLS section 3.10.3: Boolean Literals</a>
+   * @see <a href="https://docs.oracle.com/javase/specs/jls/se17/html/jls-3.html#jls-3.10.8">JDK 17
+   *     JLS section 3.10.8: The Null Literal</a>
+   */
+  private static final ImmutableSet<String> BOOLEAN_AND_NULL_LITERALS =
+      ImmutableSet.of("true", "false", "null");
   /**
    * List of all reserved keywords in the Java language.
    *
@@ -64,7 +76,6 @@ public final class JavaKeywords {
           "void",
           "volatile",
           "while");
-
   /**
    * List of all contextual keywords in the Java language.
    *
@@ -89,13 +100,28 @@ public final class JavaKeywords {
           "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 valid identifier in the Java language.
+   *
+   * @param str The string of interest.
+   * @return {@code true} if the given string is a valid identifier in the Java language.
+   * @see <a href="https://docs.oracle.com/javase/specs/jls/se17/html/jls-3.html#jls-3.8">JDK 17 JLS
+   *     section 3.8: Identifiers</a>
+   */
+  public static boolean isValidIdentifier(String str) {
+    return !str.isEmpty()
+        && !isReservedKeyword(str)
+        && !BOOLEAN_AND_NULL_LITERALS.contains(str)
+        && Character.isJavaIdentifierStart(str.codePointAt(0))
+        && str.codePoints().skip(1).allMatch(Character::isUnicodeIdentifierPart);
+  }
+
   /**
    * Tells whether the given string is a reserved keyword in the Java language.
    *

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

@@ -91,6 +91,9 @@ void identification() {
             "  private void tearDown8() {}",
             "",
             "  @Test",
+            "  void test() {}",
+            "",
+            "  @Test",
             "  void method1() {}",
             "",
             "  @Test",
@@ -144,8 +147,13 @@ void identification() {
             "  void test5() {}",
             "",
             "  @Test",
-            "  // BUG: Diagnostic contains: (but note that a method named `overload` already exists in this",
-            "  // class)",
+            "  // BUG: Diagnostic contains: (but note that a method named `toString` is already defined in this",
+            "  // class or a supertype)",
+            "  void testToString() {}",
+            "",
+            "  @Test",
+            "  // BUG: Diagnostic contains: (but note that a method named `overload` is already defined in this",
+            "  // class or a supertype)",
             "  void testOverload() {}",
             "",
             "  void overload() {}",
@@ -155,8 +163,20 @@ void identification() {
             "  void testArguments() {}",
             "",
             "  @Test",
-            "  // BUG: Diagnostic contains: (but note that `public` is a reserved keyword)",
+            "  // BUG: Diagnostic contains: (but note that `public` is not a valid identifier)",
             "  void testPublic() {}",
+            "",
+            "  @Test",
+            "  // BUG: Diagnostic contains: (but note that `null` is not a valid identifier)",
+            "  void testNull() {}",
+            "",
+            "  @Test",
+            "  // BUG: Diagnostic contains:",
+            "  void testRecord() {}",
+            "",
+            "  @Test",
+            "  // BUG: Diagnostic contains:",
+            "  void testMethodThatIsOverriddenWithoutOverrideAnnotation() {}",
             "}")
         .addSourceLines(
             "B.java",
@@ -218,6 +238,10 @@ void identification() {
             "",
             "  @Override",
             "  @Test",
+            "  void test() {}",
+            "",
+            "  @Override",
+            "  @Test",
             "  void method1() {}",
             "",
             "  @Override",
@@ -267,6 +291,10 @@ void identification() {
             "",
             "  @Override",
             "  @Test",
+            "  void testToString() {}",
+            "",
+            "  @Override",
+            "  @Test",
             "  void testOverload() {}",
             "",
             "  @Override",
@@ -279,6 +307,17 @@ void identification() {
             "  @Override",
             "  @Test",
             "  void testPublic() {}",
+            "",
+            "  @Override",
+            "  @Test",
+            "  void testNull() {}",
+            "",
+            "  @Override",
+            "  @Test",
+            "  void testRecord() {}",
+            "",
+            "  @Test",
+            "  void testMethodThatIsOverriddenWithoutOverrideAnnotation() {}",
             "}")
         .addSourceLines(
             "C.java",
@@ -352,6 +391,9 @@ void replacement() {
             "  protected void quux() {}",
             "",
             "  @Test",
+            "  public void testToString() {}",
+            "",
+            "  @Test",
             "  public void testOverload() {}",
             "",
             "  void overload() {}",
@@ -361,6 +403,9 @@ void replacement() {
             "",
             "  @Test",
             "  private void testClass() {}",
+            "",
+            "  @Test",
+            "  private void testTrue() {}",
             "}")
         .addOutputLines(
             "A.java",
@@ -407,6 +452,9 @@ void replacement() {
             "  void quux() {}",
             "",
             "  @Test",
+            "  void testToString() {}",
+            "",
+            "  @Test",
             "  void testOverload() {}",
             "",
             "  void overload() {}",
@@ -416,6 +464,9 @@ void replacement() {
             "",
             "  @Test",
             "  void testClass() {}",
+            "",
+            "  @Test",
+            "  void testTrue() {}",
             "}")
         .doTest(TestMode.TEXT_MATCH);
   }

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

@@ -0,0 +1,34 @@
+package tech.picnic.errorprone.bugpatterns.util;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.junit.jupiter.params.provider.Arguments.arguments;
+
+import java.util.stream.Stream;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
+
+final class JavaKeywordsTest {
+  private static Stream<Arguments> isValidIdentifierTestCases() {
+    /* { str, expected } */
+    return Stream.of(
+        arguments("", false),
+        arguments("public", false),
+        arguments("true", false),
+        arguments("false", false),
+        arguments("null", false),
+        arguments("0", false),
+        arguments("\0", false),
+        arguments("a%\0", false),
+        arguments("a", true),
+        arguments("a0", true),
+        arguments("_a0", true),
+        arguments("test", true));
+  }
+
+  @MethodSource("isValidIdentifierTestCases")
+  @ParameterizedTest
+  void isValidIdentifier(String str, boolean expected) {
+    assertThat(JavaKeywords.isValidIdentifier(str)).isEqualTo(expected);
+  }
+}