Allow Library Models to override annotations. #624

nimakarimipour · 2022-07-13T17:13:36Z

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:

-XepOpt:NullAway:AllowLibraryModelsOverrideAnnotations=true

The main motivation for this PR is to prepare the foundation for Annotator to perform upstream API analysis and is an alternative way for GH-620.

nimakarimipour · 2022-07-13T17:18:45Z

Hi @lazaroclapp @msridhar. This PR is ready for review. Thank you.

lazaroclapp

Some style/naming/organization change requests, and one more test case needed, but the actual behavior of the change seems good to me!

lazaroclapp · 2022-07-13T17:24:22Z

nullaway/src/test/java/com/uber/nullaway/NullAwayCoreTests.java

+                "-d",
+                temporaryFolder.getRoot().getAbsolutePath(),
+                "-XepOpt:NullAway:AnnotatedPackages=com.uber",
+                "-XepOpt:NullAway:AllowLibraryModelsOverrideAnnotations=true"))


Worth adding the converse test. A test case showing that, when XepOpt:NullAway:AllowLibraryModelsOverrideAnnotations is not set (or set to false, if you think we might change the default), the library model for com.uber.Foo is still ignored.

lazaroclapp · 2022-07-13T17:31:50Z

nullaway/src/test/java/com/uber/nullaway/NullAwayCoreTests.java

@@ -709,4 +710,40 @@ public void staticCallZeroArgsNullCheck() {
            "}")
        .doTest();
  }
