Return the "real" Symbol instead of sometimes a Symbol whose `own…

…er` is a subclass.

We accomplish this by calling `baseSymbol()` on the initial `Symbol`.

(We likely should do this in more places in Error Prone, including the location in `ASTHelpers.getSymbol` that I've flagged and perhaps anywhere else that we directly access `tree.sym`.)

Thanks to @msridhar for links to relevant JDK discussions, notably [JDK-8293890](https://bugs.openjdk.org/browse/JDK-8293890).

Effects:
- This change definitely helps with [static imports](uber/NullAway#764), as demonstrated in a new test in `ParameterNameTest`, which fails before this change. (I didn't actually look into why the test fails before. Maybe the "fake" `Symbol` lacks the parameter names of the original?)
- I didn't check whether it helps with [`someSerializable.equals`](uber/NullAway#897). It seems likely to, but _shrug_ for now.
- I had expected this to help with the `TestCase.fail` handling in `ReturnMissingNullable`, but it turns out that I'm wrong on multiple levels. I updated its comments and tests accordingly:
   - `TestCase.fail` _does_ exist as a declared method nowadays; it's not merely inherited from `Assert`. I must have been looking at [quite an old version](junit-team/junit4@dde798f#diff-f9834c0e0ef1d54e1757e221b7b4248c2ba8e0de41d2f0fadac5927b906edd85R226).
   - The problem there comes not from the need for `ASTHelpers.getSymbol` to use `baseSymbol()` but instead for the need for `MethodMatchState.ownerType` to look at the actual `owner` instead of the type of the receiver tree. (If it started to do that, it would then benefit from this change to produce the correct `owner`.) I added a link to the appropriate bug there.
PiperOrigin-RevId: 601163959

check_api/src/main/java/com/google/errorprone/matchers/method/MethodMatchState.java

@@ -30,7 +30,7 @@ abstract class MethodMatchState implements MatchState {
 
   @Override
   public Type ownerType() {
-    // TODO(cushon): should this be the symbol's owner type, not the receiver's owner type?
+    // TODO: b/130658266 - should this be the symbol's owner type, not the receiver's owner type?
     return ASTHelpers.getReceiverType(tree());
   }
 

check_api/src/main/java/com/google/errorprone/util/ASTHelpers.java

@@ -267,6 +267,7 @@ public static Symbol getSymbol(Tree tree) {
       return ASTHelpers.getSymbol((NewClassTree) tree);
     }
     if (tree instanceof MemberReferenceTree) {
+      // TODO: b/285157761 - Delegate to the MemberReferenceTree overload.
       return ((JCMemberReference) tree).sym;
     }
     if (tree instanceof JCAnnotatedType) {
@@ -319,7 +320,7 @@ public static MethodSymbol getSymbol(MethodInvocationTree tree) {
       // Defensive. Would only occur if there are errors in the AST.
       throw new IllegalArgumentException(tree.toString());
     }
-    return (MethodSymbol) sym;
+    return (MethodSymbol) sym.baseSymbol();
   }
 
   /** Gets the symbol for a member reference. */
@@ -329,7 +330,7 @@ public static MethodSymbol getSymbol(MemberReferenceTree tree) {
       // Defensive. Would only occur if there are errors in the AST.
       throw new IllegalArgumentException(tree.toString());
     }
-    return (MethodSymbol) sym;
+    return (MethodSymbol) sym.baseSymbol();
   }
 
   /**

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

@@ -84,11 +84,26 @@ public class ReturnMissingNullable extends BugChecker implements CompilationUnit
               anyMethod().anyClass().withNameMatching(compile("throw.*Exception")),
               staticMethod()
                   /*
-                   * 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.
+                   * There are a few reasons to look at *descendants* of the listed classes:
+                   *
+                   * - It enables our listing for junit.framework.Assert.fail to catch the
+                   *   equivalent (but redeclared) junit.framework.TestCase.fail for free. This
+                   *   comes up a good number of times. (Of course, we could also easily handle it
+                   *   by adding an explicit listing for junit.framework.TestCase.fail.)
+                   *
+                   * - It enables our listing for junit.framework.Assert.fail to catch custom "fail"
+                   *   methods in TestCase subclasses. There are admittedly only a handful of such
+                   *   methods, only one of which looks even vaguely relevant to
+                   *   ReturnMissingNullable.
+                   *
+                   * - It enables our listing for junit.framework.Assert.fail to catch non-canonical
+                   *   static imports of it and junit.framework.TestCase.fail (e.g., `import static
+                   *   com.foo.BarTest.fail`). I'm not sure that this ever comes up.
+                   *
+                   * There are two issues that have combined to produce the problem here:
+                   * b/285157761 (which makes getSymbol return the wrong thing, which we've fixed)
+                   * and b/130658266 (which makes MethodMatchers look at the Tree again to extract
+                   * the receiver type, which sidesteps the getSymbol fix).
                    */
                   .onDescendantOfAny("org.junit.Assert", "junit.framework.Assert")
                   .named("fail"),

core/src/test/java/com/google/errorprone/bugpatterns/ParameterNameTest.java

@@ -821,4 +821,18 @@ public void exemptPackage() {
         .setArgs(ImmutableList.of("-XepOpt:ParameterName:exemptPackagePrefixes=test.a,test.b"))
         .doTest();
   }
+
+  @Test
+  public void nonCanonicalMockitoImport() {
+    testHelper
+        .addSourceLines(
+            "test/a/A.java",
+            "package test.a;",
+            "import static org.mockito.Mockito.eq;",
+            "public class A {",
+            "  // BUG: Diagnostic contains:",
+            "  Object x = eq(/* notValue= */ 1);",
+            "}")
+        .doTest();
+  }
 }

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

@@ -1336,7 +1336,7 @@ public void negativeCases_unreachableExit() {
   }
 
   @Test
-  public void negativeCases_unreachableFail() {
+  public void negativeCases_unreachableAssertFail() {
     createCompilationTestHelper()
         .addSourceLines(
             "com/google/errorprone/bugpatterns/nullness/LiteralNullReturnTest.java",
@@ -1352,7 +1352,7 @@ public void negativeCases_unreachableFail() {
   }
 
   @Test
-  public void negativeCases_unreachableFailNonCanonicalImport() {
+  public void negativeCases_unreachableTestCaseFail() {
     createCompilationTestHelper()
         .addSourceLines(
             "com/google/errorprone/bugpatterns/nullness/LiteralNullReturnTest.java",
@@ -1367,6 +1367,28 @@ public void negativeCases_unreachableFailNonCanonicalImport() {
         .doTest();
   }
 
+  @Test
+  public void negativeCases_unreachableFailNonCanonicalImport() {
+    createCompilationTestHelper()
+        .addSourceLines(
+            "com/foo/BarTestCase.java",
+            "package foo;",
+            "import junit.framework.TestCase;",
+            "class BarTestCase extends TestCase {",
+            "}")
+        .addSourceLines(
+            "com/foo/OtherTestCase.java",
+            "package foo;",
+            "import static foo.BarTestCase.fail;",
+            "class OtherTestCase {",
+            "  public String getMessage() {",
+            "    fail();",
+            "    return null;",
+            "  }",
+            "}")
+        .doTest();
+  }
+
   @Test
   public void negativeCases_unreachableThrowExceptionMethod() {
     createCompilationTestHelper()