Calling .tail() on a empty slice panics

[elinorbgr]

Calling .tail() on a slice does no check at all (see https://github.com/rust-lang/rust/blob/master/src/libcore/slice.rs#L210 ), and will panic if called on a empty slice.

Nothing suggests it in the documentation, I personally expected that calling .tail() on an empty slice would return an empty slice, as .first() returns an Option.

[richo]

It seems like it should either return an empty slice, or just an Option ?

[oli-obk]

imho either an Option or a panic are the right choices. An Option is nicer since you can choose to unwrap or not.

[steveklabnik]

Array access out of bound generally panics. Doesn't mean the docs can't be clearer, though.

I thought we discussed Option, but I can't remember.

[elinorbgr]

Also, I find it weird to have .head() return an option and .tail() panicking, as they are more or less counterparts of each other.
One would expect that both have the same behavior, in my opinion.

[GuillaumeGomez]

I agree with @vberger. It seems logical that tail and head methods return the same type.

[richo]

I will PR this tonight, does it require an RFC?

On Tue, Apr 7, 2015 at 12:41 PM, Guillaume Gomez notifications@github.com
wrote:

I agree with @vberger https://github.com/vberger. It seems logical that
tail and head methods return the same type.

—
Reply to this email directly or view it on GitHub
#24141 (comment).

[GuillaumeGomez]

@richo: I was going to do it when someone would have give the start. Too bad for me...

[richo]

If you want to do it, feel free!

On Tue, Apr 7, 2015 at 1:28 PM, Guillaume Gomez notifications@github.com
wrote:

@richo https://github.com/richo: I was going to do it when someone
would have give the start. Too bad for me...

—
Reply to this email directly or view it on GitHub
#24141 (comment).

[GuillaumeGomez]

@richo: Well then I do it ! Thanks ^^

[richo]

Still needs decision on RFC though, @steveklabnik ?

[Gankra]

Probably doesn't need a full RFC, but not guaranteed to be merged.

[steveklabnik]

Given that this is an unstable interface, I would assume changing it is just fine. But given the recent events today, I'm not exactly jumping at stamping non-rfc breaking changes, know what I mean?

The PR will be small and not likely to rot, so I don't think it'd be harmful to send it regardless, though.

[GuillaumeGomez]

Ok, thanks for your confirmation @steveklabnik. I'm "fixing" that then !

[barosl]

Also, I find it weird to have .head() return an option and .tail() panicking, as they are more or less counterparts of each other.

Strangely enough, there is no head() in std. I guess first() is the function you meant, which returns Option<&T>.

So first() + tail() == init() + last() == the original list. This sounds a bit confusing as there is tail() but not head(). Is this name deliberate?

[lilyball]

It's only confusing if you're coming from a language like Haskell that defines head and tail. I'd actually like it if we could get away from tail / init terminology entirely, as it's not really that intuitive, but I don't know what a good alternative is. For comparison, Swift uses the names dropFirst and dropLast, but I don't think we want to use those names here (as we already use the term "drop" to mean something specific).

I wonder if maybe we should actually move to something like

fn pop_first<'a>(&'a self) -> Option<&'a self::Item, &'a [Self::Item]>
fn pop_last<'a>(&'a self) -> Option<&'a self::Item, &'a [Self::Item]>