+
+  @Test
+  public void allowLibraryModelsOverrideAnnotations() {


Maybe worth it to have test cases for this in a new test class, rather than NullAwayCoreTests? Maybe a NullAwayCustomLibraryModelsTests.java (extending NullAwayTestsBase) which could:

Have a general method to get a test helper with TestLibraryModels.class already in the processor path: makeLibraryModelsTestHelperWithArgs

Include this test case and its converse (see comment below) in that new class.

Move libraryModelsOverrideRestrictiveAnnotations from NullAwayAcknowledgeRestrictiveAnnotationsTests to this new class, so we have all the test cases that load TestLibraryModels.class into -processorpath in the same place.

lazaroclapp · 2022-07-13T17:32:38Z

nullaway/src/test/java/com/uber/nullaway/NullAwayCoreTests.java

+            "       return bar();",
+            "   }",
+            "   void run() {",
+            "       // just to make sure, flow analysis is also impacted by library models information",


lazaroclapp · 2022-07-13T17:45:06Z

nullaway/src/main/java/com/uber/nullaway/AbstractConfig.java

@@ -349,4 +351,9 @@ public String getErrorURL() {
  public boolean acknowledgeAndroidRecent() {
    return acknowledgeAndroidRecent;
  }
+
+  @Override
+  public boolean allowLibraryModelsOverrideAnnotations() {


Ok, naming here could be improved. We already allow library models to override annotations (namely, they override restrictive annotations in "unannotated" code - ok, fine, that one was bad naming on our part! 😉 But the point remains...)

I would call this flag:

-XepOpt:NullAway:AcknowledgeLibraryModelsOfAnnotatedCode for the EP flag(I am also fine with "allow" vs "acknowledge", but I am thinking of consistency with some existing flags)

acknowledgeLibraryModelsOfAnnotatedCode() for the method and acknowledgeLibraryModelsOfAnnotatedCode for the field (I don't see what "activation" means or adds and it isn't necessary, see acknowledgeAndroidRecent for an example of how this looks like)

"Allow Library Models of annotated code" as the PR's title (side note, generally, imperative mood is recommended in commit subjects - this is part of Uber's Git commit message best practices, but I've also seen it as standard practice elsewhere in most major OSS projects. So, "Allow" instead of "Allowing" or "Allows").

Did all the renamings 13eb085
Thank you very much for the note, I will definitely follow this practice in future PRs and commits :)

lazaroclapp · 2022-07-13T17:46:11Z

nullaway/src/main/java/com/uber/nullaway/Config.java

@@ -292,4 +292,12 @@ public interface Config {
   *     similarly for {@code @RecentlyNonNull}
   */
  boolean acknowledgeAndroidRecent();
+
+  /**
+   * Checks if {@link LibraryModels} can override annotations on annotated source codes.


"source code". It's an uncountable noun (like "water"), unless you are talking about "return codes" or "opcodes" or some other discrete set of codes.

Fixed it fa145f2

msridhar · 2022-07-14T00:55:12Z

Removed myself from reviewers as I won't have time to look at this one. But the approach looks good!

nimakarimipour · 2022-07-15T10:03:41Z

Hi @lazaroclapp sorry I forgot to request a review when I resolved the comments, it is now ready for another round of review.

lazaroclapp

Some minor nits on this pass. Test refactoring looks good, though!

lazaroclapp · 2022-07-15T17:04:43Z

nullaway/src/main/java/com/uber/nullaway/Config.java

+   * 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 codes.


"source code"

lazaroclapp · 2022-07-15T17:05:35Z

nullaway/src/main/java/com/uber/nullaway/ErrorProneCLIFlagsConfig.java

@@ -84,6 +84,9 @@ final class ErrorProneCLIFlagsConfig extends AbstractConfig {
  static final String FL_FIX_SERIALIZATION_CONFIG_PATH =
      EP_FL_NAMESPACE + ":FixSerializationConfigPath";

+  static final String FL_ALLOW_LIBRARY_MODELS_OVERRIDE_ANNOTATIONS =


Missed this one on my original list, but let's change the name of this constant to also match the new name of the flag.

lazaroclapp · 2022-07-15T18:02:00Z

nullaway/src/test/java/com/uber/nullaway/NullAwayCustomLibraryModelsTests.java

+  private CompilationTestHelper makeLibraryModelsTestHelperWithArgs(List<String> args) {
+    // Adding directly to args will throw an UnsupportedOperationException. Throughout all tests in
+    // NullAway, list of args are created via Arrays.asList which the returned list does not support
+    // addAll. To comply with the rest of the tests, we created tests arguments with the same


This comment is a bit cumbersome to parse, at least to me, I would rewrite it as:

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.

…mipour/NullAway into nimak/library-models-controller

lazaroclapp

LGTM. Landing it once the build passes 🎉

This PR enables `Annotator` to process downstream dependencies while making decisions for public methods with non-primitive return types. This PR is a follow up for [#624](uber/NullAway#624) Before starting the main process, annotator will do the followings: 1. Collects `public` method with **non-primitive-return-type** 2. Collects regions in downstream dependencies that will potentially introduce a new error if a method in target module is annotated. 3. Constructs the conflict graph and computes the effect in the collected regions for each method. 4. Aggregates the result of analysis on each sub-module and makes a uniformed report. 5. Stores the effect on submodule and will provide the information to main process while making decisions. To activate this feature, flags below must be passed to `Annotator`: 1. `-ddbc or --downstream-dependencies-build-command` path to a file containing all downstream dependencies build commands separated by new line. 2. `-nlmlp or --nullaway-library-model-loader-path` path to library model loader for `NullAway`. This PR also adds javadoc / perform refactoring on huge segment of the code.

This is a follow up to changes in #639, #636, and #624. Here we generalize the former Handler callback: - onUnannotatedInvocationGetExplicitlyNonNullReturn which was used to change the nullability of method returns in unannotated code for the purpose of method `@Override` checks (i.e. subtyping). We provide instead: - onOverrideMethodInvocationReturnNullability Which allows handlers to change the nullability of method returns for both annotated and unannotated code, for the same purposes. Additionally, we implement this new handler callback in `LibraryModelsHandler`. Before this change, marking a return as non-null in a library model affected checking of method invocations, but not necessarily overriding/subtyping checks.

…#641) This is a follow up to changes in #639, #636, and #624. Here we generalize the former Handler callback: - onUnannotatedInvocationGetExplicitlyNonNullReturn which was used to change the nullability of method returns in unannotated code for the purpose of method `@Override` checks (i.e. subtyping). We provide instead: - onOverrideMethodInvocationReturnNullability Which allows handlers to change the nullability of method returns for both annotated and unannotated code, for the same purposes. Additionally, we implement this new handler callback in `LibraryModelsHandler`. Before this change, marking a return as non-null in a library model affected checking of method invocations, but not necessarily overriding/subtyping checks.

init

2c96902

nimakarimipour requested review from msridhar and lazaroclapp July 13, 2022 17:18

lazaroclapp reviewed Jul 13, 2022

View reviewed changes

msridhar removed their request for review July 14, 2022 00:54

nimakarimipour added 2 commits July 14, 2022 15:18

Added test for flag turned off

9b141bc

moved to a separate class

54a149c

nimakarimipour changed the title ~~Allowing Library Models override annotations.~~ Allow Library Models override annotations. Jul 15, 2022

nimakarimipour added 3 commits July 14, 2022 19:13

Rename to AcknowledgeLibraryModelsOfAnnotatedCode

13eb085

update javadoc

fa145f2

Merge branch 'master' into nimak/library-models-controller

a559d4c

nimakarimipour requested a review from lazaroclapp July 15, 2022 10:03

lazaroclapp reviewed Jul 15, 2022

View reviewed changes

lazaroclapp changed the title ~~Allow Library Models override annotations.~~ Allow Library Models to override annotations. Jul 15, 2022

lazaroclapp mentioned this pull request Jul 15, 2022

Upstream API Analysis #620

Closed

nimakarimipour added 4 commits July 15, 2022 13:17

update javadoc

1391b99

Merge branch 'nimak/library-models-controller' of github.com:nimakari…

3861780

…mipour/NullAway into nimak/library-models-controller

rename field

25a99bf

updated comment

15cef2f

lazaroclapp approved these changes Jul 15, 2022

View reviewed changes

lazaroclapp merged commit 4cef771 into uber:master Jul 15, 2022

nimakarimipour deleted the nimak/library-models-controller branch July 15, 2022 20:33

nimakarimipour mentioned this pull request Jul 20, 2022

Enable downstream dependency analysis with updated javadoc ucr-riple/NullAwayAnnotator#26

Merged

lazaroclapp mentioned this pull request Aug 24, 2022

Generalize handler APIs for return nullability on (un-)annotated code #641

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow Library Models to override annotations. #624

Allow Library Models to override annotations. #624

nimakarimipour commented Jul 13, 2022 •

edited by lazaroclapp

nimakarimipour commented Jul 13, 2022

lazaroclapp left a comment

lazaroclapp Jul 13, 2022

nimakarimipour Jul 14, 2022

lazaroclapp Jul 13, 2022

nimakarimipour Jul 15, 2022

lazaroclapp Jul 13, 2022

lazaroclapp Jul 13, 2022

nimakarimipour Jul 15, 2022

lazaroclapp Jul 13, 2022

nimakarimipour Jul 15, 2022

msridhar commented Jul 14, 2022

nimakarimipour commented Jul 15, 2022

lazaroclapp left a comment

lazaroclapp Jul 15, 2022

nimakarimipour Jul 15, 2022

lazaroclapp Jul 15, 2022

nimakarimipour Jul 15, 2022

lazaroclapp Jul 15, 2022

nimakarimipour Jul 15, 2022

lazaroclapp left a comment

Allow Library Models to override annotations. #624

Allow Library Models to override annotations. #624

Conversation

nimakarimipour commented Jul 13, 2022 • edited by lazaroclapp

nimakarimipour commented Jul 13, 2022

lazaroclapp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msridhar commented Jul 14, 2022

nimakarimipour commented Jul 15, 2022

lazaroclapp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lazaroclapp left a comment

Choose a reason for hiding this comment

nimakarimipour commented Jul 13, 2022 •

edited by lazaroclapp