implement slice negative step handling

tests/snippets/builtin_slice.py

@@ -15,6 +15,38 @@ assert b[-2**100:] == [1, 2]
 assert b[2**100:] == []
 assert b[::2**100] == [1]
 assert b[-10:1] == [1]
+assert b[0:0] == []
+assert b[1:0] == []
+
+try:
+    _ = b[::0]
+except ValueError:
+    pass
+else:
+    assert False, "Zero step slice should raise ValueError"
+
+assert b[::-1] == [2, 1]
+assert b[1::-1] == [2, 1]
+assert b[0::-1] == [1]
+assert b[0:-5:-1] == [1]
+assert b[:0:-1] == [2]
+assert b[5:0:-1] == [2]
+
+c = list(range(10))
+
+assert c[9:6:-3] == [9]
+assert c[9::-3] == [9, 6, 3, 0]
+assert c[9::-4] == [9, 5, 1]
+assert c[8::-2**100] == [8]
+
+assert c[7:7:-2] == []
+assert c[7:8:-2] == []
+
+d = "123456"
+
+assert d[3::-1] == "4321"
+assert d[4::-3] == "52"
+
 
 slice_a = slice(5)
 assert slice_a.start is None

vm/src/obj/objsequence.rs

@@ -3,14 +3,17 @@ use super::super::vm::VirtualMachine;
 use super::objbool;
 use super::objint;
 use num_bigint::BigInt;
-use num_traits::{Signed, ToPrimitive};
+use num_traits::{One, Signed, ToPrimitive, Zero};
 use std::cell::{Ref, RefMut};
 use std::marker::Sized;
