Make MaybeUninit #[repr(transparent)] #61802

mjbshaw · 2019-06-13T13:59:12Z

Tracking issue: #60405

rust-highfive · 2019-06-13T13:59:23Z

r? @dtolnay

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

mjbshaw · 2019-06-13T15:07:27Z

cc @Centril @RalfJung

Centril · 2019-06-13T19:19:38Z

r? @cramertj

cramertj · 2019-06-13T20:58:09Z

We discussed this in the lang team meeting today, and the conclusion was that we'd like to document and guarantee that MaybeUninit<T> share's T's size, alignment, and ABI when being passed directly by-value (not necessarily as a member of another type).

It's unclear exactly what we might want #[repr(transparent)] to mean on unions, or if we should even allow it. All existing uses of #[repr(transparent)} also grant that e.g. Option<Wrapper<T>> has the same representation as Option<T>, but this is not true for #[repr(transparent)] unions, and cannot be true. With that in mind, we'd like to note that MaybeUninit<T> is not guaranteed to be #[repr(transparent)], but only the three above properties (size, align, direct by-value ABI).

Implementation wise, this works out to exactly what this PR does-- sticking #[repr(transparent)] on the type, so I think we're all okay with merging this so long as we document it appropriately.

hanna-kruppe · 2019-06-13T21:44:04Z

It's unclear exactly what we might want #[repr(transparent)] to mean on unions, or if we should even allow it. All existing uses of #[repr(transparent)} also grant that e.g. Option<Wrapper<T>> has the same representation as Option<T>, but this is not true for #[repr(transparent)] unions, and cannot be true. With that in mind, we'd like to note that MaybeUninit<T> is not guaranteed to be #[repr(transparent)], but only the three above properties (size, align, direct by-value ABI).

So, uh, this sounds like y'all don't want RFC 2645 after all. Not that I disagree, but I'm surprised to hear this so soon after the RFC was merged, especially since this exact concern was brough up and marked resolved during the FCP. Or do I misunderstand?

Implementation wise, this works out to exactly what this PR does-- sticking #[repr(transparent)] on the type, so I think we're all okay with merging this so long as we document it appropriately.

Note that the repr(transparent) attribute is shown in rustdoc, so this is not just an implementation concern. More generally, one argument in favor of the RFC was that having this general, "public" attribute would be a cleaner way for (programmers, docs, tools, etc.) to learn about the size/align/by-value-ABI transparency vs just stating those guarantees in prose as one-off properties of MaybeUninit. If we end up making MaybeUninit special anyway, the rationale for transparent unions is even weaker than it IMO already way.

Centril · 2019-06-13T21:47:09Z

So, uh, this sounds like y'all don't want RFC 2645 after all. Not that I disagree, but I'm surprised to hear this so soon after the RFC was merged, especially since this exact concern was brough up and marked resolved during the FCP. Or do I misunderstand?

I still want the RFC and to affirm the guarantee through an attribute eventually.

If we end up making MaybeUninit special anyway, the rationale for transparent unions is even weaker than it IMO already way.

Our discussions was mostly around landing this PR before we stabilize the attribute itself, which I do think we should eventually.

cramertj · 2019-06-13T21:53:07Z

@rkruppe yeah to be clear, I don't think we're going back on what we said in that RFC, only saying that we don't want this particular addition to be a de-facto stabilization of that RFC's behavior.

hanna-kruppe · 2019-06-13T22:01:42Z

Thanks for the clarification. I would like some further clarification on one point: was this decision made aware of the fact that repr(transparent) is shown on the type in rustdoc? I, personally, do worry that this makes adding the attribute a de-facto stabilization of transparent unions as a concept, and by extension (since the surprise around Option is fundamental and everything else is dead obvious) the semantics currently given to them.

Centril · 2019-06-13T22:06:23Z

@rkruppe

was this decision made aware of the fact that repr(transparent) is shown on the type in rustdoc?

I don't believe this fact was brought up in the discussions.

RalfJung · 2019-06-14T06:51:15Z

Implementation wise, this works out to exactly what this PR does

Notice that this PR is effectively a NOP -- repr(transparent) does not actually change how we compute layout, it just adds some extra checks that ensure that we end up in the "happy path".

Tracking issue: rust-lang#60405

mjbshaw · 2019-06-18T13:32:46Z

I have revised the documentation on MaybeUninit that hopefully addresses some of the concerns here. If it is insufficient or requires more work, let me know.

