From c360b87de414d13378d86ddb2732204f00a9043f Mon Sep 17 00:00:00 2001
From: Jim Turner <github@turner.link>
Date: Thu, 9 May 2019 15:18:12 -0400
Subject: [PATCH] Fix slice offset computation for special cases

With the old implementation, this resulted in undefined behavior in
release mode and a panic in debug mode:

```rust
let mut arr = Array2::<i32>::zeros((5, 5));
arr.slice_axis_inplace(Axis(0), Slice::new(0, Some(0), -1));
```

as did this:

```rust
let mut arr = Array2::from_shape_vec((1, 1).strides((10, 1)), vec![5]).unwrap();
arr.slice_axis_inplace(Axis(0), Slice::new(1, Some(1), 1));
```

Now, both examples operate correctly.
---
 src/dimension/mod.rs | 35 +++++++++++++++++++++++------------
 tests/array.rs       | 10 ++++++++++
 2 files changed, 33 insertions(+), 12 deletions(-)

diff --git a/src/dimension/mod.rs b/src/dimension/mod.rs
index aebcfc662..f8667d272 100644
--- a/src/dimension/mod.rs
+++ b/src/dimension/mod.rs
@@ -367,18 +367,29 @@ pub fn do_slice(dim: &mut usize, stride: &mut usize, slice: Slice) -> isize {
     let m = end - start;
     let s = (*stride) as isize;
 
-    // Data pointer offset
-    let mut offset = stride_offset(start, *stride);
-    // Adjust for strides
-    //
-    // How to implement negative strides:
-    //
-    // Increase start pointer by
-    // old stride * (old dim - 1)
-    // to put the pointer completely in the other end
-    if step < 0 {
-        offset += stride_offset(m - 1, *stride);
-    }
+    // Compute data pointer offset.
+    let offset = if m == 0 {
+        // In this case, the resulting array is empty, so we *can* avoid performing a nonzero
+        // offset.
+        //
+        // In two special cases (which are the true reason for this `m == 0` check), we *must* avoid
+        // the nonzero offset corresponding to the general case.
+        //
+        // * When `end == 0 && step < 0`. (These conditions imply that `m == 0` since `to_abs_slice`
+        //   ensures that `0 <= start <= end`.) We cannot execute `stride_offset(end - 1, *stride)`
+        //   because the `end - 1` would underflow.
+        //
+        // * When `start == *dim && step > 0`. (These conditions imply that `m == 0` since
+        //   `to_abs_slice` ensures that `start <= end <= *dim`.) We cannot use the offset returned
+        //   by `stride_offset(start, *stride)` because that would be past the end of the axis.
+        0
+    } else if step < 0 {
+        // When the step is negative, the new first element is `end - 1`, not `start`, since the
+        // direction is reversed.
+        stride_offset(end - 1, *stride)
+    } else {
+        stride_offset(start, *stride)
+    };
 
     // Update dimension.
     let abs_step = step.abs() as usize;
diff --git a/tests/array.rs b/tests/array.rs
index 2739dd582..de6de93c2 100644
--- a/tests/array.rs
+++ b/tests/array.rs
@@ -96,6 +96,16 @@ fn test_slice()
     assert!(vi.iter().zip(A.iter()).all(|(a, b)| a == b));
 }
 
+#[test]
+fn test_slice_edge_cases() {
+    let mut arr = Array3::<u8>::zeros((3, 4, 5));
+    arr.slice_collapse(s![0..0;-1, .., ..]);
+    assert_eq!(arr.shape(), &[0, 4, 5]);
+    let mut arr = Array2::<u8>::from_shape_vec((1, 1).strides((10, 1)), vec![5]).unwrap();
+    arr.slice_collapse(s![1..1, ..]);
+    assert_eq!(arr.shape(), &[0, 1]);
+}
+
 #[test]
 fn test_slice_inclusive_range() {
     let arr = array![[1, 2, 3], [4, 5, 6]];