From c5789a03a32d76e83cc5da0a9ed6e434997ff277 Mon Sep 17 00:00:00 2001 From: Adam Kelly Date: Sun, 3 Mar 2019 14:06:36 +0000 Subject: [PATCH 1/5] Proper construction of scope for exec/eval. --- tests/snippets/test_exec.py | 12 ++++++++++++ vm/src/builtins.rs | 35 ++++++++++++++++++----------------- vm/src/frame.rs | 6 ++++++ vm/src/vm.rs | 8 +++++++- 4 files changed, 43 insertions(+), 18 deletions(-) create mode 100644 tests/snippets/test_exec.py diff --git a/tests/snippets/test_exec.py b/tests/snippets/test_exec.py new file mode 100644 index 0000000000..c610af565c --- /dev/null +++ b/tests/snippets/test_exec.py @@ -0,0 +1,12 @@ +exec("def square(x):\n return x * x\n") +assert 16 == square(4) + +d = {} +exec("def square(x):\n return x * x\n", {}, d) +assert 16 == d['square'](4) + +exec("assert 2 == x", {}, {'x': 2}) +exec("assert 2 == x", {'x': 2}, {}) +exec("assert 4 == x", {'x': 2}, {'x': 4}) + +exec("assert max(1, 2) == 2", {}, {}) diff --git a/vm/src/builtins.rs b/vm/src/builtins.rs index e7a3bbb448..3b8ea0f2a0 100644 --- a/vm/src/builtins.rs +++ b/vm/src/builtins.rs @@ -18,7 +18,6 @@ use crate::frame::{Scope, ScopeRef}; use crate::pyobject::{ AttributeProtocol, IdProtocol, PyContext, PyFuncArgs, PyObjectRef, PyResult, TypeProtocol, }; -use std::rc::Rc; #[cfg(not(target_arch = "wasm32"))] use crate::stdlib::io::io_open; @@ -192,7 +191,7 @@ fn builtin_eval(vm: &mut VirtualMachine, args: PyFuncArgs) -> PyResult { args, required = [(source, None)], optional = [ - (_globals, Some(vm.ctx.dict_type())), + (globals, Some(vm.ctx.dict_type())), (locals, Some(vm.ctx.dict_type())) ] ); @@ -215,7 +214,7 @@ fn builtin_eval(vm: &mut VirtualMachine, args: PyFuncArgs) -> PyResult { return Err(vm.new_type_error("code argument must be str or code object".to_string())); }; - let scope = make_scope(vm, locals); + let scope = make_scope(vm, globals, locals); // Run the source: vm.run_code_obj(code_obj.clone(), scope) @@ -229,7 +228,7 @@ fn builtin_exec(vm: &mut VirtualMachine, args: PyFuncArgs) -> PyResult { args, required = [(source, None)], optional = [ - (_globals, Some(vm.ctx.dict_type())), + (globals, Some(vm.ctx.dict_type())), (locals, Some(vm.ctx.dict_type())) ] ); @@ -252,26 +251,28 @@ fn builtin_exec(vm: &mut VirtualMachine, args: PyFuncArgs) -> PyResult { return Err(vm.new_type_error("source argument must be str or code object".to_string())); }; - let scope = make_scope(vm, locals); + let scope = make_scope(vm, globals, locals); // Run the code: vm.run_code_obj(code_obj, scope) } -fn make_scope(vm: &mut VirtualMachine, locals: Option<&PyObjectRef>) -> ScopeRef { - // handle optional global and locals - let locals = if let Some(locals) = locals { - locals.clone() - } else { - vm.new_dict() +fn make_scope( + vm: &mut VirtualMachine, + globals: Option<&PyObjectRef>, + locals: Option<&PyObjectRef>, +) -> ScopeRef { + let current_scope = vm.current_scope(); + let parent = match globals { + Some(dict) => Some(Scope::new(dict.clone(), Some(vm.get_builtin_scope()))), + None => current_scope.parent.clone(), + }; + let locals = match locals { + Some(dict) => dict.clone(), + None => current_scope.locals.clone(), }; - // TODO: handle optional globals - // Construct new scope: - Rc::new(Scope { - locals, - parent: None, - }) + Scope::new(locals, parent) } fn builtin_format(vm: &mut VirtualMachine, args: PyFuncArgs) -> PyResult { diff --git a/vm/src/frame.rs b/vm/src/frame.rs index ec0c099238..c76552b76e 100644 --- a/vm/src/frame.rs +++ b/vm/src/frame.rs @@ -35,6 +35,12 @@ pub struct Scope { } pub type ScopeRef = Rc; +impl Scope { + pub fn new(locals: PyObjectRef, parent: Option) -> ScopeRef { + Rc::new(Scope { locals, parent }) + } +} + #[derive(Clone, Debug)] struct Block { /// The type of block. diff --git a/vm/src/vm.rs b/vm/src/vm.rs index c4b6c14ce8..60a260479b 100644 --- a/vm/src/vm.rs +++ b/vm/src/vm.rs @@ -90,6 +90,12 @@ impl VirtualMachine { result } + pub fn current_scope(&self) -> &ScopeRef { + let current_frame = &self.frames[self.frames.len() - 1]; + let frame = objframe::get_value(current_frame); + &frame.scope + } + /// Create a new python string object. pub fn new_str(&self, s: String) -> PyObjectRef { self.ctx.new_str(s) @@ -218,7 +224,7 @@ impl VirtualMachine { &self.ctx } - pub fn get_builtin_scope(&mut self) -> ScopeRef { + pub fn get_builtin_scope(&self) -> ScopeRef { let a2 = &*self.builtins; match a2.payload { PyObjectPayload::Module { ref scope, .. } => scope.clone(), From 4dd03047122d35ec35a40660b45f6c6a5bb0ec7b Mon Sep 17 00:00:00 2001 From: Adam Kelly Date: Sun, 3 Mar 2019 16:33:45 +0000 Subject: [PATCH 2/5] Add check_but_allow_none macro for optional argument checking. --- vm/src/macros.rs | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/vm/src/macros.rs b/vm/src/macros.rs index f91cd7c614..167a305a18 100644 --- a/vm/src/macros.rs +++ b/vm/src/macros.rs @@ -35,6 +35,34 @@ macro_rules! type_check { }; } +#[macro_export] +macro_rules! check_but_allow_none { + ( $vm: ident, $arg: ident, $expected_type: expr ) => { + let $arg = match $arg { + Some(arg) => { + if arg.is(&$vm.get_none()) { + None + } else { + if $crate::obj::objtype::real_isinstance($vm, arg, &$expected_type)? { + Some(arg) + } else { + let arg_typ = arg.typ(); + let actual_type = $vm.to_pystr(&arg_typ)?; + let expected_type_name = $vm.to_pystr(&$expected_type)?; + return Err($vm.new_type_error(format!( + "{} must be a {}, not {}", + stringify!($arg), + expected_type_name, + actual_type + ))); + } + } + } + None => None, + }; + }; +} + #[macro_export] macro_rules! arg_check { ( $vm: ident, $args:ident ) => { From 6548c365cb7a9556dc2161b827b72fae8be9e57c Mon Sep 17 00:00:00 2001 From: Adam Kelly Date: Sun, 3 Mar 2019 16:45:48 +0000 Subject: [PATCH 3/5] Use check_but_allow_none for exec/eval. --- tests/snippets/test_exec.py | 9 +++++++++ vm/src/builtins.rs | 14 ++++++-------- 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/tests/snippets/test_exec.py b/tests/snippets/test_exec.py index c610af565c..852ef61432 100644 --- a/tests/snippets/test_exec.py +++ b/tests/snippets/test_exec.py @@ -10,3 +10,12 @@ exec("assert 4 == x", {'x': 2}, {'x': 4}) exec("assert max(1, 2) == 2", {}, {}) + +exec("max(1, 5, square(5)) == 25", None) + +try: + exec("", 1) +except TypeError: + pass +else: + raise TypeError("exec should fail unless globals is a dict or None") diff --git a/vm/src/builtins.rs b/vm/src/builtins.rs index 3b8ea0f2a0..71d6112c57 100644 --- a/vm/src/builtins.rs +++ b/vm/src/builtins.rs @@ -190,12 +190,11 @@ fn builtin_eval(vm: &mut VirtualMachine, args: PyFuncArgs) -> PyResult { vm, args, required = [(source, None)], - optional = [ - (globals, Some(vm.ctx.dict_type())), - (locals, Some(vm.ctx.dict_type())) - ] + optional = [(globals, None), (locals, Some(vm.ctx.dict_type()))] ); + check_but_allow_none!(vm, globals, vm.ctx.dict_type()); + // Determine code object: let code_obj = if objtype::isinstance(source, &vm.ctx.code_type()) { source.clone() @@ -227,12 +226,11 @@ fn builtin_exec(vm: &mut VirtualMachine, args: PyFuncArgs) -> PyResult { vm, args, required = [(source, None)], - optional = [ - (globals, Some(vm.ctx.dict_type())), - (locals, Some(vm.ctx.dict_type())) - ] + optional = [(globals, None), (locals, Some(vm.ctx.dict_type()))] ); + check_but_allow_none!(vm, globals, vm.ctx.dict_type()); + // Determine code object: let code_obj = if objtype::isinstance(source, &vm.ctx.str_type()) { let mode = compile::Mode::Exec; From 0d4887abe6d6fe77ffdaae9dc669cf865778adfe Mon Sep 17 00:00:00 2001 From: Adam Kelly Date: Mon, 4 Mar 2019 14:22:48 +0000 Subject: [PATCH 4/5] Fix missing asserts and add (disabled) tests for things that don't work yet. --- tests/snippets/test_exec.py | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/tests/snippets/test_exec.py b/tests/snippets/test_exec.py index 852ef61432..37ba33ff13 100644 --- a/tests/snippets/test_exec.py +++ b/tests/snippets/test_exec.py @@ -11,7 +11,28 @@ exec("assert max(1, 2) == 2", {}, {}) -exec("max(1, 5, square(5)) == 25", None) +exec("assert max(1, 5, square(5)) == 25", None) + +# +# These doesn't work yet: +# +# Local environment shouldn't replace global environment: +# +# exec("assert max(1, 5, square(5)) == 25", None, {}) +# +# Closures aren't available if local scope is replaced: +# +# def g(): +# seven = "seven" +# def f(): +# try: +# exec("seven", None, {}) +# except NameError: +# pass +# else: +# raise NameError("seven shouldn't be in scope") +# f() +# g() try: exec("", 1) From c4953ee9ec51d0ec4fb0bfca0b1b2a1a99c8a339 Mon Sep 17 00:00:00 2001 From: Adam Kelly Date: Mon, 4 Mar 2019 14:29:44 +0000 Subject: [PATCH 5/5] Remove new macro and inline check into make_scope. --- vm/src/builtins.rs | 34 ++++++++++++++++++++++++++-------- vm/src/macros.rs | 28 ---------------------------- 2 files changed, 26 insertions(+), 36 deletions(-) diff --git a/vm/src/builtins.rs b/vm/src/builtins.rs index 71d6112c57..b146fcf913 100644 --- a/vm/src/builtins.rs +++ b/vm/src/builtins.rs @@ -193,7 +193,7 @@ fn builtin_eval(vm: &mut VirtualMachine, args: PyFuncArgs) -> PyResult { optional = [(globals, None), (locals, Some(vm.ctx.dict_type()))] ); - check_but_allow_none!(vm, globals, vm.ctx.dict_type()); + let scope = make_scope(vm, globals, locals)?; // Determine code object: let code_obj = if objtype::isinstance(source, &vm.ctx.code_type()) { @@ -213,8 +213,6 @@ fn builtin_eval(vm: &mut VirtualMachine, args: PyFuncArgs) -> PyResult { return Err(vm.new_type_error("code argument must be str or code object".to_string())); }; - let scope = make_scope(vm, globals, locals); - // Run the source: vm.run_code_obj(code_obj.clone(), scope) } @@ -229,7 +227,7 @@ fn builtin_exec(vm: &mut VirtualMachine, args: PyFuncArgs) -> PyResult { optional = [(globals, None), (locals, Some(vm.ctx.dict_type()))] ); - check_but_allow_none!(vm, globals, vm.ctx.dict_type()); + let scope = make_scope(vm, globals, locals)?; // Determine code object: let code_obj = if objtype::isinstance(source, &vm.ctx.str_type()) { @@ -249,8 +247,6 @@ fn builtin_exec(vm: &mut VirtualMachine, args: PyFuncArgs) -> PyResult { return Err(vm.new_type_error("source argument must be str or code object".to_string())); }; - let scope = make_scope(vm, globals, locals); - // Run the code: vm.run_code_obj(code_obj, scope) } @@ -259,7 +255,29 @@ fn make_scope( vm: &mut VirtualMachine, globals: Option<&PyObjectRef>, locals: Option<&PyObjectRef>, -) -> ScopeRef { +) -> PyResult { + let dict_type = vm.ctx.dict_type(); + let globals = match globals { + Some(arg) => { + if arg.is(&vm.get_none()) { + None + } else { + if vm.isinstance(arg, &dict_type)? { + Some(arg) + } else { + let arg_typ = arg.typ(); + let actual_type = vm.to_pystr(&arg_typ)?; + let expected_type_name = vm.to_pystr(&dict_type)?; + return Err(vm.new_type_error(format!( + "globals must be a {}, not {}", + expected_type_name, actual_type + ))); + } + } + } + None => None, + }; + let current_scope = vm.current_scope(); let parent = match globals { Some(dict) => Some(Scope::new(dict.clone(), Some(vm.get_builtin_scope()))), @@ -270,7 +288,7 @@ fn make_scope( None => current_scope.locals.clone(), }; - Scope::new(locals, parent) + Ok(Scope::new(locals, parent)) } fn builtin_format(vm: &mut VirtualMachine, args: PyFuncArgs) -> PyResult { diff --git a/vm/src/macros.rs b/vm/src/macros.rs index 167a305a18..f91cd7c614 100644 --- a/vm/src/macros.rs +++ b/vm/src/macros.rs @@ -35,34 +35,6 @@ macro_rules! type_check { }; } -#[macro_export] -macro_rules! check_but_allow_none { - ( $vm: ident, $arg: ident, $expected_type: expr ) => { - let $arg = match $arg { - Some(arg) => { - if arg.is(&$vm.get_none()) { - None - } else { - if $crate::obj::objtype::real_isinstance($vm, arg, &$expected_type)? { - Some(arg) - } else { - let arg_typ = arg.typ(); - let actual_type = $vm.to_pystr(&arg_typ)?; - let expected_type_name = $vm.to_pystr(&$expected_type)?; - return Err($vm.new_type_error(format!( - "{} must be a {}, not {}", - stringify!($arg), - expected_type_name, - actual_type - ))); - } - } - } - None => None, - }; - }; -} - #[macro_export] macro_rules! arg_check { ( $vm: ident, $args:ident ) => {