implement list.__setitem__ with slice and int indexing #805
Conversation
optimisation is uncertain correctness is uncertain but it does appear to work
I attempted to cover every case with the snippets but I easily could of missed some.
vm/src/obj/objlist.rs
Outdated
| fn _set_slice(self, range: Range<usize>, 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) { |
There was a problem hiding this comment.
I don't think we should catch random exceptions here.
CPython propagates exceptions thrown by the iterator:
>>> x[2:4] = F()
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "<stdin>", line 3, in __iter__
NameError: name 'noname' is not defined
I think you can just do:
let items = vm.extract_elements(&sec)?;
It looks like extract_elements will raise a reasonable exception, though if we want the perfect exception maybe something with PyIterable's try_from_object might work?
vm/src/obj/objlist.rs
Outdated
| // 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) { |
There was a problem hiding this comment.
As above, catching too many exceptions.
vm/src/obj/objlist.rs
Outdated
| // 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) { |
There was a problem hiding this comment.
Can these extracts be moved outwards to set_slice? (I wonder if set_slice could take a PyIterable).
There was a problem hiding this comment.
I could move the check up to setslice and use PyIterable but I will have to patch PyIterable::try_from_object to correctly transform sequences. right now it doesn't work on strings (because no __iter__ has been implemented) and classes with a __getitem__ but no __iter__ as it should.
I actually already have a patch that does that but I'm not sure it would be a good idea to push it on this branch. thoughts?
There was a problem hiding this comment.
for reference
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]will work in python but PyIterable needs to looks somthing like
impl<T> TryFromObject for PyIterable<T>
where
T: TryFromObject,
{
fn try_from_object(vm: &VirtualMachine, obj: PyObjectRef) -> PyResult<Self> {
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)))
}
}
}There was a problem hiding this comment.
Weird. I did not know that was possible. Especially as list and tuple do implement __iter__. This exists to optimise strings I guess?
Either here or separate PR is fine. Should I merge this and you can continue this elsewhere?
There was a problem hiding this comment.
I actually just found a bug related to the same error as #746 so I'm going to push some more commits here shortly that will include that fix and the try_from_object patch
There was a problem hiding this comment.
and done, should be good now.
|
This looks great, thanks!
Rust takes care of this for you, so there's not much you can do wrong here unless you start using unsafe. One easy thing to get wrong is multiple borrows, but I don't think there's any problems here. (I wondered about splicing a list into itself, but it's fine because you extract the elements first.) |
after working on #783 it felt wrong to leave
list.__setitem__broken in that it could not do slice assignment.ref #251
I tried to work off the CPython list code for this and I think I assessed all the special cases. It was a bit hard to follow as it's all done with
memmovein C.I've added a large number of test snippets in hopes of covering all the use cases of list assignment and checking for correct behavior, but it would probably be a good idea for someone to double check my work on that.
I've tried to use fairly optimal modifications of the underlying
Vecby relegating most of the work toVec.spliceexcept for stepped slices. I trust Rust's internals to be smarter than me.stepped slices use a zipped iter of indexes to modify and the values to set. Pretty sure there is not a better way but please feel free to point it out it there is.
hopefuly refrences are counted corrrectly too, that would be a good thing to double check.