[generator_interior] Be more precise with scopes of borrowed places #94309

eholk · 2022-02-23T23:58:13Z

Previously the generator interior type checking analysis would use the nearest temporary scope as the scope of a borrowed value. This ends up being overly broad for cases such as:

fn status(_client_status: &Client) -> i16 {
    200
}

fn main() {
    let client = Client;
    let g = move || match status(&client) {
        _status => yield,
    };
    assert_send(g);
}

In this case, the borrow &client could be considered in scope for the entirety of the match expression, meaning it would be viewed as live across the yield, therefore making the generator not Send.

In most cases, we want to use the enclosing expression as the scope for a borrowed value which will be less than or equal to the nearest temporary scope. This PR changes the analysis to use the enclosing expression as the scope for most borrows, with the exception of borrowed RValues which are true temporary values that should have the temporary scope. There's one further exception where borrows of a copy such as happens in autoref cases also should be ignored despite being RValues.

Joint work with @nikomatsakis

Fixes #57017

r? @tmandry

bors · 2022-02-25T11:43:33Z

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

compiler/rustc_typeck/src/check/generator_interior.rs

compiler/rustc_typeck/src/expr_use_visitor.rs

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

bors · 2022-03-05T08:02:04Z

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

Co-authored-by: Eric Holk <[email protected]>

This is all almost certainly wrong

Also includes a lengthy comment arguing the correctness. Co-authored-by: Niko Matsakis <[email protected]>

tmandry · 2022-03-17T15:48:55Z

The change makes sense to me and is well documented. Thanks!

@bors r+

bors · 2022-03-17T15:51:44Z

📌 Commit 2fcd542 has been approved by tmandry

bors · 2022-03-17T15:51:46Z

🌲 The tree is currently closed for pull requests below priority 100. This pull request will be tested once the tree is reopened.

Rollup of 10 pull requests Successful merges: - rust-lang#91133 (Improve `unsafe` diagnostic) - rust-lang#93222 (Make ErrorReported impossible to construct outside `rustc_errors`) - rust-lang#93745 (Stabilize ADX target feature) - rust-lang#94309 ([generator_interior] Be more precise with scopes of borrowed places) - rust-lang#94698 (Remove redundant code from copy-suggestions) - rust-lang#94731 (Suggest adding `{ .. }` around a const function call with arguments) - rust-lang#94960 (Fix many spelling mistakes) - rust-lang#94982 (Add deprecated_safe feature gate and attribute, cc rust-lang#94978) - rust-lang#94997 (debuginfo: Fix ICE when generating name for type that produces a layout error.) - rust-lang#95000 (Fixed wrong type name in comment) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

[generator_interior] Be more precise with scopes of borrowed places Previously the generator interior type checking analysis would use the nearest temporary scope as the scope of a borrowed value. This ends up being overly broad for cases such as: ```rust fn status(_client_status: &Client) -> i16 { 200 } fn main() { let client = Client; let g = move || match status(&client) { _status => yield, }; assert_send(g); } ``` In this case, the borrow `&client` could be considered in scope for the entirety of the `match` expression, meaning it would be viewed as live across the `yield`, therefore making the generator not `Send`. In most cases, we want to use the enclosing expression as the scope for a borrowed value which will be less than or equal to the nearest temporary scope. This PR changes the analysis to use the enclosing expression as the scope for most borrows, with the exception of borrowed RValues which are true temporary values that should have the temporary scope. There's one further exception where borrows of a copy such as happens in autoref cases also should be ignored despite being RValues. Joint work with `@nikomatsakis` Fixes rust-lang#57017 r? `@tmandry`

Drop Tracking: Implement `fake_read` callback This PR updates drop tracking's use of `ExprUseVisitor` so that we treat `fake_read` events as borrows. Without doing this, we were not handling match expressions correctly, which showed up as a breakage in the `addassign-yield.rs` test. We did not previously notice this because we still had rather large temporary scopes that we held borrows for, which changed in rust-lang#94309. This PR also includes a variant of the `addassign-yield.rs` test case to make sure we continue to have correct behavior here with drop tracking. r? `@nikomatsakis`

rust-highfive assigned tmandry Feb 23, 2022

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Feb 23, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 23, 2022

This comment has been minimized.

Sign in to view

nikomatsakis reviewed Feb 28, 2022

View reviewed changes

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

nikomatsakis reviewed Feb 28, 2022

View reviewed changes

compiler/rustc_typeck/src/expr_use_visitor.rs Outdated Show resolved Hide resolved

nikomatsakis requested changes Feb 28, 2022

View reviewed changes

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

eholk force-pushed the issue-57017 branch from 3f67153 to 91d9bee Compare March 1, 2022 00:39

nikomatsakis and others added 6 commits March 7, 2022 09:43

try to fix issue 57017, but not quite there yet

60f5cad

Co-authored-by: Eric Holk <[email protected]>

Move test to right place

513a9c6

Trying to detect autorefs to avoid unnecessary borrowed temporaries

ac804f2

This is all almost certainly wrong

Distinguish borrows of copies from other borrows

87ad668

Update clippy to new ExprUseVisitor delegate

9f0f46f

Add comments based on code review feedback

170b027

eholk force-pushed the issue-57017 branch from 479a0d0 to 170b027 Compare March 7, 2022 18:11

eholk and others added 2 commits March 10, 2022 17:14

Use projections rather than is_autoref

12d8ca1

Also includes a lengthy comment arguing the correctness. Co-authored-by: Niko Matsakis <[email protected]>

Remove is_autoref parameter

2fcd542

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 Mar 17, 2022

Dylan-DPC mentioned this pull request Mar 17, 2022

Rollup of 10 pull requests #95056

Merged

bors merged commit 8499a8b into rust-lang:master Mar 18, 2022

rustbot added this to the 1.61.0 milestone Mar 18, 2022

eholk mentioned this pull request May 10, 2022

Drop Tracking: Implement fake_read callback #96923

Merged

eholk mentioned this pull request Jul 6, 2022

Fix drop-tracking ICE when a struct containing a field with a significant drop is used across an await #98754

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[generator_interior] Be more precise with scopes of borrowed places #94309

[generator_interior] Be more precise with scopes of borrowed places #94309

eholk commented Feb 23, 2022

This comment has been minimized.

bors commented Feb 25, 2022

bors commented Mar 5, 2022

tmandry commented Mar 17, 2022

bors commented Mar 17, 2022

bors commented Mar 17, 2022

[generator_interior] Be more precise with scopes of borrowed places #94309

[generator_interior] Be more precise with scopes of borrowed places #94309

Conversation

eholk commented Feb 23, 2022

This comment has been minimized.

bors commented Feb 25, 2022

bors commented Mar 5, 2022

tmandry commented Mar 17, 2022

bors commented Mar 17, 2022

bors commented Mar 17, 2022