Tracking issue for slice_take

[cramertj]

Feature gate: #![feature(slice_take)]

Public API

impl<T> [T] {
    fn split_off<'a, R: OneSidedRange<usize>>(self: &mut &'a Self, range: R) -> Option<&'a Self>;
    fn split_off_mut<'a, R: OneSidedRange<usize>>(self: &mut &'a mut Self, range: R) -> Option<&'a mut Self>;
    fn split_off_first<'a>(self: &mut &'a Self) -> Option<&'a T>;
    fn split_off_first_mut<'a>(self: &mut &'a mut Self) -> Option<&'a mut T>;
    fn split_off_last<'a>(self: &mut &'a Self) -> Option<&'a T>;
    fn split_off_last_mut<'a>(self: &mut &'a mut Self) -> Option<&'a mut T>;
}


// core::ops

trait OneSidedRange<T: ?Sized>: RangeBounds<T> {}
impl<T> OneSidedRange<T> for RangeTo<T> where Self: RangeBounds<T>;
impl<T> OneSidedRange<T> for RangeFrom<T> where Self: RangeBounds<T>;
impl<T> OneSidedRange<T> for RangeToInclusive<T> where Self: RangeBounds<T>;

Steps / History

• Introduce <&[_]>::split_off and <&str>::split_off #49173
• Add take_... functions to slices #62282
• Take 2: Add take_... functions to slices #77065
• Add slice take methods #88502
• Rename slice::take... methods to split_off... #136555
• Final comment period (FCP)
• Stabilization PR

Unresolved Questions

[SimonSapin]

https://doc.rust-lang.org/nightly/std/ops/trait.OneSidedRange.html links to #69780 which is closed. Either it should be reopened (and some details filled in) or #[unstable] attributes should be changed to point here instead, depending on whether it’s useful to track OneSidedRange separately.

[lilyball]

Can we implement OneSidedRange for RangeFull as well? I'd like to be able to say self.slice.take(..) as a shorthand for std::mem::swap(&mut self.slice, &[]) (or I suppose for self.slice.take(0..)). This doesn't fit the naming OneSidedRange or the description that says the range is unbounded on one side¹, but I don't see any reason why RangeFull should not be usable here.

Footnotes

1. Technically it doesn't say it has to be bounded on the other side, but it does list .. as a non-compliant type. ↩

[andersk]

Can we implement OneSidedRange for RangeFull as well?

Perhaps also rename OneSidedRange to UnboundedRange, in that case?

[safinaskar]

Please, rename. Method take is unusable with its current name, because it conflicts with std::io::Read::take.

Let me describe my actual production use case.

First I read some data from file using std::io::Read::read_to_end. To be able to read data I have to write use std::io::Read. Then I started to parse that data using various functions, including <[T]>::take. But I was unable to do this because of use std::io::Read, see playground: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=e467873898a5f160420a5d6f13274ffa

[clarfonthey]

FYI: #69780 is still being linked for OneSideRange in the docs when that issue is closed; should the tracking issue be moved to here?

[jongiddy]

I agree with @safinaskar that a different name for take (and take_mut) should be considered. I like take_slice and take_slice_mut for the current API.

But changing the API to replace take(range) with take_before(usize) and take_after(usize) (and possibly take_all()) would be clearer and removes the need to resolve issues with OneSidedRange.

[jongiddy]

I'd love to see taking from a slice stabilized. But the introduction of OneSidedRange seems to be the wrong path to take. I looked at the previous discussion for the benefits of adding OneSidedRange. They are:

• "pretty neat"
• "looks nice"
• "works with regular indices rather than distances from the end of the slice"
• "don't have the method naming problem and don't have to add so many methods"
• "might be useful in a couple of other scenarios"

OneSidedRange did not solve the method naming problem, the original design without OneSidedRange can also use regular indices rather than length from the end, and I don't see any other code that uses OneSidedRange. The only objective benefit realized from the above is that the number of methods reduced from 8 to 6.

Meanwhile the OneSidedRange code requires two match statements to turn the range parameter back into a single index and then call separate paths for the two possible directions. It also adds panic code for the unreachable variants when changing the range to an index and a direction.

The benefits of OneSidedRange just do not justify the additional complexity, especially as part of the standard library.

I propose the creation of a new implementation PR to remove OneSidedRange and replace take with take_before and take_after methods + _mut variants. I'm happy to make the PR (but also happy for someone else to do it). But I'd like to check if anyone is strongly opposed first.

cc @nox @cramertj @timvermeulen @ibraheemdev who worked on the previous implementation PR's.

[WaffleLapkin]

@safinaskar this conflict is indeed very unfortunate, however do note that there are workarounds. Notably you can:

• Import the Read trait in a different module or function (you can write use inside a function) than the usage of the take method
• Explicitly take mutable reference ((&mut s).take(..1))
• Explicitly write the function (<[_]>::take(&mut s, ..1))
• Use the method on a mut ref (s.take(..1) where s has type &mut &[_])