-use std::ops::{Deref, DerefMut};
+use std::ops::{Deref, DerefMut, Range};
 
 pub trait PySliceableSequence {
-    fn do_slice(&self, start: usize, stop: usize) -> Self;
-    fn do_stepped_slice(&self, start: usize, stop: usize, step: usize) -> Self;
+    fn do_slice(&self, range: Range<usize>) -> Self;
+    fn do_slice_reverse(&self, range: Range<usize>) -> Self;
+    fn do_stepped_slice(&self, range: Range<usize>, step: usize) -> Self;
+    fn do_stepped_slice_reverse(&self, range: Range<usize>, step: usize) -> Self;
+    fn empty() -> Self;
     fn len(&self) -> usize;
     fn get_pos(&self, p: i32) -> Option<usize> {
         if p < 0 {
@@ -41,38 +44,56 @@ pub trait PySliceableSequence {
         }
     }
 
-    fn get_slice_items(&self, slice: &PyObjectRef) -> Self
+    fn get_slice_range(&self, start: &Option<BigInt>, stop: &Option<BigInt>) -> Range<usize> {
+        let start = start.as_ref().map(|x| self.get_slice_pos(x)).unwrap_or(0);
+        let stop = stop
+            .as_ref()
+            .map(|x| self.get_slice_pos(x))
+            .unwrap_or(self.len());
+
+        start..stop
+    }
+
+    fn get_slice_items(
+        &self,
+        vm: &mut VirtualMachine,
+        slice: &PyObjectRef,
+    ) -> Result<Self, PyObjectRef>
     where
         Self: Sized,
     {
         // TODO: we could potentially avoid this copy and use slice
         match &(slice.borrow()).payload {
             PyObjectPayload::Slice { start, stop, step } => {
-                let start = if let Some(start) = start {
-                    self.get_slice_pos(start)
-                } else {
-                    0
-                };
-                let stop = if let Some(stop) = stop {
-                    self.get_slice_pos(stop)
-                } else {
-                    self.len()
-                };
-                if let Some(step) = step {
-                    match step.to_i32() {
-                        Some(1) => self.do_slice(start, stop),
-                        Some(num) => self.do_stepped_slice(start, stop, num as usize),
-                        None => self.do_slice(
-                            start,
-                            if start == self.len() {
-                                start
-                            } else {
-                                start + 1
-                            },
-                        ),
+                let step = step.clone().unwrap_or(BigInt::one());
+                if step.is_zero() {
+                    Err(vm.new_value_error("slice step cannot be zero".to_string()))
+                } else if step.is_positive() {
+                    let range = self.get_slice_range(start, stop);
+                    if range.start < range.end {
+                        match step.to_i32() {
+                            Some(1) => Ok(self.do_slice(range)),
+                            Some(num) => Ok(self.do_stepped_slice(range, num as usize)),
+                            None => Ok(self.do_slice(range.start..range.start + 1)),
+                        }
+                    } else {
+                        Ok(Self::empty())
                     }
                 } else {
-                    self.do_slice(start, stop)
+                    // calculate the range for the reverse slice, first the bounds needs to be made
+                    // exclusive around stop, the lower number
+                    let start = start.as_ref().map(|x| x + 1);
+                    let stop = stop.as_ref().map(|x| x + 1);
+                    let range = self.get_slice_range(&stop, &start);
+                    if range.start < range.end {
+                        match (-step).to_i32() {
+                            Some(1) => Ok(self.do_slice_reverse(range)),
+                            Some(num) => Ok(self.do_stepped_slice_reverse(range, num as usize)),
+                            None => Ok(self.do_slice(range.end - 1..range.end)),
+                        }
+                    } else {
+                        Ok(Self::empty())
+                    }
                 }
             }
             payload => panic!("get_slice_items called with non-slice: {:?}", payload),
@@ -81,12 +102,28 @@ pub trait PySliceableSequence {
 }
 
 impl<T: Clone> PySliceableSequence for Vec<T> {
-    fn do_slice(&self, start: usize, stop: usize) -> Self {
-        self[start..stop].to_vec()
+    fn do_slice(&self, range: Range<usize>) -> Self {
+        self[range].to_vec()
     }
-    fn do_stepped_slice(&self, start: usize, stop: usize, step: usize) -> Self {
-        self[start..stop].iter().step_by(step).cloned().collect()
+
+    fn do_slice_reverse(&self, range: Range<usize>) -> Self {
+        let mut slice = self[range].to_vec();
+        slice.reverse();
+        slice
     }
+
+    fn do_stepped_slice(&self, range: Range<usize>, step: usize) -> Self {
+        self[range].iter().step_by(step).cloned().collect()
+    }
+
+    fn do_stepped_slice_reverse(&self, range: Range<usize>, step: usize) -> Self {
+        self[range].iter().rev().step_by(step).cloned().collect()
+    }
+
+    fn empty() -> Self {
+        Vec::new()
+    }
+
     fn len(&self) -> usize {
         self.len()
     }
@@ -116,7 +153,7 @@ pub fn get_item(
         PyObjectPayload::Slice { .. } => Ok(PyObject::new(
             match &(sequence.borrow()).payload {
                 PyObjectPayload::Sequence { .. } => PyObjectPayload::Sequence {
-                    elements: elements.to_vec().get_slice_items(&subscript),
+                    elements: elements.to_vec().get_slice_items(vm, &subscript)?,
                 },
                 ref payload => panic!("sequence get_item called for non-sequence: {:?}", payload),
             },

vm/src/obj/objstr.rs

@@ -8,6 +8,7 @@ use super::objsequence::PySliceableSequence;
 use super::objtype;
 use num_traits::ToPrimitive;
 use std::hash::{Hash, Hasher};
+use std::ops::Range;
 // rust's builtin to_lowercase isn't sufficient for casefold
 extern crate caseless;
 extern crate unicode_segmentation;
@@ -1003,14 +1004,23 @@ fn str_new(vm: &mut VirtualMachine, args: PyFuncArgs) -> PyResult {
 }
 
 impl PySliceableSequence for String {
-    fn do_slice(&self, start: usize, stop: usize) -> Self {
+    fn do_slice(&self, range: Range<usize>) -> Self {
         to_graphemes(self)
-            .get(start..stop)
+            .get(range)
             .map_or(String::default(), |c| c.join(""))
     }
 
-    fn do_stepped_slice(&self, start: usize, stop: usize, step: usize) -> Self {
-        if let Some(s) = to_graphemes(self).get(start..stop) {
+    fn do_slice_reverse(&self, range: Range<usize>) -> Self {
+        to_graphemes(self)
+            .get_mut(range)
+            .map_or(String::default(), |slice| {
+                slice.reverse();
+                slice.join("")
+            })
+    }
+
+    fn do_stepped_slice(&self, range: Range<usize>, step: usize) -> Self {
+        if let Some(s) = to_graphemes(self).get(range) {
             return s
                 .iter()
                 .cloned()
@@ -1021,6 +1031,23 @@ impl PySliceableSequence for String {
         String::default()
     }
 
+    fn do_stepped_slice_reverse(&self, range: Range<usize>, step: usize) -> Self {
+        if let Some(s) = to_graphemes(self).get(range) {
+            return s
+                .iter()
+                .rev()
+                .cloned()
+                .step_by(step)
+                .collect::<Vec<String>>()
+                .join("");
+        }
+        String::default()
+    }
+
+    fn empty() -> Self {
+        String::default()
+    }
+
     fn len(&self) -> usize {
         to_graphemes(self).len()
     }
@@ -1052,7 +1079,8 @@ pub fn subscript(vm: &mut VirtualMachine, value: &str, b: PyObjectRef) -> PyResu
     } else {
         match (*b.borrow()).payload {
             PyObjectPayload::Slice { .. } => {
-                Ok(vm.new_str(value.to_string().get_slice_items(&b).to_string()))
+                let string = value.to_string().get_slice_items(vm, &b)?;
+                Ok(vm.new_str(string))
             }
             _ => panic!(
                 "TypeError: indexing type {:?} with index {:?} is not supported (yet?)",