[modules] Introduce new VariableLocation for module imports/exports.

[modules] Introduce new VariableLocation for module imports/exports.

Introduces a new VariableLocation MODULE for variables that live in a
module's export table.  Scope analysis sets this for the approriate variables.
Not yet supported by any backend.

Also, treats all imports as CONST bindings (including namespace imports), rather
than having new special variable modes.

BUG=

Committed: https://crrev.com/4df91581d1dd74511ea29482117f54e0083807bd
Cr-Commit-Position: refs/heads/master@{#38426}

[neis, 2016-08-02 12:41:35]

The CQ bit was checked by neis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-02 12:41:45]

Dry run: 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-08-02 13:05:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-02 13:05:18]

Dry run: This issue passed the CQ dry run.

[neis, 2016-08-02 14:45:45]

Description was changed from

==========
[modules] Introduce new VariableLocation for module imports/exports.

BUG=
==========

to

==========
[modules] Introduce new VariableLocation for module imports/exports.

Introduces a new VariableLocation value MODULE for variables that live in a
module's export table.  This is not yet supported by any backend.

Also treats all imports as CONST bindings.

BUG=
==========

[neis, 2016-08-02 14:45:46]

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

[neis, 2016-08-02 14:51:30]

Description was changed from

==========
[modules] Introduce new VariableLocation for module imports/exports.

Introduces a new VariableLocation value MODULE for variables that live in a
module's export table.  This is not yet supported by any backend.

Also treats all imports as CONST bindings.

BUG=
==========

to

==========
[modules] Introduce new VariableLocation for module imports/exports.

Introduces a new VariableLocation MODULE for variables that live in a
module's export table.  Scope analysis sets this for the approriate variables.
Not yet supported by any backend.

Also, treats all imports as CONST bindings (including namespace imports), rather
than having new special variable modes.

BUG=
==========

[neis, 2016-08-02 14:52:04]

PTAL

[adamk, 2016-08-02 16:34:31]

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

https://codereview.chromium.org/2199283002/diff/20001/src/ast/modules.cc#newc...
src/ast/modules.cc:125: 
Can probably just drop this file from the CL :)

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

https://codereview.chromium.org/2199283002/diff/20001/src/ast/scopes.cc#newco...
src/ast/scopes.cc:864: scope = scope->inner_scope();
Why not just walk up till you hit the module scope (or not)? They should never
be nested...

https://codereview.chromium.org/2199283002/diff/20001/src/ast/scopes.cc#newco...
src/ast/scopes.cc:1602: Scope* module_scope = ModuleScope();
Does this need to walk up to the ModuleScope? I would have thought it would only
make sense to call it on the module's scope itself.

https://codereview.chromium.org/2199283002/diff/20001/src/ast/scopes.cc#newco...
src/ast/scopes.cc:1609: var->AllocateTo(VariableLocation::MODULE, 42);
For imports, I think we want to check Scope::MustAllocate() (and remove the
is_module_scope() logic from that method) before allocating. If an import is
unreferenced, we shouldn't need space for it (but I admit that's kind of a weird
case).

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

https://codereview.chromium.org/2199283002/diff/20001/src/parsing/parser.cc#n...
src/parsing/parser.cc:1588: // DeclareImport(module_namespace_binding, pos,
CHECK_OK_VOID);
Leave a TODO here?

[neis, 2016-08-03 09:16:30]

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

https://codereview.chromium.org/2199283002/diff/20001/src/ast/scopes.cc#newco...
src/ast/scopes.cc:864: scope = scope->inner_scope();
On 2016/08/02 16:34:31, adamk wrote:
> Why not just walk up till you hit the module scope (or not)? They should never
> be nested...

Because "this" may already be the script scope. See what Analyze does before it
calls AllocateVariables:

  // Traverse the scope tree up to the first unresolved scope or the global
  // scope and start scope resolution and variable allocation from that scope.
  while (!top->is_script_scope() &&
         !top->outer_scope()->already_resolved()) {
    top = top->outer_scope();
  }

https://codereview.chromium.org/2199283002/diff/20001/src/ast/scopes.cc#newco...
src/ast/scopes.cc:1602: Scope* module_scope = ModuleScope();
On 2016/08/02 16:34:31, adamk wrote:
> Does this need to walk up to the ModuleScope? I would have thought it would
only
> make sense to call it on the module's scope itself.

I'm calling AllocateModuleVariables inside AllocateVariables, which may be
called on some quasi-random scope (see above).  But I'm open for suggestions.

