Skip to content

Commit ce7963b

Browse files
aneeshdkaneeshdk
committed
[MERGE chakra-core#892] Fixing scope issues with function expressions with split scope
Merge pull request chakra-core#892 from aneeshdk:SplitScopeFuncExpr This change fixes multiple issues with using function expression symbols in split scope. One issue was if the function expression is not captured and has not eval then we were adding the function expression symbol to body scope. In split scope case I changed it to add to the param scope. But we don't have to duplicate the function expression name symbol in the body. Function expression should be pop out only after taking out the param scope. Another issue was while creating the new frame display during BeginBodyScope we have to skip the function expression. In split scope the body scope was always getting marked as must instantiate even though no closure was there in the body scope. I added a new flag to the scope object to mark whether a closure is there.
2 parents 655dc14 + 3fff752 commit ce7963b

9 files changed

Lines changed: 140 additions & 42 deletions

File tree

‎lib/Backend/IRBuilder.cpp‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6704,13 +6704,21 @@ IRBuilder::BuildEmpty(Js::OpCode newOpcode, uint32 offset)
67046704
Js::Constants::NoByteCodeOffset);
67056705
}
67066706

6707+
IR::RegOpnd* tempRegOpnd = IR::RegOpnd::New(StackSym::New(this->m_func), TyVar, this->m_func);
67076708
this->AddInstr(
67086709
IR::Instr::New(
67096710
Js::OpCode::LdFrameDisplay,
6710-
this->BuildDstOpnd(this->m_func->GetJnFunction()->GetLocalFrameDisplayRegister()),
6711+
tempRegOpnd,
67116712
this->BuildSrcOpnd(this->m_func->GetJnFunction()->GetLocalClosureRegister()),
67126713
this->BuildSrcOpnd(this->m_func->GetJnFunction()->GetLocalFrameDisplayRegister()),
6713-
m_func),
6714+
this->m_func),
6715+
Js::Constants::NoByteCodeOffset);
6716+
this->AddInstr(
6717+
IR::Instr::New(
6718+
Js::OpCode::MOV,
6719+
this->BuildDstOpnd(this->m_func->GetJnFunction()->GetLocalFrameDisplayRegister()),
6720+
tempRegOpnd,
6721+
this->m_func),
67146722
Js::Constants::NoByteCodeOffset);
67156723
}
67166724
break;

‎lib/Runtime/ByteCode/ByteCodeEmitter.cpp‎

