Reflect.construct has the crazy part in the spec. It can replace the `newTarget` dynamically. Since the current JSC has an assumption that `newTarget` is fixed when the derived constructor is defined, it breaks this assumption. For example, Reflect.construct(function () { }, [], Map) does not work (breaks in LLInt layer) because the byte code op_create_this assumes that the newTarget is always the JSFunction (not InternalFunction). I think Reflect.construct is the only place where we can replace the newTarget dynamically. So the easiest way to implement it is adding a special (slow) path to Reflect.construct.
Created attachment 272843 [details] Patch
Comment on attachment 272843 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=272843&action=review > Source/JavaScriptCore/runtime/ReflectObject.cpp:103 > + if (!target.isObject()) > + return JSValue::encode(throwTypeError(exec, ASCIILiteral("Reflect.construct requires the first argument be a constructor"))); > + JSObject* targetObject = asObject(target); > + ConstructData constructData; > + ConstructType constructType = JSC::getConstructData(targetObject, constructData); > + if (constructType == ConstructTypeNone) I think this could just be if (!target.isConstructor()) return JSValue::encode(throwTypeError(exec, ASCIILiteral("Reflect.construct requires the first argument be a constructor"))); > Source/JavaScriptCore/tests/stress/reflect-construct.js:155 > + shouldBe(Reflect.construct(FailedMap, [], Map).__proto__, Map.prototype); > + shouldThrow(() => { > + let map = Reflect.construct(FailedMap, [], Map); > + map.set(20, 30); > + }, `TypeError: Map operation called on non-Map object`); It might also be worthwhile to test Reflect.construct of InternalFunction with another InternalFunction as new.target.
Comment on attachment 272843 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=272843&action=review > Source/JavaScriptCore/runtime/ReflectObject.cpp:94 > +// http://www.ecma-international.org/ecma-262/6.0/#sec-reflect.construct Also, as a note, apparently all the other browsers and internally at Apple we have started using https://tc39.github.io/ecma262/ as the official spec since it is more up to date with the future plans of ECMA script. The github one is also a little bit nicer in my opinion. Although it looks like the implementation for Reflect.construct is the same in both specs. :)
Comment on attachment 272843 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=272843&action=review Thanks! >> Source/JavaScriptCore/runtime/ReflectObject.cpp:94 >> +// http://www.ecma-international.org/ecma-262/6.0/#sec-reflect.construct > > Also, as a note, apparently all the other browsers and internally at Apple we have started using https://tc39.github.io/ecma262/ as the official spec since it is more up to date with the future plans of ECMA script. The github one is also a little bit nicer in my opinion. Although it looks like the implementation for Reflect.construct is the same in both specs. :) Thanks, changed. >> Source/JavaScriptCore/runtime/ReflectObject.cpp:103 >> + if (constructType == ConstructTypeNone) > > I think this could just be > > if (!target.isConstructor()) > return JSValue::encode(throwTypeError(exec, ASCIILiteral("Reflect.construct requires the first argument be a constructor"))); These `constructType` and `constructData` are later used to call `construct(exec, target, constructType, constructData, arguments, newTarget)`. >> Source/JavaScriptCore/tests/stress/reflect-construct.js:155 >> + }, `TypeError: Map operation called on non-Map object`); > > It might also be worthwhile to test Reflect.construct of InternalFunction with another InternalFunction as new.target. Nice! Added.
Created attachment 272853 [details] Patch
Comment on attachment 272853 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=272853&action=review r=me with comments and suggestions > Source/JavaScriptCore/runtime/ReflectObject.cpp:103 > + ConstructType constructType = JSC::getConstructData(asObject(target), constructData); > + if (constructType == ConstructTypeNone) > + return JSValue::encode(throwTypeError(exec, ASCIILiteral("Reflect.construct requires the first argument be a constructor"))); I think it's best to write another version of isConstructor() that take in ConstructData& and ConstructType& so that we still have a common implementation of that function. The current implementation can just call into the new implementation you'll add > Source/JavaScriptCore/runtime/ReflectObject.cpp:125 > + unsigned count = getLength(exec, argumentsObject); > + if (exec->hadException()) > + return JSValue::encode(jsUndefined()); > + > + for (unsigned index = 0; index < count; ++index) { > + JSValue element = argumentsObject->getIndex(exec, index); > + if (exec->hadException()) > + return JSValue::encode(jsUndefined()); > + arguments.append(element); > + } I already added the function createListFromArrayLike to our code base. You should use that here. The code works by taking a lambda so you can build up your list. It also takes a RuntimeTypeMask as the type filter. It might be best to either move the RuntimeTypeMask filter parameter to the last parameter and make it default to all types, or you can create a predefined RuntimeTypeMask called RuntimeTypeMaskAllTypes or something to just pass in here. > Source/JavaScriptCore/tests/stress/reflect-construct.js:1 > +function shouldBe(actual, expected) { It would be cool to add a test with Proxy.
Comment on attachment 272853 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=272853&action=review Thanks! >> Source/JavaScriptCore/runtime/ReflectObject.cpp:103 >> + return JSValue::encode(throwTypeError(exec, ASCIILiteral("Reflect.construct requires the first argument be a constructor"))); > > I think it's best to write another version of isConstructor() that take in ConstructData& and ConstructType& so that > we still have a common implementation of that function. The current implementation can just call into the new implementation > you'll add Sounds nice. To allow forward declaration of ConstructType and ConstructData, I'll change ConstructType to enum class. >> Source/JavaScriptCore/runtime/ReflectObject.cpp:125 >> + } > > I already added the function createListFromArrayLike to our code base. You should use that here. > The code works by taking a lambda so you can build up your list. It also takes a RuntimeTypeMask > as the type filter. It might be best to either move the RuntimeTypeMask filter parameter > to the last parameter and make it default to all types, or you can create a predefined RuntimeTypeMask > called RuntimeTypeMaskAllTypes or something to just pass in here. Nice. I'll change it so :) >> Source/JavaScriptCore/tests/stress/reflect-construct.js:1 >> +function shouldBe(actual, expected) { > > It would be cool to add a test with Proxy. Nice, I'll add it.
Committed r197614: <http://trac.webkit.org/changeset/197614>
I'll check the build.
I'll update the binding test results...
Ah, no problem. Sorry.
WinCairo build seems failed. But it seems that it does not rebuild some cpps correctly. So clean build fixes the probelm.