diff --git a/tests/snippets/builtin_slice.py b/tests/snippets/builtin_slice.py index 402a90133..b7c3922c0 100644 --- a/tests/snippets/builtin_slice.py +++ b/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 diff --git a/vm/src/obj/objsequence.rs b/vm/src/obj/objsequence.rs index bc4d46008..5d85b12a0 100644 --- a/vm/src/obj/objsequence.rs +++ b/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) -> Self; + fn do_slice_reverse(&self, range: Range) -> Self; + fn do_stepped_slice(&self, range: Range, step: usize) -> Self; + fn do_stepped_slice_reverse(&self, range: Range, step: usize) -> Self; + fn empty() -> Self; fn len(&self) -> usize; fn get_pos(&self, p: i32) -> Option { 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, stop: &Option) -> Range { + 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 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 PySliceableSequence for Vec { - fn do_slice(&self, start: usize, stop: usize) -> Self { - self[start..stop].to_vec() + fn do_slice(&self, range: Range) -> 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) -> Self { + let mut slice = self[range].to_vec(); + slice.reverse(); + slice } + + fn do_stepped_slice(&self, range: Range, step: usize) -> Self { + self[range].iter().step_by(step).cloned().collect() + } + + fn do_stepped_slice_reverse(&self, range: Range, 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), }, diff --git a/vm/src/obj/objstr.rs b/vm/src/obj/objstr.rs index 88561a899..3ec520aed 100644 --- a/vm/src/obj/objstr.rs +++ b/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) -> 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) -> Self { + to_graphemes(self) + .get_mut(range) + .map_or(String::default(), |slice| { + slice.reverse(); + slice.join("") + }) + } + + fn do_stepped_slice(&self, range: Range, 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, step: usize) -> Self { + if let Some(s) = to_graphemes(self).get(range) { + return s + .iter() + .rev() + .cloned() + .step_by(step) + .collect::>() + .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?)",