From e9b0454f1cc0263b5cf557b32d6d1cf86a08e4e8 Mon Sep 17 00:00:00 2001 From: Aaron Date: Mon, 25 Feb 2019 00:12:15 -0800 Subject: [PATCH 1/7] fix invalid break continue return yield --- vm/src/bytecode.rs | 3 +++ vm/src/compile.rs | 44 +++++++++++++++++++++++++++++--------------- vm/src/error.rs | 8 ++++++-- 3 files changed, 38 insertions(+), 17 deletions(-) diff --git a/vm/src/bytecode.rs b/vm/src/bytecode.rs index 1e0b85f133..8e3a26b6f4 100644 --- a/vm/src/bytecode.rs +++ b/vm/src/bytecode.rs @@ -26,6 +26,7 @@ pub struct CodeObject { pub first_line_number: usize, pub obj_name: String, // Name of the object that created this code object pub is_generator: bool, + pub is_function_obj: bool } bitflags! { @@ -252,6 +253,7 @@ impl CodeObject { source_path: String, first_line_number: usize, obj_name: String, + is_function_obj: bool ) -> CodeObject { CodeObject { instructions: Vec::new(), @@ -265,6 +267,7 @@ impl CodeObject { first_line_number, obj_name, is_generator: false, + is_function_obj } } diff --git a/vm/src/compile.rs b/vm/src/compile.rs index 866b0dad74..0c6705ceba 100644 --- a/vm/src/compile.rs +++ b/vm/src/compile.rs @@ -16,7 +16,7 @@ struct Compiler { nxt_label: usize, source_path: Option, current_source_location: ast::Location, - in_loop: bool, + loop_depth: usize, } /// Compile a given sourcecode into a bytecode object. @@ -74,7 +74,7 @@ impl Compiler { nxt_label: 0, source_path: None, current_source_location: ast::Location::default(), - in_loop: false, + loop_depth: 0, } } @@ -88,6 +88,7 @@ impl Compiler { self.source_path.clone().unwrap(), line_number, obj_name, + false )); } @@ -232,9 +233,9 @@ impl Compiler { self.compile_test(test, None, Some(else_label), EvalContext::Statement)?; - self.in_loop = true; + self.loop_depth += 1; self.compile_statements(body)?; - self.in_loop = false; + self.loop_depth -= 1; self.emit(Instruction::Jump { target: start_label, }); @@ -295,10 +296,9 @@ impl Compiler { // Start of loop iteration, set targets: self.compile_store(target)?; - // Body of loop: - self.in_loop = true; + self.loop_depth += 1; self.compile_statements(body)?; - self.in_loop = false; + self.loop_depth -= 1; self.emit(Instruction::Jump { target: start_label, @@ -431,6 +431,9 @@ impl Compiler { decorator_list, } => { // Create bytecode for this function: + // remember to restore self.loop_depth to the original after the function is compiled + let original_loop_depth = self.loop_depth; + self.loop_depth = 0; let flags = self.enter_function(name, args)?; self.compile_statements(body)?; @@ -458,6 +461,7 @@ impl Compiler { self.emit(Instruction::StoreName { name: name.to_string(), }); + self.loop_depth = original_loop_depth; } ast::Statement::ClassDef { name, @@ -477,6 +481,7 @@ impl Compiler { self.source_path.clone().unwrap(), line_number, name.clone(), + false )); self.emit(Instruction::LoadName { name: String::from("__locals__"), @@ -572,18 +577,21 @@ impl Compiler { self.set_label(end_label); } ast::Statement::Break => { - if !self.in_loop { + if self.loop_depth == 0 { return Err(CompileError::InvalidBreak); } self.emit(Instruction::Break); } ast::Statement::Continue => { - if !self.in_loop { + if self.loop_depth == 0 { return Err(CompileError::InvalidContinue); } self.emit(Instruction::Continue); } ast::Statement::Return { value } => { + if !self.current_code_object().is_function_obj { + return Err(CompileError::InvalidReturn); + } match value { Some(e) => { let size = e.len(); @@ -663,7 +671,7 @@ impl Compiler { name: &str, args: &ast::Parameters, ) -> Result { - self.in_loop = false; + let have_kwargs = !args.defaults.is_empty(); if have_kwargs { // Construct a tuple: @@ -686,6 +694,7 @@ impl Compiler { self.source_path.clone().unwrap(), line_number, name.to_string(), + true )); let mut flags = bytecode::FunctionOpArg::empty(); @@ -971,6 +980,9 @@ impl Compiler { self.emit(Instruction::BuildSlice { size }); } ast::Expression::Yield { value } => { + if !self.current_code_object().is_function_obj { + return Err(CompileError::InvalidYield); + } self.mark_generator(); match value { Some(expression) => self.compile_expression(expression)?, @@ -1021,6 +1033,7 @@ impl Compiler { } ast::Expression::Lambda { args, body } => { let name = "".to_string(); + // no need to worry about the self.loop_depth because there are no loops in lambda expressions let flags = self.enter_function(&name, args)?; self.compile_expression(body)?; self.emit(Instruction::ReturnValue); @@ -1199,6 +1212,7 @@ impl Compiler { self.source_path.clone().unwrap(), line_number, name.clone(), + false )); // Create empty object of proper type: @@ -1362,10 +1376,11 @@ impl Compiler { // Low level helper functions: fn emit(&mut self, instruction: Instruction) { - self.current_code_object().instructions.push(instruction); - // TODO: insert source filename let location = self.current_source_location.clone(); - self.current_code_object().locations.push(location); + let mut cur_code_obj = self.current_code_object(); + cur_code_obj.instructions.push(instruction); + cur_code_obj.locations.push(location); + // TODO: insert source filename } fn current_code_object(&mut self) -> &mut CodeObject { @@ -1374,9 +1389,8 @@ impl Compiler { // Generate a new label fn new_label(&mut self) -> Label { - let l = self.nxt_label; self.nxt_label += 1; - l + self.nxt_label - 1 } // Assign current position the given label diff --git a/vm/src/error.rs b/vm/src/error.rs index 70c8ff71ec..8f152a6ca3 100644 --- a/vm/src/error.rs +++ b/vm/src/error.rs @@ -19,6 +19,8 @@ pub enum CompileError { InvalidBreak, /// Continue statement outside of loop. InvalidContinue, + InvalidReturn, + InvalidYield } impl fmt::Display for CompileError { @@ -29,8 +31,10 @@ impl fmt::Display for CompileError { CompileError::ExpectExpr => write!(f, "Expecting expression, got statement"), CompileError::Parse(err) => write!(f, "{}", err), CompileError::StarArgs => write!(f, "Two starred expressions in assignment"), - CompileError::InvalidBreak => write!(f, "break outside loop"), - CompileError::InvalidContinue => write!(f, "continue outside loop"), + CompileError::InvalidBreak => write!(f, "'break' outside loop"), + CompileError::InvalidContinue => write!(f, "'continue' outside loop"), + CompileError::InvalidReturn => write!(f, "'return' outside function"), + CompileError::InvalidYield => write!(f, "'yield' outside function") } } } From 7610d448653ab6036c11bc9995fbda6d9c5c4d66 Mon Sep 17 00:00:00 2001 From: Aaron Date: Mon, 25 Feb 2019 00:26:06 -0800 Subject: [PATCH 2/7] cargo fmt --- vm/src/bytecode.rs | 6 +++--- vm/src/compile.rs | 9 ++++----- vm/src/error.rs | 4 ++-- 3 files changed, 9 insertions(+), 10 deletions(-) diff --git a/vm/src/bytecode.rs b/vm/src/bytecode.rs index 8e3a26b6f4..dac290453d 100644 --- a/vm/src/bytecode.rs +++ b/vm/src/bytecode.rs @@ -26,7 +26,7 @@ pub struct CodeObject { pub first_line_number: usize, pub obj_name: String, // Name of the object that created this code object pub is_generator: bool, - pub is_function_obj: bool + pub is_function_obj: bool, } bitflags! { @@ -253,7 +253,7 @@ impl CodeObject { source_path: String, first_line_number: usize, obj_name: String, - is_function_obj: bool + is_function_obj: bool, ) -> CodeObject { CodeObject { instructions: Vec::new(), @@ -267,7 +267,7 @@ impl CodeObject { first_line_number, obj_name, is_generator: false, - is_function_obj + is_function_obj, } } diff --git a/vm/src/compile.rs b/vm/src/compile.rs index 0c6705ceba..bf9b21b515 100644 --- a/vm/src/compile.rs +++ b/vm/src/compile.rs @@ -88,7 +88,7 @@ impl Compiler { self.source_path.clone().unwrap(), line_number, obj_name, - false + false, )); } @@ -481,7 +481,7 @@ impl Compiler { self.source_path.clone().unwrap(), line_number, name.clone(), - false + false, )); self.emit(Instruction::LoadName { name: String::from("__locals__"), @@ -671,7 +671,6 @@ impl Compiler { name: &str, args: &ast::Parameters, ) -> Result { - let have_kwargs = !args.defaults.is_empty(); if have_kwargs { // Construct a tuple: @@ -694,7 +693,7 @@ impl Compiler { self.source_path.clone().unwrap(), line_number, name.to_string(), - true + true, )); let mut flags = bytecode::FunctionOpArg::empty(); @@ -1212,7 +1211,7 @@ impl Compiler { self.source_path.clone().unwrap(), line_number, name.clone(), - false + false, )); // Create empty object of proper type: diff --git a/vm/src/error.rs b/vm/src/error.rs index 8f152a6ca3..06eb387e2e 100644 --- a/vm/src/error.rs +++ b/vm/src/error.rs @@ -20,7 +20,7 @@ pub enum CompileError { /// Continue statement outside of loop. InvalidContinue, InvalidReturn, - InvalidYield + InvalidYield, } impl fmt::Display for CompileError { @@ -34,7 +34,7 @@ impl fmt::Display for CompileError { CompileError::InvalidBreak => write!(f, "'break' outside loop"), CompileError::InvalidContinue => write!(f, "'continue' outside loop"), CompileError::InvalidReturn => write!(f, "'return' outside function"), - CompileError::InvalidYield => write!(f, "'yield' outside function") + CompileError::InvalidYield => write!(f, "'yield' outside function"), } } } From 2f0f1b091312d033c5ef233eb030fd32cdbb4998 Mon Sep 17 00:00:00 2001 From: Aaron Date: Mon, 25 Feb 2019 09:08:36 -0800 Subject: [PATCH 3/7] use boolean flag --- vm/src/compile.rs | 27 +++++++++++++++------------ 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/vm/src/compile.rs b/vm/src/compile.rs index bf9b21b515..c22e00dcfe 100644 --- a/vm/src/compile.rs +++ b/vm/src/compile.rs @@ -16,7 +16,7 @@ struct Compiler { nxt_label: usize, source_path: Option, current_source_location: ast::Location, - loop_depth: usize, + in_loop: bool, } /// Compile a given sourcecode into a bytecode object. @@ -74,7 +74,7 @@ impl Compiler { nxt_label: 0, source_path: None, current_source_location: ast::Location::default(), - loop_depth: 0, + in_loop: false, } } @@ -233,9 +233,10 @@ impl Compiler { self.compile_test(test, None, Some(else_label), EvalContext::Statement)?; - self.loop_depth += 1; + let was_in_loop = self.in_loop; + self.in_loop = true; self.compile_statements(body)?; - self.loop_depth -= 1; + self.in_loop = was_in_loop; self.emit(Instruction::Jump { target: start_label, }); @@ -296,9 +297,10 @@ impl Compiler { // Start of loop iteration, set targets: self.compile_store(target)?; - self.loop_depth += 1; + let was_in_loop = self.in_loop; + self.in_loop = true; self.compile_statements(body)?; - self.loop_depth -= 1; + self.in_loop = was_in_loop; self.emit(Instruction::Jump { target: start_label, @@ -431,9 +433,9 @@ impl Compiler { decorator_list, } => { // Create bytecode for this function: - // remember to restore self.loop_depth to the original after the function is compiled - let original_loop_depth = self.loop_depth; - self.loop_depth = 0; + // remember to restore self.in_loop to the original after the function is compiled + let was_in_loop = self.in_loop; + self.in_loop = false; let flags = self.enter_function(name, args)?; self.compile_statements(body)?; @@ -461,7 +463,7 @@ impl Compiler { self.emit(Instruction::StoreName { name: name.to_string(), }); - self.loop_depth = original_loop_depth; + self.in_loop = was_in_loop; } ast::Statement::ClassDef { name, @@ -577,13 +579,13 @@ impl Compiler { self.set_label(end_label); } ast::Statement::Break => { - if self.loop_depth == 0 { + if !self.in_loop { return Err(CompileError::InvalidBreak); } self.emit(Instruction::Break); } ast::Statement::Continue => { - if self.loop_depth == 0 { + if !self.in_loop { return Err(CompileError::InvalidContinue); } self.emit(Instruction::Continue); @@ -1419,6 +1421,7 @@ mod tests { use crate::bytecode::Constant::*; use crate::bytecode::Instruction::*; use rustpython_parser::parser; + fn compile_exec(source: &str) -> CodeObject { let mut compiler = Compiler::new(); compiler.source_path = Some("source_path".to_string()); From 4a7d2cb01072ff377e06f88971786fcf4705e06f Mon Sep 17 00:00:00 2001 From: Aaron Date: Mon, 25 Feb 2019 09:42:02 -0800 Subject: [PATCH 4/7] reset in_loop in class definition block --- vm/src/compile.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/vm/src/compile.rs b/vm/src/compile.rs index c22e00dcfe..94f85a990d 100644 --- a/vm/src/compile.rs +++ b/vm/src/compile.rs @@ -472,6 +472,8 @@ impl Compiler { keywords, decorator_list, } => { + let was_in_loop = self.in_loop; + self.in_loop = false; self.prepare_decorators(decorator_list)?; self.emit(Instruction::LoadBuildClass); let line_number = self.get_source_line_number(); @@ -553,6 +555,7 @@ impl Compiler { self.emit(Instruction::StoreName { name: name.to_string(), }); + self.in_loop = was_in_loop; } ast::Statement::Assert { test, msg } => { // TODO: if some flag, ignore all assert statements! From f965c49704a7d53999a66fb783bde476c08252a1 Mon Sep 17 00:00:00 2001 From: Aaron Date: Mon, 25 Feb 2019 20:31:03 -0800 Subject: [PATCH 5/7] compiler flag in_function_def --- vm/src/bytecode.rs | 3 --- vm/src/compile.rs | 57 +++++++++++++++++++++++----------------------- 2 files changed, 29 insertions(+), 31 deletions(-) diff --git a/vm/src/bytecode.rs b/vm/src/bytecode.rs index dac290453d..1e0b85f133 100644 --- a/vm/src/bytecode.rs +++ b/vm/src/bytecode.rs @@ -26,7 +26,6 @@ pub struct CodeObject { pub first_line_number: usize, pub obj_name: String, // Name of the object that created this code object pub is_generator: bool, - pub is_function_obj: bool, } bitflags! { @@ -253,7 +252,6 @@ impl CodeObject { source_path: String, first_line_number: usize, obj_name: String, - is_function_obj: bool, ) -> CodeObject { CodeObject { instructions: Vec::new(), @@ -267,7 +265,6 @@ impl CodeObject { first_line_number, obj_name, is_generator: false, - is_function_obj, } } diff --git a/vm/src/compile.rs b/vm/src/compile.rs index 94f85a990d..a5dc050d57 100644 --- a/vm/src/compile.rs +++ b/vm/src/compile.rs @@ -17,6 +17,7 @@ struct Compiler { source_path: Option, current_source_location: ast::Location, in_loop: bool, + in_function_def: bool, } /// Compile a given sourcecode into a bytecode object. @@ -75,6 +76,7 @@ impl Compiler { source_path: None, current_source_location: ast::Location::default(), in_loop: false, + in_function_def: false, } } @@ -88,7 +90,6 @@ impl Compiler { self.source_path.clone().unwrap(), line_number, obj_name, - false, )); } @@ -160,30 +161,30 @@ impl Compiler { symbol, alias, } in import_parts - { - match symbol { - Some(name) if name == "*" => { - self.emit(Instruction::ImportStar { - name: module.clone(), - }); - } - _ => { - self.emit(Instruction::Import { - name: module.clone(), - symbol: symbol.clone(), - }); - self.emit(Instruction::StoreName { - name: match alias { - Some(alias) => alias.clone(), - None => match symbol { - Some(symbol) => symbol.clone(), - None => module.clone(), + { + match symbol { + Some(name) if name == "*" => { + self.emit(Instruction::ImportStar { + name: module.clone(), + }); + } + _ => { + self.emit(Instruction::Import { + name: module.clone(), + symbol: symbol.clone(), + }); + self.emit(Instruction::StoreName { + name: match alias { + Some(alias) => alias.clone(), + None => match symbol { + Some(symbol) => symbol.clone(), + None => module.clone(), + }, }, - }, - }); + }); + } } } - } } ast::Statement::Expression { expression } => { self.compile_expression(expression)?; @@ -435,7 +436,9 @@ impl Compiler { // Create bytecode for this function: // remember to restore self.in_loop to the original after the function is compiled let was_in_loop = self.in_loop; + let was_in_function_def = self.in_function_def; self.in_loop = false; + self.in_function_def = true; let flags = self.enter_function(name, args)?; self.compile_statements(body)?; @@ -464,6 +467,7 @@ impl Compiler { name: name.to_string(), }); self.in_loop = was_in_loop; + self.in_function_def = was_in_function_def; } ast::Statement::ClassDef { name, @@ -485,7 +489,6 @@ impl Compiler { self.source_path.clone().unwrap(), line_number, name.clone(), - false, )); self.emit(Instruction::LoadName { name: String::from("__locals__"), @@ -594,7 +597,7 @@ impl Compiler { self.emit(Instruction::Continue); } ast::Statement::Return { value } => { - if !self.current_code_object().is_function_obj { + if !self.in_function_def { return Err(CompileError::InvalidReturn); } match value { @@ -698,7 +701,6 @@ impl Compiler { self.source_path.clone().unwrap(), line_number, name.to_string(), - true, )); let mut flags = bytecode::FunctionOpArg::empty(); @@ -984,7 +986,7 @@ impl Compiler { self.emit(Instruction::BuildSlice { size }); } ast::Expression::Yield { value } => { - if !self.current_code_object().is_function_obj { + if !self.in_function_def { return Err(CompileError::InvalidYield); } self.mark_generator(); @@ -1204,7 +1206,7 @@ impl Compiler { ast::ComprehensionKind::Set { .. } => "", ast::ComprehensionKind::Dict { .. } => "", } - .to_string(); + .to_string(); let line_number = self.get_source_line_number(); // Create magnificent function : @@ -1216,7 +1218,6 @@ impl Compiler { self.source_path.clone().unwrap(), line_number, name.clone(), - false, )); // Create empty object of proper type: From f037244641b80a840c731a0d3e1c11795ec9c352 Mon Sep 17 00:00:00 2001 From: Aaron Date: Mon, 25 Feb 2019 20:33:03 -0800 Subject: [PATCH 6/7] fmt --- vm/src/compile.rs | 44 ++++++++++++++++++++++---------------------- 1 file changed, 22 insertions(+), 22 deletions(-) diff --git a/vm/src/compile.rs b/vm/src/compile.rs index a5dc050d57..eda9c6ec45 100644 --- a/vm/src/compile.rs +++ b/vm/src/compile.rs @@ -161,30 +161,30 @@ impl Compiler { symbol, alias, } in import_parts - { - match symbol { - Some(name) if name == "*" => { - self.emit(Instruction::ImportStar { - name: module.clone(), - }); - } - _ => { - self.emit(Instruction::Import { - name: module.clone(), - symbol: symbol.clone(), - }); - self.emit(Instruction::StoreName { - name: match alias { - Some(alias) => alias.clone(), - None => match symbol { - Some(symbol) => symbol.clone(), - None => module.clone(), - }, + { + match symbol { + Some(name) if name == "*" => { + self.emit(Instruction::ImportStar { + name: module.clone(), + }); + } + _ => { + self.emit(Instruction::Import { + name: module.clone(), + symbol: symbol.clone(), + }); + self.emit(Instruction::StoreName { + name: match alias { + Some(alias) => alias.clone(), + None => match symbol { + Some(symbol) => symbol.clone(), + None => module.clone(), }, - }); - } + }, + }); } } + } } ast::Statement::Expression { expression } => { self.compile_expression(expression)?; @@ -1206,7 +1206,7 @@ impl Compiler { ast::ComprehensionKind::Set { .. } => "", ast::ComprehensionKind::Dict { .. } => "", } - .to_string(); + .to_string(); let line_number = self.get_source_line_number(); // Create magnificent function : From fee6a47d88670a00e3e7f0ead287d7ea49d6f17d Mon Sep 17 00:00:00 2001 From: Aaron Date: Mon, 25 Feb 2019 20:35:43 -0800 Subject: [PATCH 7/7] rever new_label() --- vm/src/compile.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/vm/src/compile.rs b/vm/src/compile.rs index eda9c6ec45..9c455581e4 100644 --- a/vm/src/compile.rs +++ b/vm/src/compile.rs @@ -1394,8 +1394,9 @@ impl Compiler { // Generate a new label fn new_label(&mut self) -> Label { + let l = self.nxt_label; self.nxt_label += 1; - self.nxt_label - 1 + l } // Assign current position the given label