cramertj · 2019-06-18T16:47:13Z

Nice!

@bors r+ rollup

bors · 2019-06-18T16:47:15Z

📌 Commit 0f9dc6c has been approved by cramertj

RalfJung · 2019-06-18T17:41:52Z

src/libcore/mem/maybe_uninit.rs

+///
+/// While `MaybeUninit` is `#[repr(transparent)]` (indicating it guarantees the same size,
+/// alignment, and ABI as `T`), this does *not* change any of the previous caveats. `Option<T>` and
+/// `Option<MaybeUninit<T>>` may still have different sizes, and types containing a field of type


This is odd. We just said two paragraphs earlier that this is the case, why repeat it here?

I think it makes more sense to change the previous paragraph (starting "However remember that a type containing a MaybeUninit<T>") to say something like "Despite being repr(transparent), ..."

@mjbshaw can you file a follow-up PR to improve the wording here or should I do that?

…=cramertj Make MaybeUninit #[repr(transparent)] Tracking issue: rust-lang#60405

@ghost

Rollup of 11 pull requests Successful merges: - #61505 (Only show methods that appear in `impl` blocks in the Implementors sections of trait doc pages) - #61701 (move stray run-pass const tests into const/ folder) - #61748 (Tweak transparent enums and unions diagnostic spans) - #61802 (Make MaybeUninit #[repr(transparent)]) - #61839 (ci: Add a script for generating CPU usage graphs) - #61842 (Remove unnecessary lift calls) - #61843 (Turn down the myriad-closures test) - #61896 (rustc_typeck: correctly compute `Substs` for `Res::SelfCtor`.) - #61898 (syntax: Factor out common fields from `SyntaxExtension` variants) - #61938 (create an issue for miri even in status test-fail) - #61941 (Preserve generator and yield source for error messages) Failed merges: r? @ghost

@ghost

Rollup of 11 pull requests Successful merges: - #61505 (Only show methods that appear in `impl` blocks in the Implementors sections of trait doc pages) - #61701 (move stray run-pass const tests into const/ folder) - #61748 (Tweak transparent enums and unions diagnostic spans) - #61802 (Make MaybeUninit #[repr(transparent)]) - #61839 (ci: Add a script for generating CPU usage graphs) - #61842 (Remove unnecessary lift calls) - #61843 (Turn down the myriad-closures test) - #61896 (rustc_typeck: correctly compute `Substs` for `Res::SelfCtor`.) - #61898 (syntax: Factor out common fields from `SyntaxExtension` variants) - #61938 (create an issue for miri even in status test-fail) - #61941 (Preserve generator and yield source for error messages) Failed merges: r? @ghost

@ghost

Rollup of 11 pull requests Successful merges: - #61505 (Only show methods that appear in `impl` blocks in the Implementors sections of trait doc pages) - #61701 (move stray run-pass const tests into const/ folder) - #61748 (Tweak transparent enums and unions diagnostic spans) - #61802 (Make MaybeUninit #[repr(transparent)]) - #61839 (ci: Add a script for generating CPU usage graphs) - #61842 (Remove unnecessary lift calls) - #61843 (Turn down the myriad-closures test) - #61896 (rustc_typeck: correctly compute `Substs` for `Res::SelfCtor`.) - #61898 (syntax: Factor out common fields from `SyntaxExtension` variants) - #61938 (create an issue for miri even in status test-fail) - #61941 (Preserve generator and yield source for error messages) Failed merges: r? @ghost

