External Library Models: Adding support for @Nullable Method parameters

[akulk022]

[msridhar]

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]

@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]

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 https://github.com/uber/NullAway/pull/1006#issuecomment-2249881239, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABPEUIETSC2LUVSLMLXFCLZOC77NAVCNFSM6AAAAABLIZBTHSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENBZHA4DCMRTHE . You are receiving this because you were mentioned.Message ID: @.***>

[lazaroclapp]

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]

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]

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:

1. In a separate PR, get rid of InferredJarModelsHandler and consolidate any extra logic into the LibraryModelsHandler code.
2. 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]

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[bot]]

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 :warning:
...ava/com/uber/nullaway/handlers/StubxCacheUtil.java	83.33%	0 Missing and 1 partial :warning:

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     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.