Merge 6cc3e83 into 38f3081

uber · Aug 4, 2023 · f405772 · f405772
2 parents 38f3081 + 6cc3e83
commit f405772
Show file tree

Hide file tree

Showing 2 changed files with 69 additions and 4 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,13 @@ private Description.Builder addSuggestedSuppression(
       case PASS_NULLABLE:
       case ASSIGN_FIELD_NULLABLE:
       case SWITCH_EXPRESSION_NULLABLE:
-        if (config.getCastToNonNullMethod() != null) {
+        if (config.getCastToNonNullMethod() != null && canBeCastToNonNull(suggestTree)) {
           builder = addCastToNonNullFix(suggestTree, builder);
         } else {
-          builder = addSuppressWarningsFix(suggestTree, builder, suppressionName);
+          Tree suppressibleNode = suppressibleNode(state.getPath());
+          if (suppressibleNode != null) {
+            builder = addSuppressWarningsFix(suppressibleNode, builder, suppressionName);
+          }
         }
         break;
       case CAST_TO_NONNULL_ARG_NONNULL:
@@ -320,6 +325,34 @@ private Tree suppressibleNode(@Nullable TreePath path) {
         .orElse(null);
   }
 
+  /**
+   * Checks if it would be appropriate to wrap {@code tree} in a {@code castToNonNull} call. There
+   * are two cases where this method returns {@code false}:
+   *
+   * <ol>
+   *   <li>{@code tree} represents the {@code null} literal
+   *   <li>{@code tree} represents a {@code @Nullable} formal parameter of the enclosing method
+   * </ol>
+   */
+  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 != null
+            && symbol.getKind().equals(ElementKind.PARAMETER)
+            && hasNullableAnnotation(symbol, config));
+      default:
+        return true;
+    }
+  }
+
   private Description.Builder addCastToNonNullFix(Tree suggestTree, Description.Builder builder) {
     final String fullMethodName = config.getCastToNonNullMethod();
     if (fullMethodName == null) {

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