[MERGE #5218 @sethbrenith] Fix construction of bound classes

sethbrenith · sethbrenith · commit df8f38b913d7 · 2018-05-24T09:02:33.000-07:00
Merge pull request #5218 from sethbrenith:user/sethb/bound-class The calling convention for class constructors is totally different than ordinary functions: they expect newTarget in the arguments 0 slot, where "this" usually goes. Recent changes to fix the construction of bound functions regressed the construction of bound classes. This change adds support (and tests) for the class case. Fixes OS:17555846 Fixes OS:17554002
diff --git a/lib/Runtime/Library/BoundFunction.cpp b/lib/Runtime/Library/BoundFunction.cpp
@@ -142,12 +142,25 @@ namespace Js
                 Var newTargetVar = args.GetNewTarget();
                 AssertOrFailFastMsg(JavascriptOperators::IsConstructor(newTargetVar), "newTarget must be a constructor");
                 RecyclableObject* newTarget = RecyclableObject::UnsafeFromVar(newTargetVar);
-                args.Values[0] = newVarInstance = JavascriptOperators::CreateFromConstructor(newTarget, scriptContext);
+
+                // Class constructors expect newTarget to be in args slot 0 (usually "this"),
+                // because "this" is not constructed until we reach the most-super superclass.
+                FunctionInfo* functionInfo = JavascriptOperators::GetConstructorFunctionInfo(targetFunction, scriptContext);
+                if (functionInfo && functionInfo->IsClassConstructor())
+                {
+                    args.Values[0] = newVarInstance = newTarget;
+                }
+                else
+                {
+                    args.Values[0] = newVarInstance = JavascriptOperators::CreateFromConstructor(newTarget, scriptContext);
+                }
             }
             else if (!JavascriptProxy::Is(targetFunction))
             {
-                // no new target and target is not a proxy can make a new object in a "normal" way
-                args.Values[0] = newVarInstance = JavascriptOperators::CreateFromConstructor(targetFunction, scriptContext);
+                // No new target and target is not a proxy can make a new object in a "normal" way.
+                // NewScObjectNoCtor will either construct an object or return targetFunction depending
+                // on whether targetFunction is a class constructor.
+                args.Values[0] = newVarInstance = JavascriptOperators::NewScObjectNoCtor(targetFunction, scriptContext);
             }
             else
             {
diff --git a/test/es6/boundConstruction.js b/test/es6/boundConstruction.js
@@ -8,10 +8,22 @@ WScript.LoadScriptFile("..\\UnitTestFramework\\UnitTestFramework.js");
 //setup code
 let constructionCount = 0;
 let functionCallCount = 0;
+let classConstructorCount = 0;
 function a(arg1, arg2) {
     this[arg1] = arg2;
+    this.localVal = this.protoVal;
     ++functionCallCount;
 }
+a.prototype.protoVal = 1;
+
+class A {
+    constructor(arg1, arg2) {
+        this[arg1] = arg2;
+        this.localVal = this.protoVal;
+        ++classConstructorCount;
+    }
+}
+A.prototype.protoVal = 2;
 
 const trapped = new Proxy(a, {
     construct: function (x, y, z) {
@@ -25,84 +37,140 @@ const noTrap = new Proxy(a, {});
 const boundObject = {};
 
 const boundFunc = a.bind(boundObject, "prop-name");
+boundFunc.prototype = {}; // so we can extend from it
+const boundClass = A.bind(boundObject, "prop-name");
+boundClass.prototype = {};
 
 const boundTrapped = trapped.bind(boundObject, "prop-name");
 const boundUnTrapped = noTrap.bind(boundObject, "prop-name");
 
+class newTarget {}
+newTarget.prototype.protoVal = 3;
+
+class ExtendsBoundFunc extends boundFunc {}
+ExtendsBoundFunc.prototype.protoVal = 4;
+class ExtendsBoundClass extends boundClass {}
+ExtendsBoundClass.prototype.protoVal = 5;
+
+class ExtendsFunc extends a {}
+ExtendsFunc.prototype.protoVal = 6;
+class ExtendsClass extends A {}
+ExtendsClass.prototype.protoVal = 7;
+
+const boundClassExtendsFunc = ExtendsFunc.bind(boundObject, "prop-name");
+const boundClassExtendsClass = ExtendsClass.bind(boundObject, "prop-name");
+
+function test(ctor, useReflect, expectedPrototype, expectedConstructionCount, expectedFunctionCallCount, expectedClassConstructorCount) {
+    constructionCount = 0;
+    functionCallCount = 0;
+    classConstructorCount = 0;
+    const obj = useReflect ? Reflect.construct(ctor, ["prop-value"], newTarget) : new ctor("prop-value");
+    assert.areNotEqual(boundObject, obj, "bound function should ignore bound this when constructing");
+    assert.areEqual("prop-value", obj["prop-name"], "bound function should keep bound arguments when constructing");
+    assert.areEqual(expectedConstructionCount, constructionCount, `proxy construct trap should be called ${expectedConstructionCount} times`);
+    assert.areEqual(expectedFunctionCallCount, functionCallCount, `base function-style constructor should be called ${expectedFunctionCallCount} times`);
+    assert.areEqual(expectedClassConstructorCount, classConstructorCount, `base class constructor should be called ${expectedClassConstructorCount} times`);
+    assert.strictEqual(expectedPrototype.prototype, obj.__proto__, useReflect ? "bound function should use explicit newTarget if provided" : "constructed object should be instance of original function");
+    assert.areEqual(expectedPrototype.prototype.protoVal, obj.localVal, "prototype should be available during construction");
+}
+
 const tests = [
     {
         name : "Construct trapped bound proxy with new",
         body : function() {
-            constructionCount = 0;
-            functionCallCount = 0;
-            const obj = new boundTrapped("prop-value");
-            assert.areNotEqual(boundObject, obj, "bound function should ignore bound this when constructing");
-            assert.areEqual("prop-value", obj["prop-name"], "bound function should keep bound arguments when constructing");
-            assert.areEqual(1, constructionCount, "bound proxy should be constructed once");
-            assert.areEqual(1, functionCallCount, "bound proxy function should be called once");
-            assert.strictEqual(a.prototype, obj.__proto__, "constructed object should be instance of original function");
+            test(boundTrapped, false, a, 1, 1, 0);
         }
     },
     {
         name : "Construct trapped bound proxy with Reflect",
         body : function() {
-            class newTarget {}
-            constructionCount = 0;
-            functionCallCount = 0;
-            const obj = Reflect.construct(boundTrapped, ["prop-value-2"], newTarget);
-            assert.areNotEqual(boundObject, obj, "bound function should ignore bound this when constructing");
-            assert.strictEqual(newTarget.prototype, obj.__proto__, "bound function should use explicit newTarget if provided");
-            assert.areEqual("prop-value-2", obj["prop-name"], "bound function should keep bound arguments when constructing");
-            assert.areEqual(1, constructionCount, "bound proxy should be constructed once");
-            assert.areEqual(1, functionCallCount, "bound proxy function should be called once");
+            test(boundTrapped, true, newTarget, 1, 1, 0);
         }
     },
     {
         name : "Construct bound proxy with new",
         body : function() {
-            functionCallCount = 0;
-            const obj = new boundUnTrapped("prop-value");
-            assert.areNotEqual(boundObject, obj, "bound function should ignore bound this when constructing");
-            assert.areEqual("prop-value", obj["prop-name"], "bound function should keep bound arguments when constructing");
-            assert.areEqual(1, functionCallCount, "bound proxy function should be called once");
-            assert.strictEqual(a.prototype, obj.__proto__, "constructed object should be instance of original function");
+            test(boundUnTrapped, false, a, 0, 1, 0);
         }
     },
     {
         name : "Construct bound proxy with Reflect",
         body : function() {
-            class newTarget {}
-            functionCallCount = 0;
-            const obj = Reflect.construct(boundUnTrapped, ["prop-value-2"], newTarget);
-            assert.areNotEqual(boundObject, obj, "bound function should ignore bound this when constructing");
-            assert.strictEqual(newTarget.prototype, obj.__proto__, "bound function should use explicit newTarget if provided");
-            assert.areEqual("prop-value-2", obj["prop-name"], "bound function should keep bound arguments when constructing");
-            assert.areEqual(1, functionCallCount, "bound proxy function should be called once");
+            test(boundUnTrapped, true, newTarget, 0, 1, 0);
         }
     },
     {
         name : "Construct bound function with Reflect",
         body : function() {
-            class newTarget {}
-            functionCallCount = 0;
-            const obj = Reflect.construct(boundFunc, ["prop-value-2"], newTarget);
-            assert.areNotEqual(boundObject, obj, "bound function should ignore bound this when constructing");
-            assert.strictEqual(newTarget.prototype, obj.__proto__, "bound function should use explicit newTarget if provided");
-            assert.areEqual("prop-value-2", obj["prop-name"], "bound function should keep bound arguments when constructing");
-            assert.areEqual(1, functionCallCount, "bound function should be called once");
+            test(boundFunc, true, newTarget, 0, 1, 0);
         }
     },
     {
         name : "Construct bound function with new",
         body : function() {
-            functionCallCount = 0;
-            const obj = new boundFunc("prop-value-2");
-            assert.areNotEqual(boundObject, obj, "bound function should ignore bound this when constructing");
-            assert.strictEqual(a.prototype, obj.__proto__, "bound function should use explicit newTarget if provided");
-            assert.areEqual("prop-value-2", obj["prop-name"], "bound function should keep bound arguments when constructing");
-            assert.areEqual(1, functionCallCount, "bound function should be called once");
+            test(boundFunc, false, a, 0, 1, 0);
+        }
+    },
+    {
+        name : "Construct bound class with new",
+        body : function() {
+            test(boundClass, false, A, 0, 0, 1);
+        }
+    },
+    {
+        name : "Construct bound class with Reflect",
+        body : function() {
+            test(boundClass, true, newTarget, 0, 0, 1);
+        }
+    },
+    {
+        name : "Construct class extending bound function with new",
+        body : function() {
+            test(ExtendsBoundFunc, false, ExtendsBoundFunc, 0, 1, 0);
+        }
+    },
+    {
+        name : "Construct class extending bound function with Reflect",
+        body : function() {
+            test(ExtendsBoundFunc, true, newTarget, 0, 1, 0);
+        }
+    },
+    {
+        name : "Construct class extending bound class with new",
+        body : function() {
+            test(ExtendsBoundClass, false, ExtendsBoundClass, 0, 0, 1);
+        }
+    },
+    {
+        name : "Construct class extending bound class with Reflect",
+        body : function() {
+            test(ExtendsBoundClass, true, newTarget, 0, 0, 1);
+        }
+    },
+    {
+        name : "Construct bound class that extends a function with new",
+        body : function() {
+            test(boundClassExtendsFunc, false, ExtendsFunc, 0, 1, 0);
+        }
+    },
+    {
+        name : "Construct bound class that extends a function with Reflect",
+        body : function() {
+            test(boundClassExtendsFunc, true, newTarget, 0, 1, 0);
+        }
+    },
+    {
+        name : "Construct bound class that extends another class with new",
+        body : function() {
+            test(boundClassExtendsClass, false, ExtendsClass, 0, 0, 1);
+        }
+    },
+    {
+        name : "Construct bound class that extends another class with Reflect",
+        body : function() {
+            test(boundClassExtendsClass, true, newTarget, 0, 0, 1);
         }
     }
 ];
 
-testRunner.runTests(tests, { verbose: WScript.Arguments[0] != "summary" }); 
+testRunner.runTests(tests, { verbose: WScript.Arguments[0] != "summary" });
