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

Tracking Issue for option_as_slice #108545

Closed
3 tasks done
llogiq opened this issue Feb 27, 2023 · 12 comments
Closed
3 tasks done

Tracking Issue for option_as_slice #108545

llogiq opened this issue Feb 27, 2023 · 12 comments
Labels
C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC 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. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@llogiq
Copy link
Contributor

llogiq commented Feb 27, 2023

Feature gate: #![feature(option_as_slice)]

This is a tracking issue for the Option::as_slice and Option::as_mut_slice methods.

The functions return an immutable or mutable slice to the value contained in the Option, if any. Otherwise an empty slice is returned.

Public API

// core::option

impl Option {
    pub fn as_slice(&self) -> &[T] { .. }

    pub fn as_mut_slice(&mut self) -> &mut [T] { .. }
}

Steps / History

Unresolved Questions

The current implementation contains an optimization which relies on the fact that layout randomization as currently implemented only applies to individual variants, never to the discriminant, and thus the offset of the value within Some(value) is always either 0 (because of niche optimization) or would be the same if the type was Option<MaybeUninit<T>> instead of Option<T>.

Before stabilization, the implementation should be changed to either use an intrinsic to get the offset or extend the offset_of! macro (recently merged RFC#3308) to cover enum variants and use that. There might also be a smaller solution (that we'd still need to check re the rules of Rust layout), which would simply be mem::size_of::<Option<T>>() - mem::size_of::<T>(). I can see that this works for all types I've tested it with, and indeed I'm quite sure that it will work perfectly with the current layout implementation, but I'd like someone from the types team have a look at it before actually using it in a stable API.

The implementation now uses an approach that's always sound (just less efficient than optimal if the hack guesses wrong). A stabilization conversation would probably still want to discuss whether we're comfortable with shipping that approach as stable (since the implementation could be improved non-breakingly later) or we'd prefer to wait for a more principled approach (such as an extension of offset_of! from rust-lang/rfcs#3308 that allows enum variants) first.

Footnotes

  1. https://std-dev-guide.rust-lang.org/feature-lifecycle/stabilization.html

@llogiq llogiq added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC labels Feb 27, 2023
bors added a commit to rust-lang-ci/rust that referenced this issue Mar 13, 2023
…, r=the8472

Move `Option::as_slice` to an always-sound implementation

This approach depends on CSE to not have any branches or selects when the guessed offset is correct -- which it always will be right now -- but to also be *sound* (just less efficient) if the layout algorithms change such that the guess is incorrect.

The codegen test confirms that CSE handles this as expected, leaving the optimal codegen.

cc JakobDegen rust-lang#108545
@scottmcm
Copy link
Member

Updated the discussion in the OP now that the implementation relies on the layout estimate only for performance, not for soundness.

@llogiq
Copy link
Contributor Author

llogiq commented Mar 13, 2023

I'll push another version using an intrinsic shortly, so we'll have performance and soundness.

Edit: #109095

@llogiq
Copy link
Contributor Author

llogiq commented Mar 19, 2023

Superceded by #109179 with a more direct approach using an intrinsic instead of a hacky estimate.

@llogiq
Copy link
Contributor Author

llogiq commented Sep 17, 2023

Stabilization Report

Implementation History

API summary

The API is summarized at the top of this tracking issue.

Experience Report

Shortly after the initial implementation, #108678 dogfooded the feature in the compiler, adding 8 lines and removing 17. All four of the cases were both perf and legibility wins. The author notes that there he found some other cases outside of rustc where the feature could be applied, though isn't yet because of not being stabilized, even though his search was merely for a naive implementation of the feature, there may be cases where people have resorted to using a Vec or other type instead of an Option to get as_slice. Those cases aren't as easy to find though.

Stabilizing this feature will bring the standard library up to parity with the author's optional crate (though that is only for bools, ints and floats).


The API as it is now mirrors that of other collections (notably array, Vec, BinaryHeap and VecDeque as well as various of their iterators. The only possible extension that was hinted at during development was to Deref Option into a possibly empty slice, but this wasn't attempted due to being a breaking change.

Finally, I want to note that we can still change the implementation once offset_of! on enums is available, as the API surface will stay the same. So this seems like as good a time as any to stabilize this feature. Shall we?

ping @rust-lang/libs-api

@BurntSushi
Copy link
Member

@rfcbot merge

@rfcbot
Copy link

rfcbot commented Sep 17, 2023

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

@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. 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 Sep 17, 2023
@rfcbot
Copy link

rfcbot commented Sep 17, 2023

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

@AnthonyMikh
Copy link
Contributor

Is it a hard requirement for an empty slice to point to the same place as if Option was Some? Because the pointer part of slice just need to be non-null and sufficiently aligned, so using the address of an Option itself (as well as NonNull::dangling().as_ptr()) is always the sound approach.

@scottmcm
Copy link
Member

Is it a hard requirement for an empty slice to point to the same place as if Option was Some?

Well, the API doesn't guarantee it in the docs, so I don't think it's a hard requirement.

It's an important QoI detail for performance, though. By returning the empty slice pointing at where the item would be, it's always returning the same pointer. That simplifies codegen and optimizations, by not needing the extra conditional instruction.

Compare these: https://rust.godbolt.org/z/reqcM84xz

@llogiq
Copy link
Contributor Author

llogiq commented Sep 27, 2023

As there were no concerns brought up so far, I'll likely push a stabilization PR later today.

@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 27, 2023
@rfcbot
Copy link

rfcbot commented Sep 27, 2023

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.

@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Sep 28, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Oct 5, 2023
…r=BurntSushi

stabilize `Option::as_`(`mut_`)`slice`

This is the stabilization to rust-lang#108545. Thanks to everyone who helped getting this into Rust proper.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Oct 5, 2023
…r=BurntSushi

stabilize `Option::as_`(`mut_`)`slice`

This is the stabilization to rust-lang#108545. Thanks to everyone who helped getting this into Rust proper.
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Oct 5, 2023
Rollup merge of rust-lang#116220 - llogiq:stabilize-option-as-slice, r=BurntSushi

stabilize `Option::as_`(`mut_`)`slice`

This is the stabilization to rust-lang#108545. Thanks to everyone who helped getting this into Rust proper.
@ChayimFriedman2
Copy link
Contributor

This should be closed as the feature was stabilized.

@dtolnay dtolnay closed this as completed Feb 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC 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. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

8 participants