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 slice_first_last_chunk feature (slice::{split_,}{first,last}_chunk{,_mut}) #111774

Closed
2 of 6 tasks
clarfonthey opened this issue May 19, 2023 · 11 comments · Fixed by #117561 or #130428
Closed
2 of 6 tasks
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

@clarfonthey
Copy link
Contributor

clarfonthey commented May 19, 2023

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

  • 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 Add slice::{split_,}{first,last}_chunk{,_mut} #95198 (comment), @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.
  • ...

Footnotes

  1. https://std-dev-guide.rust-lang.org/feature-lifecycle/api-change-proposals.html

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

@clarfonthey clarfonthey added C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels May 19, 2023
@faern
Copy link
Contributor

faern commented May 26, 2023

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 ✨

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: mullvad/udp-over-tcp@8bdff7a#diff-0ff1562d6dac69ac5760658a8efce5e0368695f966f0c807bb5f1e023d1365b6R130-R131

@clarfonthey
Copy link
Contributor Author

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
Copy link
Contributor

faern commented May 27, 2023

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
Copy link
Contributor

Note previous similar proposal: #90091 . Both currently are implemented, so I think one of them should be removed from codebase

@Xiretza
Copy link
Contributor

Xiretza commented Jun 1, 2023

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
Copy link
Contributor

tgross35 commented Nov 1, 2023

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 #95198 (comment), @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?

@bors bors closed this as completed in 64461da Jan 20, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Jan 20, 2024
Rollup merge of rust-lang#117561 - tgross35:split-array, r=scottmcm

Stabilize `slice_first_last_chunk`

This PR does a few different things based around stabilizing `slice_first_last_chunk`. They are split up so this PR can be by-commit reviewed, I can move parts to a separate PR if desired.

This feature provides a very elegant API to extract arrays from either end of a slice, such as for parsing integers from binary data.

## Stabilize `slice_first_last_chunk`

ACP: rust-lang/libs-team#69
Implementation: rust-lang#90091
Tracking issue: rust-lang#111774

This stabilizes the functionality from rust-lang#111774:

```rust
impl [T] {
    pub const fn first_chunk<const N: usize>(&self) -> Option<&[T; N]>;
    pub 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 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 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], &[T; N])>;
    pub fn split_last_chunk_mut<const N: usize>(&mut self) -> Option<(&mut [T], &mut [T; N])>;
}
```

Const stabilization is included for all non-mut methods, which are blocked on `const_mut_refs`. This change includes marking the trivial function `slice_split_at_unchecked` const-stable for internal use (but not fully stable).

## Remove `split_array` slice methods

Tracking issue: rust-lang#90091
Implementation: rust-lang#83233 (review)

This PR also removes the following unstable methods from the `split_array` feature, rust-lang#90091:

```rust
impl<T> [T] {
    pub fn split_array_ref<const N: usize>(&self) -> (&[T; N], &[T]);
    pub fn split_array_mut<const N: usize>(&mut self) -> (&mut [T; N], &mut [T]);

    pub fn rsplit_array_ref<const N: usize>(&self) -> (&[T], &[T; N]);
    pub fn rsplit_array_mut<const N: usize>(&mut self) -> (&mut [T], &mut [T; N]);
}
```

This is done because discussion at rust-lang#90091 and its implementation PR indicate a strong preference for nonpanicking APIs that return `Option`. The only difference between functions under the `split_array` and `slice_first_last_chunk` features is `Option` vs. panic, so remove the duplicates as part of this stabilization.

This does not affect the array methods from `split_array`. We will want to revisit these once `generic_const_exprs` is further along.

## Reverse order of return tuple for `split_last_chunk{,_mut}`

An unresolved question for rust-lang#111774 is whether to return `(preceding_slice, last_chunk)` (`(&[T], &[T; N])`) or the reverse (`(&[T; N], &[T])`), from `split_last_chunk` and `split_last_chunk_mut`. It is currently implemented as `(last_chunk, preceding_slice)` which matches `split_last -> (&T, &[T])`. The first commit changes these to `(&[T], &[T; N])` for these reasons:

