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

Fix ByMove coroutine-closure shim (for 2021 precise closure capturing behavior) #123518

Merged
merged 4 commits into from Apr 8, 2024

Conversation

compiler-errors
Copy link
Member

This PR reworks the way that we perform the ByMove coroutine-closure shim to account for the fact that the upvars of the outer coroutine-closure and the inner coroutine might not line up due to edition-2021 closure capture rules changes.

Specifically, the number of upvars may differ and/or the inner coroutine may have additional projections applied to an upvar. This PR reworks the information we pass into the ByMoveBody MIR visitor to account for both of these facts.

I tried to leave comments explaining exactly what everything is doing, but let me know if you have questions.

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. labels Apr 5, 2024
@rustbot
Copy link
Collaborator

rustbot commented Apr 5, 2024

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

async fn async_main() {
let x = &mut 0;
let y = &mut 0;
let c = async || {
Copy link
Member Author

Choose a reason for hiding this comment

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

Before this last commit, we accidentally matched the upvar for y against x because they both "match" (they both have no projections). Assertions should make sure we're not doing that, lol.

@rust-log-analyzer

This comment has been minimized.

let final_deref = if needs_deref {
let Some((mir::ProjectionElem::Deref, rest)) = projection.split_first() else {
bug!();
let [mir::ProjectionElem::Deref] = projection else {
Copy link
Member Author

Choose a reason for hiding this comment

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

Ok this assumption was way too strong. Fixed in a follow-up.

@oli-obk
Copy link
Contributor

oli-obk commented Apr 8, 2024

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Apr 8, 2024

📌 Commit ad0fcac 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 Apr 8, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 8, 2024
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#122781 (Fix argument ABI for overaligned structs on ppc64le)
 - rust-lang#123367 (Safe Transmute: Compute transmutability from `rustc_target::abi::Layout`)
 - rust-lang#123518 (Fix `ByMove` coroutine-closure shim (for 2021 precise closure capturing behavior))
 - rust-lang#123547 (bootstrap: remove unused pub fns)
 - rust-lang#123564 (Don't emit divide-by-zero panic paths in `StepBy::len`)
 - rust-lang#123578 (Restore `pred_known_to_hold_modulo_regions`)
 - rust-lang#123591 (Remove unnecessary cast from `LLVMRustGetInstrProfIncrementIntrinsic`)
 - rust-lang#123632 (parser: reduce visibility of unnecessary public `UnmatchedDelim`)
 - rust-lang#123635 (CFI: Fix ICE in KCFI non-associated function pointers)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 36d1915 into rust-lang:master Apr 8, 2024
11 checks passed
@rustbot rustbot added this to the 1.79.0 milestone Apr 8, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Apr 8, 2024
Rollup merge of rust-lang#123518 - compiler-errors:by-move-fixes, r=oli-obk

Fix `ByMove` coroutine-closure shim (for 2021 precise closure capturing behavior)

This PR reworks the way that we perform the `ByMove` coroutine-closure shim to account for the fact that the upvars of the outer coroutine-closure and the inner coroutine might not line up due to edition-2021 closure capture rules changes.

Specifically, the number of upvars may differ *and/or* the inner coroutine may have additional projections applied to an upvar. This PR reworks the information we pass into the `ByMoveBody` MIR visitor to account for both of these facts.

I tried to leave comments explaining exactly what everything is doing, but let me know if you have questions.

r? oli-obk
@compiler-errors compiler-errors deleted the by-move-fixes branch April 8, 2024 23:57
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants