Allow Library Models to override annotations. (#624)
Currently `LibraryModels` can only give information regarding **unannotated** source code. This PR enables `LibraryModels` to override annotations in an **annotated** source code. This behavior is controlled by a flag.
To enable this feature, the following error prone flag must be passed:
```shell
-XepOpt:NullAway:AllowLibraryModelsOverrideAnnotations=true
```
The main motivation for this PR is to prepare the foundation for [Annotator](https://github.com/nimakarimipour/NullAwayAnnotator) to perform upstream API analysis and is an alternative way for #620.
diff --git a/nullaway/src/main/java/com/uber/nullaway/AbstractConfig.java b/nullaway/src/main/java/com/uber/nullaway/AbstractConfig.java
index 57af691..3611c5f 100644
--- a/nullaway/src/main/java/com/uber/nullaway/AbstractConfig.java
+++ b/nullaway/src/main/java/com/uber/nullaway/AbstractConfig.java
@@ -129,6 +129,8 @@
protected FixSerializationConfig fixSerializationConfig;
+ protected boolean acknowledgeLibraryModelsOfAnnotatedCode;
+
@Override
public boolean serializationIsActive() {
return serializationActivationFlag;
@@ -349,4 +351,9 @@
public boolean acknowledgeAndroidRecent() {
return acknowledgeAndroidRecent;
}
+
+ @Override
+ public boolean acknowledgeLibraryModelsOfAnnotatedCode() {
+ return acknowledgeLibraryModelsOfAnnotatedCode;
+ }
}
diff --git a/nullaway/src/main/java/com/uber/nullaway/Config.java b/nullaway/src/main/java/com/uber/nullaway/Config.java
index f727093..5055b86 100644
--- a/nullaway/src/main/java/com/uber/nullaway/Config.java
+++ b/nullaway/src/main/java/com/uber/nullaway/Config.java
@@ -292,4 +292,12 @@
* similarly for {@code @RecentlyNonNull}
*/
boolean acknowledgeAndroidRecent();
+
+ /**
+ * Checks if {@link LibraryModels} can override annotations on annotated source code.
+ *
+ * @return true if NullAway should use information provided by {@link LibraryModels} on annotated
+ * source code.
+ */
+ boolean acknowledgeLibraryModelsOfAnnotatedCode();
}
diff --git a/nullaway/src/main/java/com/uber/nullaway/DummyOptionsConfig.java b/nullaway/src/main/java/com/uber/nullaway/DummyOptionsConfig.java
index 9fcfb543..0aebd44 100644
--- a/nullaway/src/main/java/com/uber/nullaway/DummyOptionsConfig.java
+++ b/nullaway/src/main/java/com/uber/nullaway/DummyOptionsConfig.java
@@ -215,4 +215,9 @@
public boolean acknowledgeAndroidRecent() {
throw new IllegalStateException(ERROR_MESSAGE);
}
+
+ @Override
+ public boolean acknowledgeLibraryModelsOfAnnotatedCode() {
+ 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 06923c3..49ba811 100644
--- a/nullaway/src/main/java/com/uber/nullaway/ErrorProneCLIFlagsConfig.java
+++ b/nullaway/src/main/java/com/uber/nullaway/ErrorProneCLIFlagsConfig.java
@@ -84,6 +84,9 @@
static final String FL_FIX_SERIALIZATION_CONFIG_PATH =
EP_FL_NAMESPACE + ":FixSerializationConfigPath";
+ static final String FL_ACKNOWLEDGE_LIBRARY_MODELS_OF_ANNOTATED_CODE =
+ EP_FL_NAMESPACE + ":AcknowledgeLibraryModelsOfAnnotatedCode";
+
private static final String DELIMITER = ",";
static final ImmutableSet<String> DEFAULT_CLASS_ANNOTATIONS_TO_EXCLUDE =
@@ -248,6 +251,8 @@
+ FL_SUGGEST_SUPPRESSIONS
+ ")");
}
+ acknowledgeLibraryModelsOfAnnotatedCode =
+ flags.getBoolean(FL_ACKNOWLEDGE_LIBRARY_MODELS_OF_ANNOTATED_CODE).orElse(false);
}
private static ImmutableSet<String> getFlagStringSet(ErrorProneFlags flags, String flagName) {
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 9a075c2..3252e4f 100644
--- a/nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java
+++ b/nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java
@@ -107,7 +107,9 @@
if (expr.getKind() == Tree.Kind.METHOD_INVOCATION) {
OptimizedLibraryModels optLibraryModels = getOptLibraryModels(state.context);
Symbol.MethodSymbol methodSymbol = (Symbol.MethodSymbol) ASTHelpers.getSymbol(expr);
- if (!getClassAnnotationInfo(state.context).isSymbolUnannotated(methodSymbol, this.config)) {
+ if (!config.acknowledgeLibraryModelsOfAnnotatedCode()
+ && !getClassAnnotationInfo(state.context)
+ .isSymbolUnannotated(methodSymbol, this.config)) {
// We only look at library models for unannotated (i.e. third-party) code.
return exprMayBeNull;
} else if (optLibraryModels.hasNullableReturn(methodSymbol, state.getTypes())
@@ -167,7 +169,8 @@
AccessPathNullnessPropagation.Updates bothUpdates) {
Symbol.MethodSymbol callee = ASTHelpers.getSymbol(node.getTree());
Preconditions.checkNotNull(callee);
- if (!getClassAnnotationInfo(context).isSymbolUnannotated(callee, this.config)) {
+ if (!config.acknowledgeLibraryModelsOfAnnotatedCode()
+ && !getClassAnnotationInfo(context).isSymbolUnannotated(callee, this.config)) {
// Ignore annotated methods, library models should only apply to "unannotated" code.
return NullnessHint.UNKNOWN;
}
diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayAcknowledgeRestrictiveAnnotationsTests.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayAcknowledgeRestrictiveAnnotationsTests.java
index 479e5ca..1c1b099 100644
--- a/nullaway/src/test/java/com/uber/nullaway/NullAwayAcknowledgeRestrictiveAnnotationsTests.java
+++ b/nullaway/src/test/java/com/uber/nullaway/NullAwayAcknowledgeRestrictiveAnnotationsTests.java
@@ -1,6 +1,5 @@
package com.uber.nullaway;
-import com.uber.nullaway.testlibrarymodels.TestLibraryModels;
import java.util.Arrays;
import org.junit.Test;
@@ -292,44 +291,4 @@
"}")
.doTest();
}
-
- @Test
- public void libraryModelsOverrideRestrictiveAnnotations() {
- makeTestHelperWithArgs(
- Arrays.asList(
- "-processorpath",
- TestLibraryModels.class
- .getProtectionDomain()
- .getCodeSource()
- .getLocation()
- .getPath(),
- "-d",
- temporaryFolder.getRoot().getAbsolutePath(),
- "-XepOpt:NullAway:AnnotatedPackages=com.uber",
- "-XepOpt:NullAway:UnannotatedSubPackages=com.uber.lib.unannotated",
- "-XepOpt:NullAway:AcknowledgeRestrictiveAnnotations=true"))
- .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, overridden by library model",
- " // BUG: Diagnostic contains: passing @Nullable parameter 'null' where @NonNull",
- " f.apply(null);",
- " }",
- " void foo() {",
- " RestrictivelyAnnotatedFIWithModelOverride func = (x) -> {",
- " // Param is @NullableDecl in bytecode, overridden by library model, thus safe",
- " return x.toString();",
- " };",
- " }",
- " void baz() {",
- " // Safe to pass, since Function can't have a null instance parameter",
- " bar(Object::toString);",
- " }",
- "}")
- .doTest();
- }
}
diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayCustomLibraryModelsTests.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayCustomLibraryModelsTests.java
new file mode 100644
index 0000000..6cafcef
--- /dev/null
+++ b/nullaway/src/test/java/com/uber/nullaway/NullAwayCustomLibraryModelsTests.java
@@ -0,0 +1,139 @@
+/*
+ * 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;
+
+import com.google.errorprone.CompilationTestHelper;
+import com.uber.nullaway.testlibrarymodels.TestLibraryModels;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+import org.junit.Test;
+
+public class NullAwayCustomLibraryModelsTests extends NullAwayTestsBase {
+
+ private CompilationTestHelper makeLibraryModelsTestHelperWithArgs(List<String> args) {
+ // Adding directly to args will throw an UnsupportedOperationException, since that list is
+ // created by calling Arrays.asList (for consistency with the rest of NullAway's test cases),
+ // which produces a list which doesn't support add/addAll. Because of this, before we add our
+ // additional arguments, we must first copy the list into a mutable ArrayList.
+ List<String> extendedArguments = new ArrayList<>(args);
+ extendedArguments.addAll(
+ 0,
+ Arrays.asList(
+ "-processorpath",
+ TestLibraryModels.class.getProtectionDomain().getCodeSource().getLocation().getPath()));
+ return makeTestHelperWithArgs(extendedArguments);
+ }
+
+ @Test
+ public void allowLibraryModelsOverrideAnnotations() {
+ makeLibraryModelsTestHelperWithArgs(
+ Arrays.asList(
+ "-d",
+ temporaryFolder.getRoot().getAbsolutePath(),
+ "-XepOpt:NullAway:AnnotatedPackages=com.uber",
+ "-XepOpt:NullAway:AcknowledgeLibraryModelsOfAnnotatedCode=true"))
+ .addSourceLines(
+ "Foo.java",
+ "package com.uber;",
+ "public class Foo {",
+ " Object field = new Object();",
+ " Object bar() {",
+ " return new Object();",
+ " }",
+ " Object nullableReturn() {",
+ " // BUG: Diagnostic contains: returning @Nullable",
+ " return bar();",
+ " }",
+ " void run() {",
+ " // just to make sure, flow analysis is also impacted by library models information",
+ " Object temp = bar();",
+ " // BUG: Diagnostic contains: assigning @Nullable",
+ " this.field = temp;",
+ " }",
+ "}")
+ .doTest();
+ }
+
+ @Test
+ public void allowLibraryModelsOverrideAnnotationsFlagTest() {
+ makeLibraryModelsTestHelperWithArgs(
+ Arrays.asList(
+ "-d",
+ temporaryFolder.getRoot().getAbsolutePath(),
+ "-XepOpt:NullAway:AnnotatedPackages=com.uber",
+ "-XepOpt:NullAway:AcknowledgeLibraryModelsOfAnnotatedCode=false"))
+ .addSourceLines(
+ "Foo.java",
+ "package com.uber;",
+ "public class Foo {",
+ " Object field = new Object();",
+ " Object bar() {",
+ " return new Object();",
+ " }",
+ " Object nullableReturn() {",
+ " return bar();",
+ " }",
+ " void run() {",
+ " // just to make sure, flow analysis is not impacted by library models information",
+ " Object temp = bar();",
+ " this.field = temp;",
+ " }",
+ "}")
+ .doTest();
+ }
+
+ @Test
+ public void libraryModelsOverrideRestrictiveAnnotations() {
+ makeLibraryModelsTestHelperWithArgs(
+ Arrays.asList(
+ "-d",
+ temporaryFolder.getRoot().getAbsolutePath(),
+ "-XepOpt:NullAway:AnnotatedPackages=com.uber",
+ "-XepOpt:NullAway:UnannotatedSubPackages=com.uber.lib.unannotated",
+ "-XepOpt:NullAway:AcknowledgeRestrictiveAnnotations=true"))
+ .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, overridden by library model",
+ " // BUG: Diagnostic contains: passing @Nullable parameter 'null' where @NonNull",
+ " f.apply(null);",
+ " }",
+ " void foo() {",
+ " RestrictivelyAnnotatedFIWithModelOverride func = (x) -> {",
+ " // Param is @NullableDecl in bytecode, overridden by library model, thus safe",
+ " return x.toString();",
+ " };",
+ " }",
+ " void baz() {",
+ " // Safe to pass, since Function can't have a null instance parameter",
+ " bar(Object::toString);",
+ " }",
+ "}")
+ .doTest();
+ }
+}
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 5fbe670..debf6b1 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
@@ -63,7 +63,7 @@
@Override
public ImmutableSet<MethodRef> nullableReturns() {
- return ImmutableSet.of();
+ return ImmutableSet.of(methodRef("com.uber.Foo", "bar()"));
}
@Override
