Rework fake borrow calculation

Author: Nadrieril

compiler/rustc_middle/src/mir/statement.rs

@@ -236,6 +236,11 @@ impl<'tcx> PlaceRef<'tcx> {
         }
     }
 
+    #[inline]
+    pub fn to_place(&self, tcx: TyCtxt<'tcx>) -> Place<'tcx> {
+        Place { local: self.local, projection: tcx.mk_place_elems(self.projection) }
+    }
+
     #[inline]
     pub fn last_projection(&self) -> Option<(PlaceRef<'tcx>, PlaceElem<'tcx>)> {
         if let &[ref proj_base @ .., elem] = self.projection {

compiler/rustc_mir_build/src/build/matches/mod.rs

@@ -5,15 +5,12 @@
 //! This also includes code for pattern bindings in `let` statements and
 //! function parameters.
 
-use crate::build::expr::as_place::{PlaceBase, PlaceBuilder};
+use crate::build::expr::as_place::PlaceBuilder;
 use crate::build::scope::DropKind;
 use crate::build::ForGuard::{self, OutsideGuard, RefWithinGuard};
 use crate::build::{BlockAnd, BlockAndExtension, Builder};
 use crate::build::{GuardFrame, GuardFrameLocal, LocalsForNode};
-use rustc_data_structures::{
-    fx::{FxHashSet, FxIndexMap, FxIndexSet},
-    stack::ensure_sufficient_stack,
-};
+use rustc_data_structures::{fx::FxIndexMap, stack::ensure_sufficient_stack};
 use rustc_hir::{BindingMode, ByRef};
 use rustc_middle::middle::region;
 use rustc_middle::mir::{self, *};
@@ -209,7 +206,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
     /// 2. Create the decision tree ([Builder::lower_match_tree]).
     /// 3. Determine the fake borrows that are needed from the places that were
     ///    matched against and create the required temporaries for them
-    ///    ([Builder::calculate_fake_borrows]).
+    ///    ([util::collect_fake_borrows]).
     /// 4. Create everything else: the guards and the arms ([Builder::lower_match_arms]).
     ///
     /// ## False edges
@@ -379,11 +376,18 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
         match_has_guard: bool,
         candidates: &mut [&mut Candidate<'pat, 'tcx>],
     ) -> Vec<(Place<'tcx>, Local)> {
-        // The set of places that we are creating fake borrows of. If there are
-        // no match guards then we don't need any fake borrows, so don't track
-        // them.
-        let fake_borrows = match_has_guard
-            .then(|| util::FakeBorrowCollector::collect_fake_borrows(self, candidates));
+        // The set of places that we are creating fake borrows of. If there are no match guards then
+        // we don't need any fake borrows, so don't track them.
+        let fake_borrows: Vec<(Place<'tcx>, Local)> = if match_has_guard {
+            util::collect_fake_borrows(
+                self,
+                candidates,
+                scrutinee_span,
+                scrutinee_place_builder.base(),
+            )
+        } else {
+            Vec::new()
+        };
 
         // See the doc comment on `match_candidates` for why we have an
         // otherwise block. Match checking will ensure this is actually
@@ -437,11 +441,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
             });
         }
 
-        if let Some(ref borrows) = fake_borrows {
-            self.calculate_fake_borrows(borrows, scrutinee_place_builder.base(), scrutinee_span)
-        } else {
-            Vec::new()
-        }
+        fake_borrows
     }
 
     /// Lower the bindings, guards and arm bodies of a `match` expression.
@@ -1911,91 +1911,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
             target_blocks,
         );
     }
