From 2ad41dd01d95fc9b11305ec8ad4249669246af29 Mon Sep 17 00:00:00 2001
From: Stephan Schroevers <stephan.schroevers@teampicnic.com>
Date: Fri, 3 Mar 2023 07:48:42 +0100
Subject: [PATCH] Suggestions

---
 .../BugPatternTestExtractor.java              | 81 ++++++++++---------
 .../errorprone/documentation/Extractor.java   |  3 +
 2 files changed, 47 insertions(+), 37 deletions(-)

diff --git a/documentation-support/src/main/java/tech/picnic/errorprone/documentation/BugPatternTestExtractor.java b/documentation-support/src/main/java/tech/picnic/errorprone/documentation/BugPatternTestExtractor.java
index 444da4ef5c5..cb733c2ca03 100644
--- a/documentation-support/src/main/java/tech/picnic/errorprone/documentation/BugPatternTestExtractor.java
+++ b/documentation-support/src/main/java/tech/picnic/errorprone/documentation/BugPatternTestExtractor.java
@@ -25,6 +25,8 @@
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Optional;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.regex.Pattern;
 import org.jspecify.annotations.Nullable;
 import tech.picnic.errorprone.documentation.BugPatternTestExtractor.BugPatternTestDocumentation;
 
@@ -34,8 +36,17 @@
  */
 @Immutable
 final class BugPatternTestExtractor implements Extractor<BugPatternTestDocumentation> {
+  private static final Pattern TEST_CLASS_NAME_PATTERN = Pattern.compile("(.*)Test");
   private static final Matcher<Tree> JUNIT_TEST_METHOD =
       toType(MethodTree.class, hasAnnotation("org.junit.jupiter.api.Test"));
+  private static final Matcher<MethodInvocationTree> BUG_PATTERN_TEST_METHOD =
+      allOf(
+          staticMethod()
+              .onDescendantOfAny(
+                  "com.google.errorprone.CompilationTestHelper",
+                  "com.google.errorprone.BugCheckerRefactoringTestHelper")
+              .named("newInstance"),
+          argument(0, classLiteral(anything())));
 
   // XXX: Improve support for correctly extracting multiple sources from a single
   // `{BugCheckerRefactoring,Compilation}TestHelper` test.
@@ -49,53 +60,49 @@ public BugPatternTestDocumentation extract(ClassTree tree, VisitorState state) {
       }
     }
 
-    String className = tree.getSimpleName().toString();
+    String bugPatternName =
+        getClassUnderTest(tree)
+            .orElseThrow(
+                () ->
+                    new IllegalArgumentException(
+                        String.format(
+                            "Name of given class does not match '%s'", TEST_CLASS_NAME_PATTERN)));
     return new AutoValue_BugPatternTestExtractor_BugPatternTestDocumentation(
-        className.substring(0, className.lastIndexOf("Test")),
-        scanner.getIdentificationTests(),
-        scanner.getReplacementTests());
+        bugPatternName, scanner.getIdentificationTests(), scanner.getReplacementTests());
   }
 
   @Override
   public boolean canExtract(ClassTree tree, VisitorState state) {
-    String className = tree.getSimpleName().toString();
-    if (!className.endsWith("Test")) {
-      return false;
-    }
-
-    ScanBugPatternTest scanBugPatternTest = new ScanBugPatternTest();
-    scanBugPatternTest.scan(tree, state);
-
-    String bugPatternName = className.substring(0, className.lastIndexOf("Test"));
-    return scanBugPatternTest.hasTestUsingClassInstance(bugPatternName);
+    return getClassUnderTest(tree)
+        .filter(bugPatternName -> testsBugPattern(bugPatternName, tree, state))
+        .isPresent();
   }
 
-  // XXX: Consider replacing this type with an anonymous class in a method. Possibly also below.
-  private static final class ScanBugPatternTest extends TreeScanner<@Nullable Void, VisitorState> {
-    private static final Matcher<MethodInvocationTree> BUG_PATTERN_TEST_METHOD =
-        allOf(
-            staticMethod()
-                .onDescendantOfAny(
-                    "com.google.errorprone.CompilationTestHelper",
-                    "com.google.errorprone.BugCheckerRefactoringTestHelper")
-                .named("newInstance"),
-            argument(0, classLiteral(anything())));
-
-    private final List<String> encounteredClasses = new ArrayList<>();
-
-    boolean hasTestUsingClassInstance(String clazz) {
-      return encounteredClasses.contains(clazz);
-    }
+  private static boolean testsBugPattern(
+      String bugPatternName, ClassTree tree, VisitorState state) {
+    AtomicBoolean result = new AtomicBoolean(false);
+
+    new TreeScanner<@Nullable Void, @Nullable Void>() {
+      @Override
+      public @Nullable Void visitMethodInvocation(MethodInvocationTree node, @Nullable Void v) {
+        if (BUG_PATTERN_TEST_METHOD.matches(node, state)) {
+          MemberSelectTree firstArgumentTree = (MemberSelectTree) node.getArguments().get(0);
+          result.compareAndSet(
+              /* expectedValue= */ false,
+              bugPatternName.equals(firstArgumentTree.getExpression().toString()));
+        }
 
-    @Override
-    public @Nullable Void visitMethodInvocation(MethodInvocationTree node, VisitorState state) {
-      if (BUG_PATTERN_TEST_METHOD.matches(node, state)) {
-        MemberSelectTree firstArgumentTree = (MemberSelectTree) node.getArguments().get(0);
-        encounteredClasses.add(firstArgumentTree.getExpression().toString());
+        return super.visitMethodInvocation(node, v);
       }
+    }.scan(tree, null);
 
-      return super.visitMethodInvocation(node, state);
-    }
+    return result.get();
+  }
+
+  private static Optional<String> getClassUnderTest(ClassTree tree) {
+    return Optional.of(TEST_CLASS_NAME_PATTERN.matcher(tree.getSimpleName().toString()))
+        .filter(java.util.regex.Matcher::matches)
+        .map(m -> m.group(1));
   }
 
   private static final class CollectBugPatternTests
diff --git a/documentation-support/src/main/java/tech/picnic/errorprone/documentation/Extractor.java b/documentation-support/src/main/java/tech/picnic/errorprone/documentation/Extractor.java
index 1fb3eca673f..96c51368f95 100644
--- a/documentation-support/src/main/java/tech/picnic/errorprone/documentation/Extractor.java
+++ b/documentation-support/src/main/java/tech/picnic/errorprone/documentation/Extractor.java
@@ -10,6 +10,9 @@
  *
  * @param <T> The type of data that is extracted.
  */
+// XXX: Here and in the implementations, either:
+// 1. Swap `canExtract` and `extract`.
+// 2. Combine the methods into a single `Optional<T> tryExtract`.
 @Immutable
 interface Extractor<T> {
   /**