Skip to content

Check IdentityHashMap method arguments for value classes #6034

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.

Sign up for GitHub

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

Open
jihxonx wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Check IdentityHashMap method arguments for value classes #6034

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
64 changes: 64 additions & 0 deletions core/src/main/java/com/google/errorprone/bugpatterns/ValueClassIdentity.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,17 @@ public class ValueClassIdentity extends BugChecker
private static final Matcher<ExpressionTree> OBJECT_WAIT_NOTIFY =
instanceMethod().onDescendantOf("java.lang.Object").namedAnyOf("wait", "notify", "notifyAll");

private static final Matcher<ExpressionTree> IDENTITY_HASH_MAP_METHOD =
anyOf(
instanceMethod()
.onDescendantOf("java.util.IdentityHashMap")
.namedAnyOf("containsKey", "containsValue", "get", "remove")
.withParameters("java.lang.Object"),
instanceMethod()
.onDescendantOf("java.util.IdentityHashMap")
.namedAnyOf("put", "putIfAbsent", "remove")
.withParameters("java.lang.Object", "java.lang.Object"));

private static final Matcher<ExpressionTree> CACHE_OR_MAP_BUILDER =
anyOf(
instanceMethod()
Expand Down Expand Up @@ -197,6 +208,10 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState
return Description.NO_MATCH;
}

if (IDENTITY_HASH_MAP_METHOD.matches(tree, state)) {
return checkIdentityHashMapArguments(tree, state);
}

if (CACHE_OR_MAP_BUILDER.matches(tree, state)) {
Type returnType = ASTHelpers.getType(tree);
if (returnType != null && returnType.getTypeArguments().size() >= 2) {
Expand Down Expand Up @@ -224,6 +239,55 @@ && isValueClass(valueType, state)) {
return Description.NO_MATCH;
}

/**
* Checks for value-based classes passed directly as arguments to {@link
* java.util.IdentityHashMap} methods.
*
* <p>Only the static types of the argument expressions are considered. Cases that would require
* dataflow, such as a value-based instance assigned to an {@code Object} local first or a
* receiver declared as {@code Map}, are not handled.
*/
private Description checkIdentityHashMapArguments(MethodInvocationTree tree, VisitorState state) {
List<? extends ExpressionTree> arguments = tree.getArguments();
String methodName = ASTHelpers.getSymbol(tree).getSimpleName().toString();

if (methodName.equals("containsValue")) {
if (arguments.size() != 1) {
return Description.NO_MATCH;
}
ExpressionTree value = arguments.get(0);
if (isValueClass(ASTHelpers.getType(value), state)) {
return buildDescription(value)
.setMessage(
message("passing a value-based class as a value to IdentityHashMap is unsafe."))
.build();
}
return Description.NO_MATCH;
}
if (arguments.isEmpty()) {
return Description.NO_MATCH;
}

ExpressionTree key = arguments.get(0);
if (isValueClass(ASTHelpers.getType(key), state)) {
return buildDescription(key)
.setMessage(message("passing a value-based class as a key to IdentityHashMap is unsafe."))
.build();
}

if (arguments.size() == 2) {
ExpressionTree value = arguments.get(1);
if (isValueClass(ASTHelpers.getType(value), state)) {
return buildDescription(value)
.setMessage(
message("passing a value-based class as a value to IdentityHashMap is unsafe."))
.build();
}
}

return Description.NO_MATCH;
}

private static boolean hasMethodCallInChain(
ExpressionTree tree, Matcher<ExpressionTree> matcher, VisitorState state) {
while (tree instanceof MethodInvocationTree methodInvocation) {
Expand Down
161 changes: 161 additions & 0 deletions core/src/test/java/com/google/errorprone/bugpatterns/ValueClassIdentityTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,167 @@ void f() {
.doTest();
}

@Test
public void identityHashMapArgumentPositive() {
compilationHelper
.addSourceLines(
"Test.java",
"""
import java.time.LocalDate;
import java.util.IdentityHashMap;

class Test {
void f(IdentityHashMap<Object, Object> map, LocalDate date) {
// BUG: Diagnostic contains: ValueClassIdentity
map.put(date, "x");
// BUG: Diagnostic contains: ValueClassIdentity
map.putIfAbsent(date, "x");
// BUG: Diagnostic contains: ValueClassIdentity
map.put("k", date);
// BUG: Diagnostic contains: ValueClassIdentity
map.get(date);
// BUG: Diagnostic contains: ValueClassIdentity
map.containsKey(date);
// BUG: Diagnostic contains: ValueClassIdentity
map.containsValue(date);
// BUG: Diagnostic contains: ValueClassIdentity
map.remove(date);
// BUG: Diagnostic contains: ValueClassIdentity
map.remove("k", date);
}
}
""")
.doTest();
}

@Test
public void identityHashMapArgumentBoxedPositive() {
compilationHelper
.addSourceLines(
"Test.java",
"""
import java.util.IdentityHashMap;

class Test {
void f(IdentityHashMap<Object, Object> map, Integer i) {
// BUG: Diagnostic contains: ValueClassIdentity
map.put(i, "x");
}
}
""")
.doTest();
}

@Test
public void identityHashMapArgumentNegative() {
compilationHelper
.addSourceLines(
"Test.java",
"""
import java.util.IdentityHashMap;

class Test {
void f(IdentityHashMap<Object, Object> map, String s) {
map.put(s, s);
map.get(s);
map.containsValue(s);
map.remove(s);
}
}
""")
.doTest();
}

@Test
public void identityHashMapArgumentNoVisibleConstructionPositive() {
compilationHelper
.addSourceLines(
"Test.java",
"""
import java.time.LocalDate;
import java.util.IdentityHashMap;

class Test {
// The map is not created in this compilation unit, so the construction site
// diagnostic never fires here.
void fromParameter(IdentityHashMap<LocalDate, String> map, LocalDate date) {
// BUG: Diagnostic contains: ValueClassIdentity
map.put(date, "x");
}
}
""")
.doTest();
}

@Test
public void identityHashMapArgumentSubclassPositive() {
compilationHelper
.addSourceLines(
"Test.java",
"""
import java.time.LocalDate;
import java.util.IdentityHashMap;

class Test {
static class MyMap<K, V> extends IdentityHashMap<K, V> {}

void f(MyMap<Object, Object> map, LocalDate date) {
// BUG: Diagnostic contains: ValueClassIdentity
map.put(date, "x");
}
}
""")
.doTest();
}

@Test
public void identityHashMapArgumentUnrelatedOverloadNegative() {
compilationHelper
.addSourceLines(
"Test.java",
"""
import java.time.LocalDate;
import java.util.IdentityHashMap;

class Test {
static class MyMap<K, V> extends IdentityHashMap<K, V> {
void get(LocalDate date) {}
}

void f(MyMap<Object, Object> map) {
map.get(LocalDate.now());
}
}
""")
.doTest();
}

@Test
public void identityHashMapArgumentRequiringDataflowNegative() {
compilationHelper
.addSourceLines(
"Test.java",
"""
import java.time.LocalDate;
import java.util.IdentityHashMap;
import java.util.Map;

class Test {
// Seeing through the Object local would require dataflow.
void widenedLocal(IdentityHashMap<Object, Object> map) {
Object key = LocalDate.now();
map.put(key, "x");
}

// The static receiver type is Map, so IdentityHashMap methods do not match.
void declaredAsMap(Map<Object, Object> map) {
map.put(LocalDate.now(), "x");
}
}
""")
.doTest();
}

@Test
public void identityHashMapNegative() {
compilationHelper
Expand Down