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

Kill conflicting borrows of places with projections. #62010

Merged
merged 6 commits into from
Jun 22, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 25 additions & 30 deletions src/librustc_mir/dataflow/impls/borrows.rs
Original file line number Diff line number Diff line change
Expand Up @@ -193,43 +193,38 @@ impl<'a, 'tcx> Borrows<'a, 'tcx> {
place: &Place<'tcx>
) {
debug!("kill_borrows_on_place: place={:?}", place);
// Handle the `Place::Local(..)` case first and exit early.
if let Place::Base(PlaceBase::Local(local)) = place {
if let Some(borrow_indices) = self.borrow_set.local_map.get(&local) {
debug!("kill_borrows_on_place: borrow_indices={:?}", borrow_indices);
sets.kill_all(borrow_indices);

if let Some(local) = place.base_local() {
let other_borrows_of_local = self
.borrow_set
.local_map
.get(&local)
.into_iter()
.flat_map(|bs| bs.into_iter());

// If the borrowed place is a local with no projections, all other borrows of this
// local must conflict. This is purely an optimization so we don't have to call
// `places_conflict` for every borrow.
if let Place::Base(PlaceBase::Local(_)) = place {
sets.kill_all(other_borrows_of_local);
return;
}
}

// Otherwise, look at all borrows that are live and if they conflict with the assignment
// into our place then we can kill them.
let mut borrows = sets.on_entry.clone();
let _ = borrows.union(sets.gen_set);
for borrow_index in borrows.iter() {
let borrow_data = &self.borrows()[borrow_index];
debug!(
"kill_borrows_on_place: borrow_index={:?} borrow_data={:?}",
borrow_index, borrow_data,
);

// By passing `PlaceConflictBias::NoOverlap`, we conservatively assume that any given
// pair of array indices are unequal, so that when `places_conflict` returns true, we
// will be assured that two places being compared definitely denotes the same sets of
// locations.
if places_conflict::places_conflict(
self.tcx,
self.body,
&borrow_data.borrowed_place,
place,
places_conflict::PlaceConflictBias::NoOverlap,
) {
debug!(
"kill_borrows_on_place: (kill) borrow_index={:?} borrow_data={:?}",
borrow_index, borrow_data,
);
sets.kill(borrow_index);
}
let definitely_conflicting_borrows = other_borrows_of_local
.filter(|&&i| {
places_conflict::places_conflict(
self.tcx,
self.body,
&self.borrow_set.borrows[i].borrowed_place,
place,
places_conflict::PlaceConflictBias::NoOverlap)
});

sets.kill_all(definitely_conflicting_borrows);
}
}
}
Expand Down
27 changes: 27 additions & 0 deletions src/test/run-pass/borrowck/issue-62007-assign-box.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
// run-pass

// Issue #62007: assigning over a deref projection of a box (in this
// case, `*list = n;`) should be able to kill all borrows of `*list`,
// so that `*list` can be borrowed on the next iteration through the
// loop.

#![allow(dead_code)]

struct List<T> {
value: T,
next: Option<Box<List<T>>>,
}

fn to_refs<T>(mut list: Box<&mut List<T>>) -> Vec<&mut T> {
let mut result = vec![];
loop {
result.push(&mut list.value);
if let Some(n) = list.next.as_mut() {
*list = n;
} else {
return result;
}
}
}

fn main() {}
26 changes: 26 additions & 0 deletions src/test/run-pass/borrowck/issue-62007-assign-field.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
// run-pass

// Issue #62007: assigning over a field projection (`list.0 = n;` in
// this case) should be able to kill all borrows of `list.0`, so that
// `list.0` can be borrowed on the next iteration through the loop.

#![allow(dead_code)]

struct List<T> {
value: T,
next: Option<Box<List<T>>>,
}

fn to_refs<T>(mut list: (&mut List<T>,)) -> Vec<&mut T> {
let mut result = vec![];
loop {
result.push(&mut (list.0).value);
if let Some(n) = (list.0).next.as_mut() {
list.0 = n;
} else {
return result;
}
}
}

fn main() {}
32 changes: 32 additions & 0 deletions src/test/ui/nll/issue-62007-assign-const-index.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
// Issue #62007: assigning over a const-index projection of an array
// (in this case, `list[I] = n;`) should in theory be able to kill all borrows
// of `list[0]`, so that `list[0]` could be borrowed on the next
// iteration through the loop.
//
// Currently the compiler does not allow this. We may want to consider
// loosening that restriction in the future. (However, doing so would
// at *least* require T-lang team approval, and probably an RFC; e.g.
// such loosening might make complicate the user's mental mode; it
// also would make code more brittle in the face of refactorings that
// replace constants with variables.

#![allow(dead_code)]

struct List<T> {
value: T,
next: Option<Box<List<T>>>,
}

fn to_refs<T>(mut list: [&mut List<T>; 2]) -> Vec<&mut T> {
let mut result = vec![];
loop {
result.push(&mut list[0].value); //~ ERROR cannot borrow `list[_].value` as mutable
if let Some(n) = list[0].next.as_mut() { //~ ERROR cannot borrow `list[_].next` as mutable
list[0] = n;
} else {
return result;
}
}
}

fn main() {}
27 changes: 27 additions & 0 deletions src/test/ui/nll/issue-62007-assign-const-index.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
error[E0499]: cannot borrow `list[_].value` as mutable more than once at a time
--> $DIR/issue-62007-assign-const-index.rs:23:21
|
LL | fn to_refs<T>(mut list: [&mut List<T>; 2]) -> Vec<&mut T> {
| - let's call the lifetime of this reference `'1`
...
LL | result.push(&mut list[0].value);
| ^^^^^^^^^^^^^^^^^^ mutable borrow starts here in previous iteration of loop
...
LL | return result;
| ------ returning this value requires that `list[_].value` is borrowed for `'1`

error[E0499]: cannot borrow `list[_].next` as mutable more than once at a time
--> $DIR/issue-62007-assign-const-index.rs:24:26
|
LL | fn to_refs<T>(mut list: [&mut List<T>; 2]) -> Vec<&mut T> {
| - let's call the lifetime of this reference `'1`
...
LL | if let Some(n) = list[0].next.as_mut() {
| ^^^^^^^^^^^^---------
| |
| mutable borrow starts here in previous iteration of loop
| argument requires that `list[_].next` is borrowed for `'1`

error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0499`.
25 changes: 25 additions & 0 deletions src/test/ui/nll/issue-62007-assign-differing-fields.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
// Double-check we didn't go too far with our resolution to issue
// #62007: assigning over a field projection (`list.1 = n;` in this
// case) should kill only borrows of `list.1`; `list.0` can *not*
// necessarily be borrowed on the next iteration through the loop.

#![allow(dead_code)]

struct List<T> {
value: T,
next: Option<Box<List<T>>>,
}

fn to_refs<'a, T>(mut list: (&'a mut List<T>, &'a mut List<T>)) -> Vec<&'a mut T> {
let mut result = vec![];
loop {
result.push(&mut (list.0).value); //~ ERROR cannot borrow `list.0.value` as mutable
if let Some(n) = (list.0).next.as_mut() { //~ ERROR cannot borrow `list.0.next` as mutable
list.1 = n;
} else {
return result;
}
}
}

fn main() {}
27 changes: 27 additions & 0 deletions src/test/ui/nll/issue-62007-assign-differing-fields.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
error[E0499]: cannot borrow `list.0.value` as mutable more than once at a time
--> $DIR/issue-62007-assign-differing-fields.rs:16:21
|
LL | fn to_refs<'a, T>(mut list: (&'a mut List<T>, &'a mut List<T>)) -> Vec<&'a mut T> {
| -- lifetime `'a` defined here
...
LL | result.push(&mut (list.0).value);
| ^^^^^^^^^^^^^^^^^^^ mutable borrow starts here in previous iteration of loop
...
LL | return result;
| ------ returning this value requires that `list.0.value` is borrowed for `'a`

error[E0499]: cannot borrow `list.0.next` as mutable more than once at a time
--> $DIR/issue-62007-assign-differing-fields.rs:17:26
|
LL | fn to_refs<'a, T>(mut list: (&'a mut List<T>, &'a mut List<T>)) -> Vec<&'a mut T> {
| -- lifetime `'a` defined here
...
LL | if let Some(n) = (list.0).next.as_mut() {
| ^^^^^^^^^^^^^---------
| |
| mutable borrow starts here in previous iteration of loop
| argument requires that `list.0.next` is borrowed for `'a`

error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0499`.