Lines changed: 44 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -3278,9 +3278,9 @@ void ByteCodeGenerator::EmitOneFunction(ParseNode *pnode)
32783278
{
32793279
// Emit bytecode to copy the initial values from param names to their corresponding body bindings.
32803280
// We have to do this after the rest param is marked as false for need declaration.
3281-
paramScope->ForEachSymbol([this, funcInfo, paramScope, byteCodeFunction](Symbol* param) {
3281+
paramScope->ForEachSymbol([&](Symbol* param) {
32823282
Symbol* varSym = funcInfo->GetBodyScope()->FindLocalSymbol(param->GetName());
3283-
Assert(varSym || param->GetIsArguments());
3283+
Assert(varSym || param->GetIsArguments() || pnode->sxFnc.pnodeName->sxVar.sym == param);
32843284
Assert(param->GetIsArguments() || param->IsInSlot(funcInfo));
32853285
if (varSym && varSym->GetSymbolType() == STVariable && (varSym->IsInSlot(funcInfo) || varSym->GetLocation() != Js::Constants::NoRegister))
32863286
{
@@ -3570,22 +3570,7 @@ void ByteCodeGenerator::EmitScopeList(ParseNode *pnode, ParseNode *breakOnBodySc
35703570
this->StartEmitFunction(pnode);
35713571

35723572
Scope* paramScope = pnode->sxFnc.funcInfo->GetParamScope();
3573-
Scope* bodyScope = pnode->sxFnc.funcInfo->GetBodyScope();
35743573

3575-
if (paramScope && !paramScope->GetCanMergeWithBodyScope())
3576-
{
3577-
ParseNodePtr paramBlock = pnode->sxFnc.pnodeScopes;
3578-
Assert(paramBlock->nop == knopBlock && paramBlock->sxBlock.blockType == Parameter);
3579-
3580-
PushScope(paramScope);
3581-
3582-
// While emitting the functions we have to stop when we see the body scope block.
3583-
// Otherwise functions defined in the body scope will not be able to get the right references.
3584-
this->EmitScopeList(paramBlock->sxBlock.pnodeScopes, pnode->sxFnc.pnodeBodyScope);
3585-
Assert(this->GetCurrentScope() == paramScope);
3586-
}
3587-
3588-
PushScope(bodyScope);
35893574
// Persist outer func scope info if nested func is deferred
35903575
if (CONFIG_FLAG(DeferNested))
35913576
{
@@ -3605,12 +3590,6 @@ void ByteCodeGenerator::EmitScopeList(ParseNode *pnode, ParseNode *breakOnBodySc
36053590

36063591
this->EmitOneFunction(pnode);
36073592
this->EndEmitFunction(pnode);
3608-
3609-
if (paramScope && !paramScope->GetCanMergeWithBodyScope())
3610-
{
3611-
Assert(this->GetCurrentScope() == paramScope);
3612-
PopScope(); // Pop the param scope
3613-
}
36143593
}
36153594
pnode = pnode->sxFnc.pnodeNext;
36163595
break;
@@ -3768,7 +3747,14 @@ void ByteCodeGenerator::StartEmitFunction(ParseNode *pnodeFnc)
37683747
else
37693748
{
37703749
Symbol *sym = funcInfo->root->sxFnc.GetFuncSymbol();
3771-
funcInfo->bodyScope->AddSymbol(sym);
3750+
if (funcInfo->paramScope->GetCanMergeWithBodyScope())
3751+
{
3752+
funcInfo->bodyScope->AddSymbol(sym);
3753+
}
3754+
else
3755+
{
3756+
funcInfo->paramScope->AddSymbol(sym);
3757+
}
37723758
}
37733759
}
37743760

@@ -3803,9 +3789,6 @@ void ByteCodeGenerator::StartEmitFunction(ParseNode *pnodeFnc)
38033789
}
38043790
}
38053791

3806-
bodyScope->SetMustInstantiate(funcInfo->frameObjRegister != Js::Constants::NoRegister || funcInfo->frameSlotsRegister != Js::Constants::NoRegister);
3807-
paramScope->SetMustInstantiate(!paramScope->GetCanMergeWithBodyScope());
3808-
38093792
if (bodyScope->GetIsObject())
38103793
{
38113794
// Win8 908700: Disable under F12 debugger because there are too many cached scopes holding onto locals.
@@ -4016,6 +3999,18 @@ void ByteCodeGenerator::StartEmitFunction(ParseNode *pnodeFnc)
40163999
this->EnsureLetConstScopeSlots(pnodeFnc->sxFnc.pnodeBodyScope, funcInfo);
40174000
}
40184001
}
4002+
4003+
if (!paramScope->GetCanMergeWithBodyScope() && bodyScope->GetScopeSlotCount() == 0 && !bodyScope->GetHasOwnLocalInClosure())
4004+
{
4005+
// When we have split scope the body scope may be wrongly marked as must instantiate even though the capture occurred
4006+
// in param scope. This check is to make sure if no capture occurs in body scope make in not must instantiate.
4007+
bodyScope->SetMustInstantiate(false);
4008+
}
4009+
else
4010+
{
4011+
bodyScope->SetMustInstantiate(funcInfo->frameObjRegister != Js::Constants::NoRegister || funcInfo->frameSlotsRegister != Js::Constants::NoRegister);
4012+
}
4013+
paramScope->SetMustInstantiate(!paramScope->GetCanMergeWithBodyScope());
40194014
}
40204015
else
40214016
{
@@ -4026,6 +4021,21 @@ void ByteCodeGenerator::StartEmitFunction(ParseNode *pnodeFnc)
40264021
Assert(bodyScope->GetIsObject());
40274022
}
40284023
}
4024+
4025+
if (paramScope && !paramScope->GetCanMergeWithBodyScope())
4026+
{
4027+
ParseNodePtr paramBlock = pnodeFnc->sxFnc.pnodeScopes;
4028+
Assert(paramBlock->nop == knopBlock && paramBlock->sxBlock.blockType == Parameter);
4029+
4030+
PushScope(paramScope);
4031+
4032+
// While emitting the functions we have to stop when we see the body scope block.
4033+
// Otherwise functions defined in the body scope will not be able to get the right references.
4034+
this->EmitScopeList(paramBlock->sxBlock.pnodeScopes, pnodeFnc->sxFnc.pnodeBodyScope);
4035+
Assert(this->GetCurrentScope() == paramScope);
4036+
}
4037+
4038+
PushScope(bodyScope);
40294039
}
40304040

40314041
void ByteCodeGenerator::EmitModuleExportAccess(Symbol* sym, Js::OpCode opcode, Js::RegSlot location, FuncInfo* funcInfo)
@@ -4151,6 +4161,13 @@ void ByteCodeGenerator::EndEmitFunction(ParseNode *pnodeFnc)
41514161

41524162
FuncInfo *funcInfo = pnodeFnc->sxFnc.funcInfo;
41534163

4164+
Scope* paramScope = funcInfo->paramScope;
4165+
if (paramScope && !paramScope->GetCanMergeWithBodyScope())
4166+
{
4167+
Assert(this->GetCurrentScope() == paramScope);
4168+
PopScope(); // Pop the param scope
4169+
}
4170+
41544171
Scope *scope = funcInfo->funcExprScope;
41554172
if (scope && scope->GetMustInstantiate())
41564173
{

‎lib/Runtime/ByteCode/ByteCodeGenerator.cpp‎

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -998,15 +998,10 @@ void ByteCodeGenerator::RestoreScopeInfo(Js::FunctionBody* functionBody)
998998
bodyScope = Anew(alloc, Scope, alloc, ScopeType_FunctionBody, true);
999999
}
10001000
}
1001+
bodyScope->SetHasOwnLocalInClosure(scopeInfo->GetHasOwnLocalInClosure());
10011002