Pkgsrc changes: * Add a patch to llvm to deal with const dli_saddr. * Adapt two other patches. * Cross-build currently fails, so i386, powerpc and sparc64 bootstrap kits for 1.37.0 are built natively. Missing aarch64 hardware, so that's not available yet. * Bump bootstrap requirements to 1.36.0 except for armv7-unknown-netbsd-eabihf which I've not managed to cross-build. Upstream changes: Version 1.37.0 (2019-08-15) ========================== Language -------- - `#[must_use]` will now warn if the type is contained in a [tuple][61100], [`Box`][62228], or an [array][62235] and unused. - [You can now use the `cfg` and `cfg_attr` attributes on generic parameters.][61547] - [You can now use enum variants through type alias.][61682] e.g. You can write the following: ```rust type MyOption = Option<u8>; fn increment_or_zero(x: MyOption) -> u8 { match x { MyOption::Some(y) => y + 1, MyOption::None => 0, } } ``` - [You can now use `_` as an identifier for consts.][61347] e.g. You can write `const _: u32 = 5;`. - [You can now use `#[repr(align(X)]` on enums.][61229] - [The `?`/_"Kleene"_ macro operator is now available in the 2015 edition.][60932] Compiler -------- - [You can now enable Profile-Guided Optimization with the `-C profile-generate` and `-C profile-use` flags.][61268] For more information on how to use profile guided optimization, please refer to the [rustc book][rustc-book-pgo]. - [The `rust-lldb` wrapper script should now work again.][61827] Libraries --------- - [`mem::MaybeUninit<T>` is now ABI-compatible with `T`.][61802] Stabilized APIs --------------- - [`BufReader::buffer`] - [`BufWriter::buffer`] - [`Cell::from_mut`] - [`Cell<[T]>::as_slice_of_cells`][`Cell<slice>::as_slice_of_cells`] - [`DoubleEndedIterator::nth_back`] - [`Option::xor`] - [`Wrapping::reverse_bits`] - [`i128::reverse_bits`] - [`i16::reverse_bits`] - [`i32::reverse_bits`] - [`i64::reverse_bits`] - [`i8::reverse_bits`] - [`isize::reverse_bits`] - [`slice::copy_within`] - [`u128::reverse_bits`] - [`u16::reverse_bits`] - [`u32::reverse_bits`] - [`u64::reverse_bits`] - [`u8::reverse_bits`] - [`usize::reverse_bits`] Cargo ----- - [`Cargo.lock` files are now included by default when publishing executable crates with executables.][cargo/7026] - [You can now specify `default-run="foo"` in `[package]` to specify the default executable to use for `cargo run`.][cargo/7056] Misc ---- Compatibility Notes ------------------- - [Using `...` for inclusive range patterns will now warn by default.][61342] Please transition your code to using the `..=` syntax for inclusive ranges instead. - [Using a trait object without the `dyn` will now warn by default.][61203] Please transition your code to use `dyn Trait` for trait objects instead. [62228]: rust-lang/rust#62228 [62235]: rust-lang/rust#62235 [61802]: rust-lang/rust#61802 [61827]: rust-lang/rust#61827 [61547]: rust-lang/rust#61547 [61682]: rust-lang/rust#61682 [61268]: rust-lang/rust#61268 [61342]: rust-lang/rust#61342 [61347]: rust-lang/rust#61347 [61100]: rust-lang/rust#61100 [61203]: rust-lang/rust#61203 [61229]: rust-lang/rust#61229 [60932]: rust-lang/rust#60932 [cargo/7026]: rust-lang/cargo#7026 [cargo/7056]: rust-lang/cargo#7056 [`BufReader::buffer`]: https://doc.rust-lang.org/std/io/struct.BufReader.html#method.buffer [`BufWriter::buffer`]: https://doc.rust-lang.org/std/io/struct.BufWriter.html#method.buffer [`Cell::from_mut`]: https://doc.rust-lang.org/std/cell/struct.Cell.html#method.from_mut [`Cell<slice>::as_slice_of_cells`]: https://doc.rust-lang.org/std/cell/struct.Cell.html#method.as_slice_of_cells [`DoubleEndedIterator::nth_back`]: https://doc.rust-lang.org/std/iter/trait.DoubleEndedIterator.html#method.nth_back [`Option::xor`]: https://doc.rust-lang.org/std/option/enum.Option.html#method.xor [`RefCell::try_borrow_unguarded`]: https://doc.rust-lang.org/std/cell/struct.RefCell.html#method.try_borrow_unguarded [`Wrapping::reverse_bits`]: https://doc.rust-lang.org/std/num/struct.Wrapping.html#method.reverse_bits [`i128::reverse_bits`]: https://doc.rust-lang.org/std/primitive.i128.html#method.reverse_bits [`i16::reverse_bits`]: https://doc.rust-lang.org/std/primitive.i16.html#method.reverse_bits [`i32::reverse_bits`]: https://doc.rust-lang.org/std/primitive.i32.html#method.reverse_bits [`i64::reverse_bits`]: https://doc.rust-lang.org/std/primitive.i64.html#method.reverse_bits [`i8::reverse_bits`]: https://doc.rust-lang.org/std/primitive.i8.html#method.reverse_bits [`isize::reverse_bits`]: https://doc.rust-lang.org/std/primitive.isize.html#method.reverse_bits [`slice::copy_within`]: https://doc.rust-lang.org/std/primitive.slice.html#method.copy_within [`u128::reverse_bits`]: https://doc.rust-lang.org/std/primitive.u128.html#method.reverse_bits [`u16::reverse_bits`]: https://doc.rust-lang.org/std/primitive.u16.html#method.reverse_bits [`u32::reverse_bits`]: https://doc.rust-lang.org/std/primitive.u32.html#method.reverse_bits [`u64::reverse_bits`]: https://doc.rust-lang.org/std/primitive.u64.html#method.reverse_bits [`u8::reverse_bits`]: https://doc.rust-lang.org/std/primitive.u8.html#method.reverse_bits [`usize::reverse_bits`]: https://doc.rust-lang.org/std/primitive.usize.html#method.reverse_bits [rustc-book-pgo]: https://doc.rust-lang.org/rustc/profile-guided-optimization.html

