Bug 147422 – [ES6] Support Module Syntax

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Yusuke Suzuki]

Just complete the module syntax.

[Yusuke Suzuki]

Created attachment 257800 [details]
Patch

[Yusuke Suzuki]

Created attachment 257801 [details]
Patch

[Yusuke Suzuki]

I've rebased the patch. Ready for review.

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/parser/Parser.cpp:212:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/parser/Parser.cpp:2359:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/parser/ParserTokens.h:79:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/parser/ParserTokens.h:80:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 4 in 19 files


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

[Oliver Hunt]

Comment on attachment 257801 [details]
Patch

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

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2905
> +

this implies that the result of eval("module ....") is the module object -- is that correct?

> Source/JavaScriptCore/jsc.cpp:497
> +#if ENABLE(WEBASSEMBLY)
> +static EncodedJSValue JSC_HOST_CALL functionLoadWebAssembly(ExecState*);
> +#endif

Separate patch?

> Source/JavaScriptCore/jsc.cpp:666
> +#if ENABLE(WEBASSEMBLY)
> +        addFunction(vm, "loadWebAssembly", functionLoadWebAssembly, 1);
> +#endif

Separate patch?

> Source/JavaScriptCore/jsc.cpp:1215
> +#if ENABLE(WEBASSEMBLY)
> +EncodedJSValue JSC_HOST_CALL functionLoadWebAssembly(ExecState* exec)
> +{
> +    String fileName = exec->argument(0).toString(exec)->value(exec);
> +    Vector<char> buffer;
> +    if (!fillBufferWithContentsOfFile(fileName, buffer))
> +        return JSValue::encode(exec->vm().throwException(exec, createError(exec, ASCIILiteral("Could not open file."))));
> +    RefPtr<WebAssemblySourceProvider> sourceProvider = WebAssemblySourceProvider::create(reinterpret_cast<Vector<uint8_t>&>(buffer), fileName);
> +    SourceCode source(sourceProvider);
> +    String errorMessage;
> +    JSWASMModule* module = parseWebAssembly(exec, source, errorMessage);
> +    if (!module)
> +        return JSValue::encode(exec->vm().throwException(exec, createSyntaxError(exec, errorMessage)));
> +    return JSValue::encode(module);
> +}
> +#endif

Separate patch?

> Source/JavaScriptCore/parser/Nodes.h:1662
> +    class ImportSpecifierListNode : public ParserArenaFreeable {

This (and the latter *ListNodes) should just be Vectors (or similar) rather than linked lists. The existing linked list based structures are simply old crufty code dating back to the days we used YACC/Bison.

> Source/JavaScriptCore/parser/Parser.cpp:2185
> +        failDueToUnexpectedToken();

I think there's a benefit to ensuring that we have good error messages in all new code.

> Source/JavaScriptCore/parser/Parser.cpp:2274
> +                failIfFalse(match(IDENT) || m_token.m_type & KeywordTokenFlag, "Expected a local name for a targeted import declaration");

Don't we have a helper for this condition already?

[Build Bot]

Comment on attachment 257801 [details]
Patch

Attachment 257801 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5100260917510144

New failing tests:
sputnik/Conformance/07_Lexical_Conventions/7.5_Tokens/7.5.3_Future_Reserved_Words/S7.5.3_A1.16.html
js/dom/reserved-words-as-property.html
sputnik/Conformance/07_Lexical_Conventions/7.5_Tokens/7.5.3_Future_Reserved_Words/S7.5.3_A1.10.html

[Build Bot]

Created attachment 257803 [details]
Archive of layout-test-results from ews100 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Yusuke Suzuki]

Comment on attachment 257801 [details]
Patch

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

Thank you for your review! I'll fix the pointed ones and fix the failed test due to changing export/import to Keyword (IMPORT/EXPORT) from the RESERVED.

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2905
>> +
> 
> this implies that the result of eval("module ....") is the module object -- is that correct?

