put guards' fake reads in the failure block and only put necessary fake reads in the success block

compiler/rustc_middle/src/mir/mod.rs

@@ -1099,6 +1099,8 @@ pub enum LocalInfo<'tcx> {
     DerefTemp,
     /// A temporary created for borrow checking.
     FakeBorrow,
+    /// A temporary created to hold the result of a `deref` or `deref_mut` call for a deref pattern.
+    PatternTemp { matched_place: Place<'tcx> },
     /// A local without anything interesting about it.
     Boring,
 }

compiler/rustc_mir_build/src/builder/matches/match_pair.rs

@@ -1,7 +1,9 @@
 use std::sync::Arc;
 
 use rustc_abi::FieldIdx;
-use rustc_middle::mir::{Pinnedness, Place, PlaceElem, ProjectionElem};
+use rustc_middle::mir::{
+    ClearCrossCrate, LocalDecl, LocalInfo, Pinnedness, Place, PlaceElem, ProjectionElem,
+};
 use rustc_middle::thir::{Ascription, DerefPatBorrowMode, FieldPat, Pat, PatKind};
 use rustc_middle::ty::consts::ConstExt;
 use rustc_middle::ty::{self, Ty, TypeVisitableExt};
@@ -442,10 +444,14 @@ impl<'tcx> InterPat<'tcx> {
             } => {
                 // Create a new temporary for each deref pattern.
                 // FIXME(deref_patterns): dedup temporaries to avoid multiple `deref()` calls?
-                let temp = cx.temp(
+                let mut temp_decl = LocalDecl::new(
                     Ty::new_ref(cx.tcx, cx.tcx.lifetimes.re_erased, subpattern.ty, mutability),
                     pattern.span,
                 );
+                temp_decl.local_info = ClearCrossCrate::Set(Box::new(LocalInfo::PatternTemp {
+                    matched_place: unwrap_place(),
+                }));
+                let temp = Place::from(cx.local_decls.push(temp_decl));
                 let subpat =
                     InterPat::lower_thir_pat(cx, PlaceBuilder::from(temp).deref(), subpattern);
                 InterPatKind::Refutable {

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

@@ -2464,9 +2464,44 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
             let guard_frame = self.guard_context.pop().unwrap();
             debug!("Exiting guard building context with locals: {:?}", guard_frame);
 
-            for &(_, temp, _) in fake_borrows {
+            let by_value_bindings = sub_branch
+                .bindings
+                .iter()
+                .filter(|binding| matches!(binding.binding_mode.0, ByRef::No));
+
+            for &(fake_borrowed_place, temp, _) in fake_borrows {
                 let cause = FakeReadCause::ForMatchGuard;
-                self.cfg.push_fake_read(guard_true_block, guard_end, cause, Place::from(temp));
+                // We put fake reads of fake borrows on the guard's failure path to make sure
+                // they're reachable if we continue matching (#161578). This includes all matched-on
+                // subplaces of the scrutinee place and all references they're under, so that we can
+                // assume in match lowering that discriminants haven't been modified.
+                self.cfg.push_fake_read(guard_false_block, guard_end, cause, Place::from(temp));
+                // We put fake reads on the success path to make sure we can create by-value
+                // bindings. For that, we only need to keep fake borrows alive that are prefixes of
+                // a place we move or copy from to make a by-value binding.
+                if by_value_bindings.clone().any(|binding| {
+                    // We don't make fake borrows relative to deref pattern temporaries. If the
+                    // binding is under deref patterns, we find the place matched on by the
+                    // outermost deref pattern and add fake borrows for it and its prefixes.
+                    let mut matched_place = binding.source;
+                    while matched_place.local != fake_borrowed_place.local {
+                        let &LocalInfo::PatternTemp { matched_place: deref_pat_place } =
+                            self.local_decls[matched_place.local].local_info()
+                        else {
+                            bug!(
+                                "binding created relative to non-matched-on local {:?}",
+                                matched_place.local
+                            );
+                        };
+                        matched_place = deref_pat_place;
+                    }
+                    matched_place.projection.starts_with(fake_borrowed_place.projection)
+                }) {
+                    self.cfg.push_fake_read(guard_true_block, guard_end, cause, Place::from(temp));
+                }
+                // This won't keep fake borrows live on paths where the guard diverges
+                // unconditionally, but that should be sound since we can't continue matching or
+                // make bindings after diverging.
             }
 
             self.cfg.goto(guard_false_block, source_info, sub_branch.otherwise_block);
@@ -2497,10 +2532,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
             // ```
             //
             // and that is clearly not correct.
-            let by_value_bindings = sub_branch
-                .bindings
-                .iter()
-                .filter(|binding| matches!(binding.binding_mode.0, ByRef::No));
+
             // Read all of the by reference bindings to ensure that the
             // place they refer to can't be modified by the guard.
             for binding in by_value_bindings.clone() {

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

@@ -106,6 +106,7 @@ fn full_tested_match() -> () {
     bb13: {
         StorageDead(_7);
         StorageDead(_6);
+        FakeRead(ForMatchGuard, _3);
         goto -> bb9;
     }
 

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

@@ -106,6 +106,7 @@ fn full_tested_match2() -> () {
     bb13: {
         StorageDead(_7);
         StorageDead(_6);
+        FakeRead(ForMatchGuard, _3);
         goto -> bb9;
     }
 

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

@@ -137,6 +137,7 @@ fn main() -> () {
     bb17: {
         StorageDead(_8);
         StorageDead(_7);
+        FakeRead(ForMatchGuard, _3);
         goto -> bb13;
     }
 
@@ -165,6 +166,7 @@ fn main() -> () {
         StorageDead(_13);
         StorageDead(_12);
         StorageDead(_11);
+        FakeRead(ForMatchGuard, _3);
         goto -> bb10;
     }
 

tests/mir-opt/building/match/sort_candidates.constant_eq.SimplifyCfg-initial.after.mir

@@ -102,15 +102,15 @@ fn constant_eq(_1: &str, _2: bool) -> u32 {
 
     bb16: {
         StorageDead(_13);
-        FakeRead(ForMatchGuard, _6);
-        FakeRead(ForMatchGuard, _7);
-        FakeRead(ForMatchGuard, _8);
         _0 = const 1_u32;
         goto -> bb18;
     }
 
     bb17: {
         StorageDead(_13);
+        FakeRead(ForMatchGuard, _6);
+        FakeRead(ForMatchGuard, _7);
+        FakeRead(ForMatchGuard, _8);
         falseEdge -> [real: bb3, imaginary: bb5];
     }
 

tests/mir-opt/building/match/sort_candidates.disjoint_ranges.SimplifyCfg-initial.after.mir

@@ -72,13 +72,13 @@ fn disjoint_ranges(_1: i32, _2: bool) -> u32 {
 
     bb12: {
         StorageDead(_8);
-        FakeRead(ForMatchGuard, _3);
         _0 = const 0_u32;
         goto -> bb14;
     }
 
     bb13: {
         StorageDead(_8);
+        FakeRead(ForMatchGuard, _3);
         falseEdge -> [real: bb1, imaginary: bb3];
     }
 

tests/mir-opt/coroutine/async_closure_fake_read_for_by_move.foo-{closure#0}-{closure#0}.built.after.mir

@@ -47,8 +47,6 @@ yields ()
 
     bb7: {
         StorageDead(_6);
-        FakeRead(ForMatchGuard, _3);
-        FakeRead(ForMatchGuard, _4);
         _0 = const ();
         goto -> bb10;
     }
@@ -59,6 +57,8 @@ yields ()
 
     bb9: {
         StorageDead(_6);
+        FakeRead(ForMatchGuard, _3);
+        FakeRead(ForMatchGuard, _4);
         goto -> bb6;
     }
 

tests/mir-opt/coroutine/async_closure_fake_read_for_by_move.foo-{closure#0}-{synthetic#0}.built.after.mir

@@ -35,14 +35,14 @@ yields ()
 
     bb4: {
         StorageDead(_6);
-        FakeRead(ForMatchGuard, _3);
-        FakeRead(ForMatchGuard, _4);
         _0 = const ();
         goto -> bb6;
     }
 
     bb5: {
         StorageDead(_6);
+        FakeRead(ForMatchGuard, _3);
+        FakeRead(ForMatchGuard, _4);
         falseEdge -> [real: bb1, imaginary: bb1];
     }
 

tests/mir-opt/match_arm_scopes.complicated_match.panic-abort.SimplifyCfg-initial.after-ElaborateDrops.after.diff

@@ -135,7 +135,6 @@
           StorageDead(_10);
           StorageDead(_9);
 -         FakeRead(ForMatchGuard, _3);
--         FakeRead(ForMatchGuard, _4);
 -         FakeRead(ForGuardBinding, _6);
 -         FakeRead(ForGuardBinding, _8);
           StorageLive(_5);
@@ -152,6 +151,8 @@
           StorageDead(_9);
           StorageDead(_8);
           StorageDead(_6);
+-         FakeRead(ForMatchGuard, _3);
+-         FakeRead(ForMatchGuard, _4);
 -         falseEdge -> [real: bb3, imaginary: bb3];
 +         goto -> bb2;
       }
@@ -176,7 +177,6 @@
 +     bb14: {
           StorageDead(_13);
           StorageDead(_12);
--         FakeRead(ForMatchGuard, _3);
 -         FakeRead(ForMatchGuard, _4);
 -         FakeRead(ForGuardBinding, _6);
 -         FakeRead(ForGuardBinding, _8);
@@ -194,6 +194,8 @@
           StorageDead(_12);
           StorageDead(_8);
           StorageDead(_6);
+-         FakeRead(ForMatchGuard, _3);
+-         FakeRead(ForMatchGuard, _4);
 -         falseEdge -> [real: bb1, imaginary: bb1];
 +         goto -> bb1;
       }

tests/mir-opt/match_arm_scopes.complicated_match.panic-unwind.SimplifyCfg-initial.after-ElaborateDrops.after.diff

@@ -135,7 +135,6 @@
           StorageDead(_10);
           StorageDead(_9);
 -         FakeRead(ForMatchGuard, _3);
--         FakeRead(ForMatchGuard, _4);
 -         FakeRead(ForGuardBinding, _6);
 -         FakeRead(ForGuardBinding, _8);
           StorageLive(_5);
@@ -152,6 +151,8 @@
           StorageDead(_9);
           StorageDead(_8);
           StorageDead(_6);
+-         FakeRead(ForMatchGuard, _3);
+-         FakeRead(ForMatchGuard, _4);
 -         falseEdge -> [real: bb3, imaginary: bb3];
 +         goto -> bb2;
       }
@@ -176,7 +177,6 @@
 +     bb14: {
           StorageDead(_13);
           StorageDead(_12);
--         FakeRead(ForMatchGuard, _3);
 -         FakeRead(ForMatchGuard, _4);
 -         FakeRead(ForGuardBinding, _6);
 -         FakeRead(ForGuardBinding, _8);
@@ -194,6 +194,8 @@
           StorageDead(_12);
           StorageDead(_8);
           StorageDead(_6);
+-         FakeRead(ForMatchGuard, _3);
+-         FakeRead(ForMatchGuard, _4);
 -         falseEdge -> [real: bb1, imaginary: bb1];
 +         goto -> bb1;
       }

tests/mir-opt/remove_fake_borrows.match_guard.CleanupPostBorrowck.panic-abort.diff

@@ -48,21 +48,21 @@
 
       bb5: {
           StorageDead(_8);
+          _0 = const 0_i32;
+          goto -> bb7;
+      }
+
+      bb6: {
+          StorageDead(_8);
 -         FakeRead(ForMatchGuard, _3);
 -         FakeRead(ForMatchGuard, _4);
 -         FakeRead(ForMatchGuard, _5);
 -         FakeRead(ForMatchGuard, _6);
+-         falseEdge -> [real: bb1, imaginary: bb1];
 +         nop;
 +         nop;
 +         nop;
 +         nop;
-          _0 = const 0_i32;
-          goto -> bb7;
-      }
-
-      bb6: {
-          StorageDead(_8);
--         falseEdge -> [real: bb1, imaginary: bb1];
 +         goto -> bb1;
       }
 

tests/mir-opt/remove_fake_borrows.match_guard.CleanupPostBorrowck.panic-unwind.diff

@@ -48,21 +48,21 @@
 
       bb5: {
           StorageDead(_8);
+          _0 = const 0_i32;
+          goto -> bb7;
+      }
+
+      bb6: {
+          StorageDead(_8);
 -         FakeRead(ForMatchGuard, _3);
 -         FakeRead(ForMatchGuard, _4);
 -         FakeRead(ForMatchGuard, _5);
 -         FakeRead(ForMatchGuard, _6);
+-         falseEdge -> [real: bb1, imaginary: bb1];
 +         nop;
 +         nop;
 +         nop;
 +         nop;
-          _0 = const 0_i32;
-          goto -> bb7;
-      }
-
-      bb6: {
-          StorageDead(_8);
--         falseEdge -> [real: bb1, imaginary: bb1];
 +         goto -> bb1;
       }
 

tests/ui/match/divergent-guard-binding-permissiveness.rs

@@ -0,0 +1,25 @@
+//! On a guard's success path, we only add fake-reads for fake borrows on prefixes of by-value
+//! bindings' places, since at that point we only need to be able to create by-value bindings.
+//! Consequently, guards that can't fail after a matched-on place has been modified may still
+//! borrow-check successfully if no by-value bindings depended on that place to still be valid. See
+//! `divergent-guard-binding-restrictiveness.rs` for cases where the presence of bindings causes a
+//! borrow-checking failure.
+//@ check-pass
+#![allow(unused)]
+
+fn main() {
+    let mut x: (Option<&Box<u64>>, u64) = (Some(&Box::new(7)), 0);
+    match x {
+        // By-ref bindings are created before evaluating the guard, so we don't need fake borrows
+        // to catch when they would be invalidated. As long as we don't use `b` after, it's ok.
+        (Some(ref b), c) if { x.0 = None; false } || return => {}
+        (Some(ref b), c) if false && ({ x.0 = None; false } || return) => {}
+
+        // The assignment doesn't invalidate borrows of the lifetime-extended temporary box. It's
+        // unchanged even when `x` is set to `None`, so it's fine to read from after.
+        (Some(&ref b), c) if { x.0 = None; false } || return => { **b; }
+        (Some(&ref b), c) if false && ({ x.0 = None; false } || return) => { **b; }
+
+        a => { a; }
+    }
+}

tests/ui/match/divergent-guard-binding-restrictiveness.rs

@@ -0,0 +1,31 @@
+//! On a guard's success path, we add fake-reads for fake-borrows on prefixes of by-value bindings'
+//! places, since at that point we need to be able to create by-value bindings. For cases where we
+//! omit unnecessary fake reads on the success path, see `divergent-guard-binding-permissivenss.rs`.
+
+fn main() {
+    let mut x: Option<&u64> = Some(&7);
+    match x {
+        // These guards can succeed but not fail after `x = None`. Since `x` is a prefix of the by-
+        // value binding `b`'s place, we can't bind `b` if we do that.
+        Some(&b) if { x = None; false } || return => {}
+        //~^ ERROR: cannot assign `x` in match guard
+        Some(&b) if false || ({ x = None; false } || return) => {}
+        //~^ ERROR: cannot assign `x` in match guard
+
+        // If there isn't a deref between the modified place and the bound place, this may be caught
+        // instead by the fake read on the `RefWithinGuard` borrow of the binding.
+        Some(b) if { x = None; false } || return => {}
+        //~^ ERROR: cannot assign to `x` because it is borrowed
+        Some(b) if false || ({ x = None; false } || return) => {}
+        //~^ ERROR: cannot assign to `x` because it is borrowed
+
+        // By-ref bindings are created before evaluating the guard, so we don't need fake borrows
+        // to catch when they would be invalidated.
+        Some(ref b) if { x = None; false } || return => { b; }
+        //~^ ERROR: cannot assign to `x` because it is borrowed
+        Some(ref b) if false || ({ x = None; false } || return) => { b; }
+        //~^ ERROR: cannot assign to `x` because it is borrowed
+
+        _ => {}
+    }
+}