Open akulk022 opened 1 month ago
@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?
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: @.***>
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 theastubx
files in theandroid-jarinfer-models-sdk*
artifacts. I think we either need to somehow support oldastubx
file versions through some "slow path" where we do a lookup based on simple type names, or we need to update theastubx
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 ofastubx
in the wild, and for any other users they can just re-generate theirastubx
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:
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.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?
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.
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:
InferredJarModelsHandler
and consolidate any extra logic into the LibraryModelsHandler
code.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).
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
.
Attention: Patch coverage is 89.23077%
with 7 lines
in your changes missing coverage. Please review.
Project coverage is 87.26%. Comparing base (
e2e5394
) to head (5b29341
).
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
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 inLibraryModelsHandler
, 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 theastubx
format, so we need to be careful with it.I propose the following:
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 theastubx
files in theandroid-jarinfer-models-sdk*
artifacts. I think we either need to somehow support oldastubx
file versions through some "slow path" where we do a lookup based on simple type names, or we need to update theastubx
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 ofastubx
in the wild, and for any other users they can just re-generate theirastubx
files using the latest release to update.@lazaroclapp do you have any thoughts / feedback on this?