Guarantee representation of None in NPO

[joshlf]

This allows users to soundly transmute zeroes into Option types subject to the null pointer optimization (NPO). It unblocks google/zerocopy#293.

[rustbot]

r? @joshtriplett

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

[slanterns]

IMO it may not be sound due to niche-filling.

#![feature(rustc_attrs)]
#[rustc_layout_scalar_valid_range_end(1)]
struct a(i64);
unsafe { std::mem::transmute::<_, i64>(Option::<a>::None) }
--> 2

[joshlf]

IMO it may not be sound due to niche-filling.

#![feature(rustc_attrs)]
#[rustc_layout_scalar_valid_range_end(1)]
struct a(i64);
unsafe { std::mem::transmute::<_, i64>(Option::<a>::None) }
--> 2

Would this be categorized as a null-pointer optimization/included in the list of types in this doc section? Presumably if we added a type like that we'd just update the docs to make sure the sentence I'm adding doesn't apply to it, right?

[slanterns]

Would this be categorized as a null-pointer optimization/included in the list of types in this doc section? Presumably if we added a type like that we'd just update the docs to make sure the sentence I'm adding doesn't apply to it, right?

Oh yeah for the listed types I believe None will be put at 0.

[asquared31415]

I am not sure what the status of "alignment niches" is, but I think that this guarantee could interfere with a case where &u32 understands that 0x0..=0x3 are free, and chooses None::<&u32> to be 0x02 for example. It is likely possible to add this guarantee and alignment niches if the implementation is careful, and I may be misunderstanding, but I think that adding this guarantee complicates things.

[Noratrieb]

Nominating for lang team discussion.

Question

Should it be guaranteed that the option null pointer optimization is actually a null pointer optimization, choosing the null pointer (0) to represent None?

[joshlf]

I am not sure what the status of "alignment niches" is, but I think that this guarantee could interfere with a case where &u32 understands that 0x0..=0x3 are free, and chooses None::<&u32> to be 0x02 for example. It is likely possible to add this guarantee and alignment niches if the implementation is careful, and I may be misunderstanding, but I think that adding this guarantee complicates things.

My understanding is that a big selling point of these optimizations is that they make Option FFI-compatible if you assume that None is represented as 0. While I agree that it's technically not the guaranteed behavior today, I wonder how much code is already relying on this. I don't have a citation for it, but I remember folks being excited about the implications for FFI when this was first introduced.

[ChrisDenton]

Do we need to note nested niches? E.g. Option<Option<Option<NonZeroUsize>>> may not be zero.

[asquared31415]

Option<Option<NonZeroUsize>> is not one of the listed types that gets the optimization, so adding another option around it does not get the optimization. I think that the docs are clear in that regard.

[slanterns]

https://github.com/rust-lang/rfcs/blob/master/text/3391-result_ffi_guarantees.md has an RFC. Does this also need one?

[scottmcm]

For some of the types, this is actually documented on the inner types instead, like https://doc.rust-lang.org/nightly/std/num/struct.NonZeroU32.html#layout-1

This seems like an obvious yes for NonNull and & and &mut, but I don't know that writing the guarantee here is the right way.

[asquared31415]

NonNull and NonZeroX types are an obvious yes by nature of "there's one value that we have to represent and only one bit pattern for it". I think that adding the documentation for those is somewhat recent.

While I think that it's a valid concern to avoid documenting things multiple times, I think that it's important that there's something here. This is the documentation about the NPO generally.

[RalfJung]

We have an accepted RFC that this optimization applies not only to Option but also some cases of Result. So medium-term Option isn't really the right place to put this. We'll have to find some central place describing the idea of null-pointer opt, and then each individual involved type should reference that central place.

[m-ou-se]

I'm afraid this might make it sound like we guarantee None is always all zeros, even for Option<char> or Option<ascii::Char>, which are actually nonzero.

[m-ou-se]

Can we guarantee that None::<&T> and None::<NonNull> are always represented as all zeros? Could we ever support platforms where NULL isn't 0?

[joshlf]

We have an accepted RFC that this optimization applies not only to Option but also some cases of Result. So medium-term Option isn't really the right place to put this. We'll have to find some central place describing the idea of null-pointer opt, and then each individual involved type should reference that central place.

