External Library Models: Adding support for @Nullable Method parameters #1006

akulk022 · 2024-07-22T17:45:12Z

With these changes we can read annotations from method parameters and use them with our External Library Models in the following manner.

Externally annotated code example:

@NullMarked
public class ParameterAnnotationExample {

    public static Integer add(Integer a, Integer b) {
        return a + b;
    }

    public static Object getNewObjectIfNull(@Nullable Object object) {
        if (object == null) {
            return new Object();
        } else {
            return object;
        }
    }
}

class Test {
    static ParameterAnnotationExample annotationExample = new ParameterAnnotationExample();
    static void testPositive() {
      // BUG: Diagnostic contains: passing @Nullable parameter 'null' where @NonNull is required
      annotationExample.add(5,null);
    }
    static void testNegative() {
      annotationExample.getNewObjectIfNull(null);
    }
}

In order to accomplish this, we had to change the astubx format to use fully-qualified names for argument and return types. This also required changes to JarInfer. These are backward-incompatible changes, so we have changed the magic number for astubx files.

… parameters

…nto library_model_nullable_params

msridhar · 2024-07-24T21:01:47Z

I wanted to summarize some thoughts and issues on how to move forward with this PR, to possibly get some help.

As part of #950, we need a way to store in an astubx file the explicitly @Nullable method parameters in a @NullMarked class; then, all other method parameters in that class can be properly treated as @NonNull. (We already added support for storing which classes are @NullUnmarked in a stub file.) The code to make use of this stub information is around here in LibraryModelsHandler, and this code requires that the library models have fully-qualified parameter type names for its lookups. Before, for some reason, we only stored simple type names for parameters in stub files. So, we went ahead and modified our stub file writing logic to try to write fully-qualified type names.

