From 4a0f0c8ac1d53f05ba5a8ba8967450edb189e99d Mon Sep 17 00:00:00 2001 From: Pritham Marupaka Date: Wed, 5 Apr 2023 18:04:39 -0400 Subject: [PATCH 1/8] wip --- .../errorprone/SortedStreamFirstElement.java | 97 +++++++++++++ .../SortedStreamFirstElementTest.java | 133 ++++++++++++++++++ 2 files changed, 230 insertions(+) create mode 100644 baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/SortedStreamFirstElement.java create mode 100644 baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/SortedStreamFirstElementTest.java diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/SortedStreamFirstElement.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/SortedStreamFirstElement.java new file mode 100644 index 000000000..5637e08fb --- /dev/null +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/SortedStreamFirstElement.java @@ -0,0 +1,97 @@ +/* + * (c) Copyright 2023 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.BugPattern.SeverityLevel; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker; +import com.google.errorprone.fixes.SuggestedFix; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.matchers.Matcher; +import com.google.errorprone.matchers.Matchers; +import com.google.errorprone.matchers.method.MethodMatchers; +import com.google.errorprone.util.ASTHelpers; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.MethodInvocationTree; +import java.util.Comparator; +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 = SeverityLevel.SUGGESTION, + summary = "TODO") +public final class SortedStreamFirstElement extends BugChecker implements BugChecker.MethodInvocationTreeMatcher { + + private static final Matcher STREAM_SORTED_FIND_FIRST_MATCHER = MethodMatchers.instanceMethod() + .onDescendantOf(Stream.class.getName()) + .namedAnyOf("findFirst") + .withNoParameters(); + + private static final Matcher STREAM_SORTED_NO_PARAMS_MATCHER = MethodMatchers.instanceMethod() + .onDescendantOf(Stream.class.getName()) + .named("sorted") + .withNoParameters(); + + private static final Matcher STREAM_SORTED_WITH_COMPARATOR_MATCHER = MethodMatchers.instanceMethod() + .onDescendantOf(Stream.class.getName()) + .named("sorted") + .withParameters(Comparator.class.getName()); + + @Override + public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { + if (!STREAM_SORTED_FIND_FIRST_MATCHER.matches(tree, state)) { + return Description.NO_MATCH; + } + + ExpressionTree sorted = ASTHelpers.getReceiver(tree); + if (sorted == null) { + return Description.NO_MATCH; + } + MethodInvocationTree sortedTree = (MethodInvocationTree) sorted; + ExpressionTree stream = ASTHelpers.getReceiver(sorted); + if (stream == null) { + return Description.NO_MATCH; + } + + if (Matchers.receiverOfInvocation(STREAM_SORTED_NO_PARAMS_MATCHER).matches(tree, state)) { + return describeMatch( + tree, + SuggestedFix.builder() + .replace(tree, state.getSourceForNode(stream) + ".min(Comparator.naturalOrder())") + .addImport(Comparator.class.getCanonicalName()) + .build()); + } else if (Matchers.receiverOfInvocation(STREAM_SORTED_WITH_COMPARATOR_MATCHER) + .matches(tree, state)) { + return describeMatch( + tree, + SuggestedFix.builder() + .replace( + tree, + state.getSourceForNode(stream) + ".min(" + + state.getSourceForNode( + sortedTree.getArguments().get(0)) + ")") + .addImport(Comparator.class.getCanonicalName()) + .build()); + } + + return Description.NO_MATCH; + } +} diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/SortedStreamFirstElementTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/SortedStreamFirstElementTest.java new file mode 100644 index 000000000..1c0755363 --- /dev/null +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/SortedStreamFirstElementTest.java @@ -0,0 +1,133 @@ +/* + * (c) Copyright 2023 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 java.util.Comparator; +import java.util.Optional; +import java.util.stream.Stream; +import org.junit.jupiter.api.Test; + +public class SortedStreamFirstElementTest { + + @Test + public void test_basic() { + fix().addInputLines( + "TestBasic.java", + "import " + Optional.class.getCanonicalName() + ";", + "import " + Stream.class.getCanonicalName() + ";", + "class TestBasic {", + " public Optional basic(Stream s) {", + " return s.sorted().findFirst();", + " }", + "}") + .addOutputLines( + "TestBasic.java", + "import " + Comparator.class.getCanonicalName() + ";", + "import " + Optional.class.getCanonicalName() + ";", + "import " + Stream.class.getCanonicalName() + ";", + "class TestBasic {", + " public Optional basic(Stream s) {", + " return s.min(Comparator.naturalOrder());", + " }", + "}") + .doTest(); + } + + @Test + public void test_comparator_already_imported() { + fix().addInputLines( + "TestComparatorAlreadyImported.java", + "import " + Comparator.class.getCanonicalName() + ";", + "import " + Optional.class.getCanonicalName() + ";", + "import " + Stream.class.getCanonicalName() + ";", + "class TestComparatorAlreadyImported {", + " public Optional f(Stream s) {", + " return s.sorted().findFirst();", + " }", + " public Optional g(Stream s) {", + " return s.min(Comparator.naturalOrder());", + " }", + "}") + .addOutputLines( + "TestComparatorAlreadyImported.java", + "import " + Comparator.class.getCanonicalName() + ";", + "import " + Optional.class.getCanonicalName() + ";", + "import " + Stream.class.getCanonicalName() + ";", + "class TestComparatorAlreadyImported {", + " public Optional f(Stream s) {", + " return s.min(Comparator.naturalOrder());", + " }", + " public Optional g(Stream s) {", + " return s.min(Comparator.naturalOrder());", + " }", + "}") + .doTest(); + } + + @Test + public void test_templated() { + fix().addInputLines( + "TestBasic.java", + "import " + Optional.class.getCanonicalName() + ";", + "import " + Stream.class.getCanonicalName() + ";", + "class TestBasic> {", + " public Optional basic(Stream s) {", + " return s.sorted().findFirst();", + " }", + "}") + .addOutputLines( + "TestBasic.java", + "import " + Comparator.class.getCanonicalName() + ";", + "import " + Optional.class.getCanonicalName() + ";", + "import " + Stream.class.getCanonicalName() + ";", + "class TestBasic> {", + " public Optional basic(Stream s) {", + " return s.min(Comparator.naturalOrder());", + " }", + "}") + .doTest(); + } + + @Test + public void test_comparator() { + fix().addInputLines( + "TestComparator.java", + "import " + Comparator.class.getCanonicalName() + ";", + "import " + Optional.class.getCanonicalName() + ";", + "import " + Stream.class.getCanonicalName() + ";", + "class TestComparator {", + " public Optional f(Stream s, Comparator c) {", + " return s.sorted(c).findFirst();", + " }", + "}") + .addOutputLines( + "TestComparator.java", + "import " + Comparator.class.getCanonicalName() + ";", + "import " + Optional.class.getCanonicalName() + ";", + "import " + Stream.class.getCanonicalName() + ";", + "class TestComparator {", + " public Optional f(Stream s, Comparator c) {", + " return s.min(c);", + " }", + "}") + .doTest(); + } + + private RefactoringValidator fix() { + return RefactoringValidator.of(SortedStreamFirstElement.class, getClass()); + } +} From 24ec4093e38f1593d5edc4ccc2b9640bd311876d Mon Sep 17 00:00:00 2001 From: Pritham Marupaka Date: Thu, 13 Apr 2023 11:55:18 -0400 Subject: [PATCH 2/8] add comment --- .../palantir/baseline/errorprone/SortedStreamFirstElement.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/SortedStreamFirstElement.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/SortedStreamFirstElement.java index 5637e08fb..372f2333b 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/SortedStreamFirstElement.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/SortedStreamFirstElement.java @@ -63,11 +63,13 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState ExpressionTree sorted = ASTHelpers.getReceiver(tree); if (sorted == null) { + // Not expected. return Description.NO_MATCH; } MethodInvocationTree sortedTree = (MethodInvocationTree) sorted; ExpressionTree stream = ASTHelpers.getReceiver(sorted); if (stream == null) { + // Not expected. return Description.NO_MATCH; } From 27f3ecb9c545254ef1bc1f2a84503c9d0d3f2c3a Mon Sep 17 00:00:00 2001 From: Pritham Marupaka Date: Fri, 14 Apr 2023 13:11:48 -0400 Subject: [PATCH 3/8] update comment --- .../palantir/baseline/errorprone/SortedStreamFirstElement.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/SortedStreamFirstElement.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/SortedStreamFirstElement.java index 372f2333b..6c2aa3690 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/SortedStreamFirstElement.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/SortedStreamFirstElement.java @@ -37,7 +37,8 @@ link = "https://github.com/palantir/gradle-baseline#baseline-error-prone-checks", linkType = BugPattern.LinkType.CUSTOM, severity = SeverityLevel.SUGGESTION, - summary = "TODO") + summary = "Using Stream::min is more efficient than finding the first element of the sorted stream. " + + "Stream::min performs a linear scan through the stream to find the smallest element.") public final class SortedStreamFirstElement extends BugChecker implements BugChecker.MethodInvocationTreeMatcher { private static final Matcher STREAM_SORTED_FIND_FIRST_MATCHER = MethodMatchers.instanceMethod() From 7a9325971039f134aa022f410632a9343d992e99 Mon Sep 17 00:00:00 2001 From: svc-changelog Date: Fri, 14 Apr 2023 17:14:14 +0000 Subject: [PATCH 4/8] Add generated changelog entries --- changelog/@unreleased/pr-2555.v2.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelog/@unreleased/pr-2555.v2.yml diff --git a/changelog/@unreleased/pr-2555.v2.yml b/changelog/@unreleased/pr-2555.v2.yml new file mode 100644 index 000000000..73e99a44b --- /dev/null +++ b/changelog/@unreleased/pr-2555.v2.yml @@ -0,0 +1,5 @@ +type: feature +feature: + description: Add check replacing `stream.sorted().findFirst()` with `stream.min()` + links: + - https://github.com/palantir/gradle-baseline/pull/2555 From c287681209ea22686d2c8f1da30963e430b51dc9 Mon Sep 17 00:00:00 2001 From: Pritham Marupaka Date: Wed, 31 May 2023 17:27:02 -0400 Subject: [PATCH 5/8] review changes --- .../errorprone/SortedStreamFirstElement.java | 43 +++++++++++-------- .../tasks/CheckClassUniquenessLockTask.java | 1 + 2 files changed, 27 insertions(+), 17 deletions(-) diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/SortedStreamFirstElement.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/SortedStreamFirstElement.java index 6c2aa3690..e5585a382 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/SortedStreamFirstElement.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/SortedStreamFirstElement.java @@ -29,6 +29,7 @@ import com.google.errorprone.util.ASTHelpers; import com.sun.source.tree.ExpressionTree; import com.sun.source.tree.MethodInvocationTree; +import com.sun.tools.javac.tree.JCTree.JCMethodInvocation; import java.util.Comparator; import java.util.stream.Stream; @@ -41,24 +42,30 @@ + "Stream::min performs a linear scan through the stream to find the smallest element.") public final class SortedStreamFirstElement extends BugChecker implements BugChecker.MethodInvocationTreeMatcher { - private static final Matcher STREAM_SORTED_FIND_FIRST_MATCHER = MethodMatchers.instanceMethod() + private static final Matcher STREAM_FIND_FIRST_MATCHER = MethodMatchers.instanceMethod() .onDescendantOf(Stream.class.getName()) - .namedAnyOf("findFirst") + .named("findFirst") .withNoParameters(); - private static final Matcher STREAM_SORTED_NO_PARAMS_MATCHER = MethodMatchers.instanceMethod() - .onDescendantOf(Stream.class.getName()) - .named("sorted") - .withNoParameters(); + private static final Matcher RECEIVER_OF_STREAM_SORTED_NO_PARAMS_MATCHER = + Matchers.receiverOfInvocation(MethodMatchers.instanceMethod() + .onDescendantOf(Stream.class.getName()) + .named("sorted") + .withNoParameters()); - private static final Matcher STREAM_SORTED_WITH_COMPARATOR_MATCHER = MethodMatchers.instanceMethod() - .onDescendantOf(Stream.class.getName()) - .named("sorted") - .withParameters(Comparator.class.getName()); + private static final Matcher RECEIVER_OF_STREAM_SORTED_WITH_COMPARATOR_MATCHER = + Matchers.receiverOfInvocation(MethodMatchers.instanceMethod() + .onDescendantOf(Stream.class.getName()) + .named("sorted") + .withParameters(Comparator.class.getName())); + private static final Matcher MATCHER = Matchers.allOf( + STREAM_FIND_FIRST_MATCHER, + Matchers.anyOf( + RECEIVER_OF_STREAM_SORTED_NO_PARAMS_MATCHER, RECEIVER_OF_STREAM_SORTED_WITH_COMPARATOR_MATCHER)); @Override public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { - if (!STREAM_SORTED_FIND_FIRST_MATCHER.matches(tree, state)) { + if (!MATCHER.matches(tree, state)) { return Description.NO_MATCH; } @@ -74,24 +81,26 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState return Description.NO_MATCH; } - if (Matchers.receiverOfInvocation(STREAM_SORTED_NO_PARAMS_MATCHER).matches(tree, state)) { + if (RECEIVER_OF_STREAM_SORTED_NO_PARAMS_MATCHER.matches(tree, state)) { return describeMatch( tree, SuggestedFix.builder() - .replace(tree, state.getSourceForNode(stream) + ".min(Comparator.naturalOrder())") + .replace( + ((JCMethodInvocation) tree).getStartPosition(), + state.getEndPosition(tree), + state.getSourceForNode(stream) + ".min(Comparator.naturalOrder())") .addImport(Comparator.class.getCanonicalName()) .build()); - } else if (Matchers.receiverOfInvocation(STREAM_SORTED_WITH_COMPARATOR_MATCHER) - .matches(tree, state)) { + } else if (RECEIVER_OF_STREAM_SORTED_WITH_COMPARATOR_MATCHER.matches(tree, state)) { return describeMatch( tree, SuggestedFix.builder() .replace( - tree, + ((JCMethodInvocation) tree).getStartPosition(), + state.getEndPosition(tree), state.getSourceForNode(stream) + ".min(" + state.getSourceForNode( sortedTree.getArguments().get(0)) + ")") - .addImport(Comparator.class.getCanonicalName()) .build()); } diff --git a/gradle-baseline-java/src/main/groovy/com/palantir/baseline/tasks/CheckClassUniquenessLockTask.java b/gradle-baseline-java/src/main/groovy/com/palantir/baseline/tasks/CheckClassUniquenessLockTask.java index 55d8230b8..fef3a8af2 100644 --- a/gradle-baseline-java/src/main/groovy/com/palantir/baseline/tasks/CheckClassUniquenessLockTask.java +++ b/gradle-baseline-java/src/main/groovy/com/palantir/baseline/tasks/CheckClassUniquenessLockTask.java @@ -30,6 +30,7 @@ import java.util.Objects; import java.util.Optional; import java.util.Set; +import java.util.function.Function; import java.util.stream.Collectors; import java.util.stream.Stream; import org.gradle.api.DefaultTask; From cb54f1726d139cd339177e13de24f84eb80c0284 Mon Sep 17 00:00:00 2001 From: Pritham Marupaka Date: Wed, 31 May 2023 17:28:08 -0400 Subject: [PATCH 6/8] format --- .../palantir/baseline/tasks/CheckClassUniquenessLockTask.java | 1 - 1 file changed, 1 deletion(-) diff --git a/gradle-baseline-java/src/main/groovy/com/palantir/baseline/tasks/CheckClassUniquenessLockTask.java b/gradle-baseline-java/src/main/groovy/com/palantir/baseline/tasks/CheckClassUniquenessLockTask.java index fef3a8af2..55d8230b8 100644 --- a/gradle-baseline-java/src/main/groovy/com/palantir/baseline/tasks/CheckClassUniquenessLockTask.java +++ b/gradle-baseline-java/src/main/groovy/com/palantir/baseline/tasks/CheckClassUniquenessLockTask.java @@ -30,7 +30,6 @@ import java.util.Objects; import java.util.Optional; import java.util.Set; -import java.util.function.Function; import java.util.stream.Collectors; import java.util.stream.Stream; import org.gradle.api.DefaultTask; From 0910447ef5085144442876b621a9e083e68989bb Mon Sep 17 00:00:00 2001 From: Pritham Marupaka Date: Wed, 31 May 2023 17:28:59 -0400 Subject: [PATCH 7/8] add nl --- .../palantir/baseline/errorprone/SortedStreamFirstElement.java | 1 + 1 file changed, 1 insertion(+) diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/SortedStreamFirstElement.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/SortedStreamFirstElement.java index e5585a382..4736f9300 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/SortedStreamFirstElement.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/SortedStreamFirstElement.java @@ -58,6 +58,7 @@ public final class SortedStreamFirstElement extends BugChecker implements BugChe .onDescendantOf(Stream.class.getName()) .named("sorted") .withParameters(Comparator.class.getName())); + private static final Matcher MATCHER = Matchers.allOf( STREAM_FIND_FIRST_MATCHER, Matchers.anyOf( From 7541a254c7d2ac8fb3d842c9d792db75b72118e6 Mon Sep 17 00:00:00 2001 From: Pritham Marupaka Date: Wed, 31 May 2023 17:31:26 -0400 Subject: [PATCH 8/8] move getStartPosition into private static method --- .../baseline/errorprone/SortedStreamFirstElement.java | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/SortedStreamFirstElement.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/SortedStreamFirstElement.java index 4736f9300..059b6cac7 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/SortedStreamFirstElement.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/SortedStreamFirstElement.java @@ -29,6 +29,7 @@ import com.google.errorprone.util.ASTHelpers; import com.sun.source.tree.ExpressionTree; import com.sun.source.tree.MethodInvocationTree; +import com.sun.source.tree.Tree; import com.sun.tools.javac.tree.JCTree.JCMethodInvocation; import java.util.Comparator; import java.util.stream.Stream; @@ -87,7 +88,7 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState tree, SuggestedFix.builder() .replace( - ((JCMethodInvocation) tree).getStartPosition(), + getStartPosition(tree), state.getEndPosition(tree), state.getSourceForNode(stream) + ".min(Comparator.naturalOrder())") .addImport(Comparator.class.getCanonicalName()) @@ -97,7 +98,7 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState tree, SuggestedFix.builder() .replace( - ((JCMethodInvocation) tree).getStartPosition(), + getStartPosition(tree), state.getEndPosition(tree), state.getSourceForNode(stream) + ".min(" + state.getSourceForNode( @@ -107,4 +108,8 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState return Description.NO_MATCH; } + + private static int getStartPosition(Tree tree) { + return ((JCMethodInvocation) tree).getStartPosition(); + } }