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

Stop making any assumption about the projections applied to the upvars in the ByMoveBody pass #123658

Merged
merged 1 commit into from
Apr 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
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
36 changes: 17 additions & 19 deletions compiler/rustc_mir_transform/src/coroutine/by_move_body.rs
Original file line number Diff line number Diff line change
Expand Up @@ -281,14 +281,14 @@ impl<'tcx> MutVisitor<'tcx> for MakeByMoveBody<'tcx> {
if place.local == ty::CAPTURE_STRUCT_LOCAL
&& let Some((&mir::ProjectionElem::Field(idx, _), projection)) =
place.projection.split_first()
&& let Some(&(remapped_idx, remapped_ty, needs_deref, additional_projections)) =
&& let Some(&(remapped_idx, remapped_ty, needs_deref, bridging_projections)) =
self.field_remapping.get(&idx)
{
// As noted before, if the parent closure captures a field by value, and
// the child captures a field by ref, then for the by-move body we're
// generating, we also are taking that field by value. Peel off a deref,
// since a layer of reffing has now become redundant.
let final_deref = if needs_deref {
// since a layer of ref'ing has now become redundant.
let final_projections = if needs_deref {
let Some((mir::ProjectionElem::Deref, projection)) = projection.split_first()
else {
bug!(
Expand All @@ -302,20 +302,18 @@ impl<'tcx> MutVisitor<'tcx> for MakeByMoveBody<'tcx> {
projection
};

// The only thing that should be left is a deref, if the parent captured
// an upvar by-ref.
std::assert_matches::assert_matches!(final_deref, [] | [mir::ProjectionElem::Deref]);
Copy link
Member Author

Choose a reason for hiding this comment

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

This assertion only makes sense when we assume that captures are going to be as precise as they can be. This is not true when we truncate captures when they don't give us anything (truncate_capture_for_optimization), and also I'm pretty sure that edition 2018 doesn't even work like that at all.


// For all of the additional projections that come out of precise capturing,
// re-apply these projections.
let additional_projections =
additional_projections.iter().map(|elem| match elem.kind {
ProjectionKind::Deref => mir::ProjectionElem::Deref,
ProjectionKind::Field(idx, VariantIdx::ZERO) => {
mir::ProjectionElem::Field(idx, elem.ty)
}
_ => unreachable!("precise captures only through fields and derefs"),
});
// These projections are applied in order to "bridge" the local that we are
// currently transforming *from* the old upvar that the by-ref coroutine used
// to capture *to* the upvar of the parent coroutine-closure. For example, if
// the parent captures `&s` but the child captures `&(s.field)`, then we will
// apply a field projection.
let bridging_projections = bridging_projections.iter().map(|elem| match elem.kind {
ProjectionKind::Deref => mir::ProjectionElem::Deref,
ProjectionKind::Field(idx, VariantIdx::ZERO) => {
mir::ProjectionElem::Field(idx, elem.ty)
}
_ => unreachable!("precise captures only through fields and derefs"),
});

// We start out with an adjusted field index (and ty), representing the
// upvar that we get from our parent closure. We apply any of the additional
Expand All @@ -326,8 +324,8 @@ impl<'tcx> MutVisitor<'tcx> for MakeByMoveBody<'tcx> {
projection: self.tcx.mk_place_elems_from_iter(
[mir::ProjectionElem::Field(remapped_idx, remapped_ty)]
.into_iter()
.chain(additional_projections)
.chain(final_deref.iter().copied()),
.chain(bridging_projections)
.chain(final_projections.iter().copied()),
),
};
}
Expand Down
17 changes: 17 additions & 0 deletions tests/ui/async-await/async-closures/truncated-fields-when-imm.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
//@ edition: 2021
//@ check-pass

#![feature(async_closure)]

pub struct Struct {
pub path: String,
}

// In `upvar.rs`, `truncate_capture_for_optimization` means that we don't actually
// capture `&(*s.path)` here, but instead just `&(*s)`, but ONLY when the upvar is
// immutable. This means that the assumption we have in `ByMoveBody` pass is wrong.
pub fn test(s: &Struct) {
let c = async move || { let path = &s.path; };
}

fn main() {}
Loading