Skip to content

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

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

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 2 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
42 changes: 41 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,47 @@ 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| {
for (p1, p2) in capture1.place.projections.iter().zip(&capture2.place.projections) {
match (p1.kind, p2.kind) {
// 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);
}

(l, r) => bug!("ProjectionKinds were different: ({:?}, {:?})", 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
98 changes: 98 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,98 @@
// 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.

#![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: First Pass analysis includes:
//~| ERROR: Min Capture analysis includes:
println!("{:?}", a.0);
//~^ NOTE: Capturing a[(0, 0)] -> ImmBorrow
//~| NOTE: Min Capture a[(0, 0)] -> ImmBorrow
println!("{:?}", a.1);
//~^ NOTE: Capturing a[(1, 0)] -> ImmBorrow
//~| NOTE: Min Capture a[(1, 0)] -> ImmBorrow

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

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: First Pass analysis includes:
//~| ERROR: Min Capture analysis includes:
println!("{:?}", a.1);
//~^ NOTE: Capturing a[(1, 0)] -> ImmBorrow
//~| NOTE: Min Capture a[(1, 0)] -> ImmBorrow
println!("{:?}", a.0);
//~^ NOTE: Capturing a[(0, 0)] -> ImmBorrow
//~| NOTE: Min Capture a[(0, 0)] -> ImmBorrow

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

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: First Pass analysis includes:
//~| ERROR: Min Capture analysis includes:
println!("{:?}", b.1);
//~^ NOTE: Capturing b[(1, 0)] -> ImmBorrow
//~| NOTE: Min Capture b[(1, 0)] -> ImmBorrow
println!("{:?}", a.1);
//~^ NOTE: Capturing a[(1, 0)] -> ImmBorrow
//~| NOTE: Min Capture a[(1, 0)] -> ImmBorrow
println!("{:?}", a.0);
//~^ NOTE: Capturing a[(0, 0)] -> ImmBorrow
//~| NOTE: Min Capture a[(0, 0)] -> ImmBorrow

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

fn main() {
test_one();
test_two();
test_three();
}
228 changes: 228 additions & 0 deletions src/test/ui/closures/2229_closure_analysis/preserve_field_drop_order.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,228 @@
error[E0658]: attributes on expressions are experimental
--> $DIR/preserve_field_drop_order.rs:20:13
|
LL | let c = #[rustc_capture_analysis]
| ^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: see issue #15701 <https://github.com/rust-lang/rust/issues/15701> for more information
= help: add `#![feature(stmt_expr_attributes)]` to the crate attributes to enable

error[E0658]: attributes on expressions are experimental
--> $DIR/preserve_field_drop_order.rs:46:13
|
LL | let c = #[rustc_capture_analysis]
| ^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: see issue #15701 <https://github.com/rust-lang/rust/issues/15701> for more information
= help: add `#![feature(stmt_expr_attributes)]` to the crate attributes to enable

error[E0658]: attributes on expressions are experimental
--> $DIR/preserve_field_drop_order.rs:72:13
|
LL | let c = #[rustc_capture_analysis]
| ^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: see issue #15701 <https://github.com/rust-lang/rust/issues/15701> for more information
= help: add `#![feature(stmt_expr_attributes)]` to the crate attributes to enable

error: First Pass analysis includes:
--> $DIR/preserve_field_drop_order.rs:23:5
|
LL | / || {
LL | |
LL | |
LL | | println!("{:?}", a.0);
... |
LL | |
LL | | };
| |_____^
|
note: Capturing a[(0, 0)] -> ImmBorrow
--> $DIR/preserve_field_drop_order.rs:26:26
|
LL | println!("{:?}", a.0);
| ^^^
note: Capturing a[(1, 0)] -> ImmBorrow
--> $DIR/preserve_field_drop_order.rs:29:26
|
LL | println!("{:?}", a.1);
| ^^^
note: Capturing b[(0, 0)] -> ImmBorrow
--> $DIR/preserve_field_drop_order.rs:33:26
|
LL | println!("{:?}", b.0);
| ^^^
note: Capturing b[(1, 0)] -> ImmBorrow
--> $DIR/preserve_field_drop_order.rs:36:26
|
LL | println!("{:?}", b.1);
| ^^^

error: Min Capture analysis includes:
--> $DIR/preserve_field_drop_order.rs:23:5
|
LL | / || {
LL | |
LL | |
LL | | println!("{:?}", a.0);
... |
LL | |
LL | | };
| |_____^
|
note: Min Capture a[(0, 0)] -> ImmBorrow
--> $DIR/preserve_field_drop_order.rs:26:26
|
LL | println!("{:?}", a.0);
| ^^^
note: Min Capture a[(1, 0)] -> ImmBorrow
--> $DIR/preserve_field_drop_order.rs:29:26
|
LL | println!("{:?}", a.1);
| ^^^
note: Min Capture b[(0, 0)] -> ImmBorrow
--> $DIR/preserve_field_drop_order.rs:33:26
|
LL | println!("{:?}", b.0);
| ^^^
note: Min Capture b[(1, 0)] -> ImmBorrow
--> $DIR/preserve_field_drop_order.rs:36:26
|
LL | println!("{:?}", b.1);
| ^^^

error: First Pass analysis includes:
--> $DIR/preserve_field_drop_order.rs:49:5
|
LL | / || {
LL | |
LL | |
LL | | println!("{:?}", a.1);
... |
LL | |
LL | | };
| |_____^
|
note: Capturing a[(1, 0)] -> ImmBorrow
--> $DIR/preserve_field_drop_order.rs:52:26
|
LL | println!("{:?}", a.1);
| ^^^
note: Capturing a[(0, 0)] -> ImmBorrow
--> $DIR/preserve_field_drop_order.rs:55:26
|
LL | println!("{:?}", a.0);
| ^^^
note: Capturing b[(1, 0)] -> ImmBorrow
--> $DIR/preserve_field_drop_order.rs:59:26
|
LL | println!("{:?}", b.1);
| ^^^
note: Capturing b[(0, 0)] -> ImmBorrow
--> $DIR/preserve_field_drop_order.rs:62:26
|
LL | println!("{:?}", b.0);
| ^^^

error: Min Capture analysis includes:
--> $DIR/preserve_field_drop_order.rs:49:5
|
LL | / || {
LL | |
LL | |
LL | | println!("{:?}", a.1);
... |
LL | |
LL | | };
| |_____^
|
note: Min Capture a[(0, 0)] -> ImmBorrow
--> $DIR/preserve_field_drop_order.rs:55:26
|
LL | println!("{:?}", a.0);
| ^^^
note: Min Capture a[(1, 0)] -> ImmBorrow
--> $DIR/preserve_field_drop_order.rs:52:26
|
LL | println!("{:?}", a.1);
| ^^^
note: Min Capture b[(0, 0)] -> ImmBorrow
--> $DIR/preserve_field_drop_order.rs:62:26
|
LL | println!("{:?}", b.0);
| ^^^
note: Min Capture b[(1, 0)] -> ImmBorrow
--> $DIR/preserve_field_drop_order.rs:59:26
|
LL | println!("{:?}", b.1);
| ^^^

error: First Pass analysis includes:
--> $DIR/preserve_field_drop_order.rs:75:5
|
LL | / || {
LL | |
LL | |
LL | | println!("{:?}", b.1);
... |
LL | |
LL | | };
| |_____^
|
note: Capturing b[(1, 0)] -> ImmBorrow
--> $DIR/preserve_field_drop_order.rs:78:26
|
LL | println!("{:?}", b.1);
| ^^^
note: Capturing a[(1, 0)] -> ImmBorrow
--> $DIR/preserve_field_drop_order.rs:81:26
|
LL | println!("{:?}", a.1);
| ^^^
note: Capturing a[(0, 0)] -> ImmBorrow
--> $DIR/preserve_field_drop_order.rs:84:26
|
LL | println!("{:?}", a.0);
| ^^^
note: Capturing b[(0, 0)] -> ImmBorrow
--> $DIR/preserve_field_drop_order.rs:88:26
|
LL | println!("{:?}", b.0);
| ^^^

error: Min Capture analysis includes:
--> $DIR/preserve_field_drop_order.rs:75:5
|
LL | / || {
LL | |
LL | |
LL | | println!("{:?}", b.1);
... |
LL | |
LL | | };
| |_____^
|
note: Min Capture b[(0, 0)] -> ImmBorrow
--> $DIR/preserve_field_drop_order.rs:88:26
|
LL | println!("{:?}", b.0);
| ^^^
note: Min Capture b[(1, 0)] -> ImmBorrow
--> $DIR/preserve_field_drop_order.rs:78:26
|
LL | println!("{:?}", b.1);
| ^^^
note: Min Capture a[(0, 0)] -> ImmBorrow
--> $DIR/preserve_field_drop_order.rs:84:26
|
LL | println!("{:?}", a.0);
| ^^^
note: Min Capture a[(1, 0)] -> ImmBorrow
--> $DIR/preserve_field_drop_order.rs:81:26
|
LL | println!("{:?}", a.1);
| ^^^

error: aborting due to 9 previous errors

For more information about this error, try `rustc --explain E0658`.
Loading