From fdd2b550ea51630c1d60142f87afb37ecd24f27c Mon Sep 17 00:00:00 2001
From: Manu Sridharan <msridhar@gmail.com>
Date: Tue, 6 Feb 2024 16:10:31 -0800
Subject: [PATCH] Fix handling of static imports from subclasses (#904)

Fixes #764
---
 .../main/java/com/uber/nullaway/NullAway.java | 22 +++++++-------
 .../com/uber/nullaway/NullAwayCoreTests.java  | 29 +++++++++++++++++++
 2 files changed, 40 insertions(+), 11 deletions(-)

diff --git a/nullaway/src/main/java/com/uber/nullaway/NullAway.java b/nullaway/src/main/java/com/uber/nullaway/NullAway.java
index b437623b29..5902b20275 100644
--- a/nullaway/src/main/java/com/uber/nullaway/NullAway.java
+++ b/nullaway/src/main/java/com/uber/nullaway/NullAway.java
@@ -400,29 +400,29 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState
     if (!withinAnnotatedCode(state)) {
       return Description.NO_MATCH;
     }
-    Symbol.MethodSymbol methodSymbol = getSymbolForMethodInvocation(tree, state);
+    Symbol.MethodSymbol methodSymbol = getSymbolForMethodInvocation(tree);
     handler.onMatchMethodInvocation(this, tree, state, methodSymbol);
     // assuming this list does not include the receiver
     List<? extends ExpressionTree> actualParams = tree.getArguments();
     return handleInvocation(tree, state, methodSymbol, actualParams);
   }
 
-  private static Symbol.MethodSymbol getSymbolForMethodInvocation(
-      MethodInvocationTree tree, VisitorState state) {
+  private static Symbol.MethodSymbol getSymbolForMethodInvocation(MethodInvocationTree tree) {
     Symbol.MethodSymbol methodSymbol = ASTHelpers.getSymbol(tree);
     Verify.verify(methodSymbol != null, "not expecting unresolved method here");
+    // In certain cases, we need to get the base symbol for the method rather than the symbol
+    // attached to the call.
     // For interface methods, if the method is an implicit method corresponding to a method from
-    // java.lang.Object, use the symbol for the java.lang.Object method instead.  We do this to
+    // java.lang.Object, the base symbol is for the java.lang.Object method.  We need this to
     // properly treat the method as unannotated, which is particularly important for equals()
     // methods.  This is an adaptation to a change in JDK 18; see
     // https://bugs.openjdk.org/browse/JDK-8272564
-    if (methodSymbol.owner.isInterface()) {
-      Symbol.MethodSymbol baseSymbol = (Symbol.MethodSymbol) methodSymbol.baseSymbol();
-      if (baseSymbol != methodSymbol && baseSymbol.owner == state.getSymtab().objectType.tsym) {
-        methodSymbol = baseSymbol;
-      }
-    }
-    return methodSymbol;
+    // Also, sometimes we need the base symbol to properly deal with static imports; see
+    // https://github.com/uber/NullAway/issues/764
+    // We can remove this workaround once we require the version of Error Prone released after
+    // 2.24.1, to get
+    // https://github.com/google/error-prone/commit/e5a6d0d8f9f96bda8e9952b7817cd0d2b63e51be
+    return (Symbol.MethodSymbol) methodSymbol.baseSymbol();
   }
 
   @Override
diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayCoreTests.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayCoreTests.java
index 3649de9564..d8167564f9 100644
--- a/nullaway/src/test/java/com/uber/nullaway/NullAwayCoreTests.java
+++ b/nullaway/src/test/java/com/uber/nullaway/NullAwayCoreTests.java
@@ -1001,4 +1001,33 @@ public void testDefaultEqualsInInterfaceTakesNullable() {
             "}")
         .doTest();
   }
+
+  @Test
+  public void testStaticImportFromSubclass() {
+    defaultCompilationHelper
+        .addSourceLines(
+            "Superclass.java",
+            "package com.uber;",
+            "import javax.annotation.Nullable;",
+            "class Superclass {",
+            "    public static String foo(@Nullable Object ignored) { return \"\"; };",
+            "}")
+        .addSourceLines(
+            "Subclass.java", "package com.uber;", "class Subclass extends Superclass {}")
+        .addSourceLines(
+            "Test.java",
+            "package com.uber;",
+            "import static com.uber.Subclass.foo;",
+            "class Test {",
+            "    static void m() {",
+            "        // Calling foo() the obvious way: safe because @Nullable arg",
+            "        System.out.println(Superclass.foo(null));",
+            "        // Calling foo() through Subclass: also safe",
+            "        System.out.println(Subclass.foo(null));",
+            "        // Static import from Subclass: also safe",
+            "        System.out.println(foo(null));",
+            "    }",
+            "}")
+        .doTest();
+  }
 }