im-rs icon indicating copy to clipboard operation
im-rs copied to clipboard

`Focus::narrow` panics on empty ranges, ranges to 0, indexing into empty `Vector` - `std::slice` does not

Open micouy opened this issue 4 years ago • 1 comments

In Focus::narrow's docs it says:

Focus::narrow(range) has the same effect as &slice[range], without actually modifying the underlying vector. Panics if the range isn't fully inside the current focus.

When trying to narrow the focus to an empty range (either x..x or ..0), the program panics:

// im
let data = vector![1, 2, 3, 4, 5];

let s1 = data.focus().narrow(..0); // panics
let s2 = data.focus().narrow(2..2); // panics

However, analogous indexing into std::slice does not cause a panic:

// std
let data = vec![1, 2, 3, 4, 5];

let s3 = &data[..0]; // does not panic, returns []
let s4 = &data[2..2]; // does not panic

Neither does indexing into an empty slice.

// std
let data: Vec<i32> = vec![];
let s5 = &data[..0]; // does not panic
let s6 = &data[..]; // does not panic
// im
let data: Vector<i32> = vector![];
let s7 = data.focus().narrow(..); // panics

Slice indexing panics only when begin > end or end > slice.len().

// std
let data = vec![1, 2, 3, 4, 5];

let s8 = &data[3..2]; // panics
let s9 = &data[10..10]; // panics
let s10 = &data[0..10]; // panics

This is how SliceIndex<[T]> is implemented for Range<usize>:

// std
fn index(self, slice: &[T]) -> &[T] {
    if self.start > self.end {
        slice_index_order_fail(self.start, self.end);
    } else if self.end > slice.len() {
        slice_index_len_fail(self.end, slice.len());
    }
    unsafe { self.get_unchecked(slice) }
}

Other range types reuse this code, i. e. RangeTo constructs a new Range:

// std
(0..self.end).get(slice)

Here's im's code:

// im
if r.start >= r.end || r.start >= self.len() {
    panic!("vector::Focus::narrow: range out of bounds");
}

This behavior not only differs from std's implementation, it also doesn't allow to write code generic over Vector's length and forces the user to perform checks. Either Focus::narrow should be fixed or the docs changed so that the rules are clear and warn about panics. "Panics if the range isn't fully inside the current focus." is too ambiguous in this case.

micouy avatar Jul 24 '20 12:07 micouy

I hit the same issue. I tried to work around it using focus.split_at(0).0 in the 0 case and that also panics.

Its almost like empty focus is not allowed, but it is, since in my case I have an empty vector, and I can get a focus to it (which is empty) but none of the other ways to create an empty focus work.

This seems very inconsistent.

Craig-Macomber avatar Sep 23 '22 01:09 Craig-Macomber