From 5d8d87f007efa54d2c82c4aaf41d2d0d55733d9a Mon Sep 17 00:00:00 2001 From: cpovirk Date: Wed, 24 Jan 2024 10:31:37 -0800 Subject: [PATCH] Return the "real" `Symbol` instead of sometimes a `Symbol` whose `owner` 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](https://github.com/uber/NullAway/issues/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`](https://github.com/uber/NullAway/issues/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](https://github.com/junit-team/junit4/commit/dde798f89fbd1295bf7345ccfab17242ae9d01c0#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 --- .../matchers/method/MethodMatchState.java | 2 +- .../google/errorprone/util/ASTHelpers.java | 5 ++-- .../nullness/ReturnMissingNullable.java | 25 ++++++++++++++---- .../bugpatterns/ParameterNameTest.java | 14 ++++++++++ .../nullness/ReturnMissingNullableTest.java | 26 +++++++++++++++++-- 5 files changed, 62 insertions(+), 10 deletions(-) diff --git a/check_api/src/main/java/com/google/errorprone/matchers/method/MethodMatchState.java b/check_api/src/main/java/com/google/errorprone/matchers/method/MethodMatchState.java index a33710e5524f..e54932596c96 100644 --- a/check_api/src/main/java/com/google/errorprone/matchers/method/MethodMatchState.java +++ b/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()); } diff --git a/check_api/src/main/java/com/google/errorprone/util/ASTHelpers.java b/check_api/src/main/java/com/google/errorprone/util/ASTHelpers.java index 54eeefbbfcbe..1c196bb60489 100644 --- a/check_api/src/main/java/com/google/errorprone/util/ASTHelpers.java +++ b/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(); } /** 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 2f32a9dc2f07..2d1525c09d3e 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,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"), diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/ParameterNameTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/ParameterNameTest.java index 3769df4c3790..e727a647edea 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/ParameterNameTest.java +++ b/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(); + } } 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 fb85ffc548ef..3fbb6ec97c61 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 @@ -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()