Skip to content

Commit cabfa36

Browse files
aneeshdkaneeshdk
committed
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.
1 parent 066df8a commit cabfa36

7 files changed

Lines changed: 196 additions & 123 deletions

File tree

‎lib/Parser/Hash.h‎

Lines changed: 3 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -190,20 +190,11 @@ struct Ident
190190
return prevRef;
191191
}
192192

193-
PidRefStack * FindOrAddPidRef(ArenaAllocator *alloc, int scopeId, int maxScopeId = -1)
193+
PidRefStack * FindOrAddPidRef(ArenaAllocator *alloc, int scopeId)
194194
{
195-
// If we were supplied with a maxScopeId, then we potentially need to look one more
196-
// scope level out. This can happen if we have a declaration in function scope shadowing
197-
// a parameter scope declaration. In this case we'd need to look beyond the body scope (scopeId)
198-
// to the outer parameterScope (maxScopeId).
199-
if (maxScopeId == -1)
200-
{
201-
maxScopeId = scopeId;
202-
}
203-
204195
// If the stack is empty, or we are pushing to the innermost scope already,
205196
// we can go ahead and push a new PidRef on the stack.
206-
if (m_pidRefStack == nullptr || m_pidRefStack->id < maxScopeId)
197+
if (m_pidRefStack == nullptr)
207198
{
208199
PidRefStack *newRef = Anew(alloc, PidRefStack, scopeId);
209200
if (newRef == nullptr)
@@ -226,15 +217,7 @@ struct Ident
226217
return ref;
227218
}
228219

229-
if (ref->id == maxScopeId
230-
// If we match the different maxScopeId, then this match is sufficient if it is a decl.
231-
// This is because the parameter scope decl would have been created before this point.
232-
&& ref->sym != nullptr)
233-
{
234-
return ref;
235-
}
236-
237-
if (ref->prev == nullptr || ref->prev->id < maxScopeId)
220+
if (ref->prev == nullptr || ref->id < scopeId)
238221
{
239222
// No existing PidRef for this scopeId, so create and insert one at this position.
240223
PidRefStack *newRef = Anew(alloc, PidRefStack, scopeId);

‎lib/Parser/Parse.cpp‎

Lines changed: 71 additions & 65 deletions
Original file line numberDiff line numberDiff line change
@@ -820,31 +820,7 @@ Symbol* Parser::AddDeclForPid(ParseNodePtr pnode, IdentPtr pid, SymbolType symbo
820820
blockInfo = blockInfo->pBlockInfoOuter;
821821
}
822822

823-
int maxScopeId = blockInfo->pnodeBlock->sxBlock.blockId;
824-
825-
// The body of catch may have let declared variable. In the case of pattern, found at catch parameter level,
826-
// we need to search the duplication at that scope level as well - thus extending the scope lookup range.
827-
if (IsES6DestructuringEnabled()
828-
&& fBlockScope
829-
&& blockInfo->pBlockInfoOuter != nullptr
830-
&& blockInfo->pBlockInfoOuter->pnodeBlock->sxBlock.scope != nullptr
831-
&& blockInfo->pBlockInfoOuter->pnodeBlock->sxBlock.scope->GetScopeType() == ScopeType_CatchParamPattern)
832-
{
833-
maxScopeId = blockInfo->pBlockInfoOuter->pnodeBlock->sxBlock.blockId;
834-
}
835-
836-
if (blockInfo->pnodeBlock->sxBlock.scope != nullptr && blockInfo->pnodeBlock->sxBlock.scope->GetScopeType() == ScopeType_FunctionBody)
837-
{
838-
// Even though we have separate param and body scope it can get merged into one later.
839-
// So when looking up the symbol check if there is a parameter scope and try to get it first.
840-
BlockInfoStack *outerBlockInfo = blockInfo->pBlockInfoOuter;
841-
if (outerBlockInfo != nullptr && outerBlockInfo->pnodeBlock->sxBlock.blockType == PnodeBlockType::Parameter && outerBlockInfo->pnodeBlock->sxBlock.scope->GetCanMergeWithBodyScope())
842-
{
843-
maxScopeId = outerBlockInfo->pnodeBlock->sxBlock.blockId;
844-
}
845-
}
846-
847-
refForDecl = this->FindOrAddPidRef(pid, blockInfo->pnodeBlock->sxBlock.blockId, maxScopeId);
823+
refForDecl = this->FindOrAddPidRef(pid, blockInfo->pnodeBlock->sxBlock.blockId);
848824

849825
if (refForDecl == nullptr)
850826
{
@@ -1572,6 +1548,19 @@ ParseNodePtr Parser::ParseBlock(ParseNodePtr pnodeLabel, LabelId* pLabelId)
15721548

15731549
pnodeBlock = StartParseBlock<buildAST>(PnodeBlockType::Regular, ScopeType_Block, pnodeLabel, pLabelId);
15741550

1551+
BlockInfoStack* outerBlockInfo = m_currentBlockInfo->pBlockInfoOuter;
1552+
if (outerBlockInfo != nullptr && outerBlockInfo->pnodeBlock != nullptr
1553+
&& outerBlockInfo->pnodeBlock->sxBlock.scope != nullptr
1554+
&& outerBlockInfo->pnodeBlock->sxBlock.scope->GetScopeType() == ScopeType_CatchParamPattern)
1555+
{
1556+
// If we are parsing the catch block then destructured params can have let declrations. Let's add them to the new block.
1557+
for (ParseNodePtr pnode = m_currentBlockInfo->pBlockInfoOuter->pnodeBlock->sxBlock.pnodeLexVars; pnode; pnode = pnode->sxVar.pnodeNext)
1558+
{
1559+
PidRefStack* ref = PushPidRef(pnode->sxVar.sym->GetPid());
1560+
ref->SetSym(pnode->sxVar.sym);
1561+
}
1562+
}
1563+
15751564
ChkCurTok(tkLCurly, ERRnoLcurly);
15761565
ParseNodePtr * ppnodeList = nullptr;
15771566
if (buildAST)
@@ -5012,6 +5001,43 @@ bool Parser::ParseFncDeclHelper(ParseNodePtr pnodeFnc, ParseNodePtr pnodeFncPare
50125001
pnodeFnc->sxFnc.nestedCount++;
50135002
}
50145003

5004+
Scope* paramScope = pnodeFnc->sxFnc.pnodeScopes ? pnodeFnc->sxFnc.pnodeScopes->sxBlock.scope : nullptr;
5005+
if (paramScope != nullptr && pnodeFnc->sxFnc.HasNonSimpleParameterList() && !fAsync)
5006+
{
5007+
Assert(paramScope != nullptr);
5008+
5009+
if (paramScope->GetCanMergeWithBodyScope())
5010+
{
5011+
paramScope->ForEachSymbolUntil([this, paramScope](Symbol* sym) {
5012+
if (sym->GetPid()->GetTopRef()->sym == nullptr)
5013+
{
5014+
// One of the symbol has non local reference. Mark the param scope as we can't merge it with body scope.
5015+
paramScope->SetCannotMergeWithBodyScope();
5016+
return true;
5017+
}
5018+
else
5019+
{
5020+
// If no non-local references are there then the top of the ref stack should point to the same symbol.
5021+
Assert(sym->GetPid()->GetTopRef()->sym == sym);
5022+
}
5023+
return false;
5024+
});
5025+
}
5026+
}
5027+
5028+
// If the param scope is merged with the body scope we want to use the param scope symbols in the body scope.
5029+
// So add a pid ref for the body using the param scope symbol. Note that in this case the same symbol will occur twice
5030+
// in the same pid ref stack.
5031+
if (paramScope != nullptr && paramScope->GetCanMergeWithBodyScope() && (isTopLevelDeferredFunc || !fAsync))
5032+
{
5033+
paramScope->ForEachSymbol([this](Symbol* paramSym)
5034+
{
5035+
Symbol* sym = paramSym->GetPid()->GetTopRef()->GetSym();
5036+
PidRefStack* ref = PushPidRef(paramSym->GetPid());
5037+
ref->SetSym(sym);
5038+
});
5039+
}
5040+
50155041
if (isTopLevelDeferredFunc || (m_InAsmMode && m_deferAsmJs))
50165042
{
50175043
AssertMsg(!fLambda, "Deferring function parsing of a function does not handle lambda syntax");
@@ -5054,40 +5080,16 @@ bool Parser::ParseFncDeclHelper(ParseNodePtr pnodeFnc, ParseNodePtr pnodeFncPare
50545080
pnodeFnc->sxFnc.pnodeVars = nullptr;
50555081
m_ppnodeVar = &pnodeFnc->sxFnc.pnodeVars;
50565082

5057-
if (pnodeFnc->sxFnc.HasNonSimpleParameterList() && !fAsync)
5083+
if (paramScope != nullptr && !paramScope->GetCanMergeWithBodyScope())
50585084
{
5059-
Scope* paramScope = pnodeFnc->sxFnc.pnodeScopes->sxBlock.scope;
5060-
Assert(paramScope != nullptr);
5061-
5062-
if (paramScope->GetCanMergeWithBodyScope())
5063-
{
5064-
paramScope->ForEachSymbolUntil([this, paramScope](Symbol* sym) {
5065-
if (sym->GetPid()->GetTopRef()->sym == nullptr)
5066-
{
5067-
// One of the symbol has non local reference. Mark the param scope as we can't merge it with body scope.
5068-
paramScope->SetCannotMergeWithBodyScope();
5069-
return true;
5070-
}
5071-
else
5072-
{
5073-
// If no non-local references are there then the top of the ref stack should point to the same symbol.
5074-
Assert(sym->GetPid()->GetTopRef()->sym == sym);
5075-
}
5076-
return false;
5077-
});
5078-
}
5079-
5080-
if (!paramScope->GetCanMergeWithBodyScope())
5081-
{
5082-
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"));
5083-
// Add a new symbol reference for each formal in the param scope to the body scope.
5084-
paramScope->ForEachSymbol([this](Symbol* param) {
5085-
OUTPUT_TRACE_DEBUGONLY(Js::ParsePhase, _u("Creating a duplicate symbol for the parameter %s in the body scope\n"), param->GetPid()->Psz());
5086-
ParseNodePtr paramNode = this->CreateVarDeclNode(param->GetPid(), STVariable, false, nullptr, false);
5087-
Assert(paramNode && paramNode->sxVar.sym->GetScope()->GetScopeType() == ScopeType_FunctionBody);
5088-
paramNode->sxVar.sym->SetHasInit(true);
5089-
});
5090-
}
5085+
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"));
5086+
// Add a new symbol reference for each formal in the param scope to the body scope.
5087+
paramScope->ForEachSymbol([this](Symbol* param) {
5088+
OUTPUT_TRACE_DEBUGONLY(Js::ParsePhase, _u("Creating a duplicate symbol for the parameter %s in the body scope\n"), param->GetPid()->Psz());
5089+
ParseNodePtr paramNode = this->CreateVarDeclNode(param->GetPid(), STVariable, false, nullptr, false);
5090+
Assert(paramNode && paramNode->sxVar.sym->GetScope()->GetScopeType() == ScopeType_FunctionBody);
5091+
paramNode->sxVar.sym->SetHasInit(true);
5092+
});
50915093
}
50925094

50935095
// Keep nested function declarations and expressions in the same list at function scope.
@@ -7453,6 +7455,14 @@ void Parser::TransformAsyncFncDeclAST(ParseNodePtr *pnodeBody, bool fLambda)
74537455
// meaning post-parsing that won't match the actual parameter list of the generator.
74547456
pnodeFncGenerator->sxFnc.SetHasNonSimpleParameterList(hasNonSimpleParameterList);
74557457

7458+
// We always merge the param scope and body scope for async methods right now.
7459+
// So adding an additional reference for the param symbols to the body.
7460+
paramScope->ForEachSymbol([this] (Symbol* param)
7461+
{
7462+
Symbol* sym = param->GetPid()->GetTopRef()->GetSym();
7463+
PidRefStack* ref = PushPidRef(param->GetPid());
7464+
ref->SetSym(sym);
7465+
});
74567466
pnodeFncGenerator->sxFnc.pnodeBody = nullptr;
74577467
if (fLambda)
74587468
{
@@ -8486,11 +8496,7 @@ PidRefStack* Parser::PushPidRef(IdentPtr pid)
84868496
Assert(GetCurrentBlock() != nullptr);
84878497
AssertMsg(pid != nullptr, "PID should be created");
84888498
PidRefStack *ref = pid->GetTopRef();
8489-
if (!ref || (ref->GetScopeId() < GetCurrentBlock()->sxBlock.blockId)
8490-
// 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.
8491-
&& !(m_currentBlockInfo->pBlockInfoOuter->pnodeBlock->sxBlock.blockId == ref->GetScopeId()
8492-
&& m_currentBlockInfo->pBlockInfoOuter->pnodeBlock->sxBlock.blockType == PnodeBlockType::Parameter
8493-
&& m_currentBlockInfo->pBlockInfoOuter->pnodeBlock->sxBlock.scope->GetCanMergeWithBodyScope()))
8499+
if (!ref || (ref->GetScopeId() < GetCurrentBlock()->sxBlock.blockId))
84948500
{
84958501
ref = Anew(&m_nodeAllocator, PidRefStack);
84968502
if (ref == nullptr)
@@ -8503,9 +8509,9 @@ PidRefStack* Parser::PushPidRef(IdentPtr pid)
85038509
return ref;
85048510
}
85058511

8506-
PidRefStack* Parser::FindOrAddPidRef(IdentPtr pid, int scopeId, int maxScopeId)
8512+
PidRefStack* Parser::FindOrAddPidRef(IdentPtr pid, int scopeId)
85078513
{
8508-
PidRefStack *ref = pid->FindOrAddPidRef(&m_nodeAllocator, scopeId, maxScopeId);
8514+
PidRefStack *ref = pid->FindOrAddPidRef(&m_nodeAllocator, scopeId);
85098515
if (ref == NULL)
85108516
{
85118517
Error(ERRnoMemory);

‎lib/Parser/Parse.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -928,7 +928,7 @@ class Parser
928928
private:
929929
void DeferOrEmitPotentialSpreadError(ParseNodePtr pnodeT);
930930
PidRefStack* PushPidRef(IdentPtr pid);
931-
PidRefStack* FindOrAddPidRef(IdentPtr pid, int blockId, int maxScopeId = -1);
931+
PidRefStack* FindOrAddPidRef(IdentPtr pid, int blockId);
932932
void RemovePrevPidRef(IdentPtr pid, PidRefStack *lastRef);
933933
void SetPidRefsInScopeDynamic(IdentPtr pid, int blockId);
934934

‎lib/Runtime/ByteCode/Scope.cpp‎

Lines changed: 2 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -115,41 +115,12 @@ void Scope::MergeParamAndBodyScopes(ParseNode *pnodeScope, ByteCodeGenerator *by
115115
return;
116116
}
117117

118-
bodyScope->ForEachSymbol([&](Symbol * sym)
119-
{
120-
// Duplicate 'arguments' - param scope arguments wins.
121-
if (byteCodeGenerator->UseParserBindings()
122-
&& sym->GetDecl()->sxVar.pid == byteCodeGenerator->GetParser()->names()->arguments)
123-
{
124-
return;
125-
}
126-
127-
Assert(paramScope->m_symList == nullptr || paramScope->FindLocalSymbol(sym->GetName()) == nullptr);
128-
paramScope->AddNewSymbol(sym);
129-
});
130-
131-
// Reassign non-formal slot positions. Formals need to keep their slot positions to ensure
132-
// the argument object works properly. Other symbols need to be reassigned slot positions.
133-
// The sym belonging to arguments will use the same slot.
118+
bodyScope->scopeSlotCount = paramScope->scopeSlotCount;
134119
paramScope->ForEachSymbol([&](Symbol * sym)
135120
{
136-
if (sym->GetSymbolType() != STFormal && sym->GetScopeSlot() != Js::Constants::NoProperty && !sym->GetIsArguments())
137-
{
138-
sym->SetScopeSlot(Js::Constants::NoProperty);
139-
sym->EnsureScopeSlot(pnodeScope->sxFnc.funcInfo);
140-
}
141-
sym->SetScope(bodyScope);
121+
bodyScope->AddNewSymbol(sym);
142122
});
143123

144-
bodyScope->m_count = paramScope->m_count;
145-
bodyScope->m_symList = paramScope->m_symList;
146-
bodyScope->scopeSlotCount = paramScope->scopeSlotCount;
147-
if (bodyScope->symbolTable != nullptr)
148-
{
149-
Adelete(byteCodeGenerator->GetAllocator(), bodyScope->symbolTable);
150-
bodyScope->symbolTable = nullptr;
151-
}
152-
bodyScope->symbolTable = paramScope->symbolTable;
153124
if (paramScope->GetIsObject())
154125
{
155126
bodyScope->SetIsObject();

‎lib/Runtime/ByteCode/ScopeInfo.cpp‎

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -212,10 +212,20 @@ namespace Js
212212
}
213213
else
214214
{
215-
Assert((currentScope->GetEnclosingScope() ==
216-
(parentFunc->IsGlobalFunction() ? parentFunc->GetGlobalEvalBlockScope() : parentFunc->GetBodyScope())) ||
217-
// The method can be defined in the parameter scope of the parent function
218-
(currentScope->GetEnclosingScope() == parentFunc->GetParamScope() && !parentFunc->GetParamScope()->GetCanMergeWithBodyScope()));
215+
if (currentScope->GetEnclosingScope() == parentFunc->GetParamScope())
216+
{
217+
Assert(!parentFunc->GetParamScope()->GetCanMergeWithBodyScope());
218+
Assert(funcInfo->GetParamScope()->GetCanMergeWithBodyScope());
219+
}
220+
else if (currentScope->GetEnclosingScope() == funcInfo->GetParamScope())
221+
{
222+
Assert(!funcInfo->GetParamScope()->GetCanMergeWithBodyScope());
223+
}
224+
else
225+
{
226+
Assert(currentScope->GetEnclosingScope() ==
227+
(parentFunc->IsGlobalFunction() ? parentFunc->GetGlobalEvalBlockScope() : parentFunc->GetBodyScope()));
228+
}
219229
}
220230
#endif
221231
Js::ScopeInfo::SaveParentScopeInfo(parentFunc, funcInfo);

‎test/es6/default-splitscope.js‎

Lines changed: 67 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ var tests = [
5757
assert.areEqual(10, f5()()(), "Parameter scope works fine with nested functions");
5858

5959
var a1 = 10;
60-
function f6(b = function () { return a1; }) {
60+
function f6(a, b = function () { a; return a1; }) {
6161
assert.areEqual(undefined, a1, "Inside the function body the assignment hasn't happened yet");
6262
var a1 = 20;
6363
assert.areEqual(20, a1, "Assignment to the symbol inside the function changes the value");
@@ -69,7 +69,7 @@ var tests = [
6969
a = 20;
7070
return b;
7171
}
72-
assert.areEqual(10, f7().iFnc(), "Function definition inside the object literal should capture the formal from the param scope");
72+
assert.areEqual(10, f7().iFnc(), "Function definition inside the object literal should capture the formal from the param scope");
7373
}
7474
},
7575
{
@@ -505,6 +505,71 @@ var tests = [
505505
assert.areEqual(11, f9()()()(), "Split scope function defined within the param scope should capture the formals from the corresponding param scope in nested scope");
506506
}
507507
},
508+
{
509+
name: "Split scope with symbol overriding",
510+
body: function () {
511+
function f1(a = 10, b = function () { return a; }) {
512+
assert.areEqual(100, a(), "Function definition inside the body is hoisted");
513+
function a () {
514+
return 100;
515+
}
516+
return b;
517+
}
518+
assert.areEqual(10, f1()(), "Function definition in the param scope captures the symbol from the param scope");
519+
520+
function f2(a = 10, b = function () { return a; }, c = b) {
521+
a = 20;
522+
assert.areEqual(20, b(), "Function definition in the body scope captures the body symbol");
523+
function b() {
524+
return a;
525+
}
526+
return [c, b];
527+
}
528+
var result = f2();
529+
assert.areEqual(10, result[0](), "Function definition in the param scope captures the param scope symbol");
530+
assert.areEqual(20, result[1](), "Function definition in the body captures the body symbol");
531+
532+
var g = 1;
533+
function f3(a = 10, b = function () { a; return g;}) {
534+
assert.areEqual(10, g(), "Function definition inside the body is unaffected by the outer variable");
535+
function g() {
536+
return 10;
537+
}
538+
return b;
539+
}
540+
assert.areEqual(1, f3()(), "Function definition in the param scope captures the outer scoped var");
541+
542+
function f4(a = x1, b = function g() {
543+
a;
544+
return function h() {
545+
assert.areEqual(10, x1, "x1 is captured from the outer scope");
546+
};
547+
}) {
548+
var x1 = 100;
549+
b()();
550+
};
551+
var x1 = 10;
552+
f4();
553+
554+
var x2 = 1;
555+
function f5(a = x2, b = function() { a; return x2; }) {
556+
{
557+
function x2() {
558+
}
559+
}
560+
var x2 = 2;
561+
return b;
562+
}
563+
assert.areEqual(1, f5()(), "Symbol capture at the param scope is unaffected by the inner definitions");
564+
565+
var x3 = 1;
566+
function f6(a = x3, b = function(_x) { a; return x3; }) {
567+
var x3 = 2;
568+
return b;
569+
}
570+
assert.areEqual(1, f6()(), "Symbol capture at the param scope is unaffected by other references in the body and param");
571+
}
572+
},
508573
{
509574
name: "Split parameter scope and eval",
510575
body: function () {

0 commit comments

Comments
 (0)