[MERGE #5489 @akroshg] Trailing comma in an arrow function should be valid.

Merge pull request #5489 from akroshg:comma

Trailing commas is allowed for all function including arrow function. Since we don't know that an expression can be formals of the lambda function we need to defer the error till we determine that
we indeed are going to have the arrow function.

Author: akroshg

lib/Parser/Parse.cpp

@@ -84,6 +84,7 @@ Parser::Parser(Js::ScriptContext* scriptContext, BOOL strictMode, PageAllocator
     m_isInParsingArgList(false),
     m_hasDestructuringPattern(false),
     m_hasDeferredShorthandInitError(false),
+    m_deferCommaError(false),
     m_pnestedCount(nullptr),
 
     wellKnownPropertyPids(), // should initialize to nullptrs
@@ -99,7 +100,7 @@ Parser::Parser(Js::ScriptContext* scriptContext, BOOL strictMode, PageAllocator
     m_funcParenExprDepth(0),
     m_deferEllipsisError(false),
     m_deferEllipsisErrorLoc(), // calls default initializer
-
+    m_deferCommaErrorLoc(),
     m_tryCatchOrFinallyDepth(0),
 
     m_pstmtCur(nullptr),
@@ -3077,9 +3078,7 @@ ParseNodePtr Parser::ParseTerm(BOOL fAllowCall,
         uint saveCurrBlockId = GetCurrentBlock()->blockId;
         GetCurrentBlock()->blockId = m_nextBlockId++;
 
-        // Push the deferred error state for ellipsis errors. It is possible that another syntax error will occur before we undefer this one.
-        bool deferEllipsisErrorSave = m_deferEllipsisError;
-        RestorePoint ellipsisErrorLocSave = m_deferEllipsisErrorLoc;
+        AutoDeferErrorsRestore deferErrorRestore(this);
 
         this->m_funcParenExprDepth++;
         pnode = ParseExpr<buildAST>(koplNo, &fCanAssign, TRUE, FALSE, nullptr, nullptr /*nameLength*/, nullptr  /*pShortNameOffset*/, &term, true, nullptr, plastRParen);
@@ -3099,22 +3098,22 @@ ParseNodePtr Parser::ParseTerm(BOOL fAllowCall,
             // Put back the original next-block-ID so the existing pid ref stacks will be correct.
             m_nextBlockId = saveNextBlockId;
         }
-
-        // Emit a deferred ... error if one was parsed.
-        if (m_deferEllipsisError && m_token.tk != tkDArrow)
-        {
-            this->GetScanner()->SeekTo(m_deferEllipsisErrorLoc);
-            Error(ERRInvalidSpreadUse);
-        }
         else
         {
-            m_deferEllipsisError = false;
+            // Emit a deferred ... error if one was parsed.
+            if (m_deferEllipsisError)
+            {
+                this->GetScanner()->SeekTo(m_deferEllipsisErrorLoc);
+                Error(ERRInvalidSpreadUse);
+            }
+            else if (m_deferCommaError)
+            {
+                // Emit a comma error if that was deferred.
+                this->GetScanner()->SeekTo(m_deferCommaErrorLoc);
+                Error(ERRsyntax);
+            }
         }
 
-        // We didn't error out, so restore the deferred error state.
-        m_deferEllipsisError = deferEllipsisErrorSave;
-        m_deferEllipsisErrorLoc = ellipsisErrorLocSave;
-
         break;
     }
 
@@ -8801,6 +8800,14 @@ ParseNodePtr Parser::ParseExpr(int oplMin,
         }
         else // a binary operator
         {
+            if (nop == knopComma && m_token.tk == tkRParen)
+            {
+                // Trailing comma
+                this->GetScanner()->Capture(&m_deferCommaErrorLoc);
+                m_deferCommaError = true;
+                break;
+            }
+
             ParseNode* pnode1 = pnode;
 
             // Parse the operand, make a new node, and look for more

lib/Parser/Parse.h

@@ -455,6 +455,8 @@ class Parser
     bool m_hasDestructuringPattern;
     // This bool is used for deferring the shorthand initializer error ( {x = 1}) - as it is allowed in the destructuring grammar.
     bool m_hasDeferredShorthandInitError;
+    bool m_deferEllipsisError;
+    bool m_deferCommaError;
     uint * m_pnestedCount; // count of functions nested at one level below the current node
 
     struct WellKnownPropertyPids
@@ -495,8 +497,8 @@ class Parser
 
     // Used for issuing spread and rest errors when there is ambiguity with lambda parameter lists and parenthesized expressions
     uint m_funcParenExprDepth;
-    bool m_deferEllipsisError;
     RestorePoint m_deferEllipsisErrorLoc;
+    RestorePoint m_deferCommaErrorLoc;
 
     uint m_tryCatchOrFinallyDepth;  // Used to determine if parsing is currently in a try/catch/finally block in order to throw error on yield expressions inside them
 
@@ -585,6 +587,33 @@ class Parser
 #endif
     };
 
+    class AutoDeferErrorsRestore
+    {
+    public:
+        AutoDeferErrorsRestore(Parser *p)
+            : m_parser(p)
+        {
+            m_deferEllipsisErrorSave = m_parser->m_deferEllipsisError;
+            m_deferCommaError = m_parser->m_deferCommaError;
+            m_ellipsisErrorLocSave = m_parser->m_deferEllipsisErrorLoc;
+            m_commaErrorLocSave = m_parser->m_deferCommaErrorLoc;
+        }
+
+        ~AutoDeferErrorsRestore()
+        {
+            m_parser->m_deferEllipsisError = m_deferEllipsisErrorSave;
+            m_parser->m_deferCommaError = m_deferCommaError;
+            m_parser->m_deferEllipsisErrorLoc = m_ellipsisErrorLocSave;
+            m_parser->m_deferCommaErrorLoc = m_commaErrorLocSave;
+        }
+    private:
+        Parser *m_parser;
+        RestorePoint m_ellipsisErrorLocSave;
+        RestorePoint m_commaErrorLocSave;
+        bool m_deferEllipsisErrorSave;
+        bool m_deferCommaError;
+    };
+
     // This function is going to capture some of the important current state of the parser to an object. Once we learn
     // that we need to reparse the grammar again we could use RestoreStateFrom to restore that state to the parser.
     void CaptureState(ParserState *state);

test/es6/lambda1.js

@@ -169,8 +169,8 @@ var tests = [
     {
         name: "Interesting valid and invalid syntax",
         body: function () {
+            assert.doesNotThrow(() => { eval('(x, ) => {};'); }, SyntaxError, "Trailing comma is valid syntax");
             assert.throws(() => { eval('(var x) => {};'); }, SyntaxError, "var not used in formals declaration", "Syntax error");
-            assert.throws(() => { eval('(x, ) => {};'); }, SyntaxError, "missing formal identifier in parameter list", "Syntax error");
             assert.throws(() => { eval('a.x => {};'); }, SyntaxError, "valid expression syntax that is invalid parameter list syntax on lhs of =>", "Syntax error");
             assert.throws(() => { eval('(x, y)[7] => {};'); }, SyntaxError, "valid expression syntax that is invalid parameter list syntax on lhs of =>", "Expected '=>'");
             assert.throws(() => { eval('x() => {};'); }, SyntaxError, "valid expression syntax that is invalid parameter list syntax on lhs of =>", "Syntax error");

test/es6/trailingcomma.js

@@ -24,6 +24,16 @@ var tests = [
             assert.doesNotThrow(function () { eval("function foo() {}; foo(1, 2,);"); }, "Trailing comma in a user function call is a valid syntax");
             assert.doesNotThrow(function () { eval("Math.min(1, 2, );"); }, "Trailing comma in built-in function call is a valid syntax");
         }
+    },
+    {
+        name: "Trailing comma in arrow function",
+        body: function () {
+            assert.doesNotThrow(function () { eval("(a,) => {}"); }, "Trailing comma in an arrow function is a valid syntax");
+            assert.doesNotThrow(function () { eval("let f = (a,) => {}"); }, "Trailing comma in an arrow function is a valid syntax");
+            assert.doesNotThrow(function () { eval("(b = (a,) => {}) => {}"); }, "Trailing comma in an nested arrow function is a valid syntax");
+            assert.doesNotThrow(function () { eval("({b = (a,) => {}}) => {}"); }, "Trailing comma in an nested arrow function in destructuring is a valid syntax");
+            assert.throws(function () { eval("(b = (a,)) => {}"); }, SyntaxError, "Trailing comma in a comma expression is not valid syntax");
+        }
     }
 ];