No. We cannot use `eval`, `Function` to evaluate some code as the module. For example,
eval("import * as ns from 'mod'") should be a syntax error.
Module syntax and semantics are only allowed for the module code, that is produced by the script tag or script file (platform dependent way).

BTW, this patch aims at only implementing the syntax part. So the above process is just the same to the ProgramNode's one.
To make it explicitly, I'll drop the code in `ModuleNode::emitBytecode`. Sorry for confusing.

>> Source/JavaScriptCore/jsc.cpp:497
>> +#endif
> 
> Separate patch?

Oops. Thank you. They are mis-rebased part. I'll drop them.

>> Source/JavaScriptCore/jsc.cpp:666
>> +#endif
> 
> Separate patch?

Ditto.

>> Source/JavaScriptCore/jsc.cpp:1215
>> +#endif
> 
> Separate patch?

Yes, this is mis-rebasing results. I'll drop them.

>> Source/JavaScriptCore/parser/Nodes.h:1662
>> +    class ImportSpecifierListNode : public ParserArenaFreeable {
> 
> This (and the latter *ListNodes) should just be Vectors (or similar) rather than linked lists. The existing linked list based structures are simply old crufty code dating back to the days we used YACC/Bison.

OK, so is changing the nodes to ParserArenaDeletable better? (not to leak the vector's memory in the object allocated in the parser arena)
I'll change them.

>> Source/JavaScriptCore/parser/Parser.cpp:2185
>> +        failDueToUnexpectedToken();
> 
> I think there's a benefit to ensuring that we have good error messages in all new code.

Agreed. I'll change it to produce more reasonable messages.

>> Source/JavaScriptCore/parser/Parser.cpp:2274
>> +                failIfFalse(match(IDENT) || m_token.m_type & KeywordTokenFlag, "Expected a local name for a targeted import declaration");
> 
> Don't we have a helper for this condition already?

We have `LexerFlagsIgnoreReservedWords`, it produces IDENT even for keywords.
But here, the lexed token may be treated in either IdentifierName (including keywords) or Identifier (not including keywords).
For example,

import { A } from "Cocoa"

case, A is limited to Identifier. (So, `import { enum } from "Cocoa"` is syntax invalid).
But,

import { A as B } from "Cocoa"

case, A is IdentifierName. So we can write `import { enum as B } from "Cocoa"`.
I'll investigate the better way to handle these cases. If I found we don't have a helper, I'll add it and use here :)

[Yusuke Suzuki]

Created attachment 257825 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:79:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/parser/ParserTokens.h:80:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/parser/Parser.cpp:212:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/parser/Parser.cpp:2355:  Multi line control clauses should use braces.  [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.

[Build Bot]

Comment on attachment 257825 [details]
Patch

Attachment 257825 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4515479645323264

New failing tests:
sputnik/Conformance/07_Lexical_Conventions/7.5_Tokens/7.5.3_Future_Reserved_Words/S7.5.3_A1.16.html
sputnik/Conformance/07_Lexical_Conventions/7.5_Tokens/7.5.3_Future_Reserved_Words/S7.5.3_A1.10.html

[Build Bot]

Created attachment 257826 [details]
Archive of layout-test-results from ews103 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 257825 [details]
Patch

Attachment 257825 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5641379552165888

New failing tests:
sputnik/Conformance/07_Lexical_Conventions/7.5_Tokens/7.5.3_Future_Reserved_Words/S7.5.3_A1.16.html
sputnik/Conformance/07_Lexical_Conventions/7.5_Tokens/7.5.3_Future_Reserved_Words/S7.5.3_A1.10.html

[Build Bot]

Created attachment 257827 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Yusuke Suzuki]

Created attachment 257834 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:79:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/parser/ParserTokens.h:80:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/parser/Parser.cpp:212:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/parser/Parser.cpp:2355:  Multi line control clauses should use braces.  [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 257836 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:79:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/parser/ParserTokens.h:80:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/parser/Parser.cpp:212:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/parser/Parser.cpp:2355:  Multi line control clauses should use braces.  [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]

tests are passed. the patch is ready for review.

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:79:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/parser/ParserTokens.h:80:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/parser/Parser.cpp:212:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/parser/Parser.cpp:2355:  Multi line control clauses should use braces.  [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.

[Saam Barati]

Comment on attachment 257836 [details]
Patch

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

> Source/JavaScriptCore/parser/Parser.cpp:426
> +            statement = parseStatementListItem(context, directive, &directiveLiteralLength);

Do we need to keep this AST Node around or keep it in the source elements list for the top level parse?
If not, we could get fancy and just use the SyntaxChecker for this case to prevent unneeded allocations.

[Yusuke Suzuki]

Comment on attachment 257836 [details]
Patch

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

>> Source/JavaScriptCore/parser/Parser.cpp:426
>> +            statement = parseStatementListItem(context, directive, &directiveLiteralLength);
> 
> Do we need to keep this AST Node around or keep it in the source elements list for the top level parse?
> If not, we could get fancy and just use the SyntaxChecker for this case to prevent unneeded allocations.

For the top level parse, if we will execute the module body with it, the result of parseStatementListItem is necessary.
But if we just collect the module information, we can use SyntaxChecker here.

[Yusuke Suzuki]

Created attachment 258003 [details]
Patch

[Yusuke Suzuki]

Created attachment 258004 [details]
Patch

[Yusuke Suzuki]

Created attachment 258005 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:79:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/parser/ParserTokens.h:80:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/parser/Parser.cpp:212:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/parser/Parser.cpp:2402:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 4 in 24 files


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

[Yusuke Suzuki]

Comment on attachment 258005 [details]
Patch

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

Updated the patch, now, parser for modules include almost all early error checks.

> Source/JavaScriptCore/parser/Parser.cpp:421
> +    SyntaxChecker moduleStatementItemBuilder(const_cast<VM*>(m_vm), m_lexer.get());

Since currently we don't introduce ModuleAnalyzer yet, we just use SyntaxChecker directly here.
This will be replaced with typedefed type under the TreeBuilder in https://bugs.webkit.org/show_bug.cgi?id=147353

> Source/JavaScriptCore/parser/Parser.cpp:445
> +        semanticFailIfFalse(currentScope()->hasDeclaredVariable(uid) || currentScope()->hasLexicallyDeclaredVariable(uid), "No binding exists for the exported local binding '", uid.get(), "'");

Check all exported bindings are resolvable.
http://www.ecma-international.org/ecma-262/6.0/#sec-module-semantics-static-semantics-early-errors
"It is a Syntax Error if any element of the ExportedBindings of ModuleItemList does not also occur in either the VarDeclaredNames of ModuleItemList, or the LexicallyDeclaredNames of ModuleItemList."

> Source/JavaScriptCore/parser/Parser.cpp:2266
> +    DeclarationResultMask declarationResult = declareVariable(localName, DeclarationType::ConstDeclaration);

Declare the imported binding name as an immutable binding.
http://www.ecma-international.org/ecma-262/6.0/#sec-createimportbinding

> Source/JavaScriptCore/parser/Parser.cpp:3322
> +        semanticFailIfFalse(currentScope()->isFunction(), "super is only valid inside functions");

super and new.target is not allowed under the script / module context.

> Source/JavaScriptCore/parser/Parser.h:149
> +    }

We collect the exported names and exported binding names to produce early errors defined in http://www.ecma-international.org/ecma-262/6.0/#sec-module-semantics-static-semantics-early-errors.
declareVariable check is not enough for Module's exportName check because when we write the following code
   export { A as B } from "mod"
in this case, this declaration import the name A and export it as B from this module. But this declaration does not define any bindings in the current module.
So this exported binding name does not seen in the current module (we cannot access it here with A or B)
This means that exported name "B" is not checked by declareVariable. So we collect exported names by using this method and when we saw the duplicate exported name, we raise the early error.

> Source/JavaScriptCore/parser/Parser.h:1053
> +    enum class ExportType { Exported, NotExported };

This enum represents the currently parsed variable name will be exported or not.
If the name is exported, we need to check whether we already have the duplicate exported name.

> Source/JavaScriptCore/tests/stress/modules-syntax-error-with-names.js:195
> +// http://www.ecma-international.org/ecma-262/6.0/#sec-module-semantics-static-semantics-early-errors

When this is fixed, Module / Script will raise errors for
`
var a = 20;
const a = 30;
`
case and
`
import a from "mod"
var a = 40;
`
case

> Source/JavaScriptCore/tests/stress/modules-syntax-error.js:320
> +`, `SyntaxError: super is only valid inside functions.:2`);

super and new.target is not allowed under the script / module context.

[Build Bot]

Comment on attachment 258005 [details]
Patch

Attachment 258005 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/5453

New failing tests:
sputnik/Conformance/07_Lexical_Conventions/7.5_Tokens/7.5.3_Future_Reserved_Words/S7.5.3_A1.27.html

[Build Bot]

Created attachment 258007 [details]
Archive of layout-test-results from ews102 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 258005 [details]
Patch

Attachment 258005 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5465

New failing tests:
sputnik/Conformance/07_Lexical_Conventions/7.5_Tokens/7.5.3_Future_Reserved_Words/S7.5.3_A1.27.html

[Build Bot]

Created attachment 258008 [details]
Archive of layout-test-results from ews105 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Yusuke Suzuki]

Created attachment 258012 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:79:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/parser/ParserTokens.h:80:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/parser/Parser.cpp:212:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/parser/Parser.cpp:2402:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 4 in 25 files


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

[Yusuke Suzuki]

The patch is ready :)

[Saam Barati]

Comment on attachment 258012 [details]
Patch

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

patch looks good to me.
Some suggestions/nits below.

> Source/JavaScriptCore/parser/Nodes.h:1625
> +    class ModuleNode : public ScopeNode {

I wonder if maybe ModuleProgramNode is a better name?
What do you think?

> Source/JavaScriptCore/parser/Nodes.h:1673
> +        Specifiers m_specifiers { };

Is this the same as "Specifiers m_specifiers;"?
If so, we should do that.

> Source/JavaScriptCore/parser/Parser.cpp:420
> +    // like TreeBuilder::ModuleStatementItemBuilder.

Do we have a bug # for this?

> Source/JavaScriptCore/parser/Parser.cpp:445
> +        semanticFailIfFalse(currentScope()->hasDeclaredVariable(uid) || currentScope()->hasLexicallyDeclaredVariable(uid), "No binding exists for the exported local binding '", uid.get(), "'");

Maybe we can have this error message be:
"exported binding '" + uid + "' needs to refer to a top-level declared variable" ?
Or something similar to this?

> Source/JavaScriptCore/parser/Parser.cpp:602
> +            semanticFailIfFalse(exportType != ExportType::Exported || currentScope()->moduleScopeData().exportName(*name), "Cannot export a duplicate name '", name->impl(), "'");

Nit: to me this reads easier as:
semanticFailIfTrue(exportType == ExportType::Exported && !currentScope()->moduleScopeData().exportName(*name), ...)
But maybe this is just my preference.

> Source/JavaScriptCore/parser/Parser.cpp:2202
> +template <class TreeBuilder> typename TreeBuilder::ModuleSpecifier Parser<LexerType>::parseModuleSpecifier(TreeBuilder& context)

I think for some of these functions it might be helpful just to name the section of the spec or have a link to the grammar portion of the spec.
I know for me, I didn't understand what any of these names were until I read the grammar in the spec. 
That said, I think it's good to follow the grammar production names used in the spec.

> Source/JavaScriptCore/parser/Parser.cpp:2205
> +    failIfFalse(match(STRING), "Expected a module specifier string");

I would say:
"imported modules names must be string literals" or something similar as the error message here.

> Source/JavaScriptCore/parser/Parser.cpp:2212
> +template <class TreeBuilder> typename TreeBuilder::ImportSpecifier Parser<LexerType>::parseImportSpecifier(TreeBuilder& context, ImportSpecifierType specifierType)

lets call this parseImportClause because it seems most similar to that grammar production.

> Source/JavaScriptCore/parser/Parser.cpp:2221
> +        // * as ns

// * as namespace.

> Source/JavaScriptCore/parser/Parser.cpp:2229
> +        matchOrFail(IDENT, "Expected a local name for a targeted import declaration");

"Expected a variable name for the import declaration"

> Source/JavaScriptCore/parser/Parser.cpp:2234
> +    break;

break should be in above block statement

> Source/JavaScriptCore/parser/Parser.cpp:2247
> +            matchOrFail(IDENT, "Expected a local name for a targeted import declaration");

"Expected a variable name for the import declaration"

> Source/JavaScriptCore/parser/Parser.cpp:2253
> +    break;

ditto

> Source/JavaScriptCore/parser/Parser.cpp:2262
> +    break;

ditto.

> Source/JavaScriptCore/parser/Parser.cpp:2265
> +    semanticFailIfTrue(localNameToken.m_type & KeywordTokenFlag, "Expected an imported binding name");

I would say: "can't use keyword as imported binding name"

> Source/JavaScriptCore/parser/Parser.cpp:2293
> +    bool hasDefaultSpecifier = false;

Nit:
Maybe the logic would be easier to read if we did something like this:
bool isFinishedParsingImport = false;

> Source/JavaScriptCore/parser/Parser.cpp:2298
> +        hasDefaultSpecifier = true;

and then here have:
if (match(COMMA)) next();
else isFinishedParsing = true;

> Source/JavaScriptCore/parser/Parser.cpp:2301
> +    if (!hasDefaultSpecifier || match(COMMA)) {

and then switch this to:
"if (!isFinishedParsingImport)".

> Source/JavaScriptCore/parser/Parser.cpp:2302
> +        if (hasDefaultSpecifier)

and remove this.

> Source/JavaScriptCore/parser/Parser.cpp:2334
> +            failWithMessage("Expected export specifiers");

"Expected namespace import or import list"

> Source/JavaScriptCore/parser/Parser.cpp:2337
> +    failIfFalse(matchContextualKeyword(m_vm->propertyNames->from), "Expected 'from' leading a module specifier");

"Expected 'from' before module name".
In general, I think we should shy away from "specifier" in error names.
To me "specifier" means a lot less than "name" in an error message.

> Source/JavaScriptCore/parser/Parser.cpp:2360
> +        failIfFalse(matchIdentifierOrKeyword(), "Expected an exported name for a targeted export declaration");

So we can do: "export foo as let" but not "import let"?
If so, the ES6 spec is pretty hilarious in what it allows/disallows.

> Source/JavaScriptCore/parser/Parser.cpp:2375
> +    next(); // EXPORT

nit: comment not needed.

> Source/JavaScriptCore/parser/Parser.cpp:2380
> +        next(); // TIMES

nit: comment not needed.

> Source/JavaScriptCore/parser/Parser.cpp:2382
> +        failIfFalse(matchContextualKeyword(m_vm->propertyNames->from), "Expected 'from' leading a module specifier");

"Expected 'from' before exported module name".

> Source/JavaScriptCore/parser/Parser.cpp:2389
> +    break;

this and below "break"s should be inside the block.

> Source/JavaScriptCore/parser/Parser.cpp:2394
> +        // export default [lookahead â {function, class}] AssignmentExpression[In] ;

Looks like unicode 'not-in' epsilon character is being cut off.
Maybe just write "lookahead not-in ..."

> Source/JavaScriptCore/parser/Parser.cpp:2462
> +            while (match(COMMA)) {

If you switch to a do-while loop you can remove the above code.

> Source/JavaScriptCore/parser/Parser.h:888
> +    ALWAYS_INLINE bool matchContextualKeyword(const Identifier& identifier)

This is a nice helper function.

[Yusuke Suzuki]

Comment on attachment 258012 [details]
Patch

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

Thank you for your review! Fixed the all your suggestions. That's so helpful :)

>> Source/JavaScriptCore/parser/Nodes.h:1625
>> +    class ModuleNode : public ScopeNode {
> 
> I wonder if maybe ModuleProgramNode is a better name?
> What do you think?

Sounds nice. ModuleProgramNode name explicitly says it's the same level node to the ProgramNode.
I'll change it.

>> Source/JavaScriptCore/parser/Nodes.h:1673
>> +        Specifiers m_specifiers { };
> 
> Is this the same as "Specifiers m_specifiers;"?
> If so, we should do that.

Yes, dropped "{}".

>> Source/JavaScriptCore/parser/Parser.cpp:420
>> +    // like TreeBuilder::ModuleStatementItemBuilder.
> 
> Do we have a bug # for this?

https://bugs.webkit.org/show_bug.cgi?id=147353 is the bug for this.
I'll add link here.

>> Source/JavaScriptCore/parser/Parser.cpp:445
>> +        semanticFailIfFalse(currentScope()->hasDeclaredVariable(uid) || currentScope()->hasLexicallyDeclaredVariable(uid), "No binding exists for the exported local binding '", uid.get(), "'");
> 
> Maybe we can have this error message be:
> "exported binding '" + uid + "' needs to refer to a top-level declared variable" ?
> Or something similar to this?

Yup. I've fixed.

>> Source/JavaScriptCore/parser/Parser.cpp:602
>> +            semanticFailIfFalse(exportType != ExportType::Exported || currentScope()->moduleScopeData().exportName(*name), "Cannot export a duplicate name '", name->impl(), "'");
> 
> Nit: to me this reads easier as:
> semanticFailIfTrue(exportType == ExportType::Exported && !currentScope()->moduleScopeData().exportName(*name), ...)
> But maybe this is just my preference.

Looks nice. Changed.

>> Source/JavaScriptCore/parser/Parser.cpp:2202
>> +template <class TreeBuilder> typename TreeBuilder::ModuleSpecifier Parser<LexerType>::parseModuleSpecifier(TreeBuilder& context)
> 
> I think for some of these functions it might be helpful just to name the section of the spec or have a link to the grammar portion of the spec.
> I know for me, I didn't understand what any of these names were until I read the grammar in the spec. 
> That said, I think it's good to follow the grammar production names used in the spec.

Make sense. I've added the links.

>> Source/JavaScriptCore/parser/Parser.cpp:2205
>> +    failIfFalse(match(STRING), "Expected a module specifier string");
> 
> I would say:
> "imported modules names must be string literals" or something similar as the error message here.

Looks nice to me. Fixed.

>> Source/JavaScriptCore/parser/Parser.cpp:2212
>> +template <class TreeBuilder> typename TreeBuilder::ImportSpecifier Parser<LexerType>::parseImportSpecifier(TreeBuilder& context, ImportSpecifierType specifierType)
> 
> lets call this parseImportClause because it seems most similar to that grammar production.

Since it does not include `{` / `}` for NamedImports, so I think parseImportClauseItem seems nice. I'll change it.

>> Source/JavaScriptCore/parser/Parser.cpp:2221
>> +        // * as ns
> 
> // * as namespace.

Fixed.

>> Source/JavaScriptCore/parser/Parser.cpp:2229
>> +        matchOrFail(IDENT, "Expected a local name for a targeted import declaration");
> 
> "Expected a variable name for the import declaration"

Fixed.

>> Source/JavaScriptCore/parser/Parser.cpp:2234
>> +    break;
> 
> break should be in above block statement

Fixed.

>> Source/JavaScriptCore/parser/Parser.cpp:2247
>> +            matchOrFail(IDENT, "Expected a local name for a targeted import declaration");
> 
> "Expected a variable name for the import declaration"

Fixed.

>> Source/JavaScriptCore/parser/Parser.cpp:2253
>> +    break;
> 
> ditto

Fixed.

>> Source/JavaScriptCore/parser/Parser.cpp:2262
>> +    break;
> 
> ditto.

Fixed.

>> Source/JavaScriptCore/parser/Parser.cpp:2265
>> +    semanticFailIfTrue(localNameToken.m_type & KeywordTokenFlag, "Expected an imported binding name");
> 
> I would say: "can't use keyword as imported binding name"

Fixed.

>> Source/JavaScriptCore/parser/Parser.cpp:2293
>> +    bool hasDefaultSpecifier = false;
> 
> Nit:
> Maybe the logic would be easier to read if we did something like this:
> bool isFinishedParsingImport = false;

Looks nice. I'll change the code to follow your suggestion.

>> Source/JavaScriptCore/parser/Parser.cpp:2298
>> +        hasDefaultSpecifier = true;
> 
> and then here have:
> if (match(COMMA)) next();
> else isFinishedParsing = true;

Fixed.

>> Source/JavaScriptCore/parser/Parser.cpp:2301
>> +    if (!hasDefaultSpecifier || match(COMMA)) {
> 
> and then switch this to:
> "if (!isFinishedParsingImport)".

Fixed.

>> Source/JavaScriptCore/parser/Parser.cpp:2302
>> +        if (hasDefaultSpecifier)
> 
> and remove this.

Fixed.

>> Source/JavaScriptCore/parser/Parser.cpp:2334
>> +            failWithMessage("Expected export specifiers");
> 
> "Expected namespace import or import list"

Fixed.

>> Source/JavaScriptCore/parser/Parser.cpp:2337
>> +    failIfFalse(matchContextualKeyword(m_vm->propertyNames->from), "Expected 'from' leading a module specifier");
> 
> "Expected 'from' before module name".
> In general, I think we should shy away from "specifier" in error names.
> To me "specifier" means a lot less than "name" in an error message.

Yup. I've dropped the "specifier" in error messages.

>> Source/JavaScriptCore/parser/Parser.cpp:2360
>> +        failIfFalse(matchIdentifierOrKeyword(), "Expected an exported name for a targeted export declaration");
> 
> So we can do: "export foo as let" but not "import let"?
> If so, the ES6 spec is pretty hilarious in what it allows/disallows.

Yeah. We can export the keyword named bindings, but we cannot import the keyword named bindings!
This is because we need to allow the following case for example.

In mod A
    export { func as let }

In mod B
    import * as A from "A"
    A.let();

>> Source/JavaScriptCore/parser/Parser.cpp:2375
>> +    next(); // EXPORT
> 
> nit: comment not needed.

Dropped.

>> Source/JavaScriptCore/parser/Parser.cpp:2380
>> +        next(); // TIMES
> 
> nit: comment not needed.

Dropped.

>> Source/JavaScriptCore/parser/Parser.cpp:2382
>> +        failIfFalse(matchContextualKeyword(m_vm->propertyNames->from), "Expected 'from' leading a module specifier");
> 
> "Expected 'from' before exported module name".

Fixed.

>> Source/JavaScriptCore/parser/Parser.cpp:2389
>> +    break;
> 
> this and below "break"s should be inside the block.

OK, here, we already have `return` at the end of the block. So Just dropped `break`.

>> Source/JavaScriptCore/parser/Parser.cpp:2394
>> +        // export default [lookahead â {function, class}] AssignmentExpression[In] ;
> 
> Looks like unicode 'not-in' epsilon character is being cut off.
> Maybe just write "lookahead not-in ..."

Looks nice. I've changed it.

>> Source/JavaScriptCore/parser/Parser.cpp:2462
>> +            while (match(COMMA)) {
> 
> If you switch to a do-while loop you can remove the above code.

Yeah, I think so. This while style is the one when I was using the intrusive list for export specifier list. Now, it's not true. So we can just use the do-while loop :)
Due to accepting the `export { A, }`, the loop becomes a little bit dirty, but do-while removes the duplicate entries.
Fixed.

[Yusuke Suzuki]

Committed r187890: <http://trac.webkit.org/changeset/187890>