Skip to content

Commit 6ab8c30

Browse files
committed
[MERGE #5147 @rhuanjl] Fix errors in construction of bound Functions
Merge pull request #5147 from rhuanjl:boundProxy Revisiting boundProxies found what I hope are the right fixes this time. This PR addresses the following issues: 1. Constructing bound Proxy would call construct trap twice or if there wasn't a construct trap targetFunction would be called twice, 2. Reflect.construct in construct trap of bound Proxy would attempt to construct "this" and error as it's not a constructor (issue 4918) 3. Constructing a bound function via Reflect with a newTarget would ignore the newTarget. Added test cases for these points and some related patterns that do work but weren't being tested and which some other ways of fixing points 1-3 would break. Also corrected the baseline for proxytest9.js which was testing the incorrect behaviour of construct traps for bound proxies being called twice. The interaction of bound functions and proxies is complex enough that I'm afraid I'm confident that there'll be one or more bugs left after this, but this should be a reasonable improvement from the current state. @sethbrenith I borrowed your test case from 4918 as the starting point for the cases here, I hope that's ok. Note I split it into two as it highlighted 2 different issues. Fixes: #4918 Fixes: #5151
2 parents 8ec116d + e4e50ff commit 6ab8c30

File tree

5 files changed

+140
-16
lines changed

5 files changed

+140
-16
lines changed

lib/Runtime/Library/BoundFunction.cpp

Lines changed: 21 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -127,7 +127,7 @@ namespace Js
127127
}
128128

129129
BoundFunction *boundFunction = (BoundFunction *) function;
130-
Var targetFunction = boundFunction->targetFunction;
130+
RecyclableObject *targetFunction = boundFunction->targetFunction;
131131

132132
//
133133
// var o = new boundFunction()
@@ -136,16 +136,25 @@ namespace Js
136136
Var newVarInstance = nullptr;
137137
if (callInfo.Flags & CallFlags_New)
138138
{
139-
if (JavascriptProxy::Is(targetFunction))
140-
{
141-
JavascriptProxy* proxy = JavascriptProxy::FromVar(targetFunction);
142-
Arguments proxyArgs(CallInfo(CallFlags_New, 1), &targetFunction);
143-
args.Values[0] = newVarInstance = proxy->ConstructorTrap(proxyArgs, scriptContext, 0);
144-
}
145-
else
146-
{
147-
args.Values[0] = newVarInstance = JavascriptOperators::NewScObjectNoCtor(targetFunction, scriptContext);
148-
}
139+
if (args.HasNewTarget())
140+
{
141+
// target has an overriden new target make a new object from the newTarget
142+
Var newTargetVar = args.GetNewTarget();
143+
AssertOrFailFastMsg(JavascriptOperators::IsConstructor(newTargetVar), "newTarget must be a constructor");
144+
RecyclableObject* newTarget = RecyclableObject::UnsafeFromVar(newTargetVar);
145+
args.Values[0] = newVarInstance = JavascriptOperators::CreateFromConstructor(newTarget, scriptContext);
146+
}
147+
else if (!JavascriptProxy::Is(targetFunction))
148+
{
149+
// no new target and target is not a proxy can make a new object in a "normal" way
150+
args.Values[0] = newVarInstance = JavascriptOperators::CreateFromConstructor(targetFunction, scriptContext);
151+
}
152+
else
153+
{
154+
// target is a proxy without an overriden new target
155+
// give nullptr - FunctionCallTrap will make a new object
156+
args.Values[0] = newVarInstance;
157+
}
149158
}
150159

151160
Js::Arguments actualArgs = args;
@@ -206,9 +215,8 @@ namespace Js
206215
}
207216
}
208217

209-
RecyclableObject* actualFunction = RecyclableObject::FromVar(targetFunction);
210218
// Number of arguments are allowed to be more than Constants::MaxAllowedArgs in runtime. Need to use the larger argcount logic for this call.
211-
Var aReturnValue = JavascriptFunction::CallFunction<true>(actualFunction, actualFunction->GetEntryPoint(), actualArgs, /* useLargeArgCount */ true);
219+
Var aReturnValue = JavascriptFunction::CallFunction<true>(targetFunction, targetFunction->GetEntryPoint(), actualArgs, /* useLargeArgCount */ true);
212220

213221
//
214222
// [[Construct]] and call returned a non-object

test/UnitTestFramework/UnitTestFramework.js

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626
// example: testRunner.LoadModule(source, 'samethread', false);
2727
//
2828
// How to use assert:
29+
// assert.strictEqual(expected, actual, "those two should be strictly equal (i.e. === comparison)");
2930
// assert.areEqual(expected, actual, "those two should be equal (i.e. deep equality of objects using ===)");
3031
// assert.areNotEqual(expected, actual, "those two should NOT be equal");
3132
// assert.areAlmostEqual(expected, actual, "those two should be almost equal, numerically (allows difference by epsilon)");
@@ -407,6 +408,10 @@ var assert = function assert() {
407408
}
408409

