stabilize union with 'ManuallyDrop' fields and 'impl Drop for Union' #77547

RalfJung · 2020-10-04T20:56:54Z

As discussed by @SimonSapin and @withoutboats, 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-highfive · 2020-10-04T20:56:58Z

r? @matthewjasper

(rust_highfive has picked a reviewer for you, use r? to override)

scottmcm · 2020-10-05T00:24:53Z

We've had ~~enums~~unions of Copy types stable for a while, but non-Copy was an open question largely about the potential non-obviousness about drop glue not happening. This proposes to sidestep that question by allowing ManuallyDrop<T> in unions for all Ts, since that way people visibly opt-in to the leaking behaviour. And that's also safe, so creating a union remains safe, even if that union has a non-drop type in it (via ManuallyDrop).

This also proposes allowing Drop implementations on union types, which seems hard to me to actually use, but I don't really have a problem with allowing if someone can find a way that they'd care.

See the OP here for more details.

@rfcbot fcp merge

--

@RalfJung Would this also be enough to remove #![feature(untagged_unions)] from at least core and std? I did a quick rg and everything looked like it's either using Copy types or is already doing ManuallyDrop.

rfcbot · 2020-10-05T00:24:54Z

Team member @scottmcm has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

RalfJung · 2020-10-05T07:21:46Z

We've had enums of Copy types stable for a while

s/enums/unions/

Would this also be enough to remove #![feature(untagged_unions)] from at least core and std? I did a quick rg and everything looked like it's either using Copy types or is already doing ManuallyDrop.

Good point, done -- except for the one in stdarch.

rfcbot · 2020-10-05T20:01:57Z

🔔 This is now entering its final comment period, as per the review above. 🔔

rfcbot · 2020-10-15T20:02:38Z

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

bors · 2020-10-16T13:49:15Z

📌 Commit defcd7f has been approved by matthewjasper

@SimonSapin

