-
Notifications
You must be signed in to change notification settings - Fork 13.6k
Ignore coroutine witness type region args in auto trait confirmation #145194
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
what a mess, the reason this works is even more subtle. While adding member constraints does ignore witnesses, any lifetime that occurs both in the witness and somewhere else also causes lifetimes in the witness to get mapped back to that lifetime, see https://rust-lang.zulipchat.com/#narrow/channel/144729-t-types/topic/nested.20bodies.20in.20opaque.20types/near/523055535 where I explain how this manifests for closures. However, we never actually check that the witness args are equal to the coroutine args, so in MIR borrowck, coroutines are all Manually erasing the regions in the trait solver sgtm I think I would just backport this PR @bors r+ |
Rollup of 5 pull requests Successful merges: - #135331 (Reject relaxed bounds inside associated type bounds (ATB)) - #144156 (Check coroutine upvars in dtorck constraint) - #145091 (`NllRegionVariableOrigin` remove `from_forall`) - #145194 (Ignore coroutine witness type region args in auto trait confirmation) - #145225 (Fix macro infinite recursion test to not trigger warning about semicolon in expr) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of #145194 - compiler-errors:coro-witness-re, r=lcnr Ignore coroutine witness type region args in auto trait confirmation ## The problem Consider code like: ``` async fn process<'a>() { Box::pin(process()).await; } fn require_send(_: impl Send) {} fn main() { require_send(process()); } ``` When proving that the coroutine `{coroutine@process}::<'?0>: Send`, we end up instantiating a nested goal `{witness@process}::<'?0>: Send` by synthesizing a witness type from the coroutine's args: Proving a coroutine witness type implements an auto trait requires looking up the coroutine's witness types. The witness types are a binder that look like `for<'r> { Pin<Box<{coroutine@process}::<'r>>> }`. We instantiate this binder with placeholders and prove `Send` on the witness types. This ends up eventually needing to prove something like `{coroutine@process}::<'!1>: Send`. Repeat this process, and we end up in an overflow during fulfillment, since fulfillment does not use freshening. This can be visualized with a trait stack that ends up looking like: * `{coroutine@process}::<'?0>: Send` * `{witness@process}::<'?0>: Send` * `Pin<Box<{coroutine@process}::<'!1>>>: Send` * `{coroutine@process}::<'!1>: Send` * ... * `{coroutine@process}::<'!2>: Send` * `{witness@process}::<'!2>: Send` * ... * overflow! The problem here specifically comes from the first step: synthesizing a witness type from the coroutine's args. ## Why wasn't this an issue before? Specifically, before 63f6845, this wasn't an issue because we were instead extracting the witness from the coroutine type itself. It turns out that given some `{coroutine@process}::<'?0>`, the witness type was actually something like `{witness@process}::<'erased>`! So why do we end up with a witness type with `'erased` in its args? This is due to the fact that opaque type inference erases all regions from the witness. This is actually explicitly part of opaque type inference -- changing this to actually visit the witness types actually replicates this overflow even with 63f6845 reverted: https://github.com/rust-lang/rust/blob/ca77504943887037504c7fc0b9bf06dab3910373/compiler/rustc_borrowck/src/type_check/opaque_types.rs#L303-L313 To better understand this difference and how it avoids a cycle, if you look at the trait stack before 63f6845, we end up with something like: * `{coroutine@process}::<'?0>: Send` * `{witness@process}::<'erased>: Send` **<-- THIS CHANGED** * `Pin<Box<{coroutine@process}::<'!1>>>: Send` * `{coroutine@process}::<'!1>: Send` * ... * `{coroutine@process}::<'erased>: Send` **<-- THIS CHANGED** * `{witness@process}::<'erased>: Send` **<-- THIS CHANGED** * coinductive cycle! 🎉 ## So what's the fix? This hack replicates the behavior in opaque type inference to erase regions from the witness type, but instead erasing the regions during auto trait confirmation. This is kinda a hack, but is sound. It does not need to be replicated in the new trait solver, of course. --- I hope this explanation makes sense. We could beta backport this instead of the revert #145193, but then I'd like to un-revert that on master in this PR along with landing this this hack. Thoughts? r? lcnr
def_id, | ||
self.tcx().mk_args(args.as_coroutine().parent_args()), | ||
ty::GenericArgs::for_item(tcx, def_id, |def, _| match def.kind { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need generic args for typeck_root, not the coroutine here
gonna fix soon
The problem
Consider code like:
When proving that the coroutine
{coroutine@process}::<'?0>: Send
, we end up instantiating a nested goal{witness@process}::<'?0>: Send
by synthesizing a witness type from the coroutine's args:Proving a coroutine witness type implements an auto trait requires looking up the coroutine's witness types. The witness types are a binder that look like
for<'r> { Pin<Box<{coroutine@process}::<'r>>> }
. We instantiate this binder with placeholders and proveSend
on the witness types. This ends up eventually needing to prove something like{coroutine@process}::<'!1>: Send
. Repeat this process, and we end up in an overflow during fulfillment, since fulfillment does not use freshening.This can be visualized with a trait stack that ends up looking like:
{coroutine@process}::<'?0>: Send
{witness@process}::<'?0>: Send
Pin<Box<{coroutine@process}::<'!1>>>: Send
{coroutine@process}::<'!1>: Send
{coroutine@process}::<'!2>: Send
{witness@process}::<'!2>: Send
The problem here specifically comes from the first step: synthesizing a witness type from the coroutine's args.
Why wasn't this an issue before?
Specifically, before 63f6845, this wasn't an issue because we were instead extracting the witness from the coroutine type itself. It turns out that given some
{coroutine@process}::<'?0>
, the witness type was actually something like{witness@process}::<'erased>
!So why do we end up with a witness type with
'erased
in its args? This is due to the fact that opaque type inference erases all regions from the witness. This is actually explicitly part of opaque type inference -- changing this to actually visit the witness types actually replicates this overflow even with 63f6845 reverted:rust/compiler/rustc_borrowck/src/type_check/opaque_types.rs
Lines 303 to 313 in ca77504
To better understand this difference and how it avoids a cycle, if you look at the trait stack before 63f6845, we end up with something like:
{coroutine@process}::<'?0>: Send
{witness@process}::<'erased>: Send
<-- THIS CHANGEDPin<Box<{coroutine@process}::<'!1>>>: Send
{coroutine@process}::<'!1>: Send
{coroutine@process}::<'erased>: Send
<-- THIS CHANGEDSo what's the fix?
This hack replicates the behavior in opaque type inference to erase regions from the witness type, but instead erasing the regions during auto trait confirmation. This is kinda a hack, but is sound. It does not need to be replicated in the new trait solver, of course.
I hope this explanation makes sense.
We could beta backport this instead of the revert #145193, but then I'd like to un-revert that on master in this PR along with landing this this hack. Thoughts?
r? lcnr