Skip to content

Commit c603793

Browse files
cpovirkError Prone Team
authored and
Error Prone Team
committed
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 uber/NullAway#764 PiperOrigin-RevId: 552893469
1 parent bbf7cd9 commit c603793

File tree

2 files changed

+24
-11
lines changed

2 files changed

+24
-11
lines changed

core/src/main/java/com/google/errorprone/bugpatterns/nullness/ReturnMissingNullable.java

Lines changed: 8 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -84,17 +84,14 @@ public class ReturnMissingNullable extends BugChecker implements CompilationUnit
8484
anyOf(
8585
anyMethod().anyClass().withNameMatching(compile("throw.*Exception")),
8686
staticMethod()
87-
.onClassAny(
88-
"org.junit.Assert",
89-
"junit.framework.Assert",
90-
/*
91-
* I'm not sure if TestCase is necessary, as it doesn't define its own fail()
92-
* method, but it commonly appears in lists like this one, so I've included
93-
* it. (Maybe the method was defined on TestCase many versions ago?)
94-
*
95-
* TODO(cpovirk): Confirm need, or remove from everywhere.
96-
*/
97-
"junit.framework.TestCase")
87+
/*
88+
* b/285157761: The reason to look at descendants of the listed classes is mostly
89+
* to catch non-canonical static imports: While TestCase doesn't define its own
90+
* fail() method, javac still lets users import it with "import static
91+
* junit.framework.TestCase.fail." And when users do that, javac acts as if fail()
92+
* is a member of TestCase. We still want to cover it here.
93+
*/
94+
.onDescendantOfAny("org.junit.Assert", "junit.framework.Assert")
9895
.named("fail"),
9996
staticMethod().onClass("java.lang.System").named("exit")));
10097

core/src/test/java/com/google/errorprone/bugpatterns/nullness/ReturnMissingNullableTest.java

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1389,6 +1389,22 @@ public void negativeCases_unreachableFail() {
13891389
.doTest();
13901390
}
13911391

1392+
@Test
1393+
public void negativeCases_unreachableFailNonCanonicalImport() {
1394+
createCompilationTestHelper()
1395+
.addSourceLines(
1396+
"com/google/errorprone/bugpatterns/nullness/LiteralNullReturnTest.java",
1397+
"package com.google.errorprone.bugpatterns.nullness;",
1398+
"import static junit.framework.TestCase.fail;",
1399+
"class LiteralNullReturnTest {",
1400+
" public String getMessage() {",
1401+
" fail();",
1402+
" return null;",
1403+
" }",
1404+
"}")
1405+
.doTest();
1406+
}
1407+
13921408
@Test
13931409
public void negativeCases_unreachableThrowExceptionMethod() {
13941410
createCompilationTestHelper()

0 commit comments

Comments
 (0)