From c603793f0938a6d590b98c04614ac0bf05866970 Mon Sep 17 00:00:00 2001
From: cpovirk <cpovirk@google.com>
Date: Tue, 1 Aug 2023 12:55:02 -0700
Subject: [PATCH] Document and test why it's important to recognize
 `TestCase.fail` even though `TestCase` doesn't declare a method of that name.
 Also, rework the implementation to detect it differently (though the change
 appears to make no practical difference for this check).

See https://github.com/uber/NullAway/issues/764

PiperOrigin-RevId: 552893469
---
 .../nullness/ReturnMissingNullable.java       | 19 ++++++++-----------
 .../nullness/ReturnMissingNullableTest.java   | 16 ++++++++++++++++
 2 files changed, 24 insertions(+), 11 deletions(-)

diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/nullness/ReturnMissingNullable.java b/core/src/main/java/com/google/errorprone/bugpatterns/nullness/ReturnMissingNullable.java
index 17e4181c312..8f066e93afd 100644
--- a/core/src/main/java/com/google/errorprone/bugpatterns/nullness/ReturnMissingNullable.java
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/nullness/ReturnMissingNullable.java
@@ -84,17 +84,14 @@ public class ReturnMissingNullable extends BugChecker implements CompilationUnit
           anyOf(
               anyMethod().anyClass().withNameMatching(compile("throw.*Exception")),
               staticMethod()
-                  .onClassAny(
-                      "org.junit.Assert",
-                      "junit.framework.Assert",
-                      /*
-                       * I'm not sure if TestCase is necessary, as it doesn't define its own fail()
-                       * method, but it commonly appears in lists like this one, so I've included
-                       * it. (Maybe the method was defined on TestCase many versions ago?)
-                       *
-                       * TODO(cpovirk): Confirm need, or remove from everywhere.
-                       */
-                      "junit.framework.TestCase")
+                  /*
+                   * b/285157761: The reason to look at descendants of the listed classes is mostly
+                   * to catch non-canonical static imports: While TestCase doesn't define its own
+                   * fail() method, javac still lets users import it with "import static
+                   * junit.framework.TestCase.fail." And when users do that, javac acts as if fail()
+                   * is a member of TestCase. We still want to cover it here.
+                   */
+                  .onDescendantOfAny("org.junit.Assert", "junit.framework.Assert")
                   .named("fail"),
               staticMethod().onClass("java.lang.System").named("exit")));
 
diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/nullness/ReturnMissingNullableTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/nullness/ReturnMissingNullableTest.java
index d4894c7fef6..37daeb2cb79 100644
--- a/core/src/test/java/com/google/errorprone/bugpatterns/nullness/ReturnMissingNullableTest.java
+++ b/core/src/test/java/com/google/errorprone/bugpatterns/nullness/ReturnMissingNullableTest.java
@@ -1389,6 +1389,22 @@ public void negativeCases_unreachableFail() {
         .doTest();
   }
 
+  @Test
+  public void negativeCases_unreachableFailNonCanonicalImport() {
+    createCompilationTestHelper()
+        .addSourceLines(
+            "com/google/errorprone/bugpatterns/nullness/LiteralNullReturnTest.java",
+            "package com.google.errorprone.bugpatterns.nullness;",
+            "import static junit.framework.TestCase.fail;",
+            "class LiteralNullReturnTest {",
+            "  public String getMessage() {",
+            "    fail();",
+            "    return null;",
+            "  }",
+            "}")
+        .doTest();
+  }
+
   @Test
   public void negativeCases_unreachableThrowExceptionMethod() {
     createCompilationTestHelper()