Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
External Library Models Integration #922
External Library Models Integration #922
Changes from 45 commits
1d48e6e
b0631cb
e5353d1
ea7d117
e9a06dc
19b020b
31f569c
a26f856
a863363
ee56362
3806c80
32f0990
3dedc77
07586ae
4b7891e
10c36e3
36b5a21
a6c8204
0c2cd44
ac14c99
2dbc283
32e80d5
2b637f1
888c1b2
fb81869
e889dc3
26efa96
cbe7bf5
600164c
d1fed93
db993b8
09c0f4c
161c6ac
27ebcc9
5ee2428
8c442f7
609f2e5
fb83aff
629ec5b
5190597
fd7d546
f213d82
1137aa9
6e0ef33
41bc90f
26c4907
dc0618a
5fa73da
14e39b6
f390cd1
ad44788
2a1c2da
8f86b36
35b988c
c1fe36b
d301e28
439ca14
e2a9c60
a072bbd
75785fa
80be0d4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this CLI going to be a new maven artifact on the next (or a soon to come) NullAway release?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't have an immediate plan to release the new CLI as a Maven artifact. We are still missing many features; this PR is just to get something barely functional into place. Also, for jspecify/jdk, rather than distributing the CLI tool, my thought was that we would eventually distribute a jar with the JDK astubx file inside. And then, maybe NullAway could depend on that jar (eventually), or users who wanted those models could add a dependency to opt in to using them. Or maybe the astubx file for the JDK should just eventually be included as part of the main NullAway jar?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say a separate artifact, like we do for the JarInfered Android SDK (but hopefully with a more automated process to keep the jar up to date...). I do think eventually providing the CLI tool might be useful too (someone might want to patch the JDK to remove/add a few annotations, or generate stub models for some other third-party library they don't control), but agree there is no rush for that.
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not that I care about this anymore, but I suspect Uber would want these copyright headers to be updated for the right years 😉 (e.g.
2024
here and20[...]-2024
for the files that were altered, I think)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check the number of args and print a basic usage message? (Not sure if we do that for JarInfer, and I don't think we need a full argument parser here yet, a check on the length of
args
would suffice for now)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @yuxincs that overloading the meaning of the
JarInfer[...]
CLI options for NullAway for this is bad practice, specially because other than the Android SDK, JarInfer no longer uses.astubx
but actually modifies the bytecode of jars to add annotations (so this options aren't used with JarInfer except for that Android SDK jar).It can certainly be a follow up PR, but I'd be in favor of rethinking these CLI flags. Maybe
-XepOpt:NullAway:JarInferEnabled
is actually something like-XepOpt:NullAway:ScanClassPathForAstubXModels
? Is there a better name? A nicer way to specify which.astubx
files we want to load without breaking things in our build internally? (I think there is! We only really care about that Android SDK astubx file right now, so it's not like this changes for each target like it used to, cc: @yuxincs )There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am in favor of changing these, but maybe in a follow-up PR. I don't think we need to go as far as keeping the old flag but deprecating it, as long as we document properly in release notes. But we should probably check as to what this breaks (if anything) internally at Uber.
@akulk022 can you open an issue on renaming these flags?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've created the issue #940
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fun! This wasn't possible in my version of the
astubx
format, correct? 🙂Edit: Ah, never mind, it uses the same position of annotation as the case above, we haven't introduced yet a way to annotate both the elements of an array and the array itself, correct? But this parses the annotation in the correct/JSpecify location.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you are correct. jspecify/jdk puts the annotation in the right spot, so if you parse it the incorrect way you get incorrect specs :-) You're right that we don't yet support annotations in both positions. Do you think this needs a code comment @lazaroclapp?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't hurt, but I also think that behavior will get less and less surprising with time as the "JSpecify way" becomes the default overall for NullAway and arrays (at least for type-use annotations).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a test case without the
-XepOpt:NullAway:JarInferEnabled
flag or whatever we end up calling it, to check that this functionality is off without it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, may I suggest a case with a model adding
@NonNull
to a method argument, just for completeness here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lazaroclapp We have not added the functionality for processing the annotations on method parameters in this PR so a test case with
@NonNull
on a method parameter will not be supported on these changes.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also @lazaroclapp I don't think supporting
@NonNull
on parameters will be a priority. Instead we are focusing on supporting treatment of a modeled class as@NullMarked
, so everything is@NonNull
by default, and then pulling in any explicit@Nullable
annotations on parameters into the modelThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There isn't a chance that specific classes will be
@NullUnmarked
with a few@NonNull
locations? Either way, understood, not needed as part of this PR. I do feel in the medium term we might get surprised by the lack of symmetry here when using this for other kinds of library models.