-
-    /// Determine the fake borrows that are needed from a set of places that
-    /// have to be stable across match guards.
-    ///
-    /// Returns a list of places that need a fake borrow and the temporary
-    /// that's used to store the fake borrow.
-    ///
-    /// Match exhaustiveness checking is not able to handle the case where the
-    /// place being matched on is mutated in the guards. We add "fake borrows"
-    /// to the guards that prevent any mutation of the place being matched.
-    /// There are a some subtleties:
-    ///
-    /// 1. Borrowing `*x` doesn't prevent assigning to `x`. If `x` is a shared
-    ///    reference, the borrow isn't even tracked. As such we have to add fake
-    ///    borrows of any prefixes of a place
-    /// 2. We don't want `match x { _ => (), }` to conflict with mutable
-    ///    borrows of `x`, so we only add fake borrows for places which are
-    ///    bound or tested by the match.
-    /// 3. We don't want the fake borrows to conflict with `ref mut` bindings,
-    ///    so we use a special BorrowKind for them.
-    /// 4. The fake borrows may be of places in inactive variants, so it would
-    ///    be UB to generate code for them. They therefore have to be removed
-    ///    by a MIR pass run after borrow checking.
-    fn calculate_fake_borrows<'b>(
-        &mut self,
-        fake_borrows: &'b FxIndexSet<Place<'tcx>>,
-        scrutinee_base: PlaceBase,
-        temp_span: Span,
-    ) -> Vec<(Place<'tcx>, Local)> {
-        let tcx = self.tcx;
-
-        debug!("add_fake_borrows fake_borrows = {:?}", fake_borrows);
-
-        let mut all_fake_borrows = Vec::with_capacity(fake_borrows.len());
-
-        // Insert a Shallow borrow of the prefixes of any fake borrows.
-        for place in fake_borrows {
-            if let PlaceBase::Local(l) = scrutinee_base
-                && l != place.local
-            {
-                // The base of this place is a temporary created for deref patterns. We don't emit
-                // fake borrows for these as they are not initialized in all branches.
-                // FIXME(deref_patterns): is this sound?
-                continue;
-            }
-
-            let mut cursor = place.projection.as_ref();
-            while let [proj_base @ .., elem] = cursor {
-                cursor = proj_base;
-
-                if let ProjectionElem::Deref = elem {
-                    // Insert a shallow borrow after a deref. For other
-                    // projections the borrow of prefix_cursor will
-                    // conflict with any mutation of base.
-                    all_fake_borrows.push(PlaceRef { local: place.local, projection: proj_base });
-                }
-            }
-
-            all_fake_borrows.push(place.as_ref());
-        }
-
-        // Deduplicate
-        let mut dedup = FxHashSet::default();
-        all_fake_borrows.retain(|b| dedup.insert(*b));
-
-        debug!("add_fake_borrows all_fake_borrows = {:?}", all_fake_borrows);
-
-        all_fake_borrows
-            .into_iter()
-            .map(|matched_place_ref| {
-                let matched_place = Place {
-                    local: matched_place_ref.local,
-                    projection: tcx.mk_place_elems(matched_place_ref.projection),
-                };
-                let fake_borrow_deref_ty = matched_place.ty(&self.local_decls, tcx).ty;
-                let fake_borrow_ty =
-                    Ty::new_imm_ref(tcx, tcx.lifetimes.re_erased, fake_borrow_deref_ty);
-                let mut fake_borrow_temp = LocalDecl::new(fake_borrow_ty, temp_span);
-                fake_borrow_temp.local_info = ClearCrossCrate::Set(Box::new(LocalInfo::FakeBorrow));
-                let fake_borrow_temp = self.local_decls.push(fake_borrow_temp);
-
-                (matched_place, fake_borrow_temp)
-            })
-            .collect()
-    }
 }
 
 ///////////////////////////////////////////////////////////////////////////

compiler/rustc_mir_build/src/build/matches/util.rs

@@ -7,6 +7,7 @@ use rustc_middle::mir::*;
 use rustc_middle::thir::{self, *};
 use rustc_middle::ty::TypeVisitableExt;
 use rustc_middle::ty::{self, Ty};
