Skip to content

Commit

Permalink
[1.8>1.9] [MERGE #4629 @boingoing] OS#15334058: Fix `class extends nu…
Browse files Browse the repository at this point in the history 
…ll` and `this` access for super property reference

Merge pull request #4629 from boingoing:ClassExtendsNull

We were supporting some old version of the spec language around classes which extend from null. Instead of leaving `this` undeclared, we were constructing an object and assigning it the `this` binding. This meant we were not throwing ReferenceErrors on access to `this` when we should have been.

Also fixed the case where a super property reference failed to access `this` resulting in trying to load a property from null.

Fixes:
https://microsoft.visualstudio.com/OS/_workitems/edit/15334058
  • Loading branch information
@boingoing
boingoing committed Feb 1, 2018
2 parents 8c0764e + 5ebaf10 commit 4115143
Show file tree
Hide file tree
Showing 4 changed files with 45 additions and 76 deletions.
51 changes: 20 additions & 31 deletions lib/Runtime/ByteCode/ByteCodeEmitter.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2134,31 +2134,20 @@ void ByteCodeGenerator::LoadThisObject(FuncInfo *funcInfo, bool thisLoadedFromPa

if (this->scriptContext->GetConfig()->IsES6ClassAndExtendsEnabled() && funcInfo->IsClassConstructor())
{
// Derived class constructors initialize 'this' to be Undecl except "extends null" cases
// Derived class constructors initialize 'this' to be Undecl
// - we'll check this value during a super call and during 'this' access
//
// Base class constructors or "extends null" cases initialize 'this' to a new object using new.target
// Base class constructors initialize 'this' to a new object using new.target
if (funcInfo->IsBaseClassConstructor())
{
EmitBaseClassConstructorThisObject(funcInfo);
Symbol* newTargetSym = funcInfo->GetNewTargetSymbol();
Assert(newTargetSym);

this->Writer()->Reg2(Js::OpCode::NewScObjectNoCtorFull, thisSym->GetLocation(), newTargetSym->GetLocation());
}
else
{
Js::ByteCodeLabel thisLabel = this->Writer()->DefineLabel();
Js::ByteCodeLabel skipLabel = this->Writer()->DefineLabel();

Js::RegSlot tmpReg = funcInfo->AcquireTmpRegister();
this->Writer()->Reg1(Js::OpCode::LdFuncObj, tmpReg);
this->Writer()->BrReg1(Js::OpCode::BrOnBaseConstructorKind, thisLabel, tmpReg); // branch when [[ConstructorKind]]=="base"
funcInfo->ReleaseTmpRegister(tmpReg);

this->m_writer.Reg1(Js::OpCode::InitUndecl, thisSym->GetLocation()); // not "extends null" case
this->Writer()->Br(Js::OpCode::Br, skipLabel);

this->Writer()->MarkLabel(thisLabel);
EmitBaseClassConstructorThisObject(funcInfo); // "extends null" case

this->Writer()->MarkLabel(skipLabel);
this->m_writer.Reg1(Js::OpCode::InitUndecl, thisSym->GetLocation());
}
}
else if (!funcInfo->IsGlobalFunction())
Expand Down Expand Up @@ -2207,11 +2196,11 @@ void ByteCodeGenerator::LoadSuperConstructorObject(FuncInfo *funcInfo)
{
Symbol* superConstructorSym = funcInfo->GetSuperConstructorSymbol();
Assert(superConstructorSym);
Assert(!funcInfo->IsLambda());
Assert(!funcInfo->IsLambda());
Assert(funcInfo->IsDerivedClassConstructor());

m_writer.Reg1(Js::OpCode::LdFuncObj, superConstructorSym->GetLocation());
}
m_writer.Reg1(Js::OpCode::LdFuncObj, superConstructorSym->GetLocation());
}

void ByteCodeGenerator::LoadSuperObject(FuncInfo *funcInfo)
{
Expand Down Expand Up @@ -2338,11 +2327,6 @@ void ByteCodeGenerator::EmitClassConstructorEndCode(FuncInfo *funcInfo)
}
}

void ByteCodeGenerator::EmitBaseClassConstructorThisObject(FuncInfo *funcInfo)
{
this->Writer()->Reg2(Js::OpCode::NewScObjectNoCtorFull, funcInfo->GetThisSymbol()->GetLocation(), funcInfo->GetNewTargetSymbol()->GetLocation());
}

void ByteCodeGenerator::EmitThis(FuncInfo *funcInfo, Js::RegSlot lhsLocation, Js::RegSlot fromRegister)
{
if (funcInfo->byteCodeFunction->GetIsStrictMode() && !funcInfo->IsGlobalFunction() && !funcInfo->IsLambda())
Expand Down Expand Up @@ -4916,14 +4900,14 @@ void ByteCodeGenerator::EmitPropLoadThis(Js::RegSlot lhsLocation, ParseNode *pno
}
else
{
this->EmitPropLoad(lhsLocation, pnode->sxPid.sym, pnode->sxPid.pid, funcInfo, true);
this->EmitPropLoad(lhsLocation, pnode->sxPid.sym, pnode->sxPid.pid, funcInfo, true);

if ((!sym || sym->GetNeedDeclaration()) && chkUndecl)
{
this->Writer()->Reg1(Js::OpCode::ChkUndecl, lhsLocation);
if ((!sym || sym->GetNeedDeclaration()) && chkUndecl)
{
this->Writer()->Reg1(Js::OpCode::ChkUndecl, lhsLocation);
}
}
}
}

void ByteCodeGenerator::EmitPropStoreForSpecialSymbol(Js::RegSlot rhsLocation, Symbol *sym, IdentPtr pid, FuncInfo *funcInfo, bool init)
{
Expand Down Expand Up @@ -6804,6 +6788,11 @@ void EmitAssignment(

if (ByteCodeGenerator::IsSuper(lhs->sxBin.pnode1))
{
// We need to emit the 'this' node for the super reference even if we aren't planning to use the 'this' value.
// This is because we might be in a derived class constructor where we haven't yet called super() to bind the 'this' value.
// See ecma262 abstract operation 'MakeSuperPropertyReference'
Emit(lhs->sxSuperReference.pnodeThis, byteCodeGenerator, funcInfo, false);
funcInfo->ReleaseLoc(lhs->sxSuperReference.pnodeThis);
targetLocation = byteCodeGenerator->EmitLdObjProto(Js::OpCode::LdHomeObjProto, targetLocation, funcInfo);
}

Expand Down
5 changes: 0 additions & 5 deletions lib/Runtime/Language/JavascriptOperators.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7087,11 +7087,6 @@ namespace Js
ctorProtoObj->EnsureProperty(Js::PropertyIds::constructor);
ctorProtoObj->SetEnumerable(Js::PropertyIds::constructor, FALSE);

if (ScriptFunctionBase::Is(constructor))
{
ScriptFunctionBase::FromVar(constructor)->GetFunctionInfo()->SetBaseConstructorKind();
}

break;
}

Expand Down
25 changes: 0 additions & 25 deletions test/Basics/SpecialSymbolCapture.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -516,31 +516,6 @@ var tests = [
}
}
assert.throws(() => new DerivedSuper(), TypeError, "Class derived from null can't make a super call", "Function is not a constructor");

class DerivedEmpty extends null {
constructor() { }
}
assert.areEqual(DerivedEmpty, new DerivedEmpty().constructor, "Default instance for 'extends null' case is a real instance of the derived class");

var called = false;
class DerivedVerifyNewTarget extends null {
constructor() {
assert.areEqual(DerivedVerifyNewTarget, new.target, "Derived class called as new expression gets new.target");
called = true;
}
}
assert.areEqual(DerivedVerifyNewTarget, new DerivedVerifyNewTarget().constructor, "Default instance for 'extends null' case is a real instance of the derived class");
assert.isTrue(called, "Constructor was actually called");

called = false;
class DerivedVerifyThis extends null {
constructor() {
assert.areEqual(DerivedVerifyThis, this.constructor, "Derived from null class called as new expression gets this instance of the derived class");
called = true;
}
}
assert.areEqual(DerivedVerifyThis, new DerivedVerifyThis().constructor, "Default instance for 'extends null' case is a real instance of the derived class");
assert.isTrue(called, "Constructor was actually called");
}
},
{
Expand Down
40 changes: 25 additions & 15 deletions test/es6/ES6Class_BaseClassConstruction.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -108,16 +108,13 @@ var tests = [
}
},
{
name: "Class that extends null binds this in constructor",
name: "Class that extends null doesn't bind 'this' implicitly",
body: function () {
var thisVal;
class B extends null {
constructor() { thisVal = this; }
constructor() { }
}

var b = new B();
assert.areEqual(true, b instanceof B);
assert.areEqual(thisVal, b);
assert.throws(() => new B(), ReferenceError, "implicit return of 'this' throws", "Use before declaration");
}
},
{
Expand All @@ -140,27 +137,40 @@ var tests = [
}
},
{
name: "Class that extends null with implicit return in constructor",
name: "Class that extends null with explicit return in constructor",
body: function () {
class A extends null {
constructor() {}
constructor() { return {}; }
}

var a;
assert.doesNotThrow(()=>{a = new A()});
assert.areEqual(A.prototype, Object.getPrototypeOf(a));
assert.areEqual(Object.prototype, Object.getPrototypeOf(a));
}
},
{
name: "Class that extends null with explicit return in constructor",
name: "Class that extends null with super references",
body: function () {
class A extends null {
constructor() { return {}; }
constructor() { super['prop'] = 'something'; return {}; }
}

var a;
assert.doesNotThrow(()=>{a = new A()});
assert.areEqual(Object.prototype, Object.getPrototypeOf(a));
assert.throws(() => new A(), ReferenceError, "super reference loads 'this' and throws if it's undecl ", "Use before declaration");

var prop = 'prop';
class B extends null {
constructor() { super[prop] = 'something'; return {}; }
}
assert.throws(() => new B(), ReferenceError, "super reference loads 'this' and throws if it's undecl ", "Use before declaration");

class C extends null {
constructor() { super['prop']; return {}; }
}
assert.throws(() => new C(), ReferenceError, "super reference loads 'this' and throws if it's undecl ", "Use before declaration");

class D extends null {
constructor() { super[prop]; return {}; }
}
assert.throws(() => new D(), ReferenceError, "super reference loads 'this' and throws if it's undecl ", "Use before declaration");
}
},
];
Expand Down

0 comments on commit 4115143

Please sign in to comment.