From 29469cbbfd8065928ce882c963a0bb6eb11f47be Mon Sep 17 00:00:00 2001 From: Eric Staffas <114935344+eric-picnic@users.noreply.github.com> Date: Mon, 13 Feb 2023 12:43:17 +0100 Subject: [PATCH] Introduce `ConflictDetection` utility class (#478) --- .../bugpatterns/JUnitMethodDeclaration.java | 65 +--------------- .../bugpatterns/util/ConflictDetection.java | 75 +++++++++++++++++++ .../util/ConflictDetectionTest.java | 73 ++++++++++++++++++ 3 files changed, 151 insertions(+), 62 deletions(-) create mode 100644 error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/ConflictDetection.java create mode 100644 error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/ConflictDetectionTest.java 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 20afd2fc96..b7ca52153c 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 @@ -9,7 +9,6 @@ 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.isValidIdentifier; import static tech.picnic.errorprone.bugpatterns.util.MoreJUnitMatchers.SETUP_OR_TEARDOWN_METHOD; import static tech.picnic.errorprone.bugpatterns.util.MoreJUnitMatchers.TEST_METHOD; @@ -25,15 +24,11 @@ import com.google.errorprone.matchers.Description; import com.google.errorprone.matchers.Matcher; 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.SourceCode; +import tech.picnic.errorprone.bugpatterns.util.ConflictDetection; /** A {@link BugChecker} that flags non-canonical JUnit method declarations. */ // XXX: Consider introducing a class-level check that enforces that test classes: @@ -90,7 +85,7 @@ private void suggestTestMethodRenameIfApplicable( tryCanonicalizeMethodName(symbol) .ifPresent( newName -> - findMethodRenameBlocker(symbol, newName, state) + ConflictDetection.findMethodRenameBlocker(symbol, newName, state) .ifPresentOrElse( blocker -> reportMethodRenameBlocker(tree, blocker, state), () -> fixBuilder.merge(SuggestedFixes.renameMethod(tree, newName, state)))); @@ -106,61 +101,7 @@ private void reportMethodRenameBlocker(MethodTree tree, String reason, VisitorSt .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: - * - *

- */ - 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` is already defined in this class or a supertype", newName)); - } - - if (isSimpleNameStaticallyImported(newName, state)) { - return Optional.of(String.format("`%s` is already statically imported", newName)); - } - - 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) - .map(ImportTree::getQualifiedIdentifier) - .map(tree -> getStaticImportSimpleName(tree, state)) - .anyMatch(simpleName::contentEquals); - } - - private static CharSequence getStaticImportSimpleName(Tree tree, VisitorState state) { - String source = SourceCode.treeToString(tree, state); - return source.subSequence(source.lastIndexOf('.') + 1, source.length()); - } - - private static Optional tryCanonicalizeMethodName(Symbol symbol) { + private static Optional tryCanonicalizeMethodName(MethodSymbol symbol) { return Optional.of(symbol.getQualifiedName().toString()) .filter(name -> name.startsWith(TEST_PREFIX)) .map(name -> name.substring(TEST_PREFIX.length())) 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 new file mode 100644 index 0000000000..42e8286fcf --- /dev/null +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/ConflictDetection.java @@ -0,0 +1,75 @@ +package tech.picnic.errorprone.bugpatterns.util; + +import static tech.picnic.errorprone.bugpatterns.util.JavaKeywords.isValidIdentifier; + +import com.google.errorprone.VisitorState; +import com.google.errorprone.util.ASTHelpers; +import com.sun.source.tree.ImportTree; +import com.sun.source.tree.Tree; +import com.sun.tools.javac.code.Symbol.MethodSymbol; +import com.sun.tools.javac.code.Type; +import java.util.Optional; + +/** A set of helper methods for detecting conflicts that would be caused when applying fixes. */ +public final class ConflictDetection { + private ConflictDetection() {} + + /** + * 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 in its class or a supertype. + *
  • 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.) + *
+ * + * @param method The method considered for renaming. + * @param newName The newly proposed name for the method. + * @param state The {@link VisitorState} to use when searching for blockers. + * @return A human-readable argument against assigning the proposed name to the given method, or + * {@link Optional#empty()} if no blocker was found. + */ + public 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` is already defined in this class or a supertype", newName)); + } + + if (isSimpleNameStaticallyImported(newName, state)) { + return Optional.of(String.format("`%s` is already statically imported", newName)); + } + + 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) + .map(ImportTree::getQualifiedIdentifier) + .map(tree -> getStaticImportSimpleName(tree, state)) + .anyMatch(simpleName::contentEquals); + } + + private static CharSequence getStaticImportSimpleName(Tree tree, VisitorState state) { + String source = SourceCode.treeToString(tree, state); + return source.subSequence(source.lastIndexOf('.') + 1, source.length()); + } +} diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/ConflictDetectionTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/ConflictDetectionTest.java new file mode 100644 index 0000000000..70282dd4fa --- /dev/null +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/ConflictDetectionTest.java @@ -0,0 +1,73 @@ +package tech.picnic.errorprone.bugpatterns.util; + +import static com.google.errorprone.BugPattern.SeverityLevel.ERROR; + +import com.google.errorprone.BugPattern; +import com.google.errorprone.CompilationTestHelper; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker; +import com.google.errorprone.bugpatterns.BugChecker.MethodTreeMatcher; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.util.ASTHelpers; +import com.sun.source.tree.MethodTree; +import com.sun.tools.javac.code.Symbol.MethodSymbol; +import org.junit.jupiter.api.Test; + +final class ConflictDetectionTest { + @Test + void matcher() { + CompilationTestHelper.newInstance(RenameBlockerFlagger.class, getClass()) + .addSourceLines( + "pkg/A.java", + "package pkg;", + "", + "import static pkg.A.B.method3t;", + "", + "import pkg.A.method4t;", + "", + "class A {", + " void method1() {", + " method3t();", + " method4(method4t.class);", + " }", + "", + " // BUG: Diagnostic contains: a method named `method2t` is already defined in this class or a", + " // supertype", + " void method2() {}", + "", + " void method2t() {}", + "", + " // BUG: Diagnostic contains: `method3t` is already statically imported", + " void method3() {}", + "", + " void method4(Object o) {}", + "", + " // BUG: Diagnostic contains: `int` is not a valid identifier", + " void in() {}", + "", + " static class B {", + " static void method3t() {}", + " }", + "", + " class method4t {}", + "}") + .doTest(); + } + + /** + * A {@link BugChecker} that uses {@link ConflictDetection#findMethodRenameBlocker(MethodSymbol, + * String, VisitorState)} to flag methods of which the name cannot be suffixed with a {@code t}. + */ + @BugPattern(summary = "Interacts with `ConflictDetection` for testing purposes", severity = ERROR) + public static final class RenameBlockerFlagger extends BugChecker implements MethodTreeMatcher { + private static final long serialVersionUID = 1L; + + @Override + public Description matchMethod(MethodTree tree, VisitorState state) { + return ConflictDetection.findMethodRenameBlocker( + ASTHelpers.getSymbol(tree), tree.getName() + "t", state) + .map(blocker -> buildDescription(tree).setMessage(blocker).build()) + .orElse(Description.NO_MATCH); + } + } +}