Serialization version 3: update method serialization to exclude type …

…use annotations and type arguments (#735) This PR updates serialization to version `3` and burns version `2`. Changes in serialization version 3: - Type arguments and Type use annotations are excluded from the serialized method signatures. --- Additional context: This PR updates method serialization to ensure only relevant information in a method signature is serialized. Previous to this PR, method signatures serialization was relying on calling `.toString()` method, but if a parameter contains an annotation of type `@Target({TYPE_USE})` the annotation will exist in the `.toSting()` output which is not part of a method signature. This PR updates serialization to ensure exact method signature as expected is serialized.
uber · Feb 24, 2023 · 16923e5 · 16923e5
1 parent ff6b090
commit 16923e5
Show file tree

Hide file tree

Showing 5 changed files with 282 additions and 31 deletions.
diff --git a/nullaway/src/main/java/com/uber/nullaway/fixserialization/FixSerializationConfig.java b/nullaway/src/main/java/com/uber/nullaway/fixserialization/FixSerializationConfig.java
@@ -25,7 +25,7 @@
 import com.google.common.base.Preconditions;
 import com.uber.nullaway.fixserialization.adapters.SerializationAdapter;
 import com.uber.nullaway.fixserialization.adapters.SerializationV1Adapter;
-import com.uber.nullaway.fixserialization.adapters.SerializationV2Adapter;
+import com.uber.nullaway.fixserialization.adapters.SerializationV3Adapter;
 import com.uber.nullaway.fixserialization.out.SuggestedNullableFixInfo;
 import java.io.IOException;
 import java.nio.file.Files;
@@ -137,7 +137,10 @@ private SerializationAdapter initializeAdapter(int version) {
       case 1:
         return new SerializationV1Adapter();
       case 2:
-        return new SerializationV2Adapter();
+        throw new RuntimeException(
+            "Serialization version v2 is skipped and was used for an alpha version of the auto-annotator tool. Please use version 3 instead.");
+      case 3:
+        return new SerializationV3Adapter();
       default:
         throw new RuntimeException(
             "Unrecognized NullAway serialization version: "

diff --git a/nullaway/src/main/java/com/uber/nullaway/fixserialization/adapters/SerializationAdapter.java b/nullaway/src/main/java/com/uber/nullaway/fixserialization/adapters/SerializationAdapter.java
@@ -37,7 +37,7 @@ public interface SerializationAdapter {
    * Latest version number. If version is not defined by the user, NullAway will use the
    * corresponding adapter to this version in its serialization.
    */
-  int LATEST_VERSION = 2;
+  int LATEST_VERSION = 3;
 
   /**
    * Returns header of "errors.tsv" which contains all serialized {@link ErrorInfo} reported by

diff --git a/...tion/adapters/SerializationV2Adapter.java → ...tion/adapters/SerializationV3Adapter.java b/...tion/adapters/SerializationV2Adapter.java → ...tion/adapters/SerializationV3Adapter.java
@@ -23,15 +23,18 @@
 package com.uber.nullaway.fixserialization.adapters;
 
 import static com.uber.nullaway.fixserialization.out.ErrorInfo.EMPTY_NONNULL_TARGET_LOCATION_STRING;
+import static java.util.stream.Collectors.joining;
 
 import com.sun.tools.javac.code.Symbol;
+import com.sun.tools.javac.code.Type;
+import com.sun.tools.javac.util.Name;
 import com.uber.nullaway.fixserialization.SerializationService;
 import com.uber.nullaway.fixserialization.Serializer;
 import com.uber.nullaway.fixserialization.location.SymbolLocation;
 import com.uber.nullaway.fixserialization.out.ErrorInfo;
 
 /**
- * Adapter for serialization version 2.
+ * Adapter for serialization version 3.
  *
  * <p>Updates to previous version (version 1):
  *
@@ -40,9 +43,10 @@
  *       the error is reported.
  *   <li>Serialized errors contain an extra column indicating the path to the containing source file
  *       where the error is reported
+ *   <li>Type arguments and Type use annotations are excluded from the serialized method signatures.
  * </ul>
  */
-public class SerializationV2Adapter implements SerializationAdapter {
+public class SerializationV3Adapter implements SerializationAdapter {
 
   @Override
   public String getErrorsOutputFileHeader() {
@@ -80,11 +84,42 @@ public String serializeError(ErrorInfo errorInfo) {
 
   @Override
   public int getSerializationVersion() {
-    return 2;
+    return 3;
   }
 
   @Override
   public String serializeMethodSignature(Symbol.MethodSymbol methodSymbol) {
-    return methodSymbol.toString();
+    StringBuilder sb = new StringBuilder();
+    if (methodSymbol.isConstructor()) {
+      // For constructors, method's simple name is <init> and not the enclosing class, need to
+      // locate the enclosing class.
+      Symbol.ClassSymbol encClass = methodSymbol.owner.enclClass();
+      Name name = encClass.getSimpleName();
+      if (name.isEmpty()) {
+        // An anonymous class cannot declare its own constructor. Based on this assumption and our
+        // use case, we should not serialize the method signature.
+        throw new RuntimeException(
+            "Did not expect method serialization for anonymous class constructor: "
+                + methodSymbol
+                + ", in anonymous class: "
+                + encClass);
+      }
+      sb.append(name);
+    } else {
+      // For methods, we use the name of the method.
+      sb.append(methodSymbol.getSimpleName());
+    }
+    sb.append(
+        methodSymbol.getParameters().stream()
+            .map(
+                parameter ->
+                    // check if array
+                    (parameter.type instanceof Type.ArrayType)
+                        // if is array, get the element type and append "[]"
+                        ? ((Type.ArrayType) parameter.type).elemtype.tsym + "[]"
+                        // else, just get the type
+                        : parameter.type.tsym.toString())
+            .collect(joining(",", "(", ")")));
+    return sb.toString();
   }
 }
diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwaySerializationTest.java b/nullaway/src/test/java/com/uber/nullaway/NullAwaySerializationTest.java
@@ -30,7 +30,7 @@
 import com.sun.tools.javac.code.Symbol;
 import com.uber.nullaway.fixserialization.FixSerializationConfig;
 import com.uber.nullaway.fixserialization.adapters.SerializationV1Adapter;
-import com.uber.nullaway.fixserialization.adapters.SerializationV2Adapter;
+import com.uber.nullaway.fixserialization.adapters.SerializationV3Adapter;
 import com.uber.nullaway.fixserialization.out.FieldInitializationInfo;
 import com.uber.nullaway.fixserialization.out.SuggestedNullableFixInfo;
 import com.uber.nullaway.tools.DisplayFactory;
@@ -67,7 +67,7 @@ public class NullAwaySerializationTest extends NullAwayTestsBase {
   private static final String SUGGEST_FIX_FILE_HEADER = SuggestedNullableFixInfo.header();
   private static final String ERROR_FILE_NAME = "errors.tsv";
   private static final String ERROR_FILE_HEADER =
-      new SerializationV2Adapter().getErrorsOutputFileHeader();
+      new SerializationV3Adapter().getErrorsOutputFileHeader();
   private static final String FIELD_INIT_FILE_NAME = "field_init.tsv";
   private static final String FIELD_INIT_HEADER = FieldInitializationInfo.header();
 
@@ -383,7 +383,7 @@ public void suggestNullableParamGenericsTest() {
         .setExpectedOutputs(
             new FixDisplay(
                 "nullable",
-                "newStatement(T,java.util.ArrayList<T>,boolean,boolean)",
+                "newStatement(T,java.util.ArrayList,boolean,boolean)",
                 "lhs",
                 "PARAMETER",
                 "com.uber.Super",
@@ -1431,8 +1431,9 @@ public void errorSerializationTestLocalClassField() {
   public void verifySerializationVersionIsSerialized() {
     // Check for serialization version 1.
     checkVersionSerialization(1);
-    // Check for serialization version 2.
-    checkVersionSerialization(2);
+    // Check for serialization version 3 (recall: 2 is skipped and was only used for an alpha
+    // release of auto-annotator).
+    checkVersionSerialization(3);
   }
 
   @Test
@@ -1788,6 +1789,58 @@ public void fieldRegionComputationWithMemberSelectTest() {
         .doTest();
   }
 
+  @Test
+  public void constructorSerializationTest() {
+    SerializationTestHelper<ErrorDisplay> tester = new SerializationTestHelper<>(root);
+    tester
+        .setArgs(
+            Arrays.asList(
+                "-d",
+                temporaryFolder.getRoot().getAbsolutePath(),
+                "-XepOpt:NullAway:AnnotatedPackages=com.uber",
+                "-XepOpt:NullAway:SerializeFixMetadata=true",
+                "-XepOpt:NullAway:FixSerializationConfigPath=" + configPath))
+        .addSourceLines(
+            "com/uber/A.java",
+            "package com.uber;",
+            "import javax.annotation.Nullable;",
+            "public class A {",
+            "   @Nullable Object f;",
+            "   A(Object p){",
+            "       // BUG: Diagnostic contains: dereferenced expression",
+            "       Integer hashCode = f.hashCode();",
+            "   }",
+            "   public void bar(){",
+            "       // BUG: Diagnostic contains: passing @Nullable parameter",
+            "       A a = new A(null);",
+            "   }",
+            "}")
+        .setExpectedOutputs(
+            new ErrorDisplay(
+                "DEREFERENCE_NULLABLE",
+                "dereferenced expression f is @Nullable",
+                "com.uber.A",
+                "A(java.lang.Object)",
+                194,
+                "com/uber/A.java"),
+            new ErrorDisplay(
+                "PASS_NULLABLE",
+                "passing @Nullable parameter",
+                "com.uber.A",
+                "bar()",
+                312,
+                "com/uber/A.java",
+                "PARAMETER",
+                "com.uber.A",
+                "A(java.lang.Object)",
+                "p",
+                "0",
+                "com/uber/A.java"))
+        .setFactory(errorDisplayFactory)
+        .setOutputFileNameAndHeader(ERROR_FILE_NAME, ERROR_FILE_HEADER)
+        .doTest();
+  }
+
   @Test
   public void fieldRegionComputationWithMemberSelectForInnerClassTest() {
     SerializationTestHelper<ErrorDisplay> tester = new SerializationTestHelper<>(root);
@@ -1859,24 +1912,6 @@ public void fieldRegionComputationWithMemberSelectForInnerClassTest() {
 
   @Test
   public void errorSerializationVersion1() {
-    DisplayFactory<ErrorDisplayV1> errorDisplayV1Factor =
-        values -> {
-          Preconditions.checkArgument(
-              values.length == 10,
-              "Needs exactly 10 values to create ErrorDisplay for version 1 object but found: "
-                  + values.length);
-          return new ErrorDisplayV1(
-              values[0],
-              values[1],
-              values[2],
-              values[3],
-              values[4],
-              values[5],
-              values[6],
-              values[7],
-              values[8],
-              SerializationTestHelper.getRelativePathFromUnitTestTempDirectory(values[9]));
-        };
     SerializationTestHelper<ErrorDisplayV1> tester = new SerializationTestHelper<>(root);
     tester
         .setArgs(
@@ -1993,7 +2028,47 @@ public void errorSerializationVersion1() {
                 "null",
                 "null",
                 "com/uber/Super.java"))
-        .setFactory(errorDisplayV1Factor)
+        .setFactory(ErrorDisplayV1.getFactory())
+        .setOutputFileNameAndHeader(
+            ERROR_FILE_NAME, new SerializationV1Adapter().getErrorsOutputFileHeader())
+        .doTest();
+  }
+
+  @Test
+  public void typeArgumentSerializationForVersion1Test() {
+    SerializationTestHelper<ErrorDisplayV1> tester = new SerializationTestHelper<>(root);
+    tester
+        .setArgs(
+            Arrays.asList(
+                "-d",
+                temporaryFolder.getRoot().getAbsolutePath(),
+                "-XepOpt:NullAway:AnnotatedPackages=com.uber",
+                "-XepOpt:NullAway:SerializeFixMetadata=true",
+                "-XepOpt:NullAway:SerializeFixMetadataVersion=1",
+                "-XepOpt:NullAway:FixSerializationConfigPath=" + configPath))
+        .addSourceLines(
+            "com/uber/Foo.java",
+            "package com.uber;",
+            "import java.util.Map;",
+            "public class Foo {",
+            "   String test(Map<Object, String> o) {",
+            "       // BUG: Diagnostic contains: returning @Nullable expression",
+            "       return null;",
+            "   }",
+            "}")
+        .setExpectedOutputs(
+            new ErrorDisplayV1(
+                "RETURN_NULLABLE",
+                "returning @Nullable expression",
+                "com.uber.Foo",
+                "test(java.util.Map<java.lang.Object,java.lang.String>)",
+                "METHOD",
+                "com.uber.Foo",
+                "test(java.util.Map<java.lang.Object,java.lang.String>)",
+                "null",
+                "null",
+                "com/uber/Foo.java"))
+        .setFactory(ErrorDisplayV1.getFactory())
         .setOutputFileNameAndHeader(
             ERROR_FILE_NAME, new SerializationV1Adapter().getErrorsOutputFileHeader())
         .doTest();
@@ -2049,4 +2124,114 @@ public void checkVersionSerialization(int version) {
           "Could not read serialization version at path: " + serializationVersionPath, e);
     }
   }
+
+  @Test
+  public void varArgsWithTypeUseAnnotationMethodSerializationTest() {
+    SerializationTestHelper<ErrorDisplay> tester = new SerializationTestHelper<>(root);
+    tester
+        .setArgs(
+            Arrays.asList(
+                "-d",
+                temporaryFolder.getRoot().getAbsolutePath(),
+                "-XepOpt:NullAway:AnnotatedPackages=com.uber",
+                "-XepOpt:NullAway:SerializeFixMetadata=true",
+                "-XepOpt:NullAway:FixSerializationConfigPath=" + configPath))
+        .addSourceLines(
+            // toString() call on method symbol will serialize the annotation below which should not
+            // be present in string representation fo a method signature.
+            "com/uber/Custom.java",
+            "package com.uber;",
+            "import java.lang.annotation.Target;",
+            "import static java.lang.annotation.ElementType.TYPE_USE;",
+            "@Target({TYPE_USE})",
+            "public @interface Custom { }")
+        .addSourceLines(
+            "com/uber/Test.java",
+            "package com.uber;",
+            "import java.util.Map;",
+            "import java.util.List;",
+            "public class Test {",
+            "   Object m1(@Custom List<Map<String, ?>>[] p2, @Custom Map<? extends String, ?> ... p) {",
+            "      // BUG: Diagnostic contains: returning @Nullable expression",
+            "      return null;",
+            "   }",
+            "   Object m2(@Custom List<?>[] p) {",
+            "      // BUG: Diagnostic contains: returning @Nullable expression",
+            "      return null;",
+            "   }",
+            "}")
+        .setExpectedOutputs(
+            new ErrorDisplay(
+                "RETURN_NULLABLE",
+                "returning @Nullable expression",
+                "com.uber.Test",
+                "m1(java.util.List[],java.util.Map[])",
+                245,
+                "com/uber/Test.java",
+                "METHOD",
+                "com.uber.Test",
+                "m1(java.util.List[],java.util.Map[])",
+                "null",
+                "null",
+                "com/uber/Test.java"),
+            new ErrorDisplay(
+                "RETURN_NULLABLE",
+                "returning @Nullable expression",
+                "com.uber.Test",
+                "m2(java.util.List[])",
+                371,
+                "com/uber/Test.java",
+                "METHOD",
+                "com.uber.Test",
+                "m2(java.util.List[])",
+                "null",
+                "null",
+                "com/uber/Test.java"))
+        .setFactory(errorDisplayFactory)
+        .setOutputFileNameAndHeader(ERROR_FILE_NAME, ERROR_FILE_HEADER)
+        .doTest();
+  }
+
+  @Test
+  public void anonymousSubClassConstructorSerializationTest() {
+    SerializationTestHelper<ErrorDisplay> tester = new SerializationTestHelper<>(root);
+    tester
+        .setArgs(
+            Arrays.asList(
+                "-d",
+                temporaryFolder.getRoot().getAbsolutePath(),
+                "-XepOpt:NullAway:AnnotatedPackages=com.uber",
+                "-XepOpt:NullAway:SerializeFixMetadata=true",
+                "-XepOpt:NullAway:FixSerializationConfigPath=" + configPath))
+        .addSourceLines(
+            // correct serialization of names for constructors invoked while creating anonymous
+            // inner classes, where the name is technically the name of the super-class, not of the
+            // anonymous inner class
+            "com/uber/Foo.java",
+            "package com.uber;",
+            "public class Foo {",
+            "  Foo(Object o) {}",
+            "  void bar() {",
+            "    // BUG: Diagnostic contains: passing @Nullable parameter",
+            "    Foo f = new Foo(null) {};",
+            "  }",
+            "}")
+        .setExpectedOutputs(
+            new ErrorDisplay(
+                "PASS_NULLABLE",
+                "passing @Nullable parameter",
+                "com.uber.Foo",
+                "bar()",
+                144,
+                "com/uber/Foo.java",
+                "PARAMETER",
+                "com.uber.Foo",
+                "Foo(java.lang.Object)",
+                "o",
+                "0",
+                "com/uber/Foo.java"))
+        .setFactory(errorDisplayFactory)
+        .setOutputFileNameAndHeader(ERROR_FILE_NAME, ERROR_FILE_HEADER)
+        .doTest();
+  }
 }