Auto merge of #138961 - meithecatte:expr-use-visitor, r=<try>

Make closure capturing have consistent and correct behaviour around patterns

Author: rust-bors[bot]

compiler/rustc_hir_typeck/src/expr_use_visitor.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,
@@ -2030,6 +2031,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>,
@@ -2120,6 +2122,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>,
@@ -2180,6 +2183,7 @@ fn restrict_precision_for_unsafe(
 /// - 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,
@@ -2209,6 +2213,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,
@@ -2223,6 +2228,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,
@@ -2238,6 +2244,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,
@@ -2560,6 +2567,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_hir_typeck/src/upvar.rs

@@ -311,6 +311,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,

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

@@ -0,0 +1,20 @@
+// This test serves to document the change in semantics introduced by
+// rust-lang/rust#138961.
+//
+// A corollary of partial-pattern.rs: while the tuple access testcase makes
+// it clear why these semantics are useful, it is actually the dereference
+// being 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/closures/deref-in-pattern.rs

@@ -0,0 +1,20 @@
+error: Undefined Behavior: constructing invalid value: encountered a dangling reference (use-after-free)
+  --> tests/fail/closures/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/closures/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/closures/deref-in-pattern.stderr

@@ -0,0 +1,28 @@
+// This test serves to document the change in semantics introduced by
+// rust-lang/rust#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 while 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/closures/partial-pattern.rs

@@ -0,0 +1,20 @@
+error: Undefined Behavior: constructing invalid value: encountered a dangling reference (use-after-free)
+  --> tests/fail/closures/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/closures/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/closures/partial-pattern.stderr

@@ -0,0 +1,31 @@
+// Motivated by rust-lang/rust#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 rust-lang/rust#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/closures/uninhabited-variant.rs

@@ -0,0 +1,20 @@
+error: Undefined Behavior: read discriminant of an uninhabited enum variant
+  --> tests/fail/closures/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/closures/uninhabited-variant.rs:LL:CC
+note: inside `main`
+  --> tests/fail/closures/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
+