Few destructuring fixes

akroshg · akroshg · commit f2416b5ff9ef · 2016-04-25T13:36:15.000-07:00
The IsArgument optimization code did not handle the pattern nodes. Added
'cases' for those nop. This happens when we try to find out the
'arguments' usage from a parse node. Here in this case we have an empty pattern and this leads to the assert.
We have place holder slots for destructuring patterns - however when we
generate the scopeinfo for deferred function we didn't account that piece
of information. Fixed that by using the same counter (earlier used for same name args place holder) and increment that for destructuring patterns.
Added unittest for those.
diff --git a/lib/Runtime/ByteCode/ByteCodeEmitter.cpp b/lib/Runtime/ByteCode/ByteCodeEmitter.cpp
@@ -187,6 +187,9 @@ bool IsArguments(ParseNode *pnode)
         case knopTry:
         case knopTryCatch:
         case knopTryFinally:
+        case knopArrayPattern:
+        case knopObjectPattern:
+        case knopParamPattern:
             return true;
 
         default:
diff --git a/lib/Runtime/ByteCode/ByteCodeGenerator.cpp b/lib/Runtime/ByteCode/ByteCodeGenerator.cpp
@@ -2711,7 +2711,7 @@ FuncInfo* PostVisitFunction(ParseNode* pnode, ByteCodeGenerator* byteCodeGenerat
                 if (argSym)
                 {
                     Assert(top->bodyScope->GetScopeSlotCount() == 0);
-                    Assert(top->sameNameArgsPlaceHolderSlotCount == 0);
+                    Assert(top->argsPlaceHolderSlotCount == 0);
                     byteCodeGenerator->AssignRegister(argSym);
                     uint i = 0;
                     auto setArgScopeSlot = [&](ParseNode *pnodeArg)
@@ -2721,10 +2721,14 @@ FuncInfo* PostVisitFunction(ParseNode* pnode, ByteCodeGenerator* byteCodeGenerat
                             Symbol* sym = pnodeArg->sxVar.sym;
                             if (sym->GetScopeSlot() != Js::Constants::NoProperty)
                             {
-                                top->sameNameArgsPlaceHolderSlotCount++; // Same name args appeared before
+                                top->argsPlaceHolderSlotCount++; // Same name args appeared before
                             }
                             sym->SetScopeSlot(i);
                         }
+                        else if (pnodeArg->nop == knopParamPattern)
+                        {
+                            top->argsPlaceHolderSlotCount++;
+                        }
                         i++;
                     };
 
diff --git a/lib/Runtime/ByteCode/FuncInfo.cpp b/lib/Runtime/ByteCode/FuncInfo.cpp
@@ -71,7 +71,7 @@ FuncInfo::FuncInfo(
     inlineCacheMap(nullptr),
     slotProfileIdMap(alloc),
     localPropIdOffset(-1),
-    sameNameArgsPlaceHolderSlotCount(0),
+    argsPlaceHolderSlotCount(0),
     thisScopeSlot(Js::Constants::NoProperty),
     superScopeSlot(Js::Constants::NoProperty),
     superCtorScopeSlot(Js::Constants::NoProperty),
diff --git a/lib/Runtime/ByteCode/FuncInfo.h b/lib/Runtime/ByteCode/FuncInfo.h
@@ -115,7 +115,7 @@ class FuncInfo
     Js::RegSlot yieldRegister;
     Js::RegSlot firstTmpReg;
     Js::RegSlot curTmpReg;
-    int sameNameArgsPlaceHolderSlotCount; // count of place holder slots for same name args
+    int argsPlaceHolderSlotCount;   // count of place holder slots for same name args and destructuring patterns
     int localPropIdOffset;
     Js::RegSlot firstThunkArgReg;
     short thunkArgCount;
diff --git a/lib/Runtime/ByteCode/ScopeInfo.cpp b/lib/Runtime/ByteCode/ScopeInfo.cpp
@@ -54,8 +54,8 @@ namespace Js
     {
         int count = scope->Count();
 
-        // Add same name args place holder slot counts
-        AddSlotCount(count, scope->GetFunc()->sameNameArgsPlaceHolderSlotCount);
+        // Add argsPlaceHolder which includes same name args and destructuring patterns on parameters
+        AddSlotCount(count, scope->GetFunc()->argsPlaceHolderSlotCount);
         AddSlotCount(count, scope->GetFunc()->thisScopeSlot != Js::Constants::NoRegister ? 1 : 0);
         AddSlotCount(count, scope->GetFunc()->newTargetScopeSlot != Js::Constants::NoRegister ? 1 : 0);
 
diff --git a/test/es6/destructuring_bugs.js b/test/es6/destructuring_bugs.js
@@ -288,6 +288,30 @@ var tests = [
         assert.areEqual(a, undefined);
         assert.areEqual(b, 1);
     }
+  },
+  {
+    name: "Destructuring - Empty object pattern inside call node is a valid syntax",
+    body: function () {
+        function f() {
+            ({} = []).Symbol.interator();
+            eval("");
+        };
+    }
+  },
+  {
+    name: "Destructuring - Place holder slots for pattern are accounted when undeferred.",
+    body: function () {
+        function foo({a}) {
+            function x() {}
+            eval('');
+        }
+        foo({});
+        function foo1(...[b]) {
+            function x() {}
+            eval('');
+        }
+        foo1([]);
+    }
   }
 ];
 

Original file line number	Diff line number	Diff line change
`@@ -2711,7 +2711,7 @@ FuncInfo* PostVisitFunction(ParseNode* pnode, ByteCodeGenerator* byteCodeGenerat`
`2711`	`2711`	`if (argSym)`
`2712`	`2712`	`{`
`2713`	`2713`	`Assert(top->bodyScope->GetScopeSlotCount() == 0);`
`2714`		`- Assert(top->sameNameArgsPlaceHolderSlotCount == 0);`
	`2714`	`+ Assert(top->argsPlaceHolderSlotCount == 0);`
`2715`	`2715`	`byteCodeGenerator->AssignRegister(argSym);`
`2716`	`2716`	`uint i = 0;`
`2717`	`2717`	`auto setArgScopeSlot = [&](ParseNode *pnodeArg)`
`@@ -2721,10 +2721,14 @@ FuncInfo* PostVisitFunction(ParseNode* pnode, ByteCodeGenerator* byteCodeGenerat`
`2721`	`2721`	`Symbol* sym = pnodeArg->sxVar.sym;`
`2722`	`2722`	`if (sym->GetScopeSlot() != Js::Constants::NoProperty)`
`2723`	`2723`	`{`
`2724`		`- top->sameNameArgsPlaceHolderSlotCount++; // Same name args appeared before`
	`2724`	`+ top->argsPlaceHolderSlotCount++; // Same name args appeared before`
`2725`	`2725`	`}`
`2726`	`2726`	`sym->SetScopeSlot(i);`
`2727`	`2727`	`}`
	`2728`	`+ else if (pnodeArg->nop == knopParamPattern)`
	`2729`	`+ {`
	`2730`	`+ top->argsPlaceHolderSlotCount++;`
	`2731`	`+ }`
`2728`	`2732`	`i++;`
`2729`	`2733`	`};`
`2730`	`2734`
Original file line number	Diff line number	Diff line change
`@@ -54,8 +54,8 @@ namespace Js`
`54`	`54`	`{`
`55`	`55`	`int count = scope->Count();`
`56`	`56`
`57`		`- // Add same name args place holder slot counts`
`58`		`- AddSlotCount(count, scope->GetFunc()->sameNameArgsPlaceHolderSlotCount);`
	`57`	`+ // Add argsPlaceHolder which includes same name args and destructuring patterns on parameters`
	`58`	`+ AddSlotCount(count, scope->GetFunc()->argsPlaceHolderSlotCount);`
`59`	`59`	`AddSlotCount(count, scope->GetFunc()->thisScopeSlot != Js::Constants::NoRegister ? 1 : 0);`
`60`	`60`	`AddSlotCount(count, scope->GetFunc()->newTargetScopeSlot != Js::Constants::NoRegister ? 1 : 0);`
`61`	`61`