Prevent crash in fixserialization when ClassSymbol.sourcefile is null. (

#656) In general, neither `ClassSymbol.sourcefile` nor `ClassSymbol.classfile` are guaranteed to be non-null. This can lead to crashes when performing fixserialization, in particular when the symbol comes from a jar file. This PR changes our logic to provide best effort file locations for code, checking first for `sourcefile`, then `classfile`, then defaulting to the string `"null"`. We also add a test case to simulate the behavior of other build systems (namely buck) where `sourcefile` might be `null`. This requires adding significant new mockito magic to our tests, including `mockito-inline`.
uber · Sep 14, 2022 · 1cd2d28 · 1cd2d28
1 parent a7ba27e
commit 1cd2d28
Show file tree

Hide file tree

Showing 8 changed files with 217 additions and 8 deletions.
diff --git a/gradle/dependencies.gradle b/gradle/dependencies.gradle
@@ -111,6 +111,7 @@ def test = [
     springContext           : "org.springframework:spring-context:5.3.7",
     grpcCore                : "io.grpc:grpc-core:1.15.1", // Should upgrade, but this matches our guava version
     mockito                 : "org.mockito:mockito-core:4.6.1",
+    mockitoInline           : "org.mockito:mockito-inline:4.6.1",
     javaxAnnotationApi      : "javax.annotation:javax.annotation-api:1.3.2",
 ]
 

diff --git a/nullaway/build.gradle b/nullaway/build.gradle
@@ -61,6 +61,7 @@ dependencies {
     testImplementation deps.test.grpcCore
     testImplementation project(":test-java-lib-lombok")
     testImplementation deps.test.mockito
+    testImplementation deps.test.mockitoInline
     testImplementation deps.test.javaxAnnotationApi
 
     // This ends up being resolved to the NullAway jar under nullaway/build/libs

diff --git a/...way/src/main/java/com/uber/nullaway/fixserialization/location/AbstractSymbolLocation.java b/...way/src/main/java/com/uber/nullaway/fixserialization/location/AbstractSymbolLocation.java
@@ -28,15 +28,16 @@
 import com.google.errorprone.util.ASTHelpers;
 import com.sun.tools.javac.code.Symbol;
 import java.net.URI;
+import javax.annotation.Nullable;
 import javax.lang.model.element.ElementKind;
 
 /** abstract base class for {@link SymbolLocation}. */
 public abstract class AbstractSymbolLocation implements SymbolLocation {
 
   /** Element kind of the targeted symbol */
   protected final ElementKind type;
-  /** URI of the file containing the symbol. */
-  protected final URI uri;
+  /** URI of the file containing the symbol, if available. */
+  @Nullable protected final URI uri;
   /** Enclosing class of the symbol. */
   protected final Symbol.ClassSymbol enclosingClass;
 
@@ -50,6 +51,9 @@ public AbstractSymbolLocation(ElementKind type, Symbol target) {
             + ".");
     this.type = type;
     this.enclosingClass = castToNonNull(ASTHelpers.enclosingClass(target));
-    this.uri = enclosingClass.sourcefile.toUri();
+    this.uri =
+        enclosingClass.sourcefile != null
+            ? enclosingClass.sourcefile.toUri()
+            : (enclosingClass.classfile != null ? enclosingClass.classfile.toUri() : null);
   }
 }
diff --git a/nullaway/src/main/java/com/uber/nullaway/fixserialization/location/FieldLocation.java b/nullaway/src/main/java/com/uber/nullaway/fixserialization/location/FieldLocation.java
@@ -45,6 +45,6 @@ public String tabSeparatedToString() {
         "null",
         variableSymbol.toString(),
         "null",
-        uri.toASCIIString());
+        uri != null ? uri.toASCIIString() : "null");
   }
 }
diff --git a/nullaway/src/main/java/com/uber/nullaway/fixserialization/location/MethodLocation.java b/nullaway/src/main/java/com/uber/nullaway/fixserialization/location/MethodLocation.java
@@ -45,6 +45,6 @@ public String tabSeparatedToString() {
         enclosingMethod.toString(),
         "null",
         "null",
-        uri.toASCIIString());
+        uri != null ? uri.toASCIIString() : "null");
   }
 }
diff --git a/...ay/src/main/java/com/uber/nullaway/fixserialization/location/MethodParameterLocation.java b/...ay/src/main/java/com/uber/nullaway/fixserialization/location/MethodParameterLocation.java
@@ -66,6 +66,6 @@ public String tabSeparatedToString() {
         enclosingMethod.toString(),
         paramSymbol.toString(),
         String.valueOf(index),
-        uri.toASCIIString());
+        uri != null ? uri.toASCIIString() : "null");
   }
 }
diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwaySerializationTest.java b/nullaway/src/test/java/com/uber/nullaway/NullAwaySerializationTest.java
@@ -22,7 +22,11 @@
 
 package com.uber.nullaway;
 
