Make closure capturing have consistent and correct behaviour around patterns

compiler/rustc_hir_typeck/src/upvar.rs

@@ -761,6 +761,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
     ///       ],
     /// }
     /// ```
+    #[instrument(level = "debug", skip(self))]
     fn compute_min_captures(
         &self,
         closure_def_id: LocalDefId,
@@ -2028,6 +2029,7 @@ struct InferBorrowKind<'tcx> {
 }
 
 impl<'tcx> euv::Delegate<'tcx> for InferBorrowKind<'tcx> {
+    #[instrument(skip(self), level = "debug")]
     fn fake_read(
         &mut self,
         place_with_id: &PlaceWithHirId<'tcx>,
@@ -2118,6 +2120,7 @@ impl<'tcx> euv::Delegate<'tcx> for InferBorrowKind<'tcx> {
 }
 
 /// Rust doesn't permit moving fields out of a type that implements drop
+#[instrument(skip(fcx), ret, level = "debug")]
 fn restrict_precision_for_drop_types<'a, 'tcx>(
     fcx: &'a FnCtxt<'a, 'tcx>,
     mut place: Place<'tcx>,
@@ -2177,6 +2180,7 @@ fn restrict_precision_for_unsafe(
 /// - No Index projections are captured, since arrays are captured completely.
 /// - No unsafe block is required to capture `place`
 /// Returns the truncated place and updated capture mode.
+#[instrument(ret, level = "debug")]
 fn restrict_capture_precision(
     place: Place<'_>,
     curr_mode: ty::UpvarCapture,
@@ -2205,6 +2209,7 @@ fn restrict_capture_precision(
 }
 
 /// Truncate deref of any reference.
+#[instrument(ret, level = "debug")]
 fn adjust_for_move_closure(
     mut place: Place<'_>,
     mut kind: ty::UpvarCapture,
@@ -2219,6 +2224,7 @@ fn adjust_for_move_closure(
 }
 
 /// Truncate deref of any reference.
+#[instrument(ret, level = "debug")]
 fn adjust_for_use_closure(
     mut place: Place<'_>,
     mut kind: ty::UpvarCapture,
@@ -2234,6 +2240,7 @@ fn adjust_for_use_closure(
 
 /// Adjust closure capture just that if taking ownership of data, only move data
 /// from enclosing stack frame.
+#[instrument(ret, level = "debug")]
 fn adjust_for_non_move_closure(
     mut place: Place<'_>,
     mut kind: ty::UpvarCapture,
@@ -2557,6 +2564,7 @@ fn determine_place_ancestry_relation<'tcx>(
 ///     // it is constrained to `'a`
 /// }
 /// ```
+#[instrument(ret, level = "debug")]
 fn truncate_capture_for_optimization(
     mut place: Place<'_>,
     mut curr_mode: ty::UpvarCapture,

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

@@ -293,6 +293,12 @@ impl<'tcx> MatchPairTree<'tcx> {
 
         if let Some(test_case) = test_case {
             // This pattern is refutable, so push a new match-pair node.
+            //
+            // Note: unless test_case is TestCase::Or, place must not be None.
+            // This means that the closure capture analysis in
+            // rustc_hir_typeck::upvar, and in particular the pattern handling
+            // code of ExprUseVisitor, must capture all of the places we'll use.
+            // Make sure to keep these two parts in sync!
             match_pairs.push(MatchPairTree {
                 place,
                 test_case,

src/tools/miri/tests/fail/upvars/deref-in-pattern.rs

@@ -0,0 +1,19 @@
+// This test serves to document the change in semantics introduced by #138961.
+//
+// A corollary of partial-pattern.rs: while the tuple access showcases the
+// utility, it is actually the dereference performed by the pattern that
+// matters.
+
+fn main() {
+    // the inner reference is dangling
+    let x: &&u32 = unsafe {
+        let x: u32 = 42;
+        &&* &raw const x
+    };
+
+    let _ = || { //~ ERROR: encountered a dangling reference
+        match x {
+            &&_y => {},
+        }
+    };
+}

src/tools/miri/tests/fail/upvars/deref-in-pattern.stderr

@@ -0,0 +1,20 @@
+error: Undefined Behavior: constructing invalid value: encountered a dangling reference (use-after-free)
+  --> tests/fail/upvars/deref-in-pattern.rs:LL:CC
+   |
+LL |       let _ = || {
+   |  _____________^
+LL | |         match x {
+LL | |             &&_y => {},
+LL | |         }
+LL | |     };
+   | |_____^ constructing invalid value: encountered a dangling reference (use-after-free)
+   |
+   = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
+   = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
+   = note: BACKTRACE:
+   = note: inside `main` at tests/fail/upvars/deref-in-pattern.rs:LL:CC
+
+note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace
+
+error: aborting due to 1 previous error
+

src/tools/miri/tests/fail/upvars/partial-pattern.rs

@@ -0,0 +1,27 @@
+// This test serves to document the change in semantics introduced by #138961.
+//
+// Previously, the closure would capture the entirety of x, and access *(*x).0
+// when called. Now, the closure only captures *(*x).0, which means that
+// a &*(*x).0 reborrow happens when the closure is constructed.
+//
+// Hence, if one of the references is dangling, this constitutes newly introduced UB
+// in the case where the closure doesn't get called. This isn't a big deal,
+// because why opsem only now considers this to be UB, the unsafe code
+// guidelines have long recommended against any handling of dangling references.
+
+fn main() {
+    // the inner references are dangling
+    let x: &(&u32, &u32) = unsafe {
+        let a = 21;
+        let b = 37;
+        let ra = &* &raw const a;
+        let rb = &* &raw const b;
+        &(ra, rb)
+    };
+
+    let _ = || { //~ ERROR: encountered a dangling reference
+        match x {
+            (&_y, _) => {},
+        }
+    };
+}

src/tools/miri/tests/fail/upvars/partial-pattern.stderr

@@ -0,0 +1,20 @@
+error: Undefined Behavior: constructing invalid value: encountered a dangling reference (use-after-free)
+  --> tests/fail/upvars/partial-pattern.rs:LL:CC
+   |
+LL |       let _ = || {
+   |  _____________^
+LL | |         match x {
+LL | |             (&_y, _) => {},
+LL | |         }
+LL | |     };
+   | |_____^ constructing invalid value: encountered a dangling reference (use-after-free)
+   |
+   = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
+   = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
+   = note: BACKTRACE:
+   = note: inside `main` at tests/fail/upvars/partial-pattern.rs:LL:CC
+
+note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace
+
+error: aborting due to 1 previous error
+

src/tools/miri/tests/fail/upvars/uninhabited-variant.rs

@@ -0,0 +1,31 @@
+// Motivated by #138961, this shows how invalid discriminants interact with
+// closure captures.
+#![feature(never_type)]
+
+#[repr(C)]
+#[allow(dead_code)]
+enum E {
+  V0, // discriminant: 0
+  V1, // 1
+  V2(!), // 2
+}
+
+fn main() {
+    assert_eq!(std::mem::size_of::<E>(), 4);
+
+    let val = 2u32;
+    let ptr = (&raw const val).cast::<E>();
+    let r = unsafe { &*ptr };
+    let f = || {
+        // After #138961, constructing the closure performs a reborrow of r.
+        // Nevertheless, the discriminant is only actually inspected when the closure
+        // is called.
+        match r { //~ ERROR: read discriminant of an uninhabited enum variant
+            E::V0 => {}
+            E::V1 => {}
+            E::V2(_) => {}
+        }
+    };
+
+    f();
+}

src/tools/miri/tests/fail/upvars/uninhabited-variant.stderr

@@ -0,0 +1,20 @@
+error: Undefined Behavior: read discriminant of an uninhabited enum variant
+  --> tests/fail/upvars/uninhabited-variant.rs:LL:CC
+   |
+LL |         match r {
+   |               ^ read discriminant of an uninhabited enum variant
+   |
+   = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
+   = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
+   = note: BACKTRACE:
+   = note: inside closure at tests/fail/upvars/uninhabited-variant.rs:LL:CC
+note: inside `main`
+  --> tests/fail/upvars/uninhabited-variant.rs:LL:CC
+   |
+LL |     f();
+   |     ^^^
+
+note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace
+
+error: aborting due to 1 previous error
+

tests/crashes/119786-2.rs

@@ -0,0 +1,15 @@
+//@ known-bug: #119786
+//@ edition:2021
+
+fn enum_upvar() {
+    type T = impl Copy;
+    let foo: T = Some((1u32, 2u32));
+    let x = move || {
+        match foo {
+            None => (),
+            Some(_) => (),
+        }
+    };
+}
+
+pub fn main() {}

tests/crashes/119786-3.rs

@@ -0,0 +1,15 @@
+//@ known-bug: #119786
+//@ edition:2021
+
+fn enum_upvar() {
+    type T = impl Copy;
+    let foo: T = Some((1u32, 2u32));
+    let x = move || {
+        match foo {
+            None => (),
+            Some((a, b)) => (),
+        }
+    };
+}
+
+pub fn main() {}

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

@@ -4,18 +4,16 @@ fn foo::{closure#0}::{closure#0}(_1: {async closure body@$DIR/async_closure_fake
 yields ()
  {
     debug _task_context => _2;
-    debug f => (*(_1.0: &&Foo));
+    debug f => (*(_1.0: &Foo));
     let mut _0: ();
     let mut _3: &Foo;
     let mut _4: &&Foo;
-    let mut _5: &&&Foo;
-    let mut _6: isize;
-    let mut _7: bool;
+    let mut _5: isize;
+    let mut _6: bool;
 
     bb0: {
-        PlaceMention((*(_1.0: &&Foo)));
-        _6 = discriminant((*(*(_1.0: &&Foo))));
-        switchInt(move _6) -> [0: bb2, otherwise: bb1];
+        _5 = discriminant((*(_1.0: &Foo)));
+        switchInt(move _5) -> [0: bb2, otherwise: bb1];
     }
 
     bb1: {
@@ -32,28 +30,25 @@ yields ()
     }
 
     bb4: {
-        FakeRead(ForMatchedPlace(None), (*(_1.0: &&Foo)));
         unreachable;
     }
 
     bb5: {
-        _3 = &fake shallow (*(*(_1.0: &&Foo)));
-        _4 = &fake shallow (*(_1.0: &&Foo));
-        _5 = &fake shallow (_1.0: &&Foo);
-        StorageLive(_7);
-        _7 = const true;
-        switchInt(move _7) -> [0: bb8, otherwise: bb7];
+        _3 = &fake shallow (*(_1.0: &Foo));
+        _4 = &fake shallow (_1.0: &Foo);
+        StorageLive(_6);
+        _6 = const true;
+        switchInt(move _6) -> [0: bb8, otherwise: bb7];
     }
 
     bb6: {
         falseEdge -> [real: bb3, imaginary: bb1];
     }
 
     bb7: {
-        StorageDead(_7);
+        StorageDead(_6);
         FakeRead(ForMatchGuard, _3);
         FakeRead(ForMatchGuard, _4);
-        FakeRead(ForMatchGuard, _5);
         _0 = const ();
         goto -> bb10;
     }
@@ -63,7 +58,7 @@ yields ()
     }
 
     bb9: {
-        StorageDead(_7);
+        StorageDead(_6);
         goto -> bb6;
     }