From 22f19a04c819471ed1f5a71d4e739d5f4f49395b Mon Sep 17 00:00:00 2001 From: Lazaro Clapp Date: Tue, 1 Aug 2023 15:58:35 -0400 Subject: [PATCH] Improve auto-fixing of unnecessary castToNonNull calls In particular, this allows removing unnecessary casts where the castToNonNull method is specified by a library model, rather than a CLI argument. --- .../java/com/uber/nullaway/ErrorBuilder.java | 30 +++++++++--------- .../main/java/com/uber/nullaway/NullAway.java | 4 ++- .../nullaway/NullAwayAutoSuggestTest.java | 31 ++++++++++++++++++- 3 files changed, 48 insertions(+), 17 deletions(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java b/nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java index eeff202842..e85f04d3b5 100755 --- a/nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java +++ b/nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java @@ -133,7 +133,7 @@ public Description createErrorDescription( } if (config.suggestSuppressions() && suggestTree != null) { - builder = addSuggestedSuppression(errorMessage, suggestTree, builder); + builder = addSuggestedSuppression(errorMessage, suggestTree, builder, state); } if (config.serializationIsActive()) { @@ -187,7 +187,10 @@ private boolean hasPathSuppression(TreePath treePath, String subcheckerName) { } private Description.Builder addSuggestedSuppression( - ErrorMessage errorMessage, Tree suggestTree, Description.Builder builder) { + ErrorMessage errorMessage, + Tree suggestTree, + Description.Builder builder, + VisitorState state) { switch (errorMessage.messageType) { case DEREFERENCE_NULLABLE: case RETURN_NULLABLE: @@ -201,7 +204,7 @@ private Description.Builder addSuggestedSuppression( } break; case CAST_TO_NONNULL_ARG_NONNULL: - builder = removeCastToNonNullFix(suggestTree, builder); + builder = removeCastToNonNullFix(suggestTree, builder, state); break; case WRONG_OVERRIDE_RETURN: builder = addSuppressWarningsFix(suggestTree, builder, suppressionName); @@ -334,20 +337,17 @@ private Description.Builder addCastToNonNullFix(Tree suggestTree, Description.Bu } private Description.Builder removeCastToNonNullFix( - Tree suggestTree, Description.Builder builder) { - assert suggestTree.getKind() == Tree.Kind.METHOD_INVOCATION; - final MethodInvocationTree invTree = (MethodInvocationTree) suggestTree; - final Symbol.MethodSymbol methodSymbol = ASTHelpers.getSymbol(invTree); - final String qualifiedName = - ASTHelpers.enclosingClass(methodSymbol) + "." + methodSymbol.getSimpleName().toString(); - if (!qualifiedName.equals(config.getCastToNonNullMethod())) { - throw new RuntimeException("suggestTree should point to the castToNonNull invocation."); - } + Tree suggestTree, Description.Builder builder, VisitorState state) { + // Note: Here suggestTree refers to the argument being cast. We need to find the + // castToNonNull(...) invocation to be replaced by it. Fortunately, state.getPath() + // should be currently pointing at said call. + Tree currTree = state.getPath().getLeaf(); + assert currTree.getKind() == Tree.Kind.METHOD_INVOCATION; + final MethodInvocationTree invTree = (MethodInvocationTree) currTree; + assert invTree.getArguments().contains(suggestTree); // Remove the call to castToNonNull: final SuggestedFix fix = - SuggestedFix.builder() - .replace(suggestTree, invTree.getArguments().get(0).toString()) - .build(); + SuggestedFix.builder().replace(invTree, suggestTree.toString()).build(); return builder.addFix(fix); } diff --git a/nullaway/src/main/java/com/uber/nullaway/NullAway.java b/nullaway/src/main/java/com/uber/nullaway/NullAway.java index b78c228657..d7c0afb42c 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullAway.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullAway.java @@ -1712,7 +1712,9 @@ private Description checkCastToNonNullTakesNullable( + "at the invocation site, but which are known not to be null at runtime."; return errorBuilder.createErrorDescription( new ErrorMessage(MessageTypes.CAST_TO_NONNULL_ARG_NONNULL, message), - tree, + // The Tree passed as suggestTree is the expression being cast + // to avoid recomputing the arg index: + actual, buildDescription(tree), state, null); diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayAutoSuggestTest.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayAutoSuggestTest.java index 5aec8370a2..93e796d8c7 100644 --- a/nullaway/src/test/java/com/uber/nullaway/NullAwayAutoSuggestTest.java +++ b/nullaway/src/test/java/com/uber/nullaway/NullAwayAutoSuggestTest.java @@ -24,6 +24,7 @@ import com.google.errorprone.BugCheckerRefactoringTestHelper; import com.google.errorprone.ErrorProneFlags; +import com.uber.nullaway.testlibrarymodels.TestLibraryModels; import java.io.IOException; import org.junit.Before; import org.junit.Rule; @@ -37,7 +38,7 @@ public class NullAwayAutoSuggestTest { @Rule public TemporaryFolder temporaryFolder = new TemporaryFolder(); - private ErrorProneFlags flags; + public ErrorProneFlags flags; @Before public void setup() { @@ -57,6 +58,8 @@ private BugCheckerRefactoringTestHelper makeTestHelper() { .setArgs( "-d", temporaryFolder.getRoot().getAbsolutePath(), + "-processorpath", + TestLibraryModels.class.getProtectionDomain().getCodeSource().getLocation().getPath(), // the remaining args are not needed right now, but they will be necessary when we // switch to the more modern newInstance() API "-XepOpt:NullAway:AnnotatedPackages=com.uber,com.ubercab,io.reactivex", @@ -132,6 +135,32 @@ public void removeUnnecessaryCastToNonNull() throws IOException { .doTest(); } + @Test + public void removeUnnecessaryCastToNonNullFromLibraryModel() throws IOException { + makeTestHelper() + .addInputLines( + "Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "import static com.uber.nullaway.testdata.Util.castToNonNull;", + "class Test {", + " Object test1(Object o) {", + " return castToNonNull(\"CAST_REASON\",o,42);", + " }", + "}") + .addOutputLines( + "out/Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "import static com.uber.nullaway.testdata.Util.castToNonNull;", + "class Test {", + " Object test1(Object o) {", + " return o;", + " }", + "}") + .doTest(); + } + @Test public void suggestSuppressionOnMethodRef() throws IOException { makeTestHelper()