From 6d806cb616f7ae525d662e61461ca366dda82456 Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Thu, 5 Dec 2024 20:49:35 -0800 Subject: [PATCH 1/3] WIP --- .../uber/nullaway/dataflow/AccessPath.java | 19 +++++++++++---- .../com/uber/nullaway/AccessPathsTests.java | 24 +++++++++++++++++++ 2 files changed, 38 insertions(+), 5 deletions(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPath.java b/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPath.java index a75fd858a9..a7b2d0a4e1 100644 --- a/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPath.java +++ b/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPath.java @@ -28,6 +28,8 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.google.errorprone.VisitorState; +import com.google.errorprone.suppliers.Supplier; +import com.google.errorprone.suppliers.Suppliers; import com.google.errorprone.util.ASTHelpers; import com.sun.source.tree.LiteralTree; import com.sun.source.tree.MethodInvocationTree; @@ -75,6 +77,12 @@ */ public final class AccessPath implements MapKey { + private static final Supplier INTEGER_TYPE_SUPPLIER = + Suppliers.typeFromString("java.lang.Integer"); + + private static final Supplier LONG_TYPE_SUPPLIER = + Suppliers.typeFromString("java.lang.Long"); + /** * A prefix added for elements appearing in method invocation APs which represent fields that can * be proven to be class-initialization time constants (i.e. static final fields of a type known @@ -278,14 +286,15 @@ private static Node stripCasts(Node node) { return new NumericMapKey(((LongLiteralNode) argument).getValue()); case METHOD_INVOCATION: MethodAccessNode target = ((MethodInvocationNode) argument).getTarget(); - Node receiver = stripCasts(target.getReceiver()); List arguments = ((MethodInvocationNode) argument).getArguments(); // Check for int/long boxing. if (target.getMethod().getSimpleName().toString().equals("valueOf") - && arguments.size() == 1 - && castToNonNull(receiver.getTree()).getKind().equals(Tree.Kind.IDENTIFIER) - && (receiver.toString().equals("Integer") || receiver.toString().equals("Long"))) { - return argumentToMapKeySpecifier(arguments.get(0), state, apContext); + && arguments.size() == 1) { + Type ownerType = ((Symbol.MethodSymbol) target.getMethod()).owner.type; + if (ASTHelpers.isSameType(ownerType, INTEGER_TYPE_SUPPLIER.get(state), state) + || ASTHelpers.isSameType(ownerType, LONG_TYPE_SUPPLIER.get(state), state)) { + return argumentToMapKeySpecifier(arguments.get(0), state, apContext); + } } // Fine to fallthrough: default: diff --git a/nullaway/src/test/java/com/uber/nullaway/AccessPathsTests.java b/nullaway/src/test/java/com/uber/nullaway/AccessPathsTests.java index 1061e9f7c6..c72172d451 100644 --- a/nullaway/src/test/java/com/uber/nullaway/AccessPathsTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/AccessPathsTests.java @@ -434,4 +434,28 @@ public void testAccessUsingExplicitThis() { "}") .doTest(); } + + @Test + public void testValueOfNoReceiver() { + defaultCompilationHelper + .addSourceLines( + "Foo.java", + "package com.uber;", + "import java.util.HashMap;", + "import java.util.Map;", + "public class Foo {", + " private final Map map = new HashMap<>();", + " static Integer valueOf(int i) { return 0; }", + " public void putThenGetFooValueOf() {", + " map.put(valueOf(10), new Object());", + " // BUG: Diagnostic contains: dereferenced expression map.get(valueOf(10)) is @Nullable", + " map.get(valueOf(10)).toString();", + " }", + " public void putThenGetFooIntegerValueOf() {", + " map.put(Integer.valueOf(10), new Object());", + " map.get(Integer.valueOf(10)).toString();", + " }", + "}") + .doTest(); + } } From 223f8ef630b47292c0223b26b408fc00300622bc Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Fri, 6 Dec 2024 15:54:32 -0800 Subject: [PATCH 2/3] test tweaks --- .../com/uber/nullaway/AccessPathsTests.java | 33 ++++++++++++++++--- 1 file changed, 29 insertions(+), 4 deletions(-) diff --git a/nullaway/src/test/java/com/uber/nullaway/AccessPathsTests.java b/nullaway/src/test/java/com/uber/nullaway/AccessPathsTests.java index c72172d451..9294e71e61 100644 --- a/nullaway/src/test/java/com/uber/nullaway/AccessPathsTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/AccessPathsTests.java @@ -436,7 +436,7 @@ public void testAccessUsingExplicitThis() { } @Test - public void testValueOfNoReceiver() { + public void mapKeysFromValueOf() { defaultCompilationHelper .addSourceLines( "Foo.java", @@ -445,15 +445,40 @@ public void testValueOfNoReceiver() { "import java.util.Map;", "public class Foo {", " private final Map map = new HashMap<>();", + " private final Map longMap = new HashMap<>();", " static Integer valueOf(int i) { return 0; }", + " public void putThenGetIntegerValueOf() {", + " map.put(Integer.valueOf(10), new Object());", + " map.get(Integer.valueOf(10)).toString();", + " }", + " public void putThenGetLongValueOf() {", + " longMap.put(Long.valueOf(10), new Object());", + " longMap.get(Long.valueOf(10)).toString();", + " }", " public void putThenGetFooValueOf() {", " map.put(valueOf(10), new Object());", + " // Unknown valueOf method so we report a warning", " // BUG: Diagnostic contains: dereferenced expression map.get(valueOf(10)) is @Nullable", " map.get(valueOf(10)).toString();", " }", - " public void putThenGetFooIntegerValueOf() {", - " map.put(Integer.valueOf(10), new Object());", - " map.get(Integer.valueOf(10)).toString();", + "}") + .doTest(); + } + + @Test + public void mapKeyFromIntegerValueOfStaticImport() { + defaultCompilationHelper + .addSourceLines( + "Foo.java", + "package com.uber;", + "import java.util.HashMap;", + "import java.util.Map;", + "import static java.lang.Integer.valueOf;", + "public class Foo {", + " private final Map map = new HashMap<>();", + " public void putThenGet() {", + " map.put(valueOf(10), new Object());", + " map.get(valueOf(10)).toString();", " }", "}") .doTest(); From e9a1faba553a98cb9ce4623316f27937f392690b Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Fri, 6 Dec 2024 16:14:30 -0800 Subject: [PATCH 3/3] improve test coverage --- .../src/test/java/com/uber/nullaway/AccessPathsTests.java | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/nullaway/src/test/java/com/uber/nullaway/AccessPathsTests.java b/nullaway/src/test/java/com/uber/nullaway/AccessPathsTests.java index 9294e71e61..354b00cf35 100644 --- a/nullaway/src/test/java/com/uber/nullaway/AccessPathsTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/AccessPathsTests.java @@ -447,6 +447,7 @@ public void mapKeysFromValueOf() { " private final Map map = new HashMap<>();", " private final Map longMap = new HashMap<>();", " static Integer valueOf(int i) { return 0; }", + " static Integer valueOf(int i, int j) { return i+j; }", " public void putThenGetIntegerValueOf() {", " map.put(Integer.valueOf(10), new Object());", " map.get(Integer.valueOf(10)).toString();", @@ -460,6 +461,10 @@ public void mapKeysFromValueOf() { " // Unknown valueOf method so we report a warning", " // BUG: Diagnostic contains: dereferenced expression map.get(valueOf(10)) is @Nullable", " map.get(valueOf(10)).toString();", + " map.put(valueOf(10,20), new Object());", + " // Unknown valueOf method so we report a warning", + " // BUG: Diagnostic contains: dereferenced expression map.get(valueOf(10,20)) is @Nullable", + " map.get(valueOf(10,20)).toString();", " }", "}") .doTest();