External Library Models: Adding support for Nullable upper bounds of Generic Type parameters by akulk022 · Pull Request #949 · uber/NullAway

akulk022 · 2024-04-13T14:48:41Z

With these changes we are able to read and create library models for Nullable upper bounds of generic type parameters from externally annotated source code as shown in the below example.

Externally annotated source code example:

public static class GenericExample<T extends @Nullable Object> {
        public T getNull() {
            return null;
        }
 }

public static class GenericExample<T> {
    public T getNull() {
      return null;
    }
}
static GenericExample<@Nullable Object> genericExample = new GenericExample<@Nullable Object>();
static void test(Object value){}
// BUG: Diagnostic contains: passing @Nullable parameter 'genericExample.getNull()'
test(genericExample.getNull());

…ernal library models

codecov · 2024-04-13T15:12:08Z

Codecov Report

Attention: Patch coverage is 66.90647% with 46 lines in your changes missing coverage. Please review.

Project coverage is 85.97%. Comparing base (6cec433) to head (71ce2e2).

❗ Current head 71ce2e2 differs from pull request most recent head 3f4c3e4

Please upload reports for the commit 3f4c3e4 to get more accurate results.

Files	Patch %	Lines
.../uber/nullaway/libmodel/LibraryModelGenerator.java	0.00%	23 Missing ⚠️
...ava/com/uber/nullaway/handlers/StubxCacheUtil.java	81.33%	9 Missing and 5 partials ⚠️
...n/java/com/uber/nullaway/libmodel/StubxWriter.java	27.27%	7 Missing and 1 partial ⚠️
...er/nullaway/handlers/InferredJARModelsHandler.java	75.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master     #949      +/-   ##
============================================
- Coverage     86.17%   85.97%   -0.20%     
- Complexity     2037     2046       +9     
============================================
  Files            81       82       +1     
  Lines          6691     6760      +69     
  Branches       1290     1301      +11     
============================================
+ Hits           5766     5812      +46     
- Misses          515      536      +21     
- Partials        410      412       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

msridhar

I have a few initial comments. I got lost reviewing the changes under nullaway/src/main/java/com/uber/nullaway/handlers. Can you describe the purpose of the new StubxCacheUtil class and how the refactored code works, for both old JarInfer stubs and newly generated ones?

...or/src/main/resources/sample_annotated/src/com/uber/nullaway/libmodel/AnnotationExample.java

...r-integration-test/src/test/java/com/uber/nullaway/libmodel/LibraryModelIntegrationTest.java

.../library-model-generator/src/main/java/com/uber/nullaway/libmodel/LibraryModelGenerator.java

nullaway/src/main/java/com/uber/nullaway/handlers/StubxCacheUtil.java

…nd' into library_model_nullable_upper_bound

msridhar

Looking mostly good! A few more comments

msridhar · 2024-06-04T18:51:30Z

...test-library-model-generator/src/main/java/com/uber/nullaway/libmodel/AnnotationExample.java

+  public static class UpperBoundExample<T> {
+
+    public T getNull() {
+      return null;
+    }
+  }


This is a bit weird since the method always returns null, so the return type should really be @Nullable T. Maybe, for readability, rewrite this class to have a public @Nullable field and return that field from getNull()? And maybe rename to getNullable()? Same changes in the annotated version

Should the return type of the method be @Nullable T in the unannotated version as well or only in the annotated version?

I'm not suggesting to change the return type to @Nullable T. I'm suggesting to have a field of type T and to return the value of that field, rather than just returning null. Then the return type T would be appropriate.

Oh that makes sense, thanks!

msridhar · 2024-06-04T18:55:34Z

.../library-model-generator/src/main/java/com/uber/nullaway/libmodel/LibraryModelGenerator.java

