[1.8>1.9] [MERGE #4629 @boingoing] OS#15334058: Fix class extends null 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

Author: boingoing

Diff for: lib/Runtime/ByteCode/ByteCodeEmitter.cpp

@@ -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())
@@ -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)
 {
@@ -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())
@@ -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)
 {
@@ -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);
         }
 

Diff for: lib/Runtime/Language/JavascriptOperators.cpp

@@ -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;
                 }
 

Diff for: test/Basics/SpecialSymbolCapture.js

@@ -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");
         }
     },
     {

Diff for: test/es6/ES6Class_BaseClassConstruction.js

@@ -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");
         }
     },
     {
@@ -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");
         }
     },
 ];