From 09d849ab0a07eb6af41d9eea4297995f11824b28 Mon Sep 17 00:00:00 2001 From: Jeong YunWon Date: Wed, 13 May 2020 01:42:18 +0900 Subject: [PATCH 1/4] Distinguish MultipleStarArgs and InvalidStarArgs compile errors --- compiler/src/compile.rs | 8 ++------ compiler/src/error.rs | 9 +++++++-- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/compiler/src/compile.rs b/compiler/src/compile.rs index 42c78b2897..df543eb7cd 100644 --- a/compiler/src/compile.rs +++ b/compiler/src/compile.rs @@ -1374,7 +1374,7 @@ impl Compiler { for (i, element) in elements.iter().enumerate() { if let ast::ExpressionType::Starred { .. } = &element.node { if seen_star { - return Err(self.error(CompileErrorType::StarArgs)); + return Err(self.error(CompileErrorType::MultipleStarArgs)); } else { seen_star = true; self.emit(Instruction::UnpackEx { @@ -1782,11 +1782,7 @@ impl Compiler { self.compile_comprehension(kind, generators)?; } Starred { .. } => { - return Err( - self.error(CompileErrorType::SyntaxError(std::string::String::from( - "Invalid starred expression", - ))), - ); + return Err(self.error(CompileErrorType::InvalidStarExpr)); } IfExpression { test, body, orelse } => { let no_label = self.new_label(); diff --git a/compiler/src/error.rs b/compiler/src/error.rs index 85bc9f4625..38e5430095 100644 --- a/compiler/src/error.rs +++ b/compiler/src/error.rs @@ -47,7 +47,9 @@ pub enum CompileErrorType { Parse(ParseErrorType), SyntaxError(String), /// Multiple `*` detected - StarArgs, + MultipleStarArgs, + /// Misplaced `*` expression + InvalidStarExpr, /// Break statement outside of loop. InvalidBreak, /// Continue statement outside of loop. @@ -97,7 +99,10 @@ impl fmt::Display for CompileError { CompileErrorType::ExpectExpr => "Expecting expression, got statement".to_owned(), CompileErrorType::Parse(err) => err.to_string(), CompileErrorType::SyntaxError(err) => err.to_string(), - CompileErrorType::StarArgs => "Two starred expressions in assignment".to_owned(), + CompileErrorType::MultipleStarArgs => { + "two starred expressions in assignment".to_owned() + } + CompileErrorType::InvalidStarExpr => "can't use starred expression here".to_owned(), CompileErrorType::InvalidBreak => "'break' outside loop".to_owned(), CompileErrorType::InvalidContinue => "'continue' outside loop".to_owned(), CompileErrorType::InvalidReturn => "'return' outside function".to_owned(), From e4be376336981c43eb5d65ae76edee50d22e6302 Mon Sep 17 00:00:00 2001 From: Jeong YunWon Date: Wed, 13 May 2020 01:43:09 +0900 Subject: [PATCH 2/4] comprehension starred expression compatibility --- compiler/src/compile.rs | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/compiler/src/compile.rs b/compiler/src/compile.rs index df543eb7cd..4a8ee90256 100644 --- a/compiler/src/compile.rs +++ b/compiler/src/compile.rs @@ -2027,21 +2027,33 @@ impl Compiler { } } + let mut compile_element = |element| { + self.compile_expression(element).map_err(|e| { + if matches!(e.error, CompileErrorType::InvalidStarExpr) { + self.error(CompileErrorType::SyntaxError( + "iterable unpacking cannot be used in comprehension".to_owned(), + )) + } else { + e + } + }) + }; + match kind { ast::ComprehensionKind::GeneratorExpression { element } => { - self.compile_expression(element)?; + compile_element(element)?; self.mark_generator(); self.emit(Instruction::YieldValue); self.emit(Instruction::Pop); } ast::ComprehensionKind::List { element } => { - self.compile_expression(element)?; + compile_element(element)?; self.emit(Instruction::ListAppend { i: 1 + generators.len(), }); } ast::ComprehensionKind::Set { element } => { - self.compile_expression(element)?; + compile_element(element)?; self.emit(Instruction::SetAdd { i: 1 + generators.len(), }); From 7bce7baf4a1c4c61efe51370ce16698602cbf078 Mon Sep 17 00:00:00 2001 From: Jeong YunWon Date: Wed, 13 May 2020 01:46:37 +0900 Subject: [PATCH 3/4] Fix SyntaxError visualize order: message first location later --- compiler/src/error.rs | 2 +- parser/src/location.rs | 10 +++------- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/compiler/src/error.rs b/compiler/src/error.rs index 38e5430095..86635bc951 100644 --- a/compiler/src/error.rs +++ b/compiler/src/error.rs @@ -125,7 +125,7 @@ impl fmt::Display for CompileError { if self.location.column() > 0 { if let Some(line) = statement.lines().nth(self.location.row() - 1) { // visualize the error, when location and statement are provided - return write!(f, "\n{}\n{}", line, self.location.visualize(&error_desc)); + return write!(f, "{}", self.location.visualize(line, &error_desc)); } } } diff --git a/parser/src/location.rs b/parser/src/location.rs index 1006d80605..1f543e3e36 100644 --- a/parser/src/location.rs +++ b/parser/src/location.rs @@ -16,13 +16,9 @@ impl fmt::Display for Location { } impl Location { - pub fn visualize(&self, desc: &str) -> String { - format!( - "{}↑\n{}{}", - " ".repeat(self.column - 1), - " ".repeat(self.column - 1), - desc - ) + pub fn visualize(&self, line: &str, desc: &str) -> String { + // desc.to_owned() + format!("{}\n{}\n{}↑", desc, line, " ".repeat(self.column - 1)) } } From 9338dcdf45196494290787cf6b227fdf82d35dde Mon Sep 17 00:00:00 2001 From: Jeong YunWon Date: Wed, 13 May 2020 01:48:40 +0900 Subject: [PATCH 4/4] Fix unpack_ex related messages --- compiler/src/compile.rs | 9 ++++++++- vm/src/frame.rs | 7 +++++-- vm/src/obj/objiter.rs | 2 +- 3 files changed, 14 insertions(+), 4 deletions(-) diff --git a/compiler/src/compile.rs b/compiler/src/compile.rs index 4a8ee90256..53aa36267d 100644 --- a/compiler/src/compile.rs +++ b/compiler/src/compile.rs @@ -1399,7 +1399,14 @@ impl Compiler { } } } - _ => return Err(self.error(CompileErrorType::Assign(target.name()))), + _ => { + return Err(self.error(match target.node { + ast::ExpressionType::Starred { .. } => CompileErrorType::SyntaxError( + "starred assignment target must be in a list or tuple".to_owned(), + ), + _ => CompileErrorType::Assign(target.name()), + })) + } } Ok(()) diff --git a/vm/src/frame.rs b/vm/src/frame.rs index e522a8b4f3..0b4416fdb8 100644 --- a/vm/src/frame.rs +++ b/vm/src/frame.rs @@ -940,7 +940,9 @@ impl ExecutingFrame<'_> { if unpack { for obj in self.pop_multiple(size) { // Take all key-value pairs from the dict: - let dict: PyDictRef = obj.downcast().expect("Need a dictionary to build a map."); + let dict: PyDictRef = obj.downcast().map_err(|obj| { + vm.new_type_error(format!("'{}' object is not a mapping", obj.class().name)) + })?; for (key, value) in dict { if for_call { if map_obj.contains_key(&key, vm) { @@ -1014,6 +1016,7 @@ impl ExecutingFrame<'_> { let kwargs = if *has_kwargs { let kw_dict: PyDictRef = match self.pop_value().downcast() { Err(_) => { + // TODO: check collections.abc.Mapping return Err(vm.new_type_error("Kwargs must be a dict.".to_owned())); } Ok(x) => x, @@ -1137,7 +1140,7 @@ impl ExecutingFrame<'_> { let min_expected = before + after; if elements.len() < min_expected { Err(vm.new_value_error(format!( - "Not enough values to unpack (expected at least {}, got {}", + "not enough values to unpack (expected at least {}, got {})", min_expected, elements.len() ))) diff --git a/vm/src/obj/objiter.rs b/vm/src/obj/objiter.rs index e79ae0d4da..675df6f33a 100644 --- a/vm/src/obj/objiter.rs +++ b/vm/src/obj/objiter.rs @@ -26,7 +26,7 @@ pub fn get_iter(vm: &VirtualMachine, iter_target: &PyObjectRef) -> PyResult { vm.invoke(&method, vec![]) } else { vm.get_method_or_type_error(iter_target.clone(), "__getitem__", || { - format!("Cannot iterate over {}", iter_target.class().name) + format!("'{}' object is not iterable", iter_target.class().name) })?; Ok(PySequenceIterator::new_forward(iter_target.clone()) .into_ref(vm)