Document NonZeroXxx layout guarantees

[joshlf]

Document that NonZeroXxx has the same layout and bit validity as Xxx with the exception of 0.

[rust-highfive]

r? @scottmcm

(rust-highfive has picked a reviewer for you, use r? to override)

[scottmcm]

This seems fine to me, but I'm unsure whether procedurally it needs an FCP. (It might be "this was always the intention" and not need it, but it could be "this is a new thing" and thus need it.) So I'll assign it over to the libs-api member who commented on the zulip thread about this:

r? @joshtriplett

[joshtriplett]

I feel like this was always the commitment, but at the same time, better to confirm that consensus rather than assuming it.

@rust-lang/libs-api, are we comfortable committing to the guarantee that NonZeroX is compatible with X?

[joshtriplett]

@rfcbot merge

[rfcbot]

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

• @Amanieu
• @BurntSushi
• @dtolnay
• @joshtriplett
• @m-ou-se
• @yaahc

Concerns:

• implications resolved by Document NonZeroXxx layout guarantees #94786 (comment)

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.

[dtolnay]

Seems okay but do we further want to guarantee that it has the same ABI as X, for example that extern "C" fn() -> NonZeroU8 is bindable to/from C uint8_t(void)? That isn't covered by what's written in the PR.

[joshlf]

Seems okay but do we further want to guarantee that it has the same ABI as X, for example that extern "C" fn() -> NonZeroU8 is bindable to/from C uint8_t(void)? That isn't covered by what's written in the PR.

Presumably we'd need to clarify that 0 is an exception? It's not safe in the general case to use uint8_t(void) as extern "C" fn() -> NonZeroU8 (since the uint8_t(void) might return 0), nor is it safe in the general case to use extern "C" fn(NonZeroU8) as void(uint8_t) (since the void(uint8_t) might be passed 0).

[dtolnay]

Committing to the ABI being the same isn't about saying it's safe/correct to arbitrarily interchange the two signatures. It's just about whether the registers involved in the calling convention are guaranteed to line up, e.g. that if fn() -> u8 returns its result in a particular register then fn() -> NonZeroU8 also returns in that same register. This is something you need to guarantee and is not the case in some "obvious" cases, for example that std::unique_ptr<T> () and T* () do not return the same way, even though unique_ptr is "just a pointer" and has the same layout as a pointer.

[joshtriplett]

Seems okay but do we further want to guarantee that it has the same ABI as X, for example that extern "C" fn() -> NonZeroU8 is bindable to/from C uint8_t(void)? That isn't covered by what's written in the PR.

I think it was intended to be, but making that more explicit would help. We should document that it is repr(transparent) and can be used in FFI.

[m-ou-se]

@rfcbot concern implications

I think it's important to also say that this doesn't mean that e.g. Option<NonZeroU8> and Option<u8> have the same layout. Saying that NonZeroU8 and u8 have the same layout might seem to imply that they always take the same amount of space, even when combined with other things.

[scottmcm]

@m-ou-se For Option<NonZeroU##> specifically, that has already been a guarantee since this FCP in 2018.

But perhaps it would be good to merge that text into this section, and say something like "As a special case, Option<NonZeroU##> is also guaranteed to have the same size, alignment, and ABI as U##. The compiler may also perform additional layout optimizations using this type, but those are not guaranteed."

[workingjubilee]

I agree that clarifying the differences between Option<u8>, Option<NZu8>, and u8 is important. In particular, while I believe that they have the same size, and I believe NZu8 has the same ABI as u8, Option<NZu8> and u8 would not necessarily be ABI interchangeable even if NZu8 and u8 are, even if they have the same layout.

It is very, very plausible to me that someone gets this wrong as a result while trying to do FFI or such.

[dtolnay]

As far as I know:

• Option<NonZeroU8> guarantees same layout as u8 since Make the size of Option<NonZero*> a documented guarantee. #51396.
• NonZeroU8 guarantees same layout as u8 once the FCP here has gone through.
• IMO NonZeroU8 should further guarantee same ABI as u8, but I wouldn't block on this and it can happen later. People are going to assume this no matter what.
• Option<NonZeroU8> continues to not have a specific guaranteed ABI. Whether it's the same as C uint8_t or as C struct S { uint8_t x; }; (which are not the same as one another) has never been settled.

[joshtriplett]

@m-ou-se Does #94786 (comment) address your concern?

[JohnCSimon]

triage:
@m-ou-se ☝️ ?

[m-ou-se]

@rfcbot resolve implications

[rustbot]

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

• Stabilizing library features
• Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
• Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
• Changing public documentation in ways that create new stability guarantees
• Changing observable runtime behavior of library APIs

[joshlf]

Thanks, @JohnCSimon!

@rustbot ready

[dtolnay]

@bors r+ rollup

[bors]

📌 Commit 9a3346b has been approved by dtolnay

It is now in the queue for this repository.

[bors]

⌛ Testing commit 9a3346b with merge f98a271...

[bors]

☀️ Test successful - checks-actions
Approved by: dtolnay
Pushing f98a271 to master...

[rust-timer]

Finished benchmarking commit (f98a271): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.7%	[-1.7%, -1.7%]	1
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.8%	[1.5%, 4.0%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.7%	[-2.7%, -2.7%]	1
All ❌✅ (primary)	-	-	0

Cycles

This benchmark run did not return any relevant results for this metric.

[joshlf]

@joshtriplett I've realized that I may have prematurely accepted your suggested language that made it into the merge:

#[doc = concat!("`Option<", stringify!($Ty), ">` is guaranteed to be compatible with `", stringify!($Int), "`,")]
/// including in FFI.

Does "compatible with" have a technical definition? In #104082, there's discussion of using the language "layout and bit validity", although I'm not sure whether that implicitly provides the same guarantees as your "...including in FFI" language.

Sorry for only bringing this up after the merge! Whatever the outcome of this discussion is, I can include any proposed changes in #104082.