From eecddd417bb4f335f079c2be7aed51857cfd8ca3 Mon Sep 17 00:00:00 2001 From: Daniel Watkins Date: Sat, 25 Aug 2018 19:25:12 -0400 Subject: [PATCH 1/6] Distinguish between a mutable and non-mutable borrow of vm.current_frame This renames the existing (mutable) VirtualMachine.current_frame to current_frame_mut and introduces VirtualMachine.current_frame which is (a) public, and (b) does not take a mutable reference. (This also switches callers to use the non-mutable version where possible.) --- vm/src/vm.rs | 30 +++++++++++++++++------------- 1 file changed, 17 insertions(+), 13 deletions(-) diff --git a/vm/src/vm.rs b/vm/src/vm.rs index 781cb3315c..8fe5d216b0 100644 --- a/vm/src/vm.rs +++ b/vm/src/vm.rs @@ -57,7 +57,7 @@ impl VirtualMachine { } pub fn new_scope(&mut self) -> PyObjectRef { - let parent_scope = self.current_frame().locals.clone(); + let parent_scope = self.current_frame_mut().locals.clone(); self.ctx.new_scope(Some(parent_scope)) } @@ -123,7 +123,11 @@ impl VirtualMachine { obj.borrow().str() } - fn current_frame(&mut self) -> &mut Frame { + pub fn current_frame(&self) -> &Frame { + self.frames.last().unwrap() + } + + fn current_frame_mut(&mut self) -> &mut Frame { self.frames.last_mut().unwrap() } @@ -132,15 +136,15 @@ impl VirtualMachine { } fn push_block(&mut self, block: Block) { - self.current_frame().push_block(block); + self.current_frame_mut().push_block(block); } fn pop_block(&mut self) -> Option { - self.current_frame().pop_block() + self.current_frame_mut().pop_block() } fn last_block(&mut self) -> &Block { - self.current_frame().last_block() + self.current_frame_mut().last_block() } fn unwind_loop(&mut self) -> Block { @@ -172,24 +176,24 @@ impl VirtualMachine { } fn push_value(&mut self, obj: PyObjectRef) { - self.current_frame().push_value(obj); + self.current_frame_mut().push_value(obj); } fn pop_value(&mut self) -> PyObjectRef { - self.current_frame().pop_value() + self.current_frame_mut().pop_value() } fn pop_multiple(&mut self, count: usize) -> Vec { - self.current_frame().pop_multiple(count) + self.current_frame_mut().pop_multiple(count) } fn last_value(&mut self) -> PyObjectRef { - self.current_frame().last_value() + self.current_frame_mut().last_value() } fn store_name(&mut self, name: &String) -> Option { let obj = self.pop_value(); - self.current_frame().locals.set_item(name, obj); + self.current_frame_mut().locals.set_item(name, obj); None } @@ -539,7 +543,7 @@ impl VirtualMachine { // Execute a single instruction: fn execute_instruction(&mut self) -> Option { - let instruction = self.current_frame().fetch_instruction(); + let instruction = self.current_frame_mut().fetch_instruction(); { trace!("======="); /* TODO: @@ -866,7 +870,7 @@ impl VirtualMachine { } bytecode::Instruction::StoreLocals => { let locals = self.pop_value(); - let ref mut frame = self.current_frame(); + let ref mut frame = self.current_frame_mut(); match frame.locals.borrow_mut().kind { PyObjectKind::Scope { ref mut scope } => { scope.locals = locals; @@ -879,7 +883,7 @@ impl VirtualMachine { } fn jump(&mut self, label: &bytecode::Label) { - let current_frame = self.current_frame(); + let current_frame = self.current_frame_mut(); let target_pc = current_frame.code.label_map[label]; trace!( "program counter from {:?} to {:?}", From 767ceaefb58a5578c31320f39b77e05567b34ace Mon Sep 17 00:00:00 2001 From: Daniel Watkins Date: Sat, 25 Aug 2018 19:32:33 -0400 Subject: [PATCH 2/6] Start storing the source_path of a code object This will be required for (at least) import and exception display. --- src/main.rs | 10 +++++----- vm/src/builtins.rs | 2 +- vm/src/bytecode.rs | 4 +++- vm/src/compile.rs | 16 ++++++++++------ vm/src/eval.rs | 2 +- vm/src/import.rs | 7 ++++++- 6 files changed, 26 insertions(+), 15 deletions(-) diff --git a/src/main.rs b/src/main.rs index 698e248ae8..771f643c85 100644 --- a/src/main.rs +++ b/src/main.rs @@ -49,9 +49,9 @@ fn main() { } } -fn _run_string(source: &String) { +fn _run_string(source: &String, source_path: Option) { let mut vm = VirtualMachine::new(); - let code_obj = compile::compile(&mut vm, &source, compile::Mode::Exec).unwrap(); + let code_obj = compile::compile(&mut vm, &source, compile::Mode::Exec, source_path).unwrap(); debug!("Code object: {:?}", code_obj.borrow()); let builtins = vm.get_builtin_scope(); let vars = vm.context().new_scope(Some(builtins)); // Keep track of local variables @@ -68,7 +68,7 @@ fn run_command(source: &mut String) { // This works around https://github.com/RustPython/RustPython/issues/17 source.push_str("\n"); - _run_string(source) + _run_string(source, None) } fn run_script(script_file: &String) { @@ -76,7 +76,7 @@ fn run_script(script_file: &String) { // Parse an ast from it: let filepath = Path::new(script_file); match parser::read_file(filepath) { - Ok(source) => _run_string(&source), + Ok(source) => _run_string(&source, Some(filepath.to_str().unwrap().to_string())), Err(msg) => { error!("Parsing went horribly wrong: {}", msg); std::process::exit(1); @@ -85,7 +85,7 @@ fn run_script(script_file: &String) { } fn shell_exec(vm: &mut VirtualMachine, source: &String, scope: PyObjectRef) -> bool { - match compile::compile(vm, source, compile::Mode::Single) { + match compile::compile(vm, source, compile::Mode::Single, None) { Ok(code) => { match vm.run_code_obj(code, scope.clone()) { Ok(_value) => { diff --git a/vm/src/builtins.rs b/vm/src/builtins.rs index ab6b4bd01b..7076bacbb3 100644 --- a/vm/src/builtins.rs +++ b/vm/src/builtins.rs @@ -102,7 +102,7 @@ fn builtin_compile(vm: &mut VirtualMachine, args: PyFuncArgs) -> PyResult { let mode = compile::Mode::Eval; let source = args.args[0].borrow().str(); - match compile::compile(vm, &source, mode) { + match compile::compile(vm, &source, mode, None) { Ok(value) => Ok(value), Err(msg) => Err(vm.new_exception(msg)), } diff --git a/vm/src/bytecode.rs b/vm/src/bytecode.rs index 2f7b6853dd..d2a61f1261 100644 --- a/vm/src/bytecode.rs +++ b/vm/src/bytecode.rs @@ -22,15 +22,17 @@ pub struct CodeObject { pub label_map: HashMap, pub locations: Vec, pub arg_names: Vec, + pub source_path: Option, } impl CodeObject { - pub fn new(arg_names: Vec) -> CodeObject { + pub fn new(arg_names: Vec, source_path: Option) -> CodeObject { CodeObject { instructions: Vec::new(), label_map: HashMap::new(), locations: Vec::new(), arg_names: arg_names, + source_path: source_path, } } } diff --git a/vm/src/compile.rs b/vm/src/compile.rs index 5eeccc1f3a..99a5c98c64 100644 --- a/vm/src/compile.rs +++ b/vm/src/compile.rs @@ -19,9 +19,10 @@ pub fn compile( vm: &mut VirtualMachine, source: &String, mode: Mode, + source_path: Option, ) -> Result { let mut compiler = Compiler::new(); - compiler.push_new_code_object(); + compiler.push_new_code_object(source_path); match mode { Mode::Exec => match parser::parse_program(source) { Ok(ast) => { @@ -84,8 +85,9 @@ impl Compiler { } } - fn push_new_code_object(&mut self) { - self.code_object_stack.push(CodeObject::new(Vec::new())); + fn push_new_code_object(&mut self, source_path: Option) { + self.code_object_stack + .push(CodeObject::new(Vec::new(), source_path.clone())); } fn pop_code_object(&mut self) -> CodeObject { @@ -341,7 +343,8 @@ impl Compiler { } ast::Statement::FunctionDef { name, args, body } => { // Create bytecode for this function: - self.code_object_stack.push(CodeObject::new(args.to_vec())); + self.code_object_stack + .push(CodeObject::new(args.to_vec(), None)); self.compile_statements(body); // Emit None at end: @@ -369,7 +372,7 @@ impl Compiler { ast::Statement::ClassDef { name, body, args } => { self.emit(Instruction::LoadBuildClass); self.code_object_stack - .push(CodeObject::new(vec![String::from("__locals__")])); + .push(CodeObject::new(vec![String::from("__locals__")], None)); self.emit(Instruction::LoadName { name: String::from("__locals__"), }); @@ -694,7 +697,8 @@ impl Compiler { }); } ast::Expression::Lambda { args, body } => { - self.code_object_stack.push(CodeObject::new(args.to_vec())); + self.code_object_stack + .push(CodeObject::new(args.to_vec(), None)); self.compile_expression(body); self.emit(Instruction::ReturnValue); let code = self.code_object_stack.pop().unwrap(); diff --git a/vm/src/eval.rs b/vm/src/eval.rs index cb0f1a0212..dc7dce297e 100644 --- a/vm/src/eval.rs +++ b/vm/src/eval.rs @@ -5,7 +5,7 @@ use super::pyobject::{PyObjectRef, PyResult}; use super::vm::VirtualMachine; pub fn eval(vm: &mut VirtualMachine, source: &String, scope: PyObjectRef) -> PyResult { - match compile::compile(vm, source, compile::Mode::Eval) { + match compile::compile(vm, source, compile::Mode::Eval, None) { Ok(bytecode) => { debug!("Code object: {:?}", bytecode); vm.run_code_obj(bytecode, scope) diff --git a/vm/src/import.rs b/vm/src/import.rs index cafdcaa025..666f98181d 100644 --- a/vm/src/import.rs +++ b/vm/src/import.rs @@ -26,7 +26,12 @@ fn import_module(vm: &mut VirtualMachine, module: &String) -> PyResult { let source = parser::read_file(filepath.as_path()) .map_err(|e| vm.new_exception(format!("Error: {:?}", e)))?; - let code_obj = match compile::compile(vm, &source, compile::Mode::Exec) { + let code_obj = match compile::compile( + vm, + &source, + compile::Mode::Exec, + Some(filepath.to_str().unwrap().to_string()), + ) { Ok(bytecode) => { debug!("Code object: {:?}", bytecode); bytecode From a457153ce16e90061b5e51e332370bf0161f6c6e Mon Sep 17 00:00:00 2001 From: Daniel Watkins Date: Sat, 25 Aug 2018 19:33:03 -0400 Subject: [PATCH 3/6] Consider imports relative to their source Currently they are considered relative to the current working directory of the interpreter, which is incorrect. Fixes #83. --- tests/test_snippets.py | 2 -- vm/src/import.rs | 15 ++++++++++++++- vm/src/sysmodule.rs | 3 +-- 3 files changed, 15 insertions(+), 5 deletions(-) diff --git a/tests/test_snippets.py b/tests/test_snippets.py index c7725b2333..77774f2056 100644 --- a/tests/test_snippets.py +++ b/tests/test_snippets.py @@ -54,7 +54,6 @@ def perform_test(filename, method, test_type): def run_via_cpython(filename): """ Simply invoke python itself on the script """ env = os.environ.copy() - env['PYTHONPATH'] = '.' subprocess.check_call([sys.executable, filename], env=env) @@ -78,7 +77,6 @@ def run_via_rustpython(filename, test_type): log_level = 'info' if test_type == _TestType.benchmark else 'trace' env['RUST_LOG'] = '{},cargo=error,jobserver=error'.format(log_level) env['RUST_BACKTRACE'] = '1' - env['PYTHONPATH'] = os.path.dirname(filename) subprocess.check_call( ['cargo', 'run', '--release', filename], env=env) diff --git a/vm/src/import.rs b/vm/src/import.rs index 666f98181d..83f0aea455 100644 --- a/vm/src/import.rs +++ b/vm/src/import.rs @@ -70,7 +70,7 @@ pub fn import(vm: &mut VirtualMachine, module: &String, symbol: &Option) fn find_source(vm: &VirtualMachine, name: &String) -> io::Result { let sys_path = vm.sys_module.get_item("path").unwrap(); - let paths: Vec = match sys_path.borrow().kind { + let mut paths: Vec = match sys_path.borrow().kind { PyObjectKind::List { ref elements } => elements .iter() .filter_map(|item| match item.borrow().kind { @@ -80,6 +80,19 @@ fn find_source(vm: &VirtualMachine, name: &String) -> io::Result { _ => panic!("sys.path unexpectedly not a list"), }; + let source_path = &vm.current_frame().code.source_path; + paths.insert( + 0, + match source_path { + Some(source_path) => { + let mut source_pathbuf = PathBuf::from(source_path); + source_pathbuf.pop(); + source_pathbuf + } + None => PathBuf::from("."), + }, + ); + let suffixes = [".py", "/__init__.py"]; let mut filepaths = vec![]; for path in paths { diff --git a/vm/src/sysmodule.rs b/vm/src/sysmodule.rs index ea73866faf..27260d6bbc 100644 --- a/vm/src/sysmodule.rs +++ b/vm/src/sysmodule.rs @@ -8,13 +8,12 @@ use std::env; use super::pyobject::{DictProtocol, PyContext, PyObjectRef}; pub fn mk_module(ctx: &PyContext) -> PyObjectRef { - let mut path_list = match env::var_os("PYTHONPATH") { + let path_list = match env::var_os("PYTHONPATH") { Some(paths) => env::split_paths(&paths) .map(|path| ctx.new_str(path.to_str().unwrap().to_string())) .collect(), None => vec![], }; - path_list.insert(0, ctx.new_str("".to_string())); let path = ctx.new_list(path_list); let modules = ctx.new_dict(); let sys_name = "sys".to_string(); From fb2c959cc45d17d9db92d4b463d6977f121a9b66 Mon Sep 17 00:00:00 2001 From: Adam Kelly Date: Sat, 25 Aug 2018 08:34:32 +0100 Subject: [PATCH 4/6] Missing __bool__ is truthy and None is falsey. --- tests/snippets/bools.py | 2 ++ vm/src/objbool.rs | 18 +++++++++++------- vm/src/objobject.rs | 5 ----- 3 files changed, 13 insertions(+), 12 deletions(-) diff --git a/tests/snippets/bools.py b/tests/snippets/bools.py index e4ff5c52cb..4ff508d0dc 100644 --- a/tests/snippets/bools.py +++ b/tests/snippets/bools.py @@ -10,6 +10,8 @@ assert not {} assert not "" assert not 0.0 +assert not None + assert bool() == False assert bool(1) == True assert bool({}) == False diff --git a/vm/src/objbool.rs b/vm/src/objbool.rs index b0252dd7dd..b9f9f59542 100644 --- a/vm/src/objbool.rs +++ b/vm/src/objbool.rs @@ -13,14 +13,18 @@ pub fn boolval(vm: &mut VirtualMachine, obj: PyObjectRef) -> Result !elements.is_empty(), PyObjectKind::Dict { ref elements } => !elements.is_empty(), PyObjectKind::String { ref value } => !value.is_empty(), + PyObjectKind::None { .. } => false, _ => { - let f = objtype::get_attribute(vm, obj.clone(), &String::from("__bool__"))?; - match vm.invoke(f, PyFuncArgs::new()) { - Ok(result) => match result.borrow().kind { - PyObjectKind::Boolean { value } => value, - _ => return Err(vm.new_type_error(String::from("TypeError"))), - }, - Err(err) => return Err(err), + if let Ok(f) = objtype::get_attribute(vm, obj.clone(), &String::from("__bool__")) { + match vm.invoke(f, PyFuncArgs::new()) { + Ok(result) => match result.borrow().kind { + PyObjectKind::Boolean { value } => value, + _ => return Err(vm.new_type_error(String::from("TypeError"))), + }, + Err(err) => return Err(err), + } + } else { + true } } }; diff --git a/vm/src/objobject.rs b/vm/src/objobject.rs index 56f985de9d..0d8a3cabae 100644 --- a/vm/src/objobject.rs +++ b/vm/src/objobject.rs @@ -32,7 +32,6 @@ pub fn init(context: &PyContext) { let ref object = context.object; object.set_attr("__new__", context.new_rustfunc(new_instance)); object.set_attr("__dict__", context.new_member_descriptor(object_dict)); - object.set_attr("__bool__", context.new_rustfunc(object_bool)); } fn object_dict(vm: &mut VirtualMachine, args: PyFuncArgs) -> PyResult { @@ -42,7 +41,3 @@ fn object_dict(vm: &mut VirtualMachine, args: PyFuncArgs) -> PyResult { _ => Err(vm.new_type_error("TypeError: no dictionary.".to_string())), } } - -fn object_bool(vm: &mut VirtualMachine, _args: PyFuncArgs) -> PyResult { - Ok(vm.context().new_bool(true)) -} From e10dfa4f8760e3cd7adea271ed37e9afb8626946 Mon Sep 17 00:00:00 2001 From: Adam Kelly Date: Sat, 25 Aug 2018 08:53:40 +0100 Subject: [PATCH 5/6] Compile and and or. --- tests/snippets/bools.py | 9 ++++++ vm/src/bytecode.rs | 6 ++++ vm/src/compile.rs | 69 ++++++++++++++--------------------------- vm/src/vm.rs | 50 ++++++++++++++++++----------- 4 files changed, 71 insertions(+), 63 deletions(-) diff --git a/tests/snippets/bools.py b/tests/snippets/bools.py index 4ff508d0dc..440bcde900 100644 --- a/tests/snippets/bools.py +++ b/tests/snippets/bools.py @@ -30,3 +30,12 @@ class Falsey: return False assert not Falsey() + +assert (True or fake) +assert (False or True) +assert not (False or False) +assert ("thing" or 0) == "thing" + +assert (True and True) +assert not (False and fake) +assert (True and 5) == 5 diff --git a/vm/src/bytecode.rs b/vm/src/bytecode.rs index 2f7b6853dd..dccf7f6a16 100644 --- a/vm/src/bytecode.rs +++ b/vm/src/bytecode.rs @@ -83,9 +83,15 @@ pub enum Instruction { JumpIf { target: Label, }, + JumpIfOrPop { + target: Label, + }, JumpIfFalse { target: Label, }, + JumpIfFalseOrPop { + target: Label, + }, MakeFunction, CallFunction { count: usize, diff --git a/vm/src/compile.rs b/vm/src/compile.rs index 5eeccc1f3a..d4a1848d8a 100644 --- a/vm/src/compile.rs +++ b/vm/src/compile.rs @@ -148,13 +148,15 @@ impl Compiler { match orelse { None => { // Only if: - self.compile_test(test, end_label); + self.compile_expression(test); + self.emit(Instruction::JumpIfFalse { target: end_label }); self.compile_statements(body); } Some(statements) => { // if - else: let else_label = self.new_label(); - self.compile_test(test, else_label); + self.compile_expression(test); + self.emit(Instruction::JumpIfFalse { target: else_label }); self.compile_statements(body); self.emit(Instruction::Jump { target: end_label }); @@ -180,7 +182,8 @@ impl Compiler { self.set_label(start_label); - self.compile_test(test, end_label); + self.compile_expression(test); + self.emit(Instruction::JumpIfFalse { target: end_label }); self.compile_statements(body); self.emit(Instruction::Jump { target: start_label, @@ -527,31 +530,6 @@ impl Compiler { self.emit(Instruction::BinaryOperation { op: i }); } - fn compile_test(&mut self, expression: &ast::Expression, not_label: Label) { - // Compile expression for test, and jump to label if false - match expression { - ast::Expression::BoolOp { a, op, b } => match op { - ast::BooleanOperator::And => { - self.compile_test(a, not_label); - self.compile_test(b, not_label); - } - ast::BooleanOperator::Or => { - // TODO: Implement boolean or - // TODO: implement short circuit code by fiddeling with the labels - self.new_label(); - self.compile_test(a, not_label); - self.compile_test(b, not_label); - panic!("Not impl"); - } - }, - _ => { - // If all else fail, fall back to simple checking of boolean value: - self.compile_expression(expression); - self.emit(Instruction::JumpIfFalse { target: not_label }); - } - } - } - fn compile_expression(&mut self, expression: &ast::Expression) { trace!("Compiling {:?}", expression); match expression { @@ -563,23 +541,24 @@ impl Compiler { } self.emit(Instruction::CallFunction { count: count }); } - ast::Expression::BoolOp { a: _, op: _, b: _ } => { - let not_label = self.new_label(); - let end_label = self.new_label(); - self.compile_test(expression, not_label); - // Load const True - self.emit(Instruction::LoadConst { - value: bytecode::Constant::Boolean { value: true }, - }); - self.emit(Instruction::Jump { target: end_label }); - - self.set_label(not_label); - // Load const False - self.emit(Instruction::LoadConst { - value: bytecode::Constant::Boolean { value: false }, - }); - self.set_label(end_label); - } + ast::Expression::BoolOp { a, op, b } => match op { + ast::BooleanOperator::And => { + let false_label = self.new_label(); + self.compile_expression(a); + self.emit(Instruction::JumpIfFalseOrPop { + target: false_label, + }); + self.compile_expression(b); + self.set_label(false_label); + } + ast::BooleanOperator::Or => { + let true_label = self.new_label(); + self.compile_expression(a); + self.emit(Instruction::JumpIfOrPop { target: true_label }); + self.compile_expression(b); + self.set_label(true_label); + } + }, ast::Expression::Binop { a, op, b } => { self.compile_expression(&*a); self.compile_expression(&*b); diff --git a/vm/src/vm.rs b/vm/src/vm.rs index 22e62fd0e0..8a99f6cd94 100644 --- a/vm/src/vm.rs +++ b/vm/src/vm.rs @@ -773,28 +773,22 @@ impl VirtualMachine { } bytecode::Instruction::JumpIf { target } => { let obj = self.pop_value(); - match objbool::boolval(self, obj) { - Ok(value) => { - if value { - self.jump(target); - } - None - } - Err(value) => Some(Err(value)), - } + self.jump_if(target, obj, true, false) } bytecode::Instruction::JumpIfFalse { target } => { let obj = self.pop_value(); - match objbool::boolval(self, obj) { - Ok(value) => { - if !value { - self.jump(target); - } - None - } - Err(value) => Some(Err(value)), - } + self.jump_if(target, obj, false, false) + } + + bytecode::Instruction::JumpIfOrPop { target } => { + let obj = self.last_value(); + self.jump_if(target, obj, true, true) + } + + bytecode::Instruction::JumpIfFalseOrPop { target } => { + let obj = self.last_value(); + self.jump_if(target, obj, false, true) } bytecode::Instruction::Raise { argc } => { @@ -879,6 +873,26 @@ impl VirtualMachine { } } + fn jump_if( + &mut self, + label: &bytecode::Label, + obj: PyObjectRef, + jump_on: bool, + or_pop: bool, + ) -> Option { + match objbool::boolval(self, obj) { + Ok(value) => { + if value == jump_on { + self.jump(label); + } else if or_pop { + self.pop_value(); + } + None + } + Err(value) => Some(Err(value)), + } + } + fn jump(&mut self, label: &bytecode::Label) { let current_frame = self.current_frame(); let target_pc = current_frame.code.label_map[label]; From de0dde21f0396f2e5ceb3962e1f18ec74896ffbd Mon Sep 17 00:00:00 2001 From: Adam Kelly Date: Sun, 26 Aug 2018 08:36:13 +0100 Subject: [PATCH 6/6] Don't create extra JumpIfOrPop instructions. As discussed: https://github.com/RustPython/RustPython/pull/82#discussion_r212809238 --- vm/src/bytecode.rs | 6 ------ vm/src/compile.rs | 8 ++++++-- vm/src/vm.rs | 50 +++++++++++++++++----------------------------- 3 files changed, 24 insertions(+), 40 deletions(-) diff --git a/vm/src/bytecode.rs b/vm/src/bytecode.rs index dccf7f6a16..2f7b6853dd 100644 --- a/vm/src/bytecode.rs +++ b/vm/src/bytecode.rs @@ -83,15 +83,9 @@ pub enum Instruction { JumpIf { target: Label, }, - JumpIfOrPop { - target: Label, - }, JumpIfFalse { target: Label, }, - JumpIfFalseOrPop { - target: Label, - }, MakeFunction, CallFunction { count: usize, diff --git a/vm/src/compile.rs b/vm/src/compile.rs index d4a1848d8a..a95a8d9bd0 100644 --- a/vm/src/compile.rs +++ b/vm/src/compile.rs @@ -545,16 +545,20 @@ impl Compiler { ast::BooleanOperator::And => { let false_label = self.new_label(); self.compile_expression(a); - self.emit(Instruction::JumpIfFalseOrPop { + self.emit(Instruction::Duplicate); + self.emit(Instruction::JumpIfFalse { target: false_label, }); + self.emit(Instruction::Pop); self.compile_expression(b); self.set_label(false_label); } ast::BooleanOperator::Or => { let true_label = self.new_label(); self.compile_expression(a); - self.emit(Instruction::JumpIfOrPop { target: true_label }); + self.emit(Instruction::Duplicate); + self.emit(Instruction::JumpIf { target: true_label }); + self.emit(Instruction::Pop); self.compile_expression(b); self.set_label(true_label); } diff --git a/vm/src/vm.rs b/vm/src/vm.rs index 8a99f6cd94..22e62fd0e0 100644 --- a/vm/src/vm.rs +++ b/vm/src/vm.rs @@ -773,22 +773,28 @@ impl VirtualMachine { } bytecode::Instruction::JumpIf { target } => { let obj = self.pop_value(); - self.jump_if(target, obj, true, false) + match objbool::boolval(self, obj) { + Ok(value) => { + if value { + self.jump(target); + } + None + } + Err(value) => Some(Err(value)), + } } bytecode::Instruction::JumpIfFalse { target } => { let obj = self.pop_value(); - self.jump_if(target, obj, false, false) - } - - bytecode::Instruction::JumpIfOrPop { target } => { - let obj = self.last_value(); - self.jump_if(target, obj, true, true) - } - - bytecode::Instruction::JumpIfFalseOrPop { target } => { - let obj = self.last_value(); - self.jump_if(target, obj, false, true) + match objbool::boolval(self, obj) { + Ok(value) => { + if !value { + self.jump(target); + } + None + } + Err(value) => Some(Err(value)), + } } bytecode::Instruction::Raise { argc } => { @@ -873,26 +879,6 @@ impl VirtualMachine { } } - fn jump_if( - &mut self, - label: &bytecode::Label, - obj: PyObjectRef, - jump_on: bool, - or_pop: bool, - ) -> Option { - match objbool::boolval(self, obj) { - Ok(value) => { - if value == jump_on { - self.jump(label); - } else if or_pop { - self.pop_value(); - } - None - } - Err(value) => Some(Err(value)), - } - } - fn jump(&mut self, label: &bytecode::Label) { let current_frame = self.current_frame(); let target_pc = current_frame.code.label_map[label];