Auto merge of #78068 - RalfJung:union-safe-assign, r=nikomatsakis

consider assignments of union field of ManuallyDrop type safe

Assigning to `Copy` union fields is safe because that assignment will never drop anything. However, with https://github.com/rust-lang/rust/pull/77547, unions may also have `ManuallyDrop` fields, and their assignments are currently still unsafe. That seems unnecessary though, as assigning `ManuallyDrop` does not drop anything either, and is thus safe even for union fields.

I assume this will at least require FCP.

compiler/rustc_middle/src/mir/mod.rs

@@ -1756,6 +1756,21 @@ impl<'tcx> Place<'tcx> {
     pub fn as_ref(&self) -> PlaceRef<'tcx> {
         PlaceRef { local: self.local, projection: &self.projection }
     }
+
+    /// Iterate over the projections in evaluation order, i.e., the first element is the base with
+    /// its projection and then subsequently more projections are added.
+    /// As a concrete example, given the place a.b.c, this would yield:
+    /// - (a, .b)
+    /// - (a.b, .c)
+    /// Given a place without projections, the iterator is empty.
+    pub fn iter_projections(
+        self,
+    ) -> impl Iterator<Item = (PlaceRef<'tcx>, PlaceElem<'tcx>)> + DoubleEndedIterator {
+        self.projection.iter().enumerate().map(move |(i, proj)| {
+            let base = PlaceRef { local: self.local, projection: &self.projection[..i] };
+            (base, proj)
+        })
+    }
 }
 
 impl From<Local> for Place<'_> {

compiler/rustc_middle/src/mir/query.rs