+import static org.mockito.ArgumentMatchers.any;
+
 import com.google.common.base.Preconditions;
+import com.google.errorprone.util.ASTHelpers;
+import com.sun.tools.javac.code.Symbol;
 import com.uber.nullaway.fixserialization.FixSerializationConfig;
 import com.uber.nullaway.fixserialization.out.ErrorInfo;
 import com.uber.nullaway.fixserialization.out.FieldInitializationInfo;
@@ -42,6 +46,9 @@
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
+import org.mockito.MockedStatic;
+import org.mockito.Mockito;
+import org.mockito.stubbing.Answer;
 
 /** Unit tests for {@link com.uber.nullaway.NullAway}. */
 @RunWith(JUnit4.class)
@@ -69,7 +76,9 @@ public NullAwaySerializationTest() {
           // relative paths are getting compared.
           FixDisplay display =
               new FixDisplay(values[7], values[2], values[3], values[0], values[1], values[5]);
-          display.uri = display.uri.substring(display.uri.indexOf("com/uber/"));
+          if (display.uri.contains("com/uber/")) {
+            display.uri = display.uri.substring(display.uri.indexOf("com/uber/"));
+          }
           return display;
         };
     this.errorDisplayFactory =
@@ -90,7 +99,9 @@ public NullAwaySerializationTest() {
           FieldInitDisplay display =
               new FieldInitDisplay(
                   values[6], values[2], values[3], values[0], values[1], values[5]);
-          display.uri = display.uri.substring(display.uri.indexOf("com/uber/"));
+          if (display.uri.contains("com/uber/")) {
+            display.uri = display.uri.substring(display.uri.indexOf("com/uber/"));
+          }
           return display;
         };
   }
@@ -1357,4 +1368,154 @@ public void errorSerializationTestLocalClassField() {
         .setOutputFileNameAndHeader(ERROR_FILE_NAME, ERROR_FILE_HEADER)
         .doTest();
   }
