Document and test why it's important to recognize TestCase.fail eve…

…n 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 uber/NullAway#764 PiperOrigin-RevId: 552893469
google · Aug 1, 2023 · c603793 · c603793
1 parent bbf7cd9
commit c603793
Show file tree

Hide file tree

Showing 2 changed files with 24 additions and 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
@@ -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
@@ -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()