Skip to content

Commit

Permalink
Improve JUnitMethodDeclaration check
Browse files Browse the repository at this point in the history
Implemented changes:
- Ignore method overrides even if not annotated `@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`.
  • Loading branch information
Stephan202 committed Dec 11, 2022
1 parent 0153c14 commit 33ac331
Show file tree
Hide file tree
Showing 4 changed files with 153 additions and 32 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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. */
Expand All @@ -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;
}

Expand All @@ -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))));
Expand Down Expand Up @@ -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), x -> true, clazz, state.getTypes())
.findAny()
.isPresent();
}

private static boolean isSimpleNameStaticallyImported(String simpleName, VisitorState state) {
return state.getPath().getCompilationUnit().getImports().stream()
.filter(ImportTree::isStatic)
Expand All @@ -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();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*
Expand Down Expand Up @@ -64,7 +76,6 @@ public final class JavaKeywords {
"void",
"volatile",
"while");

/**
* List of all contextual keywords in the Java language.
*
Expand All @@ -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.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,9 @@ void identification() {
" private void tearDown8() {}",
"",
" @Test",
" void test() {}",
"",
" @Test",
" void method1() {}",
"",
" @Test",
Expand Down Expand Up @@ -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() {}",
Expand All @@ -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",
Expand Down Expand Up @@ -218,6 +238,10 @@ void identification() {
"",
" @Override",
" @Test",
" void test() {}",
"",
" @Override",
" @Test",
" void method1() {}",
"",
" @Override",
Expand Down Expand Up @@ -267,6 +291,10 @@ void identification() {
"",
" @Override",
" @Test",
" void testToString() {}",
"",
" @Override",
" @Test",
" void testOverload() {}",
"",
" @Override",
Expand All @@ -279,6 +307,17 @@ void identification() {
" @Override",
" @Test",
" void testPublic() {}",
"",
" @Override",
" @Test",
" void testNull() {}",
"",
" @Override",
" @Test",
" void testRecord() {}",
"",
" @Test",
" void testMethodThatIsOverriddenWithoutOverrideAnnotation() {}",
"}")
.addSourceLines(
"C.java",
Expand Down Expand Up @@ -352,6 +391,9 @@ void replacement() {
" protected void quux() {}",
"",
" @Test",
" public void testToString() {}",
"",
" @Test",
" public void testOverload() {}",
"",
" void overload() {}",
Expand All @@ -361,6 +403,9 @@ void replacement() {
"",
" @Test",
" private void testClass() {}",
"",
" @Test",
" private void testTrue() {}",
"}")
.addOutputLines(
"A.java",
Expand Down Expand Up @@ -407,6 +452,9 @@ void replacement() {
" void quux() {}",
"",
" @Test",
" void testToString() {}",
"",
" @Test",
" void testOverload() {}",
"",
" void overload() {}",
Expand All @@ -416,6 +464,9 @@ void replacement() {
"",
" @Test",
" void testClass() {}",
"",
" @Test",
" void testTrue() {}",
"}")
.doTest(TestMode.TEXT_MATCH);
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
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() {
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("test", true));
}

@MethodSource("isValidIdentifierTestCases")
@ParameterizedTest
void isValidIdentifier(String str, boolean expected) {
assertThat(JavaKeywords.isValidIdentifier(str)).isEqualTo(expected);
}
}

0 comments on commit 33ac331

Please sign in to comment.