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

ACP: slice::{split_,}{first,last}_chunk #69

Closed
clarfonthey opened this issue Jul 16, 2022 · 10 comments
Closed

ACP: slice::{split_,}{first,last}_chunk #69

clarfonthey opened this issue Jul 16, 2022 · 10 comments
Labels
ACP-accepted API Change Proposal is accepted (seconded with no objections) api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api

Comments

@clarfonthey
Copy link

Proposal

This is an ACP for rust-lang/rust#95198, which I believe predates ACPs. Either that or I just didn't know about them at the time.

Problem statement

In some cases, it's valuable to convert between slices and arrays, and there's not really a good way to do this safely.

Motivation, use-cases

The current best API we have for this is the unstable as_chunks, which leads to the below being used fairly often:

slice.as_chunks::<N>().0[0]

Which just isn't as readable.

Solution sketches

The proposed solution:

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])>;
}

This borrows directly from the first/last APIs:

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])>;
}

Links and related work

The PR with the proposed solution: rust-lang/rust#95198
Discussion in the array_chunks tracking issue: rust-lang/rust#74985

What happens now?

This issue is part of the libs-api team API change proposal process. Once this issue is filed the libs-api team will review open proposals in its weekly meeting. You should receive feedback within a week or two.

@clarfonthey clarfonthey added api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api labels Jul 16, 2022
@the8472
Copy link
Member

the8472 commented Jul 16, 2022

It's unfortunate that split_first already exists, otherwise I would have suggested naming it pop_first instead.

@clarfonthey
Copy link
Author

Naming it pop_first would definitely be confusing since the Deref implementation on Vec would imply something actually being removed, when it's just splitting the slice.

@m-ou-se
Copy link
Member

m-ou-se commented Aug 23, 2022

We briefly discussed this in the libs-api meeting just now, and we're not sure how much value this adds, since the existing array_chunks (etc.) seem to cover most (all?) of these.

We tend to be very cautious to add many more methods to very common slices like slices. Does any of the proposed functionality currently require unsafe or overly verbose code?

@m-ou-se
Copy link
Member

m-ou-se commented Aug 23, 2022

An alternative might be to consider an alternative where this functionality is added to the ArrayChunks type.

@clarfonthey
Copy link
Author

I'll have to get back to you on the iterator alternatives; I think they may cover it, but I haven't done a full sweep since I wrote this.

@cuviper
Copy link
Member

cuviper commented Aug 26, 2022

We would need array_rchunks to cover last_chunk, or you can write slice.as_rchunks().1.last(). Split is harder though, to get the remainder and all other chunks in a single slice.

@clarfonthey
Copy link
Author

Yeah, I would imagine that iterators cover most of the cases, though. Not necessarily ergonomic for the split, but I personally don't know how often people would use it.

My main issue with as_chunks is that, while it works, having to index into a slice inside a tuple makes the code look kind of cryptic. While it's not unclear, it definitely has a cognitive complexity to it-- I know that .0[0] means "the first of the first," but it takes a moment to fully realise what that means.

@shepmaster
Copy link
Member

shepmaster commented Aug 26, 2022

having to index into a slice inside a tuple

I expect most times I’ll destructure the tuple:

let (chunks, tail) = foo.as_chunks();

probably often with an assert!(tail.is_empty()).

@m-ou-se
Copy link
Member

m-ou-se commented May 17, 2023

We discussed this again in a libs meeting, and we seem to feel more positive about these methods now. Please go ahead and open a new tracking issue for these, to add these as unstable. Thanks!

@clarfonthey
Copy link
Author

Just adding this here for those following, the tracking issue is #111774 and the PR #95198 is now ready for review. (Since I don't think linking an issue triggers notifications.)

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue May 25, 2023
Add slice::{split_,}{first,last}_chunk{,_mut}

This adds to the existing tracking issue for `slice::array_chunks` (rust-lang#74985) under a separate feature, `slice_get_chunk`.

Currently, we have the existing `first`/`last` API for slices:

```rust
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])>;
}
```

This augments it with a `first_chunk`/`last_chunk` API that allows retrieving multiple elements at once:

```rust
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])>;
}
```

The code is based off of a copy of the existing API, with the documentation and examples properly modified. Currently, the most common way to perform these kinds of lookups with the existing methods is via `slice.as_chunks::<N>().0[0]` or the worse `slice.as_chunks::<N>().0[slice.len() - N]`, which is substantially less readable than `slice.first_chunk::<N>()` or `slice.last_chunk::<N>()`.

ACP: rust-lang/libs-team#69
thomcc pushed a commit to tcdi/postgrestd that referenced this issue Jul 18, 2023
Add slice::{split_,}{first,last}_chunk{,_mut}

This adds to the existing tracking issue for `slice::array_chunks` (#74985) under a separate feature, `slice_get_chunk`.

Currently, we have the existing `first`/`last` API for slices:

```rust
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])>;
}
```

This augments it with a `first_chunk`/`last_chunk` API that allows retrieving multiple elements at once:

```rust
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])>;
}
```

The code is based off of a copy of the existing API, with the documentation and examples properly modified. Currently, the most common way to perform these kinds of lookups with the existing methods is via `slice.as_chunks::<N>().0[0]` or the worse `slice.as_chunks::<N>().0[slice.len() - N]`, which is substantially less readable than `slice.first_chunk::<N>()` or `slice.last_chunk::<N>()`.

ACP: rust-lang/libs-team#69
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jan 19, 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#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
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ACP-accepted API Change Proposal is accepted (seconded with no objections) api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api
Projects
None yet
Development

No branches or pull requests

6 participants