Re-introduce ImportDeclaration to the parser

Re-introduce ImportDeclaration to the parser

This also adds a new VariableMode, IMPORT, which will be
used to do appropriate binding for Import-declared Variables.

Only named imports are handled for now. "import *" and default
import syntaxes have had their TODOs adjusted to match the new
code structure.

BUG=v8:1569
LOG=n

Committed: https://crrev.com/fa293dd79f78074a0610739cb8f669849bf5704a
Cr-Commit-Position: refs/heads/master@{#26895}

[adamk, 2015-02-25 01:19:54]

adamk@chromium.org changed reviewers:
+ arv@chromium.org, rossberg@chromium.org

[adamk, 2015-02-25 01:19:55]

This was easier than I expected, so it follows hot-on-the-heels of the
referenced cleanup patch. Review that one first.

[rossberg, 2015-02-25 13:55:49]

https://codereview.chromium.org/948303004/diff/1/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/948303004/diff/1/src/ast.h#newcode613
src/ast.h:613: const AstRawString* module_specifier() const { return
module_specifier_; }
Hm, is it necessary that the specifier is duplicated on every importdecl node?
That doesn't make sense from an AST perspective, and the fact that you then need
a setter doesn't make it better.

https://codereview.chromium.org/948303004/diff/1/src/parser.h
File src/parser.h (right):

https://codereview.chromium.org/948303004/diff/1/src/parser.h#newcode716
src/parser.h:716: ZoneList<ImportDeclaration*>* ParseNamedImports(int pos, bool*
ok);
While you're at it, can you perhaps change the other above functions to _return_
their lists as well? I don't see a good reason why they are created by the
caller.

[arv (Not doing code reviews), 2015-02-25 16:07:25]

LGTM

https://codereview.chromium.org/948303004/diff/1/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/948303004/diff/1/src/ast.h#newcode613
src/ast.h:613: const AstRawString* module_specifier() const { return
module_specifier_; }
On 2015/02/25 13:55:49, rossberg wrote:
> Hm, is it necessary that the specifier is duplicated on every importdecl node?
> That doesn't make sense from an AST perspective, and the fact that you then
need
> a setter doesn't make it better.

At the moment the actual import statement is not represented in the AST so then
I see why this is needed. I think this approach is fine (at least for now).

https://codereview.chromium.org/948303004/diff/1/src/parser.h
File src/parser.h (right):

https://codereview.chromium.org/948303004/diff/1/src/parser.h#newcode716
src/parser.h:716: ZoneList<ImportDeclaration*>* ParseNamedImports(int pos, bool*
ok);
On 2015/02/25 13:55:49, rossberg wrote:
> While you're at it, can you perhaps change the other above functions to
_return_
> their lists as well? I don't see a good reason why they are created by the
> caller.

The one above has 2 lists as out params.

https://codereview.chromium.org/948303004/diff/1/test/cctest/test-parsing.cc
File test/cctest/test-parsing.cc (right):

https://codereview.chromium.org/948303004/diff/1/test/cctest/test-parsing.cc#...
test/cctest/test-parsing.cc:5246: i::ImportDeclaration* decl =
declarations->at(i)->AsImportDeclaration();
Maybe remove the loop and check x and z separately?

[adamk, 2015-02-25 21:43:12]

https://codereview.chromium.org/948303004/diff/1/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/948303004/diff/1/src/ast.h#newcode613
src/ast.h:613: const AstRawString* module_specifier() const { return
module_specifier_; }
On 2015/02/25 16:07:25, arv wrote:
> On 2015/02/25 13:55:49, rossberg wrote:
> > Hm, is it necessary that the specifier is duplicated on every importdecl
node?
> > That doesn't make sense from an AST perspective, and the fact that you then
> need
> > a setter doesn't make it better.
> 
> At the moment the actual import statement is not represented in the AST so
then
> I see why this is needed. I think this approach is fine (at least for now).

I'm open to other suggestions, but I don't think it will be clear until I get
further along what the right organization for this data will be, which is why I
went with this for now, basically a desugaring of

import {x, y} from "m.js"; -> import {x} from "m.js"; import {y} from "m.js";

just like we do

let x, y; -> let x; let y;

https://codereview.chromium.org/948303004/diff/1/src/parser.h
File src/parser.h (right):

https://codereview.chromium.org/948303004/diff/1/src/parser.h#newcode716
src/parser.h:716: ZoneList<ImportDeclaration*>* ParseNamedImports(int pos, bool*
ok);
On 2015/02/25 16:07:25, arv wrote:
> On 2015/02/25 13:55:49, rossberg wrote:
> > While you're at it, can you perhaps change the other above functions to
> _return_
> > their lists as well? I don't see a good reason why they are created by the
> > caller.
> 
> The one above has 2 lists as out params.

Would be fine changing ParseStatementList and ParseModule (to-be-named
ParseModuleItemList) to return their arguments, though I'd rather do it in a
different patch.

https://codereview.chromium.org/948303004/diff/1/test/cctest/test-parsing.cc
File test/cctest/test-parsing.cc (right):

https://codereview.chromium.org/948303004/diff/1/test/cctest/test-parsing.cc#...
test/cctest/test-parsing.cc:5246: i::ImportDeclaration* decl =
declarations->at(i)->AsImportDeclaration();
On 2015/02/25 16:07:25, arv wrote:
> Maybe remove the loop and check x and z separately?

Done.

[adamk, 2015-02-25 21:49:07]

Patchset #2 (id:20001) has been deleted

[adamk, 2015-02-25 21:50:47]

https://codereview.chromium.org/948303004/diff/1/src/parser.h
File src/parser.h (right):

https://codereview.chromium.org/948303004/diff/1/src/parser.h#newcode716
src/parser.h:716: ZoneList<ImportDeclaration*>* ParseNamedImports(int pos, bool*
ok);
On 2015/02/25 21:43:12, adamk wrote:
> On 2015/02/25 16:07:25, arv wrote:
> > On 2015/02/25 13:55:49, rossberg wrote:
> > > While you're at it, can you perhaps change the other above functions to
> > _return_
> > > their lists as well? I don't see a good reason why they are created by the
> > > caller.
> > 
> > The one above has 2 lists as out params.
> 
> Would be fine changing ParseStatementList and ParseModule (to-be-named
> ParseModuleItemList) to return their arguments, though I'd rather do it in a
> different patch.

Actually, there is a reason for passing in body there: ParseStatementList has
multiple callers, and one of them already has a ZoneList<Statement*> with some
statements in it before passing it in. Not sure this is worth changing for now.

[rossberg, 2015-02-26 07:52:07]

LGTM

https://codereview.chromium.org/948303004/diff/1/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/948303004/diff/1/src/ast.h#newcode613
src/ast.h:613: const AstRawString* module_specifier() const { return
module_specifier_; }
On 2015/02/25 21:43:12, adamk wrote:
> On 2015/02/25 16:07:25, arv wrote:
> > On 2015/02/25 13:55:49, rossberg wrote:
> > > Hm, is it necessary that the specifier is duplicated on every importdecl
> node?
> > > That doesn't make sense from an AST perspective, and the fact that you
then
> > need
> > > a setter doesn't make it better.
> > 
> > At the moment the actual import statement is not represented in the AST so
> then
> > I see why this is needed. I think this approach is fine (at least for now).
> 
> I'm open to other suggestions, but I don't think it will be clear until I get
> further along what the right organization for this data will be, which is why
I
> went with this for now, basically a desugaring of
> 
> import {x, y} from "m.js"; -> import {x} from "m.js"; import {y} from "m.js";

But is this an adequate desugaring? Wouldn't those two forms have somewhat
different interactions with the loader, which might be observable if you
customise it?

Maybe it's fine, if this program equivalence actually holds. But keep in mind
that the AST is a rather specific abstraction, not just organising some data.

[adamk, 2015-02-26 16:55:36]

On 2015/02/26 07:52:07, rossberg wrote:
> LGTM
> 
> https://codereview.chromium.org/948303004/diff/1/src/ast.h
> File src/ast.h (right):
> 
> https://codereview.chromium.org/948303004/diff/1/src/ast.h#newcode613
> src/ast.h:613: const AstRawString* module_specifier() const { return
> module_specifier_; }
> On 2015/02/25 21:43:12, adamk wrote:
> > On 2015/02/25 16:07:25, arv wrote:
> > > On 2015/02/25 13:55:49, rossberg wrote:
> > > > Hm, is it necessary that the specifier is duplicated on every importdecl
> > node?
> > > > That doesn't make sense from an AST perspective, and the fact that you
> then
> > > need
> > > > a setter doesn't make it better.
> > > 
> > > At the moment the actual import statement is not represented in the AST so
> > then
> > > I see why this is needed. I think this approach is fine (at least for
now).
> > 
> > I'm open to other suggestions, but I don't think it will be clear until I
get
> > further along what the right organization for this data will be, which is
why
> I
> > went with this for now, basically a desugaring of
> > 
> > import {x, y} from "m.js"; -> import {x} from "m.js"; import {y} from
"m.js";
> 
> But is this an adequate desugaring? Wouldn't those two forms have somewhat
> different interactions with the loader, which might be observable if you
> customise it?

I believe it is a proper desugaring, see
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-hostresolveimported...
(tl;dr: "This operation must be idempotent if it completes normally. Each time
it is called with a specific referencingModule, specifier pair as arguments it
must return the same Module Record instance.").

I admit that it might be a challenge to ensure this invariant in a customized
loader, but that's a challenge for the Loader spec.

> Maybe it's fine, if this program equivalence actually holds. But keep in mind
> that the AST is a rather specific abstraction, not just organising some data.

Can you expand on what you mean here? It's not as if V8's AST is one-to-one
mappable to the grammar productions in the spec.

[adamk, 2015-02-26 16:55:43]

The CQ bit was checked by adamk@chromium.org

[adamk, 2015-02-26 16:55:43]

The patchset sent to the CQ was uploaded after l-g-t-m from arv@chromium.org
Link to the patchset: https://codereview.chromium.org/948303004/#ps60001 (title:
"Merged to trunk so I can run tryjobs")

[commit-bot: I haz the power, 2015-02-26 16:56:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/948303004/60001

[commit-bot: I haz the power, 2015-02-26 18:40:55]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2015-02-26 18:41:11]

Patchset 3 (id:??) landed as
https://crrev.com/fa293dd79f78074a0610739cb8f669849bf5704a
Cr-Commit-Position: refs/heads/master@{#26895}