rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
98.32k stars 12.72k forks source link

Tracking Issue for `slice_first_last_chunk` feature (`slice::{split_,}{first,last}_chunk{,_mut}`) #111774

Closed clarfonthey closed 1 month ago

clarfonthey commented 1 year ago

Feature gate: #![feature(slice_first_last_chunk)]

This is a tracking issue for the slice::{split_,}{first,last}_chunk{,_mut} API.

Public API

2024-09-06: Most of this is stable now, except for the const-ness of the mut methods.

impl [T] {
    pub const fn first_chunk<const N: usize>(&self) -> Option<&[T; N]>;
    pub const fn first_chunk_mut<const N: usize>(&mut self) -> Option<&mut [T; N]>;
    pub const fn last_chunk<const N: usize>(&self) -> Option<&[T; N]>;
    pub const fn last_chunk_mut<const N: usize>(&mut self) -> Option<&mut [T; N]>;
    pub const fn split_first_chunk<const N: usize>(&self) -> Option<(&[T; N], &[T])>;
    pub const fn split_first_chunk_mut<const N: usize>(&mut self) -> Option<(&mut [T; N], &mut [T])>;
    pub const fn split_last_chunk<const N: usize>(&self) -> Option<(&[T; N], &[T])>;
    pub const fn split_last_chunk_mut<const N: usize>(&mut self) -> Option<(&mut [T; N], &mut [T])>;
}

Note that this mirrors the existing, already-stabilised API:

impl [T] {
    pub const fn first(&self) -> Option<&T>;
    pub const fn first_mut(&mut self) -> Option<&mut T>;
    pub const fn last(&self) -> Option<&T>;
    pub const fn last_mut(&mut self) -> Option<&mut T>;
    pub const fn split_first(&self) -> Option<(&T, &[T])>;
    pub const fn split_first_mut(&mut self) -> Option<(&mut T, &mut [T])>;
    pub const fn split_last(&self) -> Option<(&T, &[T])>;
    pub const fn split_last_mut(&mut self) -> Option<(&mut T, &mut [T])>;
}

Steps / History

Unresolved Questions

faern commented 1 year ago

I have been yearning for better byte parsing with methods like these for a long time. So I jumped on trying out split_first_chunk immediately. Was hoping it would greatly simplify parsing out fields of known lengths from buffers of serialized data. And it did! It did simplify a case of: buffer.get(header_start..header_end) + <[u8; HEADER_LEN]>::try_from(header).unwrap() into just buffer.split_first_chunk::<HEADER_LEN>()? and I love this! Way less code, risk of indexing mistakes, and no unwraps/panics involved at all :sparkles:

However, the lack of non-const-generic version of the same thing did bite me right after. It's really sad that slice::split_at is panicking and not returning Option<(&[T], &[T])>. (This is being proposed in #90410)

My experiment with this new API: https://github.com/mullvad/udp-over-tcp/commit/8bdff7abc58ab326a25cf545db137f10b4780ef9#diff-0ff1562d6dac69ac5760658a8efce5e0368695f966f0c807bb5f1e023d1365b6R130-R131

clarfonthey commented 1 year ago

I'm a bit confused what you mean about there not being a const version of this API-- split_at and co. are already const-unstable, and this method is also const-unstable. There is a bit going on with the various get methods since they would rely on const traits (which are currently being removed to pave way for keyword generics), but at least those methods should work fine for you.

faern commented 1 year ago

The added features under this tracking issue are awesome. I approve. What I mean is there is no method to split a slice where the index is not const generic and it does not panic (split_at but without panic is what I want. But that's what the issue I linked to is about)

safinaskar commented 1 year ago

Note previous similar proposal: https://github.com/rust-lang/rust/issues/90091 . Both currently are implemented, so I think one of them should be removed from codebase

Xiretza commented 1 year ago

The previously stated advantage of this over the existing #90091 was "solv[ing] the Option-ness question" - I think #109049 should resolve that as well (while mostly keeping #90091 the same)?

tgross35 commented 1 year ago

Is this the best order for the tuple in split_last_chunk? Consistency with split_last would have it be -> Option<(&[T;N], &[T])> as was ACPed, but it might be confusing to have the two slice-like things in the opposite order from every other API I can think of that returns two subslices of the original. In https://github.com/rust-lang/rust/pull/95198#discussion_r1204824761, @scottmcm expected -> Option<(&[T;N], &[T])> for split_first_chunk, but for it to be -> Option<(&[T], &[T;N])> in split_last_chunk so the tuple is "in order", in some sense.

I think this should be changed such that [a, b, c, d].split_last_chunk::<2>() returns ([a, b], [c, d]). This is not consistent specifically with split_{first,last} but it is consistent with str::rsplit_once and other buffer splitting methods (slice::split_at{_mut}, slice::align_to). I think if you could ever rsplit more than one item that it would make more sense to return their natural order too, rather than reversing.

Maybe we could justify that we want to be more consistent with rsplit_once and let [a, b, middle @ .., d, e] else ..., and call the pop-like split_{first,last} methods the special case?

RalfJung commented 2 months ago

This has been closed, but is still listed as the tracking issue for const_slice_first_last_chunk. But with const_mut_refs in FCP, I think we can go ahead and stabilize const_slice_first_last_chunk as well. :)

Cc @rust-lang/libs-api

dtolnay commented 2 months ago

@rfcbot fcp merge

const for these already-stable functions:

rfcbot commented 2 months ago

Team member @dtolnay 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 commented 1 month ago

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

rfcbot commented 1 month ago

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.