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.

Sign up for GitHub

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

Jump to bottom

[rfc 2229] Drop fully captured upvars in the same order as the regular drop code #89208

Merged
merged 4 commits into from
Sep 25, 2021
Merged

[rfc 2229] Drop fully captured upvars in the same order as the regular drop code #89208

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7a3e450
Add tests to verify the drop order of fields in fully captured upvars
wesleywiser Sep 23, 2021
ab8aef4
Drop fully captured upvars in the same order as the regular drop code
wesleywiser Sep 23, 2021
3893656
Fix tidy and respond to some feedback
wesleywiser Sep 24, 2021
dd91804
Update and add more tests
wesleywiser Sep 24, 2021
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
73 changes: 72 additions & 1 deletion compiler/rustc_typeck/src/check/upvar.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -602,7 +602,78 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
}
}

debug!("For closure={:?}, min_captures={:#?}", closure_def_id, root_var_min_capture_list);
debug!(
"For closure={:?}, min_captures before sorting={:?}",
closure_def_id, root_var_min_capture_list
);

// Now that we have the minimized list of captures, sort the captures by field id.
// This causes the closure to capture the upvars in the same order as the fields are
// declared which is also the drop order. Thus, in situations where we capture all the
// fields of some type, the obserable drop order will remain the same as it previously
// was even though we're dropping each capture individually.
// See https://github.com/rust-lang/project-rfc-2229/issues/42 and
// `src/test/ui/closures/2229_closure_analysis/preserve_field_drop_order.rs`.
for (_, captures) in &mut root_var_min_capture_list {
captures.sort_by(|capture1, capture2| {
wesleywiser marked this conversation as resolved.
Show resolved Hide resolved
for (p1, p2) in capture1.place.projections.iter().zip(&capture2.place.projections) {
// We do not need to look at the `Projection.ty` fields here because at each
// step of the iteration, the projections will either be the same and therefore
// the types must be as well or the current projection will be different and
// we will return the result of comparing the field indexes.
match (p1.kind, p2.kind) {
wesleywiser marked this conversation as resolved.
Show resolved Hide resolved
// Paths are the same, continue to next loop.
(ProjectionKind::Deref, ProjectionKind::Deref) => {}
(ProjectionKind::Field(i1, _), ProjectionKind::Field(i2, _))
if i1 == i2 => {}

// Fields are different, compare them.
(ProjectionKind::Field(i1, _), ProjectionKind::Field(i2, _)) => {
return i1.cmp(&i2);
}

// We should have either a pair of `Deref`s or a pair of `Field`s.
// Anything else is a bug.
(
l @ (ProjectionKind::Deref | ProjectionKind::Field(..)),
r @ (ProjectionKind::Deref | ProjectionKind::Field(..)),
) => bug!(
"ProjectionKinds Deref and Field were mismatched: ({:?}, {:?})",
l,
r
),
(
l
@
wesleywiser marked this conversation as resolved.
Show resolved Hide resolved
(ProjectionKind::Index
| ProjectionKind::Subslice
| ProjectionKind::Deref
| ProjectionKind::Field(..)),
r
@
(ProjectionKind::Index
| ProjectionKind::Subslice
| ProjectionKind::Deref
| ProjectionKind::Field(..)),
) => bug!(
"ProjectionKinds Index or Subslice were unexpected: ({:?}, {:?})",
l,
r
),
}
}

unreachable!(
"we captured two identical projections: capture1 = {:?}, capture2 = {:?}",
capture1, capture2
);
});
}

debug!(
"For closure={:?}, min_captures after sorting={:#?}",
closure_def_id, root_var_min_capture_list
);
typeck_results.closure_min_captures.insert(closure_def_id, root_var_min_capture_list);
}

Expand Down
101 changes: 101 additions & 0 deletions src/test/ui/closures/2229_closure_analysis/preserve_field_drop_order.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
// edition:2021

