implement checks for tail calls #133607
implement checks for tail calls #133607
Conversation
this implements checks necessary to guarantee that we can actually perform a tail call. while extremely restrictive, this is what is documented in the RFC, and all these checks are needed for one reason or another.
WaffleLapkin
added
F-explicit_tail_calls
rustbot
added
S-waiting-on-review
| tcx, | ||
| thir, | ||
| found_errors: Ok(()), | ||
| // FIXME(#132279): we're clearly in a body here. |
There was a problem hiding this comment.
yes but it would define opaque types :3
but maybe we want to use @lcnr's post borrowck analysis
There was a problem hiding this comment.
I'm not sure what options get us there, but I think this code should be accepted:
fn a() -> impl Sized {
become b()
}
fn b() -> u8 { 0 }which means that we should reveal opaques before comparing things 🤔
There was a problem hiding this comment.
well, so we probably want to reveal only the opaques local to the body
There was a problem hiding this comment.
i think this is good for now, and having a fixme maybe for deciding on behavior is good enough!
|
@compiler-errors thank you for your suggestions, they made things quite a bit better :3 |
|
r? @compiler-errors as you've already partially reviewed this |
|
@bors r+ |
bors
added
S-waiting-on-bors
…ompiler-errors implement checks for tail calls Quoting the [RFC draft](https://github.com/phi-go/rfcs/blob/guaranteed-tco/text/0000-explicit-tail-calls.md): > The argument to become is a function (or method) call, that exactly matches the function signature and calling convention of the callee. The intent is to ensure a matching ABI. Note that lifetimes may differ as long as they pass borrow checking, see [below](https://github.com/phi-go/rfcs/blob/guaranteed-tco/text/0000-explicit-tail-calls.md#return-type-coercion) for specifics on the return type. > Tail calling closures and tail calling from closures is not allowed. This is due to the high implementation effort, see below, this restriction can be lifted by a future RFC. > Invocations of operators were considered as valid targets but were rejected on grounds of being too error-prone. In any case, these can still be called as methods. > Tail calling [variadic functions](https://doc.rust-lang.org/beta/unstable-book/language-features/c-variadic.html) and tail calling from variadic functions is not allowed. As support for variadic function is stabilized on a per target level, support for tail-calls regarding variadic functions would need to follow a similar approach. To avoid this complexity and to minimize implementation effort for backends, this interaction is currently not allowed but support can be added with a future RFC. ----- The checks are implemented as a query, similarly to `check_unsafety`. The code is cherry-picked straight out of rust-lang#112657 which was written more than a year ago, so I expect we might need to change some things ^^"
…llaumeGomez Rollup of 8 pull requests Successful merges: - rust-lang#132155 (Always display first line of impl blocks even when collapsed) - rust-lang#133256 (CI: use free runners for i686-gnu jobs) - rust-lang#133607 (implement checks for tail calls) - rust-lang#133821 (Replace black with ruff in `tidy`) - rust-lang#133827 (CI: rfl: move job forward to Linux v6.13-rc1) - rust-lang#133910 (Normalize target-cpus.rs stdout test for LLVM changes) - rust-lang#133921 (Adapt codegen tests for NUW inference) - rust-lang#133936 (Avoid fetching the anon const hir node that is already available) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#133607 - WaffleLapkin:tail-call-checks, r=compiler-errors implement checks for tail calls Quoting the [RFC draft](https://github.com/phi-go/rfcs/blob/guaranteed-tco/text/0000-explicit-tail-calls.md): > The argument to become is a function (or method) call, that exactly matches the function signature and calling convention of the callee. The intent is to ensure a matching ABI. Note that lifetimes may differ as long as they pass borrow checking, see [below](https://github.com/phi-go/rfcs/blob/guaranteed-tco/text/0000-explicit-tail-calls.md#return-type-coercion) for specifics on the return type. > Tail calling closures and tail calling from closures is not allowed. This is due to the high implementation effort, see below, this restriction can be lifted by a future RFC. > Invocations of operators were considered as valid targets but were rejected on grounds of being too error-prone. In any case, these can still be called as methods. > Tail calling [variadic functions](https://doc.rust-lang.org/beta/unstable-book/language-features/c-variadic.html) and tail calling from variadic functions is not allowed. As support for variadic function is stabilized on a per target level, support for tail-calls regarding variadic functions would need to follow a similar approach. To avoid this complexity and to minimize implementation effort for backends, this interaction is currently not allowed but support can be added with a future RFC. ----- The checks are implemented as a query, similarly to `check_unsafety`. The code is cherry-picked straight out of rust-lang#112657 which was written more than a year ago, so I expect we might need to change some things ^^"
|
This introduced lots of perf regressions, as reported in #133940. @WaffleLapkin: Is that expected? Can anything be done to mitigate them? |
|
@nnethercote this is not entirely expected, but I potentially see the problem. the added check loops over all THIR nodes, so even if the check doesn't do anything, it may add up into something measurable? One solution to this I could see is to merge Upd: actually, one other idea is that the issue is query rehashing the input/storing output to disk. This can easily be fixed by merging queries between |
|
@WaffleLapkin: a couple of good sounding ideas there, are you happy to try them out? |
| /// Checks well-formedness of tail calls (`become f()`). | ||
| query check_tail_calls(key: LocalDefId) -> Result<(), rustc_errors::ErrorGuaranteed> { | ||
| desc { |tcx| "tail-call-checking `{}`", tcx.def_path_str(key) } | ||
| cache_on_disk_if { true } |
There was a problem hiding this comment.
probably dont need this caching
There was a problem hiding this comment.
or at least possibly seeing if this is perf sensitive
There was a problem hiding this comment.
This was copied from check_unsafety, we might want to change it too 🤔
|
@nnethercote yes, I'll try them out. |
Quoting the RFC draft:
The checks are implemented as a query, similarly to
check_unsafety.The code is cherry-picked straight out of #112657 which was written more than a year ago, so I expect we might need to change some things ^^"