10021003
FuncInfo* func = Anew(alloc, FuncInfo, functionBody->GetDisplayName(), alloc, paramScope, bodyScope, nullptr, functionBody);
10031004

1004-
if (paramScope != nullptr)
1005-
{
1006-
paramScope->SetFunc(func);
1007-
paramScopeInfo->GetScopeInfo(nullptr, this, func, paramScope);
1008-
}
1009-
10101005
if (bodyScope->GetScopeType() == ScopeType_GlobalEvalBlock)
10111006
{
10121007
func->bodyScope = this->currentScope;
@@ -1032,6 +1027,12 @@ void ByteCodeGenerator::RestoreScopeInfo(Js::FunctionBody* functionBody)
10321027
funcExprScopeInfo->GetScopeInfo(nullptr, this, func, funcExprScope);
10331028
}
10341029

1030+
// Restore the param scope after the function expression scope
1031+
if (paramScope != nullptr)
1032+
{
1033+
paramScope->SetFunc(func);
1034+
paramScopeInfo->GetScopeInfo(nullptr, this, func, paramScope);
1035+
}
10351036
scopeInfo->GetScopeInfo(nullptr, this, func, bodyScope);
10361037
}
10371038
else

‎lib/Runtime/ByteCode/Scope.cpp‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,8 @@ bool Scope::IsBlockScope(FuncInfo *funcInfo)
1616

