Skip to content

Commit

Permalink
Add more Optional methods to ReturnValueIgnored
Browse files Browse the repository at this point in the history
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
  • Loading branch information
pkoenig10 authored and Error Prone Team committed Apr 21, 2021
1 parent f1c6c8c commit 74e8912
Show file tree
Hide file tree
Showing 2 changed files with 87 additions and 5 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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();
Expand Down

0 comments on commit 74e8912

Please sign in to comment.