Skip to content

Make needs_drop less pessimistic on generators #70015

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 3 commits into from
Apr 19, 2020
Merged

Make needs_drop less pessimistic on generators #70015

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
50c1c29
Make `needs_drop` less pessimistic on generators
jonas-schievink Mar 14, 2020
7bbdd2d
Adjust mir-opt test and make it drop something
jonas-schievink Mar 15, 2020
ae53315
Ignore generator-drop-cleanup on wasm32-bare
jonas-schievink Mar 25, 2020
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
8 changes: 3 additions & 5 deletions src/librustc_middle/ty/util.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1047,10 +1047,7 @@ pub fn needs_drop_components(
// Foreign types can never have destructors.
ty::Foreign(..) => Ok(SmallVec::new()),

// Pessimistically assume that all generators will require destructors
// as we don't know if a destructor is a noop or not until after the MIR
// state transformation pass.
ty::Generator(..) | ty::Dynamic(..) | ty::Error => Err(AlwaysRequiresDrop),
ty::Dynamic(..) | ty::Error => Err(AlwaysRequiresDrop),

ty::Slice(ty) => needs_drop_components(ty, target_layout),
ty::Array(elem_ty, size) => {
Expand Down Expand Up @@ -1083,7 +1080,8 @@ pub fn needs_drop_components(
| ty::Placeholder(..)
| ty::Opaque(..)
| ty::Infer(_)
| ty::Closure(..) => Ok(smallvec![ty]),
| ty::Closure(..)
| ty::Generator(..) => Ok(smallvec![ty]),
}
}

Expand Down
17 changes: 17 additions & 0 deletions src/librustc_ty/needs_drop.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,23 @@ where
}
}

ty::Generator(_, substs, _) => {
Copy link
Member

Choose a reason for hiding this comment

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

Do we have assertions somewhere that could catch bugs in needs_drop (i.e., catch cases where a type has drop glue but needs_drop says no)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not that I know of, but we have several mir-opt tests for generators, as well as diagnostics tests, which were (correctly) affected by this change.

let substs = substs.as_generator();
for upvar_ty in substs.upvar_tys() {
queue_type(self, upvar_ty);
}

let witness = substs.witness();
let interior_tys = match &witness.kind {
ty::GeneratorWitness(tys) => tcx.erase_late_bound_regions(tys),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not entirely sure the erase_late_bound_regions is completely correct here

_ => bug!(),
};

for interior_ty in interior_tys {
queue_type(self, interior_ty);
}
}

// Check for a `Drop` impl and whether this is a union or
// `ManuallyDrop`. If it's a struct or enum without a `Drop`
// impl then check whether the field types need `Drop`.
Expand Down
3 changes: 3 additions & 0 deletions src/test/mir-opt/generator-drop-cleanup.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,14 @@
#![feature(generators, generator_trait)]

// ignore-wasm32-bare compiled with panic=abort by default

// Regression test for #58892, generator drop shims should not have blocks
// spuriously marked as cleanup

// EMIT_MIR rustc.main-{{closure}}.generator_drop.0.mir
fn main() {
let gen = || {
let _s = String::new();
yield;
};
}
79 changes: 53 additions & 26 deletions src/test/mir-opt/generator-drop-cleanup/rustc.main-{{closure}}.generator_drop.0.mir
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,53 +1,80 @@
// MIR for `main::{{closure}}#0` 0 generator_drop
// generator_layout = GeneratorLayout { field_tys: [], variant_fields: [[], [], [], []], storage_conflicts: BitMatrix { num_rows: 0, num_columns: 0, words: [], marker: PhantomData } }
// generator_layout = GeneratorLayout { field_tys: [std::string::String], variant_fields: [[], [], [], [_0]], storage_conflicts: BitMatrix { num_rows: 1, num_columns: 1, words: [1], marker: PhantomData } }

