Add arg splat experiment initial tuple impl#153697
Add arg splat experiment initial tuple impl#153697teor2345 wants to merge 11 commits intorust-lang:mainfrom
Conversation
|
r? @JohnTitor rustbot has assigned @JohnTitor. Use Why was this reviewer chosen?The reviewer was selected based on:
|
This comment has been minimized.
This comment has been minimized.
|
It should be better for someone on https://rust-lang.zulipchat.com/#narrow/channel/213817-t-lang/topic/On.20overloading/with/573924937 to review this, @oli-obk could you take over? |
|
Let's wait for the ongoing discussion on Zulip to figure out whether we need to have a proc macro, an AST manipulating attribute (like |
89102bf to
c784a57
Compare
c784a57 to
2d9e563
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
d83e6d9 to
d4229d6
Compare
This comment has been minimized.
This comment has been minimized.
d4229d6 to
482f842
Compare
|
The perf issues above are resolved by a minor refactor, but I'd like to merge them as separate PRs/commits, just in case we need to go back to the original code for testing (or further refactors). See the perf analysis here, overall this PR plus the refactor in #155852 results in an 0.24% primary benchmark improvement, with no primary regressions: |
482f842 to
84cab65
Compare
This comment was marked as outdated.
This comment was marked as outdated.
|
|
||
| // Multiple splatted arguments are invalid: we can't know which arguments go in each splat. | ||
| if splatted_arg_indexes.len() > 1 { | ||
| self.dcx().emit_err(errors::DuplicateSplattedArgs { spans: splatted_spans.clone() }); |
There was a problem hiding this comment.
I'm somewhat worried that due to these errors being non-blocking for the rest of the compiler, that it's possible to cause weird ICEs, but that's nothing new wrt most of the ast validation checks.
There was a problem hiding this comment.
I did my best to provide sensible defaults/substitutions on error.
Specifically, c_variadic gets priority, then the first splatted argument. Every other splat generates an error, then gets ignored in later stages.
| /// Which function argument is splatted into multiple arguments in callers, if any? | ||
| /// Splatting functions with `u16::MAX` arguments is not supported, see `FnSigKind` for | ||
| /// details. | ||
| splatted: u16, |
There was a problem hiding this comment.
far future possibility: make newtype_index allow for types other than u32 to be used for the storage. Then we could just encode a new ArgIdx type that inherently forbids u16::MAX (and maybe a few others at the upper limit to allow for more niche opts)
There was a problem hiding this comment.
also 😭 why do we allow more than 250 arguments on functions
There was a problem hiding this comment.
It might make the compiler faster to limit it to 256 arguments, I wonder if that would break any code.
At least we test for u16::MAX arguments. It used to panic, see #88577.
| /// Create a new FnDeclKind with no implicit self, no lifetime elision, and no C-style variadic argument. | ||
| /// Marker index for "no splatted argument". | ||
| /// Must have the same value as `FnSigKind::NO_SPLATTED_ARG_INDEX` and `rustc_ast::FnDecl::NO_SPLATTED_ARG_INDEX`. | ||
| const NO_SPLATTED_ARG_INDEX: u16 = u16::MAX; |
There was a problem hiding this comment.
can these all be linked somehow by depending on them in the argument? Or are there two of them who don't have the others' crate in their dependency list?
There was a problem hiding this comment.
I tried, but there's no common dependency crate to put the constant in.
Some of these instances will disappear in #155852 as part of a perf refactor (if it still gives good perf after recent nearby changes).
| let ocx_error = | ||
| ocx.eq(&origin, self.param_env, calee_tuple_type, new_tupled_type); | ||
| if let Err(ocx_error) = ocx_error { | ||
| struct_span_code_err!( |
There was a problem hiding this comment.
is this error reachable? if so, please add a test specifically for it
There was a problem hiding this comment.
I'm not sure if it's reachable, and I don't know how to create a test for this. It seems like other errors happen first, but there might be a way to trigger it. I left a FIXME in the code.
I made a bunch of related fixes and tests while trying to test this:
- fixed some invalid method splatted
selfargument handling which resulted in an ICE (there's a test for it now) - cleaned up some unnecessary diagnostic function arguments
- some error messages were outdated (splat is supported on any argument) so I updated them
- use the argument span in some error messages, rather than using the argument number
- add tests for generics that might be tuples, and for unresolvable/ambiguous types
This comment has been minimized.
This comment has been minimized.
| }; | ||
|
|
||
| // Split the rust-call tupled arguments off. | ||
| // FIXME(splat): un-tuple splatted arguments in codegen, for performance |
There was a problem hiding this comment.
Why "for performance"? If this follows the RustCall approach, the ABI requires untupling for correctness. I am confused by this comment.
There was a problem hiding this comment.
Oh I see, I think I got confused by "splatted MIR lowering and tupling". This doesn't actually do anything on the ABI level yet, it passes things as structs.
Refactor FnDecl and FnSig non-type fields into a new wrapper type #### Why this Refactor? This PR is part of an initial cleanup for the [arg splat experiment](rust-lang/rust#153629), but it's a useful refactor by itself. It refactors the non-type fields of `FnDecl`, `FnSig`, and `FnHeader` into a new packed wrapper types, based on this comment in the `splat` experiment PR: rust-lang/rust#153697 (comment) It also refactors some common `FnSig` creation settings into their own methods. I did this instead of creating a struct with defaults. #### Relationship to `splat` Experiment I don't think we can use functional struct updates (`..default()`) to create `FnDecl` and `FnSig`, because we need the bit-packing for the `splat` experiment. Bit-packing will avoid breaking "type is small" assertions for commonly used types when `splat` is added. This PR packs these types: - ExternAbi: enum + `unwind` variants (38) -> 6 bits - ImplicitSelfKind: enum variants (5) -> 3 bits - lifetime_elision_allowed, safety, c_variadic: bool -> 1 bit #### Minor Changes Fixes some typos, and applies rustfmt to clippy files that got skipped somehow.
fixup! Add splat builtin attribute & feature gate
fixup! Add AST validation for #[splat]
fixup! Add HIR FnDecl for #[splat]
fixup! Impl HIR typeck for #[splat]
fixup! Impl TypeInfo for splat
84cab65 to
fdd4be9
Compare
|
The reflection data structures are tied exactly to the implementation cc @oli-obk |
There was a problem hiding this comment.
This should be ready for another review.
I've rebased this on the latest main, trying to minimise the number of rebase changes to earlier commits. The rebase was tricky, so there are some fixes which should be squashed into earlier commits, and some code might not quite be in the nicest/clearest commit.
The last 5 commits are revisions from the latest reviews, the detailed commit message says which commit they revise.
| /// Which function argument is splatted into multiple arguments in callers, if any? | ||
| /// Splatting functions with `u16::MAX` arguments is not supported, see `FnSigKind` for | ||
| /// details. | ||
| splatted: u16, |
There was a problem hiding this comment.
It might make the compiler faster to limit it to 256 arguments, I wonder if that would break any code.
At least we test for u16::MAX arguments. It used to panic, see #88577.
| /// Create a new FnDeclKind with no implicit self, no lifetime elision, and no C-style variadic argument. | ||
| /// Marker index for "no splatted argument". | ||
| /// Must have the same value as `FnSigKind::NO_SPLATTED_ARG_INDEX` and `rustc_ast::FnDecl::NO_SPLATTED_ARG_INDEX`. | ||
| const NO_SPLATTED_ARG_INDEX: u16 = u16::MAX; |
There was a problem hiding this comment.
I tried, but there's no common dependency crate to put the constant in.
Some of these instances will disappear in #155852 as part of a perf refactor (if it still gives good perf after recent nearby changes).
| splatted: Option<u16>, | ||
| args_len: usize, | ||
| ) -> Result<Self, SplattedArgIndexError> { | ||
| if let Some(splatted_arg_index) = splatted { |
There was a problem hiding this comment.
Yeah, I wasn't sure about this either.
I felt like having the option was clearer in callers, and tried to keep the sentinel value as an internal implementation detail which never appears outside the low-level bit-packing code. That way, providing u16::MAX as the splatted argument is easy to error on.
But I don't mind dropping the option, it's unlikely this case will be encountered in practice.
What do you think?
| let ocx_error = | ||
| ocx.eq(&origin, self.param_env, calee_tuple_type, new_tupled_type); | ||
| if let Err(ocx_error) = ocx_error { | ||
| struct_span_code_err!( |
There was a problem hiding this comment.
I'm not sure if it's reachable, and I don't know how to create a test for this. It seems like other errors happen first, but there might be a way to trigger it. I left a FIXME in the code.
I made a bunch of related fixes and tests while trying to test this:
- fixed some invalid method splatted
selfargument handling which resulted in an ICE (there's a test for it now) - cleaned up some unnecessary diagnostic function arguments
- some error messages were outdated (splat is supported on any argument) so I updated them
- use the argument span in some error messages, rather than using the argument number
- add tests for generics that might be tuples, and for unresolvable/ambiguous types
|
|
||
| // Multiple splatted arguments are invalid: we can't know which arguments go in each splat. | ||
| if splatted_arg_indexes.len() > 1 { | ||
| self.dcx().emit_err(errors::DuplicateSplattedArgs { spans: splatted_spans.clone() }); |
There was a problem hiding this comment.
I did my best to provide sensible defaults/substitutions on error.
Specifically, c_variadic gets priority, then the first splatted argument. Every other splat generates an error, then gets ignored in later stages.
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
View all comments
This PR is part of the argument splatting lang experiment, and FFI overloading / C++ interop project goals:
Example code using existing unstable features:
Discussion of implementation strategy:
The PR is the initial implementation of the feature:
splatincomplete feature gate#[splat]attribute on function arguments#[splat]function parameter check at THIR levelOnce this PR merges, we can add further functionality, then test it out in interop tools.
Further Work
Out of Scope for this PR