Bug 147183 – Introducing construct ability into JS executables

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

Introducing construct ability into JS executables

[Yusuke Suzuki]

Let's decouple the construct ability from the builtin functions.
Currently, builtin functions are not constructor by this patch http://trac.webkit.org/changeset/182995.
In this patch, when the given function is builtin JS function, we recognize it as the non-constructor function.

But, we need to relax it to implement some constructors in builtins JS.
By decoupling the construct ability from whether the function is builtin or not, we can provide

1. constructors written in builtin JS
2. non-constructors in normal JS functions

(1) is needed for Promise constructor. And (2) is needed for method functions and arrow functions.

[Yusuke Suzuki]

http://www.ecma-international.org/ecma-262/6.0/#sec-functioncreate
If kind is not Normal, let allocKind be "non-constructor".

[Yusuke Suzuki]

Created attachment 257243 [details]
Patch

WIP, the test forthcomming

[Yusuke Suzuki]

I've asked for the clarification in static MethodDefinition and construct ability.
https://esdiscuss.org/topic/propertydefinitionevaluation-for-static-methoddefinition

[Yusuke Suzuki]

Created attachment 257280 [details]
Patch

[Oliver Hunt]

Comment on attachment 257280 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=257280&action=review

r- as i don't like the subtlety of the current behaviour

> Source/JavaScriptCore/ChangeLog:26
> +        Currenctly, we make the builtin function the constructor when the function name is capitalized.

I think that's kind of icky -- something we consider doing that i think would be clearer would be to allow builtins defined as

constructor thing(...) { ... }

This would actually allow us (in a later patch) to use builtins to more cleanly define those functions that have distinct construct vs. call behaviours, e.g

constructor thing(...) {}
function thing( ... ) {}

would register the distinct executables for the construct vs. call behavior of "thing"

[Yusuke Suzuki]

Comment on attachment 257280 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=257280&action=review

Thank you for your review.

>> Source/JavaScriptCore/ChangeLog:26
>> +        Currenctly, we make the builtin function the constructor when the function name is capitalized.
> 
> I think that's kind of icky -- something we consider doing that i think would be clearer would be to allow builtins defined as
> 
> constructor thing(...) { ... }
> 
> This would actually allow us (in a later patch) to use builtins to more cleanly define those functions that have distinct construct vs. call behaviours, e.g
> 
> constructor thing(...) {}
> function thing( ... ) {}
> 
> would register the distinct executables for the construct vs. call behavior of "thing"

That sounds very nice to me. I'll upload the revised patch.

[Yusuke Suzuki]

Created attachment 257294 [details]
Patch

[Geoffrey Garen]

Comment on attachment 257294 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=257294&action=review

r=me with some suggested fixups below.

> Source/JavaScriptCore/builtins/BuiltinExecutables.cpp:63
> +UnlinkedFunctionExecutable* BuiltinExecutables::createExecutableInternal(const SourceCode& source, const Identifier& name, ConstructorKind constructorKind, bool hasConstruct)

Let's use the enum type (ConstructAbility) for the function argument instead of a bool. Bool is not nice as an argument type because it is often difficult to understand what "f(false)" or "f(true)" means just by reading it.

> Source/JavaScriptCore/builtins/BuiltinExecutables.h:64
> -    UnlinkedFunctionExecutable* createBuiltinExecutable(const SourceCode& code, const Identifier& name)
> +    UnlinkedFunctionExecutable* createBuiltinExecutable(const SourceCode& code, const Identifier& name, bool hasConstruct)
>      {
> -        return createExecutableInternal(code, name, ConstructorKind::None);
> +        return createExecutableInternal(code, name, ConstructorKind::None, hasConstruct);
>      }
> -    UnlinkedFunctionExecutable* createExecutableInternal(const SourceCode&, const Identifier&, ConstructorKind);
> +    UnlinkedFunctionExecutable* createExecutableInternal(const SourceCode&, const Identifier&, ConstructorKind, bool hasConstruct);

Ditto: Let's use the enum type instead of bool here.

> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.cpp:104
> +    , m_hasConstruct(constructAbility == ConstructAbility::HasConstruct)

Let's store the enum type instead of a bool.

> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.h:103
> +    HasConstruct,
> +    NoConstruct,

Let's call these "CanConstruct" and "CannotConstruct".

[Yusuke Suzuki]

Comment on attachment 257294 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=257294&action=review

Thank you for your review and suggestions.

