Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make MaybeUninit #[repr(transparent)] #61802

Merged
merged 1 commit into from Jun 19, 2019

Conversation

Projects
None yet
8 participants
@mjbshaw
Copy link
Contributor

commented Jun 13, 2019

Tracking issue: #60405

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Jun 13, 2019

r? @dtolnay

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

@mjbshaw

This comment has been minimized.

Copy link
Contributor Author

commented Jun 13, 2019

@Centril

This comment has been minimized.

Copy link
Member

commented Jun 13, 2019

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

@cramertj

This comment has been minimized.

Copy link
Member

commented Jun 13, 2019

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.

@rkruppe

This comment has been minimized.

Copy link
Member

commented Jun 13, 2019

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

This comment has been minimized.

Copy link
Member

commented Jun 13, 2019

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

This comment has been minimized.

Copy link
Member

commented Jun 13, 2019

@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.

@rkruppe

This comment has been minimized.

Copy link
Member

commented Jun 13, 2019

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

This comment has been minimized.

Copy link
Member

commented Jun 13, 2019

@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

This comment has been minimized.

Copy link
Member

commented Jun 14, 2019

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".

@mjbshaw mjbshaw force-pushed the mjbshaw:maybe-uninit-transparent branch from 82c7b83 to 0f9dc6c Jun 18, 2019

@mjbshaw

This comment has been minimized.

Copy link
Contributor Author

commented Jun 18, 2019

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

This comment has been minimized.

Copy link
Member

commented Jun 18, 2019

Nice!

@bors r+ rollup

@bors

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2019

📌 Commit 0f9dc6c has been approved by cramertj

///
/// 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 comment has been minimized.

Copy link
@RalfJung

RalfJung Jun 18, 2019

Member

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), ..."

This comment has been minimized.

Copy link
@RalfJung

RalfJung Jun 19, 2019

Member

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

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…
…=cramertj

Make MaybeUninit #[repr(transparent)]

Tracking issue: rust-lang#60405

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…
…=cramertj

Make MaybeUninit #[repr(transparent)]

Tracking issue: rust-lang#60405

bors added a commit that referenced this pull request Jun 19, 2019

Auto merge of #61945 - Centril:rollup-xdqo2mn, r=Centril
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

bors added a commit that referenced this pull request Jun 19, 2019

Auto merge of #61945 - Centril:rollup-xdqo2mn, r=Centril
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

bors added a commit that referenced this pull request Jun 19, 2019

Auto merge of #61945 - Centril:rollup-xdqo2mn, r=Centril
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

bors added a commit that referenced this pull request Jun 19, 2019

Auto merge of #61945 - Centril:rollup-xdqo2mn, r=Centril
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

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

2 checks passed

Travis CI - Pull Request Build Passed
Details
pr Build #20190618.23 succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.