https://codereview.chromium.org/2199283002/diff/20001/src/ast/scopes.cc#newco...
src/ast/scopes.cc:1609: var->AllocateTo(VariableLocation::MODULE, 42);
On 2016/08/02 16:34:31, adamk wrote:
> For imports, I think we want to check Scope::MustAllocate() (and remove the
> is_module_scope() logic from that method) before allocating. If an import is
> unreferenced, we shouldn't need space for it (but I admit that's kind of a
weird
> case).

Not sure I understand. Setting the VariableLocation to MODULE doesn't by itself
reserve any space for the variable. It just tells the compiler where to look it
up. Space will be have to be reserved by the exporting module, not by the module
we're analyzing.

I will have a look at MustAllocate and the module logic you mentioned, though.

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

https://codereview.chromium.org/2199283002/diff/20001/src/parsing/parser.cc#n...
src/parsing/parser.cc:1588: // DeclareImport(module_namespace_binding, pos,
CHECK_OK_VOID);
On 2016/08/02 16:34:31, adamk wrote:
> Leave a TODO here?

Hmm? This is the same as all the other DeclareImports here, the TODO is above.

[adamk, 2016-08-03 17:51:26]

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

https://codereview.chromium.org/2199283002/diff/20001/src/ast/scopes.cc#newco...
src/ast/scopes.cc:864: scope = scope->inner_scope();
On 2016/08/03 09:16:30, neis wrote:
> On 2016/08/02 16:34:31, adamk wrote:
> > Why not just walk up till you hit the module scope (or not)? They should
never
> > be nested...
> 
> Because "this" may already be the script scope. See what Analyze does before
it
> calls AllocateVariables:
> 
>   // Traverse the scope tree up to the first unresolved scope or the global
>   // scope and start scope resolution and variable allocation from that scope.
>   while (!top->is_script_scope() &&
>          !top->outer_scope()->already_resolved()) {
>     top = top->outer_scope();
>   }
> 

But we'll walk _down_ the scope tree as part of analysis.

Maybe my question is, why isn't AllocateModuleVariables called from
AllocateVariablesRecursively when is_module_scope() is true, like we do with
parameters for function scopes?

https://codereview.chromium.org/2199283002/diff/20001/src/ast/scopes.cc#newco...
src/ast/scopes.cc:1602: Scope* module_scope = ModuleScope();
On 2016/08/03 09:16:30, neis wrote:
> On 2016/08/02 16:34:31, adamk wrote:
> > Does this need to walk up to the ModuleScope? I would have thought it would
> only
> > make sense to call it on the module's scope itself.
> 
> I'm calling AllocateModuleVariables inside AllocateVariables, which may be
> called on some quasi-random scope (see above).  But I'm open for suggestions.

See above, I was imagining that we'd check is_module_scope() in
AllocateVariablesRecursively and call this method in that case.

https://codereview.chromium.org/2199283002/diff/20001/src/ast/scopes.cc#newco...
src/ast/scopes.cc:1609: var->AllocateTo(VariableLocation::MODULE, 42);
On 2016/08/03 09:16:30, neis wrote:
> On 2016/08/02 16:34:31, adamk wrote:
> > For imports, I think we want to check Scope::MustAllocate() (and remove the
> > is_module_scope() logic from that method) before allocating. If an import is
> > unreferenced, we shouldn't need space for it (but I admit that's kind of a
> weird
> > case).
> 
> Not sure I understand. Setting the VariableLocation to MODULE doesn't by
itself
> reserve any space for the variable. It just tells the compiler where to look
it
> up. Space will be have to be reserved by the exporting module, not by the
module
> we're analyzing.
> 
> I will have a look at MustAllocate and the module logic you mentioned, though.

My understanding may be wrong, but I thought that once we set a variable as
"allocated" it's going to get space somewhere; that is, deciding to allocate
means deciding to reserve space "somewhere". As it happens we've yet to write
any code for actually allocating slots in some heap object for MODULE variables,
though, so I guess that can wait? A TODO would be appropriate if we want to
leave this allocated-always.

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

https://codereview.chromium.org/2199283002/diff/20001/src/parsing/parser.cc#n...
src/parsing/parser.cc:1588: // DeclareImport(module_namespace_binding, pos,
CHECK_OK_VOID);
On 2016/08/03 09:16:30, neis wrote:
> On 2016/08/02 16:34:31, adamk wrote:
> > Leave a TODO here?
> 
> Hmm? This is the same as all the other DeclareImports here, the TODO is above.

Oops, didn't look in the right place, I get it.

[neis, 2016-08-05 07:32:28]

