Auto merge of rust-lang#115315 - RalfJung:field-capture-packed-alignm…

…ent, r=oli-obk

closure field capturing: don't depend on alignment of packed fields

This fixes the closure field capture part of rust-lang#115305: field capturing always stops at projections into packed structs, no matter the alignment of the field. This means changing a private field type from `u8` to `u64` can never change how closures capture fields, which is probably what we want.

Here's an example where, before this PR, changing the type of a private field in a repr(Rust) struct can change the output of a program:

```rust
#![allow(dead_code)]

mod m {
    // before patch
    #[derive(Default)]
    pub struct S1(u8);
    // after patch
    #[derive(Default)]
    pub struct S2(u64);
}

struct NoisyDrop;
impl Drop for NoisyDrop {
    fn drop(&mut self) {
        eprintln!("dropped!");
    }
}

#[repr(packed)]
struct MyType {
    field: m::S1, // output changes when this becomes S2
    other_field: NoisyDrop,
    third_field: Vec<()>,
}

fn test(r: MyType) {
    let c = || {
        let _val = std::ptr::addr_of!(r.field);
        let _val = r.third_field;
    };
    drop(c);
    eprintln!("before dropping");
}

fn main() {
    test(MyType {
        field: Default::default(),
        other_field: NoisyDrop,
        third_field: Vec::new(),
    });
}
```

Of course this is a breaking change for the same reason that doing field capturing in the first place was a breaking change. Packed fields are relatively rare and depending on drop order is relatively rare, so I don't expect this to have much impact, but it's hard to be sure and even a crater run will only tell us so much.