// Tests that in cases where we individually capture all the fields of a type,
// we still drop them in the order they would have been dropped in the 2018 edition.

// NOTE: It is *critical* that the order of the min capture NOTES in the stderr output
// does *not* change!

#![feature(rustc_attrs)]

#[derive(Debug)]
struct HasDrop;
impl Drop for HasDrop {
fn drop(&mut self) {
println!("dropped");
}
}

fn test_one() {
let a = (HasDrop, HasDrop);
let b = (HasDrop, HasDrop);

let c = #[rustc_capture_analysis]
Copy link
Member

Choose a reason for hiding this comment

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

My concern with this test is that we need to reply on checking the stderr, and I feel like if the fields order does get changed, we won't need to update the annoatations in this file and someone might ignore the stderr changes because the ouput needed is all there.

I think we should go with testing with stdout here because changes to stdout are observable behaviour to the user and might be more cause of concern in case something breaks down the line.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's useful to check both the internal results via #[rustc_capture_analysis] as well as the runtime results which is what these two tests do in combination. However, I totally agree with your point that as it is, it would be pretty easy for someone to change this test without realizing what parts are important and which parts aren't. I believe I can remove the checking for the notes we do not care about which should make it much more clear (this is the convention used in other ui tests as well).

//~^ ERROR: attributes on expressions are experimental
//~| NOTE: see issue #15701 <https://github.com/rust-lang/rust/issues/15701>
|| {
//~^ ERROR: Min Capture analysis includes:
//~| ERROR
println!("{:?}", a.0);
//~^ NOTE: Min Capture a[(0, 0)] -> ImmBorrow
//~| NOTE
println!("{:?}", a.1);
//~^ NOTE: Min Capture a[(1, 0)] -> ImmBorrow
//~| NOTE

println!("{:?}", b.0);
//~^ NOTE: Min Capture b[(0, 0)] -> ImmBorrow
//~| NOTE
println!("{:?}", b.1);
//~^ NOTE: Min Capture b[(1, 0)] -> ImmBorrow
//~| NOTE
};
}

fn test_two() {
let a = (HasDrop, HasDrop);
let b = (HasDrop, HasDrop);

let c = #[rustc_capture_analysis]
//~^ ERROR: attributes on expressions are experimental
//~| NOTE: see issue #15701 <https://github.com/rust-lang/rust/issues/15701>
|| {
//~^ ERROR: Min Capture analysis includes:
//~| ERROR
println!("{:?}", a.1);
//~^ NOTE: Min Capture a[(1, 0)] -> ImmBorrow
//~| NOTE
println!("{:?}", a.0);
//~^ NOTE: Min Capture a[(0, 0)] -> ImmBorrow
//~| NOTE

println!("{:?}", b.1);
//~^ NOTE: Min Capture b[(1, 0)] -> ImmBorrow
//~| NOTE
println!("{:?}", b.0);
//~^ NOTE: Min Capture b[(0, 0)] -> ImmBorrow
//~| NOTE
};
}

fn test_three() {
let a = (HasDrop, HasDrop);
let b = (HasDrop, HasDrop);

let c = #[rustc_capture_analysis]
//~^ ERROR: attributes on expressions are experimental
//~| NOTE: see issue #15701 <https://github.com/rust-lang/rust/issues/15701>
|| {
//~^ ERROR: Min Capture analysis includes:
//~| ERROR
println!("{:?}", b.1);
//~^ NOTE: Min Capture b[(1, 0)] -> ImmBorrow
//~| NOTE
println!("{:?}", a.1);
//~^ NOTE: Min Capture a[(1, 0)] -> ImmBorrow
//~| NOTE
println!("{:?}", a.0);
//~^ NOTE: Min Capture a[(0, 0)] -> ImmBorrow
//~| NOTE

println!("{:?}", b.0);
//~^ NOTE: Min Capture b[(0, 0)] -> ImmBorrow
//~| NOTE
};
}

fn main() {
test_one();
test_two();
test_three();
}
Loading