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: 603374262

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()