feature: Add basic support for become expr/tail calls

[WaffleLapkin]

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)

[WaffleLapkin]

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

[HKalbasi]

What is the problem of doing the same in r-a? Just instead of bug! use not_supported! or add a variant to MirLowerError.

[WaffleLapkin]

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.

[HKalbasi]

Aha. How it will be a lowered into mir? It would be a new terminator, like BecomeCall?

[WaffleLapkin]

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,
    },

[HKalbasi]

So I see three ways to implement it:

1. Handle all cases manually. Assuming that become doesn't support overloaded operators, it is just call and method call.
2. Adding a field to InferenceResult for recording the simple form of function calls (or changing the current method_resolution field). This is similar to THIR in rustc.
3. Adding a flag to MirLowerCtx, which if true it means the next Terminator::Call should be Terminator::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)

[WaffleLapkin]

I think 3 is error prone with code like

become 1; // invalid
f(); // becomes tail call

---

2 I'm not exactly sure I understand.

---

Assuming that become doesn't support overloaded operators

Yes, this is true.

[Veykril]

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

[WaffleLapkin]

This is also my understanding, yes. I'll try option 1.

[bors]

☔ The latest upstream changes (presumably #15557) made this pull request unmergeable. Please resolve the merge conflicts.

[WaffleLapkin]

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).

[Veykril]

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

[WaffleLapkin]

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.

[Veykril]

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).

[WaffleLapkin]

Yeah, I agree :)

(rebased)

[Veykril]

Thanks!
@bors r+

[bors]

📌 Commit e146139 has been approved by Veykril

It is now in the queue for this repository.

[bors]

⌛ Testing commit e146139 with merge dba5997...

[bors]

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing dba5997 to master...