Introduce ConflictDetection utility class (#478)

2026-03-10 08:11:25 +00:00 · 2023-02-13 12:43:17 +01:00
parent da9a6dd270
commit 29469cbbfd
3 changed files with 151 additions and 62 deletions
--- 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.hasModifier;
 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.fixes.SuggestedFixes;
 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 @@ public final class JUnitMethodDeclaration extends BugChecker implements MethodTr
    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 @@ public final class JUnitMethodDeclaration extends BugChecker implements MethodTr
            .build());
  }

-  /**
-   * If applicable, returns a human-readable argument against assigning the given name to an
-   * existing method.
-   *
-   * <p>This method implements imperfect heuristics. Things it currently does not consider include
-   * the following:
-   *
-   * <ul>
-   *   <li>Whether the rename would merely introduce a method overload, rather than clashing with an
-   *       existing method declaration.
-   *   <li>Whether the rename would cause a method in a superclass to be overridden.
-   *   <li>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.)
-   * </ul>
-   */
-  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` 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<String> tryCanonicalizeMethodName(Symbol symbol) {
+  private static Optional<String> tryCanonicalizeMethodName(MethodSymbol symbol) {
    return Optional.of(symbol.getQualifiedName().toString())
        .filter(name -> name.startsWith(TEST_PREFIX))
        .map(name -> name.substring(TEST_PREFIX.length()))
--- 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
@@ -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.
+   *
+   * <p>This method implements imperfect heuristics. Things it currently does not consider include
+   * the following:
+   *
+   * <ul>
+   *   <li>Whether the rename would merely introduce a method overload, rather than clashing with an
+   *       existing method declaration in its class or a supertype.
+   *   <li>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.)
+   * </ul>
+   *
+   * @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<String> 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());
+  }
+}
--- 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
@@ -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);
+    }
+  }
+}