From 16923e56c589f8581b24d4f8d588900a407a6e6a Mon Sep 17 00:00:00 2001 From: Nima Karimipour Date: Fri, 24 Feb 2023 12:57:36 -0800 Subject: [PATCH] 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. --- .../FixSerializationConfig.java | 7 +- .../adapters/SerializationAdapter.java | 2 +- ...apter.java => SerializationV3Adapter.java} | 43 +++- .../nullaway/NullAwaySerializationTest.java | 233 ++++++++++++++++-- .../tools/version1/ErrorDisplayV1.java | 28 +++ 5 files changed, 282 insertions(+), 31 deletions(-) rename nullaway/src/main/java/com/uber/nullaway/fixserialization/adapters/{SerializationV2Adapter.java => SerializationV3Adapter.java} (64%) diff --git a/nullaway/src/main/java/com/uber/nullaway/fixserialization/FixSerializationConfig.java b/nullaway/src/main/java/com/uber/nullaway/fixserialization/FixSerializationConfig.java index 1be397f7fe..2e2df8faee 100644 --- 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 index c7ef01bc0b..2f3bc5fc84 100644 --- 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/nullaway/src/main/java/com/uber/nullaway/fixserialization/adapters/SerializationV2Adapter.java b/nullaway/src/main/java/com/uber/nullaway/fixserialization/adapters/SerializationV3Adapter.java similarity index 64% rename from nullaway/src/main/java/com/uber/nullaway/fixserialization/adapters/SerializationV2Adapter.java rename to nullaway/src/main/java/com/uber/nullaway/fixserialization/adapters/SerializationV3Adapter.java index 6b72441852..a9aa74e1fe 100644 --- a/nullaway/src/main/java/com/uber/nullaway/fixserialization/adapters/SerializationV2Adapter.java +++ b/nullaway/src/main/java/com/uber/nullaway/fixserialization/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. * *

Updates to previous version (version 1): * @@ -40,9 +43,10 @@ * the error is reported. *

  • Serialized errors contain an extra column indicating the path to the containing source file * where the error is reported + *
  • Type arguments and Type use annotations are excluded from the serialized method signatures. * */ -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 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 index 45d838863a..d106be313a 100644 --- 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,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 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 tester = new SerializationTestHelper<>(root); @@ -1859,24 +1912,6 @@ public void fieldRegionComputationWithMemberSelectForInnerClassTest() { @Test public void errorSerializationVersion1() { - DisplayFactory 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 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 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 o) {", + " // BUG: Diagnostic contains: returning @Nullable expression", + " return null;", + " }", + "}") + .setExpectedOutputs( + new ErrorDisplayV1( + "RETURN_NULLABLE", + "returning @Nullable expression", + "com.uber.Foo", + "test(java.util.Map)", + "METHOD", + "com.uber.Foo", + "test(java.util.Map)", + "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 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>[] p2, @Custom Map ... 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 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(); + } } diff --git a/nullaway/src/test/java/com/uber/nullaway/tools/version1/ErrorDisplayV1.java b/nullaway/src/test/java/com/uber/nullaway/tools/version1/ErrorDisplayV1.java index 523efc6cc2..445b810732 100644 --- a/nullaway/src/test/java/com/uber/nullaway/tools/version1/ErrorDisplayV1.java +++ b/nullaway/src/test/java/com/uber/nullaway/tools/version1/ErrorDisplayV1.java @@ -21,7 +21,9 @@ */ package com.uber.nullaway.tools.version1; +import com.google.common.base.Preconditions; import com.uber.nullaway.tools.Display; +import com.uber.nullaway.tools.DisplayFactory; import com.uber.nullaway.tools.SerializationTestHelper; import java.util.Objects; @@ -133,4 +135,30 @@ public String toString() { + '\'' + '}'; } + + /** + * Returns the corresponding {@link DisplayFactory} for creating {@link ErrorDisplayV1} objects + * from an array of strings. + * + * @return a {@link DisplayFactory} for {@link ErrorDisplayV1} objects. + */ + public static DisplayFactory getFactory() { + return 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])); + }; + } }