409410
return {
411+
strictEqual: function strictEqual(expected, actual, message) {
412+
validate(expected === actual, "strictEqual", message);
413+
},
414+
410415
areEqual: function areEqual(expected, actual, message) {
411416
/// <summary>
412417
/// IMPORTANT: NaN compares equal.<br/>

test/es6/boundConstruction.js

Lines changed: 108 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,108 @@
1+
//-------------------------------------------------------------------------------------------------------
2+
// Copyright (C) Microsoft. All rights reserved.
3+
// Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
4+
//-------------------------------------------------------------------------------------------------------
5+
6+
WScript.LoadScriptFile("..\\UnitTestFramework\\UnitTestFramework.js");
7+
8+
//setup code
9+
let constructionCount = 0;
10+
let functionCallCount = 0;
11+
function a(arg1, arg2) {
12+
this[arg1] = arg2;
13+
++functionCallCount;
14+
}
15+
16+
const trapped = new Proxy(a, {
17+
construct: function (x, y, z) {
18+
++constructionCount;
19+
return Reflect.construct(x, y, z);
20+
}
21+
});
22+
23+
const noTrap = new Proxy(a, {});
24+
25+
const boundObject = {};
26+
27+
const boundFunc = a.bind(boundObject, "prop-name");
28+
29+
const boundTrapped = trapped.bind(boundObject, "prop-name");
30+
const boundUnTrapped = noTrap.bind(boundObject, "prop-name");
31+
32+
const tests = [
33+
{
34+
name : "Construct trapped bound proxy with new",
35+
body : function() {
36+
constructionCount = 0;
37+
functionCallCount = 0;
38+
const obj = new boundTrapped("prop-value");
39+
assert.areNotEqual(boundObject, obj, "bound function should ignore bound this when constructing");
40+
assert.areEqual("prop-value", obj["prop-name"], "bound function should keep bound arguments when constructing");
41+
assert.areEqual(1, constructionCount, "bound proxy should be constructed once");
42+
assert.areEqual(1, functionCallCount, "bound proxy function should be called once");
43+
assert.strictEqual(a.prototype, obj.__proto__, "constructed object should be instance of original function");
44+
}
45+
},
46+
{
47+
name : "Construct trapped bound proxy with Reflect",
48+
body : function() {
49+
class newTarget {}
50+
constructionCount = 0;
51+
functionCallCount = 0;
52+
const obj = Reflect.construct(boundTrapped, ["prop-value-2"], newTarget);
53+
assert.areNotEqual(boundObject, obj, "bound function should ignore bound this when constructing");
54+
assert.strictEqual(newTarget.prototype, obj.__proto__, "bound function should use explicit newTarget if provided");
55+
assert.areEqual("prop-value-2", obj["prop-name"], "bound function should keep bound arguments when constructing");
56+
assert.areEqual(1, constructionCount, "bound proxy should be constructed once");
57+
assert.areEqual(1, functionCallCount, "bound proxy function should be called once");
58+
}
59+
},
60+
{
61+
name : "Construct bound proxy with new",
62+
body : function() {
63+
functionCallCount = 0;
64+
const obj = new boundUnTrapped("prop-value");
65+
assert.areNotEqual(boundObject, obj, "bound function should ignore bound this when constructing");
66+
assert.areEqual("prop-value", obj["prop-name"], "bound function should keep bound arguments when constructing");
67+
assert.areEqual(1, functionCallCount, "bound proxy function should be called once");
68+
assert.strictEqual(a.prototype, obj.__proto__, "constructed object should be instance of original function");
69+
}
70+
},
71+
{
72+
name : "Construct bound proxy with Reflect",
73+
body : function() {
74+
class newTarget {}
75+
functionCallCount = 0;
76+
const obj = Reflect.construct(boundUnTrapped, ["prop-value-2"], newTarget);
77+
assert.areNotEqual(boundObject, obj, "bound function should ignore bound this when constructing");
78+
assert.strictEqual(newTarget.prototype, obj.__proto__, "bound function should use explicit newTarget if provided");
79+
assert.areEqual("prop-value-2", obj["prop-name"], "bound function should keep bound arguments when constructing");
80+
assert.areEqual(1, functionCallCount, "bound proxy function should be called once");
81+
}
82+
},
83+
{
84+
name : "Construct bound function with Reflect",
85+
body : function() {
86+
class newTarget {}
87+
functionCallCount = 0;
88+
const obj = Reflect.construct(boundFunc, ["prop-value-2"], newTarget);
89+
assert.areNotEqual(boundObject, obj, "bound function should ignore bound this when constructing");
90+
assert.strictEqual(newTarget.prototype, obj.__proto__, "bound function should use explicit newTarget if provided");
91+
assert.areEqual("prop-value-2", obj["prop-name"], "bound function should keep bound arguments when constructing");
92+
assert.areEqual(1, functionCallCount, "bound function should be called once");
93+
}
94+
},
95+
{
96+
name : "Construct bound function with new",
97+
body : function() {
98+
functionCallCount = 0;
99+
const obj = new boundFunc("prop-value-2");
100+
assert.areNotEqual(boundObject, obj, "bound function should ignore bound this when constructing");
101+
assert.strictEqual(a.prototype, obj.__proto__, "bound function should use explicit newTarget if provided");
102+
assert.areEqual("prop-value-2", obj["prop-name"], "bound function should keep bound arguments when constructing");
103+
assert.areEqual(1, functionCallCount, "bound function should be called once");
104+
}
105+
}
106+
];
107+
108+
testRunner.runTests(tests, { verbose: WScript.Arguments[0] != "summary" });

test/es6/proxytest9.baseline

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -77,9 +77,6 @@ constructor trap
7777
constructor trap
7878
get trap prototype
7979
constructor trap
80-
constructor trap
81-
get trap prototype
82-
constructor trap
8380
true
8481
get trap m
8582
def

test/es6/rlexe.xml

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -567,6 +567,12 @@
567567
<compile-flags>-ES6Classes -args summary -endargs</compile-flags>
568568
</default>
569569
</test>
570+
<test>
571+
<default>
572+
<files>boundConstruction.js</files>
573+
<compile-flags>-args summary -endargs</compile-flags>
574+
</default>
575+
</test>
570576
<test>
571577
<default>
572578
<files>CrossContextSpreadfunctionCall.js</files>

0 commit comments

Comments
 (0)