Skip to content

Commit ef0e446

Browse files
authored
Rollup merge of rust-lang#62010 - ecstatic-morse:kill-borrows-of-proj, r=pnkfelix
Kill conflicting borrows of places with projections. Resolves rust-lang#62007. Due to a bug, the previous version of this check did not actually kill all conflicting borrows unless the borrowed place had no projections. Specifically, `sets.on_entry` will always be empty when `statement_effect` is called. It does not contain the set of borrows which are live at this point in the program. @pnkfelix describes why this was not caught before in rust-lang#62007, and created an example where the current borrow checker failed unnecessarily. This PR adds their example as a test, but they will likely want to add some additional ones. r? @pnkfelix
2 parents 3945174 + f483269 commit ef0e446

7 files changed

+189
-30
lines changed

src/librustc_mir/dataflow/impls/borrows.rs

+25-30
Original file line numberDiff line numberDiff line change
@@ -193,43 +193,38 @@ impl<'a, 'tcx> Borrows<'a, 'tcx> {
193193
place: &Place<'tcx>
194194
) {
195195
debug!("kill_borrows_on_place: place={:?}", place);
196-
// Handle the `Place::Local(..)` case first and exit early.
197-
if let Place::Base(PlaceBase::Local(local)) = place {
198-
if let Some(borrow_indices) = self.borrow_set.local_map.get(&local) {
199-
debug!("kill_borrows_on_place: borrow_indices={:?}", borrow_indices);
200-
sets.kill_all(borrow_indices);
196+
197+
if let Some(local) = place.base_local() {
198+
let other_borrows_of_local = self
199+
.borrow_set
200+
.local_map
201+
.get(&local)
202+
.into_iter()
203+
.flat_map(|bs| bs.into_iter());
204+
205+
// If the borrowed place is a local with no projections, all other borrows of this
206+
// local must conflict. This is purely an optimization so we don't have to call
207+
// `places_conflict` for every borrow.
208+
if let Place::Base(PlaceBase::Local(_)) = place {
209+
sets.kill_all(other_borrows_of_local);
201210
return;
202211
}
203-
}
204-
205-
// Otherwise, look at all borrows that are live and if they conflict with the assignment
206-
// into our place then we can kill them.
207-
let mut borrows = sets.on_entry.clone();
208-
let _ = borrows.union(sets.gen_set);
209-
for borrow_index in borrows.iter() {
210-
let borrow_data = &self.borrows()[borrow_index];
211-
debug!(
212-
"kill_borrows_on_place: borrow_index={:?} borrow_data={:?}",
213-
borrow_index, borrow_data,
214-
);
215212

216213
// By passing `PlaceConflictBias::NoOverlap`, we conservatively assume that any given
217214
// pair of array indices are unequal, so that when `places_conflict` returns true, we
218215
// will be assured that two places being compared definitely denotes the same sets of
219216
// locations.
220-
if places_conflict::places_conflict(
221-
self.tcx,
222-
self.body,
223-
&borrow_data.borrowed_place,
224-
place,
225-
places_conflict::PlaceConflictBias::NoOverlap,
226-
) {
227-
debug!(
228-
"kill_borrows_on_place: (kill) borrow_index={:?} borrow_data={:?}",
229-
borrow_index, borrow_data,
230-
);
231-
sets.kill(borrow_index);
232-
}
217+
let definitely_conflicting_borrows = other_borrows_of_local
218+
.filter(|&&i| {
219+
places_conflict::places_conflict(
220+
self.tcx,
221+
self.body,
222+
&self.borrow_set.borrows[i].borrowed_place,
223+
place,
224+
places_conflict::PlaceConflictBias::NoOverlap)
225+
});
226+
227+
sets.kill_all(definitely_conflicting_borrows);
233228
}
234229
}
235230
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
// run-pass
2+
3+
// Issue #62007: assigning over a deref projection of a box (in this
4+
// case, `*list = n;`) should be able to kill all borrows of `*list`,
5+
// so that `*list` can be borrowed on the next iteration through the
6+
// loop.
7+
8+
#![allow(dead_code)]
9+
10+
struct List<T> {
11+
value: T,
12+
next: Option<Box<List<T>>>,
13+
}
14+
15+
fn to_refs<T>(mut list: Box<&mut List<T>>) -> Vec<&mut T> {
16+
let mut result = vec![];
17+
loop {
18+
result.push(&mut list.value);
19+
if let Some(n) = list.next.as_mut() {
20+
*list = n;
21+
} else {
22+
return result;
23+
}
24+
}
25+
}
26+
27+
fn main() {}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
// run-pass
2+
3+
// Issue #62007: assigning over a field projection (`list.0 = n;` in
4+
// this case) should be able to kill all borrows of `list.0`, so that
5+
// `list.0` can be borrowed on the next iteration through the loop.
6+
7+
#![allow(dead_code)]
8+
9+
struct List<T> {
10+
value: T,
11+
next: Option<Box<List<T>>>,
12+
}
13+
14+
fn to_refs<T>(mut list: (&mut List<T>,)) -> Vec<&mut T> {
15+
let mut result = vec![];
16+
loop {
17+
result.push(&mut (list.0).value);
18+
if let Some(n) = (list.0).next.as_mut() {
19+
list.0 = n;
20+
} else {
21+
return result;
22+
}
23+
}
24+
}
25+
26+
fn main() {}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
// Issue #62007: assigning over a const-index projection of an array
2+
// (in this case, `list[I] = n;`) should in theory be able to kill all borrows
3+
// of `list[0]`, so that `list[0]` could be borrowed on the next
4+
// iteration through the loop.
5+
//
6+
// Currently the compiler does not allow this. We may want to consider
7+
// loosening that restriction in the future. (However, doing so would
8+
// at *least* require T-lang team approval, and probably an RFC; e.g.
9+
// such loosening might make complicate the user's mental mode; it
10+
// also would make code more brittle in the face of refactorings that
11+
// replace constants with variables.
12+
13+
#![allow(dead_code)]
14+
15+
struct List<T> {
16+
value: T,
17+
next: Option<Box<List<T>>>,
18+
}
19+
20+
fn to_refs<T>(mut list: [&mut List<T>; 2]) -> Vec<&mut T> {
21+
let mut result = vec![];
22+
loop {
23+
result.push(&mut list[0].value); //~ ERROR cannot borrow `list[_].value` as mutable
24+
if let Some(n) = list[0].next.as_mut() { //~ ERROR cannot borrow `list[_].next` as mutable
25+
list[0] = n;
26+
} else {
27+
return result;
28+
}
29+
}
30+
}
31+
32+
fn main() {}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
error[E0499]: cannot borrow `list[_].value` as mutable more than once at a time
2+
--> $DIR/issue-62007-assign-const-index.rs:23:21
3+
|
4+
LL | fn to_refs<T>(mut list: [&mut List<T>; 2]) -> Vec<&mut T> {
5+
| - let's call the lifetime of this reference `'1`
6+
...
7+
LL | result.push(&mut list[0].value);
8+
| ^^^^^^^^^^^^^^^^^^ mutable borrow starts here in previous iteration of loop
9+
...
10+
LL | return result;
11+
| ------ returning this value requires that `list[_].value` is borrowed for `'1`
12+
13+
error[E0499]: cannot borrow `list[_].next` as mutable more than once at a time
14+
--> $DIR/issue-62007-assign-const-index.rs:24:26
15+
|
16+
LL | fn to_refs<T>(mut list: [&mut List<T>; 2]) -> Vec<&mut T> {
17+
| - let's call the lifetime of this reference `'1`
18+
...
19+
LL | if let Some(n) = list[0].next.as_mut() {
20+
| ^^^^^^^^^^^^---------
21+
| |
22+
| mutable borrow starts here in previous iteration of loop
23+
| argument requires that `list[_].next` is borrowed for `'1`
24+
25+
error: aborting due to 2 previous errors
26+
27+
For more information about this error, try `rustc --explain E0499`.
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
// Double-check we didn't go too far with our resolution to issue
2+
// #62007: assigning over a field projection (`list.1 = n;` in this
3+
// case) should kill only borrows of `list.1`; `list.0` can *not*
4+
// necessarily be borrowed on the next iteration through the loop.
5+
6+
#![allow(dead_code)]
7+
8+
struct List<T> {
9+
value: T,
10+
next: Option<Box<List<T>>>,
11+
}
12+
13+
fn to_refs<'a, T>(mut list: (&'a mut List<T>, &'a mut List<T>)) -> Vec<&'a mut T> {
14+
let mut result = vec![];
15+
loop {
16+
result.push(&mut (list.0).value); //~ ERROR cannot borrow `list.0.value` as mutable
17+
if let Some(n) = (list.0).next.as_mut() { //~ ERROR cannot borrow `list.0.next` as mutable
18+
list.1 = n;
19+
} else {
20+
return result;
21+
}
22+
}
23+
}
24+
25+
fn main() {}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
error[E0499]: cannot borrow `list.0.value` as mutable more than once at a time
2+
--> $DIR/issue-62007-assign-differing-fields.rs:16:21
3+
|
4+
LL | fn to_refs<'a, T>(mut list: (&'a mut List<T>, &'a mut List<T>)) -> Vec<&'a mut T> {
5+
| -- lifetime `'a` defined here
6+
...
7+
LL | result.push(&mut (list.0).value);
8+
| ^^^^^^^^^^^^^^^^^^^ mutable borrow starts here in previous iteration of loop
9+
...
10+
LL | return result;
11+
| ------ returning this value requires that `list.0.value` is borrowed for `'a`
12+
13+
error[E0499]: cannot borrow `list.0.next` as mutable more than once at a time
14+
--> $DIR/issue-62007-assign-differing-fields.rs:17:26
15+
|
16+
LL | fn to_refs<'a, T>(mut list: (&'a mut List<T>, &'a mut List<T>)) -> Vec<&'a mut T> {
17+
| -- lifetime `'a` defined here
18+
...
19+
LL | if let Some(n) = (list.0).next.as_mut() {
20+
| ^^^^^^^^^^^^^---------
21+
| |
22+
| mutable borrow starts here in previous iteration of loop
23+
| argument requires that `list.0.next` is borrowed for `'a`
24+
25+
error: aborting due to 2 previous errors
26+
27+
For more information about this error, try `rustc --explain E0499`.

0 commit comments

Comments
 (0)