…ewjasper stabilize union with 'ManuallyDrop' fields and 'impl Drop for Union' As [discussed by @SimonSapin and @withoutboats](rust-lang#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

@SimonSapin

…ewjasper stabilize union with 'ManuallyDrop' fields and 'impl Drop for Union' As [discussed by @SimonSapin and @withoutboats](rust-lang#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

Rollup of 10 pull requests Successful merges: - rust-lang#75209 (Suggest imports of unresolved macros) - rust-lang#77547 (stabilize union with 'ManuallyDrop' fields and 'impl Drop for Union') - rust-lang#77827 (Don't link to nightly primitives on stable channel) - rust-lang#77855 (resolve: further improvements to "try using the enum's variant" diagnostic) - rust-lang#77900 (Use fdatasync for File::sync_data on more OSes) - rust-lang#77925 (Suggest minimal subset features in `incomplete_features` lint) - rust-lang#77971 (Deny broken intra-doc links in linkchecker) - rust-lang#77991 (Bump backtrace-rs) - rust-lang#77992 (instrument-coverage: try our best to not ICE) - rust-lang#78013 (Fix sidebar scroll on mobile devices) Failed merges: r? `@ghost`

ehuss · 2020-10-18T02:11:19Z

@RalfJung Should this be documented in the reference? It doesn't appear to mention the Copy restriction (and, confusingly, it seems to imply that non-Copy fields can be used with unsafe).

RalfJung · 2020-10-18T09:16:13Z

@ehuss I suppose yes this should be documented -- I am not very fimiliar with how the reference is maintained.

RalfJung · 2020-10-18T10:01:11Z

confusingly, it seems to imply that non-Copy fields can be used with unsafe).

No, it says that writing to Copy fields is safe.

Since transmutes can cause unexpected or undefined behaviour, unsafe is
required to read from a union field or to write to a field that doesn't
implement [Copy]

RalfJung · 2020-10-18T10:10:13Z

However, that seems unnecessary for ManuallyDrop fields... I should probably submit a PR to make their assignment safe as well.

…sakis 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 rust-lang#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.

Pkgsrc changes: * Adjust patches, convert tabs to spaces so that tests pass. * Remove patches which are no longer needed (upstream changed) * Minor adjustments for SunOS, e.g. disable stack probes. * Adjust cargo checksum patching accordingly. * Remove commented-out use of PATCHELF on NetBSD, which doesn't work anyway... Upstream changes: Version 1.49.0 (2020-12-31) ============================ Language ----------------------- - [Unions can now implement `Drop`, and you can now have a field in a union with `ManuallyDrop<T>`.][77547] - [You can now cast uninhabited enums to integers.][76199] - [You can now bind by reference and by move in patterns.][76119] This allows you to selectively borrow individual components of a type. E.g. ```rust #[derive(Debug)] struct Person { name: String, age: u8, } let person = Person { name: String::from("Alice"), age: 20, }; // `name` is moved out of person, but `age` is referenced. let Person { name, ref age } = person; println!("{} {}", name, age); ``` Compiler ----------------------- - [Added tier 1\* support for `aarch64-unknown-linux-gnu`.][78228] - [Added tier 2 support for `aarch64-apple-darwin`.][75991] - [Added tier 2 support for `aarch64-pc-windows-msvc`.][75914] - [Added tier 3 support for `mipsel-unknown-none`.][78676] - [Raised the minimum supported LLVM version to LLVM 9.][78848] - [Output from threads spawned in tests is now captured.][78227] - [Change os and vendor values to "none" and "unknown" for some targets][78951] \* Refer to Rust's [platform support page][forge-platform-support] for more information on Rust's tiered platform support. Libraries ----------------------- - [`RangeInclusive` now checks for exhaustion when calling `contains` and indexing.][78109] - [`ToString::to_string` now no longer shrinks the internal buffer in the default implementation.][77997] - [`ops::{Index, IndexMut}` are now implemented for fixed sized arrays of any length.][74989] Stabilized APIs --------------- - [`slice::select_nth_unstable`] - [`slice::select_nth_unstable_by`] - [`slice::select_nth_unstable_by_key`] The following previously stable methods are now `const`. - [`Poll::is_ready`] - [`Poll::is_pending`] Cargo ----------------------- - [Building a crate with `cargo-package` should now be independently reproducible.][cargo/8864] - [`cargo-tree` now marks proc-macro crates.][cargo/8765] - [Added `CARGO_PRIMARY_PACKAGE` build-time environment variable.] [cargo/8758] This variable will be set if the crate being built is one the user selected to build, either with `-p` or through defaults. - [You can now use glob patterns when specifying packages & targets.][cargo/8752] Compatibility Notes ------------------- - [Demoted `i686-unknown-freebsd` from host tier 2 to target tier 2 support.][78746] - [Macros that end with a semi-colon are now treated as statements even if they expand to nothing.][78376] - [Rustc will now check for the validity of some built-in attributes on enum variants.][77015] Previously such invalid or unused attributes could be ignored. - Leading whitespace is stripped more uniformly in documentation comments, which may change behavior. You read [this post about the changes][rustdoc-ws-post] for more details. - [Trait bounds are no longer inferred for associated types.][79904] Internal Only ------------- These changes provide no direct user facing benefits, but represent significant improvements to the internals and overall performance of rustc and related tools. - [rustc's internal crates are now compiled using the `initial-exec` Thread Local Storage model.][78201] - [Calculate visibilities once in resolve.][78077] - [Added `system` to the `llvm-libunwind` bootstrap config option.][77703] - [Added `--color` for configuring terminal color support to bootstrap.][79004] [75991]: rust-lang/rust#75991 [78951]: rust-lang/rust#78951 [78848]: rust-lang/rust#78848 [78746]: rust-lang/rust#78746 [78376]: rust-lang/rust#78376 [78228]: rust-lang/rust#78228 [78227]: rust-lang/rust#78227 [78201]: rust-lang/rust#78201 [78109]: rust-lang/rust#78109 [78077]: rust-lang/rust#78077 [77997]: rust-lang/rust#77997 [77703]: rust-lang/rust#77703 [77547]: rust-lang/rust#77547 [77015]: rust-lang/rust#77015 [76199]: rust-lang/rust#76199 [76119]: rust-lang/rust#76119 [75914]: rust-lang/rust#75914 [74989]: rust-lang/rust#74989 [79004]: rust-lang/rust#79004 [78676]: rust-lang/rust#78676 [79904]: rust-lang/rust#79904 [cargo/8864]: rust-lang/cargo#8864 [cargo/8765]: rust-lang/cargo#8765 [cargo/8758]: rust-lang/cargo#8758 [cargo/8752]: rust-lang/cargo#8752 [`slice::select_nth_unstable`]: https://doc.rust-lang.org/nightly/std/primitive.slice.html#method.select_nth_unstable [`slice::select_nth_unstable_by`]: https://doc.rust-lang.org/nightly/std/primitive.slice.html#method.select_nth_unstable_by [`slice::select_nth_unstable_by_key`]: https://doc.rust-lang.org/nightly/std/primitive.slice.html#method.select_nth_unstable_by_key [`hint::spin_loop`]: https://doc.rust-lang.org/stable/std/hint/fn.spin_loop.html [`Poll::is_ready`]: https://doc.rust-lang.org/stable/std/task/enum.Poll.html#method.is_ready [`Poll::is_pending`]: https://doc.rust-lang.org/stable/std/task/enum.Poll.html#method.is_pending [rustdoc-ws-post]: https://blog.guillaume-gomez.fr/articles/2020-11-11+New+doc+comment+handling+in+rustdoc