Also see the [nomination comment](rust-lang#115315 (comment)).

Cc `@rust-lang/wg-rfc-2229` `@ehuss`

compiler/rustc_hir_typeck/src/upvar.rs

@@ -195,7 +195,6 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
 
         assert_eq!(self.tcx.hir().body_owner_def_id(body.id()), closure_def_id);
         let mut delegate = InferBorrowKind {
-            fcx: self,
             closure_def_id,
             capture_information: Default::default(),
             fake_reads: Default::default(),
@@ -1607,34 +1606,20 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
 /// Truncate the capture so that the place being borrowed is in accordance with RFC 1240,
 /// which states that it's unsafe to take a reference into a struct marked `repr(packed)`.
 fn restrict_repr_packed_field_ref_capture<'tcx>(
-    tcx: TyCtxt<'tcx>,
-    param_env: ty::ParamEnv<'tcx>,
     mut place: Place<'tcx>,
     mut curr_borrow_kind: ty::UpvarCapture,
 ) -> (Place<'tcx>, ty::UpvarCapture) {
     let pos = place.projections.iter().enumerate().position(|(i, p)| {
         let ty = place.ty_before_projection(i);
 
-        // Return true for fields of packed structs, unless those fields have alignment 1.
+        // Return true for fields of packed structs.
         match p.kind {
             ProjectionKind::Field(..) => match ty.kind() {
                 ty::Adt(def, _) if def.repr().packed() => {
-                    // We erase regions here because they cannot be hashed
-                    match tcx.layout_of(param_env.and(tcx.erase_regions(p.ty))) {
-                        Ok(layout) if layout.align.abi.bytes() == 1 => {
-                            // if the alignment is 1, the type can't be further
-                            // disaligned.
-                            debug!(
-                                "restrict_repr_packed_field_ref_capture: ({:?}) - align = 1",
-                                place
-                            );
-                            false
-                        }
-                        _ => {
-                            debug!("restrict_repr_packed_field_ref_capture: ({:?}) - true", place);
-                            true
-                        }
-                    }
+                    // We stop here regardless of field alignment. Field alignment can change as
+                    // types change, including the types of private fields in other crates, and that
+                    // shouldn't affect how we compute our captures.
+                    true
                 }
 
                 _ => false,
@@ -1689,9 +1674,7 @@ fn drop_location_span(tcx: TyCtxt<'_>, hir_id: hir::HirId) -> Span {
     tcx.sess.source_map().end_point(owner_span)
 }
 
-struct InferBorrowKind<'a, 'tcx> {
-    fcx: &'a FnCtxt<'a, 'tcx>,
-
+struct InferBorrowKind<'tcx> {
     // The def-id of the closure whose kind and upvar accesses are being inferred.
     closure_def_id: LocalDefId,
 
@@ -1725,7 +1708,7 @@ struct InferBorrowKind<'a, 'tcx> {
     fake_reads: Vec<(Place<'tcx>, FakeReadCause, hir::HirId)>,
 }
 
-impl<'a, 'tcx> euv::Delegate<'tcx> for InferBorrowKind<'a, 'tcx> {
+impl<'tcx> euv::Delegate<'tcx> for InferBorrowKind<'tcx> {
     fn fake_read(
         &mut self,
         place: &PlaceWithHirId<'tcx>,
@@ -1740,12 +1723,7 @@ impl<'a, 'tcx> euv::Delegate<'tcx> for InferBorrowKind<'a, 'tcx> {
 
         let (place, _) = restrict_capture_precision(place.place.clone(), dummy_capture_kind);
 
-        let (place, _) = restrict_repr_packed_field_ref_capture(
-            self.fcx.tcx,
-            self.fcx.param_env,
-            place,
-            dummy_capture_kind,
-        );
+        let (place, _) = restrict_repr_packed_field_ref_capture(place, dummy_capture_kind);
         self.fake_reads.push((place, cause, diag_expr_id));
     }
 
@@ -1780,12 +1758,8 @@ impl<'a, 'tcx> euv::Delegate<'tcx> for InferBorrowKind<'a, 'tcx> {
         // We only want repr packed restriction to be applied to reading references into a packed
         // struct, and not when the data is being moved. Therefore we call this method here instead
         // of in `restrict_capture_precision`.
-        let (place, mut capture_kind) = restrict_repr_packed_field_ref_capture(
-            self.fcx.tcx,
-            self.fcx.param_env,
-            place_with_id.place.clone(),
-            capture_kind,
-        );
+        let (place, mut capture_kind) =
+            restrict_repr_packed_field_ref_capture(place_with_id.place.clone(), capture_kind);
 
         // Raw pointers don't inherit mutability
         if place_with_id.place.deref_tys().any(Ty::is_unsafe_ptr) {

tests/ui/closures/2229_closure_analysis/repr_packed.rs

@@ -3,7 +3,8 @@
 #![feature(rustc_attrs)]
 
 // `u8` aligned at a byte and are unaffected by repr(packed).
-// Therefore we can precisely (and safely) capture references to both the fields.
+// Therefore we *could* precisely (and safely) capture references to both the fields,
+// but we don't, since we don't want capturing to change when field types change alignment.
 fn test_alignment_not_affected() {
     #[repr(packed)]
     struct Foo { x: u8, y: u8 }
@@ -17,11 +18,10 @@ fn test_alignment_not_affected() {
     //~^ ERROR: First Pass analysis includes:
     //~| ERROR: Min Capture analysis includes:
         let z1: &u8 = &foo.x;
-        //~^ NOTE: Capturing foo[(0, 0)] -> ImmBorrow
-        //~| NOTE: Min Capture foo[(0, 0)] -> ImmBorrow
+        //~^ NOTE: Capturing foo[] -> ImmBorrow
         let z2: &mut u8 = &mut foo.y;
-        //~^ NOTE: Capturing foo[(1, 0)] -> MutBorrow
-        //~| NOTE: Min Capture foo[(1, 0)] -> MutBorrow
+        //~^ NOTE: Capturing foo[] -> MutBorrow
+        //~| NOTE: Min Capture foo[] -> MutBorrow
 
         *z2 = 42;
 

tests/ui/closures/2229_closure_analysis/repr_packed.stderr

@@ -1,5 +1,5 @@
 error[E0658]: attributes on expressions are experimental
-  --> $DIR/repr_packed.rs:13:17
+  --> $DIR/repr_packed.rs:14:17
    |
 LL |     let mut c = #[rustc_capture_analysis]
    |                 ^^^^^^^^^^^^^^^^^^^^^^^^^
@@ -26,7 +26,7 @@ LL |     let c = #[rustc_capture_analysis]
    = help: add `#![feature(stmt_expr_attributes)]` to the crate attributes to enable
 
 error: First Pass analysis includes:
-  --> $DIR/repr_packed.rs:16:5
+  --> $DIR/repr_packed.rs:17:5
    |
 LL | /     || {
 LL | |
@@ -37,19 +37,19 @@ LL | |         println!("({}, {})", z1, z2);
 LL | |     };
    | |_____^
    |
-note: Capturing foo[(0, 0)] -> ImmBorrow
-  --> $DIR/repr_packed.rs:19:24
+note: Capturing foo[] -> ImmBorrow
+  --> $DIR/repr_packed.rs:20:24
    |
 LL |         let z1: &u8 = &foo.x;
    |                        ^^^^^
-note: Capturing foo[(1, 0)] -> MutBorrow
+note: Capturing foo[] -> MutBorrow
   --> $DIR/repr_packed.rs:22:32
    |
 LL |         let z2: &mut u8 = &mut foo.y;
    |                                ^^^^^
 
 error: Min Capture analysis includes:
-  --> $DIR/repr_packed.rs:16:5
+  --> $DIR/repr_packed.rs:17:5
    |
 LL | /     || {
 LL | |
@@ -60,12 +60,7 @@ LL | |         println!("({}, {})", z1, z2);
 LL | |     };
    | |_____^
    |
-note: Min Capture foo[(0, 0)] -> ImmBorrow
-  --> $DIR/repr_packed.rs:19:24
-   |
-LL |         let z1: &u8 = &foo.x;
-   |                        ^^^^^
-note: Min Capture foo[(1, 0)] -> MutBorrow
+note: Min Capture foo[] -> MutBorrow
   --> $DIR/repr_packed.rs:22:32
    |
 LL |         let z2: &mut u8 = &mut foo.y;