fn main::{{closure}}#0(_1: *mut [generator@$DIR/generator-drop-cleanup.rs:8:15: 10:6 {()}]) -> () {
let mut _0: (); // return place in scope 0 at $DIR/generator-drop-cleanup.rs:8:15: 10:6
let mut _2: (); // in scope 0 at $DIR/generator-drop-cleanup.rs:8:15: 10:6
let _3: (); // in scope 0 at $DIR/generator-drop-cleanup.rs:9:9: 9:14
let mut _4: (); // in scope 0 at $DIR/generator-drop-cleanup.rs:9:9: 9:14
let mut _5: (); // in scope 0 at $DIR/generator-drop-cleanup.rs:8:18: 8:18
let mut _6: (); // in scope 0 at $DIR/generator-drop-cleanup.rs:8:15: 10:6
let mut _7: isize; // in scope 0 at $DIR/generator-drop-cleanup.rs:8:15: 10:6
fn main::{{closure}}#0(_1: *mut [generator@$DIR/generator-drop-cleanup.rs:10:15: 13:6 {std::string::String, ()}]) -> () {
let mut _0: (); // return place in scope 0 at $DIR/generator-drop-cleanup.rs:10:15: 13:6
let mut _2: (); // in scope 0 at $DIR/generator-drop-cleanup.rs:10:15: 13:6
let _3: std::string::String; // in scope 0 at $DIR/generator-drop-cleanup.rs:11:13: 11:15
let _4: (); // in scope 0 at $DIR/generator-drop-cleanup.rs:12:9: 12:14
let mut _5: (); // in scope 0 at $DIR/generator-drop-cleanup.rs:12:9: 12:14
let mut _7: (); // in scope 0 at $DIR/generator-drop-cleanup.rs:10:18: 10:18
let mut _8: (); // in scope 0 at $DIR/generator-drop-cleanup.rs:10:15: 13:6
let mut _9: isize; // in scope 0 at $DIR/generator-drop-cleanup.rs:10:15: 13:6
scope 1 {
debug _s => (((*_1) as variant#3).0: std::string::String); // in scope 1 at $DIR/generator-drop-cleanup.rs:11:13: 11:15
}
scope 2 {
let mut _6: std::vec::Vec<u8>; // in scope 2 at $DIR/generator-drop-cleanup.rs:11:18: 11:31
scope 3 {
}
}

bb0: {
_7 = discriminant((*_1)); // bb0[0]: scope 0 at $DIR/generator-drop-cleanup.rs:8:15: 10:6
switchInt(move _7) -> [0u32: bb4, 3u32: bb7, otherwise: bb8]; // bb0[1]: scope 0 at $DIR/generator-drop-cleanup.rs:8:15: 10:6
_9 = discriminant((*_1)); // bb0[0]: scope 0 at $DIR/generator-drop-cleanup.rs:10:15: 13:6
switchInt(move _9) -> [0u32: bb7, 3u32: bb11, otherwise: bb12]; // bb0[1]: scope 0 at $DIR/generator-drop-cleanup.rs:10:15: 13:6
}

bb1: {
StorageDead(_4); // bb1[0]: scope 0 at $DIR/generator-drop-cleanup.rs:9:13: 9:14
StorageDead(_3); // bb1[1]: scope 0 at $DIR/generator-drop-cleanup.rs:9:14: 9:15
goto -> bb5; // bb1[2]: scope 0 at $DIR/generator-drop-cleanup.rs:10:5: 10:6
bb1 (cleanup): {
resume; // bb1[0]: scope 0 at $DIR/generator-drop-cleanup.rs:10:15: 13:6
}

bb2: {
return; // bb2[0]: scope 0 at $DIR/generator-drop-cleanup.rs:8:15: 10:6
bb2 (cleanup): {
nop; // bb2[0]: scope 0 at $DIR/generator-drop-cleanup.rs:13:5: 13:6
goto -> bb8; // bb2[1]: scope 0 at $DIR/generator-drop-cleanup.rs:13:5: 13:6
}

bb3: {
return; // bb3[0]: scope 0 at $DIR/generator-drop-cleanup.rs:8:15: 10:6
StorageDead(_5); // bb3[0]: scope 1 at $DIR/generator-drop-cleanup.rs:12:13: 12:14
StorageDead(_4); // bb3[1]: scope 1 at $DIR/generator-drop-cleanup.rs:12:14: 12:15
drop((((*_1) as variant#3).0: std::string::String)) -> [return: bb4, unwind: bb2]; // bb3[2]: scope 0 at $DIR/generator-drop-cleanup.rs:13:5: 13:6
}

bb4: {
goto -> bb6; // bb4[0]: scope 0 at $DIR/generator-drop-cleanup.rs:8:15: 10:6
nop; // bb4[0]: scope 0 at $DIR/generator-drop-cleanup.rs:13:5: 13:6
goto -> bb9; // bb4[1]: scope 0 at $DIR/generator-drop-cleanup.rs:13:5: 13:6
}

bb5: {
goto -> bb2; // bb5[0]: scope 0 at $DIR/generator-drop-cleanup.rs:10:5: 10:6
return; // bb5[0]: scope 0 at $DIR/generator-drop-cleanup.rs:10:15: 13:6
}

bb6: {
goto -> bb3; // bb6[0]: scope 0 at $DIR/generator-drop-cleanup.rs:8:15: 10:6
return; // bb6[0]: scope 0 at $DIR/generator-drop-cleanup.rs:10:15: 13:6
}

bb7: {
StorageLive(_3); // bb7[0]: scope 0 at $DIR/generator-drop-cleanup.rs:8:15: 10:6
StorageLive(_4); // bb7[1]: scope 0 at $DIR/generator-drop-cleanup.rs:8:15: 10:6
goto -> bb1; // bb7[2]: scope 0 at $DIR/generator-drop-cleanup.rs:8:15: 10:6
goto -> bb10; // bb7[0]: scope 0 at $DIR/generator-drop-cleanup.rs:10:15: 13:6
}

bb8 (cleanup): {
goto -> bb1; // bb8[0]: scope 0 at $DIR/generator-drop-cleanup.rs:13:5: 13:6
}

bb9: {
goto -> bb5; // bb9[0]: scope 0 at $DIR/generator-drop-cleanup.rs:13:5: 13:6
}

bb10: {
goto -> bb6; // bb10[0]: scope 0 at $DIR/generator-drop-cleanup.rs:10:15: 13:6
}

bb11: {
StorageLive(_4); // bb11[0]: scope 0 at $DIR/generator-drop-cleanup.rs:10:15: 13:6
StorageLive(_5); // bb11[1]: scope 0 at $DIR/generator-drop-cleanup.rs:10:15: 13:6
goto -> bb3; // bb11[2]: scope 0 at $DIR/generator-drop-cleanup.rs:10:15: 13:6
}

bb8: {
return; // bb8[0]: scope 0 at $DIR/generator-drop-cleanup.rs:8:15: 10:6
bb12: {
return; // bb12[0]: scope 0 at $DIR/generator-drop-cleanup.rs:10:15: 13:6
}
}
15 changes: 6 additions & 9 deletions src/test/ui/generator/borrowing.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,19 +1,16 @@
error[E0597]: `a` does not live long enough
--> $DIR/borrowing.rs:9:33
|
LL | let _b = {
| -- borrow later stored here
LL | let a = 3;
LL | Pin::new(&mut || yield &a).resume(())
| ----------^
| | |
| | borrowed value does not live long enough
| -- ^ borrowed value does not live long enough
| |
| value captured here by generator
| a temporary with access to the borrow is created here ...
LL |
LL | };
| -- ... and the borrow might be used here, when that temporary is dropped and runs the destructor for generator
| |
| `a` dropped here while still borrowed
|
= note: The temporary is part of an expression at the end of a block. Consider forcing this temporary to be dropped sooner, before the block's local variables are dropped. For example, you could save the expression's value in a new local variable `x` and then make `x` be the expression at the end of the block.
| - `a` dropped here while still borrowed

error[E0597]: `a` does not live long enough
--> $DIR/borrowing.rs:16:20
Expand Down
7 changes: 3 additions & 4 deletions src/test/ui/generator/retain-resume-ref.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,9 @@ error[E0499]: cannot borrow `thing` as mutable more than once at a time
LL | gen.as_mut().resume(&mut thing);
| ---------- first mutable borrow occurs here
LL | gen.as_mut().resume(&mut thing);
| ^^^^^^^^^^ second mutable borrow occurs here
LL |
LL | }
| - first borrow might be used here, when `gen` is dropped and runs the destructor for generator
| ------ ^^^^^^^^^^ second mutable borrow occurs here
| |
| first borrow later used by call

error: aborting due to previous error

Expand Down