diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AssociativeMethodInvocation.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AssociativeMethodInvocation.java new file mode 100644 index 0000000000..40259d7d15 --- /dev/null +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AssociativeMethodInvocation.java @@ -0,0 +1,96 @@ +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.Matchers.allOf; +import static com.google.errorprone.matchers.Matchers.not; +import static com.google.errorprone.matchers.Matchers.staticMethod; +import static com.google.errorprone.matchers.Matchers.toType; +import static tech.picnic.errorprone.bugpatterns.util.Documentation.BUG_PATTERNS_BASE_URL; + +import com.google.auto.service.AutoService; +import com.google.common.collect.ImmutableSet; +import com.google.errorprone.BugPattern; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker; +import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher; +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.Matchers; +import com.google.errorprone.refaster.Refaster; +import com.google.errorprone.suppliers.Supplier; +import com.google.errorprone.suppliers.Suppliers; +import com.google.errorprone.util.ASTHelpers; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.MethodInvocationTree; +import com.sun.tools.javac.code.Type; +import tech.picnic.errorprone.bugpatterns.util.SourceCode; + +/** + * A {@link BugChecker} that flags unnecessarily nested usage of methods that implement an + * associative operation. + * + *

The arguments to such methods can be flattened without affecting semantics, while making the + * code more readable. + */ +@AutoService(BugChecker.class) +@BugPattern( + summary = + "These methods implement an associative operation, so the list of operands can be flattened", + link = BUG_PATTERNS_BASE_URL + "AssociativeMethodInvocation", + linkType = CUSTOM, + severity = SUGGESTION, + tags = SIMPLIFICATION) +public final class AssociativeMethodInvocation extends BugChecker + implements MethodInvocationTreeMatcher { + private static final long serialVersionUID = 1L; + private static final Supplier ITERABLE = Suppliers.typeFromClass(Iterable.class); + private static final ImmutableSet> ASSOCIATIVE_OPERATIONS = + ImmutableSet.of( + allOf( + staticMethod().onClass(Suppliers.typeFromClass(Matchers.class)).named("allOf"), + toType(MethodInvocationTree.class, not(hasArgumentOfType(ITERABLE)))), + allOf( + staticMethod().onClass(Suppliers.typeFromClass(Matchers.class)).named("anyOf"), + toType(MethodInvocationTree.class, not(hasArgumentOfType(ITERABLE)))), + staticMethod().onClass(Suppliers.typeFromClass(Refaster.class)).named("anyOf")); + + /** Instantiates a new {@link AssociativeMethodInvocation} instance. */ + public AssociativeMethodInvocation() {} + + @Override + public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { + if (tree.getArguments().isEmpty()) { + /* Absent any arguments, there is nothing to simplify. */ + return Description.NO_MATCH; + } + + for (Matcher matcher : ASSOCIATIVE_OPERATIONS) { + if (matcher.matches(tree, state)) { + SuggestedFix.Builder fix = SuggestedFix.builder(); + for (ExpressionTree arg : tree.getArguments()) { + if (matcher.matches(arg, state)) { + MethodInvocationTree invocation = (MethodInvocationTree) arg; + fix.merge( + invocation.getArguments().isEmpty() + ? SuggestedFixes.removeElement(arg, tree.getArguments(), state) + : SourceCode.unwrapMethodInvocation(invocation, state)); + } + } + + return fix.isEmpty() ? Description.NO_MATCH : describeMatch(tree, fix.build()); + } + } + + return Description.NO_MATCH; + } + + private static Matcher hasArgumentOfType(Supplier type) { + return (tree, state) -> + tree.getArguments().stream() + .anyMatch(arg -> ASTHelpers.isSubtype(ASTHelpers.getType(arg), type.get(state), state)); + } +} diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/AssociativeMethodInvocationTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/AssociativeMethodInvocationTest.java new file mode 100644 index 0000000000..789fb14ec6 --- /dev/null +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/AssociativeMethodInvocationTest.java @@ -0,0 +1,110 @@ +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 AssociativeMethodInvocationTest { + @Test + void identification() { + CompilationTestHelper.newInstance(AssociativeMethodInvocation.class, getClass()) + .addSourceLines( + "A.java", + "import com.google.common.collect.ImmutableList;", + "import com.google.errorprone.matchers.Matchers;", + "import com.google.errorprone.refaster.Refaster;", + "", + "class A {", + " void m() {", + " Matchers.allOf();", + " Matchers.anyOf();", + " Refaster.anyOf();", + "", + " Matchers.allOf((t, s) -> true);", + " Matchers.anyOf((t, s) -> true);", + " Refaster.anyOf(0);", + "", + " Matchers.allOf(Matchers.anyOf((t, s) -> true));", + " Matchers.anyOf(Matchers.allOf((t, s) -> true));", + " Refaster.anyOf(Matchers.allOf((t, s) -> true));", + "", + " // BUG: Diagnostic contains:", + " Matchers.allOf(Matchers.allOf((t, s) -> true));", + " // BUG: Diagnostic contains:", + " Matchers.anyOf(Matchers.anyOf((t, s) -> true));", + " // BUG: Diagnostic contains:", + " Refaster.anyOf(Refaster.anyOf(0));", + "", + " Matchers.allOf(Matchers.allOf(ImmutableList.of((t, s) -> true)));", + " Matchers.anyOf(Matchers.anyOf(ImmutableList.of((t, s) -> true)));", + "", + " // BUG: Diagnostic contains:", + " Matchers.allOf(", + " (t, s) -> true, Matchers.allOf((t, s) -> false, (t, s) -> true), (t, s) -> false);", + " // BUG: Diagnostic contains:", + " Matchers.anyOf(", + " (t, s) -> true, Matchers.anyOf((t, s) -> false, (t, s) -> true), (t, s) -> false);", + " // BUG: Diagnostic contains:", + " Refaster.anyOf(0, Refaster.anyOf(1, 2), 3);", + " }", + "}") + .doTest(); + } + + @Test + void replacement() { + BugCheckerRefactoringTestHelper.newInstance(AssociativeMethodInvocation.class, getClass()) + .addInputLines( + "A.java", + "import com.google.errorprone.matchers.Matchers;", + "import com.google.errorprone.refaster.Refaster;", + "", + "class A {", + " void m() {", + " Matchers.allOf(Matchers.allOf());", + " Matchers.anyOf(Matchers.anyOf());", + " Refaster.anyOf(Refaster.anyOf());", + "", + " Matchers.allOf(Matchers.allOf((t, s) -> true));", + " Matchers.anyOf(Matchers.anyOf((t, s) -> true));", + " Refaster.anyOf(Refaster.anyOf(0));", + "", + " Matchers.allOf(", + " Matchers.anyOf(),", + " Matchers.allOf((t, s) -> false, (t, s) -> true),", + " Matchers.allOf(),", + " Matchers.anyOf((t, s) -> false));", + " Matchers.anyOf(", + " Matchers.allOf(),", + " Matchers.anyOf((t, s) -> false, (t, s) -> true),", + " Matchers.anyOf(),", + " Matchers.allOf((t, s) -> false));", + " Refaster.anyOf(Matchers.allOf(), Refaster.anyOf(1, 2), Matchers.anyOf());", + " }", + "}") + .addOutputLines( + "A.java", + "import com.google.errorprone.matchers.Matchers;", + "import com.google.errorprone.refaster.Refaster;", + "", + "class A {", + " void m() {", + " Matchers.allOf();", + " Matchers.anyOf();", + " Refaster.anyOf();", + "", + " Matchers.allOf((t, s) -> true);", + " Matchers.anyOf((t, s) -> true);", + " Refaster.anyOf(0);", + "", + " Matchers.allOf(", + " Matchers.anyOf(), (t, s) -> false, (t, s) -> true, Matchers.anyOf((t, s) -> false));", + " Matchers.anyOf(", + " Matchers.allOf(), (t, s) -> false, (t, s) -> true, Matchers.allOf((t, s) -> false));", + " Refaster.anyOf(Matchers.allOf(), 1, 2, Matchers.anyOf());", + " }", + "}") + .doTest(TestMode.TEXT_MATCH); + } +}