-
Notifications
You must be signed in to change notification settings - Fork 299
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
Update handling of annotations on varargs argument #1025
Merged
Merged
Changes from 1 commit
Commits
Show all changes
29 commits
Select commit
Hold shift + click to select a range
145f0e3
WIP
msridhar 50862eb
WIP
msridhar fbd131e
format
msridhar db9c3b1
test case
msridhar d25d350
tweak test
msridhar a466b27
WIP
msridhar 9f37626
WIP
msridhar c45b44c
more
msridhar 3d086f3
WIP
msridhar e75c938
WIP
msridhar 98b3d12
WIP
msridhar f32dd87
fix for nullable varargs arrays
msridhar 65dd597
JSpecify mode tests
msridhar c85fec1
initial legacy mode tests
msridhar 0878e5b
WIP
msridhar 91fe3fe
fixes
msridhar c524432
comments
msridhar 252e6c3
tweaks
msridhar 925265d
clarify comment
msridhar 58ece61
comments
msridhar 4068568
suggested comment
msridhar fe92b7d
comment suggestion
msridhar 62a7df5
Apply suggestions from code review
msridhar 893fabe
formatting
msridhar fee21e4
Apply suggestions from code review
msridhar 71301a3
more tests of passing null varargs array
msridhar bc46f2e
another test
msridhar 6b622c7
@Nullable declaration annotation makes varargs array elements @Nullab…
msridhar 535af4f
logic simplification
msridhar File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
another test
- Loading branch information
commit bc46f2e448887afa05f03cb879a403faf2013325
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
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.
Shouldn't we be checking in here that the elements are
@Nullable
? By adding the for loop and trying to deref `other? Or is the table saying that in JSpecify mode (and every mode other than legacy) we don't check declaration annotations (but acknowledge them for call sites)?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 did add the test in bc46f2e. And I have now changed the logic so that we get an error, in 6b622c7. Great catch! My table didn't have this case as an error before, due to an attempt to make treatment of array types identical for regular arrays and varargs arrays. But there are already other cases where we don't have identical treatment. And the previous table made a
@Nullable
declaration annotation a no-op within the method body, which could be confusing. So this is now fixed, and I will update the table accordingly.