Bug 147330 - [ES6] Support Reflect.construct
Summary: [ES6] Support Reflect.construct
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-07-27 12:07 PDT by Yusuke Suzuki
Modified: 2016-03-05 11:48 PST (History)
8 users (show)

See Also:


Attachments
Patch (15.65 KB, patch)
2016-03-04 01:53 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (20.98 KB, patch)
2016-03-04 07:15 PST, Yusuke Suzuki
sbarati: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2015-07-27 12:07:30 PDT
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.
Comment 1 Yusuke Suzuki 2016-03-04 01:53:09 PST
Created attachment 272843 [details]
Patch
Comment 2 Keith Miller 2016-03-04 05:34:17 PST
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 3 Keith Miller 2016-03-04 05:58:29 PST
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 4 Yusuke Suzuki 2016-03-04 07:04:12 PST
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.
Comment 5 Yusuke Suzuki 2016-03-04 07:15:56 PST
Created attachment 272853 [details]
Patch
Comment 6 Saam Barati 2016-03-04 10:24:57 PST
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 7 Yusuke Suzuki 2016-03-05 07:38:27 PST
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.
Comment 8 Yusuke Suzuki 2016-03-05 09:01:00 PST
Committed r197614: <http://trac.webkit.org/changeset/197614>
Comment 9 Yusuke Suzuki 2016-03-05 11:39:08 PST
I'll check the build.
Comment 10 Yusuke Suzuki 2016-03-05 11:40:00 PST
I'll update the binding test results...
Comment 11 Yusuke Suzuki 2016-03-05 11:42:16 PST
Ah, no problem. Sorry.
Comment 12 Yusuke Suzuki 2016-03-05 11:48:49 PST
WinCairo build seems failed. But it seems that it does not rebuild some cpps correctly. So clean build fixes the probelm.