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

Reenable generator drop tracking tests and fix mutation handling #94460

Merged
merged 4 commits into from
Mar 5, 2022

Conversation

eholk
Copy link
Contributor

@eholk eholk commented Feb 28, 2022

The previous PR, #94068, was overly zealous in counting mutations as borrows, which effectively nullified drop tracking. We would have caught this except the drop tracking tests were still ignored, despite having the option of using the -Zdrop-tracking flag now.

This PR fixes the issue introduced by #94068 by only counting mutations as borrows the mutated place has a project. This is sufficient to distinguish x.y = 42 (which should count as a borrow of x) from x = 42 (which is not a borrow of x because the whole variable is overwritten).

This PR also re-enables the drop tracking regression tests using the -Zdrop-tracking flag so we will avoid introducing these sorts of issues in the future.

Thanks to @tmiasko for noticing this problem and pointing it out!

r? @tmiasko

Because bindings also count as a mutation, the previous behavior counted
all variables as borrowed, completely negating the effect of drop
tracking.
These were still disabled from the soft revert of drop tracking, which
meant we were not catching regressions that were introduced while trying
to fix drop tracking.
@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Feb 28, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 28, 2022
@tmiasko
Copy link
Contributor

tmiasko commented Mar 1, 2022

I though that the assignments introduce borrows because the old value needs to be dropped first. The drop terminators borrows the dropped place and then the state transforms falls back to the conservative analysis based strictly on the storage makers. In that case presence of projections would be mostly insignificant. Is there something more to it? Could you explain it a bit?

To close the gap between typeck and MIR we could do some of the following:

  • Check if type needs to be dropped. The ExprKind::Assign is lowered to TerminatorKind::DropAndReplace, but only if type needs to be dropped. Otherwise it is a plain assignment.
  • Distinguish between mutation in ExprKind::Let from ExprKind::Assign. In the former case there is no existing value that would need to be dropped.
  • Check if place might be initialized before considering it to be borrowed. The drop elaboration removes drops when place is know to be uninitialized (as identified by MaybeInitializedPlaces).

Failing test case where a whole variable is overwritten:

struct A;
impl Drop for A { fn drop(&mut self) {} }

pub async fn f() {
    let mut a = A;
    a = A;
    drop(a);
    async {}.await;
}

This better captures the actual behavior, rather than using hacks around
whether the assignment has any projections.
@eholk
Copy link
Contributor Author

eholk commented Mar 4, 2022

Thanks for the feedback, @tmiasko. That makes a lot more sense that the borrow happens because it's basically doing Drop::drop(&mut x); x = new_value. I just pushed a new patch that solves the problem better by:

  1. Use Ty::needs_drop in the mutate callback, since that's what actually causes the borrow.
  2. Add a bind callback to ExprUseVisitor that by default forwards to mutate to keep the old behavior but allows us to precisely distinguish binding "assignments" from normal assignments.

I left off the third option (ignoring cases where the assignee is known to be uninitialized) for now, although we might be able to figure that case out by looking for assignments to variables that aren't preceded by a bind callback.

@tmiasko
Copy link
Contributor

tmiasko commented Mar 4, 2022

@bors r+ rollup=always

@bors
Copy link
Contributor

bors commented Mar 4, 2022

📌 Commit add169d has been approved by tmiasko

@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 Mar 4, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Mar 5, 2022
… r=tmiasko

Reenable generator drop tracking tests and fix mutation handling

The previous PR, rust-lang#94068, was overly zealous in counting mutations as borrows, which effectively nullified drop tracking. We would have caught this except the drop tracking tests were still ignored, despite having the option of using the `-Zdrop-tracking` flag now.

This PR fixes the issue introduced by rust-lang#94068 by only counting mutations as borrows the mutated place has a project. This is sufficient to distinguish `x.y = 42` (which should count as a borrow of `x`) from `x = 42` (which is not a borrow of `x` because the whole variable is overwritten).

This PR also re-enables the drop tracking regression tests using the `-Zdrop-tracking` flag so we will avoid introducing these sorts of issues in the future.

Thanks to `@tmiasko` for noticing this problem and pointing it out!

r? `@tmiasko`
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 5, 2022
Rollup of 6 pull requests

Successful merges:

 - rust-lang#94446 (UNIX `remove_dir_all()`: Try recursing first on the slow path)
 - rust-lang#94460 (Reenable generator drop tracking tests and fix mutation handling)
 - rust-lang#94620 (Edit docs on consistency of `PartialOrd` and `PartialEq`)
 - rust-lang#94624 (Downgrade `#[test]` on macro call to warning)
 - rust-lang#94626 (Add known-bug directive to issue rust-lang#47511 test case)
 - rust-lang#94631 (Fix typo in c-variadic)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit c7d2004 into rust-lang:master Mar 5, 2022
@rustbot rustbot added this to the 1.61.0 milestone Mar 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

5 participants