Suggestions

PicnicSupermarket · Oct 23, 2023 · 11ed5e5 · 11ed5e5
1 parent 3edff43
commit 11ed5e5
Show file tree

Hide file tree

Showing 5 changed files with 251 additions and 85 deletions.
diff --git a/...-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AutowiredConstructor.java b/...-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AutowiredConstructor.java
@@ -9,14 +9,14 @@
 import static tech.picnic.errorprone.bugpatterns.util.Documentation.BUG_PATTERNS_BASE_URL;
 
 import com.google.auto.service.AutoService;
-import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Iterables;
 import com.google.errorprone.BugPattern;
 import com.google.errorprone.VisitorState;
 import com.google.errorprone.bugpatterns.BugChecker;
 import com.google.errorprone.bugpatterns.BugChecker.ClassTreeMatcher;
 import com.google.errorprone.matchers.Description;
 import com.google.errorprone.matchers.MultiMatcher;
+import com.google.errorprone.matchers.MultiMatcher.MultiMatchResult;
 import com.google.errorprone.util.ASTHelpers;
 import com.sun.source.tree.AnnotationTree;
 import com.sun.source.tree.ClassTree;
@@ -48,11 +48,9 @@ public Description matchClass(ClassTree tree, VisitorState state) {
       return Description.NO_MATCH;
     }
 
-    ImmutableList<AnnotationTree> annotations =
-        AUTOWIRED_ANNOTATION
-            .multiMatchResult(Iterables.getOnlyElement(constructors), state)
-            .matchingNodes();
-    if (annotations.size() != 1) {
+    MultiMatchResult<AnnotationTree> hasAutowiredAnnotation =
+        AUTOWIRED_ANNOTATION.multiMatchResult(Iterables.getOnlyElement(constructors), state);
+    if (!hasAutowiredAnnotation.matches()) {
       return Description.NO_MATCH;
     }
 
@@ -61,7 +59,7 @@ public Description matchClass(ClassTree tree, VisitorState state) {
      * means that the associated import can be removed as well. Rather than adding code for this
      * case we leave flagging the unused import to Error Prone's `RemoveUnusedImports` check.
      */
-    AnnotationTree annotation = Iterables.getOnlyElement(annotations);
+    AnnotationTree annotation = hasAutowiredAnnotation.onlyMatchingNode();
     return describeMatch(annotation, SourceCode.deleteWithTrailingWhitespace(annotation, state));
   }
 }
