From 93576b4ca9bee04619733b037e80d708ad8ee70a Mon Sep 17 00:00:00 2001 From: Eric Staffas <114935344+eric-picnic@users.noreply.github.com> Date: Fri, 4 Nov 2022 16:09:20 +0100 Subject: [PATCH] Address review comments --- .../JUnitFactoryMethodDeclaration.java | 19 ++++++++++++------- .../bugpatterns/JUnitMethodDeclaration.java | 4 ++-- .../bugpatterns/util/ConflictDetection.java | 13 +------------ .../bugpatterns/util/MoreASTHelpers.java | 11 +++++++++++ .../{JUnit.java => MoreJUnitMatchers.java} | 4 ++-- 5 files changed, 28 insertions(+), 23 deletions(-) rename error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/{JUnit.java => MoreJUnitMatchers.java} (97%) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitFactoryMethodDeclaration.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitFactoryMethodDeclaration.java index e44803edfbc..d22f048e8d9 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitFactoryMethodDeclaration.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitFactoryMethodDeclaration.java @@ -18,8 +18,8 @@ import static javax.lang.model.element.Modifier.PRIVATE; import static tech.picnic.errorprone.bugpatterns.util.ConflictDetection.findMethodRenameBlocker; import static tech.picnic.errorprone.bugpatterns.util.Documentation.BUG_PATTERNS_BASE_URL; -import static tech.picnic.errorprone.bugpatterns.util.JUnit.HAS_METHOD_SOURCE; -import static tech.picnic.errorprone.bugpatterns.util.JUnit.TEST_METHOD; +import static tech.picnic.errorprone.bugpatterns.util.MoreJUnitMatchers.HAS_METHOD_SOURCE; +import static tech.picnic.errorprone.bugpatterns.util.MoreJUnitMatchers.TEST_METHOD; import com.google.auto.service.AutoService; import com.google.common.collect.ImmutableList; @@ -46,15 +46,20 @@ import java.util.List; import java.util.Optional; import java.util.stream.Stream; -import tech.picnic.errorprone.bugpatterns.util.JUnit; import tech.picnic.errorprone.bugpatterns.util.MoreASTHelpers; +import tech.picnic.errorprone.bugpatterns.util.MoreJUnitMatchers; /** * A {@link BugChecker} that flags non-canonical JUnit factory method declarations. * - *

A canonical JUnit factory method is one which - has the same name as the test method it - * provides test cases for, but with a `TestCases` suffix, and - has a comment which connects the - * return statement to the names of the parameters in the corresponding test method. + *

At Picnic, we consider a JUnit factory method canonical if it + * + *

*/ @AutoService(BugChecker.class) @BugPattern( @@ -92,7 +97,7 @@ public Description matchMethod(MethodTree tree, VisitorState state) { } Optional factoryMethodName = - JUnit.extractSingleFactoryMethodName(methodSourceAnnotation); + MoreJUnitMatchers.extractSingleFactoryMethodName(methodSourceAnnotation); if (factoryMethodName.isEmpty()) { /* If a test has multiple factory methods, not all of them can be given the desired name. */ 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 b5e5d37ce1c..d2ff987564c 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 @@ -13,8 +13,8 @@ import static java.util.function.Predicate.not; import static tech.picnic.errorprone.bugpatterns.util.ConflictDetection.findMethodRenameBlocker; import static tech.picnic.errorprone.bugpatterns.util.Documentation.BUG_PATTERNS_BASE_URL; -import static tech.picnic.errorprone.bugpatterns.util.JUnit.SETUP_OR_TEARDOWN_METHOD; -import static tech.picnic.errorprone.bugpatterns.util.JUnit.TEST_METHOD; +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; diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/ConflictDetection.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/ConflictDetection.java index 4c8d5cee7ee..4947d444f8a 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/ConflictDetection.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/ConflictDetection.java @@ -1,14 +1,12 @@ package tech.picnic.errorprone.bugpatterns.util; import static tech.picnic.errorprone.bugpatterns.util.JavaKeywords.isReservedKeyword; +import static tech.picnic.errorprone.bugpatterns.util.MoreASTHelpers.isMethodInEnclosingClass; import com.google.errorprone.VisitorState; -import com.sun.source.tree.ClassTree; import com.sun.source.tree.ImportTree; -import com.sun.source.tree.MethodTree; import com.sun.source.tree.Tree; import java.util.Optional; -import javax.lang.model.element.Name; /** * A set of helper methods for finding conflicts which would be caused by applying certain fixes. @@ -54,15 +52,6 @@ public static Optional findMethodRenameBlocker(String methodName, Visito return Optional.empty(); } - private static boolean isMethodInEnclosingClass(String methodName, VisitorState state) { - return state.findEnclosing(ClassTree.class).getMembers().stream() - .filter(MethodTree.class::isInstance) - .map(MethodTree.class::cast) - .map(MethodTree::getName) - .map(Name::toString) - .anyMatch(methodName::equals); - } - private static boolean isSimpleNameStaticallyImported(String simpleName, VisitorState state) { return state.getPath().getCompilationUnit().getImports().stream() .filter(ImportTree::isStatic) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/MoreASTHelpers.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/MoreASTHelpers.java index d1577f35e44..426f5450029 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/MoreASTHelpers.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/MoreASTHelpers.java @@ -3,8 +3,10 @@ import static com.google.common.collect.ImmutableList.toImmutableList; import com.google.common.collect.ImmutableList; +import com.google.errorprone.VisitorState; import com.sun.source.tree.ClassTree; import com.sun.source.tree.MethodTree; +import javax.lang.model.element.Name; /** A set of helper methods for working with the AST. */ public final class MoreASTHelpers { @@ -24,4 +26,13 @@ public static ImmutableList findMethods(ClassTree enclosingClass, St .filter(method -> method.getName().contentEquals(methodName)) .collect(toImmutableList()); } + + static boolean isMethodInEnclosingClass(String methodName, VisitorState state) { + return state.findEnclosing(ClassTree.class).getMembers().stream() + .filter(MethodTree.class::isInstance) + .map(MethodTree.class::cast) + .map(MethodTree::getName) + .map(Name::toString) + .anyMatch(methodName::equals); + } } diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/JUnit.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/MoreJUnitMatchers.java similarity index 97% rename from error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/JUnit.java rename to error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/MoreJUnitMatchers.java index 67e95f8e454..e65eaf625c1 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/JUnit.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/MoreJUnitMatchers.java @@ -20,7 +20,7 @@ import javax.lang.model.type.TypeKind; /** A set of JUnit-specific helpers for working with the AST. */ -public final class JUnit { +public final class MoreJUnitMatchers { /** Matches JUnit test methods. */ public static final MultiMatcher TEST_METHOD = annotations( @@ -45,7 +45,7 @@ public final class JUnit { public static final Matcher HAS_METHOD_SOURCE = allOf(annotations(AT_LEAST_ONE, isType("org.junit.jupiter.params.provider.MethodSource"))); - private JUnit() {} + private MoreJUnitMatchers() {} /** * Extracts the name of the JUnit factory method from a {@link