Bug 148172 – [ES6] Implement Module execution and Loader's ready / link phase

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Yusuke Suzuki]

Implement Module execution and Loader's ready / link phase

[Yusuke Suzuki]

Now, I'm now implementing the rapid prototype.
And later, I'll refactor/redesign/optimize it.

[Yusuke Suzuki]

Created attachment 259754 [details]
Patch

WIP

[Yusuke Suzuki]

WIP, still a lot of weird things.

1. C++ recursion exists
2. Looking up the hash tables repeatedly. It may be combined.
3. Not implemented the execution part (environment, instance, namespace object are partially done)

I'll complete the implementation first, and later, I'll refactor them and submit the r? patch here.

[Yusuke Suzuki]

Created attachment 259794 [details]
Patch

WIP

[Yusuke Suzuki]

Created attachment 259861 [details]
Patch

WIP: part2

[Yusuke Suzuki]

Created attachment 259868 [details]
Patch

WIP: part3

[Yusuke Suzuki]

Created attachment 259925 [details]
Patch

WIP: part4, namespace object is correctly handled

[Yusuke Suzuki]

Created attachment 259984 [details]
Patch

WIP: part5, import / export bindings are correctly supported. making dynamic handling correct under the module enviornment

[Yusuke Suzuki]

Created attachment 260009 [details]
Patch

WIP: part6, functionality is done. Let's refactor and break this into several patches

[Yusuke Suzuki]

Here, I'll just submit the patch to add the template for the link/ready phase.
I'll upload the large (the big picture of the module implementation) patch to the original meta bug.

[Yusuke Suzuki]

Created attachment 260028 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 260028 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=260028&action=review

Add comments for the ease of the review.

> Source/JavaScriptCore/builtins/ModuleLoaderObject.js:352
> +            let pair = entry.dependencies[i];

This was the bug. Change var to let to construct the lexical variable for this `pair`.

> Source/JavaScriptCore/builtins/ModuleLoaderObject.js:-372
> -                    // https://github.com/whatwg/loader/pull/66

This issue is fixed in the draft. (PR is merged.)

> Source/JavaScriptCore/runtime/JSModuleRecord.cpp:94
> +{

The implementation will be implemented in the subsequent patch.

> Source/JavaScriptCore/runtime/JSModuleRecord.cpp:99
> +    return jsUndefined();

Ditto.

[Saam Barati]

Comment on attachment 260028 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=260028&action=review

r=me

>> Source/JavaScriptCore/builtins/ModuleLoaderObject.js:352
>> +            let pair = entry.dependencies[i];
> 
> This was the bug. Change var to let to construct the lexical variable for this `pair`.

There is a fixme in the parser that may need to be fixed for this to work.
We do some validation when parsing builtins and we probably don't validate
properly for "let" variables. Can you take a look at this maybe and make
sure it's okay?

> Source/JavaScriptCore/builtins/ModuleLoaderObject.js:451
> +    entry.state = this.Ready;

Even though we know we will succeed, why not just setStateToMax to be consistent?

[Yusuke Suzuki]

Comment on attachment 260028 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=260028&action=review

Thanks, helpful comments.

>>> Source/JavaScriptCore/builtins/ModuleLoaderObject.js:352
>>> +            let pair = entry.dependencies[i];
>> 
>> This was the bug. Change var to let to construct the lexical variable for this `pair`.
> 
> There is a fixme in the parser that may need to be fixed for this to work.
> We do some validation when parsing builtins and we probably don't validate
> properly for "let" variables. Can you take a look at this maybe and make
> sure it's okay?

I've ensured this code itself is ok.
In addition to that, I've tested the top-level let variable that is captured by the closure function, and it does not raise the assertion.
This is because `capturedVariables` only collects the captured declared variables. lexical variables are ignored. `if (!m_declaredVariables.contains(*ptr))` purges the lexical variables.
Is it intentional?

>> Source/JavaScriptCore/builtins/ModuleLoaderObject.js:451
>> +    entry.state = this.Ready;
> 
> Even though we know we will succeed, why not just setStateToMax to be consistent?

This is because we know `Ready` is the highest state value. So when performing `setStateToMax` with the `Ready` state, the result is always `Ready`.
But, for the consistency and the future extension that may have the higher value than `Ready`, I'll change here to `setStateToMax`.

[Yusuke Suzuki]

Committed r189088: <http://trac.webkit.org/changeset/189088>