Auto merge of #97995 - RalfJung:union-more-nodrop, r=Mark-Simulacrum

allow unions with mutable references and tuples of allowed types

We currently allow shared references in unions, but not mutable references. That seems somewhat inconsistent. So let's allow all references, and while we are at it, let's make sure the set of allowed types is closed under tuples.

This will need T-lang FCP (at least).

Then remove the `tagged_unions` feature, since we do not plan to stabilize any more of it.
Closes #55149

Author: bors

compiler/rustc_feature/src/active.rs

@@ -525,13 +525,6 @@ declare_features! (
     (incomplete, unsized_locals, "1.30.0", Some(48055), None),
     /// Allows unsized tuple coercion.
     (active, unsized_tuple_coercion, "1.20.0", Some(42877), None),
-    /// Allows `union`s to implement `Drop`. Moreover, `union`s may now include fields
-    /// that don't implement `Copy` as long as they don't have any drop glue.
-    /// This is checked recursively. On encountering type variable where no progress can be made,
-    /// `T: Copy` is used as a substitute for "no drop glue".
-    ///
-    /// NOTE: A limited form of `union U { ... }` was accepted in 1.19.0.
-    (active, untagged_unions, "1.13.0", Some(55149), None),
     /// Allows using the `#[used(linker)]` (or `#[used(compiler)]`) attribute.
     (active, used_with_arg, "1.60.0", Some(93798), None),
     /// Allows `extern "wasm" fn`

compiler/rustc_feature/src/removed.rs

@@ -180,6 +180,9 @@ declare_features! (
     /// Allows using items which are missing stability attributes
     (removed, unmarked_api, "1.0.0", None, None, None),
     (removed, unsafe_no_drop_flag, "1.0.0", None, None, None),
+    /// Allows `union` fields that don't implement `Copy` as long as they don't have any drop glue.
+    (removed, untagged_unions, "1.13.0", Some(55149), None,
+     Some("unions with `Copy` and `ManuallyDrop` fields are stable; there is no intent to stabilize more")),
     /// Allows `#[unwind(..)]`.
     ///
     /// Permits specifying whether a function should permit unwinding or abort on unwind.

compiler/rustc_middle/src/mir/query.rs

@@ -35,7 +35,6 @@ pub enum UnsafetyViolationDetails {
     UseOfMutableStatic,
     UseOfExternStatic,
     DerefOfRawPointer,
-    AssignToDroppingUnionField,
     AccessToUnionField,
     MutationOfLayoutConstrainedField,
     BorrowOfLayoutConstrainedField,
@@ -78,11 +77,6 @@ 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",
             ),
-            AssignToDroppingUnionField => (
-                "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",
-            ),
             AccessToUnionField => (
                 "access to union field",
                 "the field may not be properly initialized: using uninitialized data will cause \

compiler/rustc_mir_build/src/check_unsafety.rs

@@ -431,16 +431,9 @@ impl<'a, 'tcx> Visitor<'a, 'tcx> for UnsafetyVisitor<'a, 'tcx> {
                 let lhs = &self.thir[lhs];
                 if let ty::Adt(adt_def, _) = lhs.ty.kind() && adt_def.is_union() {
                     if let Some((assigned_ty, assignment_span)) = self.assignment_info {
-                        // To avoid semver hazard, we only consider `Copy` and `ManuallyDrop` non-dropping.
-                        if !(assigned_ty
-                            .ty_adt_def()
-                            .map_or(false, |adt| adt.is_manually_drop())
-                            || assigned_ty
-                                .is_copy_modulo_regions(self.tcx.at(expr.span), self.param_env))
-                        {
-                            self.requires_unsafe(assignment_span, AssignToDroppingUnionField);
-                        } else {
-                            // write to non-drop union field, safe
+                        if assigned_ty.needs_drop(self.tcx, self.tcx.param_env(adt_def.did())) {
+                            // This would be unsafe, but should be outright impossible since we reject such unions.
+                            self.tcx.sess.delay_span_bug(assignment_span, "union fields that need dropping should be impossible");
                         }
                     } else {
                         self.requires_unsafe(expr.span, AccessToUnionField);
@@ -537,7 +530,6 @@ enum UnsafeOpKind {
     UseOfMutableStatic,
     UseOfExternStatic,
     DerefOfRawPointer,
-    AssignToDroppingUnionField,
     AccessToUnionField,
     MutationOfLayoutConstrainedField,
     BorrowOfLayoutConstrainedField,
@@ -555,7 +547,6 @@ impl UnsafeOpKind {
             UseOfMutableStatic => "use of mutable static",
             UseOfExternStatic => "use of extern static",
             DerefOfRawPointer => "dereference of raw pointer",
-            AssignToDroppingUnionField => "assignment to union field that might need dropping",
             AccessToUnionField => "access to union field",
             MutationOfLayoutConstrainedField => "mutation of layout constrained field",
             BorrowOfLayoutConstrainedField => {
@@ -600,11 +591,6 @@ impl UnsafeOpKind {
                 "raw pointers may be null, dangling or unaligned; they can violate aliasing rules \
                  and cause data races: all of these are undefined behavior",
             ),
-            AssignToDroppingUnionField => (
-                Cow::Borrowed(self.simple_description()),
-                "the previous content of the field will be dropped, which causes undefined \
-                 behavior if the field was not properly initialized",
-            ),
             AccessToUnionField => (
                 Cow::Borrowed(self.simple_description()),
                 "the field may not be properly initialized: using uninitialized data will cause \

compiler/rustc_mir_transform/src/check_unsafety.rs

@@ -219,22 +219,15 @@ impl<'tcx> Visitor<'tcx> for UnsafetyChecker<'_, 'tcx> {
                     // 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 assigned_ty.needs_drop(
+                        self.tcx,
+                        self.tcx.param_env(base_ty.ty_adt_def().unwrap().did()),
+                    ) {
+                        // This would be unsafe, but should be outright impossible since we reject such unions.
+                        self.tcx.sess.delay_span_bug(
+                            self.source_info.span,
+                            "union fields that need dropping should be impossible",
                         );
-                    if !nodrop {
-                        self.require_unsafe(
-                            UnsafetyViolationKind::General,
-                            UnsafetyViolationDetails::AssignToDroppingUnionField,
-                        );
-                    } else {
-                        // write to non-drop union field, safe
                     }
                 } else {
                     self.require_unsafe(

compiler/rustc_passes/src/stability.rs

@@ -13,13 +13,12 @@ use rustc_hir::{FieldDef, Generics, HirId, Item, ItemKind, TraitRef, Ty, TyKind,
 use rustc_middle::hir::nested_filter;
 use rustc_middle::middle::privacy::AccessLevels;
 use rustc_middle::middle::stability::{AllowUnstable, DeprecationEntry, Index};
-use rustc_middle::ty::{self, query::Providers, TyCtxt};
+use rustc_middle::ty::{query::Providers, TyCtxt};
 use rustc_session::lint;
 use rustc_session::lint::builtin::{INEFFECTIVE_UNSTABLE_TRAIT_IMPL, USELESS_DEPRECATED};
-use rustc_session::parse::feature_err;
 use rustc_session::Session;
 use rustc_span::symbol::{sym, Symbol};
-use rustc_span::{Span, DUMMY_SP};
+use rustc_span::Span;
 use rustc_target::spec::abi::Abi;
 
 use std::cmp::Ordering;
@@ -766,39 +765,6 @@ impl<'tcx> Visitor<'tcx> for Checker<'tcx> {
                 }
             }
 
-            // There's no good place to insert stability check for non-Copy unions,
-            // so semi-randomly perform it here in stability.rs
-            hir::ItemKind::Union(..) if !self.tcx.features().untagged_unions => {
-                let ty = self.tcx.type_of(item.def_id);
-                let ty::Adt(adt_def, substs) = ty.kind() else { bug!() };
-
-                // Non-`Copy` fields are unstable, except for `ManuallyDrop`.
-                let param_env = self.tcx.param_env(item.def_id);
-                for field in &adt_def.non_enum_variant().fields {
-                    let field_ty = field.ty(self.tcx, substs);
-                    if !field_ty.ty_adt_def().map_or(false, |adt_def| adt_def.is_manually_drop())
-                        && !field_ty.is_copy_modulo_regions(self.tcx.at(DUMMY_SP), param_env)
-                    {
-                        if field_ty.needs_drop(self.tcx, param_env) {
-                            // Avoid duplicate error: This will error later anyway because fields
-                            // that need drop are not allowed.
-                            self.tcx.sess.delay_span_bug(
-                                item.span,
-                                "union should have been rejected due to potentially dropping field",
-                            );
-                        } else {
-                            feature_err(
-                                &self.tcx.sess.parse_sess,
-                                sym::untagged_unions,
-                                self.tcx.def_span(field.did),
-                                "unions with non-`Copy` fields other than `ManuallyDrop<T>` are unstable",
-                            )
-                            .emit();
-                        }
-                    }
-                }
-            }
-
             _ => (/* pass */),
         }
         intravisit::walk_item(self, item);

compiler/rustc_typeck/src/check/check.rs

@@ -402,11 +402,37 @@ fn check_union_fields(tcx: TyCtxt<'_>, span: Span, item_def_id: LocalDefId) -> b
     let item_type = tcx.type_of(item_def_id);
     if let ty::Adt(def, substs) = item_type.kind() {
         assert!(def.is_union());
-        let fields = &def.non_enum_variant().fields;
+
+        fn allowed_union_field<'tcx>(
+            ty: Ty<'tcx>,
+            tcx: TyCtxt<'tcx>,
+            param_env: ty::ParamEnv<'tcx>,
+            span: Span,
+        ) -> bool {
+            // We don't just accept all !needs_drop fields, due to semver concerns.
+            match ty.kind() {
+                ty::Ref(..) => true, // references never drop (even mutable refs, which are non-Copy and hence fail the later check)
+                ty::Tuple(tys) => {
+                    // allow tuples of allowed types
+                    tys.iter().all(|ty| allowed_union_field(ty, tcx, param_env, span))
+                }
+                ty::Array(elem, _len) => {
+                    // Like `Copy`, we do *not* special-case length 0.
+                    allowed_union_field(*elem, tcx, param_env, span)
+                }
+                _ => {
+                    // Fallback case: allow `ManuallyDrop` and things that are `Copy`.
+                    ty.ty_adt_def().is_some_and(|adt_def| adt_def.is_manually_drop())
+                        || ty.is_copy_modulo_regions(tcx.at(span), param_env)
+                }
+            }
+        }
+
         let param_env = tcx.param_env(item_def_id);
-        for field in fields {
+        for field in &def.non_enum_variant().fields {
             let field_ty = field.ty(tcx, substs);
-            if field_ty.needs_drop(tcx, param_env) {
+
+            if !allowed_union_field(field_ty, tcx, param_env, span) {
                 let (field_span, ty_span) = match tcx.hir().get_if_local(field.did) {
                     // We are currently checking the type this field came from, so it must be local.
                     Some(Node::Field(field)) => (field.span, field.ty.span),
@@ -433,6 +459,9 @@ fn check_union_fields(tcx: TyCtxt<'_>, span: Span, item_def_id: LocalDefId) -> b
                 )
                 .emit();
                 return false;
+            } else if field_ty.needs_drop(tcx, param_env) {
+                // This should never happen. But we can get here e.g. in case of name resolution errors.
+                tcx.sess.delay_span_bug(span, "we should never accept maybe-dropping union fields");
             }
         }
     } else {

compiler/rustc_typeck/src/lib.rs

@@ -72,6 +72,7 @@ This API is completely unstable and subject to change.
 #![feature(once_cell)]
 #![feature(slice_partition_dedup)]
 #![feature(try_blocks)]
+#![feature(is_some_with)]
 #![recursion_limit = "256"]
 
 #[macro_use]

src/test/ui/associated-type-bounds/duplicate.rs

@@ -1,8 +1,8 @@
 #![feature(associated_type_bounds)]
 #![feature(type_alias_impl_trait)]
-#![feature(untagged_unions)]
 
 use std::iter;
+use std::mem::ManuallyDrop;
 
 struct SI1<T: Iterator<Item: Copy, Item: Send>> {
     //~^ ERROR the value of the associated type `Item` (from trait `Iterator`) is already specified [E0719]
@@ -74,36 +74,36 @@ where
 
 union UI1<T: Iterator<Item: Copy, Item: Send>> {
     //~^ ERROR the value of the associated type `Item` (from trait `Iterator`) is already specified [E0719]
-    f: T,
+    f: ManuallyDrop<T>,
 }
 union UI2<T: Iterator<Item: Copy, Item: Copy>> {
     //~^ ERROR the value of the associated type `Item` (from trait `Iterator`) is already specified [E0719]
-    f: T,
+    f: ManuallyDrop<T>,
 }
 union UI3<T: Iterator<Item: 'static, Item: 'static>> {
     //~^ ERROR the value of the associated type `Item` (from trait `Iterator`) is already specified [E0719]
-    f: T,
+    f: ManuallyDrop<T>,
 }
 union UW1<T>
 where
     T: Iterator<Item: Copy, Item: Send>,
     //~^ ERROR the value of the associated type `Item` (from trait `Iterator`) is already specified [E0719]
 {
-    f: T,
+    f: ManuallyDrop<T>,
 }
 union UW2<T>
 where
     T: Iterator<Item: Copy, Item: Copy>,
     //~^ ERROR the value of the associated type `Item` (from trait `Iterator`) is already specified [E0719]
 {
-    f: T,
+    f: ManuallyDrop<T>,
 }
 union UW3<T>
 where
     T: Iterator<Item: 'static, Item: 'static>,
     //~^ ERROR the value of the associated type `Item` (from trait `Iterator`) is already specified [E0719]
 {
-    f: T,
+    f: ManuallyDrop<T>,
 }
 
 fn FI1<T: Iterator<Item: Copy, Item: Send>>() {}

src/test/ui/associated-type-bounds/inside-adt.rs

@@ -1,5 +1,6 @@
 #![feature(associated_type_bounds)]
-#![feature(untagged_unions)]
+
+use std::mem::ManuallyDrop;
 
 struct S1 { f: dyn Iterator<Item: Copy> }
 //~^ ERROR associated type bounds are not allowed within structs, enums, or unions
@@ -17,12 +18,12 @@ enum E3 { V(dyn Iterator<Item: 'static>) }
 //~^ ERROR associated type bounds are not allowed within structs, enums, or unions
 //~| ERROR the size for values of type `(dyn Iterator<Item = impl Sized> + 'static)`
 
-union U1 { f: dyn Iterator<Item: Copy> }
+union U1 { f: ManuallyDrop<dyn Iterator<Item: Copy>> }
 //~^ ERROR associated type bounds are not allowed within structs, enums, or unions
 //~| ERROR the size for values of type `(dyn Iterator<Item = impl Copy> + 'static)`
-union U2 { f: Box<dyn Iterator<Item: Copy>> }
+union U2 { f: ManuallyDrop<Box<dyn Iterator<Item: Copy>>> }
 //~^ ERROR associated type bounds are not allowed within structs, enums, or unions
-union U3 { f: dyn Iterator<Item: 'static> }
+union U3 { f: ManuallyDrop<dyn Iterator<Item: 'static>> }
 //~^ ERROR associated type bounds are not allowed within structs, enums, or unions
 //~| ERROR the size for values of type `(dyn Iterator<Item = impl Sized> + 'static)`