-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Find annotation args in inline expansion #24895
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
Find annotation args in inline expansion #24895
Conversation
72117fd to
ebae84c
Compare
|
I'm glad the checks are green, but I had a local failure that I wonder if there's a way for the test rig to warn me if a failure includes a suspicious artifact. At the moment: I'm not going to investigate, but it would be nice if the test suite just ran (locally). Elsewhere, a PR includes tweaks to help run under JDK 25. Spurious warnings and errors are demoralizing and distracting. I'll add a simple test for the stability check in patterns. (Bootstrapping would fail, as it did for me spuriously, if that were broken. It complained about the vals in |
0515c55 to
cd320a8
Compare
|
Moving the second fix for |
jchyb
left a comment
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.
Thank you for the fix! (and for splitting the PR up - from what I understand, the API changes for stdlib are effectively frozen until 3.10, so the PR would have had to wait until then).
Overall the fix looks great, but I do have one question about the allArguments and allTermArguments parts.
| case Apply(fn, args) => allArguments(fn) ::: args | ||
| case TypeApply(fn, args) => allArguments(fn) ::: args | ||
| case Block(Nil, expr) => allArguments(expr) | ||
| case Inlined(_, _, expansion) => allArguments(expansion) | ||
| case _ => Nil |
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 don't really understand this part. It seems like we are removing the Inlined node from the function of Apply, rather than the argument, which seems unrelated to the rest of the PR.
Like for annotation @annot([Inlined] "someString")) allArguments will still return [Inlined] "someString"
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.
Likely! I didn't get back to write a test, a mental block. I'll follow up.
cd320a8 to
236bf2f
Compare
|
Dropped the ill-considered |
jchyb
left a comment
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.
Thank you!
Fixes #24894
Annotation args can be
inline transparent, butInliningphase doesn't inline into annotations.The
Inlinednode must be handled when emitting the annotation and also when inspecting args.