- More consistent with other splitting methods that return multiple values: `str::rsplit_once`, `slice::split_at{,_mut}`, `slice::align_to` all return tuples with the items in order
- More intuitive (arguably opinion, but it is consistent with other language elements like pattern matching `let [a, b, rest @ ..] ...`
- If we ever added a varidic way to obtain multiple chunks, it would likely return something in order: `.split_many_last::<(2, 4)>() -> (&[T], &[T; 2], &[T; 4])`
- It is the ordering used in the `rsplit_array` methods

I think the inconsistency with `split_last` could be acceptable in this case, since for `split_last` the scalar `&T` doesn't have any internal order to maintain with the other items.

## Unresolved questions

Do we want to reserve the same names on `[u8; N]` to avoid inference confusion? rust-lang#117561 (comment)

---

`slice_first_last_chunk` has only been around since early 2023, but `split_array` has been around since 2021.

`@rustbot` label -T-libs +T-libs-api -T-libs +needs-fcp
cc `@rust-lang/wg-const-eval,` `@scottmcm` who raised this topic, `@clarfonthey` implementer of `slice_first_last_chunk` `@jethrogb` implementer of `split_array`

Zulip discussion: https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/Stabilizing.20array-from-slice.20*something*.3F

Fixes: rust-lang#111774
github-actions bot pushed a commit to rust-lang/miri that referenced this issue Jan 21, 2024
Stabilize `slice_first_last_chunk`

This PR does a few different things based around stabilizing `slice_first_last_chunk`. They are split up so this PR can be by-commit reviewed, I can move parts to a separate PR if desired.

This feature provides a very elegant API to extract arrays from either end of a slice, such as for parsing integers from binary data.

## Stabilize `slice_first_last_chunk`

ACP: rust-lang/libs-team#69
Implementation: rust-lang/rust#90091
Tracking issue: rust-lang/rust#111774

This stabilizes the functionality from rust-lang/rust#111774:

```rust
impl [T] {
    pub const fn first_chunk<const N: usize>(&self) -> Option<&[T; N]>;
    pub 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 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 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], &[T; N])>;
    pub fn split_last_chunk_mut<const N: usize>(&mut self) -> Option<(&mut [T], &mut [T; N])>;
}
```

Const stabilization is included for all non-mut methods, which are blocked on `const_mut_refs`. This change includes marking the trivial function `slice_split_at_unchecked` const-stable for internal use (but not fully stable).

## Remove `split_array` slice methods

Tracking issue: rust-lang/rust#90091
Implementation: rust-lang/rust#83233 (review)

This PR also removes the following unstable methods from the `split_array` feature, rust-lang/rust#90091:

```rust
impl<T> [T] {
    pub fn split_array_ref<const N: usize>(&self) -> (&[T; N], &[T]);
    pub fn split_array_mut<const N: usize>(&mut self) -> (&mut [T; N], &mut [T]);

    pub fn rsplit_array_ref<const N: usize>(&self) -> (&[T], &[T; N]);
    pub fn rsplit_array_mut<const N: usize>(&mut self) -> (&mut [T], &mut [T; N]);
}
```

This is done because discussion at #90091 and its implementation PR indicate a strong preference for nonpanicking APIs that return `Option`. The only difference between functions under the `split_array` and `slice_first_last_chunk` features is `Option` vs. panic, so remove the duplicates as part of this stabilization.

This does not affect the array methods from `split_array`. We will want to revisit these once `generic_const_exprs` is further along.

## Reverse order of return tuple for `split_last_chunk{,_mut}`

An unresolved question for #111774 is whether to return `(preceding_slice, last_chunk)` (`(&[T], &[T; N])`) or the reverse (`(&[T; N], &[T])`), from `split_last_chunk` and `split_last_chunk_mut`. It is currently implemented as `(last_chunk, preceding_slice)` which matches `split_last -> (&T, &[T])`. The first commit changes these to `(&[T], &[T; N])` for these reasons:

- More consistent with other splitting methods that return multiple values: `str::rsplit_once`, `slice::split_at{,_mut}`, `slice::align_to` all return tuples with the items in order
- More intuitive (arguably opinion, but it is consistent with other language elements like pattern matching `let [a, b, rest @ ..] ...`
- If we ever added a varidic way to obtain multiple chunks, it would likely return something in order: `.split_many_last::<(2, 4)>() -> (&[T], &[T; 2], &[T; 4])`
- It is the ordering used in the `rsplit_array` methods

I think the inconsistency with `split_last` could be acceptable in this case, since for `split_last` the scalar `&T` doesn't have any internal order to maintain with the other items.

## Unresolved questions

Do we want to reserve the same names on `[u8; N]` to avoid inference confusion? rust-lang/rust#117561 (comment)

---

`slice_first_last_chunk` has only been around since early 2023, but `split_array` has been around since 2021.

`@rustbot` label -T-libs +T-libs-api -T-libs +needs-fcp
cc `@rust-lang/wg-const-eval,` `@scottmcm` who raised this topic, `@clarfonthey` implementer of `slice_first_last_chunk` `@jethrogb` implementer of `split_array`

Zulip discussion: https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/Stabilizing.20array-from-slice.20*something*.3F

Fixes: #111774
@RalfJung RalfJung reopened this Sep 6, 2024
@RalfJung
Copy link
Member

RalfJung commented Sep 6, 2024

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

@RalfJung RalfJung added the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Sep 6, 2024
@rfcbot
Copy link

rfcbot commented Sep 6, 2024

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 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. labels Sep 6, 2024
@dtolnay dtolnay removed the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Sep 6, 2024
@rfcbot rfcbot added 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 24, 2024
@rfcbot
Copy link

rfcbot commented Sep 24, 2024

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

@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 Oct 4, 2024
@rfcbot
Copy link

rfcbot commented Oct 4, 2024

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.

@bors bors closed this as completed in 386a0ba Oct 5, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Oct 5, 2024
Rollup merge of rust-lang#130428 - ink-feather-org:stabilize-const-split-at-mut, r=RalfJung

Stabilize `const_slice_split_at_mut` and `const_slice_first_last_chunk`

Stabilizes rust-lang#101804 and the remainder of rust-lang#111774.

FCP proposed in the tracking issue.

Requires rust-lang#130403 (or it would need a rustc_allow_const_fn_unstable for it)

Stabilized const API:
```rust
// slice
impl [T] {
    pub const fn split_at_mut(&mut self, mid: usize) -> (&mut [T], &mut [T]);
    pub const fn split_at_mut_unchecked(&mut self, mid: usize) -> (&mut [T], &mut [T]);
    pub const fn split_at_mut_checked(&mut self, mid: usize) -> Option<(&mut [T], &mut [T])>;

    pub const fn first_chunk_mut<const N: usize>(&mut self) -> Option<&mut [T; N]>;
    pub const fn last_chunk_mut<const N: usize>(&mut self) -> Option<&mut [T; N]>;
    pub const fn split_first_chunk_mut<const N: usize>(&mut self) -> Option<(&mut [T; N], &mut [T])>;
    pub const fn split_last_chunk_mut<const N: usize>(&mut self) -> Option<(&mut [T; N], &mut [T])>;
}
```

Closes rust-lang#101804
Closes rust-lang#111774

cc `@RalfJung`
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Oct 17, 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
9 participants