See playground.

[WaffleLapkin]

@jongiddy I do find the current version with OneSidedRange a lot more readable than potential take_before/take_after (and a third variant?).

Meanwhile the OneSidedRange code requires two match statements to turn the range parameter back into a single index and then call separate paths for the two possible directions. It also adds panic code for the unreachable variants when changing the range to an index and a direction.

I do not see the problem here. We have a lot of matches in std and this was never a problem. If you are concerned about speed, then you don't need to be, compiler can fully optimize those matches out, see this godbolt for example (note how function implemented as-is in std with OneSidedRange has exactly the same asm as the function which is implemented using an approach you are proposing). By the panic I assume you mean unreachable!() in split_point_of which is unreachable and is also fully optimized-out by the compiler.

[Kolsky]

Perhaps split_off could solve the naming problem? It's also reserved for collections, although in this case an argument would be R: UnboundedRange rather than usize. You could also implement both immutable and mutable variants under a single name, if orphan rules allow inherent impls on references (and change where the documentation would reside). It's not as if take would be useful on &mut [T].

Edit: Regarding the usefulness of UnboundedRange, I think it's necessary because the index approach is unclear. Which part of the slice is returned and which part stays in source? The naming alone wouldn't necessarily prevent accidental misuse, especially given code refactoring (replacing one with the other). Note that normal splits do not have this problem, because they return tuples, and tuples are explicitly ordered.

[Kimundi]

I just discovered this API, after I once again reimplemented my own helper function for splitting off a prefix or suffix of a mutable slice :D

Looking at the comments, my feedback:

• Name could be different than take, for the stated reasons
• I like the range parameter solution, as it reduced the number of named methods (we already have too many for splitting)
• I like the split_off suggestion, its just unfortunate that it overlaps with methods on owning datastructures like Vec or LinkedList. However, the BTree* types use the same name for a slightly different API already, so...
• That said, having a split_off on vecs and a split_off on slices that work differently would definitely be annoying, so we should pick a different name.
• If we wanted to be annoying, we could name it split_affix, because it removes either a suffix or prefix of the slice 😅
• How about split_in_place?
• Or maybe just the stupidly simple solution, split_range?

Example:

let x = slice.split_range(..a);
let x = slice.split_range(b..);
let x = slice.split_range(..);

let x = slice.split_range_mut(..a);
let x = slice.split_range_mut(b..);
let x = slice.split_range_mut(..);

[kennytm]

1. -1 renaming the method to split_«anything» because all existing <[T]>::split* methods use &[T] as the receiver rather than the double-reference &mut &[T]

2. If we really want to rename these methods I suggest .pop_*() or .drain_*().

3. +1 on using .verb(..4) rather than .verb_before(4), but -1 on calling the trait std::ops::OneSidedRange<usize> or std::ops::UnboundedRange<usize>. I don't think anybody needed a generic one/zero-sided range trait, in particular this trait cannot support {start: Excluded, end: Unbounded}. I think the trait could just live as std::slice::VerbRange similar to std::slice::SliceIndex.

4. these methods should can be implemented for str. what happened when the range does not hit the char boundary is an unresolved question.

   impl str {
       fn verb<'a>(self: &mut &'a Self, range: impl VerbRange) -> Option<&'a Self>;
       fn verb_mut<'a>(self: &mut &'a mut Self, range: impl VerbRange) -> Option<&'a mut Self>;
   //  fn verb_first_char<'a>(self: &mut &'a Self) -> Option<char>;
   //  fn verb_last_char<'a>(self: &mut &'a Self) -> Option<char>;
       fn verb_prefix<'a>(self: &mut &'a Self, pattern: impl Pattern<'a>) -> Option<&'a Self>;
       fn verb_suffix<'a>(self: &mut &'a Self, pattern: impl Pattern<'a>) -> Option<&'a Self>;
   }

[SimonSapin]

what happened when the range does not hit the char boundary is an unresolved question.

There is precedent for split_at to panic when given an index not at a char boundary. (Although split_at_checked is now being added)

[clarfonthey]

Side note about full ranges being included in this API, but I think calling these "unbounded ranges" instead of "one-sided ranges" would make the APIs a bit clear. Even though the one-sided ranges have bounds, they still contain an unbounded side.

[NyxCode]

I just stumbled over this feature after writing take_first (I called it pop_front) myself, and wondering if the standard library has something similar. I regularly use &mut &[T] and &mut &str, which I think is a super cool pattern I'd like to see more support for.

Has pop_front and pop_back been considered instead of take_first and take_last? That would mirror the API of VecDeque, which supports cheap "removal" of elements from the front and back as well.

[ds84182]

Should this also have an array version? e.g.

    fn take_first_chunk<'a, const N: usize>(self: &mut &'a Self) -> Option<&'a [T; N]>;
    fn take_first_chunk_mut<'a, const N: usize>(self: &mut &'a mut Self) -> Option<&'a mut [T; N]>;
    fn take_last_chunk<'a, const N: usize>(self: &mut &'a Self) -> Option<&'a [T; N]>;
    fn take_last_chunk_mut<'a, const N: usize>(self: &mut &'a mut Self) -> Option<&'a mut [T; N]>;

