From 56ac6ad19760727b927bff5dba3b3de0d578ed8b Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Sat, 15 Jan 2022 11:54:20 +0100 Subject: [PATCH 1/9] `JUnitMethodDecl`: emit warning instead of `SuggestedFix` if name clashes --- .../JUnitMethodDeclarationCheck.java | 79 ++++++++++++++-- .../JUnitMethodDeclarationCheckTest.java | 90 +++++++++++++++++++ 2 files changed, 162 insertions(+), 7 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationCheck.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationCheck.java index 7a3f9b3d3b..89c9a81830 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationCheck.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationCheck.java @@ -23,10 +23,16 @@ 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.CompilationUnitTree; +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.Objects; 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: @@ -81,24 +87,83 @@ public Description matchMethod(MethodTree tree, VisitorState state) { .ifPresent(builder::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); + String newMethodName = tryCanonicalizeMethodName(tree).orElse(""); + + if (newMethodName.isEmpty()) { + return describeMatchesIfPresent(tree, builder); + } + + boolean reportedNameClash = reportDescriptionForPossibleNameClash(tree, newMethodName, state); + if (!reportedNameClash) { + builder.merge(SuggestedFixes.renameMethod(tree, newMethodName, state)); + } } + return describeMatchesIfPresent(tree, builder); + } + private Description describeMatchesIfPresent(MethodTree tree, SuggestedFix.Builder builder) { return builder.isEmpty() ? Description.NO_MATCH : describeMatch(tree, builder.build()); } - private static Optional tryCanonicalizeMethodName( - MethodTree tree, VisitorState state) { + private boolean reportDescriptionForPossibleNameClash( + MethodTree tree, String methodName, VisitorState state) { + if (isMethodNameInClass(methodName, state)) { + state.reportMatch( + buildDescription(tree) + .setMessage( + String.format("A method with name %s already exists in the class.", methodName)) + .build()); + return true; + } + + if (isMethodNameStaticallyImported(methodName, state)) { + state.reportMatch( + buildDescription(tree) + .setMessage( + String.format( + "A method with name %s is already statically imported.", methodName)) + .build()); + return true; + } + + return false; + } + + private static boolean isMethodNameInClass(String methodName, VisitorState state) { + return state.findEnclosing(ClassTree.class).getMembers().stream() + .filter(member -> !ASTHelpers.isGeneratedConstructor((MethodTree) member)) + .map(MethodTree.class::cast) + .map(MethodTree::getName) + .map(Name::toString) + .anyMatch(n -> n.equals(methodName)); + } + + private static boolean isMethodNameStaticallyImported(String methodName, VisitorState state) { + CompilationUnitTree compilationUnit = state.getPath().getCompilationUnit(); + + return compilationUnit.getImports().stream() + .filter(Objects::nonNull) + .map(ImportTree.class::cast) + .filter(ImportTree::isStatic) + .map(ImportTree::getQualifiedIdentifier) + .map(tree -> getStaticImportIdentifier(tree, state)) + .anyMatch(methodName::contentEquals); + } + + private static CharSequence getStaticImportIdentifier(Tree tree, VisitorState state) { + String source = Util.treeToString(tree, state); + int lastDot = source.lastIndexOf('.'); + return lastDot < 0 ? source : source.subSequence(lastDot + 1, source.length()); + } + + private static Optional 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. diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationCheckTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationCheckTest.java index 8188888710..d2faf47ee5 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationCheckTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationCheckTest.java @@ -177,4 +177,94 @@ void replacement() { "}") .doTest(TestMode.TEXT_MATCH); } + + @Test + void methodAlreadyExistsInClass() { + refactoringTestHelper + .addInputLines( + "A.java", + "import org.junit.jupiter.api.Test;", + "", + "class A {", + " @Test void testFoo() {}", + " void foo() {}", + "", + " @Test void testBar() {}", + " private void bar() {}", + "", + " @Test void testFooDifferent() {}", + " @Test void testBarDifferent() {}", + "}") + .addOutputLines( + "A.java", + "import org.junit.jupiter.api.Test;", + "", + "class A {", + " @Test void testFoo() {}", + " void foo() {}", + "", + " @Test void testBar() {}", + " private void bar() {}", + "", + " @Test void fooDifferent() {}", + " @Test void barDifferent() {}", + "}") + .doTest(TestMode.TEXT_MATCH); + } + + @Test + void methodAlreadyInStaticImports() { + refactoringTestHelper + .addInputLines( + "A.java", + "import static com.google.common.collect.ImmutableSet.toImmutableSet;", + "import static org.junit.jupiter.params.provider.Arguments.arguments;", + "", + "import org.junit.jupiter.api.Test;", + "import com.google.common.collect.ImmutableSet;", + "", + "class A {", + " @Test", + " void testArguments() {", + " arguments(1, 2, 3);", + " }", + "", + " @Test", + " void testToImmutableSet() {", + " ImmutableSet.of(1).stream().filter(i -> i > 1).collect(toImmutableSet());", + " }", + "", + " @Test", + " void testArgumentsDifferentName() {}", + "", + " @Test", + " void testToImmutableSetDifferentName() {}", + "}") + .addOutputLines( + "A.java", + "import static com.google.common.collect.ImmutableSet.toImmutableSet;", + "import static org.junit.jupiter.params.provider.Arguments.arguments;", + "", + "import org.junit.jupiter.api.Test;", + "import com.google.common.collect.ImmutableSet;", + "", + "class A {", + " @Test", + " void testArguments() {", + " arguments(1, 2, 3);", + " }", + "", + " @Test", + " void testToImmutableSet() {", + " ImmutableSet.of(1).stream().filter(i -> i > 1).collect(toImmutableSet());", + " }", + "", + " @Test", + " void argumentsDifferentName() {}", + "", + " @Test", + " void toImmutableSetDifferentName() {}", + "}") + .doTest(TestMode.TEXT_MATCH); + } } From 302fb0121d1983c2197a382900addbbe75c84484 Mon Sep 17 00:00:00 2001 From: Phil Werli Date: Mon, 24 Jan 2022 21:00:43 +0100 Subject: [PATCH 2/9] Suggestions --- .../bugpatterns/JUnitMethodDeclarationCheck.java | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationCheck.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationCheck.java index 89c9a81830..2320608fd9 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationCheck.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationCheck.java @@ -87,15 +87,16 @@ public Description matchMethod(MethodTree tree, VisitorState state) { .ifPresent(builder::merge); if (isTestMethod) { - String newMethodName = tryCanonicalizeMethodName(tree).orElse(""); + Optional newMethodName = tryCanonicalizeMethodName(tree); if (newMethodName.isEmpty()) { return describeMatchesIfPresent(tree, builder); } - boolean reportedNameClash = reportDescriptionForPossibleNameClash(tree, newMethodName, state); + boolean reportedNameClash = + reportDescriptionForPossibleNameClash(tree, newMethodName.orElseThrow(), state); if (!reportedNameClash) { - builder.merge(SuggestedFixes.renameMethod(tree, newMethodName, state)); + builder.merge(SuggestedFixes.renameMethod(tree, newMethodName.orElseThrow(), state)); } } return describeMatchesIfPresent(tree, builder); @@ -131,11 +132,12 @@ private boolean reportDescriptionForPossibleNameClash( private static boolean isMethodNameInClass(String methodName, VisitorState state) { return state.findEnclosing(ClassTree.class).getMembers().stream() - .filter(member -> !ASTHelpers.isGeneratedConstructor((MethodTree) member)) + .filter(MethodTree.class::isInstance) .map(MethodTree.class::cast) + .filter(member -> !ASTHelpers.isGeneratedConstructor(member)) .map(MethodTree::getName) .map(Name::toString) - .anyMatch(n -> n.equals(methodName)); + .anyMatch(methodName::equals); } private static boolean isMethodNameStaticallyImported(String methodName, VisitorState state) { @@ -143,7 +145,6 @@ private static boolean isMethodNameStaticallyImported(String methodName, Visitor return compilationUnit.getImports().stream() .filter(Objects::nonNull) - .map(ImportTree.class::cast) .filter(ImportTree::isStatic) .map(ImportTree::getQualifiedIdentifier) .map(tree -> getStaticImportIdentifier(tree, state)) From 610cce7be06496739b1256e9bd7285b4c0b685a2 Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Wed, 26 Jan 2022 13:51:03 +0100 Subject: [PATCH 3/9] Tweaks --- .../errorprone/bugpatterns/JUnitMethodDeclarationCheck.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationCheck.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationCheck.java index 2320608fd9..dffaa34411 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationCheck.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationCheck.java @@ -153,8 +153,7 @@ private static boolean isMethodNameStaticallyImported(String methodName, Visitor private static CharSequence getStaticImportIdentifier(Tree tree, VisitorState state) { String source = Util.treeToString(tree, state); - int lastDot = source.lastIndexOf('.'); - return lastDot < 0 ? source : source.subSequence(lastDot + 1, source.length()); + return source.subSequence(source.lastIndexOf('.') + 1, source.length()); } private static Optional tryCanonicalizeMethodName(MethodTree tree) { From 8eed4f3e1e046347e67a3781d7fa2e7ac385f643 Mon Sep 17 00:00:00 2001 From: Phil Werli Date: Tue, 15 Feb 2022 13:42:43 +0100 Subject: [PATCH 4/9] Suggestions --- .../JUnitMethodDeclarationCheck.java | 21 +++++-------------- 1 file changed, 5 insertions(+), 16 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationCheck.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationCheck.java index dffaa34411..04f94fded1 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationCheck.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationCheck.java @@ -87,22 +87,11 @@ public Description matchMethod(MethodTree tree, VisitorState state) { .ifPresent(builder::merge); if (isTestMethod) { - Optional newMethodName = tryCanonicalizeMethodName(tree); - - if (newMethodName.isEmpty()) { - return describeMatchesIfPresent(tree, builder); - } - - boolean reportedNameClash = - reportDescriptionForPossibleNameClash(tree, newMethodName.orElseThrow(), state); - if (!reportedNameClash) { - builder.merge(SuggestedFixes.renameMethod(tree, newMethodName.orElseThrow(), state)); - } + tryCanonicalizeMethodName(tree) + .filter(methodName -> !reportDescriptionForPossibleNameClash(tree, methodName, state)) + .ifPresent( + methodName -> builder.merge(SuggestedFixes.renameMethod(tree, methodName, state))); } - return describeMatchesIfPresent(tree, builder); - } - - private Description describeMatchesIfPresent(MethodTree tree, SuggestedFix.Builder builder) { return builder.isEmpty() ? Description.NO_MATCH : describeMatch(tree, builder.build()); } @@ -134,7 +123,7 @@ private static boolean isMethodNameInClass(String methodName, VisitorState state return state.findEnclosing(ClassTree.class).getMembers().stream() .filter(MethodTree.class::isInstance) .map(MethodTree.class::cast) - .filter(member -> !ASTHelpers.isGeneratedConstructor(member)) + .filter(not(ASTHelpers::isGeneratedConstructor)) .map(MethodTree::getName) .map(Name::toString) .anyMatch(methodName::equals); From 41d9692bf3b080d8661798de447abab0f168dd74 Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Wed, 16 Feb 2022 11:41:24 +0100 Subject: [PATCH 5/9] JUnitMethodDeclaration: Prevent renaming methods to Java keywords --- .../JUnitMethodDeclarationCheck.java | 41 ++++++---- .../errorprone/bugpatterns/JavaKeywords.java | 77 +++++++++++++++++++ .../JUnitMethodDeclarationCheckTest.java | 40 ++++++++++ 3 files changed, 141 insertions(+), 17 deletions(-) create mode 100644 error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JavaKeywords.java diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationCheck.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationCheck.java index 04f94fded1..cd16cdf681 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationCheck.java +++ b/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.isJavaKeyword; import com.google.auto.service.AutoService; import com.google.common.collect.ImmutableSet; @@ -88,35 +89,41 @@ public Description matchMethod(MethodTree tree, VisitorState state) { if (isTestMethod) { tryCanonicalizeMethodName(tree) - .filter(methodName -> !reportDescriptionForPossibleNameClash(tree, methodName, state)) + .filter(methodName -> isValidMethodName(tree, methodName, state)) .ifPresent( methodName -> builder.merge(SuggestedFixes.renameMethod(tree, methodName, state))); } return builder.isEmpty() ? Description.NO_MATCH : describeMatch(tree, builder.build()); } - private boolean reportDescriptionForPossibleNameClash( - MethodTree tree, String methodName, VisitorState state) { + private boolean isValidMethodName(MethodTree tree, String methodName, VisitorState state) { if (isMethodNameInClass(methodName, state)) { - state.reportMatch( - buildDescription(tree) - .setMessage( - String.format("A method with name %s already exists in the class.", methodName)) - .build()); - return true; + reportIncorrectMethodName( + methodName, tree, "A method with name %s already exists in the class.", state); + return false; } if (isMethodNameStaticallyImported(methodName, state)) { - state.reportMatch( - buildDescription(tree) - .setMessage( - String.format( - "A method with name %s is already statically imported.", methodName)) - .build()); - return true; + reportIncorrectMethodName( + methodName, tree, "A method with name %s is already statically imported.", state); + return false; } - return false; + if (isJavaKeyword(methodName)) { + reportIncorrectMethodName( + methodName, + tree, + "Method name `%s` is not possible because it is a Java keyword.", + state); + return false; + } + return true; + } + + private void reportIncorrectMethodName( + String methodName, MethodTree tree, String message, VisitorState state) { + state.reportMatch( + buildDescription(tree).setMessage(String.format(message, methodName)).build()); } private static boolean isMethodNameInClass(String methodName, VisitorState state) { diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JavaKeywords.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JavaKeywords.java new file mode 100644 index 0000000000..a86e1ff603 --- /dev/null +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JavaKeywords.java @@ -0,0 +1,77 @@ +package tech.picnic.errorprone.bugpatterns; + +import com.google.common.collect.ImmutableSet; + +@SuppressWarnings("DeclarationOrder" /* The private constructor should come first. */) +final class JavaKeywords { + private JavaKeywords() {} + + /** + * List of all Java Language Keywords. + * + *

See: the Oracle + * Documentation on Java Keywords. + */ + private static final ImmutableSet JAVA_KEYWORDS = + ImmutableSet.of( + "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"); + + /** + * Check whether the String is a Java Language Keyword. + * + * @param possibleKeyword a possible Java keyword + * @return whether the String is a Java keyword + */ + public static boolean isJavaKeyword(String possibleKeyword) { + return JAVA_KEYWORDS.contains(possibleKeyword); + } +} diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationCheckTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationCheckTest.java index d2faf47ee5..9c25eb0d2e 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationCheckTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationCheckTest.java @@ -267,4 +267,44 @@ void methodAlreadyInStaticImports() { "}") .doTest(TestMode.TEXT_MATCH); } + + @Test + void methodHasJavaKeyword() { + refactoringTestHelper + .addInputLines( + "A.java", + "import org.junit.jupiter.api.Test;", + "", + "class A {", + " @Test", + " void testClass() {}", + "", + " @Test", + " void testClazz() {}", + "", + " @Test", + " void testThrow() {}", + "", + " @Test", + " void testThrowww() {}", + "}") + .addOutputLines( + "A.java", + "import org.junit.jupiter.api.Test;", + "", + "class A {", + " @Test", + " void testClass() {}", + "", + " @Test", + " void clazz() {}", + "", + " @Test", + " void testThrow() {}", + "", + " @Test", + " void throwww() {}", + "}") + .doTest(TestMode.TEXT_MATCH); + } } From 744b41a1e983d197ed5f6b1b99f709ca2cb889db Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Wed, 16 Feb 2022 13:28:10 +0100 Subject: [PATCH 6/9] JavaKeywords: Add `sealed` --- .../java/tech/picnic/errorprone/bugpatterns/JavaKeywords.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JavaKeywords.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JavaKeywords.java index a86e1ff603..c3d91f4ddc 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JavaKeywords.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JavaKeywords.java @@ -12,6 +12,8 @@ private JavaKeywords() {} *

See: the Oracle * Documentation on Java Keywords. + * + *

In addition, `sealed` is added for Java 17. See: JEP-409. */ private static final ImmutableSet JAVA_KEYWORDS = ImmutableSet.of( @@ -50,6 +52,7 @@ private JavaKeywords() {} "protected", "public", "return", + "sealed", "short", "static", "strictfp", From f2e1c7398455b3e1dd23151e8d95a43ece30cac2 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Sat, 9 Apr 2022 15:27:40 +0200 Subject: [PATCH 7/9] Review `JavaKeywords` --- .../JUnitMethodDeclarationCheck.java | 4 +- .../errorprone/bugpatterns/JavaKeywords.java | 64 ++++++++++++++----- 2 files changed, 50 insertions(+), 18 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationCheck.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationCheck.java index cd16cdf681..ea1bda6c78 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationCheck.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationCheck.java @@ -8,7 +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.isJavaKeyword; +import static tech.picnic.errorprone.bugpatterns.JavaKeywords.isReservedKeyword; import com.google.auto.service.AutoService; import com.google.common.collect.ImmutableSet; @@ -109,7 +109,7 @@ private boolean isValidMethodName(MethodTree tree, String methodName, VisitorSta return false; } - if (isJavaKeyword(methodName)) { + if (isReservedKeyword(methodName)) { reportIncorrectMethodName( methodName, tree, diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JavaKeywords.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JavaKeywords.java index c3d91f4ddc..103add1b26 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JavaKeywords.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JavaKeywords.java @@ -1,22 +1,19 @@ package tech.picnic.errorprone.bugpatterns; import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Sets; -@SuppressWarnings("DeclarationOrder" /* The private constructor should come first. */) final class JavaKeywords { - private JavaKeywords() {} - /** - * List of all Java Language Keywords. + * List of all reserved keywords in the Java language. * - *

See: the Oracle - * Documentation on Java Keywords. - * - *

In addition, `sealed` is added for Java 17. See: JEP-409. + * @see JDK 17 JLS + * section 3.9: Keywords */ - private static final ImmutableSet JAVA_KEYWORDS = + private static final ImmutableSet RESERVED_KEYWORDS = ImmutableSet.of( + "_", + "abstract", "assert", "boolean", "break", @@ -52,7 +49,6 @@ private JavaKeywords() {} "protected", "public", "return", - "sealed", "short", "static", "strictfp", @@ -69,12 +65,48 @@ private JavaKeywords() {} "while"); /** - * Check whether the String is a Java Language Keyword. + * List of all contextual keywords in the Java language. * - * @param possibleKeyword a possible Java keyword - * @return whether the String is a Java keyword + * @see JDK 17 JLS + * section 3.9: Keywords */ - public static boolean isJavaKeyword(String possibleKeyword) { - return JAVA_KEYWORDS.contains(possibleKeyword); + private static final ImmutableSet 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 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); } } From c27c0ffab6241500d0186fc5c27181eab093eabb Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Sun, 10 Apr 2022 09:07:15 +0200 Subject: [PATCH 8/9] Suggestions --- .../JUnitMethodDeclarationCheck.java | 95 ++++++----- .../JUnitMethodDeclarationCheckTest.java | 157 ++++-------------- 2 files changed, 84 insertions(+), 168 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationCheck.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationCheck.java index ea1bda6c78..9a8c598b0c 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationCheck.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationCheck.java @@ -25,12 +25,10 @@ import com.google.errorprone.util.ASTHelpers; import com.sun.source.tree.AnnotationTree; import com.sun.source.tree.ClassTree; -import com.sun.source.tree.CompilationUnitTree; 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.Objects; import java.util.Optional; import javax.lang.model.element.Modifier; import javax.lang.model.element.Name; @@ -83,50 +81,72 @@ 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) { - tryCanonicalizeMethodName(tree) - .filter(methodName -> isValidMethodName(tree, methodName, state)) - .ifPresent( - methodName -> builder.merge(SuggestedFixes.renameMethod(tree, methodName, state))); + 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 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 boolean isValidMethodName(MethodTree tree, String methodName, VisitorState state) { - if (isMethodNameInClass(methodName, state)) { - reportIncorrectMethodName( - methodName, tree, "A method with name %s already exists in the class.", state); - return false; + 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. + * + *

This method implements imperfect heuristics. Things it currently does not consider include + * the following: + * + *

    + *
  • Whether the rename would merely introduce a method overload, rather than clashing with an + * existing method declaration. + *
  • Whether the rename would cause a method in a superclass to be overridden. + *
  • 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.) + *
+ */ + private static Optional 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 (isMethodNameStaticallyImported(methodName, state)) { - reportIncorrectMethodName( - methodName, tree, "A method with name %s is already statically imported.", state); - return false; + if (isSimpleNameStaticallyImported(methodName, state)) { + return Optional.of(String.format("`%s` is already statically imported", methodName)); } if (isReservedKeyword(methodName)) { - reportIncorrectMethodName( - methodName, - tree, - "Method name `%s` is not possible because it is a Java keyword.", - state); - return false; + return Optional.of(String.format("`%s` is a reserved keyword", methodName)); } - return true; - } - private void reportIncorrectMethodName( - String methodName, MethodTree tree, String message, VisitorState state) { - state.reportMatch( - buildDescription(tree).setMessage(String.format(message, methodName)).build()); + return Optional.empty(); } - private static boolean isMethodNameInClass(String methodName, VisitorState state) { + private static boolean isMethodInEnclosingClass(String methodName, VisitorState state) { return state.findEnclosing(ClassTree.class).getMembers().stream() .filter(MethodTree.class::isInstance) .map(MethodTree.class::cast) @@ -136,18 +156,15 @@ private static boolean isMethodNameInClass(String methodName, VisitorState state .anyMatch(methodName::equals); } - private static boolean isMethodNameStaticallyImported(String methodName, VisitorState state) { - CompilationUnitTree compilationUnit = state.getPath().getCompilationUnit(); - - return compilationUnit.getImports().stream() - .filter(Objects::nonNull) + private static boolean isSimpleNameStaticallyImported(String simpleName, VisitorState state) { + return state.getPath().getCompilationUnit().getImports().stream() .filter(ImportTree::isStatic) .map(ImportTree::getQualifiedIdentifier) - .map(tree -> getStaticImportIdentifier(tree, state)) - .anyMatch(methodName::contentEquals); + .map(tree -> getStaticImportSimpleName(tree, state)) + .anyMatch(simpleName::contentEquals); } - private static CharSequence getStaticImportIdentifier(Tree tree, VisitorState state) { + private static CharSequence getStaticImportSimpleName(Tree tree, VisitorState state) { String source = Util.treeToString(tree, state); return source.subSequence(source.lastIndexOf('.') + 1, source.length()); } diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationCheckTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationCheckTest.java index 9c25eb0d2e..9e26bb8a44 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationCheckTest.java +++ b/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,136 +198,11 @@ void replacement() { " @Test void baz() {}", " @RepeatedTest(2) void qux() {}", " @ParameterizedTest void quux() {}", - "}") - .doTest(TestMode.TEXT_MATCH); - } - - @Test - void methodAlreadyExistsInClass() { - refactoringTestHelper - .addInputLines( - "A.java", - "import org.junit.jupiter.api.Test;", - "", - "class A {", - " @Test void testFoo() {}", - " void foo() {}", - "", - " @Test void testBar() {}", - " private void bar() {}", - "", - " @Test void testFooDifferent() {}", - " @Test void testBarDifferent() {}", - "}") - .addOutputLines( - "A.java", - "import org.junit.jupiter.api.Test;", - "", - "class A {", - " @Test void testFoo() {}", - " void foo() {}", - "", - " @Test void testBar() {}", - " private void bar() {}", - "", - " @Test void fooDifferent() {}", - " @Test void barDifferent() {}", - "}") - .doTest(TestMode.TEXT_MATCH); - } - - @Test - void methodAlreadyInStaticImports() { - refactoringTestHelper - .addInputLines( - "A.java", - "import static com.google.common.collect.ImmutableSet.toImmutableSet;", - "import static org.junit.jupiter.params.provider.Arguments.arguments;", - "", - "import org.junit.jupiter.api.Test;", - "import com.google.common.collect.ImmutableSet;", - "", - "class A {", - " @Test", - " void testArguments() {", - " arguments(1, 2, 3);", - " }", - "", - " @Test", - " void testToImmutableSet() {", - " ImmutableSet.of(1).stream().filter(i -> i > 1).collect(toImmutableSet());", - " }", - "", - " @Test", - " void testArgumentsDifferentName() {}", - "", - " @Test", - " void testToImmutableSetDifferentName() {}", - "}") - .addOutputLines( - "A.java", - "import static com.google.common.collect.ImmutableSet.toImmutableSet;", - "import static org.junit.jupiter.params.provider.Arguments.arguments;", - "", - "import org.junit.jupiter.api.Test;", - "import com.google.common.collect.ImmutableSet;", - "", - "class A {", - " @Test", - " void testArguments() {", - " arguments(1, 2, 3);", - " }", - "", - " @Test", - " void testToImmutableSet() {", - " ImmutableSet.of(1).stream().filter(i -> i > 1).collect(toImmutableSet());", - " }", - "", - " @Test", - " void argumentsDifferentName() {}", - "", - " @Test", - " void toImmutableSetDifferentName() {}", - "}") - .doTest(TestMode.TEXT_MATCH); - } - - @Test - void methodHasJavaKeyword() { - refactoringTestHelper - .addInputLines( - "A.java", - "import org.junit.jupiter.api.Test;", - "", - "class A {", - " @Test", - " void testClass() {}", - "", - " @Test", - " void testClazz() {}", - "", - " @Test", - " void testThrow() {}", - "", - " @Test", - " void testThrowww() {}", - "}") - .addOutputLines( - "A.java", - "import org.junit.jupiter.api.Test;", - "", - "class A {", - " @Test", - " void testClass() {}", - "", - " @Test", - " void clazz() {}", - "", - " @Test", - " void testThrow() {}", "", - " @Test", - " void throwww() {}", + " @Test void testOverload() {}", + " void overload() {}", + " @Test void testArguments() {}", + " @Test void testClass() {}", "}") .doTest(TestMode.TEXT_MATCH); } From d9f283cb0b995e042099d2477bd85a1dda0c69e2 Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Tue, 12 Apr 2022 16:53:39 +0200 Subject: [PATCH 9/9] Remove unnecessary filter on generated constructor --- .../errorprone/bugpatterns/JUnitMethodDeclarationCheck.java | 1 - 1 file changed, 1 deletion(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationCheck.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationCheck.java index 9a8c598b0c..eb28eb0e2f 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationCheck.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationCheck.java @@ -150,7 +150,6 @@ private static boolean isMethodInEnclosingClass(String methodName, VisitorState return state.findEnclosing(ClassTree.class).getMembers().stream() .filter(MethodTree.class::isInstance) .map(MethodTree.class::cast) - .filter(not(ASTHelpers::isGeneratedConstructor)) .map(MethodTree::getName) .map(Name::toString) .anyMatch(methodName::equals);