From 5625f7e15e49772cb906ba23335356d341a470a4 Mon Sep 17 00:00:00 2001 From: ben Date: Sun, 31 Mar 2019 09:29:21 +1300 Subject: [PATCH 1/5] Allow arbitrary in slice, and convert slice.__new__ to new style --- tests/snippets/builtin_slice.py | 6 ++ vm/src/frame.rs | 27 ++----- vm/src/obj/objrange.rs | 6 +- vm/src/obj/objsequence.rs | 11 +-- vm/src/obj/objslice.rs | 127 +++++++++++++++++++------------- 5 files changed, 99 insertions(+), 78 deletions(-) diff --git a/tests/snippets/builtin_slice.py b/tests/snippets/builtin_slice.py index 1d8a93b479..03f800fba3 100644 --- a/tests/snippets/builtin_slice.py +++ b/tests/snippets/builtin_slice.py @@ -59,6 +59,12 @@ assert slice_c.stop == 5 assert slice_c.step == 2 +a = object() +slice_d = slice(a, "v", 1.0) +assert slice_d.start is a +assert slice_d.stop == "v" +assert slice_d.step == 1.0 + class SubScript(object): def __getitem__(self, item): diff --git a/vm/src/frame.rs b/vm/src/frame.rs index 88a619eb36..3a5f4c5abe 100644 --- a/vm/src/frame.rs +++ b/vm/src/frame.rs @@ -2,8 +2,6 @@ use std::cell::RefCell; use std::fmt; use std::rc::Rc; -use num_bigint::BigInt; - use rustpython_parser::ast; use crate::builtins; @@ -13,7 +11,6 @@ use crate::obj::objbool; use crate::obj::objbuiltinfunc::PyBuiltinFunction; use crate::obj::objcode::PyCodeRef; use crate::obj::objdict::{self, PyDictRef}; -use crate::obj::objint::PyInt; use crate::obj::objiter; use crate::obj::objlist; use crate::obj::objslice::PySlice; @@ -404,24 +401,14 @@ impl Frame { } bytecode::Instruction::BuildSlice { size } => { assert!(*size == 2 || *size == 3); - let elements = self.pop_multiple(*size); - let mut out: Vec> = elements - .into_iter() - .map(|x| { - if x.is(&vm.ctx.none()) { - None - } else if let Some(i) = x.payload::() { - Some(i.as_bigint().clone()) - } else { - panic!("Expect Int or None as BUILD_SLICE arguments") - } - }) - .collect(); - - let start = out[0].take(); - let stop = out[1].take(); - let step = if out.len() == 3 { out[2].take() } else { None }; + let step = if *size == 3 { + Some(self.pop_value()) + } else { + None + }; + let stop = Some(self.pop_value()); + let start = Some(self.pop_value()); let obj = PySlice { start, stop, step }.into_ref(vm); self.push_value(obj.into_object()); diff --git a/vm/src/obj/objrange.rs b/vm/src/obj/objrange.rs index 04c9c273b1..c1a0b94012 100644 --- a/vm/src/obj/objrange.rs +++ b/vm/src/obj/objrange.rs @@ -264,7 +264,7 @@ impl PyRangeRef { } } Either::B(slice) => { - let new_start = if let Some(int) = slice.start.as_ref() { + let new_start = if let Some(int) = slice.start_index(vm)? { if let Some(i) = self.get(int) { PyInt::new(i).into_ref(vm) } else { @@ -274,7 +274,7 @@ impl PyRangeRef { self.start.clone() }; - let new_end = if let Some(int) = slice.stop.as_ref() { + let new_end = if let Some(int) = slice.stop_index(vm)? { if let Some(i) = self.get(int) { PyInt::new(i).into_ref(vm) } else { @@ -284,7 +284,7 @@ impl PyRangeRef { self.stop.clone() }; - let new_step = if let Some(int) = slice.step.as_ref() { + let new_step = if let Some(int) = slice.step_index(vm)? { PyInt::new(int * self.step.as_bigint()).into_ref(vm) } else { self.step.clone() diff --git a/vm/src/obj/objsequence.rs b/vm/src/obj/objsequence.rs index a07b2454de..a293314bee 100644 --- a/vm/src/obj/objsequence.rs +++ b/vm/src/obj/objsequence.rs @@ -65,14 +65,15 @@ pub trait PySliceableSequence { where Self: Sized, { - // TODO: we could potentially avoid this copy and use slice - match slice.payload() { - Some(PySlice { start, stop, step }) => { - let step = step.clone().unwrap_or_else(BigInt::one); + match slice.clone().downcast::() { + Ok(slice) => { + let start = slice.start_index(vm)?; + let stop = slice.stop_index(vm)?; + let step = slice.step_index(vm)?.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(start, stop); + let range = self.get_slice_range(&start, &stop); if range.start < range.end { #[allow(clippy::range_plus_one)] match step.to_i32() { diff --git a/vm/src/obj/objslice.rs b/vm/src/obj/objslice.rs index c82a3ca6ef..3eee8e4ec4 100644 --- a/vm/src/obj/objslice.rs +++ b/vm/src/obj/objslice.rs @@ -1,18 +1,19 @@ -use num_bigint::BigInt; - use crate::function::PyFuncArgs; -use crate::pyobject::{PyContext, PyObjectRef, PyRef, PyResult, PyValue, TypeProtocol}; +use crate::pyobject::{IdProtocol, PyContext, PyObjectRef, PyRef, PyResult, PyValue, TypeProtocol}; use crate::vm::VirtualMachine; -use super::objint; -use crate::obj::objtype::PyClassRef; +use crate::obj::objint::PyInt; +use crate::obj::objtype::{class_has_attr, PyClassRef}; +use num_bigint::BigInt; -#[derive(Debug)] +#[derive(Debug, FromArgs)] pub struct PySlice { - // TODO: should be private - pub start: Option, - pub stop: Option, - pub step: Option, + #[pyarg(positional_only)] + pub start: Option, + #[pyarg(positional_only)] + pub stop: Option, + #[pyarg(positional_only, default)] + pub step: Option, } impl PyValue for PySlice { @@ -24,51 +25,29 @@ impl PyValue for PySlice { pub type PySliceRef = PyRef; fn slice_new(vm: &VirtualMachine, args: PyFuncArgs) -> PyResult { - no_kwargs!(vm, args); - let (cls, start, stop, step): ( - &PyObjectRef, - Option<&PyObjectRef>, - Option<&PyObjectRef>, - Option<&PyObjectRef>, - ) = match args.args.len() { - 0 | 1 => Err(vm.new_type_error("slice() must have at least one arguments.".to_owned())), - 2 => { - arg_check!( - vm, - args, - required = [ - (cls, Some(vm.ctx.type_type())), - (stop, Some(vm.ctx.int_type())) - ] - ); - Ok((cls, None, Some(stop), None)) + let (cls, slice): (PyClassRef, PySlice) = match args.args.len() { + 0 | 1 => { + return Err(vm.new_type_error("slice() must have at least one arguments.".to_owned())); } - _ => { - arg_check!( - vm, - args, - required = [ - (cls, Some(vm.ctx.type_type())), - (start, Some(vm.ctx.int_type())), - (stop, Some(vm.ctx.int_type())) - ], - optional = [(step, Some(vm.ctx.int_type()))] - ); - Ok((cls, Some(start), Some(stop), step)) + 2 => { + let (cls, stop) = args.bind(vm)?; + ( + cls, + PySlice { + start: None, + stop: Some(stop), + step: None, + }, + ) } - }?; - PySlice { - start: start.map(|x| objint::get_value(x).clone()), - stop: stop.map(|x| objint::get_value(x).clone()), - step: step.map(|x| objint::get_value(x).clone()), - } - .into_ref_with_type(vm, cls.clone().downcast().unwrap()) - .map(|x| x.into_object()) + _ => args.bind(vm)?, + }; + slice.into_ref_with_type(vm, cls).map(|x| x.into_object()) } -fn get_property_value(vm: &VirtualMachine, value: &Option) -> PyObjectRef { +fn get_property_value(vm: &VirtualMachine, value: &Option) -> PyObjectRef { if let Some(value) = value { - vm.ctx.new_int(value.clone()) + value.clone() } else { vm.get_none() } @@ -86,6 +65,54 @@ impl PySliceRef { fn step(self, vm: &VirtualMachine) -> PyObjectRef { get_property_value(vm, &self.step) } + + pub fn start_index(&self, vm: &VirtualMachine) -> PyResult> { + if let Some(obj) = &self.start { + to_index_value(vm, obj) + } else { + Ok(None) + } + } + + pub fn stop_index(&self, vm: &VirtualMachine) -> PyResult> { + if let Some(obj) = &self.stop { + to_index_value(vm, obj) + } else { + Ok(None) + } + } + + pub fn step_index(&self, vm: &VirtualMachine) -> PyResult> { + if let Some(obj) = &self.step { + to_index_value(vm, obj) + } else { + Ok(None) + } + } +} + +fn to_index_value(vm: &VirtualMachine, obj: &PyObjectRef) -> PyResult> { + if obj.is(&vm.ctx.none) { + return Ok(None); + } + + if let Some(val) = obj.payload::() { + Ok(Some(val.as_bigint().clone())) + } else { + let cls = obj.class(); + if class_has_attr(&cls, "__index__") { + let index_result = vm.call_method(obj, "__index__", vec![])?; + if let Some(val) = index_result.payload::() { + Ok(Some(val.as_bigint().clone())) + } else { + Err(vm.new_type_error("__index__ method returned non integer".to_string())) + } + } else { + Err(vm.new_type_error( + "slice indices must be integers or None or have an __index__ method".to_string(), + )) + } + } } pub fn init(context: &PyContext) { From a49895ef63a8a7d7f83ddbfae6e44b6abca9c450 Mon Sep 17 00:00:00 2001 From: ben Date: Sun, 31 Mar 2019 09:55:18 +1300 Subject: [PATCH 2/5] Cleanup slice_new and added more slice tests --- tests/snippets/builtin_slice.py | 13 +++++++++++++ vm/src/function.rs | 7 +++++++ vm/src/obj/objslice.rs | 25 +++++++++++-------------- 3 files changed, 31 insertions(+), 14 deletions(-) diff --git a/tests/snippets/builtin_slice.py b/tests/snippets/builtin_slice.py index 03f800fba3..c5dbd5d908 100644 --- a/tests/snippets/builtin_slice.py +++ b/tests/snippets/builtin_slice.py @@ -11,6 +11,7 @@ b = [1, 2] assert b[:] == [1, 2] +assert b[slice(None)] == [1, 2] assert b[:2**100] == [1, 2] assert b[-2**100:] == [1, 2] assert b[2**100:] == [] @@ -77,3 +78,15 @@ def __setitem__(self, key, value): ss = SubScript() _ = ss[:] ss[:1] = 1 + + +class CustomIndex: + def __init__(self, x): + self.x = x + + def __index__(self): + return self.x + + +assert c[CustomIndex(1):CustomIndex(3)] == [1, 2] +assert d[CustomIndex(1):CustomIndex(3)] == "23" diff --git a/vm/src/function.rs b/vm/src/function.rs index aae4eefe89..9215345499 100644 --- a/vm/src/function.rs +++ b/vm/src/function.rs @@ -1,4 +1,5 @@ use std::collections::HashMap; +use std::mem; use std::ops::RangeInclusive; use crate::obj::objtype::{isinstance, PyClassRef}; @@ -46,6 +47,12 @@ impl From<(&Args, &KwArgs)> for PyFuncArgs { } } +impl FromArgs for PyFuncArgs { + fn from_args(_vm: &VirtualMachine, args: &mut PyFuncArgs) -> Result { + Ok(mem::replace(args, Default::default())) + } +} + impl PyFuncArgs { pub fn new(mut args: Vec, kwarg_names: Vec) -> PyFuncArgs { let mut kwargs = vec![]; diff --git a/vm/src/obj/objslice.rs b/vm/src/obj/objslice.rs index 3eee8e4ec4..352ba5ce54 100644 --- a/vm/src/obj/objslice.rs +++ b/vm/src/obj/objslice.rs @@ -24,25 +24,22 @@ impl PyValue for PySlice { pub type PySliceRef = PyRef; -fn slice_new(vm: &VirtualMachine, args: PyFuncArgs) -> PyResult { - let (cls, slice): (PyClassRef, PySlice) = match args.args.len() { - 0 | 1 => { +fn slice_new(cls: PyClassRef, args: PyFuncArgs, vm: &VirtualMachine) -> PyResult { + let slice: PySlice = match args.args.len() { + 0 => { return Err(vm.new_type_error("slice() must have at least one arguments.".to_owned())); } - 2 => { - let (cls, stop) = args.bind(vm)?; - ( - cls, - PySlice { - start: None, - stop: Some(stop), - step: None, - }, - ) + 1 => { + let stop = args.bind(vm)?; + PySlice { + start: None, + stop: Some(stop), + step: None, + } } _ => args.bind(vm)?, }; - slice.into_ref_with_type(vm, cls).map(|x| x.into_object()) + slice.into_ref_with_type(vm, cls) } fn get_property_value(vm: &VirtualMachine, value: &Option) -> PyObjectRef { From e0f7fbb191490a7ee27b34bb411d2f62f5f96fe1 Mon Sep 17 00:00:00 2001 From: ben Date: Sun, 31 Mar 2019 10:53:45 +1300 Subject: [PATCH 3/5] Don't derive FromArgs on payload type. --- vm/src/obj/objslice.rs | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/vm/src/obj/objslice.rs b/vm/src/obj/objslice.rs index 352ba5ce54..f6f02c8db2 100644 --- a/vm/src/obj/objslice.rs +++ b/vm/src/obj/objslice.rs @@ -1,4 +1,4 @@ -use crate::function::PyFuncArgs; +use crate::function::{OptionalArg, PyFuncArgs}; use crate::pyobject::{IdProtocol, PyContext, PyObjectRef, PyRef, PyResult, PyValue, TypeProtocol}; use crate::vm::VirtualMachine; @@ -6,13 +6,10 @@ use crate::obj::objint::PyInt; use crate::obj::objtype::{class_has_attr, PyClassRef}; use num_bigint::BigInt; -#[derive(Debug, FromArgs)] +#[derive(Debug)] pub struct PySlice { - #[pyarg(positional_only)] pub start: Option, - #[pyarg(positional_only)] pub stop: Option, - #[pyarg(positional_only, default)] pub step: Option, } @@ -37,7 +34,15 @@ fn slice_new(cls: PyClassRef, args: PyFuncArgs, vm: &VirtualMachine) -> PyResult step: None, } } - _ => args.bind(vm)?, + _ => { + let (start, stop, step): (PyObjectRef, PyObjectRef, OptionalArg) = + args.bind(vm)?; + PySlice { + start: Some(start), + stop: Some(stop), + step: step.into_option(), + } + } }; slice.into_ref_with_type(vm, cls) } From ea2622ee7b820dc4fb787aa47817b293fcb69c57 Mon Sep 17 00:00:00 2001 From: ben Date: Mon, 1 Apr 2019 19:38:20 +1300 Subject: [PATCH 4/5] Make slice.stop not an option --- vm/src/frame.rs | 11 ++++++++--- vm/src/obj/objslice.rs | 16 ++++++---------- 2 files changed, 14 insertions(+), 13 deletions(-) diff --git a/vm/src/frame.rs b/vm/src/frame.rs index 3a5f4c5abe..2cd080824e 100644 --- a/vm/src/frame.rs +++ b/vm/src/frame.rs @@ -407,10 +407,15 @@ impl Frame { } else { None }; - let stop = Some(self.pop_value()); - let start = Some(self.pop_value()); + let stop = self.pop_value(); + let start = self.pop_value(); - let obj = PySlice { start, stop, step }.into_ref(vm); + let obj = PySlice { + start: Some(start), + stop, + step, + } + .into_ref(vm); self.push_value(obj.into_object()); Ok(None) } diff --git a/vm/src/obj/objslice.rs b/vm/src/obj/objslice.rs index f6f02c8db2..82e4cef8cd 100644 --- a/vm/src/obj/objslice.rs +++ b/vm/src/obj/objslice.rs @@ -9,7 +9,7 @@ use num_bigint::BigInt; #[derive(Debug)] pub struct PySlice { pub start: Option, - pub stop: Option, + pub stop: PyObjectRef, pub step: Option, } @@ -30,7 +30,7 @@ fn slice_new(cls: PyClassRef, args: PyFuncArgs, vm: &VirtualMachine) -> PyResult let stop = args.bind(vm)?; PySlice { start: None, - stop: Some(stop), + stop, step: None, } } @@ -39,7 +39,7 @@ fn slice_new(cls: PyClassRef, args: PyFuncArgs, vm: &VirtualMachine) -> PyResult args.bind(vm)?; PySlice { start: Some(start), - stop: Some(stop), + stop, step: step.into_option(), } } @@ -60,8 +60,8 @@ impl PySliceRef { get_property_value(vm, &self.start) } - fn stop(self, vm: &VirtualMachine) -> PyObjectRef { - get_property_value(vm, &self.stop) + fn stop(self, _vm: &VirtualMachine) -> PyObjectRef { + self.stop.clone() } fn step(self, vm: &VirtualMachine) -> PyObjectRef { @@ -77,11 +77,7 @@ impl PySliceRef { } pub fn stop_index(&self, vm: &VirtualMachine) -> PyResult> { - if let Some(obj) = &self.stop { - to_index_value(vm, obj) - } else { - Ok(None) - } + to_index_value(vm, &self.stop) } pub fn step_index(&self, vm: &VirtualMachine) -> PyResult> { From 9b71424d4efe2b5de49a9f087fac7efb2a70ddce Mon Sep 17 00:00:00 2001 From: ben Date: Wed, 17 Apr 2019 20:02:08 +1200 Subject: [PATCH 5/5] Use slice.xxx_index() methods in setslice and delslice --- vm/src/obj/objlist.rs | 14 +++++++------- vm/src/obj/objslice.rs | 2 +- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/vm/src/obj/objlist.rs b/vm/src/obj/objlist.rs index a21bd9190c..bfc763d8cd 100644 --- a/vm/src/obj/objlist.rs +++ b/vm/src/obj/objlist.rs @@ -210,12 +210,12 @@ impl PyListRef { } fn setslice(self, slice: PySliceRef, sec: PyIterable, vm: &VirtualMachine) -> PyResult { - let step = slice.step.clone().unwrap_or_else(BigInt::one); + let step = slice.step_index(vm)?.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); + let range = self.get_slice_range(&slice.start_index(vm)?, &slice.stop_index(vm)?); if range.start < range.end { match step.to_i32() { Some(1) => self._set_slice(range, sec, vm), @@ -237,14 +237,14 @@ 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| { + let start = &slice.start_index(vm)?.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| { + let stop = &slice.stop_index(vm)?.as_ref().map(|x| { if *x == (-1).to_bigint().unwrap() { self.get_len().to_bigint().unwrap() } else { @@ -552,9 +552,9 @@ impl PyListRef { } fn delslice(self, slice: PySliceRef, vm: &VirtualMachine) -> PyResult { - let start = &slice.start; - let stop = &slice.stop; - let step = slice.step.clone().unwrap_or_else(BigInt::one); + let start = slice.start_index(vm)?; + let stop = slice.stop_index(vm)?; + let step = slice.step_index(vm)?.unwrap_or_else(BigInt::one); if step.is_zero() { Err(vm.new_value_error("slice step cannot be zero".to_string())) diff --git a/vm/src/obj/objslice.rs b/vm/src/obj/objslice.rs index cebfee3303..82e4cef8cd 100644 --- a/vm/src/obj/objslice.rs +++ b/vm/src/obj/objslice.rs @@ -1,5 +1,5 @@ use crate::function::{OptionalArg, PyFuncArgs}; -use crate::pyobject::{IdProtocol, PyContext, PyObjectRef, PyRef, PyResult, PyValue}; +use crate::pyobject::{IdProtocol, PyContext, PyObjectRef, PyRef, PyResult, PyValue, TypeProtocol}; use crate::vm::VirtualMachine; use crate::obj::objint::PyInt;