Summary: | [ES6] Support Module Syntax | ||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Yusuke Suzuki <ysuzuki> | ||||||||||||||||||||||||||||||
Component: | JavaScriptCore | Assignee: | Yusuke Suzuki <ysuzuki> | ||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | barraclough, buildbot, commit-queue, darin, fealebenpae, fpizlo, ggaren, ivanbuhov2, mark.lam, oliver, rniwa, saam, sam | ||||||||||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||||||||||||||
Bug Blocks: | 147340, 147353 | ||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Yusuke Suzuki
2015-07-29 14:57:21 PDT
Created attachment 257800 [details]
Patch
Created attachment 257801 [details]
Patch
I've rebased the patch. Ready for review. 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.
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? 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 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
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 :) Created attachment 257825 [details]
Patch
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.
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 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
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 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
Created attachment 257834 [details]
Patch
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.
Created attachment 257836 [details]
Patch
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.
tests are passed. the patch is ready for review. 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.
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. 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. Created attachment 258003 [details]
Patch
Created attachment 258004 [details]
Patch
Created attachment 258005 [details]
Patch
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.
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. 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 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
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 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
Created attachment 258012 [details]
Patch
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.
The patch is ready :) 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. 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. Committed r187890: <http://trac.webkit.org/changeset/187890> |