From 4d8a56aea1d86d861c488e1eb4fd8ee7669090b4 Mon Sep 17 00:00:00 2001 From: Rachel Powers Date: Sun, 7 Apr 2019 19:09:44 -0600 Subject: [PATCH 1/8] `list.__setitem__` with slice and int indexing optimisation is uncertain correctness is uncertain but it does appear to work --- vm/src/obj/objlist.rs | 173 ++++++++++++++++++++++++++++++++++++++---- 1 file changed, 159 insertions(+), 14 deletions(-) diff --git a/vm/src/obj/objlist.rs b/vm/src/obj/objlist.rs index 60c14b107..3a4035409 100644 --- a/vm/src/obj/objlist.rs +++ b/vm/src/obj/objlist.rs @@ -11,11 +11,11 @@ use crate::pyobject::{IdProtocol, PyContext, PyObjectRef, PyRef, PyResult, PyVal use crate::vm::{ReprGuard, VirtualMachine}; use super::objbool; -use super::objint; +//use super::objint; use super::objiter; use super::objsequence::{ get_elements, get_elements_cell, get_item, seq_equal, seq_ge, seq_gt, seq_le, seq_lt, seq_mul, - PySliceableSequence, SequenceIndex, + SequenceIndex, }; use super::objslice::PySliceRef; use super::objtype; @@ -181,22 +181,167 @@ impl PyListRef { } } - fn setitem(self, key: PyObjectRef, value: PyObjectRef, vm: &VirtualMachine) -> PyResult { - let mut elements = self.elements.borrow_mut(); + fn setitem( + self, + subscript: SequenceIndex, + value: PyObjectRef, + vm: &VirtualMachine, + ) -> PyResult { + match subscript { + SequenceIndex::Int(index) => self.setindex(index, value, vm), + SequenceIndex::Slice(slice) => { + // TODO check special case of if a == b, may need copy of b first? + self.setslice(slice, value, vm) + } + } + } - if objtype::isinstance(&key, &vm.ctx.int_type()) { - let idx = objint::get_value(&key).to_i32().unwrap(); - if let Some(pos_index) = elements.get_pos(idx) { - elements[pos_index] = value; - Ok(vm.get_none()) + fn setindex(self, index: i32, value: PyObjectRef, vm: &VirtualMachine) -> PyResult { + if let Some(pos_index) = self.get_pos(index) { + self.elements.borrow_mut()[pos_index] = value; + Ok(vm.get_none()) + } else { + Err(vm.new_index_error("list assignment index out of range".to_string())) + } + } + + fn setslice(self, slice: PySliceRef, sec: PyObjectRef, vm: &VirtualMachine) -> PyResult { + let step = slice.step.clone().unwrap_or_else(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(&slice.start, &slice.stop); + if range.start < range.end { + match step.to_i32() { + Some(1) => self._set_slice(range, sec, vm), + Some(num) => { + // assign to extended slice + self._set_stepped_slice(range, num as usize, sec, vm) + } + None => { + // not sure how this is reached, step too big for i32? + // then step is bigger than the than len of the list, no question + #[allow(clippy::range_plus_one)] + self._set_stepped_slice(range.start..(range.start + 1), 1, sec, vm) + } + } } else { - Err(vm.new_index_error("list index out of range".to_string())) + // this functions as an insert of sec before range.start + self._set_slice(range.start..range.start, sec, vm) + // TODO this will take some special processing } } else { - panic!( - "TypeError: indexing type {:?} with index {:?} is not supported (yet?)", - elements, key - ) + // calculate the range for the reverse slice, first the bounds needs to be made + // exclusive around stop, the lower number + let start = &slice.start.as_ref().map(|x| x + 1); + let stop = &slice.stop.as_ref().map(|x| x + 1); + let range = self.get_slice_range(&stop, &start); + match (-step).to_i32() { + Some(num) => self._set_stepped_slice_reverse(range, num as usize, sec, vm), + None => { + // not sure how this is reached, step too big for i32? + // then step is bigger than the than len of the list no question + self._set_stepped_slice_reverse(range.end - 1..range.end, 1, sec, vm) + } + } + } + } + + fn _set_slice(self, range: Range, sec: PyObjectRef, vm: &VirtualMachine) -> PyResult { + // consume the iter, we don't need it's size + // but if it's going to fail we want that to happen *before* we start modifing + if let Ok(items) = vm.extract_elements(&sec) { + // replace the range of elements with the full sequence + self.elements.borrow_mut().splice(range, items); + + Ok(vm.get_none()) + } else { + Err(vm.new_type_error("can only assign an iterable to a slice".to_string())) + } + } + + fn _set_stepped_slice( + self, + range: Range, + step: usize, + sec: PyObjectRef, + vm: &VirtualMachine, + ) -> PyResult { + // consume the iter, we need it's size + // and if it's going to fail we want that to happen *before* we start modifing + let slicelen = ((range.end - range.start - 1) / step) + 1; + if let Ok(items) = vm.extract_elements(&sec) { + let n = items.len(); + + if range.start < range.end { + if n == slicelen { + let indexes = range.step_by(step); + self._replace_indexes(indexes, &items); + Ok(vm.get_none()) + } else { + Err(vm.new_value_error(format!( + "attempt to assign sequence of size {} to extended slice of size {}", + n, slicelen + ))) + } + } else { + // empty slice but this is an error because stepped slice + Err(vm.new_value_error(format!( + "attempt to assign sequence of size {} to extended slice of size 0", + n + ))) + } + } else { + Err(vm.new_type_error("can only assign an iterable to a slice".to_string())) + } + } + + fn _set_stepped_slice_reverse( + self, + range: Range, + step: usize, + sec: PyObjectRef, + vm: &VirtualMachine, + ) -> PyResult { + // consume the iter, we need it's size + // and if it's going to fail we want that to happen *before* we start modifing + let slicelen = ((range.end - range.start - 1) / step) + 1; + if let Ok(items) = vm.extract_elements(&sec) { + let n = items.len(); + + if range.start < range.end { + if n == slicelen { + let indexes = range.rev().step_by(step); + self._replace_indexes(indexes, &items); + Ok(vm.get_none()) + } else { + Err(vm.new_value_error(format!( + "attempt to assign sequence of size {} to extended slice of size {}", + n, slicelen + ))) + } + } else { + // empty slice but this is an error because stepped slice + Err(vm.new_value_error(format!( + "attempt to assign sequence of size {} to extended slice of size 0", + n + ))) + } + } else { + Err(vm.new_type_error("can only assign an iterable to a slice".to_string())) + } + } + + fn _replace_indexes(self, indexes: I, items: &[PyObjectRef]) + where + I: Iterator, + { + let mut elements = self.elements.borrow_mut(); + + for (i, value) in indexes.zip(items) { + // clone for refrence count + elements[i] = value.clone(); } } From 118c98ccc76977945e78495b28124b42290162f6 Mon Sep 17 00:00:00 2001 From: Rachel Powers Date: Sun, 7 Apr 2019 19:12:03 -0600 Subject: [PATCH 2/8] exhaustive (hopefully) test snippets I attempted to cover every case with the snippets but I easily could of missed some. --- tests/snippets/list.py | 198 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 198 insertions(+) diff --git a/tests/snippets/list.py b/tests/snippets/list.py index 3d9743c0b..71f7dea06 100644 --- a/tests/snippets/list.py +++ b/tests/snippets/list.py @@ -201,3 +201,201 @@ assert_raises(TypeError, bad_del_1) def bad_del_2(): del ['a', 'b'][2] assert_raises(IndexError, bad_del_2) + +# __setitem__ + +# simple index +x = [1, 2, 3, 4, 5] +x[0] = 'a' +assert x == ['a', 2, 3, 4, 5] +x[-1] = 'b' +assert x == ['a', 2, 3, 4, 'b'] +# make sure refrences are assigned correctly +y = [] +x[1] = y +y.append(100) +assert x[1] == y +assert x[1] == [100] + +#index bounds +def set_index_out_of_bounds_high(): + x = [0, 1, 2, 3, 4] + x[5] = 'a' + +def set_index_out_of_bounds_low(): + x = [0, 1, 2, 3, 4] + x[-6] = 'a' + +assert_raises(IndexError, set_index_out_of_bounds_high) +assert_raises(IndexError, set_index_out_of_bounds_low) + +# non stepped slice index +a = list(range(10)) +x = a[:] +y = a[:] +assert x == [0, 1, 2, 3, 4, 5, 6, 7, 8, 9] +# replace whole list +x[:] = ['a', 'b', 'c'] +y[::1] = ['a', 'b', 'c'] +assert x == ['a', 'b', 'c'] +assert x == y +# splice list start +x = a[:] +y = a[:] +z = a[:] +zz = a[:] +x[:1] = ['a', 'b', 'c'] +y[0:1] = ['a', 'b', 'c'] +z[:1:1] = ['a', 'b', 'c'] +zz[0:1:1] = ['a', 'b', 'c'] +assert x == ['a', 'b', 'c', 1, 2, 3, 4, 5, 6, 7, 8, 9] +assert x == y +assert x == z +assert x == zz +# splice list end +x = a[:] +y = a[:] +z = a[:] +zz = a[:] +x[5:] = ['a', 'b', 'c'] +y[5::1] = ['a', 'b', 'c'] +z[5:10] = ['a', 'b', 'c'] +zz[5:10:1] = ['a', 'b', 'c'] +assert x == [0, 1, 2, 3, 4, 'a', 'b', 'c'] +assert x == y +assert x == z +assert x == zz +# insert sec +x = a[:] +y = a[:] +z = a[:] +zz = a[:] +x[1:1] = ['a', 'b', 'c'] +y[1:0] = ['a', 'b', 'c'] +z[1:1:1] = ['a', 'b', 'c'] +zz[1:0:1] = ['a', 'b', 'c'] +assert x == [0, 'a', 'b', 'c', 1, 2, 3, 4, 5, 6, 7, 8, 9] +assert x == y +assert x == z +assert x == zz +# same but negative indexes? +x = a[:] +y = a[:] +z = a[:] +zz = a[:] +x[-1:-1] = ['a', 'b', 'c'] +y[-1:9] = ['a', 'b', 'c'] +z[-1:-1:1] = ['a', 'b', 'c'] +zz[-1:9:1] = ['a', 'b', 'c'] +assert x == [0, 1, 2, 3, 4, 5, 6, 7, 8, 'a', 'b', 'c', 9] +assert x == y +assert x == z +assert x == zz +# splice mid +x = a[:] +y = a[:] +x[3:5] = ['a', 'b', 'c', 'd', 'e'] +y[3:5:1] = ['a', 'b', 'c', 'd', 'e'] +assert x == [0, 1, 2, 'a', 'b', 'c', 'd', 'e', 5, 6, 7, 8, 9] +assert x == y +x = a[:] +x[3:5] = ['a'] +assert x == [0, 1, 2, 'a', 5, 6, 7, 8, 9] +# assign empty to non stepped empty slice does nothing +x = a[:] +y = a[:] +x[5:2] = [] +y[5:2:1] = [] +assert x == a +assert y == a +# assign empty to non stepped slice removes elems +x = a[:] +y = a[:] +x[2:8] = [] +y[2:8:1] = [] +assert x == [0, 1, 8, 9] +assert x == y +# make sure refrences are assigned correctly +yy = [] +x = a[:] +y = a[:] +x[3:5] = ['a', 'b', 'c', 'd', yy] +y[3:5:1] = ['a', 'b', 'c', 'd', yy] +assert x == [0, 1, 2, 'a', 'b', 'c', 'd', [], 5, 6, 7, 8, 9] +assert x == y +yy.append(100) +assert x == [0, 1, 2, 'a', 'b', 'c', 'd', [100], 5, 6, 7, 8, 9] +assert x == y +assert x[7] == yy +assert x[7] == [100] +assert y[7] == yy +assert y[7] == [100] + +# no zero step +def no_zero_step_set(): + x = [1, 2, 3, 4, 5] + x[0:4:0] = [11, 12, 13, 14, 15] +assert_raises(ValueError, no_zero_step_set) + +# stepped slice index +# forward slice +x = a[:] +x[2:8:2] = ['a', 'b', 'c'] +assert x == [0, 1, 'a', 3, 'b', 5, 'c', 7, 8, 9] +x = a[:] +y = a[:] +z = a[:] +zz = a[:] +c = ['a', 'b', 'c', 'd', 'e'] +x[::2] = c +y[-10::2] = c +z[0:10:2] = c +zz[-13:13:2] = c # slice indexes will be truncated to bounds +assert x == ['a', 1, 'b', 3, 'c', 5, 'd', 7, 'e', 9] +assert x == y +assert x == z +assert x == zz +# backward slice +x = a[:] +x[8:2:-2] = ['a', 'b', 'c'] +assert x == [0, 1, 2, 3, 'c', 5, 'b', 7, 'a', 9] +x = a[:] +y = a[:] +z = a[:] +zz = a[:] +c = ['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j'] +x[::-1] = c +y[9:-11:-1] = c +z[9::-1] = c +zz[11:-13:-1] = c # slice indexes will be truncated to bounds +assert x == ['j', 'i', 'h', 'g', 'f', 'e', 'd', 'c', 'b', 'a'] +assert x == y +assert x == z +assert x == zz +# step size bigger than len +x = a[:] +x[::200] = ['a'] +assert x == ['a', 1, 2, 3, 4, 5, 6, 7, 8, 9] +x = a[:] +x[5::200] = ['a'] +assert x == [0, 1, 2, 3, 4, 'a', 6, 7, 8, 9] + +# bad stepped slices +def stepped_slice_assign_too_big(): + x = [0, 1, 2, 3, 4] + x[::2] = ['a', 'b', 'c', 'd'] + +assert_raises(ValueError, stepped_slice_assign_too_big) + +def stepped_slice_assign_too_small(): + x = [0, 1, 2, 3, 4] + x[::2] = ['a', 'b'] + +assert_raises(ValueError, stepped_slice_assign_too_small) + +# must assign iter t0 slice +def must_assign_iter_to_slice(): + x = [0, 1, 2, 3, 4] + x[::2] = 42 + +assert_raises(TypeError, must_assign_iter_to_slice) From a23b5bc470f0196079529b11351169cc543a362f Mon Sep 17 00:00:00 2001 From: Rachel Powers Date: Sun, 7 Apr 2019 19:32:11 -0600 Subject: [PATCH 3/8] cleanup and cargo fmt --- vm/src/obj/objlist.rs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/vm/src/obj/objlist.rs b/vm/src/obj/objlist.rs index 3a4035409..1ef01ca3e 100644 --- a/vm/src/obj/objlist.rs +++ b/vm/src/obj/objlist.rs @@ -189,10 +189,7 @@ impl PyListRef { ) -> PyResult { match subscript { SequenceIndex::Int(index) => self.setindex(index, value, vm), - SequenceIndex::Slice(slice) => { - // TODO check special case of if a == b, may need copy of b first? - self.setslice(slice, value, vm) - } + SequenceIndex::Slice(slice) => self.setslice(slice, value, vm), } } @@ -229,7 +226,6 @@ impl PyListRef { } else { // this functions as an insert of sec before range.start self._set_slice(range.start..range.start, sec, vm) - // TODO this will take some special processing } } else { // calculate the range for the reverse slice, first the bounds needs to be made From 4e277cfcc2d6eac7aa15cc493dacaa46fa8971d8 Mon Sep 17 00:00:00 2001 From: Rachel Powers Date: Mon, 8 Apr 2019 12:41:28 -0600 Subject: [PATCH 4/8] move to useing PyIterable --- vm/src/obj/objlist.rs | 108 ++++++++++++++++++++++-------------------- 1 file changed, 57 insertions(+), 51 deletions(-) diff --git a/vm/src/obj/objlist.rs b/vm/src/obj/objlist.rs index 1ef01ca3e..4c3c16383 100644 --- a/vm/src/obj/objlist.rs +++ b/vm/src/obj/objlist.rs @@ -7,7 +7,10 @@ use num_bigint::BigInt; use num_traits::{One, Signed, ToPrimitive, Zero}; use crate::function::{OptionalArg, PyFuncArgs}; -use crate::pyobject::{IdProtocol, PyContext, PyObjectRef, PyRef, PyResult, PyValue, TypeProtocol}; +use crate::pyobject::{ + IdProtocol, PyContext, PyIterable, PyObjectRef, PyRef, PyResult, PyValue, TryFromObject, + TypeProtocol, +}; use crate::vm::{ReprGuard, VirtualMachine}; use super::objbool; @@ -189,7 +192,12 @@ impl PyListRef { ) -> PyResult { match subscript { SequenceIndex::Int(index) => self.setindex(index, value, vm), - SequenceIndex::Slice(slice) => self.setslice(slice, value, vm), + SequenceIndex::Slice(slice) => { + if let Ok(sec) = PyIterable::try_from_object(vm, value) { + return self.setslice(slice, sec, vm); + } + Err(vm.new_type_error("can only assign an iterable to a slice".to_string())) + } } } @@ -202,7 +210,7 @@ impl PyListRef { } } - fn setslice(self, slice: PySliceRef, sec: PyObjectRef, vm: &VirtualMachine) -> PyResult { + fn setslice(self, slice: PySliceRef, sec: PyIterable, vm: &VirtualMachine) -> PyResult { let step = slice.step.clone().unwrap_or_else(BigInt::one); if step.is_zero() { @@ -244,52 +252,50 @@ impl PyListRef { } } - fn _set_slice(self, range: Range, sec: PyObjectRef, vm: &VirtualMachine) -> PyResult { - // consume the iter, we don't need it's size - // but if it's going to fail we want that to happen *before* we start modifing - if let Ok(items) = vm.extract_elements(&sec) { - // replace the range of elements with the full sequence - self.elements.borrow_mut().splice(range, items); + fn _set_slice(self, range: Range, sec: PyIterable, vm: &VirtualMachine) -> PyResult { + // consume the iter, we need it's size + // and if it's going to fail we want that to happen *before* we start modifing + let items: Result, _> = sec.iter(vm)?.collect(); + let items = items?; - Ok(vm.get_none()) - } else { - Err(vm.new_type_error("can only assign an iterable to a slice".to_string())) - } + // replace the range of elements with the full sequence + self.elements.borrow_mut().splice(range, items); + + Ok(vm.get_none()) } fn _set_stepped_slice( self, range: Range, step: usize, - sec: PyObjectRef, + sec: PyIterable, vm: &VirtualMachine, ) -> PyResult { + let slicelen = ((range.end - range.start - 1) / step) + 1; // consume the iter, we need it's size // and if it's going to fail we want that to happen *before* we start modifing - let slicelen = ((range.end - range.start - 1) / step) + 1; - if let Ok(items) = vm.extract_elements(&sec) { - let n = items.len(); + let items: Result, _> = sec.iter(vm)?.collect(); + let items = items?; - if range.start < range.end { - if n == slicelen { - let indexes = range.step_by(step); - self._replace_indexes(indexes, &items); - Ok(vm.get_none()) - } else { - Err(vm.new_value_error(format!( - "attempt to assign sequence of size {} to extended slice of size {}", - n, slicelen - ))) - } + let n = items.len(); + + if range.start < range.end { + if n == slicelen { + let indexes = range.step_by(step); + self._replace_indexes(indexes, &items); + Ok(vm.get_none()) } else { - // empty slice but this is an error because stepped slice Err(vm.new_value_error(format!( - "attempt to assign sequence of size {} to extended slice of size 0", - n + "attempt to assign sequence of size {} to extended slice of size {}", + n, slicelen ))) } } else { - Err(vm.new_type_error("can only assign an iterable to a slice".to_string())) + // empty slice but this is an error because stepped slice + Err(vm.new_value_error(format!( + "attempt to assign sequence of size {} to extended slice of size 0", + n + ))) } } @@ -297,35 +303,35 @@ impl PyListRef { self, range: Range, step: usize, - sec: PyObjectRef, + sec: PyIterable, vm: &VirtualMachine, ) -> PyResult { + let slicelen = ((range.end - range.start - 1) / step) + 1; + // consume the iter, we need it's size // and if it's going to fail we want that to happen *before* we start modifing - let slicelen = ((range.end - range.start - 1) / step) + 1; - if let Ok(items) = vm.extract_elements(&sec) { - let n = items.len(); + let items: Result, _> = sec.iter(vm)?.collect(); + let items = items?; - if range.start < range.end { - if n == slicelen { - let indexes = range.rev().step_by(step); - self._replace_indexes(indexes, &items); - Ok(vm.get_none()) - } else { - Err(vm.new_value_error(format!( - "attempt to assign sequence of size {} to extended slice of size {}", - n, slicelen - ))) - } + let n = items.len(); + + if range.start < range.end { + if n == slicelen { + let indexes = range.rev().step_by(step); + self._replace_indexes(indexes, &items); + Ok(vm.get_none()) } else { - // empty slice but this is an error because stepped slice Err(vm.new_value_error(format!( - "attempt to assign sequence of size {} to extended slice of size 0", - n + "attempt to assign sequence of size {} to extended slice of size {}", + n, slicelen ))) } } else { - Err(vm.new_type_error("can only assign an iterable to a slice".to_string())) + // empty slice but this is an error because stepped slice + Err(vm.new_value_error(format!( + "attempt to assign sequence of size {} to extended slice of size 0", + n + ))) } } From d0e6d2fa323992ce1bce5ad998a55c1d04c7883c Mon Sep 17 00:00:00 2001 From: Rachel Powers Date: Mon, 8 Apr 2019 13:18:37 -0600 Subject: [PATCH 5/8] add tests for custom iters --- tests/snippets/list.py | 52 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/tests/snippets/list.py b/tests/snippets/list.py index 71f7dea06..55c84fdf9 100644 --- a/tests/snippets/list.py +++ b/tests/snippets/list.py @@ -399,3 +399,55 @@ def must_assign_iter_to_slice(): x[::2] = 42 assert_raises(TypeError, must_assign_iter_to_slice) + +# other iterables? +a = list(range(10)) + +# string +x = a[:] +x[3:8] = "abcdefghi" +assert x == [0, 1, 2, 'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 8, 9] + +# tuple +x = a[:] +x[3:8] = (11, 12, 13, 14, 15) +assert x == [0, 1, 2, 11, 12, 13, 14, 15, 8, 9] + +# class +class CIterNext: + def __init__(self, sec=(1, 2, 3)): + self.sec = sec + self.index = 0 + def __iter__(self): + return self + def __next__(self): + if self.index >= len(self.sec): + raise StopIteration + v = self.sec[self.index] + self.index += 1 + return v + +x = list(range(10)) +x[3:8] = CIterNext() +assert x == [0, 1, 2, 1, 2, 3, 8, 9] + +class CIter: + def __init__(self, sec=(1, 2, 3)): + self.sec = sec + def __iter__(self): + for n in self.sec: + yield n + +x = list(range(10)) +x[3:8] = CIter() +assert x == [0, 1, 2, 1, 2, 3, 8, 9] + +class CGetItem: + def __init__(self, sec=(1, 2, 3)): + self.sec = sec + def __getitem__(self, sub): + return self.sec[sub] + +x = list(range(10)) +x[3:8] = CGetItem() +assert x == [0, 1, 2, 1, 2, 3, 8, 9] From 4c50defa37d5a38e75247d6ac32fd3c680f9e7fc Mon Sep 17 00:00:00 2001 From: Rachel Powers Date: Mon, 8 Apr 2019 13:46:23 -0600 Subject: [PATCH 6/8] extend PyIterable::try_from_object to work with sequences --- vm/src/pyobject.rs | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/vm/src/pyobject.rs b/vm/src/pyobject.rs index ee765f987..be51128ef 100644 --- a/vm/src/pyobject.rs +++ b/vm/src/pyobject.rs @@ -1,4 +1,5 @@ use std::any::Any; +use std::cell::Cell; use std::cell::RefCell; use std::collections::HashMap; use std::fmt; @@ -1043,10 +1044,24 @@ where T: TryFromObject, { fn try_from_object(vm: &VirtualMachine, obj: PyObjectRef) -> PyResult { - Ok(PyIterable { - method: vm.get_method(obj, "__iter__")?, - _item: std::marker::PhantomData, - }) + if let Ok(method) = vm.get_method(obj.clone(), "__iter__") { + Ok(PyIterable { + method: method, + _item: std::marker::PhantomData, + }) + } else if vm.get_method(obj.clone(), "__getitem__").is_ok() { + Self::try_from_object( + vm, + objiter::PySequenceIterator { + position: Cell::new(0), + obj: obj.clone(), + } + .into_ref(vm) + .into_object(), + ) + } else { + Err(vm.new_type_error(format!("'{}' object is not iterable", obj.class().name))) + } } } From 4d53ddedb020c100eeb6e45675aebd572909f9dd Mon Sep 17 00:00:00 2001 From: Rachel Powers Date: Mon, 8 Apr 2019 14:45:34 -0600 Subject: [PATCH 7/8] test for if iter raises error --- tests/snippets/list.py | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/tests/snippets/list.py b/tests/snippets/list.py index 55c84fdf9..217e79089 100644 --- a/tests/snippets/list.py +++ b/tests/snippets/list.py @@ -414,6 +414,7 @@ x[3:8] = (11, 12, 13, 14, 15) assert x == [0, 1, 2, 11, 12, 13, 14, 15, 8, 9] # class +# __next__ class CIterNext: def __init__(self, sec=(1, 2, 3)): self.sec = sec @@ -431,6 +432,7 @@ x = list(range(10)) x[3:8] = CIterNext() assert x == [0, 1, 2, 1, 2, 3, 8, 9] +# __iter__ yield class CIter: def __init__(self, sec=(1, 2, 3)): self.sec = sec @@ -442,6 +444,7 @@ x = list(range(10)) x[3:8] = CIter() assert x == [0, 1, 2, 1, 2, 3, 8, 9] +# __getitem but no __iter__ sequence class CGetItem: def __init__(self, sec=(1, 2, 3)): self.sec = sec @@ -451,3 +454,17 @@ class CGetItem: x = list(range(10)) x[3:8] = CGetItem() assert x == [0, 1, 2, 1, 2, 3, 8, 9] + +# iter raises error +class CIterError: + def __iter__(self): + for i in range(10): + if i > 5: + raise RuntimeError + yield i + +def bad_iter_assign(): + x = list(range(10)) + x[3:8] = CIterError() + +assert_raises(RuntimeError, bad_iter_assign) From b2a1f6580b6dcf3eec299ec1423513c682aa18eb Mon Sep 17 00:00:00 2001 From: Rachel Powers Date: Mon, 8 Apr 2019 15:56:06 -0600 Subject: [PATCH 8/8] fix error when start or stop of slice is -1 and step != 1 --- tests/snippets/list.py | 10 ++++++++++ vm/src/obj/objlist.rs | 36 +++++++++++++++++++++++++++++++----- 2 files changed, 41 insertions(+), 5 deletions(-) diff --git a/tests/snippets/list.py b/tests/snippets/list.py index 217e79089..dc0582d4c 100644 --- a/tests/snippets/list.py +++ b/tests/snippets/list.py @@ -468,3 +468,13 @@ def bad_iter_assign(): x[3:8] = CIterError() assert_raises(RuntimeError, bad_iter_assign) + +# slice assign when step or stop is -1 + +a = list(range(10)) +x = a[:] +x[-1:-5:-1] = ['a', 'b', 'c', 'd'] +assert x == [0, 1, 2, 3, 4, 5, 'd', 'c', 'b', 'a'] +x = a[:] +x[-5:-1:-1] = [] +assert x == [0, 1, 2, 3, 4, 5, 6, 7, 8, 9] diff --git a/vm/src/obj/objlist.rs b/vm/src/obj/objlist.rs index 4c3c16383..6de08dfef 100644 --- a/vm/src/obj/objlist.rs +++ b/vm/src/obj/objlist.rs @@ -3,7 +3,7 @@ use std::fmt; use std::ops::Range; -use num_bigint::BigInt; +use num_bigint::{BigInt, ToBigInt}; use num_traits::{One, Signed, ToPrimitive, Zero}; use crate::function::{OptionalArg, PyFuncArgs}; @@ -238,8 +238,20 @@ impl PyListRef { } else { // calculate the range for the reverse slice, first the bounds needs to be made // exclusive around stop, the lower number - let start = &slice.start.as_ref().map(|x| x + 1); - let stop = &slice.stop.as_ref().map(|x| x + 1); + let start = &slice.start.as_ref().map(|x| { + if *x == (-1).to_bigint().unwrap() { + self.get_len() + BigInt::one() //.to_bigint().unwrap() + } else { + x + 1 + } + }); + let stop = &slice.stop.as_ref().map(|x| { + if *x == (-1).to_bigint().unwrap() { + self.get_len().to_bigint().unwrap() + } else { + x + 1 + } + }); let range = self.get_slice_range(&stop, &start); match (-step).to_i32() { Some(num) => self._set_stepped_slice_reverse(range, num as usize, sec, vm), @@ -271,7 +283,11 @@ impl PyListRef { sec: PyIterable, vm: &VirtualMachine, ) -> PyResult { - let slicelen = ((range.end - range.start - 1) / step) + 1; + let slicelen = if range.end > range.start { + ((range.end - range.start - 1) / step) + 1 + } else { + 0 + }; // consume the iter, we need it's size // and if it's going to fail we want that to happen *before* we start modifing let items: Result, _> = sec.iter(vm)?.collect(); @@ -290,6 +306,9 @@ impl PyListRef { n, slicelen ))) } + } else if n == 0 { + // slice is empty but so is sequence + Ok(vm.get_none()) } else { // empty slice but this is an error because stepped slice Err(vm.new_value_error(format!( @@ -306,7 +325,11 @@ impl PyListRef { sec: PyIterable, vm: &VirtualMachine, ) -> PyResult { - let slicelen = ((range.end - range.start - 1) / step) + 1; + let slicelen = if range.end > range.start { + ((range.end - range.start - 1) / step) + 1 + } else { + 0 + }; // consume the iter, we need it's size // and if it's going to fail we want that to happen *before* we start modifing @@ -326,6 +349,9 @@ impl PyListRef { n, slicelen ))) } + } else if n == 0 { + // slice is empty but so is sequence + Ok(vm.get_none()) } else { // empty slice but this is an error because stepped slice Err(vm.new_value_error(format!(