-
Notifications
You must be signed in to change notification settings - Fork 39
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Introduce
AssociativeMethodInvocation check
- Loading branch information
1 parent
b273502
commit 4961ead
Showing
2 changed files
with
209 additions
and
0 deletions.
There are no files selected for viewing
99 changes: 99 additions & 0 deletions
99
...contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AssociativeMethodInvocation.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,99 @@ | ||
| 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 java.util.stream.Collectors.joining; | ||
| 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.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. | ||
| * | ||
| * <p>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 you can flatten the list of operands", | ||
| link = BUG_PATTERNS_BASE_URL + "AssociativeOperatorUsage", | ||
| linkType = CUSTOM, | ||
| severity = SUGGESTION, | ||
| tags = SIMPLIFICATION) | ||
| public final class AssociativeMethodInvocation extends BugChecker | ||
| implements MethodInvocationTreeMatcher { | ||
| private static final long serialVersionUID = 1L; | ||
| private static final Supplier<Type> ITERABLE = Suppliers.typeFromClass(Iterable.class); | ||
| private static final ImmutableList<Matcher<ExpressionTree>> ASSOCIATIVE_OPERATIONS = | ||
| ImmutableList.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")); | ||
|
|
||
| @Override | ||
| public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { | ||
| for (Matcher<ExpressionTree> 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) | ||
| : unwrapMethodInvocation(invocation, state)); | ||
| } | ||
| } | ||
|
|
||
| return fix.isEmpty() ? Description.NO_MATCH : describeMatch(tree, fix.build()); | ||
| } | ||
| } | ||
|
|
||
| return Description.NO_MATCH; | ||
| } | ||
|
|
||
| // XXX: Test this code with comments. | ||
| private static SuggestedFix unwrapMethodInvocation( | ||
| MethodInvocationTree tree, VisitorState state) { | ||
| return SuggestedFix.replace( | ||
| tree, | ||
| tree.getArguments().stream() | ||
| .map(arg -> SourceCode.treeToString(arg, state)) | ||
| .collect(joining(", "))); | ||
| } | ||
|
|
||
| private static Matcher<MethodInvocationTree> hasArgumentOfType(Supplier<Type> type) { | ||
| return (tree, state) -> | ||
| tree.getArguments().stream() | ||
| .anyMatch(arg -> ASTHelpers.isSubtype(ASTHelpers.getType(arg), type.get(state), state)); | ||
| } | ||
| } |
110 changes: 110 additions & 0 deletions
110
...rib/src/test/java/tech/picnic/errorprone/bugpatterns/AssociativeMethodInvocationTest.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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); | ||
| } | ||
| } |