rust-highfive assigned dtolnay Jun 13, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 13, 2019

Centril added I-nominated T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Jun 13, 2019

rust-highfive assigned cramertj and unassigned dtolnay Jun 13, 2019

cramertj added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed I-nominated S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 13, 2019

Make MaybeUninit #[repr(transparent)]

0f9dc6c

Tracking issue: rust-lang#60405

mjbshaw force-pushed the maybe-uninit-transparent branch from 82c7b83 to 0f9dc6c Compare June 18, 2019 13:31

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 18, 2019

RalfJung reviewed Jun 18, 2019

View reviewed changes

Centril added a commit to Centril/rust that referenced this pull request Jun 18, 2019

Rollup merge of rust-lang#61802 - mjbshaw:maybe-uninit-transparent, r…

32254ac

…=cramertj Make MaybeUninit #[repr(transparent)] Tracking issue: rust-lang#60405

Centril mentioned this pull request Jun 18, 2019

Rollup of 10 pull requests #61943

Closed

Centril added a commit to Centril/rust that referenced this pull request Jun 18, 2019

Rollup merge of rust-lang#61802 - mjbshaw:maybe-uninit-transparent, r…

e416932

…=cramertj Make MaybeUninit #[repr(transparent)] Tracking issue: rust-lang#60405

Centril mentioned this pull request Jun 18, 2019

Rollup of 11 pull requests #61945

Merged

bors merged commit 0f9dc6c into rust-lang:master Jun 19, 2019

carbotaniuman mentioned this pull request Jul 26, 2024

Should we / can we make MaybeUninit<T> always preserve all bytes of T? rust-lang/unsafe-code-guidelines#518

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make MaybeUninit #[repr(transparent)] #61802

Make MaybeUninit #[repr(transparent)] #61802

mjbshaw commented Jun 13, 2019

rust-highfive commented Jun 13, 2019

mjbshaw commented Jun 13, 2019

Centril commented Jun 13, 2019

cramertj commented Jun 13, 2019

hanna-kruppe commented Jun 13, 2019 •

edited

Loading

Centril commented Jun 13, 2019

cramertj commented Jun 13, 2019

hanna-kruppe commented Jun 13, 2019

Centril commented Jun 13, 2019

RalfJung commented Jun 14, 2019

mjbshaw commented Jun 18, 2019

cramertj commented Jun 18, 2019

bors commented Jun 18, 2019

RalfJung Jun 18, 2019

RalfJung Jun 19, 2019

Make MaybeUninit #[repr(transparent)] #61802

Make MaybeUninit #[repr(transparent)] #61802

Conversation

mjbshaw commented Jun 13, 2019

rust-highfive commented Jun 13, 2019

mjbshaw commented Jun 13, 2019

Centril commented Jun 13, 2019

cramertj commented Jun 13, 2019

hanna-kruppe commented Jun 13, 2019 • edited Loading

Centril commented Jun 13, 2019

cramertj commented Jun 13, 2019

hanna-kruppe commented Jun 13, 2019

Centril commented Jun 13, 2019

RalfJung commented Jun 14, 2019

mjbshaw commented Jun 18, 2019

cramertj commented Jun 18, 2019

bors commented Jun 18, 2019

RalfJung Jun 18, 2019

Choose a reason for hiding this comment

RalfJung Jun 19, 2019

Choose a reason for hiding this comment

hanna-kruppe commented Jun 13, 2019 •

edited

Loading