Bug 148172 - [ES6] Implement Module execution and Loader's ready / link phase
Summary: [ES6] Implement Module execution and Loader's ready / link phase
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords:
Depends on:
Blocks: 147340
  Show dependency treegraph
 
Reported: 2015-08-19 00:41 PDT by Yusuke Suzuki
Modified: 2015-08-27 23:22 PDT (History)
2 users (show)

See Also:


Attachments
Patch (97.70 KB, patch)
2015-08-24 02:16 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (108.67 KB, patch)
2015-08-24 17:13 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (132.55 KB, patch)
2015-08-25 11:01 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (135.94 KB, patch)
2015-08-25 12:04 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (148.45 KB, patch)
2015-08-25 22:12 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (175.47 KB, patch)
2015-08-26 15:28 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (185.60 KB, patch)
2015-08-26 17:36 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (29.67 KB, patch)
2015-08-26 21:03 PDT, Yusuke Suzuki
sbarati: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2015-08-19 00:41:56 PDT
Implement Module execution and Loader's ready / link phase
Comment 1 Yusuke Suzuki 2015-08-21 19:10:25 PDT
Now, I'm now implementing the rapid prototype.
And later, I'll refactor/redesign/optimize it.
Comment 2 Yusuke Suzuki 2015-08-24 02:16:52 PDT
Created attachment 259754 [details]
Patch

WIP
Comment 3 Yusuke Suzuki 2015-08-24 11:04:26 PDT
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.
Comment 4 Yusuke Suzuki 2015-08-24 17:13:33 PDT
Created attachment 259794 [details]
Patch

WIP
Comment 5 Yusuke Suzuki 2015-08-25 11:01:40 PDT
Created attachment 259861 [details]
Patch

WIP: part2
Comment 6 Yusuke Suzuki 2015-08-25 12:04:40 PDT
Created attachment 259868 [details]
Patch

WIP: part3
Comment 7 Yusuke Suzuki 2015-08-25 22:12:39 PDT
Created attachment 259925 [details]
Patch

WIP: part4, namespace object is correctly handled
Comment 8 Yusuke Suzuki 2015-08-26 15:28:02 PDT
Created attachment 259984 [details]
Patch

WIP: part5, import / export bindings are correctly supported. making dynamic handling correct under the module enviornment
Comment 9 Yusuke Suzuki 2015-08-26 17:36:33 PDT
Created attachment 260009 [details]
Patch

WIP: part6, functionality is done. Let's refactor and break this into several patches
Comment 10 Yusuke Suzuki 2015-08-26 20:50:26 PDT
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.
Comment 11 Yusuke Suzuki 2015-08-26 21:03:08 PDT
Created attachment 260028 [details]
Patch
Comment 12 Yusuke Suzuki 2015-08-26 21:07:15 PDT
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.
Comment 13 Saam Barati 2015-08-27 21:14:44 PDT
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?
Comment 14 Yusuke Suzuki 2015-08-27 23:08:23 PDT
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`.
Comment 15 Yusuke Suzuki 2015-08-27 23:22:23 PDT
Committed r189088: <http://trac.webkit.org/changeset/189088>