On 2016/08/03 17:51:26, adamk wrote:
> https://codereview.chromium.org/2199283002/diff/20001/src/ast/scopes.cc
> File src/ast/scopes.cc (right):
> 
>
https://codereview.chromium.org/2199283002/diff/20001/src/ast/scopes.cc#newco...
> src/ast/scopes.cc:864: scope = scope->inner_scope();
> On 2016/08/03 09:16:30, neis wrote:
> > On 2016/08/02 16:34:31, adamk wrote:
> > > Why not just walk up till you hit the module scope (or not)? They should
> never
> > > be nested...
> > 
> > Because "this" may already be the script scope. See what Analyze does before
> it
> > calls AllocateVariables:
> > 
> >   // Traverse the scope tree up to the first unresolved scope or the global
> >   // scope and start scope resolution and variable allocation from that
scope.
> >   while (!top->is_script_scope() &&
> >          !top->outer_scope()->already_resolved()) {
> >     top = top->outer_scope();
> >   }
> > 
> 
> But we'll walk _down_ the scope tree as part of analysis.
> 
> Maybe my question is, why isn't AllocateModuleVariables called from
> AllocateVariablesRecursively when is_module_scope() is true, like we do with
> parameters for function scopes?

As discussed offline, I did that now.
 
>
https://codereview.chromium.org/2199283002/diff/20001/src/ast/scopes.cc#newco...
> src/ast/scopes.cc:1609: var->AllocateTo(VariableLocation::MODULE, 42);
> On 2016/08/03 09:16:30, neis wrote:
> > On 2016/08/02 16:34:31, adamk wrote:
> > > For imports, I think we want to check Scope::MustAllocate() (and remove
the
> > > is_module_scope() logic from that method) before allocating. If an import
is
> > > unreferenced, we shouldn't need space for it (but I admit that's kind of a
> > weird
> > > case).
> > 
> > Not sure I understand. Setting the VariableLocation to MODULE doesn't by
> itself
> > reserve any space for the variable. It just tells the compiler where to look
> it
> > up. Space will be have to be reserved by the exporting module, not by the
> module
> > we're analyzing.
> > 
> > I will have a look at MustAllocate and the module logic you mentioned,
though.
> 
> My understanding may be wrong, but I thought that once we set a variable as
> "allocated" it's going to get space somewhere; that is, deciding to allocate
> means deciding to reserve space "somewhere". As it happens we've yet to write
> any code for actually allocating slots in some heap object for MODULE
variables,
> though, so I guess that can wait? A TODO would be appropriate if we want to
> leave this allocated-always.

And as discussed offline, there's no need here for a TODO because it's the
exporting module that has to allocate storage.

[neis, 2016-08-05 07:33:11]

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

[neis, 2016-08-05 07:33:40]

+bmeurer, ptal at compilers

[Benedikt Meurer, 2016-08-05 07:48:46]

LGTM (rubber-stamped)

[adamk, 2016-08-05 17:45:30]

lgtm

[neis, 2016-08-08 09:19:04]

The CQ bit was checked by neis@chromium.org

[neis, 2016-08-08 09:19:04]

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

[commit-bot: I haz the power, 2016-08-08 09:19:16]

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-08-08 09:46:57]

Description was changed from

==========
[modules] Introduce new VariableLocation for module imports/exports.

Introduces a new VariableLocation MODULE for variables that live in a
module's export table.  Scope analysis sets this for the approriate variables.
Not yet supported by any backend.

Also, treats all imports as CONST bindings (including namespace imports), rather
than having new special variable modes.

BUG=
==========

to

==========
[modules] Introduce new VariableLocation for module imports/exports.

Introduces a new VariableLocation MODULE for variables that live in a
module's export table.  Scope analysis sets this for the approriate variables.
Not yet supported by any backend.

Also, treats all imports as CONST bindings (including namespace imports), rather
than having new special variable modes.

BUG=
==========

[commit-bot: I haz the power, 2016-08-08 09:46:58]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-08-08 09:49:37]

Description was changed from

==========
[modules] Introduce new VariableLocation for module imports/exports.

Introduces a new VariableLocation MODULE for variables that live in a
module's export table.  Scope analysis sets this for the approriate variables.
Not yet supported by any backend.

Also, treats all imports as CONST bindings (including namespace imports), rather
than having new special variable modes.

BUG=
==========

to

==========
[modules] Introduce new VariableLocation for module imports/exports.

Introduces a new VariableLocation MODULE for variables that live in a
module's export table.  Scope analysis sets this for the approriate variables.
Not yet supported by any backend.

Also, treats all imports as CONST bindings (including namespace imports), rather
than having new special variable modes.

BUG=

Committed: https://crrev.com/4df91581d1dd74511ea29482117f54e0083807bd
Cr-Commit-Position: refs/heads/master@{#38426}
==========

[commit-bot: I haz the power, 2016-08-08 09:49:38]

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