From 5fd3cf2bcda2858acb5e2ca01163cf7a7a66dee1 Mon Sep 17 00:00:00 2001 From: ben Date: Sat, 6 Apr 2019 20:29:25 +1300 Subject: [PATCH] Implemented keyword only defaults --- tests/snippets/function_args.py | 36 +++++++++++---- vm/src/bytecode.rs | 1 + vm/src/compile.rs | 28 ++++++++++-- vm/src/frame.rs | 26 +++++++++-- vm/src/obj/objfunction.rs | 13 +++++- vm/src/pyobject.rs | 7 +-- vm/src/vm.rs | 79 +++++++++++++++------------------ 7 files changed, 127 insertions(+), 63 deletions(-) diff --git a/tests/snippets/function_args.py b/tests/snippets/function_args.py index 884cb47df..fd164d113 100644 --- a/tests/snippets/function_args.py +++ b/tests/snippets/function_args.py @@ -1,21 +1,28 @@ +from testutils import assertRaises + + def sum(x, y): return x+y -# def total(a, b, c, d): -# return sum(sum(a,b), sum(c,d)) -# -# assert total(1,1,1,1) == 4 -# assert total(1,2,3,4) == 10 +def total(a, b, c, d): + return sum(sum(a,b), sum(c,d)) + + +assert total(1,1,1,1) == 4 +assert total(1,2,3,4) == 10 assert sum(1, 1) == 2 assert sum(1, 3) == 4 + def sum2y(x, y): return x+y*2 + assert sum2y(1, 1) == 3 assert sum2y(1, 3) == 7 + def va(a, b=2, *c, d, **e): assert a == 1 assert b == 22 @@ -23,21 +30,34 @@ def va(a, b=2, *c, d, **e): assert d == 1337 assert e['f'] == 42 + va(1, 22, 3, 4, d=1337, f=42) + def va2(*args, **kwargs): assert args == (5, 4) assert len(kwargs) == 0 + va2(5, 4) x = (5, 4) va2(*x) va2(5, *x[1:]) -# def va3(x, *, b=2): -# pass -# va3(1, 2, 3, b=10) + +def va3(x, *, a, b=2, c=9): + return x + b + c + + +assert va3(1, a=1, b=10) == 20 + +with assertRaises(TypeError): + va3(1, 2, 3, a=1, b=10) + +with assertRaises(TypeError): + va3(1, b=10) + x = {'f': 42, 'e': 1337} y = {'d': 1337} diff --git a/vm/src/bytecode.rs b/vm/src/bytecode.rs index 2fe485b63..b24801861 100644 --- a/vm/src/bytecode.rs +++ b/vm/src/bytecode.rs @@ -31,6 +31,7 @@ pub struct CodeObject { bitflags! { pub struct FunctionOpArg: u8 { const HAS_DEFAULTS = 0x01; + const HAS_KW_ONLY_DEFAULTS = 0x02; const HAS_ANNOTATIONS = 0x04; } } diff --git a/vm/src/compile.rs b/vm/src/compile.rs index 3780595c2..dd045c3bf 100644 --- a/vm/src/compile.rs +++ b/vm/src/compile.rs @@ -422,8 +422,8 @@ impl Compiler { name: &str, args: &ast::Parameters, ) -> Result { - let have_kwargs = !args.defaults.is_empty(); - if have_kwargs { + let have_defaults = !args.defaults.is_empty(); + if have_defaults { // Construct a tuple: let size = args.defaults.len(); for element in &args.defaults { @@ -435,6 +435,25 @@ impl Compiler { }); } + let mut num_kw_only_defaults = 0; + for (kw, default) in args.kwonlyargs.iter().zip(&args.kw_defaults) { + if let Some(default) = default { + self.emit(Instruction::LoadConst { + value: bytecode::Constant::String { + value: kw.arg.clone(), + }, + }); + self.compile_expression(default)?; + num_kw_only_defaults += 1; + } + } + if num_kw_only_defaults > 0 { + self.emit(Instruction::BuildMap { + size: num_kw_only_defaults, + unpack: false, + }); + } + let line_number = self.get_source_line_number(); self.code_object_stack.push(CodeObject::new( args.args.iter().map(|a| a.arg.clone()).collect(), @@ -447,9 +466,12 @@ impl Compiler { )); let mut flags = bytecode::FunctionOpArg::empty(); - if have_kwargs { + if have_defaults { flags |= bytecode::FunctionOpArg::HAS_DEFAULTS; } + if num_kw_only_defaults > 0 { + flags |= bytecode::FunctionOpArg::HAS_KW_ONLY_DEFAULTS; + } Ok(flags) } diff --git a/vm/src/frame.rs b/vm/src/frame.rs index ca3e54d07..2570c0934 100644 --- a/vm/src/frame.rs +++ b/vm/src/frame.rs @@ -12,12 +12,13 @@ use crate::function::PyFuncArgs; use crate::obj::objbool; use crate::obj::objbuiltinfunc::PyBuiltinFunction; use crate::obj::objcode::PyCodeRef; -use crate::obj::objdict::PyDictRef; +use crate::obj::objdict::{PyDict, PyDictRef}; use crate::obj::objint::PyInt; use crate::obj::objiter; use crate::obj::objlist; use crate::obj::objslice::PySlice; use crate::obj::objstr; +use crate::obj::objtuple::PyTuple; use crate::obj::objtype; use crate::obj::objtype::PyClassRef; use crate::pyobject::{ @@ -578,16 +579,33 @@ impl Frame { vm.ctx.new_dict().into_object() }; + let kw_only_defaults = + if flags.contains(bytecode::FunctionOpArg::HAS_KW_ONLY_DEFAULTS) { + Some( + self.pop_value().downcast::().expect( + "Stack value for keyword only defaults expected to be a dict", + ), + ) + } else { + None + }; + let defaults = if flags.contains(bytecode::FunctionOpArg::HAS_DEFAULTS) { - self.pop_value() + Some( + self.pop_value() + .downcast::() + .expect("Stack value for defaults expected to be a tuple"), + ) } else { - vm.get_none() + None }; // pop argc arguments // argument: name, args, globals let scope = self.scope.clone(); - let obj = vm.ctx.new_function(code_obj, scope, defaults); + let obj = vm + .ctx + .new_function(code_obj, scope, defaults, kw_only_defaults); vm.set_attr(&obj, "__annotations__", annotations)?; diff --git a/vm/src/obj/objfunction.rs b/vm/src/obj/objfunction.rs index 410794696..e8c1d7df8 100644 --- a/vm/src/obj/objfunction.rs +++ b/vm/src/obj/objfunction.rs @@ -1,5 +1,7 @@ use crate::frame::Scope; use crate::obj::objcode::PyCodeRef; +use crate::obj::objdict::PyDictRef; +use crate::obj::objtuple::PyTupleRef; use crate::obj::objtype::PyClassRef; use crate::pyobject::{IdProtocol, PyContext, PyObjectRef, PyRef, PyResult, PyValue, TypeProtocol}; use crate::vm::VirtualMachine; @@ -11,15 +13,22 @@ pub struct PyFunction { // TODO: these shouldn't be public pub code: PyCodeRef, pub scope: Scope, - pub defaults: PyObjectRef, + pub defaults: Option, + pub kw_only_defaults: Option, } impl PyFunction { - pub fn new(code: PyCodeRef, scope: Scope, defaults: PyObjectRef) -> Self { + pub fn new( + code: PyCodeRef, + scope: Scope, + defaults: Option, + kw_only_defaults: Option, + ) -> Self { PyFunction { code, scope, defaults, + kw_only_defaults, } } } diff --git a/vm/src/pyobject.rs b/vm/src/pyobject.rs index ee765f987..4d2c8353b 100644 --- a/vm/src/pyobject.rs +++ b/vm/src/pyobject.rs @@ -48,7 +48,7 @@ use crate::obj::objslice; use crate::obj::objstaticmethod; use crate::obj::objstr; use crate::obj::objsuper; -use crate::obj::objtuple::{self, PyTuple}; +use crate::obj::objtuple::{self, PyTuple, PyTupleRef}; use crate::obj::objtype::{self, PyClass, PyClassRef}; use crate::obj::objweakref; use crate::obj::objzip; @@ -658,10 +658,11 @@ impl PyContext { &self, code_obj: PyCodeRef, scope: Scope, - defaults: PyObjectRef, + defaults: Option, + kw_only_defaults: Option, ) -> PyObjectRef { PyObject::new( - PyFunction::new(code_obj, scope, defaults), + PyFunction::new(code_obj, scope, defaults, kw_only_defaults), self.function_type(), Some(self.new_dict()), ) diff --git a/vm/src/vm.rs b/vm/src/vm.rs index 90daaa668..d43ee7bea 100644 --- a/vm/src/vm.rs +++ b/vm/src/vm.rs @@ -23,10 +23,9 @@ use crate::obj::objdict::PyDictRef; use crate::obj::objfunction::{PyFunction, PyMethod}; use crate::obj::objgenerator::PyGeneratorRef; use crate::obj::objiter; -use crate::obj::objlist::PyList; use crate::obj::objsequence; use crate::obj::objstr::{PyString, PyStringRef}; -use crate::obj::objtuple::PyTuple; +use crate::obj::objtuple::PyTupleRef; use crate::obj::objtype; use crate::obj::objtype::PyClassRef; use crate::pyobject::{ @@ -332,9 +331,10 @@ impl VirtualMachine { ref code, ref scope, ref defaults, + ref kw_only_defaults, }) = func_ref.payload() { - return self.invoke_python_function(code, scope, defaults, args); + return self.invoke_python_function(code, scope, defaults, kw_only_defaults, args); } if let Some(PyMethod { ref function, @@ -356,11 +356,18 @@ impl VirtualMachine { &self, code: &PyCodeRef, scope: &Scope, - defaults: &PyObjectRef, + defaults: &Option, + kw_only_defaults: &Option, args: PyFuncArgs, ) -> PyResult { let scope = scope.child_scope(&self.ctx); - self.fill_locals_from_args(&code.code, &scope.get_locals(), args, defaults)?; + self.fill_locals_from_args( + &code.code, + &scope.get_locals(), + args, + defaults, + kw_only_defaults, + )?; // Construct frame: let frame = Frame::new(code.clone(), scope).into_ref(self); @@ -379,12 +386,7 @@ impl VirtualMachine { cells: PyDictRef, locals: PyDictRef, ) -> PyResult { - if let Some(PyFunction { - code, - scope, - defaults: _, - }) = &function.payload() - { + if let Some(PyFunction { code, scope, .. }) = &function.payload() { let scope = scope .child_scope_with_locals(cells) .child_scope_with_locals(locals); @@ -402,7 +404,8 @@ impl VirtualMachine { code_object: &bytecode::CodeObject, locals: &PyDictRef, args: PyFuncArgs, - defaults: &PyObjectRef, + defaults: &Option, + kw_only_defaults: &Option, ) -> PyResult<()> { let nargs = args.args.len(); let nexpected_args = code_object.arg_names.len(); @@ -438,10 +441,7 @@ impl VirtualMachine { locals.set_item(vararg_name, vararg_value, self)?; } - bytecode::Varargs::Unnamed => { - // just ignore the rest of the args - } - bytecode::Varargs::None => { + bytecode::Varargs::Unnamed | bytecode::Varargs::None => { // Check the number of positional arguments if nargs > nexpected_args { return Err(self.new_type_error(format!( @@ -487,19 +487,11 @@ impl VirtualMachine { // Add missing positional arguments, if we have fewer positional arguments than the // function definition calls for if nargs < nexpected_args { - let available_defaults = if defaults.is(&self.get_none()) { - vec![] - } else if let Some(list) = defaults.payload::() { - list.elements.borrow().clone() - } else if let Some(tuple) = defaults.payload::() { - tuple.elements.borrow().clone() - } else { - panic!("function defaults not tuple or None"); - }; + let num_defaults_available = defaults.as_ref().map_or(0, |d| d.elements.borrow().len()); // Given the number of defaults available, check all the arguments for which we // _don't_ have defaults; if any are missing, raise an exception - let required_args = nexpected_args - available_defaults.len(); + let required_args = nexpected_args - num_defaults_available; let mut missing = vec![]; for i in 0..required_args { let variable_name = &code_object.arg_names[i]; @@ -514,31 +506,32 @@ impl VirtualMachine { missing ))); } - - // We have sufficient defaults, so iterate over the corresponding names and use - // the default if we don't already have a value - for (default_index, i) in (required_args..nexpected_args).enumerate() { - let arg_name = &code_object.arg_names[i]; - if !locals.contains_key(arg_name, self) { - locals.set_item(arg_name, available_defaults[default_index].clone(), self)?; + if let Some(defaults) = defaults { + let defaults = defaults.elements.borrow(); + // We have sufficient defaults, so iterate over the corresponding names and use + // the default if we don't already have a value + for (default_index, i) in (required_args..nexpected_args).enumerate() { + let arg_name = &code_object.arg_names[i]; + if !locals.contains_key(arg_name, self) { + locals.set_item(arg_name, defaults[default_index].clone(), self)?; + } } } }; // Check if kw only arguments are all present: - let kwdefs: HashMap = HashMap::new(); for arg_name in &code_object.kwonlyarg_names { if !locals.contains_key(arg_name, self) { - if kwdefs.contains_key(arg_name) { - // If not yet specified, take the default value - unimplemented!(); - } else { - // No default value and not specified. - return Err(self.new_type_error(format!( - "Missing required kw only argument: '{}'", - arg_name - ))); + if let Some(kw_only_defaults) = kw_only_defaults { + if let Some(default) = kw_only_defaults.get_item(arg_name, self) { + locals.set_item(arg_name, default, self); + continue; + } } + + // No default value and not specified. + return Err(self + .new_type_error(format!("Missing required kw only argument: '{}'", arg_name))); } }