Fix splat ICEs and ban it in closures#158645
Conversation
|
r? @folkertdev rustbot has assigned @folkertdev. Use Why was this reviewer chosen?The reviewer was selected based on:
|
| Extern::Explicit(abi_str, span) | ||
| if abi_str.symbol_unescaped.as_str() == "rust-call" => | ||
| { | ||
| SplatSemantic::NoRustCall(span) | ||
| } | ||
| Extern::Explicit(_abi_str, _span) => SplatSemantic::Yes, |
There was a problem hiding this comment.
| Extern::Explicit(abi_str, span) | |
| if abi_str.symbol_unescaped.as_str() == "rust-call" => | |
| { | |
| SplatSemantic::NoRustCall(span) | |
| } | |
| Extern::Explicit(_abi_str, _span) => SplatSemantic::Yes, | |
| Extern::Explicit(abi_str, span) => match abi_str.symbol_unescaped { | |
| sym::rust_dash_call => SplatSemantic::NoRustCall(span), | |
| _ => SplatSemantic::Yes, | |
| }, |
There was a problem hiding this comment.
and add a comment on why here too perhaps?
There was a problem hiding this comment.
I think the rust_dash_call symbol only exists in rust-analyzer, not in the compiler, but I did add the comment.
|
Reminder, once the PR becomes ready for a review, use |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
A first pass. Overall this PR seems kind of large, it would be easier to review if split up into smaller chunks.
Yeah I wondered about that, I'll split it up today, now I know which parts actually fail in CI.
@rustbot label +F-splat +I-ICE +T-compiler +C-bug
|
I split the extra tests into PR #158677, most of them didn't need any compiler changes to work, so they are fine to merge separately. |
|
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. |
|
Error: Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #triagebot on Zulip. |
|
@rustbot ready |
…eywiser Add extra splat tests This PR is part of the splat lang experiment rust-lang#153629. It adds a bunch of tests for existing splat functionality, but doesn't make any compiler code changes. Edit: split off from PR rust-lang#158645 @rustbot label +F-splat +T-compiler
| TyKind::FnPtr(bfty) => { | ||
| self.check_fn_ptr_safety(bfty.decl_span, bfty.safety); | ||
| self.check_fn_decl(&bfty.decl, SelfSemantic::No); | ||
| self.check_fn_decl(&bfty.decl, SelfSemantic::No, SplatSemantic::Yes); |
There was a problem hiding this comment.
This is wrong because FnPtrs can be rust-call, I'll add a test
https://play.rust-lang.org/?version=nightly&mode=debug&edition=2024&gist=511e43db9905a3a38acb86b175535fe2
@rustbot author
|
Ok, now splat + rust-call is banned on both function item declarations and function pointer types, and splat is banned on closures. And there are tests for all of them. @rustbot ready |
| // Whether the function is variadic (e.g. from C) | ||
| c_variadic: bool, |
There was a problem hiding this comment.
why was this removed exactly?
There was a problem hiding this comment.
It's not used in this function except for debug logging, and it's not particularly interesting in debug logging because splat and c_variadic aren't allowed on the same function at the AST level.
And the top of the function early exits if splat / Fn* traits / rust-call isn't being used, so c_variadic should always be false.
| ) | ||
| } | ||
| } else { | ||
| let fn_ty = self.typeck_results.expr_ty_adjusted(expr); |
There was a problem hiding this comment.
maybe comment here that this case is for function pointers?
There was a problem hiding this comment.
Perhaps a custom enum even makes sense? "None" meaning we're dealing with function pointers is a but subtle. On the other hand, the ergonomics of that might be worse.
There was a problem hiding this comment.
Happy to add a comment.
I expect to populate the side-table in an upcoming PR, as part of that change I might need a custom enum with different fields for FnPtrs and FnDefs. I was holding off on that to avoid churn.
| error: `#[splat]` is not allowed on functions with the `rust-call` ABI | ||
| --> $DIR/reject-closure-issue-158605.rs:10:5 | ||
| | | ||
| LL | extern "rust-call" fn method(#[splat] self: Self); | ||
| | ^^^^^^^^^^^^^^^^^^ ^^^^ | ||
| | | ||
| = help: remove `#[splat]` or change the ABI |
There was a problem hiding this comment.
is the span right here? I'd expect it on the #[splat] really?
also the #[splat] is not really "on" the function, it is on an argument. Not a native speaker, but "is not allowed in/for functions with ..." sounds more natural?
There was a problem hiding this comment.
is the span right here? I'd expect it on the #[splat] really?
Ideally on splat and the ABI, as one of them needs to be removed. I'll add a FIXME if that's not possible right now.
I think "splat is not allowed in the arguments of functions with the rust-call ABI" would be clearer, as "in" could mean the body as well.
| // FIXME(splat): should splatting extern "C" or other ABIs be allowed? | ||
| Extern::Implicit(_) => SplatSemantic::Yes, | ||
| // For now, splatting rust-call is banned, because it already de-tuples args. | ||
| Extern::Explicit(abi_str, span) if abi_str.symbol_unescaped.as_str() == "rust-call" => { |
There was a problem hiding this comment.
it would be fine to add a rust_dash_call to rustc too.
…eywiser Add extra splat tests This PR is part of the splat lang experiment rust-lang#153629. It adds a bunch of tests for existing splat functionality, but doesn't make any compiler code changes. Edit: split off from PR rust-lang#158645 @rustbot label +F-splat +T-compiler
Rollup merge of #158677 - teor2345:extra-splat-tests, r=wesleywiser Add extra splat tests This PR is part of the splat lang experiment #153629. It adds a bunch of tests for existing splat functionality, but doesn't make any compiler code changes. Edit: split off from PR #158645 @rustbot label +F-splat +T-compiler
| // Whether the function is variadic (e.g. from C) | ||
| c_variadic: bool, |
There was a problem hiding this comment.
It's not used in this function except for debug logging, and it's not particularly interesting in debug logging because splat and c_variadic aren't allowed on the same function at the AST level.
And the top of the function early exits if splat / Fn* traits / rust-call isn't being used, so c_variadic should always be false.
| ) | ||
| } | ||
| } else { | ||
| let fn_ty = self.typeck_results.expr_ty_adjusted(expr); |
There was a problem hiding this comment.
Happy to add a comment.
I expect to populate the side-table in an upcoming PR, as part of that change I might need a custom enum with different fields for FnPtrs and FnDefs. I was holding off on that to avoid churn.
| error: `#[splat]` is not allowed on functions with the `rust-call` ABI | ||
| --> $DIR/reject-closure-issue-158605.rs:10:5 | ||
| | | ||
| LL | extern "rust-call" fn method(#[splat] self: Self); | ||
| | ^^^^^^^^^^^^^^^^^^ ^^^^ | ||
| | | ||
| = help: remove `#[splat]` or change the ABI |
There was a problem hiding this comment.
is the span right here? I'd expect it on the #[splat] really?
Ideally on splat and the ABI, as one of them needs to be removed. I'll add a FIXME if that's not possible right now.
I think "splat is not allowed in the arguments of functions with the rust-call ABI" would be clearer, as "in" could mean the body as well.
|
Alright, all of those should be fixed up now. This ended up a bit more complicated than I thought, because we were missing a test (and AST handling) for multiple splats on the same argument. |
|
@rustbot ready |
View all comments
This PR is part of the splat lang experiment #153629.
It fixes one ICE, and cleans up the surrounding error handling to avoid future ICEs:
Close #158482
Improves the message and error handling in another ICE (but doesn't fix it):
cc #158603
And bans splat on closures and rust-call, because they already de-tuple arguments:
Close #158605
In the process I discovered another symbol mangling bug with splatting, which I added a test for (but didn't fix yet):
cc #158644
This test might be flaky, I'll drop it from this PR if it fails in CI.