But this change, in turn, caused problems for JarInfer, as its logic for storing stub files still uses simple type names, but we are using shared parsing logic for stub files. (At least I think that's the issue; @akulk022 can you clarify why you had to make the change in InferredJARModelsHandler at line 246?) Bottom line this is a backwards-incompatible change to the astubx format, so we need to be careful with it.

I propose the following:

We use fully-qualified type names wherever possible. Not using fully-qualified names is potentially broken in the presence of method overloading; we might as well get it right.
We bump the file version magic number for astubx files that are written and include fully-qualified names.

A question becomes, what should we do with old astubx files that come in? In particular, I'm concerned about the astubx files in the android-jarinfer-models-sdk* artifacts. I think we either need to somehow support old astubx file versions through some "slow path" where we do a lookup based on simple type names, or we need to update the astubx files in those artifacts before the next release. I'm hoping we don't need to do both things, as I'm guessing if we update those artifacts, that will handle nearly all use of astubx in the wild, and for any other users they can just re-generate their astubx files using the latest release to update.

@lazaroclapp do you have any thoughts / feedback on this?

akulk022 · 2024-07-25T09:24:49Z

@msridhar I made the change on line 246 in InferredJARModelsHandler.java because the existing test case libraryModelNullableReturnsTest was failing since argAnnotCache now contains fully qualified names for the parameters and we were not able to perform a lookup in the cache after getting a simple type name. Do you think we should separate out the logic for handling @Nullable returns for Inferred JAR Models and Library Models?

msridhar · 2024-07-25T17:14:47Z

I see. I think this is in fact an argument for unifying the logic between these two handlers and only having one handler. Right now the code structure is quite confusing and leading to problems. Maybe we should do some refactoring and unification in a separate PR even before landing this one.

…

On Jul 25, 2024 at 02:25:10, Abhijit Kulkarni ***@***.***> wrote: @msridhar <https://github.com/msridhar> I made the change on line 246 in InferredJARModelsHandler.java because the existing test case libraryModelNullableReturnsTest was failing since argAnnotCache now contains fully qualified names for the parameters and we were not able to perform a lookup in the cache after getting a simple type name. Do you think we should separate out the logic for handling @nullable returns for Inferred JAR Models and Library Models? — Reply to this email directly, view it on GitHub <#1006 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABPEUIETSC2LUVSLMLXFCLZOC77NAVCNFSM6AAAAABLIZBTHSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENBZHA4DCMRTHE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

lazaroclapp · 2024-07-25T23:44:00Z

I propose the following:

We use fully-qualified type names wherever possible. Not using fully-qualified names is potentially broken in the presence of method overloading; we might as well get it right.

We bump the file version magic number for astubx files that are written and include fully-qualified names.

A question becomes, what should we do with old astubx files that come in? In particular, I'm concerned about the astubx files in the android-jarinfer-models-sdk* artifacts. I think we either need to somehow support old astubx file versions through some "slow path" where we do a lookup based on simple type names, or we need to update the astubx files in those artifacts before the next release. I'm hoping we don't need to do both things, as I'm guessing if we update those artifacts, that will handle nearly all use of astubx in the wild, and for any other users they can just re-generate their astubx files using the latest release to update.

@lazaroclapp do you have any thoughts / feedback on this?

I agree with the approach above, and we can even just error out if the astubx file doesn't match the magic number we expect (i.e. the next NullAway version only supports the android-jarinfer-models-sdk* artifacts that have the same version number, unless supporting both versions is trivial). Normally I'd be very hesitant to suggest this, but:

In current versions of JarInfer, the preferred mode has long been to rewrite the bytecode of the jar files, rather than generate .astubx files, except for the Android SDK models.
Since that's the only astubx jar we expect to see in practice (other than the JSpecify models), and we version it together with the main NullAway jar, we might as well require people to update them concurrently, so long as we don't silently fail.

This will imply regenerating android-jarinfer-models-sdk* for all the Android SDK versions we have historically supported (and ideally we should update the models to more recent AOSP/SDK versions, btw, but I know that's a bit of a pain).

Does this seem reasonable?

msridhar · 2024-07-26T01:17:22Z

Thanks @lazaroclapp! All sounds good to me. My only concern is the following:

This will imply regenerating android-jarinfer-models-sdk* for all the Android SDK versions we have historically supported (and ideally we should update the models to more recent AOSP/SDK versions, btw, but I know that's a bit of a pain).

Thinking more about this, will this involve building each of those SDK versions from source? That may be moderately to extremely painful...we can look into it. But I agree it's the best solution and we should see if we can do it.

msridhar · 2024-08-21T19:41:07Z

I'd like to unblock this PR. I propose we go ahead and evolve the astubx format with fully-qualified types, and I will find a way to update the android-jarinfer-models-sdk* as needed. I'm wondering if the following two-step process is best:

In a separate PR, get rid of InferredJarModelsHandler and consolidate any extra logic into the LibraryModelsHandler code.
Rebase this PR on top of that one, and here we do the evolution of the astubx format and update the magic number.

@akulk022 WDYT? If you think it's ok you can go ahead with 1 as a separate PR (or I can potentially look into it).

msridhar · 2024-09-04T23:12:06Z

Update here is that it's hard to get rid of InferredJarModelsHandler without first switching to fully-qualified types in astubx files. The logic here for looking up method parameter nullability strips down parameter types to simple names, which isn't compatible with how the standard handling of library models first constructs an optimized representation (based on fully-qualified types) and then does lookups into it. So at this point, I think it makes more sense to push forward this PR, which switches over to fully-qualified types, and then get rid of InferredJarModelsHandler.

codecov · 2024-09-05T01:32:04Z

Codecov Report

Attention: Patch coverage is 93.82716% with 5 lines in your changes missing coverage. Please review.

Project coverage is 87.54%. Comparing base (3418a6c) to head (e1cbcf9).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
.../uber/nullaway/libmodel/LibraryModelGenerator.java	91.83%	1 Missing and 3 partials ⚠️
...ava/com/uber/nullaway/handlers/StubxCacheUtil.java	83.33%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #1006      +/-   ##
============================================
+ Coverage     87.23%   87.54%   +0.31%     
- Complexity     2129     2138       +9     
============================================
  Files            83       83              
  Lines          6965     7003      +38     
  Branches       1356     1367      +11     
============================================
+ Hits           6076     6131      +55     
+ Misses          458      451       -7     
+ Partials        431      421      -10

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

This is in preparation for #1006. When working on that PR I realized we really need some unit tests to more easily debug various scenarios (right now all we have are integration tests). Plus this will make our code coverage reports more useful. This adds some infrastructure for unit tests and a couple of basic tests; we will add more as we go. Also fix a couple minor issues in `LibraryModelGenerator`.

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

msridhar

Looking promising! I have a few comments

jar-infer/jar-infer-lib/src/test/java/com/uber/nullaway/jarinfer/JarInferTest.java

msridhar · 2024-09-10T20:08:18Z

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

    private final Map<String, Set<Integer>> nullableUpperBounds;
+    // TODO this is sketchy!


I added this comment 🙂 I think we need to do for arrays whatever JarInfer is now doing. We should also add integration tests.

msridhar · 2024-09-10T20:10:48Z

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

+      ResolvedType returnType = md.getReturnType();
+      if (returnType.isReferenceType()) {
+        return returnType.asReferenceType().getQualifiedName().toString();
+      } else if (returnType.isArray()) {
        return ARRAY_RETURN_TYPE_STRING;


Probably this needs to be updated

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

msridhar · 2024-09-10T20:12:29Z

...rary-model-generator/src/test/java/com/uber/nullaway/libmodel/LibraryModelGeneratorTest.java

+          "@NullMarked",
+          "public class NullMarked{",
+          "//Only this class should appear under null marked classes",
+          "}",
+          "class NotNullMarked{",
+          "}"


Indentation

msridhar · 2024-09-10T20:12:50Z

...rary-model-generator/src/test/java/com/uber/nullaway/libmodel/LibraryModelGeneratorTest.java

+        };
+    ImmutableSet<String> expectedNullMarkedClasses = ImmutableSet.of("NullMarked");
+    runTest(
+        "NullableUpperBound.java",


Weird file name. Choose one of the classes actually included in the file.

...rary-model-generator/src/test/java/com/uber/nullaway/libmodel/LibraryModelGeneratorTest.java

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

msridhar · 2024-09-10T20:14:48Z

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

@@ -1287,20 +1287,23 @@ private <T> NameIndexedMap<T> makeOptimizedLookup(
  /** Constructs Library Models from stubx files */
  private static class ExternalStubxLibraryModels implements LibraryModels {

+    @SuppressWarnings("unused")


Get rid of suppression or delete the field

msridhar · 2024-09-10T20:17:09Z

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

+            if (index >= 0) {
+              mapBuilder.put(methodRef(className, methodName), index);
+            }


Why do we need to check if index >= 0? And don't we need to check if the annotation is @Nullable? This seems to add something to the map for any annotation. If we can, we should add a test so that if the annotation is @NonNull we skip it (not sure if this is possible).

I added the index>=0 check because argAnnotCache holds the annotation for the return of the method with the index -1 and for all the parameter annotations with their respective indices, should this have been handled in a different way? I can add the @Nullable check and the corresponding test case.

…nto library_model_nullable_params

…y elements

msridhar

Getting pretty close!

msridhar · 2024-09-21T15:52:56Z

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

+   * The file magic number for version 1 .astubx files. It should be the first four bytes of any
+   * compatible .astubx file.
+   */
+  private static final int VERSION_1_FILE_MAGIC_NUMBER = 481874642;


@lazaroclapp should we have a separate magic number (for the astubx file format) and version number?

...rary-model-generator/src/test/java/com/uber/nullaway/libmodel/LibraryModelGeneratorTest.java

msridhar

Thanks for this contribution @akulk022!

akulk022 added 2 commits July 12, 2024 21:40

default and nullable params initial changes

fb19551

Adding javaparser type solver to use fully qualified names for method…

d76a3b2

… parameters

akulk022 marked this pull request as draft July 22, 2024 17:45

akulk022 and others added 4 commits July 22, 2024 23:15

Merge branch 'master' into library_model_nullable_params

6ef1afe

clean up

9b7d5c1

Merge remote-tracking branch 'origin/library_model_nullable_params' i…

0182cf9

…nto library_model_nullable_params

specify javaparser version in one place

96be113

msridhar mentioned this pull request Jul 26, 2024

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

Open

6 tasks

Merge branch 'master' into library_model_nullable_params

9be47fe

Merge branch 'master' into library_model_nullable_params

528ff22

msridhar added 3 commits September 4, 2024 16:54

WIP

972c977

fix more tests

38f639f

more

4d375d1

msridhar mentioned this pull request Sep 6, 2024

Create basic unit tests for library model generation #1031

Merged

akulk022 and others added 7 commits September 6, 2024 22:15

resolving conflicts for merge

3f25628

Merge branch 'master' into library_model_nullable_params

8abe95e

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

updating test with fully qualified name

515bb29

fix another

e1daf7b

latest javaparser

0681d1f

Merge branch 'master' into library_model_nullable_params

5b29341

adding logic and test for checking null marked classes

81ec20c

msridhar reviewed Sep 10, 2024

View reviewed changes

akulk022 and others added 23 commits September 10, 2024 16:22

removing unnecessary suppress warning

e5dc95f

Merge remote-tracking branch 'origin/library_model_nullable_params' i…

e204963

…nto library_model_nullable_params

clean up

3a5946b

adding check for Nullable annotation

d83e024

Merge branch 'master' into library_model_nullable_params

8083b57

adding test case for explicitly @nonnull parameter

32f6abb

Merge remote-tracking branch 'origin/library_model_nullable_params' i…

ef50479

…nto library_model_nullable_params

Merge branch 'master' into library_model_nullable_params

13083c1

updating logic for using Array type qualified names

b0a25f4

updating logic for correctly taking the annotation for @nullable arra…

254ada4

…y elements

updating test case logic

10ddcd3

Merge branch 'master' into library_model_nullable_params

2edafa6

handling primitive return type scenario

c3f669c

adding test cases for JarInfer

8600bf9

test case

eb832dc

tweak test

0b898b9

another test

6c1cea8

fix test?

55dc4e2

more tests and tweaks

6bd3185

fix tests

1e8ac44

bump astubx version number

16ac993

suppress

f8fd564

add integration test for nullable array parameter

d24267c

msridhar reviewed Sep 21, 2024

View reviewed changes

akulk022 and others added 3 commits September 22, 2024 14:30

adding test case for void return type and updating the NullUnmarked test

29a902b

remove unnecessary locals

5a4e7cd

Merge branch 'master' into library_model_nullable_params

e1cbcf9

msridhar approved these changes Sep 23, 2024

View reviewed changes

msridhar merged commit f36ab25 into uber:master Sep 23, 2024
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

External Library Models: Adding support for @Nullable Method parameters #1006

External Library Models: Adding support for @Nullable Method parameters #1006

akulk022 commented Jul 22, 2024 •

edited by msridhar

Loading

msridhar commented Jul 24, 2024

akulk022 commented Jul 25, 2024

msridhar commented Jul 25, 2024 via email

lazaroclapp commented Jul 25, 2024

msridhar commented Jul 26, 2024

msridhar commented Aug 21, 2024

msridhar commented Sep 4, 2024

codecov bot commented Sep 5, 2024 •

edited

Loading

msridhar left a comment

msridhar Sep 10, 2024

msridhar Sep 10, 2024

msridhar Sep 10, 2024

msridhar Sep 10, 2024

msridhar Sep 10, 2024

msridhar Sep 10, 2024

akulk022 Sep 10, 2024 •

edited

Loading

msridhar left a comment

msridhar Sep 21, 2024

msridhar left a comment

		private final Map<String, Set<Integer>> nullableUpperBounds;
		// TODO this is sketchy!

External Library Models: Adding support for @Nullable Method parameters #1006

External Library Models: Adding support for @Nullable Method parameters #1006

Conversation

akulk022 commented Jul 22, 2024 • edited by msridhar Loading

msridhar commented Jul 24, 2024

akulk022 commented Jul 25, 2024

msridhar commented Jul 25, 2024 via email

lazaroclapp commented Jul 25, 2024

msridhar commented Jul 26, 2024

msridhar commented Aug 21, 2024

msridhar commented Sep 4, 2024

codecov bot commented Sep 5, 2024 • edited Loading

Codecov Report

msridhar left a comment

Choose a reason for hiding this comment

msridhar Sep 10, 2024

Choose a reason for hiding this comment

msridhar Sep 10, 2024

Choose a reason for hiding this comment

msridhar Sep 10, 2024

Choose a reason for hiding this comment

msridhar Sep 10, 2024

Choose a reason for hiding this comment

msridhar Sep 10, 2024

Choose a reason for hiding this comment

msridhar Sep 10, 2024

Choose a reason for hiding this comment

akulk022 Sep 10, 2024 • edited Loading

Choose a reason for hiding this comment

msridhar left a comment

Choose a reason for hiding this comment

msridhar Sep 21, 2024

Choose a reason for hiding this comment

msridhar left a comment

Choose a reason for hiding this comment

akulk022 commented Jul 22, 2024 •

edited by msridhar

Loading

codecov bot commented Sep 5, 2024 •

edited

Loading

akulk022 Sep 10, 2024 •

edited

Loading