diff --git a/...ain/java/tech/picnic/errorprone/bugpatterns/JUnitNullaryParameterizedTestDeclaration.java b/...ain/java/tech/picnic/errorprone/bugpatterns/JUnitNullaryParameterizedTestDeclaration.java
@@ -0,0 +1,91 @@
+package tech.picnic.errorprone.bugpatterns;
+
+import static com.google.errorprone.BugPattern.LinkType.CUSTOM;
+import static com.google.errorprone.BugPattern.SeverityLevel.SUGGESTION;
+import static com.google.errorprone.BugPattern.StandardTags.SIMPLIFICATION;
+import static com.google.errorprone.matchers.ChildMultiMatcher.MatchType.AT_LEAST_ONE;
+import static com.google.errorprone.matchers.Matchers.annotations;
+import static com.google.errorprone.matchers.Matchers.anyOf;
+import static com.google.errorprone.matchers.Matchers.isType;
+import static tech.picnic.errorprone.bugpatterns.util.Documentation.BUG_PATTERNS_BASE_URL;
+import static tech.picnic.errorprone.bugpatterns.util.MoreMatchers.hasMetaAnnotation;
+
+import com.google.auto.service.AutoService;
+import com.google.errorprone.BugPattern;
+import com.google.errorprone.VisitorState;
+import com.google.errorprone.bugpatterns.BugChecker;
+import com.google.errorprone.bugpatterns.BugChecker.MethodTreeMatcher;
+import com.google.errorprone.fixes.SuggestedFix;
+import com.google.errorprone.fixes.SuggestedFixes;
+import com.google.errorprone.matchers.Description;
+import com.google.errorprone.matchers.Matcher;
+import com.google.errorprone.matchers.MultiMatcher;
+import com.google.errorprone.matchers.MultiMatcher.MultiMatchResult;
+import com.sun.source.tree.AnnotationTree;
+import com.sun.source.tree.MethodTree;
+import tech.picnic.errorprone.bugpatterns.util.SourceCode;
+
+/**
+ * A {@link BugChecker} that flags nullary {@link
+ * org.junit.jupiter.params.ParameterizedTest @ParameterizedTest} test methods.
+ *
+ * <p>Such tests are unnecessarily executed more than necessary. This checker suggests annotating
+ * the method with {@link org.junit.jupiter.api.Test @Test}, and to drop all declared {@link
+ * org.junit.jupiter.params.provider.ArgumentsSource argument sources}.
+ */
+@AutoService(BugChecker.class)
+@BugPattern(
+    summary = "Nullary JUnit test methods need not be parameterized",
+    link = BUG_PATTERNS_BASE_URL + "JUnitNullaryParameterizedTestDeclaration",
+    linkType = CUSTOM,
+    severity = SUGGESTION,
+    tags = SIMPLIFICATION)
+public final class JUnitNullaryParameterizedTestDeclaration extends BugChecker
+    implements MethodTreeMatcher {
+  private static final long serialVersionUID = 1L;
+  private static final MultiMatcher<MethodTree, AnnotationTree> IS_PARAMETERIZED_TEST =
+      annotations(AT_LEAST_ONE, isType("org.junit.jupiter.params.ParameterizedTest"));
+  private static final Matcher<AnnotationTree> IS_ARGUMENT_SOURCE =
+      anyOf(
+          isType("org.junit.jupiter.params.provider.ArgumentsSource"),
+          isType("org.junit.jupiter.params.provider.ArgumentsSources"),
+          hasMetaAnnotation("org.junit.jupiter.params.provider.ArgumentsSource"));
+
+  /** Instantiates a new {@link JUnitNullaryParameterizedTestDeclaration} instance. */
+  public JUnitNullaryParameterizedTestDeclaration() {}
+
+  @Override
+  public Description matchMethod(MethodTree tree, VisitorState state) {
+    if (!tree.getParameters().isEmpty()) {
+      return Description.NO_MATCH;
+    }
+
+    MultiMatchResult<AnnotationTree> isParameterizedTest =
+        IS_PARAMETERIZED_TEST.multiMatchResult(tree, state);
+    if (!isParameterizedTest.matches()) {
+      return Description.NO_MATCH;
+    }
+
+    /*
+     * This method is vacuously parameterized. Suggest replacing `@ParameterizedTest` with `@Test`.
+     * (As each method is checked independently, we cannot in general determine whether this
+     * suggestion makes a `ParameterizedTest` type import obsolete; that task is left to Error
+     * Prone's `RemoveUnusedImports` check.)
+     */
+    SuggestedFix.Builder fix = SuggestedFix.builder();
+    fix.merge(
+        SuggestedFix.replace(
+            isParameterizedTest.onlyMatchingNode(),
+            '@' + SuggestedFixes.qualifyType(state, fix, "org.junit.jupiter.api.Test")));
+
+    /*
+     * Also suggest dropping all (explicit and implicit) `@ArgumentsSource`s. No attempt is made to
+     * assess whether a dropped `@MethodSource` also makes the referenced factory method(s) unused.
+     */
+    tree.getModifiers().getAnnotations().stream()
+        .filter(a -> IS_ARGUMENT_SOURCE.matches(a, state))
+        .forEach(a -> fix.merge(SourceCode.deleteWithTrailingWhitespace(a, state)));
+
+    return describeMatch(tree, fix.build());
+  }
+}
diff --git a/...src/main/java/tech/picnic/errorprone/bugpatterns/JUnitParameterizedMethodDeclaration.java b/...src/main/java/tech/picnic/errorprone/bugpatterns/JUnitParameterizedMethodDeclaration.java
diff --git a/...java/tech/picnic/errorprone/bugpatterns/JUnitNullaryParameterizedTestDeclarationTest.java b/...java/tech/picnic/errorprone/bugpatterns/JUnitNullaryParameterizedTestDeclarationTest.java
@@ -0,0 +1,155 @@
+package tech.picnic.errorprone.bugpatterns;
+
+import com.google.errorprone.BugCheckerRefactoringTestHelper;
+import com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode;
+import com.google.errorprone.CompilationTestHelper;
+import org.junit.jupiter.api.Test;
+
+final class JUnitNullaryParameterizedTestDeclarationTest {
+  @Test
+  void identification() {
+    CompilationTestHelper.newInstance(JUnitNullaryParameterizedTestDeclaration.class, getClass())
+        .addSourceLines(
+            "A.java",
+            "import org.junit.jupiter.api.Test;",
+            "import org.junit.jupiter.params.ParameterizedTest;",
+            "import org.junit.jupiter.params.provider.ValueSource;",
+            "",
+            "class A {",
+            "  void nonTest() {}",
+            "",
+            "  @Test",
+            "  void nonParameterizedTest() {}",
+            "",
+            "  @ParameterizedTest",
+            "  @ValueSource(ints = {0, 1})",
+            "  void goodParameterizedTest(int someInt) {}",
+            "",
+            "  @ParameterizedTest",
+            "  @ValueSource(ints = {0, 1})",
+            "  // BUG: Diagnostic contains:",
+            "  void nullaryParameterizedTest() {}",
+            "}")
+        .doTest();
+  }
+
+  @Test
+  void replacement() {
+    BugCheckerRefactoringTestHelper.newInstance(
+            JUnitNullaryParameterizedTestDeclaration.class, getClass())
+        .addInputLines(
+            "A.java",
+            "import org.junit.jupiter.params.ParameterizedTest;",
+            "import org.junit.jupiter.params.provider.ArgumentsProvider;",
+            "import org.junit.jupiter.params.provider.ArgumentsSource;",
+            "import org.junit.jupiter.params.provider.ArgumentsSources;",
+            "import org.junit.jupiter.params.provider.MethodSource;",
+            "import org.junit.jupiter.params.provider.ValueSource;",
+            "",
+            "class A {",
+            "  @ParameterizedTest",
+            "  void withoutArgumentSource() {}",
+            "",
+            "  @ParameterizedTest",
+            "  @ArgumentsSource(ArgumentsProvider.class)",
+            "  void withCustomArgumentSource() {}",
+            "",
+            "  @ParameterizedTest",
+            "  @ArgumentsSources({",
+            "    @ArgumentsSource(ArgumentsProvider.class),",
+            "    @ArgumentsSource(ArgumentsProvider.class)",
+            "  })",
+            "  void withCustomerArgumentSources() {}",
+            "",
+            "  /** Foo. */",
+            "  @ParameterizedTest",
+            "  @ValueSource(ints = {0, 1})",
+            "  void withValueSourceAndJavadoc() {}",
+            "",
+            "  @ParameterizedTest",
+            "  @MethodSource(\"nonexistentMethod\")",
+            "  @SuppressWarnings(\"foo\")",
+            "  void withMethodSourceAndUnrelatedAnnotation() {}",
+            "",
+            "  @org.junit.jupiter.params.ParameterizedTest",
+            "  @ArgumentsSource(ArgumentsProvider.class)",
+            "  @ValueSource(ints = {0, 1})",
+            "  @MethodSource(\"nonexistentMethod\")",
+            "  void withMultipleArgumentSourcesAndFullyQualifiedImport() {}",
+            "",
+            "  class NestedWithTestAnnotationFirst {",
+            "    @ParameterizedTest",
+            "    @ValueSource(ints = {0, 1})",
+            "    void withValueSource() {}",
+            "  }",
+            "",
+            "  class NestedWithTestAnnotationSecond {",
+            "    @ValueSource(ints = {0, 1})",
+            "    @ParameterizedTest",
+            "    void withValueSource() {}",
+            "  }",
+            "}")
+        .addOutputLines(
+            "A.java",
+            "import org.junit.jupiter.api.Test;",
+            "import org.junit.jupiter.params.ParameterizedTest;",
+            "import org.junit.jupiter.params.provider.ArgumentsProvider;",
+            "import org.junit.jupiter.params.provider.ArgumentsSource;",
+            "import org.junit.jupiter.params.provider.ArgumentsSources;",
+            "import org.junit.jupiter.params.provider.MethodSource;",
+            "import org.junit.jupiter.params.provider.ValueSource;",
+            "",
+            "class A {",
+            "  @Test",
+            "  void withoutArgumentSource() {}",
+            "",
+            "  @Test",
+            "  void withCustomArgumentSource() {}",
+            "",
+            "  @Test",
+            "  void withCustomerArgumentSources() {}",
+            "",
+            "  /** Foo. */",
+            "  @Test",
+            "  void withValueSourceAndJavadoc() {}",
+            "",
+            "  @Test",
+            "  @SuppressWarnings(\"foo\")",
+            "  void withMethodSourceAndUnrelatedAnnotation() {}",
+            "",
+            "  @Test",
+            "  void withMultipleArgumentSourcesAndFullyQualifiedImport() {}",
+            "",
+            "  class NestedWithTestAnnotationFirst {",
+            "    @Test",
+            "    void withValueSource() {}",
+            "  }",
+            "",
+            "  class NestedWithTestAnnotationSecond {",
+            "    @Test",
+            "    void withValueSource() {}",
+            "  }",
+            "}")
+        .addInputLines(
+            "B.java",
+            "import org.junit.jupiter.params.ParameterizedTest;",
+            "",
+            "class B {",
+            "  @ParameterizedTest",
+            "  void scopeInWhichIdentifierTestIsAlreadyDeclared() {}",
+            "",
+            "  class Test {}",
+            "}")
+        .addOutputLines(
+            "B.java",
+            "import org.junit.jupiter.params.ParameterizedTest;",
+            "",
+            "class B {",
+            "  @org.junit.jupiter.api.Test",
+            "  void scopeInWhichIdentifierTestIsAlreadyDeclared() {}",
+            "",
+            "  class Test {}",
+            "}")
+        .doTest(TestMode.TEXT_MATCH);
+  }
+}
diff --git a/...test/java/tech/picnic/errorprone/bugpatterns/JUnitParameterizedMethodDeclarationTest.java b/...test/java/tech/picnic/errorprone/bugpatterns/JUnitParameterizedMethodDeclarationTest.java