External Library Models: Adding support for Nullable upper bounds of Generic Type parameters #949

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


  public InferredJARModelsHandler(Config config) {
    super();
    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

@@ -69,20 +63,22 @@ private static void LOG(boolean cond, String tag, String msg) {
  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.

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

5 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
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

External Library Models: Adding support for Nullable upper bounds of Generic Type parameters #949

External Library Models: Adding support for Nullable upper bounds of Generic Type parameters #949

akulk022 commented Apr 13, 2024 •

edited

Loading

codecov bot commented Apr 13, 2024 •

edited

Loading

msridhar left a comment

msridhar left a comment

msridhar Jun 4, 2024

akulk022 Jun 6, 2024

msridhar Jun 6, 2024

akulk022 Jun 6, 2024

msridhar Jun 4, 2024

msridhar Jun 4, 2024

msridhar Jun 4, 2024

msridhar left a comment

msridhar Jun 6, 2024

msridhar Jun 6, 2024

msridhar Jun 6, 2024

msridhar left a comment

msridhar Jun 7, 2024

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

External Library Models: Adding support for Nullable upper bounds of Generic Type parameters #949

External Library Models: Adding support for Nullable upper bounds of Generic Type parameters #949

Conversation

akulk022 commented Apr 13, 2024 • edited Loading

codecov bot commented Apr 13, 2024 • edited Loading

Codecov Report

msridhar left a comment

Choose a reason for hiding this comment

msridhar 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

msridhar 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

msridhar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akulk022 commented Apr 13, 2024 •

edited

Loading

codecov bot commented Apr 13, 2024 •

edited

Loading