Add parser support for ES6 module feature #293

Merged
merged 1 commit into from Feb 11, 2016

Projects

None yet
@boingoing
Member

Initial support for handling import and export statements in the Chakra
parser. It is gated behind a config flag (-ES6Module).

We are treating the import and export keywords like a tag for declarations
or identifier references which should be physically stored in a special
slot array which we need to build up at module link time (or, at least
before bytecode generation time). The array is not built up as part of
this change but we are collecting all the exports, imports, and referenced
modules such that building that array should be possible. Once we build
this array, accesses to identifiers which are created via an import
keyword will load from that slot array and accesses to identifiers
declared via an export keyword will load and store from the same slot
array.

Currently, we are not hooking up export declarations to their imported
usages. If you import a binding from another module and access it, we will
only guarantee that the binding exists. The value of the binding will
always be undefined.

We are keeping track of exports, imports, and referenced modules in the
top-level ParseNode of the parser (which is subsequently handed-off to the
ByteCodeGenerator). When parsing a module we set a flag (fscrIsModuleCode)
which also causes us to create the root ParseNode as a knopModule type
node. That is in turn used to allocate the ParseNode itself as a PnModule
node, which derives from PnProg but adds the lists for imports, exports,
and referenced modules. We are still treating the top-level ParseNode as a
knopProg (which is to say, we reset the nop to knopProg after allocating
it) because we handle the case of knopProg everywhere and we don't need to
complicate things by also needing to handle knopModule. Instead we just
use the PnModule extensions when we are in a module-specific parsing path
and otherwise treat the node as if it was a PnProg ParseNode anyway. Note
that import and export statements do not themselves have special ParseNode
types.

This change adds an experimental method to Jsrt (JsExperimentalRunModule)
and a ch.exe hook (WScript.LoadModuleFile) for loading a module from a
file. Loading a module via this hook acts similarly to including a module
in a script tag like <script type="module">. The experimental method is
subject to change as the feature evolves.

A couple of points about module code. Module code is always strict code.
Import and export statements may only occur as statements at global scope

  • they may not be located inside functions or inside eval'd code.

For more information about ES6 modules, please see the spec
http://tc39.github.io/ecma262/#sec-modules.

@msftclas

Hi @boingoing, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!


It looks like you're a Microsoft contributor (Taylor Woll). If you're full-time, we DON'T require a Contribution License Agreement. If you are a vendor, please DO sign the electronic Contribution License Agreement. It will take 2 minutes and there's no faxing! https://cla.microsoft.com.

TTYL, MSBOT;

@bterlson
Member

So great to see modules moving forward!! Can't wait :-D

@dilijev
Member
dilijev commented Feb 10, 2016

Is this the beginning of a modules feature branch or do you want to merge this into master and continue that way?

@boingoing
Member

We are planning to work on the modules feature in the master branch. Since we have to coordinate with browser folks, it will be better if we can take advantage of existing machinery to get our code from master to them as quickly as possible.

@abchatra
Contributor

๐Ÿ‘ So cool to see this coming!

@Yongqu Yongqu and 1 other commented on an outdated diff Feb 10, 2016
test/es6/module/SyntaxErrorImportStatement9.js
@@ -0,0 +1,8 @@
+//-------------------------------------------------------------------------------------------------------
+// Copyright (C) Microsoft. All rights reserved.
+// Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
+//-------------------------------------------------------------------------------------------------------
+
+// Syntax error if import statement has default binding after named import list
+
+import { foo }, bar from "ValidExportStatements.js";
@Yongqu
Yongqu Feb 10, 2016 Contributor

some additional invalid cases like export 12; export "hello", import {foo as 12} etc.?

@boingoing
boingoing Feb 11, 2016 Member

Good examples, thanks

