From 0da04a4442ccab887284f25ce6b4f3572dfa2e66 Mon Sep 17 00:00:00 2001 From: Patrick Koenig Date: Mon, 6 Jun 2022 11:28:18 -0700 Subject: [PATCH 1/3] Update AvoidNewHashMapInt to include HashSet --- .../errorprone/AvoidNewHashMapInt.java | 37 ++++++++++++------- .../errorprone/AvoidNewHashMapIntTest.java | 23 ++++++++++-- .../java-coding-guidelines/readme.md | 8 ++-- 3 files changed, 48 insertions(+), 20 deletions(-) diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/AvoidNewHashMapInt.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/AvoidNewHashMapInt.java index 399d47468..713daf764 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/AvoidNewHashMapInt.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/AvoidNewHashMapInt.java @@ -17,7 +17,6 @@ package com.palantir.baseline.errorprone; import com.google.auto.service.AutoService; -import com.google.common.collect.Maps; import com.google.errorprone.BugPattern; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker; @@ -34,30 +33,42 @@ link = "https://github.com/palantir/gradle-baseline#baseline-error-prone-checks", linkType = BugPattern.LinkType.CUSTOM, severity = BugPattern.SeverityLevel.WARNING, - summary = "The HashMap(int) constructor is misleading: once the HashMap reaches 3/4 of the supplied size, it" - + " will double its internal storage array. Instead use Maps.newHashMapWithExpectedSize which behaves as" - + " expected.See" + summary = "The HashMap(int) and HashSet(int) constructors are misleading: once the HashMap/HashSet reaches 3/4" + + " of the supplied size, it resize. Instead use Maps.newHashMapWithExpectedSize or" + + " Sets.newHashSetWithExpextedSize which behaves as expected. See" + " https://github.com/palantir/gradle-baseline/blob/develop/docs/best-practices/java-coding-guidelines/readme.md#avoid-new-HashMap(int)" + " for more information.") public final class AvoidNewHashMapInt extends BugChecker implements BugChecker.NewClassTreeMatcher { private static final long serialVersionUID = 1L; + private static final Matcher NEW_HASH_SET = + MethodMatchers.constructor().forClass("java.util.HashSet").withParameters("int"); private static final Matcher NEW_HASH_MAP = MethodMatchers.constructor().forClass("java.util.HashMap").withParameters("int"); @Override public Description matchNewClass(NewClassTree tree, VisitorState state) { - if (!NEW_HASH_MAP.matches(tree, state)) { - return Description.NO_MATCH; + if (NEW_HASH_SET.matches(tree, state)) { + SuggestedFix.Builder fixBuilder = SuggestedFix.builder(); + String newType = SuggestedFixes.qualifyType(state, fixBuilder, "com.google.common.collect.Sets"); + String arg = state.getSourceForNode(tree.getArguments().get(0)); + String replacement = newType + ".newHashSetWithExpectedSize(" + arg + ")"; + return buildDescription(tree) + .addFix(fixBuilder.replace(tree, replacement).build()) + .build(); } - SuggestedFix.Builder fixBuilder = SuggestedFix.builder(); - String newType = SuggestedFixes.qualifyType(state, fixBuilder, Maps.class.getName()); - String arg = state.getSourceForNode(tree.getArguments().get(0)); - String replacement = newType + ".newHashMapWithExpectedSize(" + arg + ")"; - return buildDescription(tree) - .addFix(fixBuilder.replace(tree, replacement).build()) - .build(); + if (NEW_HASH_MAP.matches(tree, state)) { + SuggestedFix.Builder fixBuilder = SuggestedFix.builder(); + String newType = SuggestedFixes.qualifyType(state, fixBuilder, "com.google.common.collect.Maps"); + String arg = state.getSourceForNode(tree.getArguments().get(0)); + String replacement = newType + ".newHashMapWithExpectedSize(" + arg + ")"; + return buildDescription(tree) + .addFix(fixBuilder.replace(tree, replacement).build()) + .build(); + } + + return Description.NO_MATCH; } } diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/AvoidNewHashMapIntTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/AvoidNewHashMapIntTest.java index 031d0ba1b..6999aa022 100644 --- a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/AvoidNewHashMapIntTest.java +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/AvoidNewHashMapIntTest.java @@ -21,20 +21,37 @@ public final class AvoidNewHashMapIntTest { + @Test + public void testRewriteHashSetConstructor() { + RefactoringValidator.of(AvoidNewHashMapInt.class, getClass()) + .addInputLines( + "Test.java", + "import java.util.HashSet;", + "import java.util.Set;", + "class Test {{ Set set = new HashSet<>(10); }}") + .addOutputLines( + "Test.java", + "import com.google.common.collect.Sets;", + "import java.util.HashSet;", // HACK + "import java.util.Set;", + "class Test {{ Set set = Sets.newHashSetWithExpectedSize(10); }}") + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + } + @Test public void testRewriteHashMapConstructor() { RefactoringValidator.of(AvoidNewHashMapInt.class, getClass()) .addInputLines( "Test.java", - "import java.util.Map;", "import java.util.HashMap;", - "class Test {{ Map map = new HashMap<>(10);}}") + "import java.util.Map;", + "class Test {{ Map map = new HashMap<>(10); }}") .addOutputLines( "Test.java", "import com.google.common.collect.Maps;", "import java.util.HashMap;", // HACK "import java.util.Map;", - "class Test {{ Map map = Maps.newHashMapWithExpectedSize(10);}}") + "class Test {{ Map map = Maps.newHashMapWithExpectedSize(10); }}") .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); } } diff --git a/docs/best-practices/java-coding-guidelines/readme.md b/docs/best-practices/java-coding-guidelines/readme.md index 2fc2c175a..7a0e07c34 100644 --- a/docs/best-practices/java-coding-guidelines/readme.md +++ b/docs/best-practices/java-coding-guidelines/readme.md @@ -651,15 +651,15 @@ non-primitive types such as Java8/Joda-Time ### Avoid new HashMap(int) -Avoid new HashMap(int), use Maps#newHashMapWithExpectedSize(int) instead. +Avoid `new HashMap(int)` and `new HashSet(int)`, use `Maps#newHashMapWithExpectedSize(int)` and `Sets#newHashSetWithExpectedSize(int)` instead. -The behavior of `new HashMap(int)` is misleading -- the parameter represents an +The behavior of `new HashMap(int)`/`new HashSet(int)` is misleading -- the parameter represents an internal size rather than an ability to hold that number of elements. If a HashMap with capacity K receives K elements, it will increase its capacity to 2*K along the way. This is because HashMap doubles its internal storage by 2 once it reaches 75% capacity. -The Guava static method `Maps.newHashMapWithExpectedSize(int)` creates a HashMap which -will not resize if provided the given number of elements. +The Guava static methods `Maps.newHashMapWithExpectedSize(int)` and `Sets#newHashSetWithExpectedSize(int)` creates a +HashMap which will not resize if provided the given number of elements. ## APIs and Interfaces From e9db9cdd6d700771218d59c0e409a12c2798215e Mon Sep 17 00:00:00 2001 From: svc-changelog Date: Mon, 6 Jun 2022 18:30:33 +0000 Subject: [PATCH 2/3] Add generated changelog entries --- changelog/@unreleased/pr-2292.v2.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelog/@unreleased/pr-2292.v2.yml diff --git a/changelog/@unreleased/pr-2292.v2.yml b/changelog/@unreleased/pr-2292.v2.yml new file mode 100644 index 000000000..8aec2ab40 --- /dev/null +++ b/changelog/@unreleased/pr-2292.v2.yml @@ -0,0 +1,5 @@ +type: improvement +improvement: + description: Update `AvoidNewHashMapInt` to warn on uses of `new HashSet(int)`. + links: + - https://github.com/palantir/gradle-baseline/pull/2292 From 953b53320bc0d90f5928551fc097680051f9942d Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Mon, 6 Jun 2022 15:29:46 -0400 Subject: [PATCH 3/3] Update baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/AvoidNewHashMapInt.java Co-authored-by: David Schlosnagle --- .../com/palantir/baseline/errorprone/AvoidNewHashMapInt.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/AvoidNewHashMapInt.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/AvoidNewHashMapInt.java index 713daf764..1f53b8d14 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/AvoidNewHashMapInt.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/AvoidNewHashMapInt.java @@ -35,7 +35,7 @@ severity = BugPattern.SeverityLevel.WARNING, summary = "The HashMap(int) and HashSet(int) constructors are misleading: once the HashMap/HashSet reaches 3/4" + " of the supplied size, it resize. Instead use Maps.newHashMapWithExpectedSize or" - + " Sets.newHashSetWithExpextedSize which behaves as expected. See" + + " Sets.newHashSetWithExpectedSize which behaves as expected. See" + " https://github.com/palantir/gradle-baseline/blob/develop/docs/best-practices/java-coding-guidelines/readme.md#avoid-new-HashMap(int)" + " for more information.") public final class AvoidNewHashMapInt extends BugChecker implements BugChecker.NewClassTreeMatcher {