Merge pull request #1922 from youknowone/unpack-ex

Fix unpack_ex error messages

compiler/src/compile.rs

@@ -1374,7 +1374,7 @@ impl<O: OutputStream> Compiler<O> {
                 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 {
@@ -1399,7 +1399,14 @@ impl<O: OutputStream> Compiler<O> {
                     }
                 }
             }
-            _ => 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(())
@@ -1782,11 +1789,7 @@ impl<O: OutputStream> Compiler<O> {
                 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();
@@ -2031,21 +2034,33 @@ impl<O: OutputStream> Compiler<O> {
             }
         }
 
+        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(),
                 });

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

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))
     }
 }
 

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()
             )))

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)