+
+  @Test
+  public void suggestNullableArgumentOnBytecode() {
+    SerializationTestHelper<FixDisplay> tester = new SerializationTestHelper<>(root);
+    tester
+        .setArgs(
+            Arrays.asList(
+                "-d",
+                temporaryFolder.getRoot().getAbsolutePath(),
+                "-XepOpt:NullAway:AnnotatedPackages=com.uber",
+                // Explicitly avoid excluding com.uber.nullaway.testdata.unannotated,
+                // so we can suggest fixes there
+                "-XepOpt:NullAway:SerializeFixMetadata=true",
+                "-XepOpt:NullAway:FixSerializationConfigPath=" + configPath))
+        .addSourceLines(
+            "com/uber/UsesUnannotated.java",
+            "package com.uber;",
+            "import com.uber.nullaway.testdata.unannotated.MinimalUnannotatedClass;",
+            "public class UsesUnannotated {",
+            "   Object test(boolean flag) {",
+            "       // BUG: Diagnostic contains: passing @Nullable parameter 'null' where @NonNull is required",
+            "       return MinimalUnannotatedClass.foo(null);",
+            "   }",
+            "}")
+        .setExpectedOutputs(
+            new FixDisplay(
+                "nullable",
+                "foo(java.lang.Object)",
+                "x",
+                "PARAMETER",
+                "com.uber.nullaway.testdata.unannotated.MinimalUnannotatedClass",
+                "com/uber/nullaway/testdata/unannotated/MinimalUnannotatedClass.java"))
+        .setFactory(fixDisplayFactory)
+        .setOutputFileNameAndHeader(SUGGEST_FIX_FILE_NAME, SUGGEST_FIX_FILE_HEADER)
+        .doTest();
+  }
+
+  @Test
+  public void suggestNullableArgumentOnBytecodeNoFileInfo() {
+    // Simulate a build system which elides sourcefile/classfile info
+    try (MockedStatic<ASTHelpers> astHelpersMockedStatic =
+        Mockito.mockStatic(ASTHelpers.class, Mockito.CALLS_REAL_METHODS)) {
+      astHelpersMockedStatic
+          .when(() -> ASTHelpers.enclosingClass(any(Symbol.class)))
+          .thenAnswer(
+              (Answer<Symbol.ClassSymbol>)
+                  invocation -> {
+                    Symbol.ClassSymbol answer = (Symbol.ClassSymbol) invocation.callRealMethod();
+                    if (answer.sourcefile != null
+                        && answer
+                            .sourcefile
+                            .toUri()
+                            .toASCIIString()
+                            .contains("com/uber/nullaway/testdata/unannotated")) {
+                      answer.sourcefile = null;
+                      answer.classfile = null;
+                    }
+                    return answer;
+                  });
+      SerializationTestHelper<FixDisplay> tester = new SerializationTestHelper<>(root);
+      tester
+          .setArgs(
+              Arrays.asList(
+                  "-d",
+                  temporaryFolder.getRoot().getAbsolutePath(),
+                  "-XepOpt:NullAway:AnnotatedPackages=com.uber",
+                  // Explicitly avoid excluding com.uber.nullaway.testdata.unannotated,
+                  // so we can suggest fixes there
+                  "-XepOpt:NullAway:SerializeFixMetadata=true",
+                  "-XepOpt:NullAway:FixSerializationConfigPath=" + configPath))
+          .addSourceLines(
+              "com/uber/UsesUnannotated.java",
+              "package com.uber;",
+              "import com.uber.nullaway.testdata.unannotated.MinimalUnannotatedClass;",
+              "public class UsesUnannotated {",
+              "   Object test(boolean flag) {",
+              "       // BUG: Diagnostic contains: passing @Nullable parameter 'null' where @NonNull is required",
+              "       return MinimalUnannotatedClass.foo(null);",
+              "   }",
+              "}")
+          .setExpectedOutputs(
+              new FixDisplay(
+                  "nullable",
+                  "foo(java.lang.Object)",
+                  "x",
+                  "PARAMETER",
+                  "com.uber.nullaway.testdata.unannotated.MinimalUnannotatedClass",
+                  "null")) // <- ! the important bit
+          .setFactory(fixDisplayFactory)
+          .setOutputFileNameAndHeader(SUGGEST_FIX_FILE_NAME, SUGGEST_FIX_FILE_HEADER)
+          .doTest();
+    }
+  }
+
+  @Test
+  public void suggestNullableArgumentOnBytecodeClassFileInfoOnly() {
+    // Simulate a build system which elides sourcefile/classfile info
+    try (MockedStatic<ASTHelpers> astHelpersMockedStatic =
+        Mockito.mockStatic(ASTHelpers.class, Mockito.CALLS_REAL_METHODS)) {
+      astHelpersMockedStatic
+          .when(() -> ASTHelpers.enclosingClass(any(Symbol.class)))
+          .thenAnswer(
+              (Answer<Symbol.ClassSymbol>)
+                  invocation -> {
+                    Symbol.ClassSymbol answer = (Symbol.ClassSymbol) invocation.callRealMethod();
+                    if (answer.sourcefile != null
+                        && answer
+                            .sourcefile
+                            .toUri()
+                            .toASCIIString()
+                            .contains("com/uber/nullaway/testdata/unannotated")) {
+                      answer.sourcefile = null;
+                    }
+                    return answer;
+                  });
+      SerializationTestHelper<FixDisplay> tester = new SerializationTestHelper<>(root);
+      tester
+          .setArgs(
+              Arrays.asList(
+                  "-d",
+                  temporaryFolder.getRoot().getAbsolutePath(),
+                  "-XepOpt:NullAway:AnnotatedPackages=com.uber",
+                  // Explicitly avoid excluding com.uber.nullaway.testdata.unannotated,
+                  // so we can suggest fixes there
+                  "-XepOpt:NullAway:SerializeFixMetadata=true",
+                  "-XepOpt:NullAway:FixSerializationConfigPath=" + configPath))
+          .addSourceLines(
+              "com/uber/UsesUnannotated.java",
+              "package com.uber;",
+              "import com.uber.nullaway.testdata.unannotated.MinimalUnannotatedClass;",
+              "public class UsesUnannotated {",
+              "   Object test(boolean flag) {",
+              "       // BUG: Diagnostic contains: passing @Nullable parameter 'null' where @NonNull is required",
+              "       return MinimalUnannotatedClass.foo(null);",
+              "   }",
+              "}")
+          .setExpectedOutputs(
+              new FixDisplay(
+                  "nullable",
+                  "foo(java.lang.Object)",
+                  "x",
+                  "PARAMETER",
+                  "com.uber.nullaway.testdata.unannotated.MinimalUnannotatedClass",
+                  // From Symbol.classfile!
+                  "com/uber/nullaway/testdata/unannotated/MinimalUnannotatedClass.java"))
+          .setFactory(fixDisplayFactory)
+          .setOutputFileNameAndHeader(SUGGEST_FIX_FILE_NAME, SUGGEST_FIX_FILE_HEADER)
+          .doTest();
+    }
+  }
 }
diff --git a/...ay/src/test/resources/com/uber/nullaway/testdata/unannotated/MinimalUnannotatedClass.java b/...ay/src/test/resources/com/uber/nullaway/testdata/unannotated/MinimalUnannotatedClass.java
@@ -0,0 +1,42 @@
+/*
+ * Copyright (c) 2022 Uber Technologies, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+package com.uber.nullaway.testdata.unannotated;
+
+/**
+ * A minimal class, used from {@link com.uber.nullaway.NullAwaySerializationTest} to avoid extra
+ * fixes.
+ */
+public class MinimalUnannotatedClass {
+
+  private MinimalUnannotatedClass() {}
+
+  /**
+   * This is an identity method, without Nullability annotations.
+   *
+   * @param x
+   * @return
+   */
+  public static Object foo(Object x) {
+    return x;
+  }
+}