>> Source/JavaScriptCore/builtins/BuiltinExecutables.cpp:63
>> +UnlinkedFunctionExecutable* BuiltinExecutables::createExecutableInternal(const SourceCode& source, const Identifier& name, ConstructorKind constructorKind, bool hasConstruct)
> 
> Let's use the enum type (ConstructAbility) for the function argument instead of a bool. Bool is not nice as an argument type because it is often difficult to understand what "f(false)" or "f(true)" means just by reading it.

OK, so I'll fix it by,

1. Create the ConstructAbility.h
2. Include the header in JSCBuiltins.cpp generated by the script
3. Use it

>> Source/JavaScriptCore/builtins/BuiltinExecutables.h:64
>> +    UnlinkedFunctionExecutable* createExecutableInternal(const SourceCode&, const Identifier&, ConstructorKind, bool hasConstruct);
> 
> Ditto: Let's use the enum type instead of bool here.

Fixed.

>> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.cpp:104
>> +    , m_hasConstruct(constructAbility == ConstructAbility::HasConstruct)
> 
> Let's store the enum type instead of a bool.

Here, due to the object size limitation, we used m_hasConstruct with the bit field (width 1). This is because the other fields also use the bit field to reduce the UnlinkedFunctionExecutable size.
Just storing the enum type enlarges the object size...
So, instead of storing the enum type, storing the bit and add the getter that converts the boolean to the enum value.

>> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.h:103
>> +    NoConstruct,
> 
> Let's call these "CanConstruct" and "CannotConstruct".

Thanks. Fixed.

[Oliver Hunt]

Comment on attachment 257294 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=257294&action=review

>>> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.cpp:104
>>> +    , m_hasConstruct(constructAbility == ConstructAbility::HasConstruct)
>> 
>> Let's store the enum type instead of a bool.
> 
> Here, due to the object size limitation, we used m_hasConstruct with the bit field (width 1). This is because the other fields also use the bit field to reduce the UnlinkedFunctionExecutable size.
> Just storing the enum type enlarges the object size...
> So, instead of storing the enum type, storing the bit and add the getter that converts the boolean to the enum value.

you can set an enum to be stored as a bitfield :D

[Yusuke Suzuki]

Comment on attachment 257294 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=257294&action=review

>>>> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.cpp:104
>>>> +    , m_hasConstruct(constructAbility == ConstructAbility::HasConstruct)
>>> 
>>> Let's store the enum type instead of a bool.
>> 
>> Here, due to the object size limitation, we used m_hasConstruct with the bit field (width 1). This is because the other fields also use the bit field to reduce the UnlinkedFunctionExecutable size.
>> Just storing the enum type enlarges the object size...
>> So, instead of storing the enum type, storing the bit and add the getter that converts the boolean to the enum value.
> 
> you can set an enum to be stored as a bitfield :D

That's fine! So just storing the enum type directly with the bitfield.

[Yusuke Suzuki]

Comment on attachment 257294 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=257294&action=review

>>>> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.cpp:104
>>>> +    , m_hasConstruct(constructAbility == ConstructAbility::HasConstruct)
>>> 
>>> Let's store the enum type instead of a bool.
>> 
>> Here, due to the object size limitation, we used m_hasConstruct with the bit field (width 1). This is because the other fields also use the bit field to reduce the UnlinkedFunctionExecutable size.
>> Just storing the enum type enlarges the object size...
>> So, instead of storing the enum type, storing the bit and add the getter that converts the boolean to the enum value.
> 
> you can set an enum to be stored as a bitfield :D

That's fine! So just storing the enum type directly with the bitfield.

[Yusuke Suzuki]

Comment on attachment 257294 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=257294&action=review

>>>>>> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.cpp:104
>>>>>> +    , m_hasConstruct(constructAbility == ConstructAbility::HasConstruct)
>>>>> 
>>>>> Let's store the enum type instead of a bool.
>>>> 
>>>> Here, due to the object size limitation, we used m_hasConstruct with the bit field (width 1). This is because the other fields also use the bit field to reduce the UnlinkedFunctionExecutable size.
>>>> Just storing the enum type enlarges the object size...
>>>> So, instead of storing the enum type, storing the bit and add the getter that converts the boolean to the enum value.
>>> 
>>> you can set an enum to be stored as a bitfield :D
>> 
>> That's fine! So just storing the enum type directly with the bitfield.
> 
> That's fine! So just storing the enum type directly with the bitfield.

Oops, warned by the style checker, "Please declare enum bitfields as unsigned integral types."
So just follow the other accessors (like FunctionMode, ConstructorKind etc.), storing enum directly to the bitfield (unsigned) and retrieve it by static_cast in the accessor.

[Yusuke Suzuki]

Committed r187205: <http://trac.webkit.org/changeset/187205>