Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve JUnitMethodDeclaration check #406

Merged
merged 3 commits into from
Dec 22, 2022
+155 −32
Merged

Improve JUnitMethodDeclaration check #406

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ee48f48
Improve `JUnitMethodDeclaration` check
Stephan202 Dec 9, 2022
7fa268f
Suggestions
rickie Dec 21, 2022
b353a53
Rename parameter
Stephan202 Dec 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 39 additions & 27 deletions ...rone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclaration.java
View file
Open in desktop
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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the reason for this change? :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More efficient storage and retrieval, just like what we do here.

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())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could signal that it's unused by naming x unused. Not sure if we gain a lot there though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, the scope is very small, so it's clear that the parameter is unused. Perhaps better would be s/x/method/, since it's a predicate over methods.

.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();
}
}
30 changes: 28 additions & 2 deletions error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/JavaKeywords.java
View file
Open in desktop
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);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pitest flags that mutants that remove .skip(1) aren't killed. Without assuming that isUnicodeIdentifierPart matches a superset of isJavaIdentifierStart I can't drop this operation. But that assumption does hold in practice, IIUC, so writing a test for it is hard/impossible. There's also a (minute) performance aspect. TL;DR: I don't see how I can avoid this Pitest warning.

}

/**
* Tells whether the given string is a reserved keyword in the Java language.
*
Expand Down
57 changes: 54 additions & 3 deletions ...-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationTest.java
View file
Open in desktop
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
34 changes: 34 additions & 0 deletions ...prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/JavaKeywordsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return Stream.of(
/* { 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);
}
}