From dc232c98641fdc7a62b617b050fa870e2cd81f6f Mon Sep 17 00:00:00 2001 From: NikitaAware Date: Wed, 22 Feb 2023 15:32:14 -0800 Subject: [PATCH] minor changes in a test case 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. Suggested change suggested changes ternary operator as a method argument Docs: `-XepExcludedPaths` was added in 2.1.3, not 2.13 (#744) Add command line option to skip specific library models. (#741) This PR adds the CLI option `-XepOpt:NullAway:IgnoreLibraryModelsFor` which takes a list of methods, given as fully qualified class name + method simple name (i.e. independent of argument types). Methods matching this list will be skipped when loading library models (from any implementation of the `LibraryModels`'s `@AutoService` as well as our included models). This can be used in a per-compilation target basis to disable NullAway's library models for ease of upgrading to new versions with stricter modeling of common JDK or third-party APIs. We considered a few alternative approaches here, but decided against: - Simply using another instance of `LibraryModels` to "invert" the models given by NullAway's default library models: This would have required no code changes, but doesn't work in all cases. If NullAway's default models make the return of method `foo()` `@Nullable`, for example, then a new model that makes the return `@NonNull` will break `@Nullable` overrides of `foo()`. In general, we want to go back to "optimistic assumptions" rather than just replace the library model. - We could have a list of methods in the `LibraryModels` for which to ignore previous models, and have those override any models on those methods coming from a different `LibraryModels` implementation. But, from the point of view of the user configuring NullAway, this is complex: they need to have an instance of custom library models in their build, and changing java plugin classpath deps on a per-target basis is more complex than changing CLI arguments (e.g. due to JVM re-use by the build system). - We could provide more specific disabling of library models (e.g. a specific method signature or removing only one particular kind of model from a method, such as keeping the model on the return value, but removing it from an argument, or removing a null-implies-false model or similar). We could revisit this in the future, but supporting this would make the syntax of the CLI values a lot more complex. For now, we believe just turning off all models for a given method is a sufficient degree of granularity. - Per-package/per-class/regex based ignore specs: See above. Avoiding complexity until we need it. Note: If and when this lands, it needs a Wiki documentation update! --------- Co-authored-by: Manu Sridharan --- README.md | 2 +- .../com/uber/nullaway/AbstractConfig.java | 7 + .../main/java/com/uber/nullaway/Config.java | 14 ++ .../com/uber/nullaway/DummyOptionsConfig.java | 5 + .../nullaway/ErrorProneCLIFlagsConfig.java | 4 + .../com/uber/nullaway/GenericsChecks.java | 6 +- .../FixSerializationConfig.java | 7 +- .../adapters/SerializationAdapter.java | 2 +- ...apter.java => SerializationV3Adapter.java} | 43 +++- .../handlers/LibraryModelsHandler.java | 42 +++- .../NullAwayCustomLibraryModelsTests.java | 118 +++++++++ .../NullAwayJSpecifyGenericsTests.java | 24 +- .../nullaway/NullAwaySerializationTest.java | 233 ++++++++++++++++-- .../tools/version1/ErrorDisplayV1.java | 28 +++ .../unannotated/UnannotatedWithModels.java | 16 ++ .../testlibrarymodels/TestLibraryModels.java | 9 +- 16 files changed, 509 insertions(+), 51 deletions(-) rename nullaway/src/main/java/com/uber/nullaway/fixserialization/adapters/{SerializationV2Adapter.java => SerializationV3Adapter.java} (64%) create mode 100644 test-java-lib/src/main/java/com/uber/lib/unannotated/UnannotatedWithModels.java diff --git a/README.md b/README.md index 21c30e1775..8e0d54c5f1 100644 --- a/README.md +++ b/README.md @@ -84,7 +84,7 @@ A complete Android `build.gradle` example is [here](https://gist.github.com/msri #### Annotation Processors / Generated Code -Some annotation processors like [Dagger](https://google.github.io/dagger/) and [AutoValue](https://github.com/google/auto/tree/master/value) generate code into the same package namespace as your own code. This can cause problems when setting NullAway to the `ERROR` level as suggested above, since errors in this generated code will block the build. Currently the best solution to this problem is to completely disable Error Prone on generated code, using the `-XepExcludedPaths` option added in Error Prone 2.13 (documented [here](http://errorprone.info/docs/flags), use `options.errorprone.excludedPaths=` in Gradle). To use, figure out which directory contains the generated code, and add that directory to the excluded path regex. +Some annotation processors like [Dagger](https://google.github.io/dagger/) and [AutoValue](https://github.com/google/auto/tree/master/value) generate code into the same package namespace as your own code. This can cause problems when setting NullAway to the `ERROR` level as suggested above, since errors in this generated code will block the build. Currently the best solution to this problem is to completely disable Error Prone on generated code, using the `-XepExcludedPaths` option added in Error Prone 2.1.3 (documented [here](http://errorprone.info/docs/flags), use `options.errorprone.excludedPaths=` in Gradle). To use, figure out which directory contains the generated code, and add that directory to the excluded path regex. **Note for Dagger users**: Dagger versions older than 2.12 can have bad interactions with NullAway; see [here](https://github.com/uber/NullAway/issues/48#issuecomment-340018409). Please update to Dagger 2.12 to fix the problem. diff --git a/nullaway/src/main/java/com/uber/nullaway/AbstractConfig.java b/nullaway/src/main/java/com/uber/nullaway/AbstractConfig.java index 01bb17777a..9303b9380c 100644 --- a/nullaway/src/main/java/com/uber/nullaway/AbstractConfig.java +++ b/nullaway/src/main/java/com/uber/nullaway/AbstractConfig.java @@ -101,6 +101,8 @@ public abstract class AbstractConfig implements Config { protected String autofixSuppressionComment; + protected ImmutableSet skippedLibraryModels; + /** --- JarInfer configs --- */ protected boolean jarInferEnabled; @@ -292,6 +294,11 @@ public boolean isContractAnnotation(String annotationName) { return contractAnnotations.contains(annotationName); } + @Override + public boolean isSkippedLibraryModel(String classDotMethod) { + return skippedLibraryModels.contains(classDotMethod); + } + @AutoValue abstract static class MethodClassAndName { diff --git a/nullaway/src/main/java/com/uber/nullaway/Config.java b/nullaway/src/main/java/com/uber/nullaway/Config.java index 51fb81e1df..8b0b084203 100644 --- a/nullaway/src/main/java/com/uber/nullaway/Config.java +++ b/nullaway/src/main/java/com/uber/nullaway/Config.java @@ -249,6 +249,20 @@ public interface Config { */ String getAutofixSuppressionComment(); + /** + * Checks if the given library model should be skipped/ignored. + * + *

For ease of configuration in the command line, this works at the level of the (class, method + * name) pair, meaning it applies for all methods with the same name in the same class, even if + * they have different signatures, and to all library models applicable to that method (i.e. on + * the method's return, arguments, etc). + * + * @param classDotMethod The method from the model, in [fully_qualified_class_name].[method_name] + * format (no args) + * @return True if the library model should be skipped. + */ + boolean isSkippedLibraryModel(String classDotMethod); + // --- JarInfer configs --- /** diff --git a/nullaway/src/main/java/com/uber/nullaway/DummyOptionsConfig.java b/nullaway/src/main/java/com/uber/nullaway/DummyOptionsConfig.java index 62e90ece71..de423e3fab 100644 --- a/nullaway/src/main/java/com/uber/nullaway/DummyOptionsConfig.java +++ b/nullaway/src/main/java/com/uber/nullaway/DummyOptionsConfig.java @@ -185,6 +185,11 @@ public String getAutofixSuppressionComment() { throw new IllegalStateException(ERROR_MESSAGE); } + @Override + public boolean isSkippedLibraryModel(String classDotMethod) { + throw new IllegalStateException(ERROR_MESSAGE); + } + @Override public boolean isJarInferEnabled() { throw new IllegalStateException(ERROR_MESSAGE); diff --git a/nullaway/src/main/java/com/uber/nullaway/ErrorProneCLIFlagsConfig.java b/nullaway/src/main/java/com/uber/nullaway/ErrorProneCLIFlagsConfig.java index 7bb663561c..939166a19a 100644 --- a/nullaway/src/main/java/com/uber/nullaway/ErrorProneCLIFlagsConfig.java +++ b/nullaway/src/main/java/com/uber/nullaway/ErrorProneCLIFlagsConfig.java @@ -72,6 +72,9 @@ final class ErrorProneCLIFlagsConfig extends AbstractConfig { static final String FL_OPTIONAL_CLASS_PATHS = EP_FL_NAMESPACE + ":CheckOptionalEmptinessCustomClasses"; static final String FL_SUPPRESS_COMMENT = EP_FL_NAMESPACE + ":AutoFixSuppressionComment"; + + static final String FL_SKIP_LIBRARY_MODELS = EP_FL_NAMESPACE + ":IgnoreLibraryModelsFor"; + /** --- JarInfer configs --- */ static final String FL_JI_ENABLED = EP_FL_NAMESPACE + ":JarInferEnabled"; @@ -209,6 +212,7 @@ final class ErrorProneCLIFlagsConfig extends AbstractConfig { throw new IllegalStateException( "Invalid -XepOpt:" + FL_SUPPRESS_COMMENT + " value. Comment must be single line."); } + skippedLibraryModels = getFlagStringSet(flags, FL_SKIP_LIBRARY_MODELS); /** --- JarInfer configs --- */ jarInferEnabled = flags.getBoolean(FL_JI_ENABLED).orElse(false); jarInferUseReturnAnnotations = flags.getBoolean(FL_JI_USE_RETURN).orElse(false); diff --git a/nullaway/src/main/java/com/uber/nullaway/GenericsChecks.java b/nullaway/src/main/java/com/uber/nullaway/GenericsChecks.java index 7f75bbd68b..8905098c83 100644 --- a/nullaway/src/main/java/com/uber/nullaway/GenericsChecks.java +++ b/nullaway/src/main/java/com/uber/nullaway/GenericsChecks.java @@ -403,11 +403,9 @@ public void checkTypeParameterNullnessForConditionalExpression(ConditionalExpres return; } - Tree truePartTree; - Tree falsePartTree; + Tree truePartTree = tree.getTrueExpression(); + Tree falsePartTree = tree.getFalseExpression(); - truePartTree = tree.getTrueExpression(); - falsePartTree = tree.getFalseExpression(); Type condExprType = getTreeType(tree); Type truePartType = getTreeType(truePartTree); Type falsePartType = getTreeType(falsePartTree); 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/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java index c98f77c2a4..3b38fab313 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java @@ -75,7 +75,7 @@ public class LibraryModelsHandler extends BaseNoOpHandler { public LibraryModelsHandler(Config config) { super(); this.config = config; - libraryModels = loadLibraryModels(); + libraryModels = loadLibraryModels(config); } @Override @@ -283,12 +283,12 @@ private void setUnconditionalArgumentNullness( } } - private static LibraryModels loadLibraryModels() { + private static LibraryModels loadLibraryModels(Config config) { Iterable externalLibraryModels = ServiceLoader.load(LibraryModels.class, LibraryModels.class.getClassLoader()); ImmutableSet.Builder libModelsBuilder = new ImmutableSet.Builder<>(); libModelsBuilder.add(new DefaultLibraryModels()).addAll(externalLibraryModels); - return new CombinedLibraryModels(libModelsBuilder.build()); + return new CombinedLibraryModels(libModelsBuilder.build(), config); } private static class DefaultLibraryModels implements LibraryModels { @@ -751,6 +751,8 @@ public ImmutableSetMultimap castToNonNullMethods() { private static class CombinedLibraryModels implements LibraryModels { + private final Config config; + private final ImmutableSetMultimap failIfNullParameters; private final ImmutableSetMultimap explicitlyNullableParameters; @@ -769,7 +771,8 @@ private static class CombinedLibraryModels implements LibraryModels { private final ImmutableSetMultimap castToNonNullMethods; - public CombinedLibraryModels(Iterable models) { + public CombinedLibraryModels(Iterable models, Config config) { + this.config = config; ImmutableSetMultimap.Builder failIfNullParametersBuilder = new ImmutableSetMultimap.Builder<>(); ImmutableSetMultimap.Builder explicitlyNullableParametersBuilder = @@ -788,34 +791,61 @@ public CombinedLibraryModels(Iterable models) { new ImmutableSetMultimap.Builder<>(); for (LibraryModels libraryModels : models) { for (Map.Entry entry : libraryModels.failIfNullParameters().entries()) { + if (shouldSkipModel(entry.getKey())) { + continue; + } failIfNullParametersBuilder.put(entry); } for (Map.Entry entry : libraryModels.explicitlyNullableParameters().entries()) { + if (shouldSkipModel(entry.getKey())) { + continue; + } explicitlyNullableParametersBuilder.put(entry); } for (Map.Entry entry : libraryModels.nonNullParameters().entries()) { + if (shouldSkipModel(entry.getKey())) { + continue; + } nonNullParametersBuilder.put(entry); } for (Map.Entry entry : libraryModels.nullImpliesTrueParameters().entries()) { + if (shouldSkipModel(entry.getKey())) { + continue; + } nullImpliesTrueParametersBuilder.put(entry); } for (Map.Entry entry : libraryModels.nullImpliesFalseParameters().entries()) { + if (shouldSkipModel(entry.getKey())) { + continue; + } nullImpliesFalseParametersBuilder.put(entry); } for (Map.Entry entry : libraryModels.nullImpliesNullParameters().entries()) { + if (shouldSkipModel(entry.getKey())) { + continue; + } nullImpliesNullParametersBuilder.put(entry); } for (MethodRef name : libraryModels.nullableReturns()) { + if (shouldSkipModel(name)) { + continue; + } nullableReturnsBuilder.add(name); } for (MethodRef name : libraryModels.nonNullReturns()) { + if (shouldSkipModel(name)) { + continue; + } nonNullReturnsBuilder.add(name); } for (Map.Entry entry : libraryModels.castToNonNullMethods().entries()) { + if (shouldSkipModel(entry.getKey())) { + continue; + } castToNonNullMethodsBuilder.put(entry); } } @@ -830,6 +860,10 @@ public CombinedLibraryModels(Iterable models) { castToNonNullMethods = castToNonNullMethodsBuilder.build(); } + private boolean shouldSkipModel(MethodRef key) { + return config.isSkippedLibraryModel(key.enclosingClass + "." + key.methodName); + } + @Override public ImmutableSetMultimap failIfNullParameters() { return failIfNullParameters; diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayCustomLibraryModelsTests.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayCustomLibraryModelsTests.java index 6718e5d9a5..0685a6566e 100644 --- a/nullaway/src/test/java/com/uber/nullaway/NullAwayCustomLibraryModelsTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/NullAwayCustomLibraryModelsTests.java @@ -107,4 +107,122 @@ public void libraryModelsOverrideRestrictiveAnnotations() { "}") .doTest(); } + + @Test + public void libraryModelsAndSelectiveSkippingViaCommandLineOptions() { + // First test with the library models in effect + makeLibraryModelsTestHelperWithArgs( + Arrays.asList( + "-d", + temporaryFolder.getRoot().getAbsolutePath(), + "-XepOpt:NullAway:AnnotatedPackages=com.uber", + "-XepOpt:NullAway:UnannotatedSubPackages=com.uber.lib.unannotated")) + .addSourceLines( + "CallMethods.java", + "package com.uber;", + "import com.uber.lib.unannotated.UnannotatedWithModels;", + "import javax.annotation.Nullable;", + "public class CallMethods {", + " Object testWithoutCheck(UnannotatedWithModels u) {", + " // BUG: Diagnostic contains: returning @Nullable expression from method with @NonNull return type", + " return u.returnsNullUnannotated();", + " }", + " Object testWithCheck(@Nullable Object o) {", + " if (UnannotatedWithModels.isNonNull(o)) {", + " return o;", + " }", + " return new Object();", + " }", + "}") + .addSourceLines( + "OverrideCheck.java", + "package com.uber;", + "import com.uber.lib.unannotated.UnannotatedWithModels;", + "import javax.annotation.Nullable;", + "public class OverrideCheck extends UnannotatedWithModels {", + " @Nullable", + " public Object returnsNullUnannotated() {", + " return null;", + " }", + "}") + .doTest(); + // Now test disabling the library model + makeLibraryModelsTestHelperWithArgs( + Arrays.asList( + "-d", + temporaryFolder.getRoot().getAbsolutePath(), + "-XepOpt:NullAway:AnnotatedPackages=com.uber", + "-XepOpt:NullAway:UnannotatedSubPackages=com.uber.lib.unannotated", + "-XepOpt:NullAway:IgnoreLibraryModelsFor=com.uber.lib.unannotated.UnannotatedWithModels.returnsNullUnannotated,com.uber.lib.unannotated.UnannotatedWithModels.isNonNull")) + .addSourceLines( + "CallMethods.java", + "package com.uber;", + "import com.uber.lib.unannotated.UnannotatedWithModels;", + "import javax.annotation.Nullable;", + "public class CallMethods {", + " Object testWithoutCheck(UnannotatedWithModels u) {", + " // Ok. Library model ignored", + " return u.returnsNullUnannotated();", + " }", + " Object testWithoutCheckNonIgnoredMethod(UnannotatedWithModels u) {", + " // BUG: Diagnostic contains: returning @Nullable expression from method with @NonNull return type", + " return u.returnsNullUnannotated2();", + " }", + " Object testWithCheck(@Nullable Object o) {", + " if (UnannotatedWithModels.isNonNull(o)) {", + " // BUG: Diagnostic contains: returning @Nullable expression from method with @NonNull return type", + " return o;", + " }", + " return new Object();", + " }", + "}") + .addSourceLines( + "OverrideCheck.java", + "package com.uber;", + "import com.uber.lib.unannotated.UnannotatedWithModels;", + "import javax.annotation.Nullable;", + "public class OverrideCheck extends UnannotatedWithModels {", + " @Nullable", // Still safe, because the method is not @NonNull, it's unannotated + // without model! + " public Object returnsNullUnannotated() {", + " return null;", + " }", + "}") + .doTest(); + } + + @Test + public void libraryModelsAndSelectiveSkippingViaCommandLineOptions2() { + makeLibraryModelsTestHelperWithArgs( + Arrays.asList( + "-d", + temporaryFolder.getRoot().getAbsolutePath(), + "-XepOpt:NullAway:AnnotatedPackages=com.uber", + "-XepOpt:NullAway:UnannotatedSubPackages=com.uber.lib.unannotated", + "-XepOpt:NullAway:AcknowledgeRestrictiveAnnotations=true", + "-XepOpt:NullAway:IgnoreLibraryModelsFor=com.uber.lib.unannotated.RestrictivelyAnnotatedFIWithModelOverride.apply")) + .addSourceLines( + "Test.java", + "package com.uber;", + "import com.uber.lib.unannotated.RestrictivelyAnnotatedFIWithModelOverride;", + "import javax.annotation.Nullable;", + "public class Test {", + " void bar(RestrictivelyAnnotatedFIWithModelOverride f) {", + " // Param is @NullableDecl in bytecode, and library model making it non-null is skipped", + " f.apply(null);", + " }", + " void foo() {", + " RestrictivelyAnnotatedFIWithModelOverride func = (x) -> {", + " // Param is @NullableDecl in bytecode, and overriding library model is ignored, thus unsafe", + " // BUG: Diagnostic contains: dereferenced expression x is @Nullable", + " return x.toString();", + " };", + " }", + " void baz() {", + " // BUG: Diagnostic contains: unbound instance method reference cannot be used", + " bar(Object::toString);", + " }", + "}") + .doTest(); + } } diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java index bea967a8c5..e2291d42d3 100644 --- a/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java @@ -566,12 +566,18 @@ public void genericsChecksForTernaryOperator() { "import org.jspecify.annotations.Nullable;", "class Test {", "static class A { }", - " static void method1(boolean t) {", - " // BUG: Diagnostic contains: Conditional expression", + " static A testPositive(A a, boolean t) {", + " // BUG: Diagnostic contains: Conditional expression must have type", " A<@Nullable String> t1 = t ? new A() : new A<@Nullable String>();", + " // BUG: Diagnostic contains: Conditional expression must have type", + " return t ? new A<@Nullable String>() : new A<@Nullable String>();", + " }", + " static void testPositiveTernaryMethodArgument(boolean t) {", + " // BUG: Diagnostic contains: Conditional expression must have type", + " A a = testPositive(t ? new A() : new A<@Nullable String>(), t);", " }", - " static A method2(boolean t) {", - " // BUG: Diagnostic contains: Conditional expression", + " static A<@Nullable String> testNegative(boolean t) {", + " A<@Nullable String> t1 = t ? new A<@Nullable String>() : new A<@Nullable String>();", " return t ? new A<@Nullable String>() : new A<@Nullable String>();", " }", "}") @@ -590,13 +596,13 @@ public void ternaryOperatorComplexSubtyping() { " static class B extends A {}", " static class C extends A {}", " static void testPositive(boolean t) {", - " // BUG: Diagnostic contains: Conditional expression", + " // BUG: Diagnostic contains: Conditional expression must have type", " A<@Nullable String> t1 = t ? new B<@Nullable String>() : new C();", - " // BUG: Diagnostic contains: Conditional expression", + " // BUG: Diagnostic contains: Conditional expression must have type", " A<@Nullable String> t2 = t ? new C() : new B<@Nullable String>();", - " // BUG: Diagnostic contains: Conditional expression", + " // BUG: Diagnostic contains:Conditional expression must have type", " A<@Nullable String> t3 = t ? new B() : new C<@Nullable String>();", - " // BUG: Diagnostic contains: Conditional expression", + " // BUG: Diagnostic contains: Conditional expression must have type", " A t4 = t ? new B<@Nullable String>() : new C<@Nullable String>();", " }", " static void testNegative(boolean t) {", @@ -621,7 +627,7 @@ public void nestedTernary() { " static class C extends A {}", " static void testPositive(boolean t) {", " A<@Nullable String> t1 = t ? new C<@Nullable String>() :", - " // BUG: Diagnostic contains: Conditional expression", + " // BUG: Diagnostic contains: Conditional expression must have type", " (t ? new B<@Nullable String>() : new A());", " }", " static void testNegative(boolean t) {", 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])); + }; + } } diff --git a/test-java-lib/src/main/java/com/uber/lib/unannotated/UnannotatedWithModels.java b/test-java-lib/src/main/java/com/uber/lib/unannotated/UnannotatedWithModels.java new file mode 100644 index 0000000000..412042dbbb --- /dev/null +++ b/test-java-lib/src/main/java/com/uber/lib/unannotated/UnannotatedWithModels.java @@ -0,0 +1,16 @@ +package com.uber.lib.unannotated; + +public class UnannotatedWithModels { + + public Object returnsNullUnannotated() { + return null; + } + + public Object returnsNullUnannotated2() { + return null; + } + + public static boolean isNonNull(Object o) { + return o != null; + } +} diff --git a/test-library-models/src/main/java/com/uber/nullaway/testlibrarymodels/TestLibraryModels.java b/test-library-models/src/main/java/com/uber/nullaway/testlibrarymodels/TestLibraryModels.java index debf6b1a5f..cd6aad1d40 100644 --- a/test-library-models/src/main/java/com/uber/nullaway/testlibrarymodels/TestLibraryModels.java +++ b/test-library-models/src/main/java/com/uber/nullaway/testlibrarymodels/TestLibraryModels.java @@ -53,7 +53,9 @@ public ImmutableSetMultimap nullImpliesTrueParameters() { @Override public ImmutableSetMultimap nullImpliesFalseParameters() { - return ImmutableSetMultimap.of(); + return ImmutableSetMultimap.of( + methodRef("com.uber.lib.unannotated.UnannotatedWithModels", "isNonNull(java.lang.Object)"), + 0); } @Override @@ -63,7 +65,10 @@ public ImmutableSetMultimap nullImpliesNullParameters() { @Override public ImmutableSet nullableReturns() { - return ImmutableSet.of(methodRef("com.uber.Foo", "bar()")); + return ImmutableSet.of( + methodRef("com.uber.Foo", "bar()"), + methodRef("com.uber.lib.unannotated.UnannotatedWithModels", "returnsNullUnannotated()"), + methodRef("com.uber.lib.unannotated.UnannotatedWithModels", "returnsNullUnannotated2()")); } @Override