Bug 165724

Summary: [JSC] Prototype dynamic-import
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: JavaScriptCoreAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, chi187, commit-queue, darin, keith_miller, mark.lam, msaboff, rniwa, sam, sbarati, sgospodarets
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 164425, 164860    
Bug Blocks: 166488, 166926    
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews102 for mac-elcapitan
none
Archive of layout-test-results from ews106 for mac-elcapitan-wk2
none
Archive of layout-test-results from ews112 for mac-elcapitan
none
Archive of layout-test-results from ews123 for ios-simulator-wk2
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
sbarati: review+
Patch for landing none

Description Yusuke Suzuki 2016-12-10 03:33:07 PST
https://tc39.github.io/proposal-dynamic-import/ stage 3
Comment 1 Yusuke Suzuki 2016-12-26 04:13:36 PST
Created attachment 297763 [details]
Patch

prototype
Comment 2 Build Bot 2016-12-26 05:14:16 PST
Comment on attachment 297763 [details]
Patch

Attachment 297763 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2789323

New failing tests:
sputnik/Conformance/07_Lexical_Conventions/7.5_Tokens/7.5.3_Future_Reserved_Words/S7.5.3_A1.16.html
Comment 3 Build Bot 2016-12-26 05:14:19 PST
Created attachment 297764 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 4 Build Bot 2016-12-26 05:18:51 PST
Comment on attachment 297763 [details]
Patch

Attachment 297763 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2789336

New failing tests:
sputnik/Conformance/07_Lexical_Conventions/7.5_Tokens/7.5.3_Future_Reserved_Words/S7.5.3_A1.16.html
Comment 5 Build Bot 2016-12-26 05:18:53 PST
Created attachment 297765 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 6 Build Bot 2016-12-26 05:23:15 PST
Comment on attachment 297763 [details]
Patch

Attachment 297763 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2789332

New failing tests:
sputnik/Conformance/07_Lexical_Conventions/7.5_Tokens/7.5.3_Future_Reserved_Words/S7.5.3_A1.16.html
Comment 7 Build Bot 2016-12-26 05:23:18 PST
Created attachment 297766 [details]
Archive of layout-test-results from ews112 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 8 Build Bot 2016-12-26 05:28:25 PST
Comment on attachment 297763 [details]
Patch

Attachment 297763 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2789333

New failing tests:
sputnik/Conformance/07_Lexical_Conventions/7.5_Tokens/7.5.3_Future_Reserved_Words/S7.5.3_A1.16.html
Comment 9 Build Bot 2016-12-26 05:28:28 PST
Created attachment 297767 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6
Comment 10 Yusuke Suzuki 2016-12-26 10:07:26 PST
Created attachment 297768 [details]
Patch

prototype
Comment 11 Yusuke Suzuki 2016-12-26 11:00:56 PST
Created attachment 297771 [details]
Patch

WIP
Comment 12 Yusuke Suzuki 2016-12-27 00:36:11 PST
Created attachment 297780 [details]
Patch
Comment 13 Yusuke Suzuki 2016-12-27 01:09:34 PST
Created attachment 297782 [details]
Patch
Comment 14 Yusuke Suzuki 2016-12-27 07:08:25 PST
Created attachment 297783 [details]
Patch
Comment 15 Saam Barati 2017-01-09 12:23:06 PST
Comment on attachment 297783 [details]
Patch

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

r=me

> Source/JavaScriptCore/parser/Parser.cpp:4390
> +        handleProductionOrFail2(OPENPAREN, "(", "start", "'import' call");

I think a better error message here would be something like: "import call expects exactly one argument"

> Source/JavaScriptCore/parser/Parser.cpp:4393
> +        handleProductionOrFail2(CLOSEPAREN, ")", "end", "'import' call");

ditto

> Source/JavaScriptCore/runtime/JSGlobalObjectFunctions.cpp:940
> +    RETURN_IF_EXCEPTION(catchScope, encodedJSValue());

Nit: you can use "{ }" instead of "encodedJSValue()"

> Source/JavaScriptCore/runtime/JSGlobalObjectFunctions.cpp:948
> +    auto* specifier = exec->argument(0).toString(exec);

Nit: Since this is just called from our own bytecode, I would RELEASE_ASSERT the argument count and use uncheckedArgument.

> JSTests/stress/import-syntax.js:25
> +testSyntaxError(`import("Hello", "Hello2";`, `SyntaxError: Unexpected token ','. Expected ')' to end an 'import' call.`);

Some more syntax error tests to add:
import(a, b)
import(a, b, c)
import(...foo)

I would also suggest adding tests that assert a synax error *was not* thrown. That way we can also test what we allow.
Comment 16 Yusuke Suzuki 2017-01-09 13:15:29 PST
Created attachment 298382 [details]
Patch for landing

Patch for landing
Comment 17 Yusuke Suzuki 2017-01-09 14:03:54 PST
Committed r210522: <http://trac.webkit.org/changeset/210522>
Comment 18 Serg Hospodarets 2017-02-12 07:50:27 PST
The dynamic import() doesn't work in the DevTools console.
Even for the correct absolute URLs it throws an error: "Cannot resolve the specifier".
Is it the intentional behavior, or a bug (just want to confirm before opening a new issue).
Comment 19 Yusuke Suzuki 2017-02-12 08:00:30 PST
(In reply to comment #18)
> The dynamic import() doesn't work in the DevTools console.
> Even for the correct absolute URLs it throws an error: "Cannot resolve the
> specifier".
> Is it the intentional behavior, or a bug (just want to confirm before
> opening a new issue).

It's intentional currently. And once this[1] patch is landed, it will be fixed. :)

[1]: https://bugs.webkit.org/show_bug.cgi?id=167457