1717
void Scope::SetHasLocalInClosure(bool has)
1818
{
19+
SetHasOwnLocalInClosure(has);
20+
1921
// (Note: if any catch var is closure-captured, we won't merge the catch scope with the function scope.
2022
// So don't mark the function scope "has local in closure".)
2123
bool notCatch = this->scopeType != ScopeType_Catch && this->scopeType != ScopeType_CatchParamPattern;

‎lib/Runtime/ByteCode/Scope.h‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@ class Scope
3838
BYTE hasCrossScopeFuncAssignment : 1;
3939
BYTE hasDuplicateFormals : 1;
4040
BYTE canMergeWithBodyScope : 1;
41+
BYTE hasLocalInClosure : 1;
4142
public:
4243
#if DBG
4344
BYTE isRestored : 1;
@@ -54,6 +55,7 @@ class Scope
5455
hasCrossScopeFuncAssignment(false),
5556
hasDuplicateFormals(false),
5657
canMergeWithBodyScope(true),
58+
hasLocalInClosure(false),
5759
location(Js::Constants::NoRegister),
5860
symbolTable(nullptr),
5961
m_symList(nullptr),
@@ -294,6 +296,8 @@ class Scope
294296
bool GetCanMergeWithBodyScope() const { return canMergeWithBodyScope; }
295297

296298
void SetHasLocalInClosure(bool has);
299+
void SetHasOwnLocalInClosure(bool has) { hasLocalInClosure = has; }
300+
bool GetHasOwnLocalInClosure() const { return hasLocalInClosure; }
297301

298302
bool HasInnerScopeIndex() const { return innerScopeIndex != (uint)-1; }
299303
uint GetInnerScopeIndex() const { return innerScopeIndex; }

‎lib/Runtime/ByteCode/ScopeInfo.cpp‎

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,7 @@ namespace Js
6868
scopeInfo->isCached = (scope->GetFunc()->GetBodyScope() == scope) && scope->GetFunc()->GetHasCachedScope();
6969
scopeInfo->isGlobalEval = scope->GetScopeType() == ScopeType_GlobalEvalBlock;
7070
scopeInfo->canMergeWithBodyScope = scope->GetCanMergeWithBodyScope();
71+
scopeInfo->hasLocalInClosure = scope->GetHasOwnLocalInClosure();
7172

7273
TRACE_BYTECODE(_u("\nSave ScopeInfo: %s parent: %s #symbols: %d %s\n"),
7374
scope->GetFunc()->name, parent->GetDisplayName(), count, scopeInfo->isObject ? _u("isObject") : _u(""));
@@ -206,9 +207,16 @@ namespace Js
206207
#if DBG
207208
if (funcInfo->GetFuncExprScope() && funcInfo->GetFuncExprScope()->GetIsObject())
208209
{
209-
Assert(currentScope->GetEnclosingScope() == funcInfo->GetFuncExprScope() &&
210-
currentScope->GetEnclosingScope()->GetEnclosingScope() ==
211-
(parentFunc->IsGlobalFunction() ? parentFunc->GetGlobalEvalBlockScope() : parentFunc->GetBodyScope()));
210+
if (funcInfo->paramScope && !funcInfo->paramScope->GetCanMergeWithBodyScope())
211+
{
212+
Assert(currentScope->GetEnclosingScope()->GetEnclosingScope() == funcInfo->GetFuncExprScope());
213+
}
214+
else
215+
{
216+
Assert(currentScope->GetEnclosingScope() == funcInfo->GetFuncExprScope() &&
217+
currentScope->GetEnclosingScope()->GetEnclosingScope() ==
218+
(parentFunc->IsGlobalFunction() ? parentFunc->GetGlobalEvalBlockScope() : parentFunc->GetBodyScope()));
219+
}
212220
}
213221
else
214222
{
@@ -262,6 +270,7 @@ namespace Js
262270
{
263271
scope->SetCannotMergeWithBodyScope();
264272
}
273+
scope->SetHasOwnLocalInClosure(this->hasLocalInClosure);
265274
if (parser)
266275
{
267276
scriptContext = parser->GetScriptContext();

‎lib/Runtime/ByteCode/ScopeInfo.h‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@ namespace Js {
4141
BYTE isGlobalEval : 1;
4242
BYTE areNamesCached : 1;
4343
BYTE canMergeWithBodyScope : 1;
44+
BYTE hasLocalInClosure : 1;
4445

4546
Scope *scope;
4647
int scopeId;
@@ -49,7 +50,7 @@ namespace Js {
4950

5051
private:
5152
ScopeInfo(FunctionBody * parent, int symbolCount)
52-
: parent(parent), funcExprScopeInfo(nullptr), paramScopeInfo(nullptr), symbolCount(symbolCount), scope(nullptr), areNamesCached(false), canMergeWithBodyScope(true)
53+
: parent(parent), funcExprScopeInfo(nullptr), paramScopeInfo(nullptr), symbolCount(symbolCount), scope(nullptr), areNamesCached(false), canMergeWithBodyScope(true), hasLocalInClosure(false)
5354
{
5455
}
5556

@@ -193,6 +194,16 @@ namespace Js {
193194
return canMergeWithBodyScope;
194195
}
195196

197+
void SetHasLocalInClosure(bool has)
198+
{
199+
hasLocalInClosure = has;
200+
}
201+
202+
bool GetHasOwnLocalInClosure() const
203+
{
204+
return hasLocalInClosure;
205+
}
206+
196207
static void SaveScopeInfoForDeferParse(ByteCodeGenerator* byteCodeGenerator, FuncInfo* parentFunc, FuncInfo* func);
197208

198209
ScopeInfo *CloneFor(ParseableFunctionInfo *body);

‎lib/Runtime/Language/InterpreterStackFrame.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1407,7 +1407,7 @@ namespace Js
14071407
if (executeFunction->HasScopeObject())
14081408
{
14091409
Js::RegSlot funcExprScopeReg = executeFunction->GetFuncExprScopeRegister();
1410-
if (funcExprScopeReg != Constants::NoRegister)
1410+
if (funcExprScopeReg != Constants::NoRegister && this->paramClosure == nullptr)
14111411
{
14121412
// t0 = NewPseudoScope
14131413
// t1 = LdFrameDisplay t0 env

‎test/es6/default-splitscope.js‎

Lines changed: 48 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,7 @@ var tests = [
7373
}
7474
},
7575
{
76-
name: "Split parameter scope in function expressions with name",
76+
name: "Split parameter scope and function expressions with name",
7777
body: function () {
7878
function f1(a = 10, b = function c() { return a; }) {
7979
assert.areEqual(10, a, "Initial value of parameter in the body scope of the method should be the same as the one in param scope");
@@ -86,7 +86,53 @@ var tests = [
8686
function f2(a = 10, b = function c(recurse = true) { return recurse ? c(false) : a; }) {
8787
return b;
8888
}
89-
assert.areEqual(10, f2()(), "Recursive function expression defined in the param scope captures the formals from the param scope not body scope");
89+
assert.areEqual(10, f2()(), "Recursive function expression defined in the param scope captures the formals from the param scope not body scope");
90+
91+
assert.areEqual(10, f2()(), "Recursive function expression defined in the param scope captures the formals from the param scope not body scope");
92+
93+
var f3 = function f4 (a = function ( ) { b; return f4(20); }, b) {
94+
if (a == 20) {
95+
return 10;
96+
}
97+
return a;
98+
}
99+
assert.areEqual(10, f3()(), "Recursive call to the function from the param scope returns the right value");
100+
101+
var f5 = function f6 (a = function ( ) { b; return f6; }, b) {
102+
if (a == 20) {
103+
return 10;
104+
}
105+
return a;
106+
}
107+
assert.areEqual(10, f5()()(20), "Recursive call to the function from the param scope returns the right value");
108+
109+
var f7 = function f8 (a = function ( ) { b; }, b) {
110+
if (a == 20) {
111+
return 10;
112+
}
113+
var a = function () { return f8(20); };
114+
return a;
115+
}
116+
assert.areEqual(10, f7()(), "Recursive call to the function from the body scope returns the right value");
117+
118+
var f9 = function f10 (a = function ( ) { b; return f10(20); }, b) {
119+
eval("");
120+
if (a == 20) {
121+
return 10;
122+
}
123+
return a;
124+
}
125+
assert.areEqual(10, f9()(), "Recursive call to the function from the param scope returns the right value when eval is there in the body");
126+
127+
var f11 = function f12 (a = function ( ) { b; }, b) {
128+
eval("");
129+
if (a == 20) {
130+
return 10;
131+
}
132+
var a = function () { return f12(20); };
133+
return a;
134+
}
135+
assert.areEqual(10, f11()(), "Recursive call to the function from the body scope returns the right value when eval is there in the body");
90136
}
91137
},
92138
{

0 commit comments

Comments
 (0)