Skip to content

Commit bf9fee8

Browse files
Dylan-DPCDylan-DPC
authored
Rollup merge of rust-lang#94460 - eholk:reenable-drop-tracking-tests, 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`
2 parents 478851c + add169d commit bf9fee8

File tree

10 files changed

+75
-52
lines changed

10 files changed

+75
-52
lines changed

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

Lines changed: 24 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -6,14 +6,14 @@ use crate::{
66
use hir::{def_id::DefId, Body, HirId, HirIdMap};
77
use rustc_data_structures::stable_set::FxHashSet;
88
use rustc_hir as hir;
9-
use rustc_middle::hir::map::Map;
9+
use rustc_middle::ty::{ParamEnv, TyCtxt};
1010

1111
pub(super) fn find_consumed_and_borrowed<'a, 'tcx>(
1212
fcx: &'a FnCtxt<'a, 'tcx>,
1313
def_id: DefId,
1414
body: &'tcx Body<'tcx>,
1515
) -> ConsumedAndBorrowedPlaces {
16-
let mut expr_use_visitor = ExprUseDelegate::new(fcx.tcx.hir());
16+
let mut expr_use_visitor = ExprUseDelegate::new(fcx.tcx, fcx.param_env);
1717
expr_use_visitor.consume_body(fcx, def_id, body);
1818
expr_use_visitor.places
1919
}
@@ -36,14 +36,16 @@ pub(super) struct ConsumedAndBorrowedPlaces {
3636
/// Interesting values are those that are either dropped or borrowed. For dropped values, we also
3737
/// record the parent expression, which is the point where the drop actually takes place.
3838
struct ExprUseDelegate<'tcx> {
39-
hir: Map<'tcx>,
39+
tcx: TyCtxt<'tcx>,
40+
param_env: ParamEnv<'tcx>,
4041
places: ConsumedAndBorrowedPlaces,
4142
}
4243

4344
impl<'tcx> ExprUseDelegate<'tcx> {
44-
fn new(hir: Map<'tcx>) -> Self {
45+
fn new(tcx: TyCtxt<'tcx>, param_env: ParamEnv<'tcx>) -> Self {
4546
Self {
46-
hir,
47+
tcx,
48+
param_env,
4749
places: ConsumedAndBorrowedPlaces {
4850
consumed: <_>::default(),
4951
borrowed: <_>::default(),
@@ -77,7 +79,7 @@ impl<'tcx> expr_use_visitor::Delegate<'tcx> for ExprUseDelegate<'tcx> {
7779
place_with_id: &expr_use_visitor::PlaceWithHirId<'tcx>,
7880
diag_expr_id: HirId,
7981
) {
80-
let parent = match self.hir.find_parent_node(place_with_id.hir_id) {
82+
let parent = match self.tcx.hir().find_parent_node(place_with_id.hir_id) {
8183
Some(parent) => parent,
8284
None => place_with_id.hir_id,
8385
};
@@ -107,11 +109,22 @@ impl<'tcx> expr_use_visitor::Delegate<'tcx> for ExprUseDelegate<'tcx> {
107109
assignee_place: &expr_use_visitor::PlaceWithHirId<'tcx>,
108110
diag_expr_id: HirId,
109111
) {
110-
debug!("mutate {:?}; diag_expr_id={:?}", assignee_place, diag_expr_id);
111-
// Count mutations as a borrow.
112-
self.places
113-
.borrowed
114-
.insert(TrackedValue::from_place_with_projections_allowed(assignee_place));
112+
debug!("mutate {assignee_place:?}; diag_expr_id={diag_expr_id:?}");
113+
// If the type being assigned needs dropped, then the mutation counts as a borrow
114+
// since it is essentially doing `Drop::drop(&mut x); x = new_value;`.
115+
if assignee_place.place.base_ty.needs_drop(self.tcx, self.param_env) {
116+
self.places
117+
.borrowed
118+
.insert(TrackedValue::from_place_with_projections_allowed(assignee_place));
119+
}
120+
}
121+
122+
fn bind(
123+
&mut self,
124+
binding_place: &expr_use_visitor::PlaceWithHirId<'tcx>,
125+
diag_expr_id: HirId,
126+
) {
127+
debug!("bind {binding_place:?}; diag_expr_id={diag_expr_id:?}");
115128
}
116129

117130
fn fake_read(

‎compiler/rustc_typeck/src/expr_use_visitor.rs‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,15 @@ pub trait Delegate<'tcx> {
5151
/// `diag_expr_id` is the id used for diagnostics (see `consume` for more details).
5252
fn mutate(&mut self, assignee_place: &PlaceWithHirId<'tcx>, diag_expr_id: hir::HirId);
5353

54+
/// The path at `binding_place` is a binding that is being initialized.
55+
///
56+
/// This covers cases such as `let x = 42;`
57+
fn bind(&mut self, binding_place: &PlaceWithHirId<'tcx>, diag_expr_id: hir::HirId) {
58+
// Bindings can normally be treated as a regular assignment, so by default we
59+
// forward this to the mutate callback.
60+
self.mutate(binding_place, diag_expr_id)
61+
}
62+
5463
/// The `place` should be a fake read because of specified `cause`.
5564
fn fake_read(&mut self, place: Place<'tcx>, cause: FakeReadCause, diag_expr_id: hir::HirId);
5665
}
@@ -648,11 +657,9 @@ impl<'a, 'tcx> ExprUseVisitor<'a, 'tcx> {
648657
let pat_ty = return_if_err!(mc.node_ty(pat.hir_id));
649658
debug!("walk_pat: pat_ty={:?}", pat_ty);
650659

651-
// Each match binding is effectively an assignment to the
652-
// binding being produced.
653660
let def = Res::Local(canonical_id);
654661
if let Ok(ref binding_place) = mc.cat_res(pat.hir_id, pat.span, pat_ty, def) {
655-
delegate.mutate(binding_place, binding_place.hir_id);
662+
delegate.bind(binding_place, binding_place.hir_id);
656663
}
657664

658665
// It is also a borrow or copy/move of the value being matched.

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

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,5 @@
11
// edition:2018
2-
// compile-flags: --crate-type lib
3-
4-
// FIXME(eholk): temporarily disabled while drop range tracking is disabled
5-
// (see generator_interior.rs:27)
6-
// ignore-test
2+
// compile-flags: --crate-type lib -Zdrop-tracking
73

84
use std::{cell::RefCell, fmt::Debug, rc::Rc};
95

‎src/test/ui/async-await/drop-and-assign.rs‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
// edition:2021
2+
// compile-flags: -Zdrop-tracking
3+
// build-pass
4+
5+
struct A;
6+
impl Drop for A { fn drop(&mut self) {} }
7+
8+
pub async fn f() {
9+
let mut a = A;
10+
a = A;
11+
drop(a);
12+
async {}.await;
13+
}
14+
15+
fn assert_send<T: Send>(_: T) {}
16+
17+
fn main() {
18+
let _ = f();
19+
}

‎src/test/ui/async-await/unresolved_type_param.rs‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,10 +2,7 @@
22
// Error message should pinpoint the type parameter T as needing to be bound
33
// (rather than give a general error message)
44
// edition:2018
5-
6-
// FIXME(eholk): temporarily disabled while drop range tracking is disabled
7-
// (see generator_interior.rs:27)
8-
// ignore-test
5+
// compile-flags: -Zdrop-tracking
96

107
async fn bar<T>() -> () {}
118

‎src/test/ui/async-await/unresolved_type_param.stderr‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,35 +1,35 @@
11
error[E0698]: type inside `async fn` body must be known in this context
2-
--> $DIR/unresolved_type_param.rs:9:5
2+
--> $DIR/unresolved_type_param.rs:10:5
33
|
44
LL | bar().await;
55
| ^^^ cannot infer type for type parameter `T` declared on the function `bar`
66
|
77
note: the type is part of the `async fn` body because of this `await`
8-
--> $DIR/unresolved_type_param.rs:9:10
8+
--> $DIR/unresolved_type_param.rs:10:10
99
|
1010
LL | bar().await;
1111
| ^^^^^^
1212

1313
error[E0698]: type inside `async fn` body must be known in this context
14-
--> $DIR/unresolved_type_param.rs:9:5
14+
--> $DIR/unresolved_type_param.rs:10:5
1515
|
1616
LL | bar().await;
1717
| ^^^ cannot infer type for type parameter `T` declared on the function `bar`
1818
|
1919
note: the type is part of the `async fn` body because of this `await`
20-
--> $DIR/unresolved_type_param.rs:9:10
20+
--> $DIR/unresolved_type_param.rs:10:10
2121
|
2222
LL | bar().await;
2323
| ^^^^^^
2424

2525
error[E0698]: type inside `async fn` body must be known in this context
26-
--> $DIR/unresolved_type_param.rs:9:5
26+
--> $DIR/unresolved_type_param.rs:10:5
2727
|
2828
LL | bar().await;
2929
| ^^^ cannot infer type for type parameter `T` declared on the function `bar`
3030
|
3131
note: the type is part of the `async fn` body because of this `await`
32-
--> $DIR/unresolved_type_param.rs:9:10
32+
--> $DIR/unresolved_type_param.rs:10:10
3333
|
3434
LL | bar().await;
3535
| ^^^^^^

‎src/test/ui/generator/drop-control-flow.rs‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,6 @@
11
// build-pass
22
// compile-flags: -Zdrop-tracking
33

4-
// FIXME(eholk): temporarily disabled while drop range tracking is disabled
5-
// (see generator_interior.rs:27)
6-
// ignore-test
7-
84
// A test to ensure generators capture values that were conditionally dropped,
95
// and also that values that are dropped along all paths to a yield do not get
106
// included in the generator type.

‎src/test/ui/generator/issue-57478.rs‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,5 @@
11
// check-pass
2-
3-
// FIXME(eholk): temporarily disabled while drop range tracking is disabled
4-
// (see generator_interior.rs:27)
5-
// ignore-test
2+
// compile-flags: -Zdrop-tracking
63

74
#![feature(negative_impls, generators)]
85

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

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,4 @@
1-
// FIXME(eholk): temporarily disabled while drop range tracking is disabled
2-
// (see generator_interior.rs:27)
3-
// ignore-test
1+
// compile-flags: -Zdrop-tracking
42

53
#![feature(negative_impls, generators)]
64

‎src/test/ui/generator/partial-drop.stderr‎

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,12 @@
11
error: generator cannot be sent between threads safely
2-
--> $DIR/partial-drop.rs:12:5
2+
--> $DIR/partial-drop.rs:14:5
33
|
44
LL | assert_send(|| {
55
| ^^^^^^^^^^^ generator is not `Send`
66
|
7-
= help: within `[generator@$DIR/partial-drop.rs:12:17: 18:6]`, the trait `Send` is not implemented for `Foo`
7+
= help: within `[generator@$DIR/partial-drop.rs:14:17: 20:6]`, the trait `Send` is not implemented for `Foo`
88
note: generator is not `Send` as this value is used across a yield
9-
--> $DIR/partial-drop.rs:17:9
9+
--> $DIR/partial-drop.rs:19:9
1010
|
1111
LL | let guard = Bar { foo: Foo, x: 42 };
1212
| ----- has type `Bar` which is not `Send`
@@ -16,20 +16,20 @@ LL | yield;
1616
LL | });
1717
| - `guard` is later dropped here
1818
note: required by a bound in `assert_send`
19-
--> $DIR/partial-drop.rs:40:19
19+
--> $DIR/partial-drop.rs:42:19
2020
|
2121
LL | fn assert_send<T: Send>(_: T) {}
2222
| ^^^^ required by this bound in `assert_send`
2323

2424
error: generator cannot be sent between threads safely
25-
--> $DIR/partial-drop.rs:20:5
25+
--> $DIR/partial-drop.rs:22:5
2626
|
2727
LL | assert_send(|| {
2828
| ^^^^^^^^^^^ generator is not `Send`
2929
|
30-
= help: within `[generator@$DIR/partial-drop.rs:20:17: 28:6]`, the trait `Send` is not implemented for `Foo`
30+
= help: within `[generator@$DIR/partial-drop.rs:22:17: 30:6]`, the trait `Send` is not implemented for `Foo`
3131
note: generator is not `Send` as this value is used across a yield
32-
--> $DIR/partial-drop.rs:27:9
32+
--> $DIR/partial-drop.rs:29:9
3333
|
3434
LL | let guard = Bar { foo: Foo, x: 42 };
3535
| ----- has type `Bar` which is not `Send`
@@ -39,20 +39,20 @@ LL | yield;
3939
LL | });
4040
| - `guard` is later dropped here
4141
note: required by a bound in `assert_send`
42-
--> $DIR/partial-drop.rs:40:19
42+
--> $DIR/partial-drop.rs:42:19
4343
|
4444
LL | fn assert_send<T: Send>(_: T) {}
4545
| ^^^^ required by this bound in `assert_send`
4646

4747
error: generator cannot be sent between threads safely
48-
--> $DIR/partial-drop.rs:30:5
48+
--> $DIR/partial-drop.rs:32:5
4949
|
5050
LL | assert_send(|| {
5151
| ^^^^^^^^^^^ generator is not `Send`
5252
|
53-
= help: within `[generator@$DIR/partial-drop.rs:30:17: 37:6]`, the trait `Send` is not implemented for `Foo`
53+
= help: within `[generator@$DIR/partial-drop.rs:32:17: 39:6]`, the trait `Send` is not implemented for `Foo`
5454
note: generator is not `Send` as this value is used across a yield
55-
--> $DIR/partial-drop.rs:36:9
55+
--> $DIR/partial-drop.rs:38:9
5656
|
5757
LL | let guard = Bar { foo: Foo, x: 42 };
5858
| ----- has type `Bar` which is not `Send`
@@ -62,7 +62,7 @@ LL | yield;
6262
LL | });
6363
| - `guard` is later dropped here
6464
note: required by a bound in `assert_send`
65-
--> $DIR/partial-drop.rs:40:19
65+
--> $DIR/partial-drop.rs:42:19
6666
|
6767
LL | fn assert_send<T: Send>(_: T) {}
6868
| ^^^^ required by this bound in `assert_send`

0 commit comments

Comments
 (0)