Is there a harm in putting this here until the Result optimization is implemented, at which point we can move the docs to a central location?

I'm afraid this might make it sound like we guarantee None is always all zeros, even for Option<char> or Option<ascii::Char>, which are actually nonzero.

Would it help if we explicitly called out that this guarantee does not apply to all types, and maybe name these as examples?

Can we guarantee that None::<&T> and None::<NonNull> are always represented as all zeros? Could we ever support platforms where NULL isn't 0?

This is a question above my pay grade, but I'd expect ~everything to break if we tried to do this 😛

On a more serious note, if we did this, what would the NPO look like? Would we expect to make guarantees about what value constitutes NULL, or would we just guarantee that some value constituted NULL, and so we could make guarantees about the size of Option<T>? Basically, I'm wondering if we could still write down the X in transmute::<_, Option<T>>(X), or if we just can't make any guarantees at all.

[RalfJung]

Is there a harm in putting this here until the Result optimization is implemented, at which point we can move the docs to a central location?

No, it's fine having this all in Option for now.

Could we ever support platforms where NULL isn't 0?

I think right now the general thinking is that no, there is no intent to support such a platform.

[tmandry]

@rfcbot fcp merge

[rfcbot]

Team member @tmandry 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
• @nikomatsakis
• @pnkfelix
• @scottmcm
• @tmandry

Concerns:

• None-value of Option<fat-reference> resolved by Guarantee representation of None in NPO #115333 (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!

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.

[nikomatsakis]

@rfcbot reviewed -- I'm in favor of making guarantees here! No strong opinion about whether this is the right place to write the doc.

[rfcbot]

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.

This will be merged soon.

[joshlf]

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.

This will be merged soon.

Is there anything we need to do to get this unstuck, or is it the expected behavior for it to take a few weeks to merge?

[RalfJung]

When FCP finishes, it's the job of the reviewer to do the usual kind of review to check that the code/implementation part of the PR is good to land. Reviewers often don't realize they are "on the hook" for this PR though, since they might have forgotten that they are the assigned reviewer and the FCP bot message doesn't remind them. (I'll file an rfcbot feature request, would be good to ping the assigned reviewer to get this back into their queue.)

In this case there's no code change so we can just land this.
@bors r+ rollup

[bors]

📌 Commit 9703cb2 has been approved by RalfJung

It is now in the queue for this repository.

[bors]

⌛ Testing commit 9703cb2 with merge 1bb6553...

[bors]

☀️ Test successful - checks-actions
Approved by: RalfJung
Pushing 1bb6553 to master...

[rust-timer]

Finished benchmarking commit (1bb6553): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Warning ⚠: The following benchmark(s) failed to build:

• stm32f4-0.14.0
• clap-3.1.6
• cargo-0.60.0

cc @rust-lang/wg-compiler-performance

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)	12.2%	[0.9%, 30.1%]	8
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	12.2%	[0.9%, 30.1%]	8

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)	2.5%	[0.8%, 3.8%]	5
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.0%	[-3.2%, -1.0%]	4
All ❌✅ (primary)	2.5%	[0.8%, 3.8%]	5

Cycles

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)	10.6%	[1.7%, 23.4%]	7
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	10.6%	[1.7%, 23.4%]	7

Binary size

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

Bootstrap: 636.863s -> 635.435s (-0.22%)
Artifact size: 304.49 MiB -> 304.48 MiB (-0.00%)

[joshlf]

Not sure if I have the permission to do this, and not sure if I'm getting the syntax right, but...

@rustbot label: +perf-regression-triaged

This is a docs change

[lqd]

It looks more like an issue during the collection process, with benchmarks timing out. We’ll keep an eye on this for the following PRs, but this PR is especially unlikely to have caused this.

[joshlf]

I've submitted #117591 to follow up on this.

[pnkfelix]

• this is a docs change.
• There was a spike of noise that somehow significantly (but temporarily) affected three instances of stm32f4 debug, as well as clap opt (all of which immediately went away in the next commit).
• there is more discussion of why on zulip
• already marked as triaged.