From 5e939659bb03d5364ff3d2a730ff45ae428816d4 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Sun, 2 Apr 2023 17:29:51 +0200 Subject: [PATCH] WIP! --- .../bugpatterns/IdentityConversion.java | 2 + .../VacuousZeroArgMethodInvocation.java | 84 +++++++++++++++++++ .../bugpatterns/IdentityConversionTest.java | 5 ++ .../bugpatterns/RefasterAnyOfUsage.java | 6 ++ 4 files changed, 97 insertions(+) create mode 100644 error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/VacuousZeroArgMethodInvocation.java diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/IdentityConversion.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/IdentityConversion.java index b35331f0..6f4c0343 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/IdentityConversion.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/IdentityConversion.java @@ -32,6 +32,7 @@ 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.util.ASTHelpers; import com.google.errorprone.util.ASTHelpers.TargetType; import com.sun.source.tree.ExpressionTree; @@ -84,6 +85,7 @@ public final class IdentityConversion extends BugChecker implements MethodInvoca ImmutableTable.class.getCanonicalName()) .named("copyOf"), staticMethod().onClass(Matchers.class.getCanonicalName()).namedAnyOf("allOf", "anyOf"), + staticMethod().onClass(Refaster.class.getCanonicalName()).namedAnyOf("anyOf"), staticMethod().onClass("reactor.adapter.rxjava.RxJava2Adapter"), staticMethod() .onClass("reactor.core.publisher.Flux") diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/VacuousZeroArgMethodInvocation.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/VacuousZeroArgMethodInvocation.java new file mode 100644 index 00000000..0c192160 --- /dev/null +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/VacuousZeroArgMethodInvocation.java @@ -0,0 +1,84 @@ +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.anyOf; +import static com.google.errorprone.matchers.Matchers.instanceMethod; +import static com.google.errorprone.matchers.method.MethodMatchers.staticMethod; +import static tech.picnic.errorprone.utils.Documentation.BUG_PATTERNS_BASE_URL; + +import com.google.auto.service.AutoService; +import com.google.common.collect.ImmutableCollection; +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.matchers.Description; +import com.google.errorprone.matchers.Matcher; +import com.google.errorprone.refaster.Refaster; +import com.google.errorprone.util.ASTHelpers; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.MethodInvocationTree; +import tech.picnic.errorprone.utils.SourceCode; + +/** + * A {@link BugChecker} that flags method invocations without arguments, in cases where such + * invocation amounts to a no-op. + */ +@AutoService(BugChecker.class) +@BugPattern( + summary = "Avoid no-op invocations of varargs methods without arguments", + link = BUG_PATTERNS_BASE_URL + "VacuousZeroArgMethodInvocation", + linkType = CUSTOM, + severity = SUGGESTION, + tags = SIMPLIFICATION) +public final class VacuousZeroArgMethodInvocation extends BugChecker + implements MethodInvocationTreeMatcher { + private static final long serialVersionUID = 1L; + private static final Matcher FLAGGED_INSTANCE_METHOD = + anyOf( + instanceMethod() + .onDescendantOf(ImmutableCollection.Builder.class.getCanonicalName()) + .named("add"), + instanceMethod() + .onDescendantOfAny( + "com.google.errorprone.BugCheckerRefactoringTestHelper", + "com.google.errorprone.CompilationTestHelper") + .named("setArgs")); + private static final Matcher FLAGGED_STATIC_METHOD = + staticMethod().onClass(Refaster.class.getCanonicalName()).named("anyOf"); + + /** Instantiates a new {@link VacuousZeroArgMethodInvocation} instance. */ + public VacuousZeroArgMethodInvocation() {} + + @Override + public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { + if (!tree.getArguments().isEmpty()) { + return Description.NO_MATCH; + } + + if (FLAGGED_INSTANCE_METHOD.matches(tree, state)) { + ExpressionTree receiver = ASTHelpers.getReceiver(tree); + if (receiver == null) { + // XXX: Test this using an `ImmutableCollection.Builder` subtype in which we call `add()`. + // XXX: This call can be removed completely, unless the result is used in some way (by being + // dereferenced, or passed as an argument to another method). + return describeMatch(tree); + } + + // XXX: This logic is also used in `NonEmptyMono`; worthy of a `SourceCode` utility method? + return describeMatch( + tree, SuggestedFix.replace(tree, SourceCode.treeToString(receiver, state))); + } + + if (FLAGGED_STATIC_METHOD.matches(tree, state)) { + // XXX: Drop the method invocation if its result is not used in some way (by being + // dereferenced, or passed as an argument to another method). + return describeMatch(tree); + } + + return Description.NO_MATCH; + } +} diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/IdentityConversionTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/IdentityConversionTest.java index 631acdad..e35c70f3 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/IdentityConversionTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/IdentityConversionTest.java @@ -28,6 +28,7 @@ final class IdentityConversionTest { "import com.google.common.collect.ImmutableTable;", "import com.google.errorprone.matchers.Matcher;", "import com.google.errorprone.matchers.Matchers;", + "import com.google.errorprone.refaster.Refaster;", "import reactor.adapter.rxjava.RxJava2Adapter;", "import reactor.core.publisher.Flux;", "import reactor.core.publisher.Mono;", @@ -157,6 +158,10 @@ final class IdentityConversionTest { " Matcher anyOf2 = Matchers.anyOf(instanceMethod(), staticMethod());", "", " // BUG: Diagnostic contains:", + " String refasterBranch1 = Refaster.anyOf(\"foo\");", + " String refasterBranch2 = Refaster.anyOf(\"foo\", \"bar\");", + "", + " // BUG: Diagnostic contains:", " Flux flux1 = Flux.just(1).flatMap(e -> RxJava2Adapter.fluxToFlowable(Flux.just(2)));", "", " // BUG: Diagnostic contains:", diff --git a/error-prone-guidelines/src/main/java/tech/picnic/errorprone/guidelines/bugpatterns/RefasterAnyOfUsage.java b/error-prone-guidelines/src/main/java/tech/picnic/errorprone/guidelines/bugpatterns/RefasterAnyOfUsage.java index c0e9ebdc..bc412215 100644 --- a/error-prone-guidelines/src/main/java/tech/picnic/errorprone/guidelines/bugpatterns/RefasterAnyOfUsage.java +++ b/error-prone-guidelines/src/main/java/tech/picnic/errorprone/guidelines/bugpatterns/RefasterAnyOfUsage.java @@ -25,6 +25,12 @@ import tech.picnic.errorprone.utils.SourceCode; *

Note that this logic can't be implemented as a Refaster rule, as the {@link Refaster} class is * treated specially. */ +// XXX: The single-argument case is also covered by the `IdentityConversion` check. Consider +// replacing this check with an `VacuousZeroArgMethodInvocation` check that flags a wider range of +// vargars method invocations without arguments. Examples of other methods to include +// `ImmutableList.Builder#add` and `CompilationTestHelper#setArgs`. This check should suggest +// dropping the method invocation if (a) it's contained in an `ExpressionStatementTree` or (b) it's +// return type matches the receiver type. @AutoService(BugChecker.class) @BugPattern( summary = "`Refaster#anyOf` should be passed at least two parameters",