From e13bd5f50ce95d891263f5305f12ce75117e10d0 Mon Sep 17 00:00:00 2001 From: jihoon Date: Sat, 15 Aug 2026 00:42:38 +0900 Subject: [PATCH] Check IdentityHashMap method arguments for value classes --- .../bugpatterns/ValueClassIdentity.java | 64 +++++++ .../bugpatterns/ValueClassIdentityTest.java | 161 ++++++++++++++++++ 2 files changed, 225 insertions(+) diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/ValueClassIdentity.java b/core/src/main/java/com/google/errorprone/bugpatterns/ValueClassIdentity.java index dd3454fc468..f4f76d0815d 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/ValueClassIdentity.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/ValueClassIdentity.java @@ -99,6 +99,17 @@ public class ValueClassIdentity extends BugChecker private static final Matcher OBJECT_WAIT_NOTIFY = instanceMethod().onDescendantOf("java.lang.Object").namedAnyOf("wait", "notify", "notifyAll"); + private static final Matcher 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 CACHE_OR_MAP_BUILDER = anyOf( instanceMethod() @@ -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) { @@ -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. + * + *

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 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 matcher, VisitorState state) { while (tree instanceof MethodInvocationTree methodInvocation) { diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/ValueClassIdentityTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/ValueClassIdentityTest.java index 025b304744e..3974344a2c0 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/ValueClassIdentityTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/ValueClassIdentityTest.java @@ -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 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 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 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 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 extends IdentityHashMap {} + + void f(MyMap 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 extends IdentityHashMap { + void get(LocalDate date) {} + } + + void f(MyMap 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 map) { + Object key = LocalDate.now(); + map.put(key, "x"); + } + + // The static receiver type is Map, so IdentityHashMap methods do not match. + void declaredAsMap(Map map) { + map.put(LocalDate.now(), "x"); + } + } + """) + .doTest(); + } + @Test public void identityHashMapNegative() { compilationHelper