Would be very useful for reading and writing chunks of bytes from a slice. u32::from_le_bytes(*(slice.take_first_chunk()))

[JarredAllen]

Is anything blocking these methods from being stabilized? I'd love to start using them (also, +1 on _chunk variants as suggested above), as I end up rewriting them often.

[cramertj]

@JarredAllen This has been stuck on naming for years.

IMO @rust-lang/libs-api should:

1. Pick the method name (the existing take or one of the above suggestions split_off_{...}, extract_{...}, remove_{...}, etc.).
2. Decide on a different name for OneSidedRange (e.g. SplitOffIndex or UnboundedRange).

I don't think there are other stabilization blockers, and I don't think this benefits from a whole lot more community discussion (the original PR was sent in 2018!). @rust-lang/libs-api should make a final call on naming so that we can stabilize.

[scottmcm]

A recent thread where these came up: https://internals.rust-lang.org/t/methods-to-pop-from-a-slice/22211?u=scottmcm

[dtolnay]

We discussed this feature in today's standard library API team meeting. We generally liked the current design, including 6 methods ({range, first, last} × {non-mut, mut}) and OneSidedRange trait. From reviewing the discussion, our most preferred naming is split_off. With s/take/split_off/, which is implemented in #136555, this should be ready to stabilize shortly.

[Amanieu]

Should the OneSidedRange::bound method also be included in stabilization? It is currently documented as an internal-only function, and yet OneSidedRange is not a sealed trait.

[cramertj]

@Amanieu I hadn't imagined that stabilizing this feature would stabilize OneSidedRange trait nor the ability to implement it. If it is to be made stable, IMO it should be sealed. Which approach sounds better to you?

[clarfonthey]

I don't think that it should be sealed, since for example SliceIndex is stable and public but its methods are not. I would imagine this should be a similar case: stabilise the trait itself, but don't stabilise any of its methods, so that people can use the trait as a bound in their code when calling this method specifically.

I also think that the idea of making the trait UnboundedRange and implementing for RangeFull is a good idea as well, but nobody has really commented on this since I mentioned it: #62280 (comment)

I'm less concerned about the specific name and more concerned about the exclusion of RangeFull, and that the name would be incorrect if it were included. (I guess that .. is equivalent to 0.. and you could argue that it does have one bounded side, but that feels like you're just trying too hard to justify a name that isn't great.)

[JarredAllen]

Any chance split_off_{first,last}_chunk{,_mut} (splitting off the first/last N elements as a reference to an array) could join in the stabilization? Those feel to me like they fit with the 6 being currently discussed for stabilization, and I'd love to have them also be available for use.

[hanna-kruppe]

I'm less concerned about the specific name and more concerned about the exclusion of RangeFull, and that the name would be incorrect if it were included. (I guess that .. is equivalent to 0.. and you could argue that it does have one bounded side, but that feels like you're just trying too hard to justify a name that isn't great.)

What’s the use case for passing RangeFull to these? I saw one comment talking about using slice.take(..) in the same way one might use vec.drain(..), which made sense to me with the old take name. But with the new split_off name, which emphasizes the “one-sided” angle, I think split_off(0..) is less confusing than split_off(..) and perfectly sufficient.

[clarfonthey]

I'm less concerned about the specific name and more concerned about the exclusion of RangeFull, and that the name would be incorrect if it were included. (I guess that .. is equivalent to 0.. and you could argue that it does have one bounded side, but that feels like you're just trying too hard to justify a name that isn't great.)

What’s the use case for passing RangeFull to these? I saw one comment talking about using slice.take(..) in the same way one might use vec.drain(..), which made sense to me with the old take name. But with the new split_off name, which emphasizes the “one-sided” angle, I think split_off(0..) is less confusing than split_off(..) and perfectly sufficient.

I agree that split_off as a name fits well with the range being one-sided, but again, I think that take(..) is a meaningful operation and we should start with the operation we want to do first, rather than trying to change the operation to justify the name.

[hanna-kruppe]

I don’t understand what you mean by that. split_off(0..) is another way to spell the same operation, right? I don’t think it’s worth enabling the spelling split_off(..) because with the split_off name it’s not very natural anymore. And I don’t want to re-open the naming bike shed just to have a different spelling for an operation that can already be expressed adequately with the currently proposed naming.

[hanna-kruppe]

Also note that the same operation can be spelled as mem::take(&mut slice) (because there's a Default impl for &[T]) in cases where the "taking the whole slice" POV is more natural/appropriate than the "splitting off the 0.. range" POV. One could even propose a new take() method (no arguments!) for slices modeled after Option::take. This may be the easiest way to side-step the thorny question of what to call a method that would cover both "split off a prefix/suffix" and "take the entire slice" use cases.