adjust union access unsafety check logic to take into account Deref and the actual type of the assignment

Author: RalfJung

compiler/rustc_mir/src/transform/check_unsafety.rs

@@ -190,7 +190,7 @@ impl<'a, 'tcx> Visitor<'tcx> for UnsafetyChecker<'a, 'tcx> {
             }
         }
 
-        for (i, elem) in place.projection.iter().enumerate() {
+        for (i, _elem) in place.projection.iter().enumerate() {
             let proj_base = &place.projection[..i];
             if context.is_borrow() {
                 if util::is_disaligned(self.tcx, self.body, self.param_env, *place) {
@@ -236,31 +236,36 @@ impl<'a, 'tcx> Visitor<'tcx> for UnsafetyChecker<'a, 'tcx> {
                     UnsafetyViolationDetails::DerefOfRawPointer,
                 ),
                 ty::Adt(adt, _) if adt.is_union() => {
-                    if context == PlaceContext::MutatingUse(MutatingUseContext::Store)
+                    let assign_to_field = 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
-                            ),
-                        };
-                        let manually_drop = elem_ty
+                        || context == PlaceContext::MutatingUse(MutatingUseContext::AsmOutput);
+                    // If there is a `Deref` further along the projection chain, this is *not* an
+                    // assignment to a union field. In that case the union field is just read to
+                    // obtain the pointer/reference.
+                    let assign_to_field = assign_to_field
+                        && !place.projection[i..]
+                            .iter()
+                            .any(|elem| matches!(elem, ProjectionElem::Deref));
+                    // 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
-                            || elem_ty.is_copy_modulo_regions(
+                            || 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 {
                             // write to non-drop union field, safe
                         }

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,6 +18,25 @@ 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 needs dropping
+    u.a.0 = RefCell::new(0); //~ ERROR assignment to union field that needs 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()); // OK (assignment does not drop)

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

@@ -1,67 +1,91 @@
 error[E0133]: access to union field is unsafe and requires unsafe function or block
-  --> $DIR/union-unsafe.rs:22:6
+  --> $DIR/union-unsafe.rs:31:5
+   |
+LL |     *(u.p) = 13;
+   |     ^^^^^^^^^^^ 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 needs 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 needs 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 needs 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 needs 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: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
+  --> $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:36:13
+  --> $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:39:14
+  --> $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:40:20
+  --> $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:45:6
+  --> $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:49:6
+  --> $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]: 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
    |
    = note: the field may not be properly initialized: using uninitialized data will cause undefined behavior
 
-error: aborting due to 8 previous errors
+error: aborting due to 11 previous errors
 
 For more information about this error, try `rustc --explain E0133`.