[modules] Implement namespace imports.

[modules] Implement namespace imports.

This implements namespace imports (import * as foo from "bar"), except for the
@@iterator property on namespace objects (to be done later).

R=adamk@chromium.org
BUG=v8:1569
Committed: https://crrev.com/57ba0ae10eef8a21845c15f4a220e97043c8a933
Cr-Commit-Position: refs/heads/master@{#40096}

[adamk, 2016-10-04 18:26:51]

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

[adamk, 2016-10-04 18:26:53]

I'd like someone more familiar with accessors.cc (Toon?) to review that part.

https://codereview.chromium.org/2388153003/diff/20001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/2388153003/diff/20001/include/v8.h#newcode3543
include/v8.h:3543: * `Reflect.get`, `Function.prototype.call`, or similar
functions, it is the
Maybe leave these changes out of this CL :)

https://codereview.chromium.org/2388153003/diff/20001/src/objects-debug.cc
File src/objects-debug.cc (right):

https://codereview.chromium.org/2388153003/diff/20001/src/objects-debug.cc#ne...
src/objects-debug.cc:940: CHECK(module_namespace()->IsUndefined(isolate) ||
Can you add code to Factory to set this to undefined explicitly, just for
clarity?

https://codereview.chromium.org/2388153003/diff/20001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/2388153003/diff/20001/src/objects.cc#newcode1...
src/objects.cc:19980: namespace {
Please add a blank line after this (and before the closing })

https://codereview.chromium.org/2388153003/diff/20001/src/objects.cc#newcode1...
src/objects.cc:19998: i::Handle<i::ModuleInfoEntry> entry(
No need for i:: prefixes here (and below).

https://codereview.chromium.org/2388153003/diff/20001/src/objects.cc#newcode2...
src/objects.cc:20029: more_exports.insert(std::make_pair(name, cell));
Please change this code to only do a single lookup, using insert() and using
result.first->second, etc.

https://codereview.chromium.org/2388153003/diff/20001/src/objects.cc#newcode2...
src/objects.cc:20053: void Module::FetchStarExports(Handle<Module> module, Zone*
zone) {
Is this called anywhere other than below? Maybe just inline it there? That'd
avoid the confusion of the same-named functions.

https://codereview.chromium.org/2388153003/diff/20001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2388153003/diff/20001/src/objects.h#newcode7909
src/objects.h:7909: class JSModuleNamespace : public JSObject {
Please add a comment for those not as well-versed in the module spec.

https://codereview.chromium.org/2388153003/diff/20001/src/objects.h#newcode7914
src/objects.h:7914: DECL_ACCESSORS(module, Module)
Comment please.

https://codereview.chromium.org/2388153003/diff/20001/src/objects.h#newcode7916
src/objects.h:7916: Object* Get(Handle<String> name);
Please add a comment. Get what? Possible return values? Etc.

Unusual to have a method that takes a handle but returns a raw pointer. Probably
this should not take a handle, but just a String*.

https://codereview.chromium.org/2388153003/diff/20001/src/objects.h#newcode7985
src/objects.h:7985: static Handle<JSModuleNamespace>
GetModuleNamespace(Handle<Module> module);
Should this function be private? It looks like the public api is the function
below.

https://codereview.chromium.org/2388153003/diff/20001/src/objects.h#newcode7986
src/objects.h:7986: static Handle<JSModuleNamespace>
GetModuleNamespace(Handle<Module> module,
Please add a comment (specifically, the second argument is somewhat mysterious
looking).

https://codereview.chromium.org/2388153003/diff/20001/src/runtime/runtime-obj...
File src/runtime/runtime-object.cc (right):

https://codereview.chromium.org/2388153003/diff/20001/src/runtime/runtime-obj...
src/runtime/runtime-object.cc:963: RUNTIME_FUNCTION(Runtime_GetModuleNamespace)
{
It occurs to me that maybe these runtime functions should be in a new
runtime-modules.cc file (no need to make that change in this CL).

https://codereview.chromium.org/2388153003/diff/20001/src/runtime/runtime-obj...
src/runtime/runtime-object.cc:966: CONVERT_ARG_HANDLE_CHECKED(Smi,
module_request, 0);
You can use CONVERT_SMI_ARG_CHECKED here to get an int directly.

https://codereview.chromium.org/2388153003/diff/20001/src/runtime/runtime-obj...
src/runtime/runtime-object.cc:967: Handle<Module>
module(isolate->context()->module());
Seems a little scary to depend on the current context; do you see us doing this
elsewhere in runtime code?

Still really confusing to me that module() walks the context chain...I guess
I'll send you a CL to change its name everywhere.

Is there any DCHECK we can put here to double-check that this runtime function
was called in the right context?

https://codereview.chromium.org/2388153003/diff/20001/test/mjsunit/modules-na...
File test/mjsunit/modules-namespace1.js (right):

https://codereview.chromium.org/2388153003/diff/20001/test/mjsunit/modules-na...
test/mjsunit/modules-namespace1.js:21: //
assertFalse(Reflect.setPrototypeOf(foo, null));
Can you change this assertion to assertTrue and add a TODO? I know you don't
like to cement "wrong" behavior, but this case is simple enough that it seems
reasonable to have  a record of our current behavior.

https://codereview.chromium.org/2388153003/diff/20001/test/mjsunit/modules-na...
test/mjsunit/modules-namespace1.js:26: // , Symbol.iterator],
Please add a TODO here.

https://codereview.chromium.org/2388153003/diff/20001/test/mjsunit/modules-na...
test/mjsunit/modules-namespace1.js:38: //
assertFalse(Reflect.defineProperty(foo, "yo", {}));
Please add a TODO above this block of asserts; I assume it has to do with the
setter return value stuff? Maybe this should have its own bug, blocking the
modules bug, since it's a somewhat separate problem.

[neis, 2016-10-05 08:18:42]

https://codereview.chromium.org/2388153003/diff/20001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/2388153003/diff/20001/include/v8.h#newcode3543
include/v8.h:3543: * `Reflect.get`, `Function.prototype.call`, or similar
functions, it is the
On 2016/10/04 18:26:52, adamk wrote:
> Maybe leave these changes out of this CL :)

Done.

https://codereview.chromium.org/2388153003/diff/20001/src/objects-debug.cc
File src/objects-debug.cc (right):

https://codereview.chromium.org/2388153003/diff/20001/src/objects-debug.cc#ne...
src/objects-debug.cc:940: CHECK(module_namespace()->IsUndefined(isolate) ||
On 2016/10/04 18:26:53, adamk wrote:
> Can you add code to Factory to set this to undefined explicitly, just for
> clarity?

Done.

https://codereview.chromium.org/2388153003/diff/20001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/2388153003/diff/20001/src/objects.cc#newcode1...
src/objects.cc:19980: namespace {
On 2016/10/04 18:26:53, adamk wrote:
> Please add a blank line after this (and before the closing })

Done.

https://codereview.chromium.org/2388153003/diff/20001/src/objects.cc#newcode1...
src/objects.cc:19998: i::Handle<i::ModuleInfoEntry> entry(
On 2016/10/04 18:26:53, adamk wrote:
> No need for i:: prefixes here (and below).

Done.

https://codereview.chromium.org/2388153003/diff/20001/src/objects.cc#newcode2...
src/objects.cc:20029: more_exports.insert(std::make_pair(name, cell));
On 2016/10/04 18:26:53, adamk wrote:
> Please change this code to only do a single lookup, using insert() and using
> result.first->second, etc.

Done.

https://codereview.chromium.org/2388153003/diff/20001/src/objects.cc#newcode2...
src/objects.cc:20053: void Module::FetchStarExports(Handle<Module> module, Zone*
zone) {
On 2016/10/04 18:26:53, adamk wrote:
> Is this called anywhere other than below? Maybe just inline it there? That'd
> avoid the confusion of the same-named functions.

Ok, inlined it.

https://codereview.chromium.org/2388153003/diff/20001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2388153003/diff/20001/src/objects.h#newcode7909
src/objects.h:7909: class JSModuleNamespace : public JSObject {
On 2016/10/04 18:26:53, adamk wrote:
> Please add a comment for those not as well-versed in the module spec.

Done.

https://codereview.chromium.org/2388153003/diff/20001/src/objects.h#newcode7914
src/objects.h:7914: DECL_ACCESSORS(module, Module)
On 2016/10/04 18:26:53, adamk wrote:
> Comment please.

Done.

https://codereview.chromium.org/2388153003/diff/20001/src/objects.h#newcode7916
src/objects.h:7916: Object* Get(Handle<String> name);
On 2016/10/04 18:26:53, adamk wrote:
> Please add a comment. Get what? Possible return values? Etc.

Done.

> Unusual to have a method that takes a handle but returns a raw pointer.
Probably
> this should not take a handle, but just a String*.

Then the first thing I would have to do in the function is create a handle for
the string, because I need to call Lookup.  (Actually all or most Lookup
functions have the same kind of type.)

https://codereview.chromium.org/2388153003/diff/20001/src/objects.h#newcode7985
src/objects.h:7985: static Handle<JSModuleNamespace>
GetModuleNamespace(Handle<Module> module);
On 2016/10/04 18:26:53, adamk wrote:
> Should this function be private? It looks like the public api is the function
> below.

Done.

https://codereview.chromium.org/2388153003/diff/20001/src/objects.h#newcode7986
src/objects.h:7986: static Handle<JSModuleNamespace>
GetModuleNamespace(Handle<Module> module,
On 2016/10/04 18:26:53, adamk wrote:
> Please add a comment (specifically, the second argument is somewhat mysterious
> looking).

Done.

https://codereview.chromium.org/2388153003/diff/20001/src/runtime/runtime-obj...
File src/runtime/runtime-object.cc (right):

https://codereview.chromium.org/2388153003/diff/20001/src/runtime/runtime-obj...
src/runtime/runtime-object.cc:963: RUNTIME_FUNCTION(Runtime_GetModuleNamespace)
{
On 2016/10/04 18:26:53, adamk wrote:
> It occurs to me that maybe these runtime functions should be in a new
> runtime-modules.cc file (no need to make that change in this CL).

Yeah I was planning to do that eventually.

https://codereview.chromium.org/2388153003/diff/20001/src/runtime/runtime-obj...
src/runtime/runtime-object.cc:966: CONVERT_ARG_HANDLE_CHECKED(Smi,
module_request, 0);
On 2016/10/04 18:26:53, adamk wrote:
> You can use CONVERT_SMI_ARG_CHECKED here to get an int directly.

Done.

https://codereview.chromium.org/2388153003/diff/20001/src/runtime/runtime-obj...
src/runtime/runtime-object.cc:967: Handle<Module>
module(isolate->context()->module());
On 2016/10/04 18:26:53, adamk wrote:
> Seems a little scary to depend on the current context; do you see us doing
this
> elsewhere in runtime code?
> 
> Still really confusing to me that module() walks the context chain...I guess
> I'll send you a CL to change its name everywhere.
> 
> Is there any DCHECK we can put here to double-check that this runtime function
> was called in the right context?

I don't understand.  All the module runtime functions do this.

https://codereview.chromium.org/2388153003/diff/20001/test/mjsunit/modules-na...
File test/mjsunit/modules-namespace1.js (right):

https://codereview.chromium.org/2388153003/diff/20001/test/mjsunit/modules-na...
test/mjsunit/modules-namespace1.js:21: //
assertFalse(Reflect.setPrototypeOf(foo, null));
On 2016/10/04 18:26:53, adamk wrote:
> Can you change this assertion to assertTrue and add a TODO? I know you don't
> like to cement "wrong" behavior, but this case is simple enough that it seems
> reasonable to have  a record of our current behavior.

Done.

https://codereview.chromium.org/2388153003/diff/20001/test/mjsunit/modules-na...
test/mjsunit/modules-namespace1.js:26: // , Symbol.iterator],
On 2016/10/04 18:26:53, adamk wrote:
> Please add a TODO here.

Done.

https://codereview.chromium.org/2388153003/diff/20001/test/mjsunit/modules-na...
test/mjsunit/modules-namespace1.js:38: //
assertFalse(Reflect.defineProperty(foo, "yo", {}));
On 2016/10/04 18:26:53, adamk wrote:
> Please add a TODO above this block of asserts; I assume it has to do with the
> setter return value stuff? Maybe this should have its own bug, blocking the
> modules bug, since it's a somewhat separate problem.

Yes, it's the setter issue. Will create a bug report.

[neis, 2016-10-05 08:19:10]

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

[commit-bot: I haz the power, 2016-10-05 08:19:19]

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

[neis, 2016-10-05 08:37:27]

Description was changed from

==========
[modules] Implement namespace imports.

This implements namespace imports (import * as foo from "bar"), except for the
@@iterator property on namespace objects (to be done later).

R=adamk@chromium.org
BUG=v8:1569
==========

to

==========
[modules] Implement namespace imports.

This implements namespace imports (import * as foo from "bar"), except for the
@@iterator property on namespace objects (to be done later).

R=adamk@chromium.org
BUG=v8:1569
==========

[neis, 2016-10-05 08:37:27]

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

[neis, 2016-10-05 08:37:49]

+mstarzinger for heap, compiler, interpreter. Thanks

[Michael Starzinger, 2016-10-05 13:07:30]

LGTM on "compiler", "heap" and "interpreter". Didn't look at the rest.

https://codereview.chromium.org/2388153003/diff/60001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2388153003/diff/60001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:677: VisitModuleNamespaceImports();
nit: Please add "// Visit module imports if needed." to indicate that it might
be a no-op.

[adamk, 2016-10-05 17:21:54]

lgtm

https://codereview.chromium.org/2388153003/diff/20001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2388153003/diff/20001/src/objects.h#newcode7916
src/objects.h:7916: Object* Get(Handle<String> name);
On 2016/10/05 08:18:41, neis wrote:
> On 2016/10/04 18:26:53, adamk wrote:
> > Please add a comment. Get what? Possible return values? Etc.
> 
> Done.
> 
> > Unusual to have a method that takes a handle but returns a raw pointer.
> Probably
> > this should not take a handle, but just a String*.
> 
> Then the first thing I would have to do in the function is create a handle for
> the string, because I need to call Lookup.  (Actually all or most Lookup
> functions have the same kind of type.)

Ah, I didn't realize ObjectHashTable had this API weirdness, thanks for the
pointer.

https://codereview.chromium.org/2388153003/diff/20001/src/runtime/runtime-obj...
File src/runtime/runtime-object.cc (right):

https://codereview.chromium.org/2388153003/diff/20001/src/runtime/runtime-obj...
src/runtime/runtime-object.cc:967: Handle<Module>
module(isolate->context()->module());
On 2016/10/05 08:18:42, neis wrote:
> On 2016/10/04 18:26:53, adamk wrote:
> > Seems a little scary to depend on the current context; do you see us doing
> this
> > elsewhere in runtime code?
> > 
> > Still really confusing to me that module() walks the context chain...I guess
> > I'll send you a CL to change its name everywhere.
> > 
> > Is there any DCHECK we can put here to double-check that this runtime
function
> > was called in the right context?
> 
> I don't understand.  All the module runtime functions do this.

Sorry, you're right. Like I said, I need to rename that function so I understand
it better :)

[neis, 2016-10-06 08:54:20]

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

[commit-bot: I haz the power, 2016-10-06 08:54:32]

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

[neis, 2016-10-06 08:56:44]

Adam, please have another look. I realized that we need to check for holes in
GetExport. Also added more tests.

https://codereview.chromium.org/2388153003/diff/60001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2388153003/diff/60001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:677: VisitModuleNamespaceImports();
On 2016/10/05 13:07:30, Michael Starzinger wrote:
> nit: Please add "// Visit module imports if needed." to indicate that it might
> be a no-op.

Done.

[commit-bot: I haz the power, 2016-10-06 09:22:40]

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

[commit-bot: I haz the power, 2016-10-06 09:22:41]

Dry run: This issue passed the CQ dry run.

[neis, 2016-10-06 13:09:52]

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

[neis, 2016-10-06 13:10:44]

+ulan: Since Toon is OOO, would you mind having a look at accessors? Thanks.

[adamk, 2016-10-06 18:20:06]

Hole checks lgtm, thanks

[adamk, 2016-10-07 19:07:12]

Given the difficulty in getting an accessors reviewer and the still-early nature
of this code (it's not running anywhere in the wild) let's land this now and we
can change any details of the accessors code if there's something we missed
(fwiw it looks reasonable to me).

[adamk, 2016-10-07 19:07:36]

The CQ bit was checked by adamk@chromium.org

[adamk, 2016-10-07 19:07:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
mstarzinger@chromium.org
Link to the patchset: https://codereview.chromium.org/2388153003/#ps100001
(title: "Add comment on VisitModuleNamespaceImports.")

[commit-bot: I haz the power, 2016-10-07 19:07:45]

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-10-07 19:37:10]

Description was changed from

==========
[modules] Implement namespace imports.

This implements namespace imports (import * as foo from "bar"), except for the
@@iterator property on namespace objects (to be done later).

R=adamk@chromium.org
BUG=v8:1569
==========

to

==========
[modules] Implement namespace imports.

This implements namespace imports (import * as foo from "bar"), except for the
@@iterator property on namespace objects (to be done later).

R=adamk@chromium.org
BUG=v8:1569
==========

[commit-bot: I haz the power, 2016-10-07 19:37:11]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-10-07 19:37:38]

Description was changed from

==========
[modules] Implement namespace imports.

This implements namespace imports (import * as foo from "bar"), except for the
@@iterator property on namespace objects (to be done later).

R=adamk@chromium.org
BUG=v8:1569
==========

to

==========
[modules] Implement namespace imports.

This implements namespace imports (import * as foo from "bar"), except for the
@@iterator property on namespace objects (to be done later).

R=adamk@chromium.org
BUG=v8:1569

Committed: https://crrev.com/57ba0ae10eef8a21845c15f4a220e97043c8a933
Cr-Commit-Position: refs/heads/master@{#40096}
==========

[commit-bot: I haz the power, 2016-10-07 19:37:39]

Patchset 6 (id:??) landed as
https://crrev.com/57ba0ae10eef8a21845c15f4a220e97043c8a933
Cr-Commit-Position: refs/heads/master@{#40096}