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
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 20 additions & 6 deletions compiler/rustc_mir_transform/src/coroutine/by_move_body.rs
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@

use rustc_data_structures::unord::UnordMap;
use rustc_hir as hir;
use rustc_middle::hir::place::{Projection, ProjectionKind};
use rustc_middle::hir::place::{PlaceBase, Projection, ProjectionKind};
use rustc_middle::mir::visit::MutVisitor;
use rustc_middle::mir::{self, dump_mir, MirPass};
use rustc_middle::ty::{self, InstanceDef, Ty, TyCtxt, TypeVisitableExt};
Expand Down Expand Up @@ -149,17 +149,25 @@
bug!("we ran out of parent captures!")
};

let PlaceBase::Upvar(parent_base) = parent_capture.place.base else {
bug!("expected capture to be an upvar");
};
let PlaceBase::Upvar(child_base) = child_capture.place.base else {
bug!("expected capture to be an upvar");
};

assert!(
child_capture.place.projections.len() >= parent_capture.place.projections.len()
);
// A parent matches a child they share the same prefix of projections.
// The child may have more, if it is capturing sub-fields out of
// something that is captured by-move in the parent closure.
if !std::iter::zip(
&child_capture.place.projections,
&parent_capture.place.projections,
)
.all(|(child, parent)| child.kind == parent.kind)
if parent_base.var_path.hir_id != child_base.var_path.hir_id
compiler-errors marked this conversation as resolved.
Show resolved Hide resolved
|| !std::iter::zip(
&child_capture.place.projections,
&parent_capture.place.projections,
)
.all(|(child, parent)| child.kind == parent.kind)
{
// Make sure the field was used at least once.
assert!(
Expand Down Expand Up @@ -217,6 +225,12 @@
}
}

// Pop the last parent capture
if field_used_at_least_once {
let _ = parent_captures.next().unwrap();
}
assert_eq!(parent_captures.next(), None, "leftover parent captures?");

if coroutine_kind == ty::ClosureKind::FnOnce {
assert_eq!(field_remapping.len(), tcx.closure_captures(parent_def_id).len());
return;
Expand Down Expand Up @@ -276,7 +290,7 @@
// since a layer of reffing has now become redundant.
let final_deref = if needs_deref {
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.

bug!("There should only be a single deref for an upvar local initialization");

Check failure on line 293 in compiler/rustc_mir_transform/src/coroutine/by_move_body.rs

View workflow job for this annotation

GitHub Actions / PR - x86_64-gnu-llvm-17

There should only be a single deref for an upvar local initialization

Check failure on line 293 in compiler/rustc_mir_transform/src/coroutine/by_move_body.rs

View workflow job for this annotation

GitHub Actions / PR - x86_64-gnu-llvm-17

There should only be a single deref for an upvar local initialization
};
&[]
} else {
Expand Down
27 changes: 27 additions & 0 deletions tests/ui/async-await/async-closures/overlapping-projs.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
//@ aux-build:block-on.rs
//@ edition:2021
//@ run-pass
//@ check-run-results

#![feature(async_closure)]

extern crate block_on;

async fn call_once(f: impl async FnOnce()) {
f().await;
}

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.

*x = 1;
*y = 2;
};
call_once(c).await;
println!("{x} {y}");
}

fn main() {
block_on::block_on(async_main());
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
1 2
Loading