[modules] AST and parser rework.

[modules] AST and parser rework.

Highlights:
- Record all imports and exports in the ModuleDescriptor.
- Remove ImportDeclaration; instead, introduce a new variable kind for imports.
- Set name on default exported anonymous functions.

Still to do: declaration of namespace imports.

BUG=v8:1569
Committed: https://crrev.com/0e000a87f1057e8eacc9516eb81d43664ab58d17
Cr-Commit-Position: refs/heads/master@{#37815}

[neis, 2016-07-13 17:02:34]

Patchset #1 (id:1) has been deleted

[neis, 2016-07-13 17:03:30]

Description was changed from

==========
[modules] Parser WIP
BUG=
==========

to

==========
[modules] Parser WIP
BUG=
COMMIT=false
==========

[neis, 2016-07-13 17:03:30]

neis@chromium.org changed reviewers:
+ adamk@chromium.org

[neis, 2016-07-13 17:03:41]

here we go

[adamk, 2016-07-13 18:38:22]

https://codereview.chromium.org/2108193003/diff/20001/src/ast/modules.cc
File src/ast/modules.cc (right):

https://codereview.chromium.org/2108193003/diff/20001/src/ast/modules.cc#newc...
src/ast/modules.cc:16: ModuleEntry* entry = new (zone->New(sizeof(ModuleEntry)))
ModuleEntry(loc);
If you make ModuleEntry a ZoneObject this gets cleaner.

https://codereview.chromium.org/2108193003/diff/20001/src/ast/modules.cc#newc...
src/ast/modules.cc:96: for (auto& it : exports_) {
"it" seems an odd name for this, as it's not an iterator..."entry"?

Also, the non-const ref looks funny. I guess the type is actually const
ModuleEntry*, so that's fine?

https://codereview.chromium.org/2108193003/diff/20001/src/ast/modules.h
File src/ast/modules.h (right):

https://codereview.chromium.org/2108193003/diff/20001/src/ast/modules.h#newcode8
src/ast/modules.h:8: #include "src/parsing/scanner.h"
This is just for Scanner::Location, right? Can you add a comment to that effect?
I suspect we might want to move that somewhere else, having an AST -> Parser
dependency seems kinda backwards.

https://codereview.chromium.org/2108193003/diff/20001/src/ast/modules.h#newco...
src/ast/modules.h:42: void AddNonStarImport(
This seems like the "common" case, how about "AddImport" for the name?

https://codereview.chromium.org/2108193003/diff/20001/src/ast/modules.h#newco...
src/ast/modules.h:57: void AddNonStarExport(
Same here and below, just "AddExport" and let "AddStarExport" speak for itself?

https://codereview.chromium.org/2108193003/diff/20001/src/ast/modules.h#newco...
src/ast/modules.h:75: // Accessors.
Don't think this comment is terribly useful or accurate anymore.

https://codereview.chromium.org/2108193003/diff/20001/src/ast/modules.h#newco...
src/ast/modules.h:77: bool Validate(
Add a comment here too?

https://codereview.chromium.org/2108193003/diff/20001/src/ast/modules.h#newco...
src/ast/modules.h:89: struct ModuleEntry {
Any reason not to have this extend ZoneObject?

https://codereview.chromium.org/2108193003/diff/20001/src/ast/modules.h#newco...
src/ast/modules.h:96: explicit ModuleEntry(const Scanner::Location loc)
No need for "const" here.

https://codereview.chromium.org/2108193003/diff/20001/src/contexts.cc
File src/contexts.cc (right):

https://codereview.chromium.org/2108193003/diff/20001/src/contexts.cc#newcode208
src/contexts.cc:208: *binding_flags = BINDING_IS_INITIALIZED;
Shouldn't this be checking the init_flag? like the other lexical bindings do?

https://codereview.chromium.org/2108193003/diff/20001/src/globals.h
File src/globals.h (right):

https://codereview.chromium.org/2108193003/diff/20001/src/globals.h#newcode899
src/globals.h:899: inline bool IsImmutableVariableMode(VariableMode mode) {
I think you'll want to add IMPORT here too (or a TODO).

https://codereview.chromium.org/2108193003/diff/20001/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/2108193003/diff/20001/src/parsing/parser.cc#n...
src/parsing/parser.cc:1342: void*
Parser::ParseModuleItemList(ZoneList<Statement*>* body, bool* ok) {
This doesn't seem to need a return value at all (could be "void" instead of
"void*"). ParseStatementList has other callers which is why it returns a void* I
think.

https://codereview.chromium.org/2108193003/diff/20001/src/parsing/parser.cc#n...
src/parsing/parser.cc:1457: DeclareImport(local_name, position(), CHECK_OK);  //
XXX
What's this XXX about? Should it be replaced with a TODO or removed?

https://codereview.chromium.org/2108193003/diff/20001/src/parsing/parser.cc#n...
src/parsing/parser.cc:1459: NamedImport* import = new
(zone()->New(sizeof(NamedImport)))
Same as elsewhere, if you make NamedImport a ZoneObject then this gets cleaner.

https://codereview.chromium.org/2108193003/diff/20001/src/parsing/parser.cc#n...
src/parsing/parser.cc:1508: DeclareImport(import_default_binding, pos,
CHECK_OK);  // XXX
???

https://codereview.chromium.org/2108193003/diff/20001/src/parsing/parser.cc#n...
src/parsing/parser.cc:1560: // DeclareImport(import_default_binding, pos,
CHECK_OK);
Are these what the XXXs are about?

https://codereview.chromium.org/2108193003/diff/20001/src/parsing/parser.cc#n...
src/parsing/parser.cc:1638: Runtime::kFunctionSetName, args, kNoSourcePosition);
Oh, I didn't understand your question this morning. You want the logic that's
currently in ParserTraits::SetFunctionNameFromIdentifierRef(), rather than this
runtime call. That'll set the name in the FunctionLiteral or ClassLiteral AST
node, which will then be properly overridden.

https://codereview.chromium.org/2108193003/diff/20001/src/parsing/parser.cc#n...
src/parsing/parser.cc:1735: if (length == 0) {
You can collapse this up into the outer if statement (I'd find it easier to
grok):

if (module_specifier == nullptr) {
} else if (length == 0) {
} else {
}

https://codereview.chromium.org/2108193003/diff/20001/src/parsing/parser.cc#n...
src/parsing/parser.cc:1740: auto& import_names = local_names;
Is this just for clarity? I'd leave it out.

https://codereview.chromium.org/2108193003/diff/20001/src/parsing/parser.cc#n...
src/parsing/parser.cc:2542: // TODO(neis): Better name.
Please expand on this TODO (why does it need a better name?) or remove it.

https://codereview.chromium.org/2108193003/diff/20001/src/parsing/parser.h
File src/parsing/parser.h (right):

https://codereview.chromium.org/2108193003/diff/20001/src/parsing/parser.h#ne...
src/parsing/parser.h:780: const Scanner::Location location)
No need for "const" here.

https://codereview.chromium.org/2108193003/diff/20001/src/runtime/runtime-fun...
File src/runtime/runtime-function.cc (right):

https://codereview.chromium.org/2108193003/diff/20001/src/runtime/runtime-fun...
src/runtime/runtime-function.cc:42: return *f;
I think you won't need this change after you change how this gets set.

https://codereview.chromium.org/2108193003/diff/20001/test/cctest/test-parsin...
File test/cctest/test-parsing.cc (right):

https://codereview.chromium.org/2108193003/diff/20001/test/cctest/test-parsin...
test/cctest/test-parsing.cc:5897: "export default (function koo() {});"
Can you also add an anonymous case? That'll be coverable once you've changed the
parser to set the default name at parse time.

https://codereview.chromium.org/2108193003/diff/20001/test/cctest/test-parsin...
test/cctest/test-parsing.cc:5926: // CHECK_EQ(1, descriptor->Length());
Is this stuff dead now?

[neis, 2016-07-14 10:28:24]

https://codereview.chromium.org/2108193003/diff/20001/src/ast/modules.cc
File src/ast/modules.cc (right):

https://codereview.chromium.org/2108193003/diff/20001/src/ast/modules.cc#newc...
src/ast/modules.cc:16: ModuleEntry* entry = new (zone->New(sizeof(ModuleEntry)))
ModuleEntry(loc);
On 2016/07/13 18:38:21, adamk wrote:
> If you make ModuleEntry a ZoneObject this gets cleaner.

Done.

https://codereview.chromium.org/2108193003/diff/20001/src/ast/modules.cc#newc...
src/ast/modules.cc:96: for (auto& it : exports_) {
On 2016/07/13 18:38:21, adamk wrote:
> "it" seems an odd name for this, as it's not an iterator..."entry"?
> 
> Also, the non-const ref looks funny. I guess the type is actually const
> ModuleEntry*, so that's fine?

Thanks, I forgot to update this when the data structure changed. Done.

https://codereview.chromium.org/2108193003/diff/20001/src/ast/modules.h
File src/ast/modules.h (right):

https://codereview.chromium.org/2108193003/diff/20001/src/ast/modules.h#newcode8
src/ast/modules.h:8: #include "src/parsing/scanner.h"
On 2016/07/13 18:38:21, adamk wrote:
> This is just for Scanner::Location, right? Can you add a comment to that
effect?

Yes. Done.

https://codereview.chromium.org/2108193003/diff/20001/src/ast/modules.h#newco...
src/ast/modules.h:42: void AddNonStarImport(
On 2016/07/13 18:38:21, adamk wrote:
> This seems like the "common" case, how about "AddImport" for the name?

Done.

https://codereview.chromium.org/2108193003/diff/20001/src/ast/modules.h#newco...
src/ast/modules.h:57: void AddNonStarExport(
On 2016/07/13 18:38:21, adamk wrote:
> Same here and below, just "AddExport" and let "AddStarExport" speak for
itself?

Done.

https://codereview.chromium.org/2108193003/diff/20001/src/ast/modules.h#newco...
src/ast/modules.h:75: // Accessors.
On 2016/07/13 18:38:21, adamk wrote:
> Don't think this comment is terribly useful or accurate anymore.

Removed this and the others.

https://codereview.chromium.org/2108193003/diff/20001/src/ast/modules.h#newco...
src/ast/modules.h:77: bool Validate(
On 2016/07/13 18:38:21, adamk wrote:
> Add a comment here too?

Done.

https://codereview.chromium.org/2108193003/diff/20001/src/ast/modules.h#newco...
src/ast/modules.h:89: struct ModuleEntry {
On 2016/07/13 18:38:21, adamk wrote:
> Any reason not to have this extend ZoneObject?

No, thans for the suggestion. Done.

https://codereview.chromium.org/2108193003/diff/20001/src/ast/modules.h#newco...
src/ast/modules.h:96: explicit ModuleEntry(const Scanner::Location loc)
On 2016/07/13 18:38:21, adamk wrote:
> No need for "const" here.

Done.

https://codereview.chromium.org/2108193003/diff/20001/src/contexts.cc
File src/contexts.cc (right):

https://codereview.chromium.org/2108193003/diff/20001/src/contexts.cc#newcode208
src/contexts.cc:208: *binding_flags = BINDING_IS_INITIALIZED;
On 2016/07/13 18:38:21, adamk wrote:
> Shouldn't this be checking the init_flag? like the other lexical bindings do?

Need to think about this.

https://codereview.chromium.org/2108193003/diff/20001/src/globals.h
File src/globals.h (right):

https://codereview.chromium.org/2108193003/diff/20001/src/globals.h#newcode899
src/globals.h:899: inline bool IsImmutableVariableMode(VariableMode mode) {
On 2016/07/13 18:38:21, adamk wrote:
> I think you'll want to add IMPORT here too (or a TODO).

Yeah, probably. Added it. I suspect there are many more places where we'll have
to account for the new mode.

https://codereview.chromium.org/2108193003/diff/20001/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/2108193003/diff/20001/src/parsing/parser.cc#n...
src/parsing/parser.cc:1342: void*
Parser::ParseModuleItemList(ZoneList<Statement*>* body, bool* ok) {
On 2016/07/13 18:38:21, adamk wrote:
> This doesn't seem to need a return value at all (could be "void" instead of
> "void*"). ParseStatementList has other callers which is why it returns a void*
I
> think.

void* is needed because of the beautiful CHECK_OK macro.

https://codereview.chromium.org/2108193003/diff/20001/src/parsing/parser.cc#n...
src/parsing/parser.cc:1457: DeclareImport(local_name, position(), CHECK_OK);  //
XXX
On 2016/07/13 18:38:22, adamk wrote:
> What's this XXX about? Should it be replaced with a TODO or removed?

Just an indicator for you (see below). Removed now.

https://codereview.chromium.org/2108193003/diff/20001/src/parsing/parser.cc#n...
src/parsing/parser.cc:1459: NamedImport* import = new
(zone()->New(sizeof(NamedImport)))
On 2016/07/13 18:38:22, adamk wrote:
> Same as elsewhere, if you make NamedImport a ZoneObject then this gets
cleaner.

Done.

https://codereview.chromium.org/2108193003/diff/20001/src/parsing/parser.cc#n...
src/parsing/parser.cc:1508: DeclareImport(import_default_binding, pos,
CHECK_OK);  // XXX
On 2016/07/13 18:38:22, adamk wrote:
> ???

Done.

https://codereview.chromium.org/2108193003/diff/20001/src/parsing/parser.cc#n...
src/parsing/parser.cc:1560: // DeclareImport(import_default_binding, pos,
CHECK_OK);
On 2016/07/13 18:38:22, adamk wrote:
> Are these what the XXXs are about?

Exactly.

https://codereview.chromium.org/2108193003/diff/20001/src/parsing/parser.cc#n...
src/parsing/parser.cc:1638: Runtime::kFunctionSetName, args, kNoSourcePosition);
On 2016/07/13 18:38:22, adamk wrote:
> Oh, I didn't understand your question this morning. You want the logic that's
> currently in ParserTraits::SetFunctionNameFromIdentifierRef(), rather than
this
> runtime call. That'll set the name in the FunctionLiteral or ClassLiteral AST
> node, which will then be properly overridden.

Ah, I see.  Done (with some refactoring of the existing SetFunctionNameFromFoo
functions).

https://codereview.chromium.org/2108193003/diff/20001/src/parsing/parser.cc#n...
src/parsing/parser.cc:1735: if (length == 0) {
On 2016/07/13 18:38:22, adamk wrote:
> You can collapse this up into the outer if statement (I'd find it easier to
> grok):
> 
> if (module_specifier == nullptr) {
> } else if (length == 0) {
> } else {
> }

Done. Also removed the TODO since the exact location doesn't really matter for
an empty import.

https://codereview.chromium.org/2108193003/diff/20001/src/parsing/parser.cc#n...
src/parsing/parser.cc:1740: auto& import_names = local_names;
On 2016/07/13 18:38:22, adamk wrote:
> Is this just for clarity? I'd leave it out.

Yes. Done. Instead, I renamed local_names to original_names.

https://codereview.chromium.org/2108193003/diff/20001/src/parsing/parser.cc#n...
src/parsing/parser.cc:2542: // TODO(neis): Better name.
On 2016/07/13 18:38:22, adamk wrote:
> Please expand on this TODO (why does it need a better name?) or remove it.

This doesn't belong into this CL, removed.

https://codereview.chromium.org/2108193003/diff/20001/src/parsing/parser.h
File src/parsing/parser.h (right):

https://codereview.chromium.org/2108193003/diff/20001/src/parsing/parser.h#ne...
src/parsing/parser.h:780: const Scanner::Location location)
On 2016/07/13 18:38:22, adamk wrote:
> No need for "const" here.

Done.

https://codereview.chromium.org/2108193003/diff/20001/src/runtime/runtime-fun...
File src/runtime/runtime-function.cc (right):

https://codereview.chromium.org/2108193003/diff/20001/src/runtime/runtime-fun...
src/runtime/runtime-function.cc:42: return *f;
On 2016/07/13 18:38:22, adamk wrote:
> I think you won't need this change after you change how this gets set.

Done.

https://codereview.chromium.org/2108193003/diff/20001/test/cctest/test-parsin...
File test/cctest/test-parsing.cc (right):

https://codereview.chromium.org/2108193003/diff/20001/test/cctest/test-parsin...
test/cctest/test-parsing.cc:5897: "export default (function koo() {});"
On 2016/07/13 18:38:22, adamk wrote:
> Can you also add an anonymous case? That'll be coverable once you've changed
the
> parser to set the default name at parse time.

Not sure what you mean. This module already has a default export, so adding
another one will cause an error. Also, I'm not checking the .name property here
anywhere.

https://codereview.chromium.org/2108193003/diff/20001/test/cctest/test-parsin...
test/cctest/test-parsing.cc:5926: // CHECK_EQ(1, descriptor->Length());
On 2016/07/13 18:38:22, adamk wrote:
> Is this stuff dead now?

Yes, removed. Will most likely re-add such tests later when we have a way of
inspecting the exports/imports.

[adamk, 2016-07-14 18:26:33]

This looks good to me with an updated/fleshed-out CL description.

https://codereview.chromium.org/2108193003/diff/20001/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/2108193003/diff/20001/src/parsing/parser.cc#n...
src/parsing/parser.cc:1342: void*
Parser::ParseModuleItemList(ZoneList<Statement*>* body, bool* ok) {
On 2016/07/14 10:28:24, neis wrote:
> On 2016/07/13 18:38:21, adamk wrote:
> > This doesn't seem to need a return value at all (could be "void" instead of
> > "void*"). ParseStatementList has other callers which is why it returns a
void*
> I
> > think.
> 
> void* is needed because of the beautiful CHECK_OK macro.

Except no one calls this with CHECK_OK. But I guess someone might in future,
doesn't matter much either way.

[neis, 2016-07-15 08:17:06]

>
https://codereview.chromium.org/2108193003/diff/20001/src/parsing/parser.cc#n...
> src/parsing/parser.cc:1342: void*
> Parser::ParseModuleItemList(ZoneList<Statement*>* body, bool* ok) {
> On 2016/07/14 10:28:24, neis wrote:
> > On 2016/07/13 18:38:21, adamk wrote:
> > > This doesn't seem to need a return value at all (could be "void" instead
of
> > > "void*"). ParseStatementList has other callers which is why it returns a
> void*
> > I
> > > think.
> > 
> > void* is needed because of the beautiful CHECK_OK macro.
> 
> Except no one calls this with CHECK_OK. But I guess someone might in future,
> doesn't matter much either way.

CHECK_OK is used inside the function itself.

[neis, 2016-07-15 08:35:37]

Description was changed from

==========
[modules] Parser WIP
BUG=
COMMIT=false
==========

to

==========
[modules] Parser rework.

Highlights:
- Record all imports and exports in the ModuleDescriptor.
- Remove ImportDeclaration; instead, introduce a new variable kind for imports.
- Set name on default exported anonymous functions.

Still to do: declaration of namespace imports.

BUG=v8:1569
==========

[neis, 2016-07-15 08:46:32]

https://codereview.chromium.org/2108193003/diff/20001/src/contexts.cc
File src/contexts.cc (right):

https://codereview.chromium.org/2108193003/diff/20001/src/contexts.cc#newcode208
src/contexts.cc:208: *binding_flags = BINDING_IS_INITIALIZED;
On 2016/07/14 10:28:23, neis wrote:
> On 2016/07/13 18:38:21, adamk wrote:
> > Shouldn't this be checking the init_flag? like the other lexical bindings
do?
> 
> Need to think about this.

For now, I'm adding IMPORT to the UNREACHABLE case.

[neis, 2016-07-15 08:49:16]

neis@chromium.org changed reviewers:
+ ahaas@chromium.org, bmeurer@chromium.org

[neis, 2016-07-15 08:50:58]

+ahaas for asmjs, +bmeurer for compiler

[ahaas, 2016-07-15 08:52:41]

On 2016/07/15 at 08:50:58, neis wrote:
> +ahaas for asmjs, +bmeurer for compiler

wasm lgtm

[neis, 2016-07-15 08:54:31]

Description was changed from

==========
[modules] Parser rework.

Highlights:
- Record all imports and exports in the ModuleDescriptor.
- Remove ImportDeclaration; instead, introduce a new variable kind for imports.
- Set name on default exported anonymous functions.

Still to do: declaration of namespace imports.

BUG=v8:1569
==========

to

==========
[modules] AST and parser rework.

Highlights:
- Record all imports and exports in the ModuleDescriptor.
- Remove ImportDeclaration; instead, introduce a new variable kind for imports.
- Set name on default exported anonymous functions.

Still to do: declaration of namespace imports.

BUG=v8:1569
==========

[adamk, 2016-07-15 18:27:53]

lgtm

[Benedikt Meurer, 2016-07-16 12:59:22]

lgtm

[neis, 2016-07-18 07:03:58]

The CQ bit was checked by neis@chromium.org

[neis, 2016-07-18 07:03:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, adamk@chromium.org, ahaas@chromium.org
Link to the patchset: https://codereview.chromium.org/2108193003/#ps100001
(title: "Rebase.")

[commit-bot: I haz the power, 2016-07-18 07:04:10]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-18 07:27:16]

Description was changed from

==========
[modules] AST and parser rework.

Highlights:
- Record all imports and exports in the ModuleDescriptor.
- Remove ImportDeclaration; instead, introduce a new variable kind for imports.
- Set name on default exported anonymous functions.

Still to do: declaration of namespace imports.

BUG=v8:1569
==========

to

==========
[modules] AST and parser rework.

Highlights:
- Record all imports and exports in the ModuleDescriptor.
- Remove ImportDeclaration; instead, introduce a new variable kind for imports.
- Set name on default exported anonymous functions.

Still to do: declaration of namespace imports.

BUG=v8:1569
==========

[commit-bot: I haz the power, 2016-07-18 07:27:17]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-07-18 07:29:35]

Description was changed from

==========
[modules] AST and parser rework.

Highlights:
- Record all imports and exports in the ModuleDescriptor.
- Remove ImportDeclaration; instead, introduce a new variable kind for imports.
- Set name on default exported anonymous functions.

Still to do: declaration of namespace imports.

BUG=v8:1569
==========

to

==========
[modules] AST and parser rework.

Highlights:
- Record all imports and exports in the ModuleDescriptor.
- Remove ImportDeclaration; instead, introduce a new variable kind for imports.
- Set name on default exported anonymous functions.

Still to do: declaration of namespace imports.

BUG=v8:1569

Committed: https://crrev.com/0e000a87f1057e8eacc9516eb81d43664ab58d17
Cr-Commit-Position: refs/heads/master@{#37815}
==========

[commit-bot: I haz the power, 2016-07-18 07:29:36]

Patchset 5 (id:??) landed as
https://crrev.com/0e000a87f1057e8eacc9516eb81d43664ab58d17
Cr-Commit-Position: refs/heads/master@{#37815}