Skip to content
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

Replace usage of ResumeTy in async lowering with Context #105250

Merged
merged 1 commit into from
Dec 6, 2022

Conversation

Swatinem
Copy link
Contributor

@Swatinem Swatinem commented Dec 4, 2022

Replaces using ResumeTy / get_context in favor of using &'static mut Context<'_>.

Usage of the 'static lifetime here is technically "cheating", and replaces the raw pointer in ResumeTy and the get_context fn that pulls the correct lifetimes out of thin air.

fixes #104828 and #104321 (comment)

r? @oli-obk

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Dec 4, 2022
@rustbot
Copy link
Collaborator

rustbot commented Dec 4, 2022

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

// Resume argument type, which should be `&mut Context<'_>`.
// NOTE: Using the `'static` lifetime here is technically cheating.
// The `Future::poll` argument really is `&'a mut Context<'b>`, but inferring that
// lifetime would lead to lifetime errors, as those lifetimes are considered captured.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since they are on the generator argument, why would they get captured?

ah this is #68923

Please reference that issue here, as we'll lose the connection now that ResumeTy is dead code

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏻 I slightly reworded that comment, referencing the given issue, and have also rebased the PR.

@oli-obk
Copy link
Contributor

oli-obk commented Dec 5, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Dec 5, 2022

📌 Commit 9dbb189 has been approved by oli-obk

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 5, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 5, 2022
Replace usage of `ResumeTy` in async lowering with `Context`

Replaces using `ResumeTy` / `get_context` in favor of using `&'static mut Context<'_>`.

Usage of the `'static` lifetime here is technically "cheating", and replaces the raw pointer in `ResumeTy` and the `get_context` fn that pulls the correct lifetimes out of thin air.

fixes rust-lang#104828 and rust-lang#104321 (comment)

r? `@oli-obk`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 5, 2022
Replace usage of `ResumeTy` in async lowering with `Context`

Replaces using `ResumeTy` / `get_context` in favor of using `&'static mut Context<'_>`.

Usage of the `'static` lifetime here is technically "cheating", and replaces the raw pointer in `ResumeTy` and the `get_context` fn that pulls the correct lifetimes out of thin air.

fixes rust-lang#104828 and rust-lang#104321 (comment)

r? ``@oli-obk``
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 5, 2022
Replace usage of `ResumeTy` in async lowering with `Context`

Replaces using `ResumeTy` / `get_context` in favor of using `&'static mut Context<'_>`.

Usage of the `'static` lifetime here is technically "cheating", and replaces the raw pointer in `ResumeTy` and the `get_context` fn that pulls the correct lifetimes out of thin air.

fixes rust-lang#104828 and rust-lang#104321 (comment)

r? ```@oli-obk```
@JohnTitor
Copy link
Member

Failed in rollup: #105335 (comment)
Seems this needs a rebase, @bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 6, 2022
Replaces using `ResumeTy` / `get_context` in favor of using `&'static mut Context<'_>`.

Usage of the `'static` lifetime here is technically "cheating", and replaces
the raw pointer in `ResumeTy` and the `get_context` fn that pulls the
correct lifetimes out of thin air.
@Swatinem
Copy link
Contributor Author

Swatinem commented Dec 6, 2022

Looks like this was conflicting with #105180.

I rebased, but I seem to have two mir-opt failures locally on Mac, though I don’t think they are related.

[mir-opt] src/test/mir-opt/inline/inline_generator.rs
[mir-opt] src/test/mir-opt/issues/issue_59352.rs

@oli-obk
Copy link
Contributor

oli-obk commented Dec 6, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Dec 6, 2022

📌 Commit cf031a3 has been approved by oli-obk

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 6, 2022
@compiler-errors
Copy link
Member

Heads up that this probably broke #105501.

