Auto 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

Author: bors

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);
         }
     }
 }

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<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() {}

src/test/run-pass/borrowck/issue-62007-assign-field.rs

@@ -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() {}

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<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() {}

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<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`.

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<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() {}

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<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`.