Bug 160499 – [ES6] Modules' `export default function/class` should be declaration

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

[Yusuke Suzuki]

[ES6] Use FunctionDeclaration and ClassDeclaration under export default context

[Yusuke Suzuki]

Created attachment 285220 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 285220 [details]
Patch

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

Add notes.

> Source/JavaScriptCore/parser/ModuleScopeData.h:51
> +        m_exportedBindings.add(localName.impl(), Vector<RefPtr<UniquedStringImpl>>()).iterator->value.append(exportedName.impl());

Since this uniqueness is guaranteed by m_exportedNames set, we can just use the vector here.

> Source/JavaScriptCore/parser/Parser.cpp:3710
> +    info.className = &m_vm->propertyNames->nullIdentifier;

Initializing the default value in the caller side. This is the same in the parseFunctionInfo case (see parseFunctionExpression).

[Yusuke Suzuki]

Created attachment 286659 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 286659 [details]
Patch

Fixing issues.

[Yusuke Suzuki]

Created attachment 286663 [details]
Patch

[Saam Barati]

Comment on attachment 286663 [details]
Patch

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

r=me with comments

> Source/JavaScriptCore/parser/Parser.cpp:2321
> +        ASSERT_WITH_MESSAGE(declarationDefaultContext != DeclarationDefaultContext::ExportDefault, "Export default case will export name and binding in caller side.");

"export name and binding in caller side" => "export the name and binding in the caller"

> Source/JavaScriptCore/parser/Parser.cpp:2348
> +    ASSERT_WITH_MESSAGE(!(requirements == FunctionRequirements::NoRequirements && !info.className), "When specifying FunctionRequirements::NoRequirements, we need to initialize info.className with the default value in the caller side.");

"caller side" => "caller"

> Source/JavaScriptCore/parser/Parser.h:73
> +enum class FunctionRequirements { NoRequirements, Named, Unnamed };

Suggestion on better names for this:

enum class FunctionNameRequirements { None, NeedsName, MustNotHaveName }
or
enum class FunctionNameRequirements { None, Named, Unnamed }

> JSTests/modules/export-default-function-name-in-assignment-expression.js:7
> +shouldBe(func.name, 'default');

What about toString()?

> JSTests/modules/export-default-function-name-in-class-declaration.js:7
> +shouldBe(cls.name, 'default');

Ditto

> JSTests/stress/method-name.js:27
> +class Hello {
> +    hello hello() { }
> +}

It might also be worth adding tests like this to the parser-syntax-check file

[Yusuke Suzuki]

Comment on attachment 286663 [details]
Patch

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

>> Source/JavaScriptCore/parser/Parser.cpp:2321
>> +        ASSERT_WITH_MESSAGE(declarationDefaultContext != DeclarationDefaultContext::ExportDefault, "Export default case will export name and binding in caller side.");
> 
> "export name and binding in caller side" => "export the name and binding in the caller"

Thanks! Fixed.

>> Source/JavaScriptCore/parser/Parser.cpp:2348
>> +    ASSERT_WITH_MESSAGE(!(requirements == FunctionRequirements::NoRequirements && !info.className), "When specifying FunctionRequirements::NoRequirements, we need to initialize info.className with the default value in the caller side.");
> 
> "caller side" => "caller"

Fixed.

>> Source/JavaScriptCore/parser/Parser.h:73
>> +enum class FunctionRequirements { NoRequirements, Named, Unnamed };
> 
> Suggestion on better names for this:
> 
> enum class FunctionNameRequirements { None, NeedsName, MustNotHaveName }
> or
> enum class FunctionNameRequirements { None, Named, Unnamed }

`enum class FunctionNameRequirements { None, Named, Unnamed }` sounds very nice. I'll change it.

>> JSTests/modules/export-default-function-name-in-assignment-expression.js:7
>> +shouldBe(func.name, 'default');
> 
> What about toString()?

Nice catch. It becomes `function () { }`. It is expected result.
I've added the test.

>> JSTests/modules/export-default-function-name-in-class-declaration.js:7
>> +shouldBe(cls.name, 'default');
> 
> Ditto

Added the test.

> JSTests/modules/export-default-function-name-in-function-declaration.js:7
> +shouldBe(func.name, 'default');

Nice catch. It shows "star default" value for func.toString().
However, I think it is due to the current Function#toString() implementation.
Currently, we use the func->name(). However, as per the latest `Function#toString()` draft, it should use the original source text[1].
In the meantime, I introduce the similar hook to the reifyName to JSFunction::name.

[1]: http://tc39.github.io/Function-prototype-toString-revision/#sec-function-definitions-runtime-semantics-instantiatefunctionobject

"NOTE: An anonymous FunctionDeclaration can only occur as part of an export default declaration.".

> JSTests/modules/export-default-function-name-in-generator-declaration.js:7
> +shouldBe(func.name, 'default');

It should be `function * () { }`. But currently, the result is `function () { }`.
Once Function#toString() draft is introduced, we need to refactor the Function#toString.

>> JSTests/stress/method-name.js:27
>> +}
> 
> It might also be worth adding tests like this to the parser-syntax-check file

Sounds nice. I'll add the new directory and runParserSyntaxCheck fund in run-esc-stress-tests for that.
https://bugs.webkit.org/show_bug.cgi?id=161094

[Yusuke Suzuki]

Committed r204842: <http://trac.webkit.org/changeset/204842>