Improve auto-fixing of unnecessary castToNonNull calls (#796)

In particular, this allows removing unnecessary casts where the castToNonNull method is specified by a library model, rather than a CLI argument.
uber · Aug 1, 2023 · b061288 · b061288
1 parent e711887
commit b061288
Show file tree

Hide file tree

Showing 3 changed files with 54 additions and 16 deletions.
diff --git a/nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java b/nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java
@@ -33,6 +33,7 @@
 import static com.uber.nullaway.NullAway.getTreesInstance;
 
 import com.google.common.base.Joiner;
+import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
@@ -133,7 +134,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 +188,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 +205,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 +338,23 @@ 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();
+    Preconditions.checkArgument(
+        currTree.getKind() == Tree.Kind.METHOD_INVOCATION,
+        String.format("Expected castToNonNull invocation expression, found:\n%s", currTree));
+    final MethodInvocationTree invTree = (MethodInvocationTree) currTree;
+    Preconditions.checkArgument(
+        invTree.getArguments().contains(suggestTree),
+        String.format(
+            "Method invocation tree %s does not contain the expression %s as an argument being cast",
+            invTree, 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
@@ -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
@@ -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;
@@ -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()