@Yongqu Yongqu commented on the diff Feb 11, 2016
lib/Parser/parse.cpp
+bool Parser::IsImportOrExportStatementValidHere()
+{
+ // Import must be located in the global scope of the module.
+ return GetCurrentFunctionNode()->nop == knopProg
+ && (this->m_grfscr & fscrEvalCode) != fscrEvalCode
+ && this->m_tryCatchOrFinallyDepth == 0
+ && this->m_currentBlockInfo->pBlockInfoOuter == nullptr;
+}
+
+template<bool buildAST>
+ParseNodePtr Parser::ParseImportDeclaration()
+{
+ Assert(m_scriptContext->GetConfig()->IsES6ModuleEnabled());
+ Assert(m_token.tk == tkIMPORT);
+
+ if (!IsImportOrExportStatementValidHere())
@Yongqu
Yongqu Feb 11, 2016 Contributor

Can buildAST be false for moduleimport parsing?

@boingoing
boingoing Feb 11, 2016 Member

Supposedly it should always be true since we don't defer global-scope. If buildAST is false, this should return us the deferred function which isn't knopProg and which indicates the import is inside a function. I suppose we could check !buildAST here, as well.

@bmeck
bmeck Feb 11, 2016

aren't imports top level only?

@boingoing
boingoing Feb 11, 2016 Member

Yes, import statements are only valid at top-level, outside of any nested scopes. If we see an import statement (inside any scope except for global) while parsing the source text we will need to throw a SyntaxError.

@Yongqu Yongqu and 1 other commented on an outdated diff Feb 11, 2016
lib/Parser/parse.cpp
+
+ IdentPtr exportName = wellKnownPropertyPids.default;
+ IdentPtr localName;
+
+ if (pnode->nop == knopFncDecl && pnode->sxFnc.pid != nullptr)
+ {
+ localName = pnode->sxFnc.pid;
+ }
+ else if (pnode->nop == knopClassDecl && pnode->sxClass.pnodeName != nullptr)
+ {
+ localName = pnode->sxClass.pnodeName->sxVar.pid;
+ }
+ else
+ {
+ // Consider: Is this observable? Can we get away with leaving localName null in this case?
+ localName = CreatePid(L"*default*", sizeof("*default*") -1);
@Yongqu
Yongqu Feb 11, 2016 Contributor

can we create an wellKnwonPropertyPids entry as well?

@Yongqu Yongqu and 1 other commented on an outdated diff Feb 11, 2016
lib/Parser/parse.cpp
+
+ // Token following * must be the identifier 'as'
+ m_pscan->Scan();
+ if (m_token.tk != tkID || wellKnownPropertyPids.as != m_token.GetIdentifier(m_phtbl))
+ {
+ Error(ERRsyntax);
+ }
+
+ // Token following 'as' must be a binding identifier.
+ m_pscan->Scan();
+ ChkCurTokNoScan(tkID, ERRsyntax);
+
+ if (buildAST)
+ {
+ IdentPtr localName = m_token.GetIdentifier(m_phtbl);
+ IdentPtr importName = CreatePid(L"*", 1);
@Yongqu
Yongqu Feb 11, 2016 Contributor

wellKnownPropertyPids?

@boingoing
boingoing Feb 11, 2016 Member

Added wellKnownPropertyPids._star for this (and wellKnownPropertyPids._starDefaultStar for the other one.

@Yongqu
Contributor
Yongqu commented Feb 11, 2016

Looks great!

@ianwjhalliday ianwjhalliday and 2 others commented on an outdated diff Feb 11, 2016
lib/jsrt/JsrtCommonExports.inc
@@ -11,6 +11,7 @@
JsGetContextData
JsSetContextData
JsRunScript
+ JsExperimentalRunModule
@ianwjhalliday
ianwjhalliday Feb 11, 2016 Member

I kinda want this to be named JsExperimentalApiRunModule so that the self reference is clear and unambiguous. What do you think?

@boingoing
boingoing Feb 11, 2016 Member

Yeah, that is a little more clear about what is experimental.

@ianwjhalliday ianwjhalliday and 2 others commented on an outdated diff Feb 11, 2016
test/es6/module-syntax.js
+
+// ES6 Module syntax tests -- verifies syntax of import and export statements
+
+WScript.LoadScriptFile("..\\UnitTestFramework\\UnitTestFramework.js");
+
+var tests = [
+ {
+ name: "All valid (non-default) export statements",
+ body: function () {
+ assert.doesNotThrow(function () { WScript.LoadModuleFile('.\\module\\ValidExportStatements.js', 'samethread'); }, "Valid export statements");
+ }
+ },
+ {
+ name: "Valid default export statements",
+ body: function () {
+ assert.doesNotThrow(function () { WScript.LoadModuleFile('.\\module\\ValidExportDefaultStatement1.js', 'samethread'); }, "Unnamed function expression default export");
@ianwjhalliday
ianwjhalliday Feb 11, 2016 Member

For test readability would it make sense to have a LoadModuleScript api that takes a string of the module contents rather than loading a file?

With the tests in separate individual files its hard to look at them all together to compare and contrast.

@tcare
tcare Feb 11, 2016 Member

Good idea

@boingoing
boingoing Feb 11, 2016 Member

I didn't do this for two reasons. First the test hook is experimental and might get replaced with a proper API. Second, different files makes it much easier to simulate real modules.

You're right, though. Figuring out what each of these files is testing is a pain. I went ahead and fleshed-out LoadModule in WScriptJsrt to load a module script from a buffer. The test cases are much more readable.

@ianwjhalliday ianwjhalliday commented on the diff Feb 11, 2016
lib/Parser/parse.cpp
@@ -1286,6 +1295,29 @@ ParseNodePtr Parser::CreateNodeWithScanner(charcount_t ichMin)
return CreateNodeT<nop>(ichMin, m_pscan->IchLimTok());
}
+ParseNodePtr Parser::CreateProgNodeWithScanner(bool isModuleSource)
+{
+ ParseNodePtr pnodeProg;
+
+ if (isModuleSource)
+ {
+ pnodeProg = CreateNodeWithScanner<knopModule>();
+
+ // knopModule is not actually handled anywhere since we would need to handle it everywhere we could
+ // have knopProg and it would be treated exactly the same except for import/export statements.
+ // We are only using it as a way to get the correct size for PnModule.
+ // Consider: Should we add a flag to PnProg which is false but set to true in PnModule?
+ // If we do, it can't be a virtual method since the parse nodes are all in a union.
@ianwjhalliday
ianwjhalliday Feb 11, 2016 Member

I would also argue that it can't be virtual because even if the parse nodes were made a proper class hierarchy instead of a union thing that hierarchy should be non-virtual to remain lean (virtualizing the hierarchy would add vtables to all node objects). I have a long back logged dream of refactoring the parse nodes into a proper hierarchy and doing away with the union and manual sizing and initialization craziness. Never time for doing it though.

@pleath
pleath Feb 11, 2016 Member

I think in this room dreams about introducing real hierarchy to ParseNode are more common than dreams about being able to fly.

--Paul

From: Ian Halliday [mailto:notifications@github.com]
Sent: Wednesday, February 10, 2016 5:24 PM
To: Microsoft/ChakraCore ChakraCore@noreply.github.com
Subject: Re: [ChakraCore] Add parser support for ES6 module feature (#293)

In lib/Parser/parse.cpphttps://github.com/Microsoft/ChakraCore/pull/293#discussion_r52554390:

@@ -1286,6 +1295,29 @@ ParseNodePtr Parser::CreateNodeWithScanner(charcount_t ichMin)

 return CreateNodeT<nop>(ichMin, m_pscan->IchLimTok());

}

+ParseNodePtr Parser::CreateProgNodeWithScanner(bool isModuleSource)

+{

  • ParseNodePtr pnodeProg;
  • if (isModuleSource)
  • {
  •    pnodeProg = CreateNodeWithScanner<knopModule>();
    
  •    // knopModule is not actually handled anywhere since we would need to handle it everywhere we could
    
  •    // have knopProg and it would be treated exactly the same except for import/export statements.
    
  •    // We are only using it as a way to get the correct size for PnModule.
    
  •    // Consider: Should we add a flag to PnProg which is false but set to true in PnModule?
    
  •    //           If we do, it can't be a virtual method since the parse nodes are all in a union.
    

I would also argue that it can't be virtual because even if the parse nodes were made a proper class hierarchy instead of a union thing that hierarchy should be non-virtual to remain lean (virtualizing the hierarchy would add vtables to all node objects). I have a long back logged dream of refactoring the parse nodes into a proper hierarchy and doing away with the union and manual sizing and initialization craziness. Never time for doing it though.

โ€”
Reply to this email directly or view it on GitHubhttps://github.com/Microsoft/ChakraCore/pull/293/files#r52554390.

@ianwjhalliday
ianwjhalliday Feb 11, 2016 Member

:) I prototyped it once while I was on my road trip last year. Hit some walls and got back from the trip with real priorities to attend to... the stashed change has long since decayed. A fair lot of it might still be applicable. One of these days...

@tcare tcare commented on the diff Feb 11, 2016
lib/Parser/parse.h
@@ -506,6 +512,26 @@ class Parser
}
public:
+ IdentPtrList* GetRequestedModulesList();
@tcare
tcare Feb 11, 2016 Member

const for all these getters?

@tcare
tcare Feb 11, 2016 Member

same in bytecodegen

@ianwjhalliday
ianwjhalliday Feb 11, 2016 Member

const is against our coding conventions; I believe the argument is that we don't use const objects and so putting it on small accessors gains us nothing but ensure those accessors don't modify any fields, which is trivially obvious from the source of the accessor itself.

@tcare tcare commented on an outdated diff Feb 11, 2016
lib/Parser/parse.cpp
@@ -7932,7 +8523,7 @@ ParseNodePtr Parser::ParseVariableDeclaration(
ulong nameHintLength = 0;
ulong nameHintOffset = 0;
Assert(declarationType == tkVAR || declarationType == tkCONST || declarationType == tkLET);
-
+
@tcare
tcare Feb 11, 2016 Member

Trailing whitespace

@tcare tcare commented on the diff Feb 11, 2016
test/es6/rlexe.xml
@@ -1128,4 +1128,12 @@
<compile-flags>-args summary -endargs</compile-flags>
</default>
</test>
+
+<test>
+ <default>
+ <files>module-syntax.js</files>
+ <compile-flags>-ES6Module -args summary -endargs</compile-flags>
@tcare
tcare Feb 11, 2016 Member

Do we want a force:deferparse variant?

@boingoing
boingoing Feb 11, 2016 Member

Probably a good idea.

@ianwjhalliday ianwjhalliday commented on an outdated diff Feb 11, 2016
lib/Parser/parse.cpp
+ModuleExportEntryList* Parser::EnsureModuleStarExportEntryList()
+{
+ if (m_currentNodeProg->sxModule.starExportEntries == nullptr)
+ {
+ m_currentNodeProg->sxModule.starExportEntries = Anew(&m_nodeAllocator, ModuleExportEntryList, &m_nodeAllocator);
+ }
+ return m_currentNodeProg->sxModule.starExportEntries;
+}
+
+void Parser::AddModuleImportEntry(ModuleImportEntryList* importEntryList, IdentPtr importName, IdentPtr localName, IdentPtr moduleRequest, ParseNodePtr declNode)
+{
+ ModuleImportEntry* importEntry = Anew(&m_nodeAllocator, ModuleImportEntry);
+
+ if (importEntry == nullptr)
+ {
+ OutOfMemory();
@ianwjhalliday
ianwjhalliday Feb 11, 2016 Member

I believe m_nodeAllocator already will through OOM if it cannot allocate

@tcare
Member
tcare commented Feb 11, 2016

Big change! Looks good.

@ianwjhalliday
Member

Agreed! Looking real good!

@Yongqu Yongqu referenced this pull request Feb 11, 2016
Merged

initial module checkin #297

@boingoing boingoing Add parser support for ES6 module feature
Initial support for handling import and export statements in the Chakra
parser. It is gated behind a config flag (-ES6Module).

We are treating the import and export keywords like a tag for declarations
or identifier references which should be physically stored in a special
slot array which we need to build up at module link time (or, at least
before bytecode generation time). The array is not built up as part of
this change but we are collecting all the exports, imports, and referenced
modules such that building that array should be possible. Once we build
this array, accesses to identifiers which are created via an import
keyword will load from that slot array and accesses to identifiers
declared via an export keyword will load and store from the same slot
array.

Currently, we are not hooking up export declarations to their imported
usages. If you import a binding from another module and access it, we will
only guarantee that the binding exists. The value of the binding will
always be undefined.

We are keeping track of exports, imports, and referenced modules in the
top-level ParseNode of the parser (which is subsequently handed-off to the
ByteCodeGenerator). When parsing a module we set a flag (fscrIsModuleCode)
which also causes us to create the root ParseNode as a knopModule type
node. That is in turn used to allocate the ParseNode itself as a PnModule
node, which derives from PnProg but adds the lists for imports, exports,
and referenced modules. We are still treating the top-level ParseNode as a
knopProg (which is to say, we reset the nop to knopProg after allocating
it) because we handle the case of knopProg everywhere and we don't need to
complicate things by also needing to handle knopModule. Instead we just
use the PnModule extensions when we are in a module-specific parsing path
and otherwise treat the node as if it was a PnProg ParseNode anyway. Note
that import and export statements do not themselves have special ParseNode
types.

This change adds an experimental method to Jsrt (JsExperimentalRunModule)
and a ch.exe hook (WScript.LoadModuleFile) for loading a module from a
file. Loading a module via this hook acts similarly to including a module
in a script tag like <script type="module">. The experimental method is
subject to change as the feature evolves.

A couple of points about module code. Module code is always strict code.
Import and export statements may only occur as statements at global scope
- they may not be located inside functions or inside eval'd code.

For more information about ES6 modules, please see the spec
http://tc39.github.io/ecma262/#sec-modules.
2c33faf
@chakrabot chakrabot merged commit 2c33faf into master Feb 11, 2016

12 checks passed

Copyright Check Build finished. No test results found.
Details
EOL Check Build finished. No test results found.
Details
Pre-Merge signing tool Sign-off
Details
Windows arm_debug Build finished. No test results found.
Details
Windows arm_release Build finished. No test results found.
Details
Windows arm_test Build finished. No test results found.
Details
Windows x64_debug Build finished. No test results found.
Details
Windows x64_release Build finished. No test results found.
Details
Windows x64_test Build finished. No test results found.
Details
Windows x86_debug Build finished. No test results found.
Details
Windows x86_release Build finished. No test results found.
Details
Windows x86_test Build finished. No test results found.
Details
@chakrabot chakrabot pushed a commit that referenced this pull request Feb 16, 2016
@Yongqu Yongqu [MERGE #297] initial module checkin
Merge pull request #297 from pr/yongqu/module
    This is initial runtime support for module ( parser change at
#293).
    This PR contain the basic support for host integration, and it can act as basic bootstrap to
enable edge support later.
    Current es6 module spec does not clarify the dependency module loading sequence, and module
loader spec is not finalized yet. At this time, we will allow browser host to start from scripttype='module'
tag, and allow host to asynchronously fetch child module sources. This is in effect adding a small state
machine between ParseModule and ModuleDeclarationInstantiation. Once all modules are parsed and
initialized, the script engine notifies the host, and ModuleEvaluation of top level module will start
asynchronously, initiated from the host.
    There are still a lot of unimplemented features, including experimental JSRT APIs that match
execution model, namespace object and ResolveExports, error handling etc. Also this change is
developed in parallel to the parser change it will be integrated with the parser part later.
d8ff8c3
@boingoing boingoing deleted the pr/boingoing/moduleparsercore branch Feb 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment