Introduce drop range tracking to generator interior analysis #91032

eholk · 2021-11-19T02:36:31Z

This PR addresses cases such as this one from #57478:

struct Foo;
impl !Send for Foo {}

let _: impl Send = || {
    let guard = Foo;
    drop(guard);
    yield;
};

Previously, the generator_interior pass would unnecessarily include the type Foo in the generator because it was not aware of the behavior of drop. We fix this issue by introducing a drop range analysis that finds portions of the code where a value is guaranteed to be dropped. If a value is dropped at all suspend points, then it is no longer included in the generator type. Note that we are using "dropped" in a generic sense to include any case in which a value has been moved. That is, we do not only look at calls to the drop function.

There are several phases to the drop tracking algorithm, and we'll go into more detail below.

Use ExprUseVisitor to find values that are consumed and borrowed.
DropRangeVisitor uses consume and borrow information to gather drop and reinitialization events, as well as build a control flow graph.
We then propagate drop and reinitialization information through the CFG until we reach a fix point (see DropRanges::propagate_to_fixpoint).
When recording a type (see InteriorVisitor::record), we check the computed drop ranges to see if that value is definitely dropped at the suspend point. If so, we skip including it in the type.

1. Use `ExprUseVisitor` to find values that are consumed and borrowed.

We use ExprUseVisitor to identify the places where values are consumed. We track both the hir_id of the value, and the hir_id of the expression that consumes it. For example, in the expression [Foo], the Foo is consumed by the array expression, so after the array expression we can consider the Foo temporary to be dropped.

In this process, we also collect values that are borrowed. The reason is that the MIR transform for generators conservatively assumes anything borrowed is live across a suspend point (see rustc_mir_transform::generator::locals_live_across_suspend_points). We match this behavior here as well.

2. Gather drop events, reinitialization events, and control flow graph

After finding the values of interest, we perform a post-order traversal over the HIR tree to find the points where these values are dropped or reinitialized. We use the post-order index of each event because this is how the existing generator interior analysis refers to the position of suspend points and the scopes of variables.

During this traversal, we also record branching and merging information to handle control flow constructs such as if, match, and loop. This is necessary because values may be dropped along some control flow paths but not others.

3. Iterate to fixed point

The previous pass found the interesting events and locations, but now we need to find the actual ranges where things are dropped. Upon entry, we have a list of nodes ordered by their position in the post-order traversal. Each node has a set of successors. For each node we additionally keep a bitfield with one bit per potentially consumed value. The bit is set if we the value is dropped along all paths entering this node.

To compute the drop information, we first reverse the successor edges to find each node's predecessors. Then we iterate through each node, and for each node we set its dropped value bitfield to the intersection of all incoming dropped value bitfields.

If any bitfield for any node changes, we re-run the propagation loop again.

4. Ignore dropped values across suspend points

At this point we have a data structure where we can ask whether a value is guaranteed to be dropped at any post order index for the HIR tree. We use this information in InteriorVisitor to check whether a value in question is dropped at a particular suspend point. If it is, we do not include that value's type in the generator type.

Note that we had to augment the region scope tree to include all yields in scope, rather than just the last one as we did before.

r? @nikomatsakis

eholk · 2021-11-19T02:37:20Z

/cc @guswynn - You are probably interested in this too, since it should hopefully solve some of the must_not_suspend lint issues too.

bors · 2021-11-20T14:09:06Z

☔ The latest upstream changes (presumably #91080) made this pull request unmergeable. Please resolve the merge conflicts.

nikomatsakis

Started reading. Here are a few comments. Will schedule some time to read more!

compiler/rustc_typeck/src/check/generator_interior.rs

compiler/rustc_typeck/src/check/generator_interior/drop_ranges.rs

nikomatsakis · 2021-12-13T15:30:04Z

@eholk did a first review, this code looks nice, left some suggestions for refactorings, and I'll take a look after you're done!

eholk · 2021-12-14T00:52:13Z

@rustbot ready

bors · 2021-12-15T07:01:15Z

☔ The latest upstream changes (presumably #91945) made this pull request unmergeable. Please resolve the merge conflicts.

nikomatsakis

Hi @eholk! More review thoughts. :)

compiler/rustc_typeck/src/check/generator_interior/drop_ranges.rs

compiler/rustc_typeck/src/check/generator_interior/drop_ranges/cfg_propagate.rs

compiler/rustc_typeck/src/check/generator_interior/drop_ranges/cfg_build.rs

nikomatsakis · 2021-12-15T21:44:38Z

compiler/rustc_typeck/src/check/generator_interior/drop_ranges/cfg_build.rs

+                        self.drop_ranges.add_control_edge(fork, self.expr_count + 1);
+                        self.visit_pat(pat);
+                        match guard {
+                            Some(Guard::If(expr)) => self.visit_expr(expr),


Isn't there a missing control edge here?

i.e., after we execute the if, we could go on to execute any future arm, right?

I wonder if it wouldn't be better to model this as

fork -> arm0 arm0 -> {guard0, arm1} guard0 -> {arm-body0, arm1} arm-body0 -> end ...

Example test case:

async fn main() { let x = vec![22_usize]; std::mem::drop(x); match y() { true if {x = vec![]; false} => {} _ => { dummy().await } } } async fn dummy() { } fn y() -> bool {true}

In this test, I imagine we might incorrectly conclude that x must be dropped and not re-initialized in this arm.

Ah, good catch. I modified your example a little bit and am adding it as a test case. I like your idea of modeling match almost more like a chain of ifs instead, so I'll do that instead.

For patterns containing multiple alternatives the execution could also go from the guard back to the same arm.

Good point, @tmiasko. Do you have an idea how we might observe this? I tried this:

let mut x = vec![22_usize]; std::mem::drop(x); match false { true | false if { yield; x = vec![]; false } => {} _ => {} }

This doesn't work because I get an error on the yield saying that the borrow of false in the match is still active at the yield point. Does this mean we just can't yield in a match guard? It seems like elsewhere we can yield with borrows still active, so I'm not sure why this case doesn't work...

Here's the error message:

error[E0626]: borrow may still be in use when generator yields --> .\src\test\ui\generator\reinit-in-match-guard.rs:26:15 | 26 | match false { | ^^^^^ ... 29 | yield; | ----- possible yield occurs here

The example could be modified to use a wildcard pattern multiple times; it doesn't introduce any borrows.

compiler/rustc_typeck/src/check/generator_interior/drop_ranges/cfg_propagate.rs

compiler/rustc_typeck/src/check/generator_interior/drop_ranges/record_consumed_borrow.rs

compiler/rustc_typeck/src/check/generator_interior/drop_ranges.rs

src/test/ui/async-await/async-fn-nonsend.rs

eholk · 2021-12-16T22:13:21Z

@nikomatsakis - thanks for the comments! I think I've addressed most of them now. I'm going to keep working this afternoon on tracking the potentially dirty sets in the CFG propagation and I'll push another patch up for that.

eholk · 2021-12-17T01:20:47Z

I just got the change tracking working, so is ready to take another look at.

eholk · 2021-12-20T21:07:11Z

@nikomatsakis and I just had a call to talk about this PR. We figured out that the PR's not handling partial drops correctly. Here's a test case that shows the issue:

#![feature(negative_impls)]

fn main() {
    gimme_send(foo());
}

fn gimme_send<T: Send>(t: T) {
    drop(t);
}

struct NotSend {}

impl Drop for NotSend {
    fn drop(&mut self) {}
}

impl !Send for NotSend {}

async fn foo() {
    let mut x = (NotSend {},);
    drop(x.0);
    x.0 = NotSend {};
    bar().await;
}

async fn bar() {}

We decided the way to work around this is to ignore partial drops, and count a partial initialization as an initialization of the whole variable. That's a safer, conservative approximation and if we want in the future we can look into handling partial drops too.

nikomatsakis

Looking pretty good! Mostly small doc requests here, one meaningful-ish change, although I'm not sure if it's observable. I'd like to review the tests once more time.

nikomatsakis · 2022-01-13T15:42:43Z

compiler/rustc_typeck/src/check/generator_interior/drop_ranges.rs

+    DropRanges { tracked_value_map: drop_ranges.tracked_value_map, nodes: drop_ranges.nodes }
+}
+
+/// Applies `f` to consumable portion of a HIR node.


I would like to see more concrete documentation, with examples. What is place and node and how are they linked?

I got rid of the node parameter and just passed hir instead so this function can call hir.find on its own, since that seems harder to get wrong. I think I wrote it this way originally because I was running into borrow checker errors where hir ended up borrowing self in the caller and f borrowed self mutably. Maybe I was passing a &Map instead of a Map...

I tried to clarify the documentation too.

compiler/rustc_typeck/src/check/generator_interior/drop_ranges.rs

nikomatsakis · 2022-01-13T15:50:40Z

compiler/rustc_typeck/src/check/generator_interior/drop_ranges/cfg_build.rs

+                let (guard_exit, arm_end_ids) = arms.iter().fold(
+                    (self.expr_index, vec![]),
+                    |(incoming_edge, mut arm_end_ids), hir::Arm { pat, body, guard, .. }| {
+                        self.drop_ranges.add_control_edge(incoming_edge, self.expr_index + 1);


This code is kind of bending my mind! I think what would help is if you had an ascii art diagram (https://asciiflow.com/ ftw!) with labels on the various edges that get added, and then you could add a comment to each of the add_control_edge calls to indicate which edge it is adding.

nikomatsakis · 2022-01-13T16:04:29Z

compiler/rustc_typeck/src/check/generator_interior/drop_ranges.rs

+}
+
+impl From<&PlaceWithHirId<'_>> for TrackedValue {
+    fn from(place_with_id: &PlaceWithHirId<'_>) -> Self {


Can you add an assertion that place_with_id.projections is empty?

Or implement TryFrom and use unwrap at the caller

I decided to go with TryFrom.

nikomatsakis · 2022-01-13T17:15:45Z

compiler/rustc_typeck/src/check/generator_interior/drop_ranges/cfg_build.rs

+        }
+    }
+
+    fn reinit_expr(&mut self, expr: &hir::Expr<'_>) {


I think it would be good to overapproximate what is initialized here. In other words, if see an assignment like a.b.c = 22, we can consider that a reinitialization of a.

We can then leave a comment that our analysis is always approximated towards more things being initialized than actually are.

It's true that this code doesn't compile today, but the way that this is setup, if we ever did make reinitialization compile, the following bit of code would go wrong I believe:

let pair: (String, String) = ...; drop(pair); pair.0 = ...; pair.1 = ...;

Here, neither pair.0 nor pair.1 would be considered to reinitialize pair, but together they would do so.

I changed this to match on expr.kind and recurse for Field expressions.

We might need to handle other expressions, including horrible things like let mut x = 5; *(loop { break &mut x; }) = 6;, but I think we're okay here for a couple of reasons:

In order to do an assignment like this, we'll have to borrow a variable somewhere. This PR already ignore drops on variables that are borrowed.

You can't borrow a variable that's been dropped.

nikomatsakis · 2022-01-13T17:24:24Z

compiler/rustc_typeck/src/check/generator_interior/drop_ranges/cfg_build.rs

+/// We are interested in points where a variables is dropped or initialized, and the control flow
+/// of the code. We identify locations in code by their post-order traversal index, so it is
+/// important for this traversal to match that in `RegionResolutionVisitor` and `InteriorVisitor`.
+struct DropRangeVisitor<'a, 'tcx> {


I think that we need a comment explaining what kind of approximations are made, particularly around partial paths. I believe the gist is:

Moving a counts as a move of a

Moving a partial path like a.b.c is ignored

Reinitializing a.b.c counts as a reinitialization of a

I would use some examples like this:

let mut a = (vec![0], vec![0]); drop(a.0); drop(a.1); // a still considered initialized

Yup, those are the rules we want. I've tried to capture them in a comment.

nikomatsakis · 2022-01-13T18:50:05Z

src/test/ui/generator/partial-drop.rs

+    assert_send(|| {
+        //~^ ERROR generator cannot be sent between threads safely
+        // FIXME: it would be nice to make this work.
+        let guard = Bar { foo: Foo, x: 42 };


can you make a test like

let guard = Bar { ... }; let Bar { foo, x } = guard; drop(foo);

Does that work? I think ... maybe? It depends a bit on what events the EUV generates.

It looks like it does not currently work.

eholk · 2022-01-15T01:45:43Z

I just pushed up a new change that should address your comments. Thanks for the helpful review, as always!

nikomatsakis

r=me

nikomatsakis · 2022-01-18T20:39:00Z

compiler/rustc_typeck/src/check/generator_interior/drop_ranges/cfg_build.rs

+                //      └─┘    ├─┴──►└─┴┐     │
+                //             │        │     │
+                //     }       ▼        ▼     │
+                //     ┌─┐◄───────────────────┘


nikomatsakis · 2022-01-18T20:39:59Z

@bors delegate

nikomatsakis · 2022-01-18T20:40:11Z

@bors delegate+

bors · 2022-01-18T20:40:13Z

✌️ @eholk can now approve this pull request

eholk · 2022-01-18T22:04:25Z

Thanks for the approval, @nikomatsakis!

r=me

This changes drop range analysis to handle uninhabited return types such as `!`. Since these calls to these functions do not return, we model them as ending in an infinite loop.

The previous commit made the non_sync_with_method_call case pass due to the await being unreachable. Unfortunately, this isn't actually the behavior the test was verifying. This change lifts the panic into a helper function so that the generator analysis still thinks the await is reachable, and therefore we preserve the same testing behavior.

This makes it clearer what values we are tracking and why.

We previously weren't tracking partial re-inits while being too aggressive around partial drops. With this change, we simply ignore partial drops, which is the safer, more conservative choice.

eholk · 2022-01-19T00:55:57Z

@bors r=me

nikomatsakis · 2022-01-20T17:11:59Z

@bors r+

bors · 2022-01-20T17:12:00Z

📌 Commit 76f6b57 has been approved by nikomatsakis

…askrgr Rollup of 17 pull requests Successful merges: - rust-lang#91032 (Introduce drop range tracking to generator interior analysis) - rust-lang#92856 (Exclude "test" from doc_auto_cfg) - rust-lang#92860 (Fix errors on blanket impls by ignoring the children of generated impls) - rust-lang#93038 (Fix star handling in block doc comments) - rust-lang#93061 (Only suggest adding `!` to expressions that can be macro invocation) - rust-lang#93067 (rustdoc mobile: fix scroll offset when jumping to internal id) - rust-lang#93086 (Add tests to ensure that `let_chains` works with `if_let_guard`) - rust-lang#93087 (Fix src/test/run-make/raw-dylib-alt-calling-convention) - rust-lang#93091 (⬆ chalk to 0.76.0) - rust-lang#93094 (src/test/rustdoc-json: Check for `struct_field`s in `variant_tuple_struct.rs`) - rust-lang#93098 (Show a more informative panic message when `DefPathHash` does not exist) - rust-lang#93099 (rustdoc: auto create output directory when "--output-format json") - rust-lang#93102 (Pretty printer algorithm revamp step 3) - rust-lang#93104 (Support --bless for pp-exact pretty printer tests) - rust-lang#93114 (update comment for `ensure_monomorphic_enough`) - rust-lang#93128 (Add script to prevent point releases with same number as existing ones) - rust-lang#93136 (Backport the 1.58.1 release notes to master) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

RalfJung · 2022-01-21T15:29:19Z

Is it possible that this causes the ICE in #93161 ?

(Btw this large PR should probably have been rollup=never)

eholk · 2022-01-21T17:04:34Z

@RalfJung - Yes, that looks like it.

I'll try and get a fix ready soon, but if you are blocked I can make a small patch that disables the drop tracking without having to untangle this PR from the ones it was rolled up with. There's basically one if that needs changed.

I'll remember rollup=never for the future, thanks for the suggestion.

rust-highfive assigned nikomatsakis Nov 19, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 19, 2021

apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Nov 22, 2021

eholk force-pushed the generator-drop-tracking branch from a2c78ba to 4cba899 Compare November 22, 2021 22:54

tmandry mentioned this pull request Nov 29, 2021

must_not_suspend should trigger for temporary in match expression (but does not) #90937

Open

nikomatsakis reviewed Dec 6, 2021

View reviewed changes

compiler/rustc_typeck/src/check/generator_interior.rs Outdated Show resolved Hide resolved

compiler/rustc_typeck/src/check/generator_interior.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

eholk force-pushed the generator-drop-tracking branch from 0e050ec to 5381442 Compare December 6, 2021 20:10

nikomatsakis requested changes Dec 13, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

eholk force-pushed the generator-drop-tracking branch from c36695e to 1dc57eb Compare December 15, 2021 20:36

nikomatsakis requested changes Dec 15, 2021

View reviewed changes

dingxiangfei2009 mentioned this pull request Jan 3, 2022

Refine scopes around temporaries generated in local accesses #92508

Closed

nikomatsakis requested changes Jan 13, 2022

View reviewed changes

This comment has been minimized.

Sign in to view

nikomatsakis approved these changes Jan 18, 2022

View reviewed changes

This comment has been minimized.

Sign in to view

eholk added 7 commits January 18, 2022 14:25

Handle uninhabited return types

787f4cb

This changes drop range analysis to handle uninhabited return types such as `!`. Since these calls to these functions do not return, we model them as ending in an infinite loop.

drop_ranges: Add TrackedValue enum

78c5644

This makes it clearer what values we are tracking and why.

Safely handle partial drops

32930d9

We previously weren't tracking partial re-inits while being too aggressive around partial drops. With this change, we simply ignore partial drops, which is the safer, more conservative choice.

Respond to code review comments

e0a5370

Use .. patterns in cfg_build.rs

d840d0c

Fix build after rebase

76f6b57

eholk force-pushed the generator-drop-tracking branch from 53e729b to 76f6b57 Compare January 18, 2022 22:45

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 20, 2022

matthiaskrgr mentioned this pull request Jan 20, 2022

Rollup of 17 pull requests #93138

Merged

bors merged commit 3d10c64 into rust-lang:master Jan 21, 2022

rustbot added this to the 1.60.0 milestone Jan 21, 2022

nico-abram mentioned this pull request Jan 21, 2022

miri no longer builds after rust-lang/rust#93138 #93149

Closed

RalfJung mentioned this pull request Jan 21, 2022

ICE for generators involving Never type #93161

Closed

eholk deleted the generator-drop-tracking branch January 21, 2022 17:55

RalfJung mentioned this pull request Jan 21, 2022

Disable drop range tracking in generators #93165

Merged

compiler-errors mentioned this pull request Jan 23, 2022

Broken MIR: generator contains type &mut Body in MIR #93246

Closed

matthiaskrgr mentioned this pull request Jan 24, 2022

Regression in async generator and fmt internals: loss of Send #93274

Closed

ecstatic-morse mentioned this pull request Jan 25, 2022

rewrite liveness analysis to be based on MIR #51003

Open

eholk mentioned this pull request Jan 26, 2022

Fix drop tracking ICEs and re-enable generator drop tracking #93180

Closed

eholk mentioned this pull request Jan 27, 2022

Dropped variables still included in generator type #57478

Closed

eholk mentioned this pull request May 23, 2022

Tracking issue for enabling -Zdrop-tracking by default #97331

Closed

7 tasks

Introduce drop range tracking to generator interior analysis #91032

Introduce drop range tracking to generator interior analysis #91032

Conversation

eholk commented Nov 19, 2021 • edited Loading

1. Use ExprUseVisitor to find values that are consumed and borrowed.

2. Gather drop events, reinitialization events, and control flow graph

3. Iterate to fixed point

4. Ignore dropped values across suspend points

eholk commented Nov 19, 2021

bors commented Nov 20, 2021

nikomatsakis left a comment

Choose a reason for hiding this comment

This comment has been minimized.

nikomatsakis commented Dec 13, 2021

This comment has been minimized.

eholk commented Dec 14, 2021

bors commented Dec 15, 2021

nikomatsakis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eholk commented Dec 16, 2021

eholk commented Dec 17, 2021

eholk commented Dec 20, 2021

nikomatsakis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eholk commented Jan 15, 2022 • edited Loading

This comment has been minimized.

nikomatsakis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis commented Jan 18, 2022

nikomatsakis commented Jan 18, 2022

bors commented Jan 18, 2022

eholk commented Jan 18, 2022

This comment has been minimized.

eholk commented Jan 19, 2022 • edited Loading

nikomatsakis commented Jan 20, 2022

bors commented Jan 20, 2022

RalfJung commented Jan 21, 2022

eholk commented Jan 21, 2022

eholk commented Nov 19, 2021 •

edited

Loading

1. Use `ExprUseVisitor` to find values that are consumed and borrowed.

eholk commented Jan 15, 2022 •

edited

Loading

eholk commented Jan 19, 2022 •

edited

Loading