Suggestions

PicnicSupermarket · Apr 4, 2023 · 0709a49 · 0709a49
1 parent 43262c5
commit 0709a49
Show file tree

Hide file tree

Showing 5 changed files with 111 additions and 6 deletions.
diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/OptionalRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/OptionalRules.java
@@ -3,12 +3,11 @@
 import static com.google.errorprone.refaster.ImportPolicy.STATIC_IMPORT_ALWAYS;
 
 import com.google.common.collect.Streams;
-import com.google.errorprone.matchers.CompileTimeConstantExpressionMatcher;
 import com.google.errorprone.refaster.Refaster;
 import com.google.errorprone.refaster.annotation.AfterTemplate;
 import com.google.errorprone.refaster.annotation.BeforeTemplate;
+import com.google.errorprone.refaster.annotation.Matches;
 import com.google.errorprone.refaster.annotation.MayOptionallyUse;
-import com.google.errorprone.refaster.annotation.NotMatches;
 import com.google.errorprone.refaster.annotation.Placeholder;
 import com.google.errorprone.refaster.annotation.UseImportPolicy;
 import java.util.Comparator;
@@ -20,6 +19,7 @@
 import java.util.stream.Stream;
 import org.jspecify.annotations.Nullable;
 import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation;
+import tech.picnic.errorprone.refaster.matchers.IsMethodInvocationWithTwoOrMoreArgs;
 
 /** Refaster rules related to expressions dealing with {@link Optional}s. */
 @OnlineDocumentation
