Don't additionally suggest var unused = ... or removing the stateme…

…nt for an `assertThat(...)` call with a missing `isTrue()` call. `isTrue()` is essentially guaranteed to be correct, so the additional suggestion is just noise and an invitation to accidentally apply the wrong fix. PiperOrigin-RevId: 520035528
google · Mar 29, 2023 · 9eed554 · 9eed554
1 parent e432465
commit 9eed554
Show file tree

Hide file tree

Showing 2 changed files with 7 additions and 2 deletions.
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/AbstractReturnValueIgnored.java b/core/src/main/java/com/google/errorprone/bugpatterns/AbstractReturnValueIgnored.java
@@ -302,6 +302,7 @@ final ImmutableList<Fix> fixesAtCallSite(ExpressionTree invocationTree, VisitorS
         }
       }
     }
+    boolean considerBlanketFixes = true;
     if (resultType != null && resultType.getKind() == TypeKind.BOOLEAN) {
       // Fix by calling either assertThat(...).isTrue() or verify(...).
       if (state.errorProneOptions().isTestOnlyTarget()) {
@@ -328,6 +329,7 @@ && matchingMethods(
                 state.getTypes())
             .anyMatch(m -> true)) {
       fixes.put("Assert that the result is true", postfixWith(invocationTree, ".isTrue()"));
+      considerBlanketFixes = false;
     }
     if (identifierExpr != null
         && symbol != null
@@ -344,7 +346,8 @@ && matchingMethods(
      * this a constructor call or build() call?"
      */
     if (parent.getKind() == EXPRESSION_STATEMENT
-        && !constantExpressions.constantExpression(invocationTree, state).isPresent()) {
+        && !constantExpressions.constantExpression(invocationTree, state).isPresent()
+        && considerBlanketFixes) {
       ImmutableSet<String> identifiersInScope =
           findAllIdents(state).stream().map(v -> v.name.toString()).collect(toImmutableSet());
       concat(Stream.of("unused"), range(2, 10).mapToObj(i -> "unused" + i))
@@ -358,7 +361,7 @@ && matchingMethods(
                       "Suppress error by assigning to a variable",
                       prefixWith(parent, format("var %s = ", n))));
     }
-    if (parent.getKind() == EXPRESSION_STATEMENT) {
+    if (parent.getKind() == EXPRESSION_STATEMENT && considerBlanketFixes) {
       if (constantExpressions.constantExpression(invocationTree, state).isPresent()) {
         fixes.put("Delete call", delete(parent));
       } else {

diff --git a/...oogle/errorprone/bugpatterns/checkreturnvalue/CheckReturnValueWellKnownLibrariesTest.java b/...oogle/errorprone/bugpatterns/checkreturnvalue/CheckReturnValueWellKnownLibrariesTest.java
@@ -19,6 +19,7 @@
 import com.google.auto.value.processor.AutoBuilderProcessor;
 import com.google.auto.value.processor.AutoValueProcessor;
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Iterables;
 import com.google.errorprone.BugCheckerRefactoringTestHelper;
 import com.google.errorprone.CompilationTestHelper;
 import com.google.errorprone.bugpatterns.CheckReturnValue;
@@ -273,6 +274,7 @@ public void truthMissingIsTrue() {
             "    assertThat(b).isTrue();",
             "  }",
             "}")
+        .setFixChooser(Iterables::getOnlyElement)
         .doTest();
   }