Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update AvoidNewHashMapInt to include HashSet #2292

Merged
merged 3 commits into from
Jun 6, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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.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 {

private static final long serialVersionUID = 1L;

private static final Matcher<ExpressionTree> NEW_HASH_SET =
MethodMatchers.constructor().forClass("java.util.HashSet").withParameters("int");
private static final Matcher<ExpressionTree> 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;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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<Integer> 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<Integer> 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<Integer, Integer> map = new HashMap<>(10);}}")
"import java.util.Map;",
"class Test {{ Map<Integer, Integer> 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<Integer, Integer> map = Maps.newHashMapWithExpectedSize(10);}}")
"class Test {{ Map<Integer, Integer> map = Maps.newHashMapWithExpectedSize(10); }}")
.doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH);
}
}
5 changes: 5 additions & 0 deletions changelog/@unreleased/pr-2292.v2.yml
Original file line number Diff line number Diff line change
@@ -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
8 changes: 4 additions & 4 deletions docs/best-practices/java-coding-guidelines/readme.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down