These would replace tail() / init() (but we'd still have first() / last() because removing those would probably be confusing). You could then recover tail() / init() with xs.pop_first().unwrap().1 and xs.pop_last().unwrap().1.

I'm suggesting this approach both because it fixes the naming, and because I'd expect that code that uses tail() / init() is likely to also want to inspect that first/last value.

[elinorbgr]

We can note that there is already StrExt::slice_shift_char that follows a similar pattern:

fn slice_shift_char<'a>(&'a self) -> Option<(char, &'a str)>

(that is what I was actually using before replacing my &strs by &[u8]s, and I was quite disappointed to not find an equivalent)

[nikomatsakis]

Copying my thoughts from #24184: Hmm, I have mixed reactions. I admire your attempt to find something categorically better than init/tail, and this may be in the right direction. And I really quite strongly dislike the name init, so I'm inclined to like anything that changes that (prefix, anybody?). But I do have some concerns:

1. The names pop_first and pop_last seem misleading in that they do not mutate the underlying data. I'd prefer a name like split, though split currently returns two slices. Perhaps split_front_elem or divide or something like that. (Reading this thread again, I recall the term shift, which might also be ok.)
2. The change to return a tuple is nice when it works, but slice.pop_last().unwrap().1 is quite a mouthful (as compared to slice.init()). On the other hand, it's probably mostly the unwrap, which is the whole point of this PR.
3. Note that we made a deliberate decision to have many slice methods continue to panic on invalid indices precisely because when we attempted to convert them all to return Option, it seemed by and large to just make code messier.

(Also, in collections, we tend to use the names "front" and "back", so maybe we should stick to that, rather than "first/last". Or does that seem specific to LinkedList?)

[bombless]

When I'm coding, I consider it this way:
Check if it is empty, if you're sure then you can take the tail.
So I feel happy even when this API is untouched.

[elinorbgr]

Indeed returning Options everywhere can be quite a heavy, but in this case it would be appropriate to document when a function can panic.

However, it raises an other point: where should it be documented in this case ? Documenting it on the trait would imply that every implementation is expected to panic, and I'm not sure we can document an implementation in a way that makes it well visible in the docs.

[GuillaumeGomez]

@bombless: Once again, I think this is a personal preference. I prefer checking a returned Option over checking length before calling the function.

@vberger: I totally agree, functions that can panic should be documented more, why it can panic is very important to avoid bad surprises.

[bombless]

Maybe off-topic, I missed the day when Vec::remove return Option<T> for a while, at that time I wrote a lot of while let Some(x) = vec.remove(0) { }, and I just like it written in this way, so I know the feeling.
(alexcrichton@9d919d2)
As for this case, I guess we can introduce new syntax similar as ->! to trait methods, perhaps -> ?!.

[GuillaumeGomez]

-> ?! seems a bit too much, no ?

[bombless]

Forget about it, it turns out that in general people can hardly guarantee if a method can panic in some condition.
Even C++ is dropping their exception specification.

[alexcrichton]

To the best of my knowledge, our consensus some time ago with respect to slicing methods was that index syntax with ranges is the main way of creating a slice, and all methods will panic if range endpoints are out of bounds. We definitely realized, however, that there was a large use case for methods returning Option, if just not as common.

If we were to provide Option-based variants, however, we would be violating one of our principle error handling guidelines which needs to be considered as well. All in all this is why we haven't stabilized these methods yet!

With respect to @kballard's proposal I agree with @nikomatsakis that it's a pretty nice signature to have, but the naming may need some work. @vberger raises a good point that slice_shift_char is quite similar in terms of signature, and we have also not stabilized that method largely because of its name (as far as I know at least).

I personally like @kballard's PR (#24184) but would want to figure out a better name (or at least convention for the name) beforehand.

[lilyball]

I chose "pop" because I wanted the connotation of "and return the removed element", but I agree that it's potentially misleading as "pop" usually mutates the receiver. I'd be fine with renaming these to shift_first() and shift_last(), if it's felt that "shift" doesn't have quite as strong a connotation of mutating the receiver. Other than that, I don't have any particularly good ideas on the naming front.

The change to return a tuple is nice when it works, but slice.pop_last().unwrap().1 is quite a mouthful (as compared to slice.init()). On the other hand, it's probably mostly the unwrap, which is the whole point of this PR.

I agree, I think it's mostly the unwrap. You'll note that I changed at least one usage of x.tail() to &x[1..] because that's simpler, but there's no clean equivalent for init() (as slicing syntax doesn't have any way to refer to end-minus-some-offset).

3 of the 4 uses of slice.pop_last().unwrap().1 are in the context of producing an iterator. I was actually a bit surprised to realize we don't have a .skipLast() iterator adaptor. If we added such an adaptor, that would let us get rid of those 3 cases and leave just a single case in the rust codebase of slice.pop_last().unwrap().1.

in collections, we tend to use the names "front" and "back", so maybe we should stick to that, rather than "first/last"

collections::slice already has methods .first() and .last(), which is why I used the first/last naming. I feel like "front" and "back" is more specific to deques / doubly-linked lists.

If we were to provide Option-based variants, however, we would be violating one of our principle error handling guidelines which needs to be considered as well.

That's an important point. Personally, I think we aren't violating it; calling .pop_first() and getting None back isn't an error, it's a successful result; an empty slice has no first element, so there's nothing to pop. Or to put it another way, the method is analogous to the stable first() function, which returns an Option (and returns None on empty slices).

---

At the moment, I'm inclined to change the name to shift_first() / shift_last() and leave it at that.

[alexcrichton]

@kballard it looks like there's actually a bit of design space here that we may want to explore in a broader forum, would be interested in writing an RFC for these two functions?

[lilyball]

Ok, I'll try to do that this evening.

[alexcrichton]

Thanks!

[lilyball]

RFC has been submitted as rust-lang/rfcs#1058

[tbu-]

Outdated because rust-lang/rfcs#1058 was accepted, and implemented in #26966, deprecating tail.

[steveklabnik]

👍