Make needs_drop less pessimistic on generators
#70015
Conversation
|
r? @davidtwco (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
|
||
| let witness = substs.witness(def_id, tcx); | ||
| let interior_tys = match &witness.kind { | ||
| ty::GeneratorWitness(tys) => tcx.erase_late_bound_regions(tys), |
There was a problem hiding this comment.
Not entirely sure the erase_late_bound_regions is completely correct here
|
@bors r+ |
|
📌 Commit 489d79d has been approved by |
bors
added
S-waiting-on-bors
…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)
|
Failed in #70325 (comment), @bors r- |
bors
added
S-waiting-on-author
|
@bors r=matthewjasper |
|
📌 Commit 1df7641 has been approved by |
bors
added
S-waiting-on-bors
…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)
…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)
…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)
|
That's weird. They pass locally even after a rebase and I'm pretty sure CI also runs mir-opt tests. |
|
Some CI builders have |
|
Ah, you're right. Can reproduce. |
|
I wonder why this wasn't hit before though – the failing test checks for cleanup blocks, which should be removed by the time the generator transform runs when |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@jonas-schievink Ping from triage, would you mind rebasing and resolve the merge conflicts? Thanks. |
|
Rebased, and ignored the test on wasm32, like some other mir-opt tests. @bors r=matthewjasper rollup=never |
|
📌 Commit ae53315 has been approved by |
bors
added
S-waiting-on-bors
| @@ -99,6 +99,23 @@ where | |||
| } | |||
| } | |||
|
|
|||
| ty::Generator(_, substs, _) => { | |||
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
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.
|
⌛ Testing commit ae53315 with merge ae96d19171e368760d20cfd76399a4e85b07f5d6... |
|
💔 Test failed - checks-azure |
bors
added
S-waiting-on-review
|
@bors retry |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-azure |
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 #69814 since that contains some fixes that are made relevant by this PR (see #69814 (comment)).(this has been merged)