feature: Add basic support for become expr/tail calls#15003
feature: Add basic support for become expr/tail calls#15003bors merged 1 commit intorust-lang:masterfrom
become expr/tail calls#15003Conversation
| self.set_terminator(current, TerminatorKind::Return, expr_id.into()); | ||
| Ok(None) | ||
| } | ||
| Expr::Become { .. } => not_supported!("tail-calls"), |
There was a problem hiding this comment.
MIR support is actually not hard (I already have a MIR impl in the compiler after all), but I'm not sure how to properly lower it from hir. become x is only valid if x is a call (f(x), y.m()) and MIR really would like to see this as simple as possible.
In the compiler I lower HIR->THIR as-is, then run a check on THIR that tail calls are well-formed and then do something akin to this:
let Expr::Call { ..... } = expr
else { bug!("THIR checks should have noticed that tail call doesn't have a call") };Not sure what is the best way to do this in r-a
There was a problem hiding this comment.
What is the problem of doing the same in r-a? Just instead of bug! use not_supported! or add a variant to MirLowerError.
There was a problem hiding this comment.
The problem is that THIR has much simpler representation of functions and I'm not sure how to correctly handle both function and method calls.
There was a problem hiding this comment.
Aha. How it will be a lowered into mir? It would be a new terminator, like BecomeCall?
There was a problem hiding this comment.
Yes, it will be a new TailCall terminator. There is an experimental branch, it looks like this:
TailCall {
/// The function that’s being called.
func: Operand<'tcx>,
/// Arguments the function is called with.
/// These are owned by the callee, which is free to modify them.
/// This allows the memory occupied by "by-value" arguments to be
/// reused across function calls without duplicating the contents.
args: Vec<Operand<'tcx>>,
// FIXME(explicit_tail_calls): should we have the span for `become`? is this span accurate? do we need it?
/// This `Span` is the span of the function, without the dot and receiver
/// (e.g. `foo(a, b)` in `x.foo(a, b)`
fn_span: Span,
},There was a problem hiding this comment.
So I see three ways to implement it:
- Handle all cases manually. Assuming that
becomedoesn't support overloaded operators, it is just call and method call. - Adding a field to
InferenceResultfor recording the simple form of function calls (or changing the currentmethod_resolutionfield). This is similar to THIR in rustc. - Adding a flag to
MirLowerCtx, which if true it means the nextTerminator::Callshould beTerminator::TailCall.
If you want to go with 2, I would suggest doing it in a separate PR, to minimize git conflicts (Assuming this PR is going to be not merged in a long time)
There was a problem hiding this comment.
I think 3 is error prone with code like
become 1; // invalid
f(); // becomes tail call2 I'm not exactly sure I understand.
Assuming that become doesn't support overloaded operators
Yes, this is true.
There was a problem hiding this comment.
I think 1 would be easiest to go with for now even if it's not the cleanest. 2 seems kind of invasive with the current structure I think
There was a problem hiding this comment.
This is also my understanding, yes. I'll try option 1.
54af603 to
6d0a24c
Compare
|
☔ The latest upstream changes (presumably #15557) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Status update: MIR support is actually harder than I anticipated 😅 This is currently blocked on rust-lang/rust#113128 (i.e. the MIR implementation for rustc). |
|
Can we just merge this without the MIR support for now? Cleaning up the PR backlog and I don't see a reason to block it on that and the current parts work well without it. Do you mind rebasing this PR @WaffleLapkin if you have the time? Otherwise I can also do that if you want |
|
I'm not sure if this is useful, given that upstream still doesn't support tail calls properly. But I don't see any problems with merging this as is, so why not. I'll rebase today/this week. |
|
It's not necessarily useful right now, but the code is so small that it won't be a problem should design around it change in the future (or it being scrapped fully). |
6d0a24c to
e146139
Compare
become expr/tail callsbecome expr/tail calls
|
Yeah, I agree :) (rebased) |
|
Thanks! |
|
☀️ Test successful - checks-actions |
This follows rust-lang/rfcs#3407 and my WIP implementation in the compiler.
Notice that I haven't even opened a compiler PR (although I plan to soon), so this feature doesn't really exist outside of my WIP branches. I've used this to help me test my implementation; opening a PR before I forget.
(feel free to ignore this for now, given all of the above)