Rollup merge of #77547 - RalfJung:stable-union-drop, r=matthewjasper

stabilize union with 'ManuallyDrop' fields and 'impl Drop for Union' As [discussed by @SimonSapin and @withoutboats](#55149 (comment)), this PR proposes to stabilize parts of the `untagged_union` feature gate: * It will be possible to have a union with field type `ManuallyDrop<T>` for any `T`. * While at it I propose we also stabilize `impl Drop for Union`; to my knowledge, there are no open concerns around this feature. In the RFC discussion, we also talked about allowing `&mut T` as another non-`Copy` non-dropping type, but that felt to me like an overly specific exception so I figured we'd wait if there is actually any use for such a special case. Some things remain unstable and still require the `untagged_union` feature gate: * Union with fields that do not drop, are not `Copy`, and are not `ManuallyDrop<_>`. The reason to not stabilize this is to avoid semver concerns around libraries adding `Drop` implementations later. (This is already not fully semver compatible as, to my knowledge, the borrow checker will exploit the non-dropping nature of any type, but it seems prudent to avoid further increasing the amount of trouble adding an `impl Drop` can cause.) Due to this, quite a few tests still need the `untagged_union` feature, but I think the ones where I could remove the feature flag provide good test coverage for the stable part. Cc @rust-lang/lang
rust-lang · Oct 16, 2020 · 3356ad7 · 3356ad7
2 parents 7581bb7 + defcd7f
commit 3356ad7
Show file tree

Hide file tree

Showing 33 changed files with 109 additions and 150 deletions.
diff --git a/compiler/rustc_passes/src/stability.rs b/compiler/rustc_passes/src/stability.rs
@@ -13,15 +13,13 @@ use rustc_hir::{Generics, HirId, Item, StructField, TraitRef, Ty, TyKind, Varian
 use rustc_middle::hir::map::Map;
 use rustc_middle::middle::privacy::AccessLevels;
 use rustc_middle::middle::stability::{DeprecationEntry, Index};
-use rustc_middle::ty::query::Providers;
-use rustc_middle::ty::TyCtxt;
+use rustc_middle::ty::{self, query::Providers, TyCtxt};
 use rustc_session::lint;
 use rustc_session::lint::builtin::INEFFECTIVE_UNSTABLE_TRAIT_IMPL;
 use rustc_session::parse::feature_err;
 use rustc_session::Session;
 use rustc_span::symbol::{sym, Symbol};
-use rustc_span::Span;
-use rustc_trait_selection::traits::misc::can_type_implement_copy;
+use rustc_span::{Span, DUMMY_SP};
 
 use std::cmp::Ordering;
 use std::mem::replace;
@@ -711,27 +709,35 @@ impl Visitor<'tcx> for Checker<'tcx> {
             // so semi-randomly perform it here in stability.rs
             hir::ItemKind::Union(..) if !self.tcx.features().untagged_unions => {
                 let def_id = self.tcx.hir().local_def_id(item.hir_id);
-                let adt_def = self.tcx.adt_def(def_id);
                 let ty = self.tcx.type_of(def_id);
+                let (adt_def, substs) = match ty.kind() {
+                    ty::Adt(adt_def, substs) => (adt_def, substs),
+                    _ => bug!(),
+                };
 
-                if adt_def.has_dtor(self.tcx) {
-                    feature_err(
-                        &self.tcx.sess.parse_sess,
-                        sym::untagged_unions,
-                        item.span,
-                        "unions with `Drop` implementations are unstable",
-                    )
-                    .emit();
-                } else {
-                    let param_env = self.tcx.param_env(def_id);
-                    if can_type_implement_copy(self.tcx, param_env, ty).is_err() {
-                        feature_err(
-                            &self.tcx.sess.parse_sess,
-                            sym::untagged_unions,
-                            item.span,
-                            "unions with non-`Copy` fields are unstable",
-                        )
-                        .emit();
+                // Non-`Copy` fields are unstable, except for `ManuallyDrop`.
+                let param_env = self.tcx.param_env(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();
+                        }
                     }
                 }
             }

diff --git a/compiler/rustc_typeck/src/check/check.rs b/compiler/rustc_typeck/src/check/check.rs
@@ -348,8 +348,7 @@ pub(super) fn check_union(tcx: TyCtxt<'_>, id: hir::HirId, span: Span) {
     check_packed(tcx, span, def);
 }
 
-/// When the `#![feature(untagged_unions)]` gate is active,
-/// check that the fields of the `union` does not contain fields that need dropping.
+/// Check that the fields of the `union` do not need dropping.
 pub(super) fn check_union_fields(tcx: TyCtxt<'_>, span: Span, item_def_id: LocalDefId) -> bool {
     let item_type = tcx.type_of(item_def_id);
     if let ty::Adt(def, substs) = item_type.kind() {

diff --git a/library/core/src/lib.rs b/library/core/src/lib.rs
@@ -131,7 +131,7 @@
 #![feature(transparent_unions)]
 #![feature(unboxed_closures)]
 #![feature(unsized_locals)]
-#![feature(untagged_unions)]
+#![cfg_attr(bootstrap, feature(untagged_unions))]
 #![feature(unwind_attributes)]
 #![feature(variant_count)]
 #![feature(tbm_target_feature)]

diff --git a/library/core/src/ptr/mod.rs b/library/core/src/ptr/mod.rs
@@ -229,6 +229,16 @@ pub(crate) struct FatPtr<T> {
     pub(crate) len: usize,
 }
 
+// Manual impl needed to avoid `T: Clone` bound.
+impl<T> Clone for FatPtr<T> {
+    fn clone(&self) -> Self {
+        *self
+    }
+}
+
+// Manual impl needed to avoid `T: Copy` bound.
+impl<T> Copy for FatPtr<T> {}
+
 /// Forms a raw slice from a pointer and a length.
 ///
 /// The `len` argument is the number of **elements**, not the number of bytes.

diff --git a/library/std/src/lib.rs b/library/std/src/lib.rs
@@ -319,7 +319,7 @@
 #![feature(unsafe_block_in_unsafe_fn)]
 #![feature(unsafe_cell_get_mut)]
 #![feature(unsafe_cell_raw_get)]
-#![feature(untagged_unions)]
+#![cfg_attr(bootstrap, feature(untagged_unions))]
 #![feature(unwind_attributes)]
 #![feature(vec_into_raw_parts)]
 #![feature(wake_trait)]

diff --git a/src/test/ui/did_you_mean/bad-assoc-ty.rs b/src/test/ui/did_you_mean/bad-assoc-ty.rs
@@ -68,7 +68,6 @@ enum N<F> where F: Fn() -> _ {
 
 union O<F> where F: Fn() -> _ {
 //~^ ERROR the type placeholder `_` is not allowed within types on item signatures
-//~| ERROR unions with non-`Copy` fields are unstable
     foo: F,
 }
 

diff --git a/src/test/ui/did_you_mean/bad-assoc-ty.stderr b/src/test/ui/did_you_mean/bad-assoc-ty.stderr
@@ -57,19 +57,6 @@ LL | type J = ty!(u8);
    |
    = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)
 
-error[E0658]: unions with non-`Copy` fields are unstable
-  --> $DIR/bad-assoc-ty.rs:69:1
-   |
-LL | / union O<F> where F: Fn() -> _ {
-LL | |
-LL | |
-LL | |     foo: F,
-LL | | }
-   | |_^
-   |
-   = note: see issue #55149 <https://github.com/rust-lang/rust/issues/55149> for more information
-   = help: add `#![feature(untagged_unions)]` to the crate attributes to enable
-
 error[E0223]: ambiguous associated type
   --> $DIR/bad-assoc-ty.rs:1:10
    |
@@ -215,7 +202,7 @@ LL | union O<F, T> where F: Fn() -> T {
    |          ^^^                   ^
 
 error[E0121]: the type placeholder `_` is not allowed within types on item signatures
-  --> $DIR/bad-assoc-ty.rs:75:29
+  --> $DIR/bad-assoc-ty.rs:74:29
    |
 LL | trait P<F> where F: Fn() -> _ {
    |                             ^ not allowed in type signatures
@@ -226,7 +213,7 @@ LL | trait P<F, T> where F: Fn() -> T {
    |          ^^^                   ^
 
 error[E0121]: the type placeholder `_` is not allowed within types on item signatures
-  --> $DIR/bad-assoc-ty.rs:80:38
+  --> $DIR/bad-assoc-ty.rs:79:38
    |
 LL |     fn foo<F>(_: F) where F: Fn() -> _ {}
    |                                      ^ not allowed in type signatures
@@ -236,7 +223,7 @@ help: use type parameters instead
 LL |     fn foo<F, T>(_: F) where F: Fn() -> T {}
    |             ^^^                         ^
 
-error: aborting due to 29 previous errors
+error: aborting due to 28 previous errors
 
-Some errors have detailed explanations: E0121, E0223, E0658.
+Some errors have detailed explanations: E0121, E0223.
 For more information about an error, try `rustc --explain E0121`.
diff --git a/src/test/ui/drop/dynamic-drop.rs b/src/test/ui/drop/dynamic-drop.rs
@@ -1,7 +1,7 @@
 // run-pass
 // ignore-wasm32-bare compiled with panic=abort by default
 
-#![feature(generators, generator_trait, untagged_unions)]
+#![feature(generators, generator_trait)]
 #![feature(bindings_after_at)]
 
 #![allow(unused_assignments)]

diff --git a/src/test/ui/dropck/dropck-union.rs b/src/test/ui/dropck/dropck-union.rs
@@ -1,5 +1,3 @@
-#![feature(untagged_unions)]
-
 use std::cell::Cell;
 use std::ops::Deref;
 use std::mem::ManuallyDrop;

diff --git a/src/test/ui/dropck/dropck-union.stderr b/src/test/ui/dropck/dropck-union.stderr
@@ -1,5 +1,5 @@
 error[E0597]: `v` does not live long enough
-  --> $DIR/dropck-union.rs:39:18
+  --> $DIR/dropck-union.rs:37:18
    |
 LL |     v.0.set(Some(&v));
    |                  ^^ borrowed value does not live long enough

diff --git a/src/test/ui/feature-gates/feature-gate-untagged_unions.rs b/src/test/ui/feature-gates/feature-gate-untagged_unions.rs
@@ -1,3 +1,5 @@
+// ignore-tidy-linelength
+
 union U1 { // OK
     a: u8,
 }
@@ -6,15 +8,23 @@ union U2<T: Copy> { // OK
     a: T,
 }
 
-union U3 { //~ ERROR unions with non-`Copy` fields are unstable
+union U22<T> { // OK
+    a: std::mem::ManuallyDrop<T>,
+}
+
+union U3 {
     a: String, //~ ERROR unions may not contain fields that need dropping
 }
 
-union U4<T> { //~ ERROR unions with non-`Copy` fields are unstable
+union U32 { // field that does not drop but is not `Copy`, either -- this is the real feature gate test!
+    a: std::cell::RefCell<i32>, //~ ERROR unions with non-`Copy` fields other than `ManuallyDrop<T>` are unstable
+}
+
+union U4<T> {
     a: T, //~ ERROR unions may not contain fields that need dropping
 }
 
-union U5 { //~ ERROR unions with `Drop` implementations are unstable
+union U5 { // Having a drop impl is OK
     a: u8,
 }
 

diff --git a/src/test/ui/feature-gates/feature-gate-untagged_unions.stderr b/src/test/ui/feature-gates/feature-gate-untagged_unions.stderr
@@ -1,61 +1,37 @@
-error[E0658]: unions with non-`Copy` fields are unstable
-  --> $DIR/feature-gate-untagged_unions.rs:9:1
+error[E0658]: unions with non-`Copy` fields other than `ManuallyDrop<T>` are unstable
+  --> $DIR/feature-gate-untagged_unions.rs:20:5
    |
-LL | / union U3 {
-LL | |     a: String,
-LL | | }
-   | |_^
-   |
-   = note: see issue #55149 <https://github.com/rust-lang/rust/issues/55149> for more information
-   = help: add `#![feature(untagged_unions)]` to the crate attributes to enable
-
-error[E0658]: unions with non-`Copy` fields are unstable
-  --> $DIR/feature-gate-untagged_unions.rs:13:1
-   |
-LL | / union U4<T> {
-LL | |     a: T,
-LL | | }
-   | |_^
-   |
-   = note: see issue #55149 <https://github.com/rust-lang/rust/issues/55149> for more information
-   = help: add `#![feature(untagged_unions)]` to the crate attributes to enable
-
-error[E0658]: unions with `Drop` implementations are unstable
-  --> $DIR/feature-gate-untagged_unions.rs:17:1
-   |
-LL | / union U5 {
-LL | |     a: u8,
-LL | | }
-   | |_^
+LL |     a: std::cell::RefCell<i32>,
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: see issue #55149 <https://github.com/rust-lang/rust/issues/55149> for more information
    = help: add `#![feature(untagged_unions)]` to the crate attributes to enable
 
 error[E0740]: unions may not contain fields that need dropping
-  --> $DIR/feature-gate-untagged_unions.rs:10:5
+  --> $DIR/feature-gate-untagged_unions.rs:16:5
    |
 LL |     a: String,
    |     ^^^^^^^^^
    |
 note: `std::mem::ManuallyDrop` can be used to wrap the type
-  --> $DIR/feature-gate-untagged_unions.rs:10:5
+  --> $DIR/feature-gate-untagged_unions.rs:16:5
    |
 LL |     a: String,
    |     ^^^^^^^^^
 
 error[E0740]: unions may not contain fields that need dropping
-  --> $DIR/feature-gate-untagged_unions.rs:14:5
+  --> $DIR/feature-gate-untagged_unions.rs:24:5
    |
 LL |     a: T,
    |     ^^^^
    |
 note: `std::mem::ManuallyDrop` can be used to wrap the type
-  --> $DIR/feature-gate-untagged_unions.rs:14:5
+  --> $DIR/feature-gate-untagged_unions.rs:24:5
    |
 LL |     a: T,
    |     ^^^^
 
-error: aborting due to 5 previous errors
+error: aborting due to 3 previous errors
 
 Some errors have detailed explanations: E0658, E0740.
 For more information about an error, try `rustc --explain E0658`.
diff --git a/src/test/ui/reject-specialized-drops-8142.stderr b/src/test/ui/reject-specialized-drops-8142.stderr
@@ -1,15 +1,3 @@
-error[E0367]: `Drop` impl requires `AddsBnd: Bound` but the union it is implemented for does not
-  --> $DIR/reject-specialized-drops-8142.rs:67:21
-   |
-LL | impl<AddsBnd:Copy + Bound> Drop for Union<AddsBnd> { fn drop(&mut self) { } } // REJECT
-   |                     ^^^^^
-   |
-note: the implementor must specify the same requirement
-  --> $DIR/reject-specialized-drops-8142.rs:21:1
-   |
-LL | union Union<T: Copy> { f: T }
-   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-
 error[E0367]: `Drop` impl requires `'adds_bnd: 'al` but the struct it is implemented for does not
   --> $DIR/reject-specialized-drops-8142.rs:23:20
    |
@@ -145,6 +133,18 @@ note: the implementor must specify the same requirement
 LL | struct TupleStruct<T>(T);
    | ^^^^^^^^^^^^^^^^^^^^^^^^^
 
+error[E0367]: `Drop` impl requires `AddsBnd: Bound` but the union it is implemented for does not
+  --> $DIR/reject-specialized-drops-8142.rs:67:21
+   |
+LL | impl<AddsBnd:Copy + Bound> Drop for Union<AddsBnd> { fn drop(&mut self) { } } // REJECT
+   |                     ^^^^^
+   |
+note: the implementor must specify the same requirement
+  --> $DIR/reject-specialized-drops-8142.rs:21:1
+   |
+LL | union Union<T: Copy> { f: T }
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
 error: aborting due to 11 previous errors
 
 Some errors have detailed explanations: E0308, E0366, E0367, E0495.

diff --git a/src/test/ui/self/self-in-typedefs.rs b/src/test/ui/self/self-in-typedefs.rs
@@ -1,7 +1,4 @@
 // build-pass (FIXME(62277): could be check-pass?)
-
-#![feature(untagged_unions)]
-
 #![allow(dead_code)]
 
 use std::mem::ManuallyDrop;

diff --git a/src/test/ui/transmute/main.rs b/src/test/ui/transmute/main.rs
@@ -1,9 +1,6 @@
 // normalize-stderr-32bit: "`&str` \(64 bits\)" -> "`&str` ($$STR bits)"
 // normalize-stderr-64bit: "`&str` \(128 bits\)" -> "`&str` ($$STR bits)"
 
-
-
-#![feature(untagged_unions)]
 use std::mem::transmute;
 
 pub trait TypeConstructor<'a> {

diff --git a/src/test/ui/transmute/main.stderr b/src/test/ui/transmute/main.stderr
@@ -1,13 +1,13 @@
 error[E0512]: cannot transmute between types of different sizes, or dependently-sized types
-  --> $DIR/main.rs:16:5
+  --> $DIR/main.rs:13:5
    |
 LL |     transmute(x)
    |     ^^^^^^^^^
    |
    = note: `<C as TypeConstructor>::T` does not have a fixed size
 
 error[E0512]: cannot transmute between types of different sizes, or dependently-sized types
-  --> $DIR/main.rs:20:17
+  --> $DIR/main.rs:17:17
    |
 LL |     let x: u8 = transmute(10u16);
    |                 ^^^^^^^^^
@@ -16,7 +16,7 @@ LL |     let x: u8 = transmute(10u16);
    = note: target type: `u8` (8 bits)
 
 error[E0512]: cannot transmute between types of different sizes, or dependently-sized types
-  --> $DIR/main.rs:24:17
+  --> $DIR/main.rs:21:17
    |
 LL |     let x: u8 = transmute("test");
    |                 ^^^^^^^^^
@@ -25,7 +25,7 @@ LL |     let x: u8 = transmute("test");
    = note: target type: `u8` (8 bits)
 
 error[E0512]: cannot transmute between types of different sizes, or dependently-sized types
-  --> $DIR/main.rs:29:18
+  --> $DIR/main.rs:26:18
    |
 LL |     let x: Foo = transmute(10);
    |                  ^^^^^^^^^
-Original file line number
+Diff line change
@@ Expand Up / @@ -68,7 +68,6 @@ enum N<F> where F: Fn() -> _ { @@
     union O<F> where F: Fn() -> _ {
     //~^ ERROR the type placeholder `_` is not allowed within types on item signatures
-    //~| ERROR unions with non-`Copy` fields are unstable
         foo: F,
     }
@@ Expand Down @@