From fca52062f4b4065e2caafb11e14720a422c34c14 Mon Sep 17 00:00:00 2001 From: David Schlosnagle Date: Mon, 11 Nov 2024 22:00:32 -0500 Subject: [PATCH 1/4] StreamOptionalGetWithoutFilter --- .../StreamOptionalGetWithoutFilter.java | 86 +++++++++++++++++ .../StreamOptionalGetWithoutFilterTest.java | 96 +++++++++++++++++++ 2 files changed, 182 insertions(+) create mode 100644 baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/StreamOptionalGetWithoutFilter.java create mode 100644 baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/StreamOptionalGetWithoutFilterTest.java diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/StreamOptionalGetWithoutFilter.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/StreamOptionalGetWithoutFilter.java new file mode 100644 index 000000000..7a25ea77f --- /dev/null +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/StreamOptionalGetWithoutFilter.java @@ -0,0 +1,86 @@ +/* + * (c) Copyright 2024 Palantir Technologies Inc. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.palantir.baseline.errorprone; + +import com.google.auto.service.AutoService; +import com.google.errorprone.BugPattern; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.matchers.Matcher; +import com.google.errorprone.matchers.Matchers; +import com.google.errorprone.util.ASTHelpers; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.MethodInvocationTree; +import java.util.Optional; +import java.util.function.Function; +import java.util.function.Predicate; +import java.util.stream.Stream; + +@AutoService(BugChecker.class) +@BugPattern( + link = "https://github.com/palantir/gradle-baseline#baseline-error-prone-checks", + linkType = BugPattern.LinkType.CUSTOM, + severity = BugPattern.SeverityLevel.WARNING, + summary = "Stream> should call filter(Optional::isPresent) before map(Optional::get)", + explanation = "Calling map(Optional::get) on a Stream> without first calling " + + "filter(Optional::isPresent) can cause NoSuchElementException.") +public final class StreamOptionalGetWithoutFilter extends BugChecker implements BugChecker.MethodInvocationTreeMatcher { + + private static final Matcher OPTIONAL_GET_METHOD = Matchers.staticMethod() + .onClass(Optional.class.getName()) + .named("get") + .withNoParameters(); + + private static final Matcher STREAM_MAP_METHOD = Matchers.instanceMethod() + .onDescendantOf(Stream.class.getName()) + .named("map") + .withParameters(Function.class.getName()); + + private static final Matcher STREAM_FILTER_METHOD = Matchers.instanceMethod() + .onDescendantOf(Stream.class.getName()) + .named("filter") + .withParameters(Predicate.class.getName()); + + @Override + public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { + if (STREAM_MAP_METHOD.matches(tree, state) && containsOptionalGet(tree, state)) { + ExpressionTree receiver = ASTHelpers.getReceiver(tree); + if (receiver != null && !hasFilterIsPresent(receiver, state)) { + return describeMatch(tree); + } + } + return Description.NO_MATCH; + } + + private static boolean containsOptionalGet(MethodInvocationTree tree, VisitorState state) { + return tree.getArguments().stream().anyMatch(arg -> OPTIONAL_GET_METHOD.matches(arg, state)); + } + + private static boolean hasFilterIsPresent(ExpressionTree receiver, VisitorState state) { + if (receiver instanceof MethodInvocationTree) { + MethodInvocationTree methodInvocationTree = (MethodInvocationTree) receiver; + if (STREAM_FILTER_METHOD.matches(methodInvocationTree, state)) { + return methodInvocationTree.getArguments().stream() + .anyMatch(arg -> "Optional::isPresent".equals(state.getSourceForNode(arg))); + } else { + return hasFilterIsPresent(methodInvocationTree.getMethodSelect(), state); + } + } + return false; + } +} diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/StreamOptionalGetWithoutFilterTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/StreamOptionalGetWithoutFilterTest.java new file mode 100644 index 000000000..85bdc88b4 --- /dev/null +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/StreamOptionalGetWithoutFilterTest.java @@ -0,0 +1,96 @@ +/* + * (c) Copyright 2024 Palantir Technologies Inc. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.palantir.baseline.errorprone; + +import com.google.errorprone.CompilationTestHelper; +import org.junit.jupiter.api.Test; + +class StreamOptionalGetWithoutFilterTest { + @Test + public void testPositive() { + compile() + .addSourceLines( + "Test.java", + "import java.util.Optional;", + "import java.util.stream.Stream;", + "public class Test {", + " void test1() {", + " Stream> stream = Stream.of(Optional.of(\"foo\"), Optional.empty());", + " // BUG: Diagnostic contains \"Stream> should call " + + "filter(Optional::isPresent) before map(Optional::get)\"", + " stream.map(Optional::get)", + " .forEach(System.out::println);", + " }", + " void test2() {", + " Stream> stream = Stream.of(Optional.of(1), Optional.empty());", + " // BUG: Diagnostic contains \"Stream> should call " + + "filter(Optional::isPresent) before map(Optional::get)\"", + " stream.map(opt -> opt.get())", + " .forEach(System.out::println);", + " }", + " void test3() {", + " Stream> stream = Stream.of(Optional.of(1.0), Optional.empty());", + " // BUG: Diagnostic contains \"Stream> should call " + + "filter(Optional::isPresent) before map(Optional::get)\"", + " stream.map(Optional::get)", + " .forEach(System.out::println);", + " }", + "}") + .doTest(); + } + + @Test + public void testNegative() { + compile() + .addSourceLines( + "Test.java", + "import java.util.Optional;", + "import java.util.stream.Stream;", + "public class Test {", + " void test1() {", + " Stream> stream = Stream.of(Optional.of(\"foo\"), Optional.empty());", + " stream.filter(Optional::isPresent)", + " .map(Optional::get)", + " .forEach(System.out::println); // No bug", + " }", + " void test2() {", + " Stream> stream = Stream.of(Optional.of(1), Optional.empty());", + " stream.filter(opt -> opt.isPresent())", + " .map(opt -> opt.get())", + " .forEach(System.out::println); // No bug", + " }", + " void test3() {", + " Stream> stream = Stream.of(Optional.of(1.0), Optional.empty());", + " stream.filter(opt -> opt.isPresent())", + " .map(Optional::get)", + " .forEach(System.out::println); // No bug", + " }", + " void test4() {", + " Stream> stream = Stream.of(Optional.of(\"foo\"), Optional.empty());", + " stream.filter(opt -> opt.isPresent())", + " .map(opt -> opt.get())", + " .filter(s -> s.length() > 2)", + " .forEach(System.out::println); // No bug", + " }", + "}") + .doTest(); + } + + private CompilationTestHelper compile() { + return CompilationTestHelper.newInstance(StreamOptionalGetWithoutFilter.class, getClass()); + } +} From e7dfbda1dd6e0ae5b20085e663015573989360d9 Mon Sep 17 00:00:00 2001 From: David Schlosnagle Date: Mon, 11 Nov 2024 22:09:39 -0500 Subject: [PATCH 2/4] cleanup --- .../StreamOptionalGetWithoutFilter.java | 29 +++++++++++-------- 1 file changed, 17 insertions(+), 12 deletions(-) diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/StreamOptionalGetWithoutFilter.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/StreamOptionalGetWithoutFilter.java index 7a25ea77f..13c1e5d6f 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/StreamOptionalGetWithoutFilter.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/StreamOptionalGetWithoutFilter.java @@ -18,8 +18,10 @@ import com.google.auto.service.AutoService; import com.google.errorprone.BugPattern; +import com.google.errorprone.BugPattern.SeverityLevel; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker; +import com.google.errorprone.matchers.ChildMultiMatcher.MatchType; import com.google.errorprone.matchers.Description; import com.google.errorprone.matchers.Matcher; import com.google.errorprone.matchers.Matchers; @@ -35,7 +37,7 @@ @BugPattern( link = "https://github.com/palantir/gradle-baseline#baseline-error-prone-checks", linkType = BugPattern.LinkType.CUSTOM, - severity = BugPattern.SeverityLevel.WARNING, + severity = SeverityLevel.WARNING, summary = "Stream> should call filter(Optional::isPresent) before map(Optional::get)", explanation = "Calling map(Optional::get) on a Stream> without first calling " + "filter(Optional::isPresent) can cause NoSuchElementException.") @@ -56,9 +58,20 @@ public final class StreamOptionalGetWithoutFilter extends BugChecker implements .named("filter") .withParameters(Predicate.class.getName()); + private static final Matcher OPTIONAL_IS_PRESENT_METHOD = Matchers.instanceMethod() + .onDescendantOf(Optional.class.getName()) + .named("isPresent") + .withNoParameters(); + + private static final Matcher STREAM_FILTER_IS_PRESENT = + Matchers.methodInvocation(STREAM_FILTER_METHOD, MatchType.LAST, OPTIONAL_IS_PRESENT_METHOD); + + private static final Matcher STREAM_MAP_OPTIONAL_GET = + Matchers.methodInvocation(STREAM_MAP_METHOD, MatchType.LAST, OPTIONAL_GET_METHOD); + @Override public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { - if (STREAM_MAP_METHOD.matches(tree, state) && containsOptionalGet(tree, state)) { + if (STREAM_MAP_OPTIONAL_GET.matches(tree, state)) { ExpressionTree receiver = ASTHelpers.getReceiver(tree); if (receiver != null && !hasFilterIsPresent(receiver, state)) { return describeMatch(tree); @@ -67,19 +80,11 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState return Description.NO_MATCH; } - private static boolean containsOptionalGet(MethodInvocationTree tree, VisitorState state) { - return tree.getArguments().stream().anyMatch(arg -> OPTIONAL_GET_METHOD.matches(arg, state)); - } - private static boolean hasFilterIsPresent(ExpressionTree receiver, VisitorState state) { if (receiver instanceof MethodInvocationTree) { MethodInvocationTree methodInvocationTree = (MethodInvocationTree) receiver; - if (STREAM_FILTER_METHOD.matches(methodInvocationTree, state)) { - return methodInvocationTree.getArguments().stream() - .anyMatch(arg -> "Optional::isPresent".equals(state.getSourceForNode(arg))); - } else { - return hasFilterIsPresent(methodInvocationTree.getMethodSelect(), state); - } + return STREAM_FILTER_IS_PRESENT.matches(methodInvocationTree, state) + || hasFilterIsPresent(methodInvocationTree.getMethodSelect(), state); } return false; } From 904cde819cf7f8abea490d4b51ef3d579958047c Mon Sep 17 00:00:00 2001 From: David Schlosnagle Date: Mon, 11 Nov 2024 22:18:47 -0500 Subject: [PATCH 3/4] simplify --- .../StreamOptionalGetWithoutFilterTest.java | 57 ++++++------------- 1 file changed, 18 insertions(+), 39 deletions(-) diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/StreamOptionalGetWithoutFilterTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/StreamOptionalGetWithoutFilterTest.java index 85bdc88b4..5ea550b51 100644 --- a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/StreamOptionalGetWithoutFilterTest.java +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/StreamOptionalGetWithoutFilterTest.java @@ -28,26 +28,13 @@ public void testPositive() { "import java.util.Optional;", "import java.util.stream.Stream;", "public class Test {", - " void test1() {", - " Stream> stream = Stream.of(Optional.of(\"foo\"), Optional.empty());", - " // BUG: Diagnostic contains \"Stream> should call " - + "filter(Optional::isPresent) before map(Optional::get)\"", - " stream.map(Optional::get)", - " .forEach(System.out::println);", + " Stream test1(Stream> stream) {", + " // BUG: Diagnostic contains filter(Optional::isPresent) before map(Optional::get)", + " return stream.map(Optional::get);", " }", - " void test2() {", - " Stream> stream = Stream.of(Optional.of(1), Optional.empty());", - " // BUG: Diagnostic contains \"Stream> should call " - + "filter(Optional::isPresent) before map(Optional::get)\"", - " stream.map(opt -> opt.get())", - " .forEach(System.out::println);", - " }", - " void test3() {", - " Stream> stream = Stream.of(Optional.of(1.0), Optional.empty());", - " // BUG: Diagnostic contains \"Stream> should call " - + "filter(Optional::isPresent) before map(Optional::get)\"", - " stream.map(Optional::get)", - " .forEach(System.out::println);", + " Stream test2(Stream> stream) {", + " // BUG: Diagnostic contains filter(Optional::isPresent) before map(Optional::get)", + " return stream.map(opt -> opt.get());", " }", "}") .doTest(); @@ -61,30 +48,22 @@ public void testNegative() { "import java.util.Optional;", "import java.util.stream.Stream;", "public class Test {", - " void test1() {", - " Stream> stream = Stream.of(Optional.of(\"foo\"), Optional.empty());", - " stream.filter(Optional::isPresent)", - " .map(Optional::get)", - " .forEach(System.out::println); // No bug", + " Stream test1(Stream> stream) {", + " return stream.filter(Optional::isPresent)", + " .map(Optional::get);", " }", - " void test2() {", - " Stream> stream = Stream.of(Optional.of(1), Optional.empty());", - " stream.filter(opt -> opt.isPresent())", - " .map(opt -> opt.get())", - " .forEach(System.out::println); // No bug", + " Stream test2(Stream> stream) {", + " return stream.filter(opt -> opt.isPresent())", + " .map(opt -> opt.get());", " }", - " void test3() {", - " Stream> stream = Stream.of(Optional.of(1.0), Optional.empty());", - " stream.filter(opt -> opt.isPresent())", - " .map(Optional::get)", - " .forEach(System.out::println); // No bug", + " Stream test3(Stream> stream) {", + " return stream.filter(opt -> opt.isPresent())", + " .map(Optional::get);", " }", - " void test4() {", - " Stream> stream = Stream.of(Optional.of(\"foo\"), Optional.empty());", - " stream.filter(opt -> opt.isPresent())", + " Stream test4(Stream> stream) {", + " return stream.filter(opt -> opt.isPresent())", " .map(opt -> opt.get())", - " .filter(s -> s.length() > 2)", - " .forEach(System.out::println); // No bug", + " .filter(s -> s.length() > 2);", " }", "}") .doTest(); From 97c9b2cf95b8333d891e2d0e0f873a15c58e9ff7 Mon Sep 17 00:00:00 2001 From: David Schlosnagle Date: Mon, 11 Nov 2024 22:54:30 -0500 Subject: [PATCH 4/4] orElseThrow --- .../StreamOptionalGetWithoutFilter.java | 12 ++++++++++-- .../StreamOptionalGetWithoutFilterTest.java | 18 ++++++++++++++++++ 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/StreamOptionalGetWithoutFilter.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/StreamOptionalGetWithoutFilter.java index 13c1e5d6f..4386d614f 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/StreamOptionalGetWithoutFilter.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/StreamOptionalGetWithoutFilter.java @@ -38,7 +38,8 @@ link = "https://github.com/palantir/gradle-baseline#baseline-error-prone-checks", linkType = BugPattern.LinkType.CUSTOM, severity = SeverityLevel.WARNING, - summary = "Stream> should call filter(Optional::isPresent) before map(Optional::get)", + summary = "Stream> should call filter(Optional::isPresent) before " + + "map(Optional::get) or map(Optional::orElseThrow)", explanation = "Calling map(Optional::get) on a Stream> without first calling " + "filter(Optional::isPresent) can cause NoSuchElementException.") public final class StreamOptionalGetWithoutFilter extends BugChecker implements BugChecker.MethodInvocationTreeMatcher { @@ -47,6 +48,13 @@ public final class StreamOptionalGetWithoutFilter extends BugChecker implements .onClass(Optional.class.getName()) .named("get") .withNoParameters(); + private static final Matcher OPTIONAL_OR_ELSE_THROW_METHOD = Matchers.staticMethod() + .onClass(Optional.class.getName()) + .named("orElseThrow") + .withNoParameters(); + + private static final Matcher OPTIONAL_UNWRAP_METHOD = + Matchers.anyOf(OPTIONAL_GET_METHOD, OPTIONAL_OR_ELSE_THROW_METHOD); private static final Matcher STREAM_MAP_METHOD = Matchers.instanceMethod() .onDescendantOf(Stream.class.getName()) @@ -67,7 +75,7 @@ public final class StreamOptionalGetWithoutFilter extends BugChecker implements Matchers.methodInvocation(STREAM_FILTER_METHOD, MatchType.LAST, OPTIONAL_IS_PRESENT_METHOD); private static final Matcher STREAM_MAP_OPTIONAL_GET = - Matchers.methodInvocation(STREAM_MAP_METHOD, MatchType.LAST, OPTIONAL_GET_METHOD); + Matchers.methodInvocation(STREAM_MAP_METHOD, MatchType.LAST, OPTIONAL_UNWRAP_METHOD); @Override public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/StreamOptionalGetWithoutFilterTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/StreamOptionalGetWithoutFilterTest.java index 5ea550b51..0ec3ff69c 100644 --- a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/StreamOptionalGetWithoutFilterTest.java +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/StreamOptionalGetWithoutFilterTest.java @@ -36,6 +36,16 @@ public void testPositive() { " // BUG: Diagnostic contains filter(Optional::isPresent) before map(Optional::get)", " return stream.map(opt -> opt.get());", " }", + " Stream test3(Stream> stream) {", + " // BUG: Diagnostic contains filter(Optional::isPresent) before map(Optional::get)" + + " or map(Optional::orElseThrow)", + " return stream.map(opt -> opt.orElseThrow());", + " }", + " Stream test4(Stream> stream) {", + " // BUG: Diagnostic contains filter(Optional::isPresent) before map(Optional::get)" + + " or map(Optional::orElseThrow)", + " return stream.map(Optional::orElseThrow);", + " }", "}") .doTest(); } @@ -65,6 +75,14 @@ public void testNegative() { " .map(opt -> opt.get())", " .filter(s -> s.length() > 2);", " }", + " Stream test5(Stream> stream) {", + " return stream.filter(opt -> opt.isPresent())", + " .map(Optional::orElseThrow);", + " }", + " Stream test6(Stream> stream) {", + " return stream.filter(opt -> opt.isPresent())", + " .map(opt -> opt.orElseThrow());", + " }", "}") .doTest(); }