Bug 147353 – [ES6] Add ES6 Modules preparsing phase to collect the dependencies

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

[Yusuke Suzuki]

Before executing the body of the modules, we need to collect the dependencies. (import / export)
This issue involves modules parsing change.

[Yusuke Suzuki]

My plan is,

1. adding some option to JSC shell (maybe, leveraging the existing --xxx=yyy system is preferable)
2. with this option, parse the module syntax
3. just parsing it currently and ensure the parsing correctness by the test

[Yusuke Suzuki]

After collecting the information from the module,

1. introducing the sort of loader into JSC shell (jsc.cpp), it's synchronous loader
2. test modules semantics under it

And after that, we'll implement asynchronous loader in the WebCore side.

[Yusuke Suzuki]

Created attachment 257764 [details]
Patch

WIP, parsing module related syntax

[Yusuke Suzuki]

Before landing the preparing system (I'm planning to create the new builder that inherits SyntaxChecker), just creating the patch to add parsing functionality.

[Yusuke Suzuki]

Created attachment 258679 [details]
Patch

WIP

[Yusuke Suzuki]

Created attachment 258696 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 258696 [details]
Patch

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

The patch is ready.

> Source/JavaScriptCore/runtime/Completion.cpp:78
> +    return true;

Since this patch just implements analyzing part, we don't have tests for this patch.

[WebKit Commit Bot]

Attachment 258696 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/NodesAnalyzeModule.cpp:27:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/parser/NodesAnalyzeModule.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/parser/Parser.h:839:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/parser/ModuleRecord.h:122:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 4 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 258697 [details]
Patch

[WebKit Commit Bot]

Attachment 258697 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/NodesAnalyzeModule.cpp:27:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/parser/NodesAnalyzeModule.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/parser/Parser.h:839:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/parser/ModuleRecord.h:122:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 4 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Comment on attachment 258697 [details]
Patch

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

Added comments for the ease of reviewing.

> Source/JavaScriptCore/parser/ModuleAnalyzer.cpp:142
> +    //     export * from "mod"

These information is required for

1. requesting dependent modules.
2. constructing the module environment before executing the module body.

For (2), we collect the exported/imported binding informations; Exported variable should be allocated in the module's environment (as a heap variable). And imported bindings should be handled in the module's environment.

[Geoffrey Garen]

Comment on attachment 258697 [details]
Patch

r=me

[Yusuke Suzuki]

(In reply to comment #12)
> Comment on attachment 258697 [details]
> Patch
> 
> r=me

Thank you. In the meantime, I'll land this patch.
But, Saam is also reviewing this, so please feel free to comment it, I'll reflect your comments at any time :)

[Yusuke Suzuki]

Committed r188355: <http://trac.webkit.org/changeset/188355>

[Saam Barati]

Comment on attachment 258697 [details]
Patch

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

Nice work. Some comments below.

> Source/JavaScriptCore/builtins/BuiltinNames.h:34
> +#define INITIALISE_BUILTIN_NAMES(name) , m_##name(Identifier::fromString(vm, #name)), m_##name##PrivateName(Identifier::fromUid(PrivateName(PrivateName::Description, ASCIILiteral("PrivateSymbol." #name))))

What is this doing?
Should this be in this patch?

> Source/JavaScriptCore/parser/ModuleAnalyzer.cpp:38
> +    , m_declaredVariables(declaredVariables)

I like the use of VariableEnvironment here. I'm glad this is being used as the way to speak about variables in the parser.

> Source/JavaScriptCore/parser/ModuleAnalyzer.cpp:100
> +Ref<ModuleRecord> ModuleAnalyzer::analyze(ModuleProgramNode& moduleProgramNode)

The comments in this function are really helpful.

> Source/JavaScriptCore/parser/ModuleAnalyzer.cpp:103
> +    // 1. Import entries

super Nit: This isn't an ordered list, these could just be bullet points using "-" or "*" instead of "1/2/3/4"

> Source/JavaScriptCore/parser/NodesAnalyzeModule.cpp:43
> +    for (StatementNode* statement = m_head; statement; statement = statement->next())

I know we ensure the Analyze phase only collects ModuleDeclarationNodes, but it would be nice to have an assert verifying this.
We could have StatementNode have a virtual function "isModuleDeclarationNode" and assert against that.

> Source/JavaScriptCore/parser/NodesAnalyzeModule.cpp:51
> +        analyzer.moduleRecord().addImportEntry({

I think having the name of the struct before "{" is useful here.

> Source/JavaScriptCore/parser/NodesAnalyzeModule.cpp:80
> +        if (m_moduleSpecifier) {

Nit: I know in an earlier patch I said it's good to use the names the spec uses, but I think I was maybe wrong.
I'm not in love with the name m_moduleSpecifier. That name doesn't really mean anything to me.
Maybe we can give this a better name, like m_moduleFileName (even though it's not just files) or m_moduleName or something similar.

> Source/JavaScriptCore/parser/Parser.cpp:238
> +String Parser<LexerType>::parseInner(const Identifier& calleeName, FunctionParseMode parseMode, ModuleParseMode moduleParseMode)

Can we just join FunctionParseMode and ModuleParseMode into one thing and give it a different name? Maybe SourceParseMode or another name?
Or even better, we can also unify this with JSParserCodeType. Or get rid of JSParserCodeType if needed.
Maybe we should open a separate bug to unify all of these into one type in another patch.

> Source/JavaScriptCore/parser/Parser.cpp:2475
> +            // const *default* = expr;

*default* is not visible to the actual JS code, right?
I think we should make that explicit in this comment.

> Source/JavaScriptCore/parser/Parser.cpp:2484
> +                internalFailWithMessage(false, "Cannot export 'default' name twice: '");

nit: should be: "Only one 'default' export is allowed"

> Source/JavaScriptCore/parser/Parser.cpp:2494
> +        semanticFailIfFalse(exportName(m_vm->propertyNames->defaultKeyword), "Cannot export 'default' name twice.");

ditto.

[Yusuke Suzuki]

Comment on attachment 258697 [details]
Patch

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

Thank you for your review, Saam. Your comments are so helpful.
I'll upload the follow-up patch to this issue. You can review it :)

>> Source/JavaScriptCore/builtins/BuiltinNames.h:34
>> +#define INITIALISE_BUILTIN_NAMES(name) , m_##name(Identifier::fromString(vm, #name)), m_##name##PrivateName(Identifier::fromUid(PrivateName(PrivateName::Description, ASCIILiteral("PrivateSymbol." #name))))
> 
> What is this doing?
> Should this be in this patch?

It adds the [[Description]] string into the private symbol to make the debug functionality of the ModuleRecord (dumping the Symbol names in ModuleRecord::dump) useful.

>> Source/JavaScriptCore/parser/ModuleAnalyzer.cpp:103
>> +    // 1. Import entries
> 
> super Nit: This isn't an ordered list, these could just be bullet points using "-" or "*" instead of "1/2/3/4"

Thanks. I'll fix it :)

>> Source/JavaScriptCore/parser/NodesAnalyzeModule.cpp:43
>> +    for (StatementNode* statement = m_head; statement; statement = statement->next())
> 
> I know we ensure the Analyze phase only collects ModuleDeclarationNodes, but it would be nice to have an assert verifying this.
> We could have StatementNode have a virtual function "isModuleDeclarationNode" and assert against that.

Sounds nice! I'll fix it.

>> Source/JavaScriptCore/parser/NodesAnalyzeModule.cpp:51
>> +        analyzer.moduleRecord().addImportEntry({
> 
> I think having the name of the struct before "{" is useful here.

Reasonable. I'll add the type name, ImportEntry.

>> Source/JavaScriptCore/parser/NodesAnalyzeModule.cpp:80
>> +        if (m_moduleSpecifier) {
> 
> Nit: I know in an earlier patch I said it's good to use the names the spec uses, but I think I was maybe wrong.
> I'm not in love with the name m_moduleSpecifier. That name doesn't really mean anything to me.
> Maybe we can give this a better name, like m_moduleFileName (even though it's not just files) or m_moduleName or something similar.

Make sense.
m_moduleName looks nice because the module's name will be resolved by the loader and it might not be the file name.
I'll rename them.

>> Source/JavaScriptCore/parser/Parser.cpp:2475
>> +            // const *default* = expr;
> 
> *default* is not visible to the actual JS code, right?
> I think we should make that explicit in this comment.

Yes. I'll add the comment.

>> Source/JavaScriptCore/parser/Parser.cpp:2484
>> +                internalFailWithMessage(false, "Cannot export 'default' name twice: '");
> 
> nit: should be: "Only one 'default' export is allowed"

Looks nice to me. I'll fix it.

>> Source/JavaScriptCore/parser/Parser.cpp:2494
>> +        semanticFailIfFalse(exportName(m_vm->propertyNames->defaultKeyword), "Cannot export 'default' name twice.");
> 
> ditto.

Fixed.

[Yusuke Suzuki]

Comment on attachment 258697 [details]
Patch

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

>> Source/JavaScriptCore/parser/Parser.cpp:238
>> +String Parser<LexerType>::parseInner(const Identifier& calleeName, FunctionParseMode parseMode, ModuleParseMode moduleParseMode)
> 
> Can we just join FunctionParseMode and ModuleParseMode into one thing and give it a different name? Maybe SourceParseMode or another name?
> Or even better, we can also unify this with JSParserCodeType. Or get rid of JSParserCodeType if needed.
> Maybe we should open a separate bug to unify all of these into one type in another patch.

I'll try it. Maybe, we can.

[Yusuke Suzuki]

Reopening to attach new patch.

[Yusuke Suzuki]

Created attachment 258918 [details]
Patch

[WebKit Commit Bot]

Attachment 258918 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ChangeLog:11:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: Source/JavaScriptCore/ChangeLog:12:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: Source/JavaScriptCore/ChangeLog:13:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: Source/JavaScriptCore/ChangeLog:14:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Total errors found: 4 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Comment on attachment 258918 [details]
Patch

The follow-up patch comes here now!

[Yusuke Suzuki]

Created attachment 258928 [details]
Patch

[Yusuke Suzuki]

Just tweak the ChangeLog.

[Saam Barati]

Comment on attachment 258928 [details]
Patch

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

r=me

> Source/JavaScriptCore/parser/ParserModes.h:58
> +inline bool isFunctionParseMode(SourceParseMode parseMode)

nice.

[Yusuke Suzuki]

Comment on attachment 258928 [details]
Patch

Thank you for your review :D

[Yusuke Suzuki]

Committed r188417: <http://trac.webkit.org/changeset/188417>