+      for (int i = 0; i < typeParamList.size(); i++) {
+        TypeParameter param = typeParamList.get(i);
+        for (ClassOrInterfaceType type : param.getTypeBound()) {
+          if (type.isAnnotationPresent(NULLABLE)) {


Don't we need logic here like we have in the isAnnotationNullable method, to handle fully-qualified annotations?

msridhar · 2024-06-04T19:01:52Z

nullaway/src/main/java/com/uber/nullaway/handlers/InferredJARModelsHandler.java

    this.config = config;
-    argAnnotCache = new LinkedHashMap<>();
-    loadStubxFiles();
+    this.cacheUtil = new StubxCacheUtil("JI");


Can we put "JI" in a suitably-named variable, for readability?

msridhar · 2024-06-04T19:02:32Z

nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java

+    private static final Map<String, Integer> upperBoundsCache;
+
+    static {
+      StubxCacheUtil cacheUtil = new StubxCacheUtil("LM");


Can we put "LM" in a suitably-named variable, for readability?

…nd' into library_model_nullable_upper_bound

…cify.annotations.Nullable

msridhar

few more comments, mostly minor

msridhar · 2024-06-06T23:24:24Z

...r-integration-test/src/test/java/com/uber/nullaway/libmodel/LibraryModelIntegrationTest.java

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


You need to also pass "-XepOpt:NullAway:JSpecifyMode=true" as an argument to get the error

msridhar · 2024-06-06T23:25:06Z

...r-integration-test/src/test/java/com/uber/nullaway/libmodel/LibraryModelIntegrationTest.java

+            "import org.jspecify.annotations.Nullable;",
+            "import com.uber.nullaway.libmodel.AnnotationExample;",
+            "class Test {",
+            "  //TODO: We should get an error here since jar infer is not enabled",


We should already get an error here if you pass the flag above

msridhar · 2024-06-06T23:31:28Z

nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java

+    private static final Map<String, Map<String, Map<Integer, Set<String>>>> argAnnotCache;
+    private static final Map<String, Integer> upperBoundsCache;


These fields should not be static! That makes them mutable global state. I think they can just be instance fields and everything will work.

Really, StubxCacheUtil.getArgAnnotCache() and StubxCacheUtil.getUpperBoundCache() should each return ImmutableMaps. But given the size of this PR, we can look into that change as a follow up.

msridhar

LGTM! One comment but it doesn't block this PR

msridhar · 2024-06-07T15:47:59Z

nullaway/src/main/java/com/uber/nullaway/handlers/InferredJARModelsHandler.java

  private final Map<String, Map<String, Map<Integer, Set<String>>>> argAnnotCache;

  private final Config config;
+  private final StubxCacheUtil cacheUtil;


As a high level comment, I'm a bit unsure why we still need this handler even after we added the new library models handler. Is it because of some Android-specific logic? We should clean this up, but maybe not for this PR.

This MR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [org.tukaani:xz](https://tukaani.org/xz/java.html) ([source](https://github.com/tukaani-project/xz-java)) | compile | minor | `1.9` -> `1.10` | | [org.eclipse:yasson](https://projects.eclipse.org/projects/ee4j.yasson) ([source](https://github.com/eclipse-ee4j/yasson)) | compile | patch | `3.0.3` -> `3.0.4` | | [org.eclipse.parsson:parsson](https://github.com/eclipse-ee4j/parsson) | compile | patch | `1.1.6` -> `1.1.7` | | [com.uber.nullaway:nullaway](https://github.com/uber/NullAway) | | patch | `0.11.0` -> `0.11.1` | | [io.hosuaby:inject-resources-junit-jupiter](https://github.com/hosuaby/inject-resources) | test | patch | `0.3.3` -> `0.3.5` | | [org.codehaus.mojo:exec-maven-plugin](https://www.mojohaus.org/exec-maven-plugin) ([source](https://github.com/mojohaus/exec-maven-plugin)) | build | minor | `3.3.0` -> `3.4.0` | --- ### Release Notes <details> <summary>tukaani-project/xz-java</summary> ### [`v1.10`](https://github.com/tukaani-project/xz-java/releases/tag/v1.10): XZ for Java 1.10 [Compare Source](tukaani-project/xz-java@v1.9...v1.10) </details> <details> <summary>eclipse-ee4j/yasson</summary> ### [`v3.0.4`](eclipse-ee4j/yasson@3.0.3...3.0.4) [Compare Source](eclipse-ee4j/yasson@3.0.3...3.0.4) </details> <details> <summary>eclipse-ee4j/parsson</summary> ### [`v1.1.7`](eclipse-ee4j/parsson@1.1.6...1.1.7) [Compare Source](eclipse-ee4j/parsson@1.1.6...1.1.7) </details> <details> <summary>uber/NullAway</summary> ### [`v0.11.1`](https://github.com/uber/NullAway/blob/HEAD/CHANGELOG.md#Version-0111) [Compare Source](uber/NullAway@v0.11.0...v0.11.1) - Fix issue 1008 ([#1009](uber/NullAway#1009)) - JSpecify: read upper bound annotations from bytecode and add tests ([#1004](uber/NullAway#1004)) - Fix crash with suggested suppressions in JSpecify mode ([#1001](uber/NullAway#1001)) - Update to JSpecify 1.0 and use JSpecify annotations in NullAway code ([#1000](uber/NullAway#1000)) - Expose [@EnsuresNonNull](https://github.com/EnsuresNonNull) and [@RequiresNonNull](https://github.com/RequiresNonNull) in annotations package ([#999](uber/NullAway#999)) - Don't report initializer warnings on [@NullUnmarked](https://github.com/NullUnmarked) constructors / methods ([#997](uber/NullAway#997)) - Strip annotations from MethodSymbol strings ([#993](uber/NullAway#993)) - JSpecify: fix crashes where declared parameter / return types were raw ([#989](uber/NullAway#989)) - JSpecify: Handle [@nullable](https://github.com/nullable) elements for enhanced-for-loops on arrays ([#986](uber/NullAway#986)) - Features/944 tidy stream nullability propagator ([#985](uber/NullAway#985)) - Tests for loops over arrays ([#982](uber/NullAway#982)) - Bug fixes for array subtyping at returns / parameter passing ([#980](uber/NullAway#980)) - JSpecify: Handle [@nonnull](https://github.com/nonnull) elements in [@nullable](https://github.com/nullable) content arrays ([#963](uber/NullAway#963)) - Don't report [@nullable](https://github.com/nullable) type argument errors for unmarked classes ([#958](uber/NullAway#958)) - External Library Models: Adding support for Nullable upper bounds of Generic Type parameters ([#949](uber/NullAway#949)) - Refactoring / code cleanups: - Test on JDK 22 ([#992](uber/NullAway#992)) - Add test case for [@nullable](https://github.com/nullable) Void with override in JSpecify mode ([#990](uber/NullAway#990)) - Enable UnnecessaryFinal and PreferredInterfaceType EP checks ([#991](uber/NullAway#991)) - Add missing [@test](https://github.com/test) annotation ([#988](uber/NullAway#988)) - Fix typo in variable name ([#987](uber/NullAway#987)) - Remove AbstractConfig class ([#974](uber/NullAway#974)) - Fix Javadoc for MethodRef ([#973](uber/NullAway#973)) - Refactored data clumps with the help of LLMs (research project) ([#960](uber/NullAway#960)) - Build / CI tooling maintenance: - Various cleanups enabled by bumping minimum Java and Error Prone versions ([#962](uber/NullAway#962)) - Disable publishing of snapshot builds from CI ([#967](uber/NullAway#967)) - Update Gradle action usage in CI workflow ([#969](uber/NullAway#969)) - Update Gradle config to always compile Java code using JDK 17 ([#971](uber/NullAway#971)) - Update JavaParser to 3.26.0 ([#970](uber/NullAway#970)) - Reenable JMH benchmarking in a safer manner ([#975](uber/NullAway#975)) - Updated JMH Benchmark Comment Action ([#976](uber/NullAway#976)) - Update to Gradle 8.8 ([#981](uber/NullAway#981)) - Update to Error Prone 2.28.0 ([#984](uber/NullAway#984)) - Update to Gradle 8.9 ([#998](uber/NullAway#998)) - Update to WALA 1.6.6 ([#1003](uber/NullAway#1003)) </details> <details> <summary>hosuaby/inject-resources</summary> ### [`v0.3.5`](hosuaby/inject-resources@v0.3.4...v0.3.5) [Compare Source](hosuaby/inject-resources@v0.3.4...v0.3.5) ### [`v0.3.4`](hosuaby/inject-resources@v0.3.3...v0.3.4) [Compare Source](hosuaby/inject-resources@v0.3.3...v0.3.4) </details> <details> <summary>mojohaus/exec-maven-plugin</summary> ### [`v3.4.0`](https://github.com/mojohaus/exec-maven-plugin/releases/tag/3.4.0) [Compare Source](mojohaus/exec-maven-plugin@3.3.0...3.4.0)  #### 🚀 New features and improvements - Allow `<includePluginDependencies>` to be specified for the exec:exec goal ([#432](mojohaus/exec-maven-plugin#432)) [@sebthom](https://github.com/sebthom) #### 🐛 Bug Fixes - Do not get UPPERCASE env vars ([#427](mojohaus/exec-maven-plugin#427)) [@wheezil](https://github.com/wheezil) #### 📦 Dependency updates - Bump org.codehaus.mojo:mojo-parent from 82 to 84 ([#434](mojohaus/exec-maven-plugin#434)) [@dependabot](https://github.com/dependabot) - Bump org.codehaus.plexus:plexus-xml from 3.0.0 to 3.0.1 ([#431](mojohaus/exec-maven-plugin#431)) [@dependabot](https://github.com/dependabot) #### 👻 Maintenance - Remove Log4j 1.2.x from ITs ([#437](mojohaus/exec-maven-plugin#437)) [@slawekjaranowski](https://github.com/slawekjaranowski) #### 🔧 Build - Use Maven 3.9.7 and 4.0.0-beta-3 ([#433](mojohaus/exec-maven-plugin#433)) [@slawekjaranowski](https://github.com/slawekjaranowski) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever MR is behind base branch, or you tick the rebase/retry checkbox. 👻 **Immortal**: This MR will be recreated if closed unmerged. Get [config help](https://github.com/renovatebot/renovate/discussions) if that's undesired. --- - [ ] If you want to rebase/retry this MR, check this box --- This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).

adding generic type parameter nullability upper bound support for ext…

628683b

…ernal library models

akulk022 marked this pull request as draft April 13, 2024 14:55

fix

0c4d694

akulk022 added 2 commits April 14, 2024 12:43

comment

ddaa68a

comment

d351cc9

msridhar mentioned this pull request Apr 16, 2024

Tracking issue: making jspecify/jdk library models available to NullAway users #950

Open

7 tasks

msridhar reviewed Apr 16, 2024

View reviewed changes

akulk022 and others added 10 commits June 3, 2024 23:30

writing and reading nullable upper bounds from stubx file

c3c37b0

Merge branch 'master' into library_model_nullable_upper_bound

8c48d3c

refactoring: changing class name

a28fa1c

Merge remote-tracking branch 'origin/library_model_nullable_upper_bou…

c1e7097

…nd' into library_model_nullable_upper_bound

fix

6cab303

docs

7d833db

add a comment

9b63aa7

Merge branch 'master' into library_model_nullable_upper_bound

d679ed0

comment

4186ca7

comment

f369817

msridhar reviewed Jun 4, 2024

View reviewed changes

akulk022 added 5 commits June 5, 2024 15:48

Merge branch 'master' into library_model_nullable_upper_bound

95b0cd0

Merge branch 'master' into library_model_nullable_upper_bound

99421aa

rewriting getNull method from UpperBoundExample and refactoring

a190b9f

Merge remote-tracking branch 'origin/library_model_nullable_upper_bou…

6220744

…nd' into library_model_nullable_upper_bound

updating Nullability logic for upper bounds to consider only org.jspe…

4f81dc0

…cify.annotations.Nullable

msridhar reviewed Jun 6, 2024

View reviewed changes

akulk022 added 2 commits June 7, 2024 20:48

making cache fields instance

71ce2e2

updating libraryModelNullableUpperBoundsWithoutJarInferTest

3f4c3e4

msridhar approved these changes Jun 7, 2024

View reviewed changes

msridhar marked this pull request as ready for review June 7, 2024 15:49

msridhar merged commit 866b140 into uber:master Jun 7, 2024

		private static final Map<String, Map<String, Map<Integer, Set<String>>>> argAnnotCache;
		private static final Map<String, Integer> upperBoundsCache;

Conversation

akulk022 commented Apr 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Apr 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

msridhar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

msridhar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

msridhar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

msridhar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

akulk022 commented Apr 13, 2024 •

edited

Loading

codecov bot commented Apr 13, 2024 •

edited

Loading