Unions with `impl Copy` and `ManuallyDrop` fields have been stabilized by [rust-lang/rust#77547][1] back in 2020, making enabling the feature already unnecessary for us. A few more field types have been added to the allowlist later by [rust-lang/rust#97995][2], which also removed the feature as there was no intent to stabilize more field types. [1]: rust-lang/rust#77547 [2]: rust-lang/rust#97995

rust-highfive assigned matthewjasper Oct 4, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 4, 2020

RalfJung added F-untagged_unions `#![feature(untagged_unions)]` I-nominated T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Oct 4, 2020

RalfJung force-pushed the stable-union-drop branch from f683a42 to 58aa80d Compare October 4, 2020 20:59

RalfJung mentioned this pull request Oct 4, 2020

Tracking issue for RFC 2514, "Union initialization and Drop" #55149

Closed

7 tasks

jonas-schievink added the relnotes Marks issues that should be documented in the release notes of the next release. label Oct 4, 2020

RalfJung force-pushed the stable-union-drop branch 2 times, most recently from 76ade65 to 4b42d43 Compare October 4, 2020 21:58

rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Oct 5, 2020

RalfJung mentioned this pull request Oct 5, 2020

Remove untagged_unions feature rust-lang/stdarch#924

Closed

scottmcm removed the I-nominated label Oct 5, 2020

rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Oct 5, 2020

rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Oct 5, 2020

matthewjasper approved these changes Oct 10, 2020

View reviewed changes

This comment has been minimized.

Sign in to view

RalfJung added 2 commits October 16, 2020 11:33

stabilize union with 'ManuallyDrop' fields and 'impl Drop for Union'

6a32e79

stop relying on feature(untagged_unions) in stdlib

defcd7f

RalfJung force-pushed the stable-union-drop branch from e87a8a1 to defcd7f Compare October 16, 2020 09:33

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 16, 2020

Dylan-DPC-zz mentioned this pull request Oct 16, 2020

Rollup of 10 pull requests #78019

Closed

JohnTitor mentioned this pull request Oct 16, 2020

Rollup of 10 pull requests #78024

Closed

JohnTitor mentioned this pull request Oct 16, 2020

Rollup of 10 pull requests #78028

Merged

bors merged commit 3356ad7 into rust-lang:master Oct 16, 2020

rustbot added this to the 1.49.0 milestone Oct 16, 2020

RalfJung deleted the stable-union-drop branch October 16, 2020 23:23

RalfJung mentioned this pull request Oct 18, 2020

mention how unions interact with dropping rust-lang/reference#897

Merged

RalfJung mentioned this pull request Oct 18, 2020

consider assignments of union field of ManuallyDrop type safe #78068

Merged

spastorino removed the to-announce Announce this issue on triage meeting label Oct 22, 2020

SimonSapin mentioned this pull request Nov 18, 2020

RFC: rewrite based on min_const_generics for 2.0 servo/rust-smallvec#240

Open

Uriopass mentioned this pull request Nov 25, 2020

Non-copy values in slotmap on stable 1.49 🎉 orlp/slotmap#46

Closed

tarcieri mentioned this pull request Dec 2, 2020

aes: autodetection support for AES-NI RustCrypto/block-ciphers#208

Merged

dfreese mentioned this pull request Mar 14, 2021

Question on grpc_wrap.cc union comment tikv/grpc-rs#519

Open

jackh726 mentioned this pull request Jul 23, 2021

ICE when generic type T is not bound to a Trait, but used as <T as Trait> in union declaration #87388

Closed

michaelwoerister mentioned this pull request Jul 1, 2022

Let the compiler do memory layout computation for tasks at compile time smol-rs/async-task#27

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stabilize union with 'ManuallyDrop' fields and 'impl Drop for Union' #77547

stabilize union with 'ManuallyDrop' fields and 'impl Drop for Union' #77547

RalfJung commented Oct 4, 2020

rust-highfive commented Oct 4, 2020

scottmcm commented Oct 5, 2020 •

edited

Loading

rfcbot commented Oct 5, 2020 •

edited by withoutboats

Loading

RalfJung commented Oct 5, 2020

rfcbot commented Oct 5, 2020

rfcbot commented Oct 15, 2020

This comment has been minimized.

bors commented Oct 16, 2020

ehuss commented Oct 18, 2020

RalfJung commented Oct 18, 2020

RalfJung commented Oct 18, 2020

RalfJung commented Oct 18, 2020

stabilize union with 'ManuallyDrop' fields and 'impl Drop for Union' #77547

stabilize union with 'ManuallyDrop' fields and 'impl Drop for Union' #77547

Conversation

RalfJung commented Oct 4, 2020

rust-highfive commented Oct 4, 2020

scottmcm commented Oct 5, 2020 • edited Loading

rfcbot commented Oct 5, 2020 • edited by withoutboats Loading

RalfJung commented Oct 5, 2020

rfcbot commented Oct 5, 2020

rfcbot commented Oct 15, 2020

This comment has been minimized.

bors commented Oct 16, 2020

ehuss commented Oct 18, 2020

RalfJung commented Oct 18, 2020

RalfJung commented Oct 18, 2020

RalfJung commented Oct 18, 2020

scottmcm commented Oct 5, 2020 •

edited

Loading

rfcbot commented Oct 5, 2020 •

edited by withoutboats

Loading