diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclaration.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclaration.java index 5b9a67388e..20afd2fc96 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclaration.java +++ b/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 ILLEGAL_MODIFIERS = - ImmutableSet.of(Modifier.PRIVATE, Modifier.PROTECTED, Modifier.PUBLIC); - private static final Matcher 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 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.) * */ - private static Optional findMethodRenameBlocker(String methodName, VisitorState state) { - if (MoreASTHelpers.methodExistsInEnclosingClass(methodName, state)) { + private static Optional 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 tryCanonicalizeMethodName(MethodTree tree) { - return Optional.of(ASTHelpers.getSymbol(tree).getQualifiedName().toString()) + private static Optional 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(); + } } diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/JavaKeywords.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/JavaKeywords.java index afb7a2985c..e9e70515c8 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/JavaKeywords.java +++ b/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 JDK 17 + * JLS section 3.10.3: Boolean Literals + * @see JDK 17 + * JLS section 3.10.8: The Null Literal + */ + private static final ImmutableSet 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 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 JDK 17 JLS + * section 3.8: Identifiers + */ + 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. * diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationTest.java index 2320daf7c9..92f2e16b8f 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationTest.java +++ b/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); } diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/JavaKeywordsTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/JavaKeywordsTest.java new file mode 100644 index 0000000000..4f4b6a8664 --- /dev/null +++ b/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 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); + } +}