Skip to content

Guarantee behavior of transmuting Option::<T>::None subject to NPO #137323

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

joshlf
Copy link
Contributor

@joshlf joshlf commented Feb 20, 2025

In #115333, we added a guarantee that transmuting from [0u8; N] to Option<P> is sound where P is a pointer type subject to the null pointer optimization (NPO). It would be useful to be able to guarantee the inverse - that a None::<P> value can be transmutes to an array and that will yield [0u8; N].

Closes #117591

@rustbot
Copy link
Collaborator

rustbot commented Feb 20, 2025

r? @tgross35

rustbot has assigned @tgross35.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Feb 20, 2025
@jieyouxu
Copy link
Member

r? libs-api (since this is making a behavior guaranteed)

@rustbot rustbot added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Feb 20, 2025
@rustbot rustbot assigned dtolnay and unassigned tgross35 Feb 20, 2025
@RalfJung
Copy link
Member

Cc @rust-lang/opsem @rust-lang/lang

This will need a t-lang FCP. @joshlf would be good to write a summary for t-lang, knowing that they will lack all the context we have here. :)

@RalfJung RalfJung added T-lang Relevant to the language team, which will review and decide on the PR/issue. needs-fcp This change is insta-stable, so needs a completed FCP to proceed. labels Feb 20, 2025
@RalfJung
Copy link
Member

Looking at the PR itself, the change LGTM.

@dtolnay dtolnay assigned RalfJung and unassigned dtolnay Feb 20, 2025
@dtolnay dtolnay removed T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Feb 20, 2025
@RalfJung RalfJung added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. I-lang-nominated Nominated for discussion during a lang team meeting. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 21, 2025
@joshlf
Copy link
Contributor Author

joshlf commented Mar 5, 2025

This will need a t-lang FCP. @joshlf would be good to write a summary for t-lang, knowing that they will lack all the context we have here. :)

Currently, zerocopy has the ability to validate at runtime whether a [u8; N] contains a valid Option<&T>. In particular, the NPO guarantees that, if the [u8; N] contains all 0u8 bytes, then it constitutes a valid Option<&T>. We do the same for other types subject to NPO: see e.g. this set of impls.

This works via our TryFromBytes trait. Under the hood, #[derive(TryFromBytes)] synthesizes a predicate over &[u8] - in particular, the TryFromBytes::is_bit_valid method takes a &[u8] (it's actually a different type, but it has the same bit validity and alignment as &[u8]) and determines whether it contains a valid Self.

Eventually, we'd like to not only support going from &[u8] to &T, but from &T to &U for a broader range of pairs of types. We can already support &T -> &U if we can soundly convert &T -> &[u8], but some cases don't satisfy that requirement. For example, consider:

#[repr(C)]
struct T {
    a: u8,
    b: u16,
}

#[repr(C)]
struct U {
    a: bool,
    b: u16,
}

Since T has inter-field padding, it's unsound to convert a &T to a &[u8]. We'd still like to be able to support the &T -> &U case, though. This requires changing TryFromBytes::is_bit_valid to no longer take a &[u8] argument, and to instead take something that we call AsInitialized. An AsInitialized<T> has the invariant that it has initialized bytes wherever T has initialized bytes, but it may be uninitializated in other byte positions. This will permit us to go from &T to &AsInitialized<U>, and have TryFromBytes::is_bit_valid take this &AsInitialized<U> as an argument.

That gets us to this PR: In order to make this change, for each type that currently implements TryFromBytes, we need a guarantee about which bytes are always initialized. Currently, if P is a type subject to NPO, we have no guarantee that an instance of Option<P> has all its bytes initialized. While getting this guarantee in full is blocked on ptr-to-int semantics, this PR is intended as a first step that will at least handle the None case.

@traviscross traviscross changed the title Guarantee behavior of transmuting Option::<T>::None subject to NPO Guarantee behavior of transmuting Option::<T>::None subject to NPO May 7, 2025
@joshtriplett
Copy link
Member

This seems consistent with how we already support using Option<&T> rather than *const T in FFI calls, in both directions. I'm surprised we don't already guarantee this; people's code would be broken if we did otherwise.

@joshtriplett
Copy link
Member

Per the above, we already have to guarantee this and there'd be widespread breakage if we ever failed to uphold it. So, let's write it down.

@rfcbot merge

@rfcbot
Copy link
Collaborator

rfcbot commented May 7, 2025

Team member @joshtriplett 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!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

@rfcbot 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 May 7, 2025
@traviscross
Copy link
Contributor

@rfcbot reviewed

@traviscross traviscross added I-lang-radar Items that are on lang's radar and will need eventual work or consideration. and removed I-lang-nominated Nominated for discussion during a lang team meeting. labels May 7, 2025
@scottmcm
Copy link
Member

scottmcm commented May 7, 2025

Under the restriction (that's in the docs) of the very specific list of NPO types, 100% agreed.

@rfcbot reviewed

(Just wanted to double-check that we weren't accidentally guaranteeing anything for Option<char> or Option<CustomEnumOneOrTwo> or similar.)

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels May 7, 2025
@rfcbot
Copy link
Collaborator

rfcbot commented May 7, 2025

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

@traviscross
Copy link
Contributor

We talked about this in the lang call today, were happy to see it go forward, and it's now in FCP.

Let's cc @rust-lang/spec, to think about the interplay between the Reference and the library documentation when making language guarantees like this.

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. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. I-lang-radar Items that are on lang's radar and will need eventual work or consideration. needs-fcp This change is insta-stable, so needs a completed FCP to proceed. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). 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.

Guarantee that it is sound to observe the bytes of None::<P> where P is a pointer type subject to NPO
10 participants