@@ -160,7 +160,8 @@ Optional<T> after(T input) {
   static final class MapOptionalToBoolean<T> {
     @BeforeTemplate
     boolean before(Optional<T> optional, Function<? super T, Boolean> predicate) {
-      return optional.map(predicate).orElse(Refaster.anyOf(false, Boolean.FALSE));
+      return Refaster.anyOf(
+          optional.map(predicate).orElse(false), optional.map(predicate).orElse(Boolean.FALSE));
     }
 
     @AfterTemplate
@@ -228,7 +229,7 @@ T after(Optional<T> o1, Optional<T> o2) {
    */
   static final class OrElseToOrElseGet<T> {
     @BeforeTemplate
-    T before(Optional<T> o, @NotMatches(CompileTimeConstantExpressionMatcher.class) T value) {
+    T before(Optional<T> o, @Matches(IsMethodInvocationWithTwoOrMoreArgs.class) T value) {
       return o.orElse(value);
     }
 

diff --git a/...ntrib/src/test/resources/tech/picnic/errorprone/refasterrules/OptionalRulesTestInput.java b/...ntrib/src/test/resources/tech/picnic/errorprone/refasterrules/OptionalRulesTestInput.java
@@ -121,6 +121,10 @@ Optional<String> testOptionalMap() {
   }
 
   ImmutableSet<String> testOrElseToOrElseGet() {
-    return ImmutableSet.of(Optional.of("foo").orElse("bar"), Optional.of("foo").orElse(toString()));
+    return ImmutableSet.of(
+        Optional.of("foo").orElse("bar"),
+        Optional.of("foo").orElse(toString()),
+        Optional.of("foo").orElse(String.valueOf(true)),
+        Optional.of("foo").orElse(String.format("%s", "bar")));
   }
 }
diff --git a/...trib/src/test/resources/tech/picnic/errorprone/refasterrules/OptionalRulesTestOutput.java b/...trib/src/test/resources/tech/picnic/errorprone/refasterrules/OptionalRulesTestOutput.java
@@ -115,6 +115,9 @@ Optional<String> testOptionalMap() {
 
   ImmutableSet<String> testOrElseToOrElseGet() {
     return ImmutableSet.of(
-        Optional.of("foo").orElse("bar"), Optional.of("foo").orElseGet(() -> toString()));
+        Optional.of("foo").orElse("bar"),
+        Optional.of("foo").orElse(toString()),
+        Optional.of("foo").orElse(String.valueOf(true)),
+        Optional.of("foo").orElseGet(() -> String.format("%s", "bar")));
   }
 }
diff --git a/...in/java/tech/picnic/errorprone/refaster/matchers/IsMethodInvocationWithTwoOrMoreArgs.java b/...in/java/tech/picnic/errorprone/refaster/matchers/IsMethodInvocationWithTwoOrMoreArgs.java
@@ -0,0 +1,24 @@
+package tech.picnic.errorprone.refaster.matchers;
+
+import static com.google.errorprone.matchers.Matchers.anyMethod;
+import static com.sun.source.tree.Tree.Kind.METHOD_INVOCATION;
+
+import com.google.errorprone.VisitorState;
+import com.google.errorprone.matchers.Matcher;
+import com.sun.source.tree.ExpressionTree;
+import com.sun.source.tree.MethodInvocationTree;
+
+/** A matcher for method invocations with two or more arguments. */
+public final class IsMethodInvocationWithTwoOrMoreArgs implements Matcher<ExpressionTree> {
+  private static final long serialVersionUID = 1L;
+
+  @Override
+  public boolean matches(ExpressionTree expressionTree, VisitorState state) {
+    if (expressionTree.getKind() == METHOD_INVOCATION) {
+      MethodInvocationTree methodInvocationTree = (MethodInvocationTree) expressionTree;
+      return anyMethod().matches(methodInvocationTree.getMethodSelect(), state)
+          && methodInvocationTree.getArguments().size() > 1;
+    }
+    return false;
+  }
+}
diff --git a/...ava/tech/picnic/errorprone/refaster/matchers/IsMethodInvocationWithTwoOrMoreArgsTest.java b/...ava/tech/picnic/errorprone/refaster/matchers/IsMethodInvocationWithTwoOrMoreArgsTest.java
@@ -0,0 +1,73 @@
+package tech.picnic.errorprone.refaster.matchers;
+
+import static com.google.errorprone.BugPattern.SeverityLevel.ERROR;
+
+import com.google.errorprone.BugPattern;
+import com.google.errorprone.CompilationTestHelper;
+import com.google.errorprone.bugpatterns.BugChecker;
+import org.junit.jupiter.api.Test;
+
+final class IsMethodInvocationWithTwoOrMoreArgsTest {
+  @Test
+  void matches() {
+    CompilationTestHelper.newInstance(
+            IsMethodInvocationWithTwoOrMoreArgsTest.MatcherTestChecker.class, getClass())
+        .addSourceLines(
+            "A.java",
+            "class A {",
+            "  String foo(String foo, String bar) {",
+            "    return foo + bar;",
+            "  }",
+            "",
+            "  String foo(String foo) {",
+            "    return foo;",
+            "  }",
+            "",
+            "  String foo() {",
+            "    return \"foo\";",
+            "  }",
+            "",
+            "  Boolean negative1() {",
+            "    return Boolean.TRUE;",
+            "  }",
+            "",
+            "  String negative2() {",
+            "    return \"foo\";",
+            "  }",
+            "",
+            "  String negative3() {",
+            "    return foo(\"foo\");",
+            "  }",
+            "",
+            "  String negative4() {",
+            "    return foo();",
+            "  }",
+            "",
+            "  String positive1() {",
+            "    // BUG: Diagnostic contains:",
+            "    return foo(\"foo\", \"bar\");",
+            "  }",
+            "",
+            "  String positive2() {",
+            "    // BUG: Diagnostic contains:",
+            "    return String.format(\"%s\", \"foo\");",
+            "  }",
+            "}")
+        .doTest();
+  }
+
+  /** A {@link BugChecker} that simply delegates to {@link IsCharacter}. */
+  @BugPattern(
+      summary = "Flags expressions matched by `IsMethodInvocationWithTwoOrMoreArgs`",
+      severity = ERROR)
+  public static final class MatcherTestChecker extends AbstractMatcherTestChecker {
+    private static final long serialVersionUID = 1L;
+
+    // XXX: This is a false positive reported by Checkstyle. See
+    // https://github.com/checkstyle/checkstyle/issues/10161#issuecomment-1242732120.
+    @SuppressWarnings("RedundantModifier")
+    public MatcherTestChecker() {
+      super(new IsMethodInvocationWithTwoOrMoreArgs());
+    }
+  }
+}