@@ -46,7 +46,7 @@ pub enum UnsafetyViolationDetails {
     UseOfMutableStatic,
     UseOfExternStatic,
     DerefOfRawPointer,
-    AssignToNonCopyUnionField,
+    AssignToDroppingUnionField,
     AccessToUnionField,
     MutationOfLayoutConstrainedField,
     BorrowOfLayoutConstrainedField,
@@ -94,8 +94,8 @@ impl UnsafetyViolationDetails {
                 "raw pointers may be NULL, dangling or unaligned; they can violate aliasing rules \
                  and cause data races: all of these are undefined behavior",
             ),
-            AssignToNonCopyUnionField => (
+            AssignToDroppingUnionField => (
-                "assignment to non-`Copy` union field",
+                "assignment to union field that might need dropping",
                 "the previous content of the field will be dropped, which causes undefined \
                  behavior if the field was not properly initialized",
             ),

compiler/rustc_middle/src/mir/tcx.rs

@@ -136,6 +136,15 @@ impl<'tcx> Place<'tcx> {
     }
 }
 
+impl<'tcx> PlaceRef<'tcx> {
+    pub fn ty<D>(&self, local_decls: &D, tcx: TyCtxt<'tcx>) -> PlaceTy<'tcx>
+    where
+        D: HasLocalDecls<'tcx>,
+    {
+        Place::ty_from(self.local, &self.projection, local_decls, tcx)
+    }
+}
+
 pub enum RvalueInitializationState {
     Shallow,
     Deep,

compiler/rustc_mir/src/transform/check_unsafety.rs

@@ -181,6 +181,9 @@ impl<'a, 'tcx> Visitor<'tcx> for UnsafetyChecker<'a, 'tcx> {
             self.check_mut_borrowing_layout_constrained_field(*place, context.is_mutating_use());
         }
 
+        // Check for borrows to packed fields.
+        // `is_disaligned` already traverses the place to consider all projections after the last
+        // `Deref`, so this only needs to be called once at the top level.
         if context.is_borrow() {
             if util::is_disaligned(self.tcx, self.body, self.param_env, *place) {
                 self.require_unsafe(
@@ -190,87 +193,105 @@ impl<'a, 'tcx> Visitor<'tcx> for UnsafetyChecker<'a, 'tcx> {
             }
         }
 
-        for (i, elem) in place.projection.iter().enumerate() {
+        // Some checks below need the extra metainfo of the local declaration.
-            let proj_base = &place.projection[..i];
+        let decl = &self.body.local_decls[place.local];
-            if context.is_borrow() {
+
-                if util::is_disaligned(self.tcx, self.body, self.param_env, *place) {
+        // Check the base local: it might be an unsafe-to-access static. We only check derefs of the
+        // temporary holding the static pointer to avoid duplicate errors
+        // <https://github.com/rust-lang/rust/pull/78068#issuecomment-731753506>.
+        if decl.internal && place.projection.first() == Some(&ProjectionElem::Deref) {
+            // If the projection root is an artifical local that we introduced when
+            // desugaring `static`, give a more specific error message
+            // (avoid the general "raw pointer" clause below, that would only be confusing).
+            if let Some(box LocalInfo::StaticRef { def_id, .. }) = decl.local_info {
+                if self.tcx.is_mutable_static(def_id) {
                     self.require_unsafe(
-                        UnsafetyViolationKind::BorrowPacked,
+                        UnsafetyViolationKind::General,
-                        UnsafetyViolationDetails::BorrowOfPackedField,
+                        UnsafetyViolationDetails::UseOfMutableStatic,
                     );
+                    return;
+                } else if self.tcx.is_foreign_item(def_id) {
+                    self.require_unsafe(
+                        UnsafetyViolationKind::General,
+                        UnsafetyViolationDetails::UseOfExternStatic,
+                    );
+                    return;
                 }
             }
-            let source_info = self.source_info;
+        }
-            if let [] = proj_base {
+
-                let decl = &self.body.local_decls[place.local];
+        // Check for raw pointer `Deref`.
-                if decl.internal {
+        for (base, proj) in place.iter_projections() {
-                    // If the projection root is an artifical local that we introduced when
+            if proj == ProjectionElem::Deref {
-                    // desugaring `static`, give a more specific error message
+                let source_info = self.source_info; // Backup source_info so we can restore it later.
-                    // (avoid the general "raw pointer" clause below, that would only be confusing).
+                if base.projection.is_empty() && decl.internal {
-                    if let Some(box LocalInfo::StaticRef { def_id, .. }) = decl.local_info {
+                    // Internal locals are used in the `move_val_init` desugaring.
-                        if self.tcx.is_mutable_static(def_id) {
+                    // We want to check unsafety against the source info of the
-                            self.require_unsafe(
+                    // desugaring, rather than the source info of the RHS.
-                                UnsafetyViolationKind::General,
+                    self.source_info = self.body.local_decls[place.local].source_info;
-                                UnsafetyViolationDetails::UseOfMutableStatic,
+                }
-                            );
+                let base_ty = base.ty(self.body, self.tcx).ty;
-                            return;
+                if base_ty.is_unsafe_ptr() {
-                        } else if self.tcx.is_foreign_item(def_id) {
+                    self.require_unsafe(
-                            self.require_unsafe(
+                        UnsafetyViolationKind::GeneralAndConstFn,
-                                UnsafetyViolationKind::General,
+                        UnsafetyViolationDetails::DerefOfRawPointer,
-                                UnsafetyViolationDetails::UseOfExternStatic,
+                    )
-                            );
+                }
-                            return;
+                self.source_info = source_info; // Restore backed-up source_info.
-                        }
+            }
+        }
+
+        // Check for union fields. For this we traverse right-to-left, as the last `Deref` changes
+        // whether we *read* the union field or potentially *write* to it (if this place is being assigned to).
+        let mut saw_deref = false;
+        for (base, proj) in place.iter_projections().rev() {
+            if proj == ProjectionElem::Deref {
+                saw_deref = true;
+                continue;
+            }
+
+            let base_ty = base.ty(self.body, self.tcx).ty;
+            if base_ty.ty_adt_def().map_or(false, |adt| adt.is_union()) {
+                // If we did not hit a `Deref` yet and the overall place use is an assignment, the
+                // rules are different.
+                let assign_to_field = !saw_deref
+                    && matches!(
+                        context,
+                        PlaceContext::MutatingUse(
+                            MutatingUseContext::Store
+                                | MutatingUseContext::Drop
+                                | MutatingUseContext::AsmOutput
+                        )
+                    );
+                // If this is just an assignment, determine if the assigned type needs dropping.
+                if assign_to_field {
+                    // We have to check the actual type of the assignment, as that determines if the
+                    // old value is being dropped.
+                    let assigned_ty = place.ty(&self.body.local_decls, self.tcx).ty;
+                    // To avoid semver hazard, we only consider `Copy` and `ManuallyDrop` non-dropping.
+                    let manually_drop = assigned_ty
+                        .ty_adt_def()
+                        .map_or(false, |adt_def| adt_def.is_manually_drop());
+                    let nodrop = manually_drop
+                        || assigned_ty.is_copy_modulo_regions(
+                            self.tcx.at(self.source_info.span),
+                            self.param_env,
+                        );
+                    if !nodrop {
+                        self.require_unsafe(
+                            UnsafetyViolationKind::GeneralAndConstFn,
+                            UnsafetyViolationDetails::AssignToDroppingUnionField,
+                        );
                     } else {
-                        // Internal locals are used in the `move_val_init` desugaring.
+                        // write to non-drop union field, safe
-                        // We want to check unsafety against the source info of the
-                        // desugaring, rather than the source info of the RHS.
-                        self.source_info = self.body.local_decls[place.local].source_info;
                     }
+                } else {
+                    self.require_unsafe(
+                        UnsafetyViolationKind::GeneralAndConstFn,
+                        UnsafetyViolationDetails::AccessToUnionField,
+                    )
                 }
             }
-            let base_ty = Place::ty_from(place.local, proj_base, self.body, self.tcx).ty;
-            match base_ty.kind() {
-                ty::RawPtr(..) => self.require_unsafe(
-                    UnsafetyViolationKind::GeneralAndConstFn,
-                    UnsafetyViolationDetails::DerefOfRawPointer,
-                ),
-                ty::Adt(adt, _) => {
-                    if adt.is_union() {
-                        if context == PlaceContext::MutatingUse(MutatingUseContext::Store)
-                            || context == PlaceContext::MutatingUse(MutatingUseContext::Drop)
-                            || context == PlaceContext::MutatingUse(MutatingUseContext::AsmOutput)
-                        {
-                            let elem_ty = match elem {
-                                ProjectionElem::Field(_, ty) => ty,
-                                _ => span_bug!(
-                                    self.source_info.span,
-                                    "non-field projection {:?} from union?",
-                                    place
-                                ),
-                            };
-                            if !elem_ty.is_copy_modulo_regions(
-                                self.tcx.at(self.source_info.span),
-                                self.param_env,
-                            ) {
-                                self.require_unsafe(
-                                    UnsafetyViolationKind::GeneralAndConstFn,
-                                    UnsafetyViolationDetails::AssignToNonCopyUnionField,
-                                )
-                            } else {
-                                // write to non-move union, safe
-                            }
-                        } else {
-                            self.require_unsafe(
-                                UnsafetyViolationKind::GeneralAndConstFn,
-                                UnsafetyViolationDetails::AccessToUnionField,
-                            )
-                        }
-                    }
-                }
-                _ => {}
-            }
-            self.source_info = source_info;
         }
     }
 }

src/test/ui/union/union-unsafe.rs

@@ -1,4 +1,6 @@
+#![feature(untagged_unions)]
 use std::mem::ManuallyDrop;
+use std::cell::RefCell;
 
 union U1 {
     a: u8
@@ -16,9 +18,28 @@ union U4<T: Copy> {
     a: T
 }
 
+union URef {
+    p: &'static mut i32,
+}
+
+union URefCell { // field that does not drop but is not `Copy`, either
+    a: (RefCell<i32>, i32),
+}
+
+fn deref_union_field(mut u: URef) {
+    // Not an assignment but an access to the union field!
+    *(u.p) = 13; //~ ERROR access to union field is unsafe
+}
+
+fn assign_noncopy_union_field(mut u: URefCell) {
+    u.a = (RefCell::new(0), 1); //~ ERROR assignment to union field that might need dropping
+    u.a.0 = RefCell::new(0); //~ ERROR assignment to union field that might need dropping
+    u.a.1 = 1; // OK
+}
+
 fn generic_noncopy<T: Default>() {
     let mut u3 = U3 { a: ManuallyDrop::new(T::default()) };
-    u3.a = ManuallyDrop::new(T::default()); //~ ERROR assignment to non-`Copy` union field is unsafe
+    u3.a = ManuallyDrop::new(T::default()); // OK (assignment does not drop)
     *u3.a = T::default(); //~ ERROR access to union field is unsafe
 }
 
@@ -41,7 +62,7 @@ fn main() {
     // let U1 { .. } = u1; // OK
 
     let mut u2 = U2 { a: ManuallyDrop::new(String::from("old")) }; // OK
-    u2.a = ManuallyDrop::new(String::from("new")); //~ ERROR assignment to non-`Copy` union
+    u2.a = ManuallyDrop::new(String::from("new")); // OK (assignment does not drop)
     *u2.a = String::from("new"); //~ ERROR access to union field is unsafe
 
     let mut u3 = U3 { a: ManuallyDrop::new(0) }; // OK
@@ -49,6 +70,6 @@ fn main() {
     *u3.a = 1; //~ ERROR access to union field is unsafe
 
     let mut u3 = U3 { a: ManuallyDrop::new(String::from("old")) }; // OK
-    u3.a = ManuallyDrop::new(String::from("new")); //~ ERROR assignment to non-`Copy` union
+    u3.a = ManuallyDrop::new(String::from("new")); // OK (assignment does not drop)
     *u3.a = String::from("new"); //~ ERROR access to union field is unsafe
 }

src/test/ui/union/union-unsafe.stderr

@@ -1,85 +1,85 @@
-error[E0133]: assignment to non-`Copy` union field is unsafe and requires unsafe function or block
+error[E0133]: access to union field is unsafe and requires unsafe function or block
-  --> $DIR/union-unsafe.rs:21:5
+  --> $DIR/union-unsafe.rs:31:5
    |
-LL |     u3.a = ManuallyDrop::new(T::default());
+LL |     *(u.p) = 13;
-   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ assignment to non-`Copy` union field
+   |     ^^^^^^^^^^^ access to union field
+   |
+   = note: the field may not be properly initialized: using uninitialized data will cause undefined behavior
+
+error[E0133]: assignment to union field that might need dropping is unsafe and requires unsafe function or block
+  --> $DIR/union-unsafe.rs:35:5
+   |
+LL |     u.a = (RefCell::new(0), 1);
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^ assignment to union field that might need dropping
+   |
+   = note: the previous content of the field will be dropped, which causes undefined behavior if the field was not properly initialized
+
+error[E0133]: assignment to union field that might need dropping is unsafe and requires unsafe function or block
+  --> $DIR/union-unsafe.rs:36:5
+   |
+LL |     u.a.0 = RefCell::new(0);
+   |     ^^^^^^^^^^^^^^^^^^^^^^^ assignment to union field that might need dropping
    |
    = note: the previous content of the field will be dropped, which causes undefined behavior if the field was not properly initialized
 
 error[E0133]: access to union field is unsafe and requires unsafe function or block
-  --> $DIR/union-unsafe.rs:22:6
+  --> $DIR/union-unsafe.rs:43:6
    |
 LL |     *u3.a = T::default();
    |      ^^^^ access to union field
    |
    = note: the field may not be properly initialized: using uninitialized data will cause undefined behavior
 
-error[E0133]: access to union field is unsafe and requires unsafe function or block
-  --> $DIR/union-unsafe.rs:28:6
-   |
-LL |     *u3.a = T::default();
-   |      ^^^^ access to union field
-   |
-   = note: the field may not be properly initialized: using uninitialized data will cause undefined behavior
-
-error[E0133]: access to union field is unsafe and requires unsafe function or block
-  --> $DIR/union-unsafe.rs:36:13
-   |
-LL |     let a = u1.a;
-   |             ^^^^ access to union field
-   |
-   = note: the field may not be properly initialized: using uninitialized data will cause undefined behavior
-
-error[E0133]: access to union field is unsafe and requires unsafe function or block
-  --> $DIR/union-unsafe.rs:39:14
-   |
-LL |     let U1 { a } = u1;
-   |              ^ access to union field
-   |
-   = note: the field may not be properly initialized: using uninitialized data will cause undefined behavior
-
-error[E0133]: access to union field is unsafe and requires unsafe function or block
-  --> $DIR/union-unsafe.rs:40:20
-   |
-LL |     if let U1 { a: 12 } = u1 {}
-   |                    ^^ access to union field
-   |
-   = note: the field may not be properly initialized: using uninitialized data will cause undefined behavior
-
-error[E0133]: assignment to non-`Copy` union field is unsafe and requires unsafe function or block
-  --> $DIR/union-unsafe.rs:44:5
-   |
-LL |     u2.a = ManuallyDrop::new(String::from("new"));
-   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ assignment to non-`Copy` union field
-   |
-   = note: the previous content of the field will be dropped, which causes undefined behavior if the field was not properly initialized
-
-error[E0133]: access to union field is unsafe and requires unsafe function or block
-  --> $DIR/union-unsafe.rs:45:6
-   |
-LL |     *u2.a = String::from("new");
-   |      ^^^^ access to union field
-   |
-   = note: the field may not be properly initialized: using uninitialized data will cause undefined behavior
-
 error[E0133]: access to union field is unsafe and requires unsafe function or block
   --> $DIR/union-unsafe.rs:49:6
    |
+LL |     *u3.a = T::default();
+   |      ^^^^ access to union field
+   |
+   = note: the field may not be properly initialized: using uninitialized data will cause undefined behavior
+
+error[E0133]: access to union field is unsafe and requires unsafe function or block
+  --> $DIR/union-unsafe.rs:57:13
+   |
+LL |     let a = u1.a;
+   |             ^^^^ access to union field
+   |
+   = note: the field may not be properly initialized: using uninitialized data will cause undefined behavior
+
+error[E0133]: access to union field is unsafe and requires unsafe function or block
+  --> $DIR/union-unsafe.rs:60:14
+   |
+LL |     let U1 { a } = u1;
+   |              ^ access to union field
+   |
+   = note: the field may not be properly initialized: using uninitialized data will cause undefined behavior
+
+error[E0133]: access to union field is unsafe and requires unsafe function or block
+  --> $DIR/union-unsafe.rs:61:20
+   |
+LL |     if let U1 { a: 12 } = u1 {}
+   |                    ^^ access to union field
+   |
+   = note: the field may not be properly initialized: using uninitialized data will cause undefined behavior
+
+error[E0133]: access to union field is unsafe and requires unsafe function or block
+  --> $DIR/union-unsafe.rs:66:6
+   |
+LL |     *u2.a = String::from("new");
+   |      ^^^^ access to union field
+   |
+   = note: the field may not be properly initialized: using uninitialized data will cause undefined behavior
+
+error[E0133]: access to union field is unsafe and requires unsafe function or block
+  --> $DIR/union-unsafe.rs:70:6
+   |
 LL |     *u3.a = 1;
    |      ^^^^ access to union field
    |
    = note: the field may not be properly initialized: using uninitialized data will cause undefined behavior
 
-error[E0133]: assignment to non-`Copy` union field is unsafe and requires unsafe function or block
-  --> $DIR/union-unsafe.rs:52:5
-   |
-LL |     u3.a = ManuallyDrop::new(String::from("new"));
-   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ assignment to non-`Copy` union field
-   |
-   = note: the previous content of the field will be dropped, which causes undefined behavior if the field was not properly initialized
-
 error[E0133]: access to union field is unsafe and requires unsafe function or block
-  --> $DIR/union-unsafe.rs:53:6
+  --> $DIR/union-unsafe.rs:74:6
    |
 LL |     *u3.a = String::from("new");
    |      ^^^^ access to union field