-
-
Notifications
You must be signed in to change notification settings - Fork 14.7k
Add arg splat experiment initial tuple impl #153697
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
base: main
Are you sure you want to change the base?
Changes from all commits
0a60096
a989358
d5f4bfa
9bbd303
8a0a4c6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| //! Attribute parsing for the `#[splat]` function argument overloading attribute. | ||
| //! This attribute modifies typecheck to support overload resolution, then modifies codegen for performance. | ||
|
|
||
| use super::prelude::*; | ||
|
|
||
| pub(crate) struct SplatParser; | ||
|
|
||
| impl<S: Stage> NoArgsAttributeParser<S> for SplatParser { | ||
| const PATH: &[Symbol] = &[sym::splat]; | ||
| const ON_DUPLICATE: OnDuplicate<S> = OnDuplicate::Warn; | ||
| const ALLOWED_TARGETS: AllowedTargets = AllowedTargets::AllowList(&[ | ||
| Allow(Target::Param), | ||
| // FIXME(splat): only allow MacroCall if the macro creates an argument | ||
| Allow(Target::MacroCall), | ||
| ]); | ||
| const CREATE: fn(Span) -> AttributeKind = AttributeKind::Splat; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1087,6 +1087,7 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> { | |
| liberated_sig.inputs().iter().copied(), | ||
| peeled_ty, | ||
| liberated_sig.c_variadic, | ||
| liberated_sig.splatted, | ||
| hir::Safety::Safe, | ||
| rustc_abi::ExternAbi::Rust, | ||
|
Comment on lines
1089
to
1092
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. preexisting, but imo this PR shows that we should do this: on main we could merge c_variadic, splatted, safety and ExternAbi into a separate struct, with some nice methods on it for creating default versions, making it easy to use structured update syntax in the cases where it's needed, and generally simplifying all the |
||
| )), | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -651,6 +651,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> { | |
| }; | ||
|
|
||
| // Special handling for the closure ABI: untuple the last argument. | ||
| // FIXME(splat): un-tuple splatted arguments that were tupled in typecheck | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't this meant to be desugared already during MIR building? That seems preferable. There's only one place where we do MIR building, but there's 3-4 MIR consumers in-tree depending on how you count, and a bunch more out-of-tree.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now that I re-read the discussion, that makes the most sense: I think I can de-tuple in MIR. It's a bit more complicated because it involves de-tupling both the caller and callee. But the code might end up nicer, because we'd just be splatting whatever's inside the tuple. So there's no messing around with different argument counts. Did you want that change in this PR?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note that MIR is generic, so this will only work if splatting doesn't support "generic forwarding". Is that meant to be possible?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure, the kind of overloading we're aiming for is in this UI test: I don't know if we'd want generic forwarding for more complex overloading situations. Would we need it to support custom C++ pointer types? Or any other interop features? Crubit wants custom auto traits eventually, they're currently using a combination of generated impls and blanket impls. (Maybe this is a broader lang + interop question.)
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. assuming we want splatting to replace
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I don't know. I haven't been following in detail. I think I also misunderstood what
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. imo it's up to the backend, but an optimizing backend should untuple the arguments
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmm @RalfJung @workingjubilee since this is a thing that solely exists for the Rust ABI, should this just be a field on
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if this can't just reuse the same logic as what RustCall already uses? It seems to have exactly the behavior we want -- in fact it's already more powerful than we need: with RustCall, caller and callee can disagree on whether splatting is happening. RustCall is really two features:
|
||
| let args: Cow<'_, [FnArg<'tcx, M::Provenance>]> = | ||
| if caller_abi == ExternAbi::RustCall && !args.is_empty() { | ||
| // Untuple | ||
|
|
||
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.
since it's a per-arg attribute, are there checks ensuring it's only used on the last arg?