From f045008f9430b238491eda3f218361190b5083a0 Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Thu, 20 Jun 2019 15:31:58 -0700 Subject: [PATCH 1/6] Kill conflicting borrows of places with projections. Resolves #62007. Due to a bug, the previous version of this check did not actually kill any conflicting borrows unless the borrowed place had no projections. Specifically, `entry_set` 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. --- src/librustc_mir/dataflow/impls/borrows.rs | 55 +++++++++---------- ...jection-kills-other-borrows-issue-62007.rs | 21 +++++++ 2 files changed, 46 insertions(+), 30 deletions(-) create mode 100644 src/test/run-pass/borrowck/borrowck-borrow-of-projection-kills-other-borrows-issue-62007.rs diff --git a/src/librustc_mir/dataflow/impls/borrows.rs b/src/librustc_mir/dataflow/impls/borrows.rs index 899765a1d2daa..7617d3b997d08 100644 --- a/src/librustc_mir/dataflow/impls/borrows.rs +++ b/src/librustc_mir/dataflow/impls/borrows.rs @@ -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); } } } diff --git a/src/test/run-pass/borrowck/borrowck-borrow-of-projection-kills-other-borrows-issue-62007.rs b/src/test/run-pass/borrowck/borrowck-borrow-of-projection-kills-other-borrows-issue-62007.rs new file mode 100644 index 0000000000000..2ab0e6cf35520 --- /dev/null +++ b/src/test/run-pass/borrowck/borrowck-borrow-of-projection-kills-other-borrows-issue-62007.rs @@ -0,0 +1,21 @@ +// run-pass +#![allow(dead_code)] + +struct List { + value: T, + next: Option>>, +} + +fn to_refs(mut list: (&mut List,)) -> 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() {} From e78361a16c3907060bbc6c9b3841e905ac74ab7d Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Fri, 21 Jun 2019 12:39:11 +0200 Subject: [PATCH 2/6] rename file because I found the old filename too long and unwieldy. --- ...s-other-borrows-issue-62007.rs => issue-62007-assign-field.rs} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename src/test/run-pass/borrowck/{borrowck-borrow-of-projection-kills-other-borrows-issue-62007.rs => issue-62007-assign-field.rs} (100%) diff --git a/src/test/run-pass/borrowck/borrowck-borrow-of-projection-kills-other-borrows-issue-62007.rs b/src/test/run-pass/borrowck/issue-62007-assign-field.rs similarity index 100% rename from src/test/run-pass/borrowck/borrowck-borrow-of-projection-kills-other-borrows-issue-62007.rs rename to src/test/run-pass/borrowck/issue-62007-assign-field.rs From 681af62ba42d020712234c3c5d6198f4333dd2fd Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Fri, 21 Jun 2019 12:38:12 +0200 Subject: [PATCH 3/6] add comment outlining test. --- src/test/run-pass/borrowck/issue-62007-assign-field.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/test/run-pass/borrowck/issue-62007-assign-field.rs b/src/test/run-pass/borrowck/issue-62007-assign-field.rs index 2ab0e6cf35520..5b21c083816a4 100644 --- a/src/test/run-pass/borrowck/issue-62007-assign-field.rs +++ b/src/test/run-pass/borrowck/issue-62007-assign-field.rs @@ -1,4 +1,9 @@ // 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 { From 241ceeda046b23651aec95a6bd59cca7aaa69ecc Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Fri, 21 Jun 2019 12:43:04 +0200 Subject: [PATCH 4/6] Added test for deref projection. --- .../borrowck/issue-62007-assign-box.rs | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) create mode 100644 src/test/run-pass/borrowck/issue-62007-assign-box.rs diff --git a/src/test/run-pass/borrowck/issue-62007-assign-box.rs b/src/test/run-pass/borrowck/issue-62007-assign-box.rs new file mode 100644 index 0000000000000..f6fbea821b521 --- /dev/null +++ b/src/test/run-pass/borrowck/issue-62007-assign-box.rs @@ -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 { + value: T, + next: Option>>, +} + +fn to_refs(mut list: Box<&mut List>) -> 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() {} From 3b8ef010309f732d4ab04e63ba454383b69db2de Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Fri, 21 Jun 2019 12:50:18 +0200 Subject: [PATCH 5/6] Add test that our handling of projections hasn't gone too far: overwriting one field should not allow reborrow of an unrelated field. --- .../issue-62007-assign-differing-fields.rs | 25 +++++++++++++++++ ...issue-62007-assign-differing-fields.stderr | 27 +++++++++++++++++++ 2 files changed, 52 insertions(+) create mode 100644 src/test/ui/nll/issue-62007-assign-differing-fields.rs create mode 100644 src/test/ui/nll/issue-62007-assign-differing-fields.stderr diff --git a/src/test/ui/nll/issue-62007-assign-differing-fields.rs b/src/test/ui/nll/issue-62007-assign-differing-fields.rs new file mode 100644 index 0000000000000..29d92b7b85c12 --- /dev/null +++ b/src/test/ui/nll/issue-62007-assign-differing-fields.rs @@ -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 { + value: T, + next: Option>>, +} + +fn to_refs<'a, T>(mut list: (&'a mut List, &'a mut List)) -> 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() {} diff --git a/src/test/ui/nll/issue-62007-assign-differing-fields.stderr b/src/test/ui/nll/issue-62007-assign-differing-fields.stderr new file mode 100644 index 0000000000000..f942d7628b507 --- /dev/null +++ b/src/test/ui/nll/issue-62007-assign-differing-fields.stderr @@ -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, &'a mut List)) -> 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, &'a mut List)) -> 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`. From f483269625f4f8f0f73bb3dc35986894fc51248a Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Fri, 21 Jun 2019 13:08:15 +0200 Subject: [PATCH 6/6] Add test checking our behavior for assigning over a `ConstIndex` projection. --- .../ui/nll/issue-62007-assign-const-index.rs | 32 +++++++++++++++++++ .../nll/issue-62007-assign-const-index.stderr | 27 ++++++++++++++++ 2 files changed, 59 insertions(+) create mode 100644 src/test/ui/nll/issue-62007-assign-const-index.rs create mode 100644 src/test/ui/nll/issue-62007-assign-const-index.stderr diff --git a/src/test/ui/nll/issue-62007-assign-const-index.rs b/src/test/ui/nll/issue-62007-assign-const-index.rs new file mode 100644 index 0000000000000..3ea5d3a7ad007 --- /dev/null +++ b/src/test/ui/nll/issue-62007-assign-const-index.rs @@ -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 { + value: T, + next: Option>>, +} + +fn to_refs(mut list: [&mut List; 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() {} diff --git a/src/test/ui/nll/issue-62007-assign-const-index.stderr b/src/test/ui/nll/issue-62007-assign-const-index.stderr new file mode 100644 index 0000000000000..758a14d017705 --- /dev/null +++ b/src/test/ui/nll/issue-62007-assign-const-index.stderr @@ -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(mut list: [&mut List; 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(mut list: [&mut List; 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`.