Push and pop paren expr depth counter for deferred rest and spread errors

Fixes OS: 12896447

There exists a situation where deferred parsing can miss a deferred spread error that will get found in the full parse. This results because we get a different paren expr depth when we reparse the nested function inside the paren expr.

The fix is to push/pop the paren expr count and spread error state every time we parse a new function. We know that when parsing a new function, any potential rest/spread errors we see are not going to be resolved in the outer function.

I also changed the naming of the paren count to make it clearer that it is for paren exprs only and per function.

lib/Parser/Parse.cpp

@@ -96,7 +96,7 @@ Parser::Parser(Js::ScriptContext* scriptContext, BOOL strictMode, PageAllocator
     m_pCurrentAstSize = nullptr;
     m_arrayDepth = 0;
     m_funcInArrayDepth = 0;
-    m_parenDepth = 0;
+    m_funcParenExprDepth = 0;
     m_funcInArray = 0;
     m_tryCatchOrFinallyDepth = 0;
     m_UsesArgumentsAtGlobal = false;
@@ -3026,9 +3026,13 @@ ParseNodePtr Parser::ParseTerm(BOOL fAllowCall,
         uint saveCurrBlockId = GetCurrentBlock()->sxBlock.blockId;
         GetCurrentBlock()->sxBlock.blockId = m_nextBlockId++;
 
-        this->m_parenDepth++;
+        // 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;
+
+        this->m_funcParenExprDepth++;
         pnode = ParseExpr<buildAST>(koplNo, &fCanAssign, TRUE, FALSE, nullptr, nullptr /*nameLength*/, nullptr  /*pShortNameOffset*/, &term, true, nullptr, plastRParen);
-        this->m_parenDepth--;
+        this->m_funcParenExprDepth--;
 
         if (buildAST && plastRParen)
         {
@@ -3048,13 +3052,18 @@ ParseNodePtr Parser::ParseTerm(BOOL fAllowCall,
         // Emit a deferred ... error if one was parsed.
         if (m_deferEllipsisError && m_token.tk != tkDArrow)
         {
-            m_pscan->SeekTo(m_EllipsisErrLoc);
+            m_pscan->SeekTo(m_deferEllipsisErrorLoc);
             Error(ERRInvalidSpreadUse);
         }
         else
         {
             m_deferEllipsisError = false;
         }
+
+        // We didn't error out, so restore the deferred error state.
+        m_deferEllipsisError = deferEllipsisErrorSave;
+        m_deferEllipsisErrorLoc = ellipsisErrorLocSave;
+
         break;
     }
 
@@ -5168,6 +5177,9 @@ bool Parser::ParseFncDeclHelper(ParseNodePtr pnodeFnc, LPCOLESTR pNameHint, usho
         ppnodeExprScopeSave = m_ppnodeExprScope;
         m_ppnodeExprScope = nullptr;
 
+        uint parenExprDepthSave = m_funcParenExprDepth;
+        m_funcParenExprDepth = 0;
+
         if (!skipFormals)
         {
             bool fLambdaParamsSave = m_reparsingLambdaParams;
@@ -5286,6 +5298,8 @@ bool Parser::ParseFncDeclHelper(ParseNodePtr pnodeFnc, LPCOLESTR pNameHint, usho
             });
         }
 
+        AssertMsg(m_funcParenExprDepth == 0, "Paren exprs should have been resolved by the time we finish function formals");
+
         if (isTopLevelDeferredFunc || (m_InAsmMode && m_deferAsmJs))
         {
 #ifdef ASMJS_PLAT
@@ -5394,6 +5408,9 @@ bool Parser::ParseFncDeclHelper(ParseNodePtr pnodeFnc, LPCOLESTR pNameHint, usho
             }
         }
 
+        // Restore the paren count for any outer spread/rest error checking.
+        m_funcParenExprDepth = parenExprDepthSave;
+
         if (pnodeInnerBlock)
         {
             FinishParseBlock(pnodeInnerBlock, *pNeedScanRCurly);
@@ -8023,14 +8040,15 @@ LPCOLESTR Parser::AppendNameHints(LPCOLESTR left, LPCOLESTR right, uint32 *pName
  */
 void Parser::DeferOrEmitPotentialSpreadError(ParseNodePtr pnodeT)
 {
-    if (m_parenDepth > 0)
+    if (m_funcParenExprDepth > 0)
     {
         if (m_token.tk == tkRParen)
         {
            if (!m_deferEllipsisError)
             {
-                // Capture only the first error instance.
-                m_pscan->Capture(&m_EllipsisErrLoc);
+                // Capture only the first error instance. Because a lambda will cause a reparse in a formals context, we can assume
+                // that this will be a spread error. Nested paren exprs will have their own error instance.
+                m_pscan->Capture(&m_deferEllipsisErrorLoc);
                 m_deferEllipsisError = true;
             }
         }

lib/Parser/Parse.h

@@ -416,7 +416,6 @@ private:
     charcount_t m_funcInArray;
     uint m_scopeCountNoAst;
 
-
     /*
      * Parsing states for super restriction
      */
@@ -426,10 +425,10 @@ private:
     uint m_parsingSuperRestrictionState;
     friend class AutoParsingSuperRestrictionStateRestorer;
 
-    // Used for issuing spread and rest errors when there is ambiguity with parameter list and parenthesized expressions
-    uint m_parenDepth;
+    // 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_EllipsisErrLoc;
+    RestorePoint m_deferEllipsisErrorLoc;
 
     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
 

test/es6/deferSpreadRestError.js

@@ -0,0 +1,60 @@
+//-------------------------------------------------------------------------------------------------------
+// Copyright (C) Microsoft. All rights reserved.
+// Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
+//-------------------------------------------------------------------------------------------------------
+
+WScript.LoadScriptFile("..\\UnitTestFramework\\UnitTestFramework.js");
+
+var tests = [
+  { 
+    name: "Deferred spread/rest errors in lambda formals",
+    body: function () {
+      assert.doesNotThrow(function () { (a, b = [...[1,2,3]], ...rest) => {}; },     "Correct spread and rest usage");
+      assert.doesNotThrow(function () { (a, b = ([...[1,2,3]]), ...rest) => {}; },   "Correct spread and rest usage with added parens");
+      assert.doesNotThrow(function () { (a, b = (([...[1,2,3]])), ...rest) => {}; }, "Correct spread and rest usage with added parens");
+      assert.throws(function () { eval("(a = ...NaN, b = [...[1,2,3]], ...rest) => {};"); },
+                    SyntaxError,
+                    "Invalid spread with valid rest throws on the first invalid spread",
+                    "Unexpected ... operator");
+      assert.throws(function () { eval("(a = (...NaN), ...b = [...[1,2,3]], ...rest) => {};"); },
+                    SyntaxError,
+                    "Invalid spread in parens with invalid and valid rest throws on the first invalid spread",
+                    "Invalid use of the ... operator. Spread can only be used in call arguments or an array literal.");
+      assert.throws(function () { eval("(a = (...NaN), ...b = [...[1,2,3]], rest) => {};"); },
+                    SyntaxError,
+                    "Invalid spread in parens with invalid rest throws on the first invalid spread",
+                    "Invalid use of the ... operator. Spread can only be used in call arguments or an array literal.");
+      assert.throws(function () { eval("(a = [...NaN], ...b = [...[1,2,3]], rest) => {};"); },
+                    SyntaxError,
+                    "Invalid spread (runtime error) with invalid rest throws on the first invalid rest",
+                    "Unexpected ... operator");
+      assert.throws(function () { eval("(a, ...b, ...rest) => {};"); },
+                    SyntaxError,
+                    "Invalid rest with valid rest throws on the first invalid rest",
+                    "Unexpected ... operator");
+      assert.throws(function () { eval("(...rest = ...NaN) => {};"); },
+                    SyntaxError,
+                    "Invalid rest with invalid spread initializer throws on the invalid rest",
+                    "The rest parameter cannot have a default initializer.");
+    }
+  },
+    {
+        name: "Nested parenthesized expressions",
+        body: function () {
+            assert.throws(function () { eval("(function f() { if (...mznxbp) { (mmqykj) => undefined; } });"); }, 
+                          SyntaxError, 
+                          "Parenthesized expression outside a function does not contribute to nested count",
+                          "Invalid use of the ... operator. Spread can only be used in call arguments or an array literal.");
+            assert.throws(function () { eval("(a, (...b, ...a))"); }, 
+                          SyntaxError, 
+                          "Nested parenthesized expression throws rest error before deferred spread error",
+                          "Unexpected ... operator");
+            assert.throws(function () { eval("((...a)) => 1"); }, 
+                          SyntaxError, 
+                          "Nested parenthesized expression as lambda formals throws deferred spread error",
+                          "Invalid use of the ... operator. Spread can only be used in call arguments or an array literal.");
+        }
+    }
+];
+
+testRunner.runTests(tests, { verbose: WScript.Arguments[0] != "summary" });

test/es6/rest.js

@@ -53,36 +53,7 @@ var tests = [
       assert.doesNotThrow(function () { function foo(...a) { eval('const a = 0;'); }; foo(); }, "Const redeclaration of a non-default parameter inside an eval does not throw with a non-simple parameter list");
 
       assert.doesNotThrow(function () { eval("function foo(a, ...args) { function args() { } }"); }, "Nested function redeclaration of a rest parameter does not throw");
-
-      // Deferred spread/rest errors in lambda formals
-      assert.doesNotThrow(function () { (a, b = [...[1,2,3]], ...rest) => {}; },     "Correct spread and rest usage");
-      assert.doesNotThrow(function () { (a, b = ([...[1,2,3]]), ...rest) => {}; },   "Correct spread and rest usage with added parens");
-      assert.doesNotThrow(function () { (a, b = (([...[1,2,3]])), ...rest) => {}; }, "Correct spread and rest usage with added parens");
-      assert.throws(function () { eval("(a = ...NaN, b = [...[1,2,3]], ...rest) => {};"); },
-                    SyntaxError,
-                    "Invalid spread with valid rest throws on the first invalid spread",
-                    "Unexpected ... operator");
-      assert.throws(function () { eval("(a = (...NaN), ...b = [...[1,2,3]], ...rest) => {};"); },
-                    SyntaxError,
-                    "Invalid spread in parens with invalid and valid rest throws on the first invalid spread",
-                    "Invalid use of the ... operator. Spread can only be used in call arguments or an array literal.");
-      assert.throws(function () { eval("(a = (...NaN), ...b = [...[1,2,3]], rest) => {};"); },
-                    SyntaxError,
-                    "Invalid spread in parens with invalid rest throws on the first invalid spread",
-                    "Invalid use of the ... operator. Spread can only be used in call arguments or an array literal.");
-      assert.throws(function () { eval("(a = [...NaN], ...b = [...[1,2,3]], rest) => {};"); },
-                    SyntaxError,
-                    "Invalid spread (runtime error) with invalid rest throws on the first invalid rest",
-                    "Unexpected ... operator");
-      assert.throws(function () { eval("(a, ...b, ...rest) => {};"); },
-                    SyntaxError,
-                    "Invalid rest with valid rest throws on the first invalid rest",
-                    "Unexpected ... operator");
-      assert.throws(function () { eval("(...rest = ...NaN) => {};"); },
-                    SyntaxError,
-                    "Invalid rest with invalid spread initializer throws on the invalid rest",
-                    "The rest parameter cannot have a default initializer.");
-
+      
       assert.throws(function () { eval("var x = { set setter(...x) {} }"); },
                     SyntaxError,
                     "Setter methods cannot have a rest parameter",

test/es6/rlexe.xml

@@ -1472,6 +1472,12 @@
       <tags>BugFix</tags>
     </default>
   </test>
+  <test>
+    <default>
+      <files>deferSpreadRestError.js</files>
+      <compile-flags>-args summary -endargs</compile-flags>
+    </default>
+  </test>
   <test>
     <default>
       <files>bug_OS10898061.js</files>