Bug 147330 – [ES6] Support Reflect.construct

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Created attachment 272843 [details]
Patch

[Keith Miller]

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.

[Keith Miller]

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. :)

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Created attachment 272853 [details]
Patch

[Saam Barati]

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.

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Committed r197614: <http://trac.webkit.org/changeset/197614>

[Yusuke Suzuki]

I'll check the build.

[Yusuke Suzuki]

I'll update the binding test results...

[Yusuke Suzuki]

Ah, no problem. Sorry.

[Yusuke Suzuki]

WinCairo build seems failed. But it seems that it does not rebuild some cpps correctly. So clean build fixes the probelm.