Skip to content
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.

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

Fix spurious error when a Drop local has an assignment in a loop #102078

Closed
wants to merge 1 commit into from

Conversation

Aaron1011
Copy link
Member

@Aaron1011 Aaron1011 commented Sep 20, 2022

Fixes #70919

DropAndReplace terminators are special - unlike all other terminators, they perform two distinct actions (a drop and a store) to the same Place. This complicates various analysis passes that we do, which expect at most one of 'Drop'/'Def'/'Use' for a local at a given MIR location.

Previously, we categorized a DropAndReplace terminator's Local access as MutatingUseContext::Drop. As a result, livness computation would not consider the DropAndReplace as a store ('Def') of a local. The "use live" set would end up being too large (a use would be seen to extend back to an earlier store, rather than the closure DropAndReplace).

We now explicitly propagate information about DropAndReplace terminators via new enum variants MutatingUseContext:DropAndReplace and DefUse::DropAndReplace. We use this information in liveness computation to record both a Drop and a Def. This prevents creating an extra-large "use live" set, while ensuring that te local is still considered "drop live" at the required locations.

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Sep 20, 2022
@rustbot
Copy link
Collaborator

rustbot commented Sep 20, 2022

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@rust-highfive
Copy link
Collaborator

r? @nagisa

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 20, 2022
@pnkfelix
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Sep 21, 2022

📌 Commit ba8d1eb689dea77b153b0ebb20bfc3d6eceb338a has been approved by pnkfelix

It is now in the queue for this repository.

@bors 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 Sep 21, 2022
@tmiasko
Copy link
Contributor

tmiasko commented Sep 21, 2022

@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 21, 2022
`DropAndReplace` terminators are special - unlike all other terminators,
they perform two distinct actions (a drop and a store) to the same
`Place`. This complicates various analysis passes that we do,
which expect at most one of 'Drop'/'Def'/'Use' for a local at
a given MIR location.

Previously, we categorized a `DropAndReplace` terminator's `Local`
access as `MutatingUseContext::Drop`. As a result, livness computation
would not consider the `DropAndReplace` as a store ('Def') of a local.
The "use live" set would end up being too large (a use would
be seen to extend back to an earlier store, rather than the closure
`DropAndReplace`).

We now explicitly propagate information about `DropAndReplace`
terminators via new enum variants `MutatingUseContext:DropAndReplace`
and `DefUse::DropAndReplace`. We use this information in liveness
computation to record *both* a Drop and a Def. This prevents creating
an extra-large "use live" set, while ensuring that te local is still
considered "drop live" at the required locations.
@Aaron1011
Copy link
Member Author

@tmiasko Updated

@tmiasko
Copy link
Contributor

tmiasko commented Sep 21, 2022

@bors r=pnkfelix,tmiasko

@bors
Copy link
Contributor

bors commented Sep 21, 2022

📌 Commit 3bb8c22 has been approved by pnkfelix,tmiasko

It is now in the queue for this repository.

@bors 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 21, 2022
@nagisa
Copy link
Member

nagisa commented Sep 22, 2022

r? @pnkfelix

@rust-highfive rust-highfive assigned pnkfelix and unassigned nagisa Sep 22, 2022
@Aaron1011
Copy link
Member Author

After doing additional testing, I've found that this PR has an odd effect on this code:

struct HasDrop<T>(T);
impl<T> Drop for HasDrop<T> {
    fn drop(&mut self) {}
}

#[allow(unused_assignments)]
#[allow(unused_variables)]
fn foo() {
    let mut first: u8 = 0;
    let mut has_drop = Some(HasDrop(&first));

    loop {
        match true {
            true => {
                has_drop = Some(HasDrop(&first));
            }
            false => {
                drop(has_drop);
                has_drop = None;
                let reborrow = &mut first;
            }
        }
        
    }
}

fn main() {}

It compiles, but only if you have both drop(has_drop) and hash_drop = None. I think the handling of DropAndReplace in ConstraintGeneration is doing something odd (it doesn't go through visit_local).

I'm getting worried that this change is actually unsound in some subtle way. If I can't figure out a simpler way of doing this, then I think we should try to get rid of DropAndReplace entirely, as discussed in https://rust-lang.zulipchat.com/#narrow/stream/233931-t-compiler.2Fmajor-changes/topic/.60DROP.60.20to.20.60DROP_IF.60.20compiler-team.23558.

@Aaron1011
Copy link
Member Author

@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 22, 2022
zeegomo added a commit to zeegomo/rust that referenced this pull request Mar 5, 2023
Desugaring DropAndReplace at MIR build (rust-lang#107844) fixed issue
70919. Add regressions tests, borrowed from rust-lang#102078, to ensure we
check for this in the future.

Co-authored-by: Aaron Hill <aa1ronham@gmail.com>
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 6, 2023
Add regression tests for issue 70919

Desugaring DropAndReplace at MIR build (rust-lang#107844) fixed rust-lang#70919.
Add regressions tests, borrowed from rust-lang#102078, to ensure we check for this in the future.

cc `@Aaron1011`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 6, 2023
Add regression tests for issue 70919

Desugaring DropAndReplace at MIR build (rust-lang#107844) fixed rust-lang#70919.
Add regressions tests, borrowed from rust-lang#102078, to ensure we check for this in the future.

cc ``@Aaron1011``
@bors
Copy link
Contributor

bors commented Mar 9, 2023

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

@Aaron1011 Aaron1011 closed this Mar 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

spurious "borrow might be used .. when [variable] is dropped and runs the destructor"
7 participants