Add more Optional methods to ReturnValueIgnored

The return values of almost all `Optional` instance methods, excluding `ifPresent` and `ifPresentOrElse`, should have their return values used. For cases where you may don't care about the return value, it's better to use `isPresent` and `isEmpty`. For example, instead of: ```java optional.orElseThrow(() -> ...); ``` Do: ```java if (optional.isEmpty()) { throw ...; } ``` --- As a motivating example, I had some code that was intended to look like: ```java if (condition) { return getOptional().orElseThrow(...); } return ...; ``` But I forgot the `return`, so the code actually looked like: ```java if (condition) { getOptional().orElseThrow(...); } return ...; ``` Detecting the unused `Optional` value here would have been useful. Fixes #2282 COPYBARA_INTEGRATE_REVIEW=#2282 from pkoenig10:optional 6fec586 PiperOrigin-RevId: 369581827
google · Apr 21, 2021 · 74e8912 · 74e8912
1 parent f1c6c8c
commit 74e8912
Show file tree

Hide file tree

Showing 2 changed files with 87 additions and 5 deletions.
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/ReturnValueIgnored.java b/core/src/main/java/com/google/errorprone/bugpatterns/ReturnValueIgnored.java
@@ -30,6 +30,7 @@
 
 import com.google.common.collect.ImmutableSet;
 import com.google.errorprone.BugPattern;
+import com.google.errorprone.ErrorProneFlags;
 import com.google.errorprone.VisitorState;
 import com.google.errorprone.matchers.Matcher;
 import com.google.errorprone.util.ASTHelpers;
@@ -204,8 +205,14 @@ private static boolean functionalMethod(ExpressionTree tree, VisitorState state)
   private static final Matcher<ExpressionTree> OPTIONAL_METHODS =
       anyOf(
           staticMethod().onClass("java.util.Optional"),
-          instanceMethod().onExactClass("java.util.Optional").named("isPresent"),
-          instanceMethod().onExactClass("java.util.Optional").named("isEmpty"));
+          instanceMethod().onExactClass("java.util.Optional").namedAnyOf("isEmpty", "isPresent"));
+
+  /**
+   * The return values of {@link java.util.Optional} methods should always be checked (except for
+   * void-returning ones, which won't be checked by AbstractReturnValueIgnored).
+   */
+  private static final Matcher<ExpressionTree> MORE_OPTIONAL_METHODS =
+      anyMethod().onClass("java.util.Optional");
 
   /**
    * The return values of {@link java.util.concurrent.TimeUnit} methods should always be checked.
@@ -251,8 +258,16 @@ private static boolean functionalMethod(ExpressionTree tree, VisitorState state)
               .namedAnyOf("containsKey", "containsValue")
               .withParameters("java.lang.Object"));
 
+  private final Matcher<? super ExpressionTree> matcher;
+
+  public ReturnValueIgnored(ErrorProneFlags flags) {
+    boolean checkOptional = flags.getBoolean("ReturnValueIgnored:MoreOptional").orElse(true);
+    this.matcher =
+        checkOptional ? anyOf(SPECIALIZED_MATCHER, MORE_OPTIONAL_METHODS) : SPECIALIZED_MATCHER;
+  }
+
   @Override
   public Matcher<? super ExpressionTree> specializedMatcher() {
-    return SPECIALIZED_MATCHER;
+    return matcher;
   }
 }
diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/ReturnValueIgnoredTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/ReturnValueIgnoredTest.java
@@ -16,7 +16,10 @@
 
 package com.google.errorprone.bugpatterns;
 
+import static org.junit.Assume.assumeTrue;
+
 import com.google.errorprone.CompilationTestHelper;
+import com.google.errorprone.util.RuntimeVersion;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
@@ -169,9 +172,73 @@ public void optionalInstanceMethods() {
             "  void optional() {",
             "    Optional<Integer> optional = Optional.of(42);",
             "    // BUG: Diagnostic contains: ReturnValueIgnored",
-            "    optional.isPresent();",
             "    optional.filter(v -> v > 40);",
-            "    optional.map(v -> Integer.toString(v));",
+            "    // BUG: Diagnostic contains: ReturnValueIgnored",
+            "    optional.flatMap(v -> Optional.of(v + 1));",
+            "    // BUG: Diagnostic contains: ReturnValueIgnored",
+            "    optional.get();",
+            "    optional.ifPresent(v -> {});",
+            "    // BUG: Diagnostic contains: ReturnValueIgnored",
+            "    optional.isPresent();",
+            "    // BUG: Diagnostic contains: ReturnValueIgnored",
+            "    optional.map(v -> v + 1);",
+            "    // BUG: Diagnostic contains: ReturnValueIgnored",
+            "    optional.orElse(40);",
+            "    // BUG: Diagnostic contains: ReturnValueIgnored",
+            "    optional.orElseGet(() -> 40);",
+            "    // BUG: Diagnostic contains: ReturnValueIgnored",
+            "    optional.orElseThrow(() -> new RuntimeException());",
+            "  }",
+            "}")
+        .doTest();
+  }
+
+  @Test
+  public void optionalInstanceMethods_jdk9() {
+    assumeTrue(RuntimeVersion.isAtLeast9());
+    compilationHelper
+        .addSourceLines(
+            "Test.java",
+            "import java.util.Optional;",
+            "class Test {",
+            "  void optional() {",
+            "    Optional<Integer> optional = Optional.of(42);",
+            "    // BUG: Diagnostic contains: ReturnValueIgnored",
+            "    optional.or(() -> Optional.empty());",
+            "  }",
+            "}")
+        .doTest();
+  }
+
+  @Test
+  public void optionalInstanceMethods_jdk10() {
+    assumeTrue(RuntimeVersion.isAtLeast10());
+    compilationHelper
+        .addSourceLines(
+            "Test.java",
+            "import java.util.Optional;",
+            "class Test {",
+            "  void optional() {",
+            "    Optional<Integer> optional = Optional.of(42);",
+            "    // BUG: Diagnostic contains: ReturnValueIgnored",
+            "    optional.orElseThrow();",
+            "  }",
+            "}")
+        .doTest();
+  }
+
+  @Test
+  public void optionalInstanceMethods_jdk11() {
+    assumeTrue(RuntimeVersion.isAtLeast11());
+    compilationHelper
+        .addSourceLines(
+            "Test.java",
+            "import java.util.Optional;",
+            "class Test {",
+            "  void optional() {",
+            "    Optional<Integer> optional = Optional.of(42);",
+            "    // BUG: Diagnostic contains: ReturnValueIgnored",
+            "    optional.isEmpty();",
             "  }",
             "}")
         .doTest();