@khuey is checking if reverting this PR makes the code compile again (that they can't share) -- This probably has to do with the same old generator-interior lifetime erasure issue, since now we have a type that has two lifetimes in the generator interior 😅.

Unclear which lifetime in &'static Context<'_> is the culprit here that's making the higher-ranked lifetime error -- we probably should be storing this as a *mut Context<'static> and transmuting it when needed :/

Anyways, at this rate, this will unfortunately probably make its way to beta, but we'll probably see regressions in the crater run after beta is cut.

@khuey
Copy link
Contributor

khuey commented Dec 9, 2022

Reverting this does indeed fix my problem.

@Swatinem
Copy link
Contributor Author

Swatinem commented Dec 9, 2022

@khuey can you do me a favor and try it with this snippet here:

https://github.com/rust-lang/rust/pull/104833/files#diff-b24a095026827d389d25a6913f9426e6cea3c0ef9b348a12a834d3a5919b8687R2720-R2743

That turns the &'static mut Context<'_> into &'static mut Context<'static>. If that doesn’t help, maybe using a *mut as @compiler-errors suggested may be worth another try.

@khuey
Copy link
Contributor

khuey commented Dec 10, 2022

Using that hunk doesn't change anything.

@Swatinem
Copy link
Contributor Author

Oh well :-(
Thanks for checking.

I think reverting back to ResumeTy it is then, as that is a basically a Send + Sync wrapper around *mut Context<'static>.

I hope adding a #[repr(transparent)] for it, and maybe with a few changes to cg_clif to make it see through #[repr(transparent)] types may help it along to not reject it.

In the meantime, it would be nice to have a minimal repro and maybe a regression test for #105501 so that does not regress again. Either way, I should be able to open a revert PR tomorrow evening or monday.

@bjorn3
Copy link
Member

bjorn3 commented Dec 10, 2022

I hope adding a #[repr(transparent)] for it, and maybe with a few changes to cg_clif to make it see through #[repr(transparent)] types may help it along to not reject it.

Please do a similar fix to #105082 instead and make sure that the generated MIR matches the function signature. Mismatches between the function signature and the MIR are bound to give issues apart from this type compatibility. For example without #105082 a projection on the return place went completely haywire causing an assignment of an i32 value to a () place which was correctly rejected with a crash.

@Fabian-Gruenbichler
Copy link
Contributor

this also causes a regression for the following (reduced from actual production code ;)) code that works fine on stable and beta:

use futures::future::FutureExt;

fn main() {
    let _ = async {
        async {
            async {}.await; // await inside a `.catch_unwind()` currently fails
        }
        .catch_unwind().await
    };
}
Compiling playground v0.0.1 (/playground)
error[[E0277]](https://doc.rust-lang.org/nightly/error-index.html#E0277): the type `&mut Context<'_>` may not be safely transferred across an unwind boundary
   --> src/main.rs:5:9
    |
5   | /         async {
6   | |             async {}.await; // await inside a `.catch_unwind()` currently fails
7   | |         }
    | |         ^
    | |         |
    | |_________`&mut Context<'_>` may not be safely transferred across an unwind boundary
    |           within this `[async block@src/main.rs:5:9: 7:10]`
    |
    = help: within `[async block@src/main.rs:5:9: 7:10]`, the trait `UnwindSafe` is not implemented for `&mut Context<'_>`
    = note: `UnwindSafe` is implemented for `&std::task::Context<'_>`, but not for `&mut std::task::Context<'_>`
note: future does not implement `UnwindSafe` as this value is used across an await
   --> src/main.rs:6:21
    |
5   | /         async {
6   | |             async {}.await; // await inside a `.catch_unwind()` currently fails
    | |                     ^^^^^^ await occurs here, with the value maybe used later
7   | |         }
    | |         -
    | |         |
    | |_________the value is later dropped here
    |           has type `&mut Context<'_>` which does not implement `UnwindSafe`
note: required by a bound in `futures::FutureExt::catch_unwind`
   --> /playground/.cargo/registry/src/github.com-1ecc6299db9ec823/futures-util-0.3.25/src/future/future/mod.rs:433:23
    |
433 |         Self: Sized + ::std::panic::UnwindSafe,
    |                       ^^^^^^^^^^^^^^^^^^^^^^^^ required by this bound in `FutureExt::catch_unwind`

For more information about this error, try `rustc --explain E0277`.
error: could not compile `playground` due to previous error

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=6da04ccbbdc8828f0872d970b0074a20

broken on current nightly as well as on a default build of 71ec145
works again on a default build of 71ec145 with cf031a3 from this PR reverted

thanks @Blub for reporting and reducing the reproducer!

@Swatinem
Copy link
Contributor Author

Further reduced:

fn is_unwindsafe(_: impl std::panic::UnwindSafe) {}

fn main() {
    is_unwindsafe(async {
        // this needs an inner await point
        async {}.await;
    });
}

Thanks for the repro. I will work on a fix later today.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 19, 2022
…rm-resumety, r=tmandry

Revert "Replace usage of `ResumeTy` in async lowering with `Context`"

Reverts rust-lang#105250
Fixes: rust-lang#105501

Following instructions from [forge](https://forge.rust-lang.org/compiler/reviews.html#reverts).

This change introduced a breaking change that is not actionable nor relevant, and is blocking updates to our toolchain. Along with other comments on the CL marking issues that are fixed by reverts, reverting is best until these issues can be resolved

cc. `@Swatinem`
Swatinem added a commit to Swatinem/rust that referenced this pull request Dec 21, 2022
This was a regression from the reverted rust-lang#105250 which is now covered by a test.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 22, 2022
…trochenkov

Test that async blocks are `UnwindSafe`

This was a regression from the reverted rust-lang#105250 which is now covered by a test.
MaciejWas pushed a commit to MaciejWas/rust that referenced this pull request Dec 27, 2022
This was a regression from the reverted rust-lang#105250 which is now covered by a test.
Swatinem added a commit to Swatinem/rust that referenced this pull request Feb 1, 2023
This changes from the stdlib supported `ResumeTy`, and converting that to a `&mut Context<'_>` to directly use `&mut Context<'_>` in lowering.

It pretty much reverts rust-lang#105977 and re-applies an updated version of rust-lang#105250.

The PR is currently failing as it depends on `-Zdrop-tracking-mir` becoming the default, so that the compiler does not falsly believe the context is being held across await points,which would make async blocks `!Send` and `!UnwindSafe`.
However it also still fails the testcase added in rust-lang#106264 for reasons I don’t understand.
thomcc pushed a commit to tcdi/postgrestd that referenced this pull request May 31, 2023
…ty, r=tmandry

Revert "Replace usage of `ResumeTy` in async lowering with `Context`"

Reverts rust-lang/rust#105250
Fixes: #105501

Following instructions from [forge](https://forge.rust-lang.org/compiler/reviews.html#reverts).

This change introduced a breaking change that is not actionable nor relevant, and is blocking updates to our toolchain. Along with other comments on the CL marking issues that are fixed by reverts, reverting is best until these issues can be resolved

cc. `@Swatinem`
Swatinem added a commit to Swatinem/rust that referenced this pull request Sep 24, 2023
Instead of using the stdlib supported `ResumeTy`, which is being converting to a `&mut Context<'_>` during the Generator MIR pass,
this will use `&mut Context<'_>` directly in HIR lowering.

It pretty much reverts rust-lang#105977 and re-applies an updated version of rust-lang#105250.

This still fails the testcase added in rust-lang#106264 however, for reasons I don’t understand.
Swatinem added a commit to Swatinem/rust that referenced this pull request Dec 28, 2023
Instead of using the stdlib supported `ResumeTy`, which is being converting to a `&mut Context<'_>` during the Generator MIR pass,
this will use `&mut Context<'_>` directly in HIR lowering.

It pretty much reverts rust-lang#105977 and re-applies an updated version of rust-lang#105250.

This still fails the testcase added in rust-lang#106264 however, for reasons I don’t understand.
Swatinem added a commit to Swatinem/rust that referenced this pull request Dec 28, 2023
Instead of using the stdlib supported `ResumeTy`, which is being converting to a `&mut Context<'_>` during the Generator MIR pass,
this will use `&mut Context<'_>` directly in HIR lowering.

It pretty much reverts rust-lang#105977 and re-applies an updated version of rust-lang#105250.

This still fails the testcase added in rust-lang#106264 however, for reasons I don’t understand.
Swatinem added a commit to Swatinem/rust that referenced this pull request Jan 6, 2024
Instead of using the stdlib supported `ResumeTy`, which is being converting to a `&mut Context<'_>` during the Generator MIR pass,
this will use `&mut Context<'_>` directly in HIR lowering.

It pretty much reverts rust-lang#105977 and re-applies an updated version of rust-lang#105250.

This still fails the testcase added in rust-lang#106264 however, for reasons I don’t understand.
Swatinem added a commit to Swatinem/rust that referenced this pull request Feb 17, 2024
Instead of using the stdlib supported `ResumeTy`, which is being converting to a `&mut Context<'_>` during the Generator MIR pass,
this will use `&mut Context<'_>` directly in HIR lowering.

It pretty much reverts rust-lang#105977 and re-applies an updated version of rust-lang#105250.

This still fails the testcase added in rust-lang#106264 however, for reasons I don’t understand.
Swatinem added a commit to Swatinem/rust that referenced this pull request May 12, 2024
Instead of using the stdlib supported `ResumeTy`, which is being converting to a `&mut Context<'_>` during the Generator MIR pass,
this will use `&mut Context<'_>` directly in HIR lowering.

It pretty much reverts rust-lang#105977 and re-applies an updated version of rust-lang#105250.

This still fails the testcase added in rust-lang#106264 however, for reasons I don’t understand.
Swatinem added a commit to Swatinem/rust that referenced this pull request Jul 30, 2024
Instead of using the stdlib supported `ResumeTy`, which is being converting to a `&mut Context<'_>` during the Generator MIR pass,
this will use `&mut Context<'_>` directly in HIR lowering.

It pretty much reverts rust-lang#105977 and re-applies an updated version of rust-lang#105250.

This still fails the testcase added in rust-lang#106264 however, for reasons I don’t understand.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove need for ResumeTy / get_context in async generators
9 participants