Avoid using castToNonNull suggested fixes in certain cases

uber · Aug 4, 2023 · 95df551 · 95df551
1 parent 1ea7f06
commit 95df551
Show file tree

Hide file tree

Showing 2 changed files with 48 additions and 5 deletions.
diff --git a/nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java b/nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java
@@ -31,6 +31,7 @@
 import static com.uber.nullaway.NullAway.INITIALIZATION_CHECK_NAME;
 import static com.uber.nullaway.NullAway.OPTIONAL_CHECK_NAME;
 import static com.uber.nullaway.NullAway.getTreesInstance;
+import static com.uber.nullaway.Nullness.hasNullableAnnotation;
 
 import com.google.common.base.Joiner;
 import com.google.common.base.Preconditions;
@@ -61,6 +62,7 @@
 import java.util.stream.StreamSupport;
 import javax.annotation.Nullable;
 import javax.lang.model.element.Element;
+import javax.lang.model.element.ElementKind;
 import javax.tools.JavaFileObject;
 
 /** A class to construct error message to be displayed after the analysis finds error. */
@@ -198,10 +200,11 @@ private Description.Builder addSuggestedSuppression(
       case PASS_NULLABLE:
       case ASSIGN_FIELD_NULLABLE:
       case SWITCH_EXPRESSION_NULLABLE:
-        if (config.getCastToNonNullMethod() != null && canBeCastToNonNull(suggestTree, state)) {
+        if (config.getCastToNonNullMethod() != null && canBeCastToNonNull(suggestTree)) {
           builder = addCastToNonNullFix(suggestTree, builder);
         } else {
-          builder = addSuppressWarningsFix(suggestTree, builder, suppressionName);
+          builder =
+              addSuppressWarningsFix(suppressibleNode(state.getPath()), builder, suppressionName);
         }
         break;
       case CAST_TO_NONNULL_ARG_NONNULL:
@@ -329,11 +332,19 @@ private Tree suppressibleNode(@Nullable TreePath path) {
    *   <li>{@code tree} represents a {@code @Nullable} formal parameter of the enclosing method
    * </ol>
    */
-  private boolean canBeCastToNonNull(Tree tree, VisitorState state) {
+  private boolean canBeCastToNonNull(Tree tree) {
     switch (tree.getKind()) {
       case NULL_LITERAL:
+        // never do castToNonNull(null)
         return false;
       case IDENTIFIER:
+        // Don't wrap a @Nullable parameter in castToNonNull, as this misleads callers into thinking
+        // they can pass in null without causing an NPE.  A more appropriate fix would likely be to
+        // make the parameter @NonNull and add casts at call sites, but that is beyond the scope of
+        // our suggested fixes
+        Symbol symbol = ASTHelpers.getSymbol(tree);
+        return !(symbol.getKind().equals(ElementKind.PARAMETER)
+            && hasNullableAnnotation(symbol, config));
       default:
         return true;
     }

diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayAutoSuggestTest.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayAutoSuggestTest.java
@@ -92,7 +92,8 @@ public void suggestCastToNonNull() throws IOException {
             "package com.uber;",
             "import javax.annotation.Nullable;",
             "class Test {",
-            "  Object test1(@Nullable Object o) {",
+            "  @Nullable Object o;",
+            "  Object test1() {",
             "    return o;",
             "  }",
             "}")
@@ -102,13 +103,44 @@ public void suggestCastToNonNull() throws IOException {
             "import static com.uber.nullaway.testdata.Util.castToNonNull;",
             "import javax.annotation.Nullable;",
             "class Test {",
-            "  Object test1(@Nullable Object o) {",
+            "  @Nullable Object o;",
+            "  Object test1() {",
             "    return castToNonNull(o);",
             "  }",
             "}")
         .doTest();
   }
 
+  @Test
+  public void suppressInsteadOfCastToNonNull() throws IOException {
+    makeTestHelper()
+        .addInputLines(
+            "Test.java",
+            "package com.uber;",
+            "import javax.annotation.Nullable;",
+            "class Test {",
+            "  Object test1(@Nullable Object o) {",
+            "    return o;",
+            "  }",
+            "  Object test2() {",
+            "    return null;",
+            "  }",
+            "}")
+        .addOutputLines(
+            "out/Test.java",
+            "package com.uber;",
+            "import javax.annotation.Nullable;",
+            "class Test {",
+            "  @SuppressWarnings(\"NullAway\") Object test1(@Nullable Object o) {",
+            "    return o;",
+            "  }",
+            "  @SuppressWarnings(\"NullAway\") Object test2() {",
+            "    return null;",
+            "  }",
+            "}")
+        .doTest();
+  }
+
   @Test
   public void removeUnnecessaryCastToNonNull() throws IOException {
     makeTestHelper()