Before executing the body of the modules, we need to collect the dependencies. (import / export) This issue involves modules parsing change.
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
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.
Created attachment 257764 [details] Patch WIP, parsing module related syntax
Before landing the preparing system (I'm planning to create the new builder that inherits SyntaxChecker), just creating the patch to add parsing functionality.
Created attachment 258679 [details] Patch WIP
Created attachment 258696 [details] Patch
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.
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.
Created attachment 258697 [details] Patch
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.
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.
Comment on attachment 258697 [details] Patch r=me
(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 :)
Committed r188355: <http://trac.webkit.org/changeset/188355>
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.
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.
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.
Reopening to attach new patch.
Created attachment 258918 [details] Patch
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.
Comment on attachment 258918 [details] Patch The follow-up patch comes here now!
Created attachment 258928 [details] Patch
Just tweak the ChangeLog.
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.
Comment on attachment 258928 [details] Patch Thank you for your review :D
Committed r188417: <http://trac.webkit.org/changeset/188417>