Skip to content

Proper construction of scope for exec/eval. #590

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Mar 4, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 42 additions & 0 deletions tests/snippets/test_exec.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
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", {}, {})

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)
except TypeError:
pass
else:
raise TypeError("exec should fail unless globals is a dict or None")
67 changes: 42 additions & 25 deletions vm/src/builtins.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -191,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()))]
);

let scope = make_scope(vm, globals, locals)?;

// Determine code object:
let code_obj = if objtype::isinstance(source, &vm.ctx.code_type()) {
source.clone()
Expand All @@ -215,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, locals);

// Run the source:
vm.run_code_obj(code_obj.clone(), scope)
}
Expand All @@ -228,12 +224,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()))]
);

let scope = make_scope(vm, globals, locals)?;

// Determine code object:
let code_obj = if objtype::isinstance(source, &vm.ctx.str_type()) {
let mode = compile::Mode::Exec;
Expand All @@ -252,26 +247,48 @@ 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);

// 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>,
) -> PyResult<ScopeRef> {
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,
};

// TODO: handle optional globals
// Construct new scope:
Rc::new(Scope {
locals,
parent: None,
})
let current_scope = vm.current_scope();
let parent = match globals {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check here if globals is passed at all, None or an actual dictionary.

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(),
};

Ok(Scope::new(locals, parent))
}

fn builtin_format(vm: &mut VirtualMachine, args: PyFuncArgs) -> PyResult {
Expand Down
6 changes: 6 additions & 0 deletions vm/src/frame.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,12 @@ pub struct Scope {
}
pub type ScopeRef = Rc<Scope>;

impl Scope {
pub fn new(locals: PyObjectRef, parent: Option<ScopeRef>) -> ScopeRef {
Rc::new(Scope { locals, parent })
}
}

#[derive(Clone, Debug)]
struct Block {
/// The type of block.
Expand Down
8 changes: 7 additions & 1 deletion vm/src/vm.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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(),
Expand Down