Skip to content

Commit

Permalink
Rollup merge of rust-lang#70015 - jonas-schievink:gen-needs-drop, r=m…
Browse files Browse the repository at this point in the history
…atthewjasper

Make `needs_drop` less pessimistic on generators

Generators only have non-trivial drop logic when they may store (in upvars or across yields) a type that does.

This prevents generation of some unnecessary MIR in simple generators. There might be some impact on compile times, but this is probably limited in real-world applications.

~~This builds off of rust-lang#69814 since that contains some fixes that are made relevant by *this* PR (see rust-lang#69814 (comment) (this has been merged)
  • Loading branch information
Centril authored Mar 23, 2020
2 parents 6d0c041 + 489d79d commit 4432cb3
Show file tree
Hide file tree
Showing 5 changed files with 59 additions and 33 deletions.
8 changes: 3 additions & 5 deletions src/librustc/ty/util.rs
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
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,23 @@ where
}
}

ty::Generator(def_id, substs, _) => {
let substs = substs.as_generator();
for upvar_ty in substs.upvar_tys(def_id, tcx) {
queue_type(self, upvar_ty);
}

let witness = substs.witness(def_id, tcx);
let interior_tys = match &witness.kind {
ty::GeneratorWitness(tys) => tcx.erase_late_bound_regions(tys),
_ => 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
45 changes: 30 additions & 15 deletions src/test/mir-opt/generator-drop-cleanup.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

fn main() {
let gen = || {
let _s = String::new();
yield;
};
}
Expand All @@ -13,35 +14,49 @@ fn main() {

// START rustc.main-{{closure}}.generator_drop.0.mir
// bb0: {
// _7 = discriminant((*_1));
// switchInt(move _7) -> [0u32: bb4, 3u32: bb7, otherwise: bb8];
// _9 = discriminant((*_1));
// switchInt(move _9) -> [0u32: bb7, 3u32: bb11, otherwise: bb12];
// }
// bb1: {
// StorageDead(_4);
// StorageDead(_3);
// goto -> bb5;
// bb1 (cleanup): {
// resume;
// }
// bb2: {
// return;
// bb2 (cleanup): {
// nop;
// goto -> bb8;
// }
// bb3: {
// return;
// StorageDead(_5);
// StorageDead(_4);
// drop((((*_1) as variant#3).0: std::string::String)) -> [return: bb4, unwind: bb2];
// }
// bb4: {
// goto -> bb6;
// nop;
// goto -> bb9;
// }
// bb5: {
// goto -> bb2;
// return;
// }
// bb6: {
// goto -> bb3;
// return;
// }
// bb7: {
// StorageLive(_3);
// StorageLive(_4);
// goto -> bb10;
// }
// bb8 (cleanup): {
// goto -> bb1;
// }
// bb8: {
// bb9: {
// goto -> bb5;
// }
// bb10: {
// goto -> bb6;
// }
// bb11: {
// StorageLive(_4);
// StorageLive(_5);
// goto -> bb3;
// }
// bb12: {
// return;
// }
// END rustc.main-{{closure}}.generator_drop.0.mir
15 changes: 6 additions & 9 deletions src/test/ui/generator/borrowing.stderr
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
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

0 comments on commit 4432cb3

Please sign in to comment.