Add panic-safe slicing methods

[barosl]

Yeppeun version

[mdinger]

Is there any reason the originals need panics? Isn't it reasonable to just return an empty slice? Other languages do that. Also, failing when the edges aren't perfect seems silly. I know panic!() or None keeps edge alignment accurate but that last and first statements still seems perfectly valid.

&[..90]  // [1, 2, 3, 4, 5]
&[90..1] // []
&[90..104] // []

[SimonSapin]

Could the existing RangeArgument trait be used instead of Rangeable?

[Gankra]

@mdinger triggering this behaviour hints at a logic error. Do we have any other cases where the standard library "squashes" inputs in a best-effort way? (nothing off the top of my head)

[mdinger]

I suppose but it also isn't a strong guarantee at all. These 2 are just as incorrect but only one of each errors:

// Both just as wrong when you want the entire slice but go one too low
&[1..]
&[-1..]
// Both just as wrong when you want up to the last element
&[..len-1]
&[..len+1]

From collections, this isn't an error but returns None:

use std::collections::BTreeMap;
let mut map = BTreeMap::new();
map.insert("foo", 89);

println!("{:?}", map.get("charles"));

So, likewise it'd make sense for a disjoint slice to return a None as well but slice already has a term for that. It's called [] (similarly, (slightly offtopic) it seems indexing off the end should return option: x[inf] return None but x[0] return Some(number) but that'd probably get really painful to work with so...).

I just can't see how panic! could be considered as a valid result for these operations. They're really really easy to recover from.

[mdinger]

If the slicing operator is supposed to preserve the length of the input in the output, that is &[0..3] -> &[7, 8, 9] but never &[0..3] -> &[7] then the current behaviour would make sense but that is definitely less flexible and weird. I'd almost expect an extra keyword to enforce this rigidity: for example &[0..3].strict().

[seanmonstar]

but never &[0..3] -> &[7] then the current behaviour would make sense but that is definitely less flexible and weird

To add to the subjectivity of what is weird or isn't: I would found it surprising if assert_eq!(&foo[..3].len(), 3) was not true.

[mdinger]

Okay, then if preserving the size is part of correct slice behaviour then the current behaviour should be fine (I don't have any exact use case anyway) but if the reason for the panics is to strictly to prevent off the end memory access, that seems silly; they should return whatever the valid intersection happens to be.

It's too bad normal slicing can't return option (too unergonomic I guess). This RFC seems like a good idea.

[Gankra]

This is kind've a fundamental philosophical difference. Some have argued that indexing should behave optimistically and do "modulo len", but I think that's definitely step too far (also incoherent on empty slices). Being strict on all indexing seems consistent to me.

Of course we can't magically detect semantic errors like len-1, but at very least we can catch the instances of "obviously something wrong" and report them to be helpful.

E: within limits. A check must be made, but making extra checks to be "extra helpful" is a harder pill to swallow.

[mdinger]

Okay. I didn't know it had previously been discussed and it looked kinda like something which had just slipped through unnoticed. Strictness does fit the Rust theme so it isn't too unusual I guess.

[glaebhoerl]

I dunno. For normal indexing where you need to return an &T, there's no reliable way to deal with an out of bounds index, notably on an empty slice, except to panic - you can't just conjure an &T out of thin air. For slicing, returning an &[T], by contrast, there very much is. Whereas with indexing there's nothing else reasonable you could do, here it's just a matter of use cases and expectations: do you expect it to always return a slice of the exact length you requested, and otherwise panic, or do you expect it to take the intersection of the requested slice and the actually-existing one? Both of these seem like perfectly reasonable expectations to me. I think the best way to resolve the question would be to collect data on which behavior is more commonly desired in code-in-the-wild.

[Gankra]

For all my code, the indices are derived from properties of the to-be-sliced value, so if they're ever messed up it means my code is wrong, and I'd like to know ASAP.

[mdinger]

Well, indexing returning option would easily handle the out of bounds case (unless it was [] without an inner type but that doesn't seem to be allowed). I don't think it would be easy to measure how desired each style is because it's heavily biased towards the currently implemented style. Also, the time when you'd very much want one over the other may be quite a niche case.

[mdinger]

An alternative would be:

assert_eq!(a.get_range(1..), &a[1..]);
assert_eq!(a.get_range(..3), &a[..3]);
assert_eq!(a.get_range(2..5), &a[2..5]);
assert_eq!(a.get_range(..6), &[]);
assert_eq!(a.get_range(4..2), &[]);

[SimonSapin]

For what it’s worth, out of bounds slices are clamped when indexing in Python:

>>> [1, 2, 3][:2]
[1, 2]
>>> [1, 2, 3][:10]
[1, 2, 3]

[seanmonstar]

Other data point, from Java, which in some ways its strictness is similar
Rust's:
http://docs.oracle.com/javase/7/docs/api/java/util/Arrays.html#copyOfRange(T[],%20int,%20int)

If start or end are out of bounds, IndexOutOfBoundsException is thrown.

On Wed, Oct 21, 2015, 1:12 AM Simon Sapin notifications@github.com wrote:

For what it’s worth, out of bounds slices are clamped when indexing in
Python:

[1, 2, 3][:2]
[1, 2]>>> [1, 2, 3][:10]
[1, 2, 3]

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

[bluss]

Fuzzy slice boundaries are even more strange if you apply the thought experiment to &str (char boundaries need to be respected).

I totally support the idea behind the RFC, nonpanicking slicing would be great. Implementation though, I think a trait named for Index would be better.

• Consider returning Result<&X, usize> — a lot of current code wants to report which index was out of bounds, for example in panic messages
• The Range trait should probably have its own RFC (see RFC: .drain(range) and .drain() #1257). Range discussion seems to be ongoing (see ordered ranges 2.0 #1254).

[ticki]

👍 We had to implement this in the Redox kernel to avoid kernel panics. We see no reason for this method not existing in libcore.

[ghost]

I also had to deal with this when writing kernel code. 👍

[Kimundi]

Could't we just make get() as generic as the indexing operators, to allow both .get(n) and .get(x..y)?

[nagisa]

@Kimundi breaking change, I think?

[Kimundi]

@nagisa: Not really, if the generic method still accepts the current usize as one possible generic type then its backwards compatible in the same way as a number of changes that already landed.

[Gankra]

Doing as @Kimundi proposes would of course necessitate making get/get_mut into traits (or: adding adhoc overloading).

There are two problems with this:

• doc regression because traits suck in docs
• need to add them to prelude to avoid breakage (OR: add inherent trait impls)

I definitely think it'd be reasonable to have a solution here, I'm just wondering if we're lacking the tooling today to make the solution we'd want longterm, at the moment.

[SimonSapin]

Instead of having get/get_mut be trait methods themselves, they could be inherent methods generic over the type of their parameter (with a trait bound on that parameter) like Vec::drain.

[Kimundi]

Yeah, just making them generic methods is what I had in mind.

[briansmith]

ring also had to implement this: https://github.com/briansmith/ring/blob/00830b53fd1c93e20bdba1f6e130ad8816b25a17/src/input.rs#L275

[Ms2ger]

Seems like a useful addition; reusing get() is nicely symmetric with the slicing syntax.

[alexcrichton]

@barosl would you be interested in updating this RFC to recommend reusing the existing get method? The libs team discussed this RFC during triage and that seemed like an appealing alternative for us.

[alexcrichton]

The libs team discussed this RFC recently and the conclusion was that we'd like to move forward, but we've been unable to contact the author. @sfackler has opened an updated version of this RFC over at #1679 which we believe represents the consensus here, so I'm going to close in favor of that.