r? @joshtriplett

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

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

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?

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.

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.

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?

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.

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

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.

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

@rfcbot fcp merge

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.

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

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.

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?

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

📌 Commit 9703cb2 has been approved by RalfJung

It is now in the queue for this repository.

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

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

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.

Max RSS (memory usage)

Cycles

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%)

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

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.

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

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