+use rustc_span::Span;
 
 impl<'a, 'tcx> Builder<'a, 'tcx> {
     pub(crate) fn field_match_pairs<'pat>(
@@ -267,19 +268,99 @@ impl<'pat, 'tcx> MatchPair<'pat, 'tcx> {
 
 pub(super) struct FakeBorrowCollector<'a, 'b, 'tcx> {
     cx: &'a mut Builder<'b, 'tcx>,
+    /// Base of the scrutinee place. Used to distinguish bindings inside the scrutinee place from
+    /// bindings inside deref patterns.
+    scrutinee_base: PlaceBase,
     fake_borrows: FxIndexSet<Place<'tcx>>,
 }
 
+/// Determine the set of places that have to be stable across match guards.
+///
+/// Returns a list of places that need a fake borrow along with a local to store it.
+///
+/// Match exhaustiveness checking is not able to handle the case where the place being matched on is
+/// mutated in the guards. We add "fake borrows" to the guards that prevent any mutation of the
+/// place being matched. There are a some subtleties:
+///
+/// 1. Borrowing `*x` doesn't prevent assigning to `x`. If `x` is a shared reference, the borrow
+///    isn't even tracked. As such we have to add fake borrows of any prefixes of a place.
+/// 2. We don't want `match x { (Some(_), _) => (), .. }` to conflict with mutable borrows of `x.1`, so we
+///    only add fake borrows for places which are bound or tested by the match.
+/// 3. We don't want `match x { Some(_) => (), .. }` to conflict with mutable borrows of `(x as
+///    Some).0`, so the borrows are a special shallow borrow that only affects the place and not its
+///    projections.
+///    ```rust
+///    let mut x = (Some(0), true);
+///    match x {
+///        (Some(_), false) => {}
+///        _ if { if let Some(ref mut y) = x.0 { *y += 1 }; true } => {}
+///        _ => {}
+///    }
+///    ```
+/// 4. The fake borrows may be of places in inactive variants, e.g. here we need to fake borrow `x`
+///    and `(x as Some).0`, but when we reach the guard `x` may not be `Some`.
+///    ```rust
+///    let mut x = (Some(Some(0)), true);
+///    match x {
+///        (Some(Some(_)), false) => {}
+///        _ if { if let Some(Some(ref mut y)) = x.0 { *y += 1 }; true } => {}
+///        _ => {}
+///    }
+///    ```
+///    So it would be UB to generate code for the fake borrows. They therefore have to be removed by
+///    a MIR pass run after borrow checking.
+pub(super) fn collect_fake_borrows<'tcx>(
+    cx: &mut Builder<'_, 'tcx>,
+    candidates: &[&mut Candidate<'_, 'tcx>],
+    temp_span: Span,
+    scrutinee_base: PlaceBase,
+) -> Vec<(Place<'tcx>, Local)> {
+    let mut collector =
+        FakeBorrowCollector { cx, scrutinee_base, fake_borrows: FxIndexSet::default() };
+    for candidate in candidates.iter() {
+        collector.visit_candidate(candidate);
+    }
+    let fake_borrows = collector.fake_borrows;
+    debug!("add_fake_borrows fake_borrows = {:?}", fake_borrows);
+    let tcx = cx.tcx;
+    fake_borrows
+        .iter()
+        .copied()
+        .map(|matched_place| {
+            let fake_borrow_deref_ty = matched_place.ty(&cx.local_decls, tcx).ty;
+            let fake_borrow_ty =
+                Ty::new_imm_ref(tcx, tcx.lifetimes.re_erased, fake_borrow_deref_ty);
+            let mut fake_borrow_temp = LocalDecl::new(fake_borrow_ty, temp_span);
+            fake_borrow_temp.local_info = ClearCrossCrate::Set(Box::new(LocalInfo::FakeBorrow));
+            let fake_borrow_temp = cx.local_decls.push(fake_borrow_temp);
+            (matched_place, fake_borrow_temp)
+        })
+        .collect()
+}
+
 impl<'a, 'b, 'tcx> FakeBorrowCollector<'a, 'b, 'tcx> {
-    pub(super) fn collect_fake_borrows(
-        cx: &'a mut Builder<'b, 'tcx>,
-        candidates: &[&mut Candidate<'_, 'tcx>],
-    ) -> FxIndexSet<Place<'tcx>> {
-        let mut collector = Self { cx, fake_borrows: FxIndexSet::default() };
-        for candidate in candidates.iter() {
-            collector.visit_candidate(candidate);
+    // Fake borrow this place and its dereference prefixes.
+    fn fake_borrow(&mut self, place: Place<'tcx>) {
+        let new = self.fake_borrows.insert(place);
+        if !new {
+            return;
+        }
+        // Also fake borrow the prefixes of any fake borrow.
+        self.fake_borrow_deref_prefixes(place);
+    }
+
+    // Fake borrow the prefixes of this place that are dereferences.
+    fn fake_borrow_deref_prefixes(&mut self, place: Place<'tcx>) {
+        for (place_ref, elem) in place.as_ref().iter_projections().rev() {
+            if let ProjectionElem::Deref = elem {
+                // Insert a shallow borrow after a deref. For other projections the borrow of
+                // `place_ref` will conflict with any mutation of `place.base`.
+                let new = self.fake_borrows.insert(place_ref.to_place(self.cx.tcx));
+                if !new {
+                    return;
+                }
+            }
         }
-        collector.fake_borrows
     }
 
     fn visit_candidate(&mut self, candidate: &Candidate<'_, 'tcx>) {
@@ -305,10 +386,28 @@ impl<'a, 'b, 'tcx> FakeBorrowCollector<'a, 'b, 'tcx> {
             for flat_pat in pats.iter() {
                 self.visit_flat_pat(flat_pat)
             }
+        } else if matches!(match_pair.test_case, TestCase::Deref { .. }) {
+            // The subpairs of a deref pattern are all places relative to the deref temporary, so we
+            // don't fake borrow them. Problem is, if we only shallowly fake-borrowed
+            // `match_pair.place`, this would allow:
+            // ```
+            // let mut b = Box::new(false);
+            // match b {
+            //     deref!(true) => {} // not reached because `*b == false`
+            //     _ if { *b = true; false } => {} // not reached because the guard is `false`
+            //     deref!(false) => {} // not reached because the guard changed it
+            //     // UB because we reached the unreachable.
+            // }
+            // ```
+            // FIXME(deref_patterns): Hence we fake borrow using a non-shallow borrow.
+            if let Some(place) = match_pair.place {
+                // FIXME(deref_patterns): use a non-shallow borrow.
+                self.fake_borrow(place);
+            }
         } else {
             // Insert a Shallow borrow of any place that is switched on.
             if let Some(place) = match_pair.place {
-                self.fake_borrows.insert(place);
+                self.fake_borrow(place);
             }
 
             for subpair in &match_pair.subpairs {
@@ -318,6 +417,14 @@ impl<'a, 'b, 'tcx> FakeBorrowCollector<'a, 'b, 'tcx> {
     }
 
     fn visit_binding(&mut self, Binding { source, .. }: &Binding<'tcx>) {
+        if let PlaceBase::Local(l) = self.scrutinee_base
+            && l != source.local
+        {
+            // The base of this place is a temporary created for deref patterns. We don't emit fake
+            // borrows for these as they are not initialized in all branches.
+            return;
+        }
+
         // Insert a borrows of prefixes of places that are bound and are
         // behind a dereference projection.
         //
@@ -334,13 +441,13 @@ impl<'a, 'b, 'tcx> FakeBorrowCollector<'a, 'b, 'tcx> {
         //     y if { y == 1 && (x = &2) == () } => y,
         //     _ => 3,
         // }
-        if let Some(i) = source.projection.iter().rposition(|elem| elem == ProjectionElem::Deref) {
-            let proj_base = &source.projection[..i];
-            self.fake_borrows.insert(Place {
-                local: source.local,
-                projection: self.cx.tcx.mk_place_elems(proj_base),
-            });
-        }
+        //
+        // We don't just fake borrow the whole place because this is allowed:
+        // match u {
+        //     _ if { u = true; false } => (),
+        //     x => (),
+        // }
+        self.fake_borrow_deref_prefixes(*source);
     }
 }
 

tests/mir-opt/building/match/match_false_edges.full_tested_match.built.after.mir

@@ -4,8 +4,8 @@ fn full_tested_match() -> () {
     let mut _0: ();
     let mut _1: (i32, i32);
     let mut _2: std::option::Option<i32>;
-    let mut _3: isize;
-    let mut _4: &std::option::Option<i32>;
+    let mut _3: &std::option::Option<i32>;
+    let mut _4: isize;
     let _5: i32;
     let _6: &i32;
     let mut _7: bool;
@@ -27,8 +27,8 @@ fn full_tested_match() -> () {
         StorageLive(_2);
         _2 = Option::<i32>::Some(const 42_i32);
         PlaceMention(_2);
-        _3 = discriminant(_2);
-        switchInt(move _3) -> [0: bb5, 1: bb2, otherwise: bb1];
+        _4 = discriminant(_2);
+        switchInt(move _4) -> [0: bb5, 1: bb2, otherwise: bb1];
     }
 
     bb1: {
@@ -60,7 +60,7 @@ fn full_tested_match() -> () {
     bb7: {
         StorageLive(_6);
         _6 = &((_2 as Some).0: i32);
-        _4 = &fake _2;
+        _3 = &fake _2;
         StorageLive(_7);
         _7 = guard() -> [return: bb8, unwind: bb16];
     }
@@ -71,7 +71,7 @@ fn full_tested_match() -> () {
 
     bb9: {
         StorageDead(_7);
-        FakeRead(ForMatchGuard, _4);
+        FakeRead(ForMatchGuard, _3);
         FakeRead(ForGuardBinding, _6);
         StorageLive(_5);
         _5 = ((_2 as Some).0: i32);