From 3dea710275d22c79c2c0b48a8d12da748b168ade Mon Sep 17 00:00:00 2001 From: Patrick Koenig Date: Mon, 12 Apr 2021 19:47:40 -0700 Subject: [PATCH 1/3] Add more Optional methods to ReturnValueIgnored --- .../bugpatterns/ReturnValueIgnored.java | 10 ++- .../bugpatterns/ReturnValueIgnoredTest.java | 70 ++++++++++++++++++- 2 files changed, 77 insertions(+), 3 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 index cf171465eda..ca9970af4df 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/ReturnValueIgnored.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/ReturnValueIgnored.java @@ -204,8 +204,16 @@ private static boolean functionalMethod(ExpressionTree tree, VisitorState state) private static final Matcher OPTIONAL_METHODS = anyOf( staticMethod().onClass("java.util.Optional"), + instanceMethod().onExactClass("java.util.Optional").named("filter"), + instanceMethod().onExactClass("java.util.Optional").named("flatMap"), + instanceMethod().onExactClass("java.util.Optional").named("get"), + instanceMethod().onExactClass("java.util.Optional").named("isEmpty"), instanceMethod().onExactClass("java.util.Optional").named("isPresent"), - instanceMethod().onExactClass("java.util.Optional").named("isEmpty")); + instanceMethod().onExactClass("java.util.Optional").named("map"), + instanceMethod().onExactClass("java.util.Optional").named("or"), + instanceMethod().onExactClass("java.util.Optional").named("orElse"), + instanceMethod().onExactClass("java.util.Optional").named("orElseGet"), + instanceMethod().onExactClass("java.util.Optional").named("orElseThrow")); /** * The return values of {@link java.util.concurrent.TimeUnit} methods should always be checked. diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/ReturnValueIgnoredTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/ReturnValueIgnoredTest.java index 43eeadcbb0d..de565e322d1 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/ReturnValueIgnoredTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/ReturnValueIgnoredTest.java @@ -17,10 +17,13 @@ package com.google.errorprone.bugpatterns; 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; +import static org.junit.Assume.assumeTrue; + /** @author alexeagle@google.com (Alex Eagle) */ @RunWith(JUnit4.class) public class ReturnValueIgnoredTest { @@ -169,9 +172,72 @@ public void optionalInstanceMethods() { " void optional() {", " Optional 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();", + " // 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 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 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 optional = Optional.of(42);", + " // BUG: Diagnostic contains: ReturnValueIgnored", + " optional.isEmpty();", " }", "}") .doTest(); From 1cbf05712bf8b9de0c90d19cb658abb93cfecd8a Mon Sep 17 00:00:00 2001 From: Patrick Koenig Date: Tue, 13 Apr 2021 07:48:20 -0700 Subject: [PATCH 2/3] Comments --- .../bugpatterns/ReturnValueIgnored.java | 42 ++++++++++++++----- 1 file changed, 31 insertions(+), 11 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 index ca9970af4df..f29db88814f 100644 --- 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,16 +205,27 @@ private static boolean functionalMethod(ExpressionTree tree, VisitorState state) private static final Matcher OPTIONAL_METHODS = anyOf( staticMethod().onClass("java.util.Optional"), - instanceMethod().onExactClass("java.util.Optional").named("filter"), - instanceMethod().onExactClass("java.util.Optional").named("flatMap"), - instanceMethod().onExactClass("java.util.Optional").named("get"), - instanceMethod().onExactClass("java.util.Optional").named("isEmpty"), - instanceMethod().onExactClass("java.util.Optional").named("isPresent"), - instanceMethod().onExactClass("java.util.Optional").named("map"), - instanceMethod().onExactClass("java.util.Optional").named("or"), - instanceMethod().onExactClass("java.util.Optional").named("orElse"), - instanceMethod().onExactClass("java.util.Optional").named("orElseGet"), - instanceMethod().onExactClass("java.util.Optional").named("orElseThrow")); + instanceMethod().onExactClass("java.util.Optional") + .namedAnyOf( + "isEmpty", + "isPresent")); + + /** + * The return values of {@link java.util.Optional} static methods and some instance methods should + * always be checked. + */ + private static final Matcher MORE_OPTIONAL_METHODS = + anyOf( + instanceMethod().onExactClass("java.util.Optional") + .namedAnyOf( + "filter", + "flatMap", + "get", + "map", + "or", + "orElse", + "orElseGet", + "orElseThrow")); /** * The return values of {@link java.util.concurrent.TimeUnit} methods should always be checked. @@ -259,8 +271,16 @@ private static boolean functionalMethod(ExpressionTree tree, VisitorState state) .namedAnyOf("containsKey", "containsValue") .withParameters("java.lang.Object")); + private final Matcher 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 specializedMatcher() { - return SPECIALIZED_MATCHER; + return matcher; } } From 6fec586661ac4034d6c743447474f249d72ab21a Mon Sep 17 00:00:00 2001 From: Patrick Koenig Date: Mon, 19 Apr 2021 23:27:15 -0700 Subject: [PATCH 3/3] Match all Optional methods --- .../bugpatterns/ReturnValueIgnored.java | 16 +++------------- .../bugpatterns/ReturnValueIgnoredTest.java | 1 + 2 files changed, 4 insertions(+), 13 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 index f29db88814f..0d2b12f5704 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/ReturnValueIgnored.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/ReturnValueIgnored.java @@ -211,21 +211,11 @@ private static boolean functionalMethod(ExpressionTree tree, VisitorState state) "isPresent")); /** - * The return values of {@link java.util.Optional} static methods and some instance methods should - * always be checked. + * 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 MORE_OPTIONAL_METHODS = - anyOf( - instanceMethod().onExactClass("java.util.Optional") - .namedAnyOf( - "filter", - "flatMap", - "get", - "map", - "or", - "orElse", - "orElseGet", - "orElseThrow")); + anyMethod().onClass("java.util.Optional"); /** * The return values of {@link java.util.concurrent.TimeUnit} methods should always be checked. diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/ReturnValueIgnoredTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/ReturnValueIgnoredTest.java index de565e322d1..27090dd4641 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/ReturnValueIgnoredTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/ReturnValueIgnoredTest.java @@ -177,6 +177,7 @@ public void optionalInstanceMethods() { " 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",