Add Positional argument error to LexicalError

vazrupe · vazrupe · commit 0ab46fd2bdef · 2019-09-13T01:46:31.000+09:00
Fixes #1046
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/parser/src/ast.rs b/parser/src/ast.rs
@@ -321,6 +321,12 @@ pub struct Comprehension {
     pub is_async: bool,
 }
 
+#[derive(Debug, PartialEq)]
+pub struct ArgumentList {
+    pub args: Vec<Expression>,
+    pub keywords: Vec<Keyword>,
+}
+
 #[derive(Debug, PartialEq)]
 pub struct Keyword {
     pub name: Option<String>,
diff --git a/parser/src/error.rs b/parser/src/error.rs
@@ -20,6 +20,7 @@ pub enum LexicalErrorType {
     StringError,
     UnicodeError,
     NestingError,
+    PositionalArgumentError,
     UnrecognizedToken { tok: char },
     FStringError(FStringErrorType),
     OtherError(String),
@@ -32,6 +33,9 @@ impl fmt::Display for LexicalErrorType {
             LexicalErrorType::FStringError(error) => write!(f, "Got error in f-string: {}", error),
             LexicalErrorType::UnicodeError => write!(f, "Got unexpected unicode"),
             LexicalErrorType::NestingError => write!(f, "Got unexpected nesting"),
+            LexicalErrorType::PositionalArgumentError => {
+                write!(f, "positional argument follows keyword argument")
+            }
             LexicalErrorType::UnrecognizedToken { tok } => {
                 write!(f, "Got unexpected token {}", tok)
             }
@@ -40,6 +44,12 @@ impl fmt::Display for LexicalErrorType {
     }
 }
 
+impl From<LexicalError> for LalrpopError<Location, Tok, LexicalError> {
+    fn from(err: LexicalError) -> Self {
+        lalrpop_util::ParseError::User { error: err }
+    }
+}
+
 // TODO: consolidate these with ParseError
 #[derive(Debug, PartialEq)]
 pub struct FStringError {
diff --git a/parser/src/function.rs b/parser/src/function.rs
@@ -0,0 +1,36 @@
+use crate::ast;
+use crate::error::{LexicalError, LexicalErrorType};
+
+type FunctionArgument = (Option<Option<String>>, ast::Expression);
+
+pub fn parse_args(func_args: Vec<FunctionArgument>) -> Result<ast::ArgumentList, LexicalError> {
+    let mut args = vec![];
+    let mut keywords = vec![];
+    for (name, value) in func_args {
+        match name {
+            Some(n) => {
+                keywords.push(ast::Keyword { name: n, value });
+            }
+            None => {
+                // Allow starred args after keyword arguments.
+                if !keywords.is_empty() && !is_starred(&value) {
+                    return Err(LexicalError {
+                        error: LexicalErrorType::PositionalArgumentError,
+                        location: value.location.clone(),
+                    });
+                }
+
+                args.push(value);
+            }
+        }
+    }
+    Ok(ast::ArgumentList { args, keywords })
+}
+
+fn is_starred(exp: &ast::Expression) -> bool {
+    if let ast::ExpressionType::Starred { .. } = exp.node {
+        true
+    } else {
+        false
+    }
+}
diff --git a/parser/src/lib.rs b/parser/src/lib.rs
@@ -8,6 +8,7 @@ use lalrpop_util::lalrpop_mod;
 pub mod ast;
 pub mod error;
 mod fstring;
+mod function;
 pub mod lexer;
 pub mod location;
 pub mod parser;
diff --git a/parser/src/python.lalrpop b/parser/src/python.lalrpop
@@ -7,6 +7,7 @@ use std::iter::FromIterator;
 
 use crate::ast;
 use crate::fstring::parse_located_fstring;
+use crate::function::parse_args;
 use crate::error::LexicalError;
 use crate::lexer;
 use crate::location;
@@ -580,7 +581,7 @@ KwargParameter<ArgType>: Option<ast::Parameter> = {
 ClassDef: ast::Statement = {
     <decorator_list:Decorator*> <location:@L> "class" <name:Identifier> <a:("(" ArgumentList ")")?> ":" <body:Suite> => {
         let (bases, keywords) = match a {
-            Some((_, args, _)) => args,
+            Some((_, arg, _)) => (arg.args, arg.keywords),
             None => (vec![], vec![]),
         };
         ast::Statement {
@@ -616,14 +617,13 @@ Path: ast::Expression = {
 Decorator: ast::Expression = {
     "@" <p:Path> <a: (@L "(" ArgumentList ")")?> "\n" => {
         match a {
-            Some((location, _, args, _)) => {
-                let (args, keywords) = args;
+            Some((location, _, arg, _)) => {
                 ast::Expression {
                     location,
                     node: ast::ExpressionType::Call {
                         function: Box::new(p),
-                        args,
-                        keywords,
+                        args: arg.args,
+                        keywords: arg.keywords,
                     }
                 }
             },
@@ -633,7 +633,7 @@ Decorator: ast::Expression = {
 };
 
 YieldExpr: ast::Expression = {
-    <location:@L> "yield" <value:TestList?> => ast::Expression { 
+    <location:@L> "yield" <value:TestList?> => ast::Expression {
         location,
         node: ast::ExpressionType::Yield { value: value.map(Box::new) }
     },
@@ -847,10 +847,9 @@ AtomExpr: ast::Expression = {
 AtomExpr2: ast::Expression = {
     Atom,
     <f:AtomExpr2> <location:@L> "(" <a:ArgumentList> ")" => {
-        let (args, keywords) = a;
         ast::Expression {
             location,
-            node: ast::ExpressionType::Call { function: Box::new(f), args, keywords }
+            node: ast::ExpressionType::Call { function: Box::new(f), args: a.args, keywords: a.keywords }
         }
     },
     <e:AtomExpr2> <location:@L> "[" <s:SubscriptList> "]" => ast::Expression {
@@ -1060,31 +1059,10 @@ SingleForComprehension: ast::Comprehension = {
 ExpressionNoCond: ast::Expression = OrTest;
 ComprehensionIf: ast::Expression = "if" <c:ExpressionNoCond> => c;
 
-ArgumentList: (Vec<ast::Expression>, Vec<ast::Keyword>) = {
-    <e: Comma<FunctionArgument>> => {
-        let mut args = vec![];
-        let mut keywords = vec![];
-        for (name, value) in e {
-            match name {
-                Some(n) => {
-                    keywords.push(ast::Keyword { name: n, value: value });
-                },
-                None => {
-                    // Allow starred args after keyword arguments.
-                    let is_starred = if let ast::ExpressionType::Starred { .. } = &value.node {
-                        true
-                    } else {
-                        false
-                    };
-
-                    if keywords.len() > 0 && !is_starred {
-                        panic!("positional argument follows keyword argument {:?}", keywords);
-                    };
-                    args.push(value);
-                },
-            }
-        }
-        (args, keywords)
+ArgumentList: ast::ArgumentList = {
+    <e: Comma<FunctionArgument>> =>? {
+        let arg_list = parse_args(e)?;
+        Ok(arg_list)
     }
 };
 
diff --git a/tests/snippets/function.py b/tests/snippets/function.py
@@ -1,3 +1,6 @@
+from testutils import assertRaises
+
+
 __name__ = "function"
 
 
@@ -88,3 +91,7 @@ def f8() -> int:
     return 10
 
 assert f8() == 10
+
+
+with assertRaises(SyntaxError):
+    exec('print(keyword=10, 20)')
