From 3b9f38e8244b059a4af57b780733adaf3cbf7144 Mon Sep 17 00:00:00 2001 From: iamdanfox Date: Sat, 6 Mar 2021 00:32:49 +0000 Subject: [PATCH] New error-prone rule: PreferImmutableStreamExCollections (#1670) A new error-prone rule `PreferImmutableStreamExCollections` converts the StreamEx `toMap()` -> `toImmutableMap()`, `toImmutableList()` and `toImmutableSet()` --- README.md | 1 + baseline-error-prone/build.gradle | 1 + .../PreferImmutableStreamExCollections.java | 120 ++++++++++++++++++ ...referImmutableStreamExCollectionsTest.java | 96 ++++++++++++++ changelog/@unreleased/pr-1670.v2.yml | 5 + .../BaselineErrorProneExtension.java | 1 + versions.lock | 1 + versions.props | 1 + 8 files changed, 226 insertions(+) create mode 100644 baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreferImmutableStreamExCollections.java create mode 100644 baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/PreferImmutableStreamExCollectionsTest.java create mode 100644 changelog/@unreleased/pr-1670.v2.yml diff --git a/README.md b/README.md index a552ed18f..7bb59065a 100644 --- a/README.md +++ b/README.md @@ -203,6 +203,7 @@ Safe Logging can be found at [github.com/palantir/safe-logging](https://github.c - `CompileTimeConstantViolatesLiskovSubstitution`: Requires consistent application of the `@CompileTimeConstant` annotation to resolve inconsistent validation based on the reference type on which the met is invoked. - `ClassInitializationDeadlock`: Detect type structures which can cause deadlocks initializing classes. - `ConsistentLoggerName`: Ensure Loggers are named consistently. +- `PreferImmutableStreamExCollections`: It's common to use toMap/toSet/toList() as the terminal operation on a stream, but would be extremely surprising to rely on the mutability of these collections. Prefer `toImmutableMap`, `toImmutableSet` and `toImmutableList`. (If the performance overhead of a stream is already acceptable, then the `UnmodifiableFoo` wrapper is likely tolerable). ### Programmatic Application diff --git a/baseline-error-prone/build.gradle b/baseline-error-prone/build.gradle index dd39a11f7..3fb93fc4f 100644 --- a/baseline-error-prone/build.gradle +++ b/baseline-error-prone/build.gradle @@ -26,6 +26,7 @@ dependencies { testCompileOnly 'org.immutables:value::annotations' testImplementation 'org.junit.jupiter:junit-jupiter' testRuntimeOnly 'org.junit.jupiter:junit-jupiter-migrationsupport' + testRuntimeOnly 'one.util:streamex' annotationProcessor 'com.google.auto.service:auto-service' annotationProcessor 'org.immutables:value' diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreferImmutableStreamExCollections.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreferImmutableStreamExCollections.java new file mode 100644 index 000000000..4734f532e --- /dev/null +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreferImmutableStreamExCollections.java @@ -0,0 +1,120 @@ +/* + * (c) Copyright 2019 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.fixes.SuggestedFixes; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.matchers.Matcher; +import com.google.errorprone.matchers.method.MethodMatchers; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.MethodInvocationTree; +import java.util.Collections; + +@AutoService(BugChecker.class) +@BugPattern( + name = "PreferImmutableStreamExCollections", + link = "https://github.com/palantir/gradle-baseline#baseline-error-prone-checks", + linkType = BugPattern.LinkType.CUSTOM, + severity = SeverityLevel.WARNING, + summary = "Prefer immutable/unmodifable collections wherever possible because they are inherently threadsafe " + + "and easier to reason about when passed between different functions." + + " If you really want a mutable output then explicitly suppress this check.") +public final class PreferImmutableStreamExCollections extends BugChecker + implements BugChecker.MethodInvocationTreeMatcher { + + private static final long serialVersionUID = 1L; + + private static final Matcher STREAMEX_TO_MAP = MethodMatchers.instanceMethod() + .onExactClass("one.util.streamex.EntryStream") + .named("toMap") + .withParameters(Collections.emptyList()); + + private static final Matcher STREAMEX_TO_SET = MethodMatchers.instanceMethod() + .onDescendantOf("one.util.streamex.AbstractStreamEx") + .named("toSet") + .withParameters(Collections.emptyList()); + + private static final Matcher STREAMEX_TO_LIST = MethodMatchers.instanceMethod() + .onDescendantOf("one.util.streamex.AbstractStreamEx") + .named("toList") + .withParameters(Collections.emptyList()); + + private static final Matcher STREAMEX_COLLECT = MethodMatchers.instanceMethod() + .onDescendantOf("one.util.streamex.AbstractStreamEx") + .named("collect"); + + private static final Matcher COLLECT_TO_SET = MethodMatchers.staticMethod() + .onClass("java.util.stream.Collectors") + .named("toSet") + .withParameters(Collections.emptyList()); + + private static final Matcher COLLECT_TO_LIST = MethodMatchers.staticMethod() + .onClass("java.util.stream.Collectors") + .named("toList") + .withParameters(Collections.emptyList()); + + @Override + public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { + if (STREAMEX_TO_MAP.matches(tree, state)) { + return buildDescription(tree) + .addFix(SuggestedFixes.renameMethodInvocation(tree, "toImmutableMap", state)) + .build(); + } + + if (STREAMEX_TO_SET.matches(tree, state)) { + return buildDescription(tree) + .addFix(SuggestedFixes.renameMethodInvocation(tree, "toImmutableSet", state)) + .build(); + } + + if (STREAMEX_TO_LIST.matches(tree, state)) { + return buildDescription(tree) + .addFix(SuggestedFixes.renameMethodInvocation(tree, "toImmutableList", state)) + .build(); + } + + if (STREAMEX_COLLECT.matches(tree, state) && tree.getArguments().size() == 1) { + ExpressionTree argument = tree.getArguments().get(0); + + if (COLLECT_TO_SET.matches(argument, state)) { + return buildDescription(tree) + .addFix(SuggestedFix.builder() + .delete(argument) + .merge(SuggestedFixes.renameMethodInvocation(tree, "toImmutableSet", state)) + .build()) + .build(); + } + + if (COLLECT_TO_LIST.matches(argument, state)) { + return buildDescription(tree) + .addFix(SuggestedFix.builder() + .delete(argument) + .merge(SuggestedFixes.renameMethodInvocation(tree, "toImmutableList", state)) + .build()) + .build(); + } + } + + return Description.NO_MATCH; + } +} diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/PreferImmutableStreamExCollectionsTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/PreferImmutableStreamExCollectionsTest.java new file mode 100644 index 000000000..e1af0c0fb --- /dev/null +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/PreferImmutableStreamExCollectionsTest.java @@ -0,0 +1,96 @@ +/* + * (c) Copyright 2021 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.BugCheckerRefactoringTestHelper; +import org.junit.jupiter.api.Test; + +public class PreferImmutableStreamExCollectionsTest { + + @Test + void toMap() { + fix().addInputLines( + "Test.java", + "import one.util.streamex.EntryStream;", + "import java.util.Map;", + "public class Test {", + " Map map = EntryStream.of(Map.of(\"hello\", \"world\")).toMap();", + " EntryStream entryStream = EntryStream.of(Map.of(\"hello\", \"world\"));", + " Map entryStream2 = entryStream.toMap();", + "}") + .addOutputLines( + "Test.java", + "import one.util.streamex.EntryStream;", + "import java.util.Map;", + "public class Test {", + " Map map = EntryStream.of(Map.of(\"hello\", \"world\")).toImmutableMap();", + " EntryStream entryStream = EntryStream.of(Map.of(\"hello\", \"world\"));", + " Map entryStream2 = entryStream.toImmutableMap();", + "}") + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + } + + @Test + void toSet() { + fix().addInputLines( + "Test.java", + "import one.util.streamex.StreamEx;", + "import java.util.stream.Collectors;", + "import java.util.Set;", + "public class Test {", + " Set s = StreamEx.of(\"Hello\").toSet();", + " Set s2 = StreamEx.of(\"Hello\").collect(Collectors.toSet());", + "}") + .addOutputLines( + "Test.java", + "import one.util.streamex.StreamEx;", + "import java.util.stream.Collectors;", + "import java.util.Set;", + "public class Test {", + " Set s = StreamEx.of(\"Hello\").toImmutableSet();", + " Set s2 = StreamEx.of(\"Hello\").toImmutableSet();", + "}") + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + } + + @Test + void toList() { + fix().addInputLines( + "Test.java", + "import one.util.streamex.StreamEx;", + "import java.util.List;", + "import static java.util.stream.Collectors.toList;", + "public class Test {", + " List s = StreamEx.of(\"Hello\").toList();", + " List s2 = StreamEx.of(\"Hello\").collect(toList());", + "}") + .addOutputLines( + "Test.java", + "import one.util.streamex.StreamEx;", + "import java.util.List;", + "import static java.util.stream.Collectors.toList;", + "public class Test {", + " List s = StreamEx.of(\"Hello\").toImmutableList();", + " List s2 = StreamEx.of(\"Hello\").toImmutableList();", + "}") + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + } + + private RefactoringValidator fix() { + return RefactoringValidator.of(new PreferImmutableStreamExCollections(), getClass()); + } +} diff --git a/changelog/@unreleased/pr-1670.v2.yml b/changelog/@unreleased/pr-1670.v2.yml new file mode 100644 index 000000000..6c5b30117 --- /dev/null +++ b/changelog/@unreleased/pr-1670.v2.yml @@ -0,0 +1,5 @@ +type: improvement +improvement: + description: A new error-prone rule `PreferImmutableStreamExCollections` converts the StreamEx `toMap()` -> `toImmutableMap()`, `toImmutableList()` and `toImmutableSet()` + links: + - https://github.com/palantir/gradle-baseline/pull/1670 diff --git a/gradle-baseline-java/src/main/groovy/com/palantir/baseline/extensions/BaselineErrorProneExtension.java b/gradle-baseline-java/src/main/groovy/com/palantir/baseline/extensions/BaselineErrorProneExtension.java index 17959a41d..57abea4cd 100644 --- a/gradle-baseline-java/src/main/groovy/com/palantir/baseline/extensions/BaselineErrorProneExtension.java +++ b/gradle-baseline-java/src/main/groovy/com/palantir/baseline/extensions/BaselineErrorProneExtension.java @@ -71,6 +71,7 @@ public class BaselineErrorProneExtension { // TODO(ckozak): re-enable pending scala check // "ThrowSpecificity", "UnsafeGaugeRegistration", + "PreferImmutableStreamExCollections", // Built-in checks "ArrayEquals", diff --git a/versions.lock b/versions.lock index 6c0b56018..e0fa1985c 100644 --- a/versions.lock +++ b/versions.lock @@ -105,6 +105,7 @@ javax.activation:javax.activation-api:1.2.0 (1 constraints: 450a28bf) javax.xml.bind:jaxb-api:2.3.1 (1 constraints: c0069559) junit:junit-dep:4.11 (1 constraints: ba1063b3) net.lingala.zip4j:zip4j:1.3.2 (1 constraints: 0805fb35) +one.util:streamex:0.7.3 (1 constraints: 0c050336) org.apiguardian:apiguardian-api:1.1.0 (6 constraints: 18697c5a) org.jooq:jooq:3.14.8 (1 constraints: 4205493b) org.junit:junit-bom:5.7.1 (7 constraints: 6377a0d6) diff --git a/versions.props b/versions.props index 782722cf0..c43c96425 100644 --- a/versions.props +++ b/versions.props @@ -32,6 +32,7 @@ org.assertj:assertj-core = 3.19.0 org.hamcrest:hamcrest-core = 2.2 org.junit.jupiter:* = 5.7.1 org.mockito:* = 3.8.0 +one.util:streamex = 0.7.3 # dependency-upgrader:OFF # Don't upgrade, we will remove this in a future release.