Fix for the pid ref issues with default parameters

When param and body scopes are merged, when a formal is referenced in the
body we were using an additional argument named maxScopeId to look up
until the param scope block. This logic was causing issues when we try to
insert new references into the pid ref linked list. This changelist gets
rid of the requirement of maxScopeId by adding additional pid refs for
each param scope symbol into the body before we start parsing the body.

lib/Parser/Hash.h

@@ -190,20 +190,11 @@ public:
         return prevRef;
     }
 
-    PidRefStack * FindOrAddPidRef(ArenaAllocator *alloc, int scopeId, int maxScopeId = -1)
+    PidRefStack * FindOrAddPidRef(ArenaAllocator *alloc, int scopeId)
     {
-        // If we were supplied with a maxScopeId, then we potentially need to look one more
-        // scope level out. This can happen if we have a declaration in function scope shadowing
-        // a parameter scope declaration. In this case we'd need to look beyond the body scope (scopeId)
-        // to the outer parameterScope (maxScopeId).
-        if (maxScopeId == -1)
-        {
-            maxScopeId = scopeId;
-        }
-
         // If the stack is empty, or we are pushing to the innermost scope already,
         // we can go ahead and push a new PidRef on the stack.
-        if (m_pidRefStack == nullptr || m_pidRefStack->id < maxScopeId)
+        if (m_pidRefStack == nullptr)
         {
             PidRefStack *newRef = Anew(alloc, PidRefStack, scopeId);
             if (newRef == nullptr)
@@ -226,15 +217,7 @@ public:
                 return ref;
             }
 
-            if (ref->id == maxScopeId
-                // If we match the different maxScopeId, then this match is sufficient if it is a decl.
-                // This is because the parameter scope decl would have been created before this point.
-                && ref->sym != nullptr)
-            {
-                return ref;
-            }
-
-            if (ref->prev == nullptr || ref->prev->id < maxScopeId)
+            if (ref->prev == nullptr || ref->id  < scopeId)
             {
                 // No existing PidRef for this scopeId, so create and insert one at this position.
                 PidRefStack *newRef = Anew(alloc, PidRefStack, scopeId);

lib/Parser/Parse.cpp

@@ -820,31 +820,7 @@ Symbol* Parser::AddDeclForPid(ParseNodePtr pnode, IdentPtr pid, SymbolType symbo
         blockInfo = blockInfo->pBlockInfoOuter;
     }
 
-    int maxScopeId = blockInfo->pnodeBlock->sxBlock.blockId;
-
-    // The body of catch may have let declared variable. In the case of pattern, found at catch parameter level,
-    // we need to search the duplication at that scope level as well - thus extending the scope lookup range.
-    if (IsES6DestructuringEnabled()
-        && fBlockScope
-        && blockInfo->pBlockInfoOuter != nullptr
-        && blockInfo->pBlockInfoOuter->pnodeBlock->sxBlock.scope != nullptr
-        && blockInfo->pBlockInfoOuter->pnodeBlock->sxBlock.scope->GetScopeType() == ScopeType_CatchParamPattern)
-    {
-        maxScopeId = blockInfo->pBlockInfoOuter->pnodeBlock->sxBlock.blockId;
-    }
-
-    if (blockInfo->pnodeBlock->sxBlock.scope != nullptr && blockInfo->pnodeBlock->sxBlock.scope->GetScopeType() == ScopeType_FunctionBody)
-    {
-        // Even though we have separate param and body scope it can get merged into one later.
-        // So when looking up the symbol check if there is a parameter scope and try to get it first.
-        BlockInfoStack *outerBlockInfo = blockInfo->pBlockInfoOuter;
-        if (outerBlockInfo != nullptr && outerBlockInfo->pnodeBlock->sxBlock.blockType == PnodeBlockType::Parameter && outerBlockInfo->pnodeBlock->sxBlock.scope->GetCanMergeWithBodyScope())
-        {
-            maxScopeId = outerBlockInfo->pnodeBlock->sxBlock.blockId;
-        }
-    }
-
-    refForDecl = this->FindOrAddPidRef(pid, blockInfo->pnodeBlock->sxBlock.blockId, maxScopeId);
+    refForDecl = this->FindOrAddPidRef(pid, blockInfo->pnodeBlock->sxBlock.blockId);
 
     if (refForDecl == nullptr)
     {
@@ -1572,6 +1548,19 @@ ParseNodePtr Parser::ParseBlock(ParseNodePtr pnodeLabel, LabelId* pLabelId)
 
     pnodeBlock = StartParseBlock<buildAST>(PnodeBlockType::Regular, ScopeType_Block, pnodeLabel, pLabelId);
 
+    BlockInfoStack* outerBlockInfo = m_currentBlockInfo->pBlockInfoOuter;
+    if (outerBlockInfo != nullptr && outerBlockInfo->pnodeBlock != nullptr
+        && outerBlockInfo->pnodeBlock->sxBlock.scope != nullptr
+        && outerBlockInfo->pnodeBlock->sxBlock.scope->GetScopeType() == ScopeType_CatchParamPattern)
+    {
+        // If we are parsing the catch block then destructured params can have let declrations. Let's add them to the new block.
+        for (ParseNodePtr pnode = m_currentBlockInfo->pBlockInfoOuter->pnodeBlock->sxBlock.pnodeLexVars; pnode; pnode = pnode->sxVar.pnodeNext)
+        {
+            PidRefStack* ref = PushPidRef(pnode->sxVar.sym->GetPid());
+            ref->SetSym(pnode->sxVar.sym);
+        }
+    }
+
     ChkCurTok(tkLCurly, ERRnoLcurly);
     ParseNodePtr * ppnodeList = nullptr;
     if (buildAST)
@@ -5012,6 +5001,43 @@ bool Parser::ParseFncDeclHelper(ParseNodePtr pnodeFnc, ParseNodePtr pnodeFncPare
             pnodeFnc->sxFnc.nestedCount++;
         }
 
+        Scope* paramScope = pnodeFnc->sxFnc.pnodeScopes ? pnodeFnc->sxFnc.pnodeScopes->sxBlock.scope : nullptr;
+        if (paramScope != nullptr && pnodeFnc->sxFnc.HasNonSimpleParameterList() && !fAsync)
+        {
+            Assert(paramScope != nullptr);
+
+            if (paramScope->GetCanMergeWithBodyScope())
+            {
+                paramScope->ForEachSymbolUntil([this, paramScope](Symbol* sym) {
+                    if (sym->GetPid()->GetTopRef()->sym == nullptr)
+                    {
+                        // One of the symbol has non local reference. Mark the param scope as we can't merge it with body scope.
+                        paramScope->SetCannotMergeWithBodyScope();
+                        return true;
+                    }
+                    else
+                    {
+                        // If no non-local references are there then the top of the ref stack should point to the same symbol.
+                        Assert(sym->GetPid()->GetTopRef()->sym == sym);
+                    }
+                    return false;
+                });
+            }
+        }
+
+        // If the param scope is merged with the body scope we want to use the param scope symbols in the body scope.
+        // So add a pid ref for the body using the param scope symbol. Note that in this case the same symbol will occur twice
+        // in the same pid ref stack.
+        if (paramScope != nullptr && paramScope->GetCanMergeWithBodyScope() && (isTopLevelDeferredFunc || !fAsync))
+        {
+            paramScope->ForEachSymbol([this](Symbol* paramSym)
+            {
+                Symbol* sym = paramSym->GetPid()->GetTopRef()->GetSym();
+                PidRefStack* ref = PushPidRef(paramSym->GetPid());
+                ref->SetSym(sym);
+            });
+        }
+
         if (isTopLevelDeferredFunc || (m_InAsmMode && m_deferAsmJs))
         {
             AssertMsg(!fLambda, "Deferring function parsing of a function does not handle lambda syntax");
@@ -5054,40 +5080,16 @@ bool Parser::ParseFncDeclHelper(ParseNodePtr pnodeFnc, ParseNodePtr pnodeFncPare
             pnodeFnc->sxFnc.pnodeVars = nullptr;
             m_ppnodeVar = &pnodeFnc->sxFnc.pnodeVars;
 
-            if (pnodeFnc->sxFnc.HasNonSimpleParameterList() && !fAsync)
+            if (paramScope != nullptr && !paramScope->GetCanMergeWithBodyScope())
             {
-                Scope* paramScope = pnodeFnc->sxFnc.pnodeScopes->sxBlock.scope;
-                Assert(paramScope != nullptr);
-
-                if (paramScope->GetCanMergeWithBodyScope())
-                {
-                    paramScope->ForEachSymbolUntil([this, paramScope](Symbol* sym) {
-                        if (sym->GetPid()->GetTopRef()->sym == nullptr)
-                        {
-                            // One of the symbol has non local reference. Mark the param scope as we can't merge it with body scope.
-                            paramScope->SetCannotMergeWithBodyScope();
-                            return true;
-                        }
-                        else
-                        {
-                            // If no non-local references are there then the top of the ref stack should point to the same symbol.
-                            Assert(sym->GetPid()->GetTopRef()->sym == sym);
-                        }
-                        return false;
-                    });
-                }
-
-                if (!paramScope->GetCanMergeWithBodyScope())
-                {
-                    OUTPUT_TRACE_DEBUGONLY(Js::ParsePhase, _u("The param and body scope of the function %s cannot be merged\n"), pnodeFnc->sxFnc.pnodeName ? pnodeFnc->sxFnc.pnodeName->sxVar.pid->Psz() : _u("Anonymous function"));
-                    // Add a new symbol reference for each formal in the param scope to the body scope.
-                    paramScope->ForEachSymbol([this](Symbol* param) {
-                        OUTPUT_TRACE_DEBUGONLY(Js::ParsePhase, _u("Creating a duplicate symbol for the parameter %s in the body scope\n"), param->GetPid()->Psz());
-                        ParseNodePtr paramNode = this->CreateVarDeclNode(param->GetPid(), STVariable, false, nullptr, false);
-                        Assert(paramNode && paramNode->sxVar.sym->GetScope()->GetScopeType() == ScopeType_FunctionBody);
-                        paramNode->sxVar.sym->SetHasInit(true);
-                    });
-                }
+                OUTPUT_TRACE_DEBUGONLY(Js::ParsePhase, _u("The param and body scope of the function %s cannot be merged\n"), pnodeFnc->sxFnc.pnodeName ? pnodeFnc->sxFnc.pnodeName->sxVar.pid->Psz() : _u("Anonymous function"));
+                // Add a new symbol reference for each formal in the param scope to the body scope.
+                paramScope->ForEachSymbol([this](Symbol* param) {
+                    OUTPUT_TRACE_DEBUGONLY(Js::ParsePhase, _u("Creating a duplicate symbol for the parameter %s in the body scope\n"), param->GetPid()->Psz());
+                    ParseNodePtr paramNode = this->CreateVarDeclNode(param->GetPid(), STVariable, false, nullptr, false);
+                    Assert(paramNode && paramNode->sxVar.sym->GetScope()->GetScopeType() == ScopeType_FunctionBody);
+                    paramNode->sxVar.sym->SetHasInit(true);
+                });
             }
 
             // Keep nested function declarations and expressions in the same list at function scope.
@@ -7453,6 +7455,14 @@ void Parser::TransformAsyncFncDeclAST(ParseNodePtr *pnodeBody, bool fLambda)
     // meaning post-parsing that won't match the actual parameter list of the generator.
     pnodeFncGenerator->sxFnc.SetHasNonSimpleParameterList(hasNonSimpleParameterList);
 
+    // We always merge the param scope and body scope for async methods right now.
+    // So adding an additional reference for the param symbols to the body.
+    paramScope->ForEachSymbol([this] (Symbol* param)
+    {
+        Symbol* sym = param->GetPid()->GetTopRef()->GetSym();
+        PidRefStack* ref = PushPidRef(param->GetPid());
+        ref->SetSym(sym);
+    });
     pnodeFncGenerator->sxFnc.pnodeBody = nullptr;
     if (fLambda)
     {
@@ -8486,11 +8496,7 @@ PidRefStack* Parser::PushPidRef(IdentPtr pid)
     Assert(GetCurrentBlock() != nullptr);
     AssertMsg(pid != nullptr, "PID should be created");
     PidRefStack *ref = pid->GetTopRef();
-    if (!ref || (ref->GetScopeId() < GetCurrentBlock()->sxBlock.blockId)
-                // We could have the ref from the parameter scope if it is merged with body scope. In that case we can skip creating a new one.
-                && !(m_currentBlockInfo->pBlockInfoOuter->pnodeBlock->sxBlock.blockId == ref->GetScopeId()
-                    && m_currentBlockInfo->pBlockInfoOuter->pnodeBlock->sxBlock.blockType == PnodeBlockType::Parameter
-                    && m_currentBlockInfo->pBlockInfoOuter->pnodeBlock->sxBlock.scope->GetCanMergeWithBodyScope()))
+    if (!ref || (ref->GetScopeId() < GetCurrentBlock()->sxBlock.blockId))
     {
         ref = Anew(&m_nodeAllocator, PidRefStack);
         if (ref == nullptr)
@@ -8503,9 +8509,9 @@ PidRefStack* Parser::PushPidRef(IdentPtr pid)
     return ref;
 }
 
-PidRefStack* Parser::FindOrAddPidRef(IdentPtr pid, int scopeId, int maxScopeId)
+PidRefStack* Parser::FindOrAddPidRef(IdentPtr pid, int scopeId)
 {
-    PidRefStack *ref = pid->FindOrAddPidRef(&m_nodeAllocator, scopeId, maxScopeId);
+    PidRefStack *ref = pid->FindOrAddPidRef(&m_nodeAllocator, scopeId);
     if (ref == NULL)
     {
         Error(ERRnoMemory);

lib/Parser/Parse.h

@@ -928,7 +928,7 @@ public:
 private:
     void DeferOrEmitPotentialSpreadError(ParseNodePtr pnodeT);
     PidRefStack* PushPidRef(IdentPtr pid);
-    PidRefStack* FindOrAddPidRef(IdentPtr pid, int blockId, int maxScopeId = -1);
+    PidRefStack* FindOrAddPidRef(IdentPtr pid, int blockId);
     void RemovePrevPidRef(IdentPtr pid, PidRefStack *lastRef);
     void SetPidRefsInScopeDynamic(IdentPtr pid, int blockId);
 

lib/Runtime/ByteCode/Scope.cpp

@@ -115,41 +115,12 @@ void Scope::MergeParamAndBodyScopes(ParseNode *pnodeScope, ByteCodeGenerator *by
         return;
     }
 
-    bodyScope->ForEachSymbol([&](Symbol * sym)
-    {
-        // Duplicate 'arguments' - param scope arguments wins.
-        if (byteCodeGenerator->UseParserBindings()
-            && sym->GetDecl()->sxVar.pid == byteCodeGenerator->GetParser()->names()->arguments)
-        {
-            return;
-        }
-
-        Assert(paramScope->m_symList == nullptr || paramScope->FindLocalSymbol(sym->GetName()) == nullptr);
-        paramScope->AddNewSymbol(sym);
-    });
-
-    // Reassign non-formal slot positions. Formals need to keep their slot positions to ensure
-    // the argument object works properly. Other symbols need to be reassigned slot positions.
-    // The sym belonging to arguments will use the same slot.
+    bodyScope->scopeSlotCount = paramScope->scopeSlotCount;
     paramScope->ForEachSymbol([&](Symbol * sym)
     {
-        if (sym->GetSymbolType() != STFormal && sym->GetScopeSlot() != Js::Constants::NoProperty && !sym->GetIsArguments())
-        {
-            sym->SetScopeSlot(Js::Constants::NoProperty);
-            sym->EnsureScopeSlot(pnodeScope->sxFnc.funcInfo);
-        }
-        sym->SetScope(bodyScope);
+        bodyScope->AddNewSymbol(sym);
     });
 
-    bodyScope->m_count = paramScope->m_count;
-    bodyScope->m_symList = paramScope->m_symList;
-    bodyScope->scopeSlotCount = paramScope->scopeSlotCount;
-    if (bodyScope->symbolTable != nullptr)
-    {
-        Adelete(byteCodeGenerator->GetAllocator(), bodyScope->symbolTable);
-        bodyScope->symbolTable = nullptr;
-    }
-    bodyScope->symbolTable = paramScope->symbolTable;
     if (paramScope->GetIsObject())
     {
         bodyScope->SetIsObject();

lib/Runtime/ByteCode/ScopeInfo.cpp

@@ -212,10 +212,20 @@ namespace Js
                 }
                 else
                 {
-                    Assert((currentScope->GetEnclosingScope() ==
-                        (parentFunc->IsGlobalFunction() ? parentFunc->GetGlobalEvalBlockScope() : parentFunc->GetBodyScope())) ||
-                        // The method can be defined in the parameter scope of the parent function
-                        (currentScope->GetEnclosingScope() == parentFunc->GetParamScope() && !parentFunc->GetParamScope()->GetCanMergeWithBodyScope()));
+                    if (currentScope->GetEnclosingScope() == parentFunc->GetParamScope())
+                    {
+                        Assert(!parentFunc->GetParamScope()->GetCanMergeWithBodyScope());
+                        Assert(funcInfo->GetParamScope()->GetCanMergeWithBodyScope());
+                    }
+                    else if (currentScope->GetEnclosingScope() == funcInfo->GetParamScope())
+                    {
+                        Assert(!funcInfo->GetParamScope()->GetCanMergeWithBodyScope());
+                    }
+                    else
+                    { 
+                        Assert(currentScope->GetEnclosingScope() ==
+                            (parentFunc->IsGlobalFunction() ? parentFunc->GetGlobalEvalBlockScope() : parentFunc->GetBodyScope()));
+                    }
                 }
 #endif
                 Js::ScopeInfo::SaveParentScopeInfo(parentFunc, funcInfo);

test/es6/default-splitscope.js

@@ -57,7 +57,7 @@ var tests = [
         assert.areEqual(10, f5()()(), "Parameter scope works fine with nested functions"); 
 
         var a1 = 10; 
-        function f6(b = function () { return a1; }) { 
+        function f6(a, b = function () { a; return a1; }) { 
             assert.areEqual(undefined, a1, "Inside the function body the assignment hasn't happened yet"); 
             var a1 = 20; 
             assert.areEqual(20, a1, "Assignment to the symbol inside the function changes the value"); 
@@ -69,7 +69,7 @@ var tests = [
             a = 20; 
             return b; 
         } 
-        assert.areEqual(10, f7().iFnc(), "Function definition inside the object literal should capture the formal from the param scope"); 
+        assert.areEqual(10, f7().iFnc(), "Function definition inside the object literal should capture the formal from the param scope");
     } 
  }, 
  { 
@@ -505,6 +505,71 @@ var tests = [
           assert.areEqual(11, f9()()()(), "Split scope function defined within the param scope should capture the formals from the corresponding param scope in nested scope"); 
     }   
   }, 
+  {
+    name: "Split scope with symbol overriding",
+    body: function () {
+          function f1(a = 10, b = function () { return a; }) {
+              assert.areEqual(100, a(), "Function definition inside the body is hoisted");
+              function a () {
+                  return 100;
+              }
+              return b;
+        }
+        assert.areEqual(10, f1()(), "Function definition in the param scope captures the symbol from the param scope");
+
+        function f2(a = 10, b = function () { return a; }, c = b) {
+            a = 20;
+            assert.areEqual(20, b(), "Function definition in the body scope captures the body symbol");
+            function b() {
+                return a;
+            }
+            return [c, b];
+        }
+        var result = f2();
+        assert.areEqual(10, result[0](), "Function definition in the param scope captures the param scope symbol");
+        assert.areEqual(20, result[1](), "Function definition in the body captures the body symbol");
+        
+        var g = 1;
+        function f3(a = 10, b = function () { a; return g;}) {
+            assert.areEqual(10, g(), "Function definition inside the body is unaffected by the outer variable");
+            function g() {
+                return 10;
+            }
+            return b;
+        }
+        assert.areEqual(1, f3()(), "Function definition in the param scope captures the outer scoped var");
+        
+        function f4(a = x1, b = function g() {
+            a;
+            return function h() {
+                assert.areEqual(10, x1, "x1 is captured from the outer scope");
+            };
+        }) {
+            var x1 = 100;
+            b()();
+        };
+        var x1 = 10;
+        f4();
+        
+        var x2 = 1;
+        function f5(a = x2, b = function() { a; return x2; }) {
+            {
+                function x2() {
+                }
+            }
+            var x2 = 2;
+            return b;
+        }
+        assert.areEqual(1, f5()(), "Symbol capture at the param scope is unaffected by the inner definitions");
+        
+        var x3 = 1;
+        function f6(a = x3, b = function(_x) { a; return x3; }) {
+            var x3 = 2;
+            return b;
+        }
+        assert.areEqual(1, f6()(), "Symbol capture at the param scope is unaffected by other references in the body and param");
+    }
+  },
   { 
     name: "Split parameter scope and eval", 
     body: function () { 

test/es6/default.js

@@ -182,6 +182,44 @@ var tests = [
       }
       var b1 = 1;
       foo6();
+      
+      var a1 = 10; 
+      function foo7(b = function () { return a1; }) { 
+          assert.areEqual(undefined, a1, "Inside the function body the assignment hasn't happened yet"); 
+          var a1 = 20; 
+          assert.areEqual(20, a1, "Assignment to the symbol inside the function changes the value"); 
+          return b; 
+      } 
+      assert.areEqual(10, foo7()(), "Function in the param scope correctly binds to the outer variable");
+      
+      function foo8(a = x1, b = function g() {
+          return function h() {
+              assert.areEqual(10, x1, "x1 is captured from the outer scope");
+          };
+      }) {
+          var x1 = 100;
+          b()();
+      };
+      var x1 = 10;
+      foo8();
+      
+      var x2 = 1;
+      function foo9(a = x2, b = function() { return x2; }) {
+          {
+             function x2() {
+            }
+          }
+          var x2 = 2;
+          return b;
+      }
+      assert.areEqual(1, foo9()(), "Symbol capture at the param scope is unaffected by the inner definitions");
+      
+      var x3 = 1;
+      function foo10(a = x3, b = function(_x) { return x3; }) {
+          var x3 = 2;
+          return b;
+      }
+      assert.areEqual(1, foo10()(), "Symbol capture at the param scope is unaffected by other references in the body and param");
     }
   },
   {