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

Explicitly document the size guarantees that Option makes. #75454

Merged
merged 8 commits into from
Sep 26, 2020
Merged

Explicitly document the size guarantees that Option makes. #75454

merged 8 commits into from
Sep 26, 2020

Conversation

ltratt
Copy link
Contributor

@ltratt ltratt commented Aug 12, 2020

Triggered by a discussion on wg-unsafe-code-guidelines about which layouts of Option<T> one can guarantee are optimised to a single pointer.

CC @RalfJung

Triggered by a discussion on wg-unsafe-code-guidelines about which layouts of
`Option<T>` one can guarantee are optimised to a single pointer.
@rust-highfive
Copy link
Collaborator

r? @dtolnay

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 12, 2020
Comment on lines 75 to 76
//! Rust guarantees to optimise the following inner types such that an [`Option`] which contains
//! them has the same size as a pointer:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
//! Rust guarantees to optimise the following inner types such that an [`Option`] which contains
//! them has the same size as a pointer:
//! Rust guarantees to optimize the following types `T` such that an [`Option<T>`]
//! has the same size as a `T`:

Giving names to things helps. :) And we typically use US spelling, AFAIK.

Also maybe say that this means that one may transmute a T to an Option<T>?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Fixed in f5118a5.

//! * [`num::NonZero*`]
//! * [`ptr::NonNull<T>`]
//! * `#[repr(transparent)]` struct around one of the types in this list.
//! * [`Box<T>`]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In terms of ordering I think it makes more sense to add this to the top or just below the references.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. Fixed in f5118a5.

@RalfJung RalfJung added I-nominated T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Aug 12, 2020
@RalfJung
Copy link
Member

Nominating for lang-team discussion.
@rust-lang/lang, I propose to guarnatee for a bunch of types that Option<T> has the same size as T, and that T may be transmuted to Option<T>. Most of this has actually already been decided in #60300, except for Box. So the FCP is basically about guaranteeing that Option<Box<T>> is subject to niche optimizations and has the same size as Box<T>.

//! * [`Box<T>`]
//!
//! For the above cases, it is guaranteed that one can use [`mem::transmute`]
//! between `T` and `Option<T>` and vice versa.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, not vice versa! If you transmute a None from Option<T> to T, you get UB.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Should there be a guarantee for non-None in such a case, or just rule out the reverse direction entirely?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Guarantee for non-None seems reasonable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 83f47aa.

library/core/src/option.rs Outdated Show resolved Hide resolved
Co-authored-by: Ralf Jung <post@ralfj.de>
Co-authored-by: Ralf Jung <post@ralfj.de>
@RalfJung
Copy link
Member

@rust-lang/lang another point that is not entirely clear yet is function pointers -- the last FCP only covered extern "C" fn. I am sure we also want this guarantee for fn (aka extern "Rust" fn), but do we just want it for all ABIs or is there a chance some ABIs might not want a niche?

Comment on lines 75 to 78
//! Rust guarantees to optimize the following types `<T>` such that an
//! [`Option<T>`] has the same size as `T`:
//!
//! * [`Box<T>`]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This wording is pretty confusing to me, I think because it uses the same name T to mean two different things. For example this line is talking about what happens when T = Box<T> ... which is never true.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I struggled to find a good phrasing here; I agree the current phrasing isn't great.

Might something like:

//! Rust guarantees that the following types are optimised such that their size
//! is identical whether or not they are surrounded by an `Option`:

be clearer?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe just use Box<_> or Box<U>. Naming things is generally a good idea, so I'd not remove the T in the introduction.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 68209c3.

@dtolnay
Copy link
Member

dtolnay commented Aug 16, 2020

Reassigning for reviewing accuracy after discussed by lang team.

r? @RalfJung

@rust-highfive rust-highfive assigned RalfJung and unassigned dtolnay Aug 16, 2020
//! in Rust are stored as efficiently as any other pointer type.
//! # Representation
//!
//! Rust guarantees to optimize the following types `<T>` such that an
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
//! Rust guarantees to optimize the following types `<T>` such that an
//! Rust guarantees to optimize the following types `T` such that an

I never saw <T> as notation for a type, not sure what that is supposed to indicate.

@RalfJung RalfJung added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 17, 2020
@nikomatsakis
Copy link
Contributor

Discussed in rust-lang/lang meeting today:

  • We agree that this is a good idea and that we expect these representations to be guaranteed.
  • As far as ABI, we're not aware of any ABIs for which this property doesn't hold, and if a new ABI were added in the future, we could exempt it if necessary.

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Aug 17, 2020

Team member @nikomatsakis 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.

@ltratt
Copy link
Contributor Author

ltratt commented Sep 2, 2020

That is certainly possible, though I am not sure if it is appropriate to make such a significant addition this late in the FCP process?

My tuppence worth: if we do add that guarantee (in some form) in, we might want to re-ask those who've signed the CFP off so far?

@RalfJung
Copy link
Member

@cramertj @pnkfelix @withoutboats there's some FCP checkboxes waiting for you here. :)

@rfcbot rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Sep 15, 2020
@rfcbot
Copy link

rfcbot commented Sep 15, 2020

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

@rfcbot 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 Sep 15, 2020
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Sep 25, 2020
@rfcbot
Copy link

rfcbot commented Sep 25, 2020

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.

@dtolnay
Copy link
Member

dtolnay commented Sep 25, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Sep 25, 2020

📌 Commit 9bac577 has been approved by dtolnay

@bors 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-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Sep 25, 2020
@bors
Copy link
Contributor

bors commented Sep 26, 2020

⌛ Testing commit 9bac577 with merge 1671a2d6368f9890d7db0f6bf5f21226715e0c45...

RalfJung added a commit to RalfJung/rust that referenced this pull request Sep 26, 2020
…es, r=dtolnay

Explicitly document the size guarantees that Option makes.

Triggered by a discussion on wg-unsafe-code-guidelines about which layouts of `Option<T>` one can guarantee are optimised to a single pointer.

CC @RalfJung
@RalfJung
Copy link
Member

Rolled up, yielding.
@bors retry rollup

bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 26, 2020
Rollup of 12 pull requests

Successful merges:

 - rust-lang#75454 (Explicitly document the size guarantees that Option makes.)
 - rust-lang#76631 (Add `x.py setup`)
 - rust-lang#77076 (Add missing code examples on slice iter types)
 - rust-lang#77093 (merge `need_type_info_err(_const)`)
 - rust-lang#77122 (Add `#![feature(const_fn_floating_point_arithmetic)]`)
 - rust-lang#77127 (Update mdBook)
 - rust-lang#77161 (Remove TrustedLen requirement from BuilderMethods::switch)
 - rust-lang#77166 (update Miri)
 - rust-lang#77181 (Add doc alias for pointer primitive)
 - rust-lang#77204 (Remove stray word from `ClosureKind::extends` docs)
 - rust-lang#77207 (Rename `whence` to `span`)
 - rust-lang#77211 (Remove unused #[allow(...)] statements from compiler/)

Failed merges:

 - rust-lang#77170 (Remove `#[rustc_allow_const_fn_ptr]` and add `#![feature(const_fn_fn_ptr_basics)]`)

r? `@ghost`
@bors bors merged commit 1e62382 into rust-lang:master Sep 26, 2020
@rustbot rustbot added this to the 1.48.0 milestone Sep 26, 2020
@spastorino spastorino removed the to-announce Announce this issue on triage meeting label Oct 1, 2020
@dtolnay dtolnay self-assigned this Mar 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.