From 29a137bf0f8722fd449e18997ced1dc964c088ea Mon Sep 17 00:00:00 2001
From: Windel Bouwman <windel@windel.nl>
Date: Wed, 3 Apr 2019 22:24:19 +0200
Subject: [PATCH] Drop testlist2. Also add test cases for trailing comma in for
 statement and tuple addition example.

---
 parser/src/python.lalrpop | 182 ++++++++++++++------------------------
 tests/snippets/for.py     |  12 +++
 tests/snippets/tuple.py   |   4 +
 3 files changed, 81 insertions(+), 117 deletions(-)

diff --git a/parser/src/python.lalrpop b/parser/src/python.lalrpop
index 151e06de2..305f97c10 100644
--- a/parser/src/python.lalrpop
+++ b/parser/src/python.lalrpop
@@ -102,9 +102,7 @@ ExpressionStatement: ast::LocatedStatement = {
           }
         }
     },
-    <loc:@L> <expr:TestOrStarExprList> <op:AugAssign> <e2:TestList2> => {
-        // TODO: this works in most cases:
-        let rhs = e2.into_iter().next().unwrap();
+    <loc:@L> <expr:TestOrStarExprList> <op:AugAssign> <rhs:TestList> => {
         ast::LocatedStatement {
             location: loc,
             node: ast::Statement::AugAssign {
@@ -122,18 +120,12 @@ AssignSuffix: ast::Expression = {
 };
 
 TestOrStarExprList: ast::Expression = {
-    <e:TestOrStarExpr> <e2:("," TestOrStarExpr)*> <comma:","?> => {
-        let mut res = vec![e];
-        res.extend(e2.into_iter().map(|x| x.1));
-
-        // First build tuple from first item:
-        let expr = if (res.len() > 1) || comma.is_some() {
-            ast::Expression::Tuple { elements: res }
+    <elements:OneOrMore<TestOrStarExpr>> <comma:","?> => {
+        if elements.len() == 1 && comma.is_none() {
+            elements.into_iter().next().unwrap()
         } else {
-            res.into_iter().next().unwrap()
-        };
-
-        expr
+            ast::Expression::Tuple { elements }
+        }
     }
 };
 
@@ -301,15 +293,11 @@ NonlocalStatement: ast::LocatedStatement = {
 };
 
 AssertStatement: ast::LocatedStatement = {
-    <loc:@L> "assert" <t:Test> <m: ("," Test)?> => {
+    <loc:@L> "assert" <test:Test> <msg: ("," Test)?> => {
         ast::LocatedStatement {
           location: loc,
           node: ast::Statement::Assert {
-            test: t,
-            msg: match m {
-              Some(e) => Some(e.1),
-              None => None,
-            }
+            test, msg: msg.map(|e| e.1)
           }
         }
     },
@@ -326,7 +314,7 @@ CompoundStatement: ast::LocatedStatement = {
 };
 
 IfStatement: ast::LocatedStatement = {
-    <loc:@L> "if" <t:Test> ":" <s1:Suite> <s2:(@L "elif" Test ":" Suite)*> <s3:("else" ":" Suite)?> => {
+    <loc:@L> "if" <test:Test> ":" <s1:Suite> <s2:(@L "elif" Test ":" Suite)*> <s3:("else" ":" Suite)?> => {
         // Determine last else:
         let mut last = s3.map(|s| s.2);
 
@@ -341,17 +329,17 @@ IfStatement: ast::LocatedStatement = {
 
         ast::LocatedStatement {
             location: loc,
-            node: ast::Statement::If { test: t, body: s1, orelse: last }
+            node: ast::Statement::If { test, body: s1, orelse: last }
         }
     },
 };
 
 WhileStatement: ast::LocatedStatement = {
-    <loc:@L> "while" <e:Test> ":" <s:Suite> <s2:("else" ":" Suite)?> => {
+    <loc:@L> "while" <test:Test> ":" <body:Suite> <s2:("else" ":" Suite)?> => {
         let or_else = s2.map(|s| s.2);
         ast::LocatedStatement {
             location: loc,
-            node: ast::Statement::While { test: e, body: s, orelse: or_else },
+            node: ast::Statement::While { test, body, orelse: or_else },
         }
     },
 };
@@ -434,9 +422,7 @@ FuncDef: ast::LocatedStatement = {
 };
 
 Parameters: ast::Parameters = {
-    "(" <a: (ParameterList<TypedParameter>)?> ")" => {
-        a.unwrap_or_else(Default::default)
-    },
+    "(" <a: (ParameterList<TypedParameter>)?> ")" => a.unwrap_or_else(Default::default),
 };
 
 // Note that this is a macro which is used once for function defs, and
@@ -619,42 +605,41 @@ YieldExpr: ast::Expression = {
 };
 
 Test: ast::Expression = {
-    <e:OrTest> <c: ("if" OrTest "else" Test)?> => {
-        match c {
-            Some(c) => {
-                ast::Expression::IfExpression {
-                    test: Box::new(c.1),
-                    body: Box::new(e),
-                    orelse: Box::new(c.3),
-                }
-            },
-            None => e,
+    <expr:OrTest> <condition: ("if" OrTest "else" Test)?> => {
+        if let Some(c) = condition {
+            ast::Expression::IfExpression {
+                test: Box::new(c.1),
+                body: Box::new(expr),
+                orelse: Box::new(c.3),
+            }
+        } else {
+            expr
         }
     },
-    <e:LambdaDef> => e,
+    LambdaDef,
 };
 
 LambdaDef: ast::Expression = {
-    "lambda" <p:ParameterList<UntypedParameter>?> ":" <b:Test> =>
+    "lambda" <p:ParameterList<UntypedParameter>?> ":" <body:Test> =>
         ast::Expression::Lambda {
             args: p.unwrap_or(Default::default()),
-            body: Box::new(b)
+            body: Box::new(body)
        }
 }
 
 OrTest: ast::Expression = {
-    <e:AndTest> => e,
+    AndTest,
     <e1:OrTest> "or" <e2:AndTest> => ast::Expression::BoolOp { a: Box::new(e1), op: ast::BooleanOperator::Or, b: Box::new(e2) },
 };
 
 AndTest: ast::Expression = {
-    <e:NotTest> => e,
+    NotTest,
     <e1:AndTest> "and" <e2:NotTest> => ast::Expression::BoolOp { a: Box::new(e1), op: ast::BooleanOperator::And, b: Box::new(e2) },
 };
 
 NotTest: ast::Expression = {
     "not" <e:NotTest> => ast::Expression::Unop { a: Box::new(e), op: ast::UnaryOperator::Not },
-    <e:Comparison> => e,
+    Comparison,
 };
 
 Comparison: ast::Expression = {
@@ -667,7 +652,7 @@ Comparison: ast::Expression = {
         }
         ast::Expression::Compare { vals, ops }
     },
-    <e:Expression> => e,
+    Expression,
 };
 
 CompOp: ast::Comparison = {
@@ -685,7 +670,7 @@ CompOp: ast::Comparison = {
 
 Expression: ast::Expression = {
     <e1:Expression> "|" <e2:XorExpression> => ast::Expression::Binop { a: Box::new(e1), op: ast::Operator::BitOr, b: Box::new(e2) },
-    <e:XorExpression> => e,
+    XorExpression,
 };
 
 XorExpression: ast::Expression = {
@@ -735,7 +720,7 @@ Factor: ast::Expression = {
     "+" <e:Factor> => ast::Expression::Unop { a: Box::new(e), op: ast::UnaryOperator::Pos },
     "-" <e:Factor> => ast::Expression::Unop { a: Box::new(e), op: ast::UnaryOperator::Neg },
     "~" <e:Factor> => ast::Expression::Unop { a: Box::new(e), op: ast::UnaryOperator::Inv },
-    <e:Power> => e,
+    Power,
 };
 
 Power: ast::Expression = {
@@ -748,7 +733,7 @@ Power: ast::Expression = {
 };
 
 AtomExpr: ast::Expression = {
-    <e:Atom> => e,
+    Atom,
     <f:AtomExpr>  "(" <a:ArgumentList> ")" => ast::Expression::Call { function: Box::new(f), args: a.0, keywords: a.1 },
     <e:AtomExpr>  "[" <s:SubscriptList> "]" => ast::Expression::Subscript { a: Box::new(e), b: Box::new(s) },
     <e:AtomExpr> "." <n:Identifier> => ast::Expression::Attribute { value: Box::new(e), name: n },
@@ -769,7 +754,7 @@ SubscriptList: ast::Expression = {
 };
 
 Subscript: ast::Expression = {
-    <e:Test> => e,
+    Test,
     <e1:Test?> ":" <e2:Test?> <e3:SliceOp?>  => {
         let s1 = e1.unwrap_or(ast::Expression::None);
         let s2 = e2.unwrap_or(ast::Expression::None);
@@ -783,30 +768,17 @@ SliceOp: ast::Expression = {
 }
 
 Atom: ast::Expression = {
-    <s:StringGroup> => ast::Expression::String { value: s },
-    <b:Bytes> => ast::Expression::Bytes { value: b },
-    <n:Number> => ast::Expression::Number { value: n },
-    <i:Identifier> => ast::Expression::Identifier { name: i },
+    <value:StringGroup> => ast::Expression::String { value },
+    <value:Bytes> => ast::Expression::Bytes { value },
+    <value:Number> => ast::Expression::Number { value },
+    <name:Identifier> => ast::Expression::Identifier { name },
     "[" <e:TestListComp?> "]" => {
         let elements = e.unwrap_or(Vec::new());
         ast::Expression::List { elements }
     },
-    "[" <e:TestListComp2> "]" => {
-        // List comprehension:
-        e
-    },
-    "(" <e:TestList2?> <trailing_comma:","?> ")" => {
-         match e {
-             None => ast::Expression::Tuple { elements: Vec::new() },
-             Some(elements) => {
-                 if elements.len() == 1 && trailing_comma.is_none() {
-                   // This is "(e)", which is equivalent to "e"
-                   elements.into_iter().next().unwrap()
-                 } else {
-                   ast::Expression::Tuple { elements }
-                 }
-             }
-         }
+    "[" <e:TestListComp2> "]" => e,
+    "(" <elements:TestList?> ")" => {
+         elements.unwrap_or(ast::Expression::Tuple { elements: Vec::new() })
     },
     "(" <e:Test> <c:CompFor> ")" => {
         ast::Expression::Comprehension {
@@ -825,9 +797,7 @@ Atom: ast::Expression = {
 };
 
 TestListComp: Vec<ast::Expression> = {
-    <e:OneOrMore<TestOrStarExpr>> <_trailing_comma:","?> => {
-        e
-    },
+    <e:OneOrMore<TestOrStarExpr>> <_trailing_comma:","?> => e,
 };
 
 TestListComp2: ast::Expression = {
@@ -840,9 +810,7 @@ TestListComp2: ast::Expression = {
 };
 
 TestDict: Vec<(ast::Expression, ast::Expression)> = {
-    <e1:OneOrMore<DictEntry>> <_trailing_comma:","?> => {
-        e1
-    }
+    <elements:OneOrMore<DictEntry>> <_trailing_comma:","?> => elements,
 };
 
 TestDictComp: ast::Expression = {
@@ -859,9 +827,7 @@ DictEntry: (ast::Expression, ast::Expression) = {
 };
 
 TestSet: Vec<ast::Expression> = {
-    <e1:OneOrMore<Test>> ","? => {
-        e1
-    }
+    <e1:OneOrMore<Test>> ","? => e1
 };
 
 TestSetComp: ast::Expression = {
@@ -873,28 +839,23 @@ TestSetComp: ast::Expression = {
     }
 };
 
+ExpressionOrStarExpression = {
+    Expression,
+    StarExpr
+};
+
 ExpressionList: ast::Expression = {
-    <e: ExpressionList2> => {
-        if e.len() == 1 {
-            e.into_iter().next().unwrap()
+    <elements: OneOrMore<ExpressionOrStarExpression>> <trailing_comma:","?> => {
+        if elements.len() == 1 && trailing_comma.is_none() {
+            elements.into_iter().next().unwrap()
         } else {
-            ast::Expression::Tuple { elements: e }
+            ast::Expression::Tuple { elements }
         }
     },
 };
 
 ExpressionList2: Vec<ast::Expression> = {
-    <e1:Expression> <e2:("," Expression)*> ","? => {
-        let mut l = vec![e1];
-        l.extend(e2.into_iter().map(|x| x.1));
-        l
-    },
-};
-
-// TODO: this TestList2 should be renamed or removed.
-#[inline]
-TestList2: Vec<ast::Expression> = {
-  <elements:OneOrMore<Test>> => elements,
+    <elements:OneOrMore<Expression>> ","? => elements,
 };
 
 // A test list is one of:
@@ -902,13 +863,11 @@ TestList2: Vec<ast::Expression> = {
 // - a single expression
 // - a single expression followed by a trailing comma
 TestList: ast::Expression = {
-    <e:OneOrMore<Test>> <trailing_comma: ","?> => {
-        if e.len() == 1 && trailing_comma.is_none() {
-            e.into_iter().next().unwrap()
+    <elements:OneOrMore<Test>> <trailing_comma: ","?> => {
+        if elements.len() == 1 && trailing_comma.is_none() {
+            elements.into_iter().next().unwrap()
         } else {
-            ast::Expression::Tuple {
-                elements: e
-            }
+            ast::Expression::Tuple { elements }
         }
     }
 };
@@ -919,27 +878,16 @@ StarExpr: ast::Expression = {
 };
 
 // Comprehensions:
-CompFor: Vec<ast::Comprehension> = {
-    <c:SingleForComprehension+> => c,
-};
+CompFor: Vec<ast::Comprehension> = <c:SingleForComprehension+> => c;
 
 SingleForComprehension: ast::Comprehension = {
-    "for" <e:ExpressionList> "in" <i:OrTest> <c2:ComprehensionIf*> => {
-        ast::Comprehension {
-            target: e,
-            iter: i,
-            ifs: c2,
-        }
+    "for" <target:ExpressionList> "in" <iter:OrTest> <c2:ComprehensionIf*> => {
+        ast::Comprehension { target, iter, ifs: c2 }
     }
 };
 
-ExpressionNoCond: ast::Expression = {
-    OrTest,
-};
-
-ComprehensionIf: ast::Expression = {
-    "if" <c:ExpressionNoCond> => c,
-};
+ExpressionNoCond: ast::Expression = OrTest;
+ComprehensionIf: ast::Expression = "if" <c:ExpressionNoCond> => c;
 
 ArgumentList: (Vec<ast::Expression>, Vec<ast::Keyword>) = {
     <e: Comma<FunctionArgument>> => {
@@ -952,7 +900,7 @@ ArgumentList: (Vec<ast::Expression>, Vec<ast::Keyword>) = {
                 },
                 None => {
                     if keywords.len() > 0 {
-                        panic!("positional argument follows keyword argument");
+                        panic!("positional argument follows keyword argument {:?}", keywords);
                     };
                     args.push(value);
                 },
@@ -996,8 +944,8 @@ OneOrMore<T>: Vec<T> = {
 };
 
 Number: ast::Number = {
-    <s:int> => { ast::Number::Integer { value: s } },
-    <s:float> => { ast::Number::Float { value: s } },
+    <value:int> => { ast::Number::Integer { value } },
+    <value:float> => { ast::Number::Float { value } },
     <s:complex> => { ast::Number::Complex { real: s.0, imag: s.1 } },
 };
 
diff --git a/tests/snippets/for.py b/tests/snippets/for.py
index 07e138e3c..e913ac907 100644
--- a/tests/snippets/for.py
+++ b/tests/snippets/for.py
@@ -10,3 +10,15 @@ else:
     x = 3
 
 assert x == 3
+
+y = []
+for x, in [(9,), [2]]:
+    y.append(x)
+
+assert y == [9, 2], str(y)
+
+y = []
+for x, *z in [(9,88,'b'), [2, 'bla'], [None]*4]:
+    y.append(z)
+
+assert y == [[88, 'b'], ['bla'], [None]*3], str(y)
diff --git a/tests/snippets/tuple.py b/tests/snippets/tuple.py
index d50f552dc..b7393fe00 100644
--- a/tests/snippets/tuple.py
+++ b/tests/snippets/tuple.py
@@ -34,3 +34,7 @@ class Foo(object):
 
 foo = Foo()
 assert (foo,) == (foo,)
+
+a = (1, 2, 3)
+a += 1,
+assert a == (1, 2, 3, 1)