1508672 - Enable test262 dynamic module import tests and fix failures

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[Jon Coppeard (:jonco)]

As pointed out in bug 1342012, there are now a bunch of test262 tests for this.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: bug1508672-enable-test262-tests

Remove the 'skip' keyword for test262 dynamic import tests and add those that fail to jstests.list.

Comment 2

[Jon Coppeard (:jonco)]

Attachment: bug1508672-new-import-syntax-error

The parser currently accepts |new import()| which is wrong because import() is not a MemberExpression.  The patch fixes this by passing allowCallSyntax into importExpr().

Comment 3

[Jon Coppeard (:jonco)]

Attachment: bug1508672-fix-minor-test-issues

Patch to fix a couple of minor issues in the tests themselves:

 - indirect-resolution-1_FIXTURE.js has a bare import specifier. This should be a relative specifier to find the indirect-resolution-2_FIXTURE.js module.

 - default-property-not-set-own.js is not a module but imports itself as a module and so gets run twice.  When this happens $DONE complains about being called twice.

I also made a pull request for these changes: https://github.com/tc39/test262/pull/1965

Comment 4

[Jon Coppeard (:jonco)]

Attachment: bug1508672-shell-finish-import-later

The spec says that success and failure actions for dynamic import must happen 'at some future time', but the shell module loader executes them immediately.  This allows a dynamic import to observe a module that is still in the evaluating state, which causes an assertion failure.

The patch uses a promise to defer these actions.

Comment 5

[Jon Coppeard (:jonco)]

Attachment: bug1508672-exception-values

Finally, the shell abortDynamicModuleImport() function used by the shell module loader assumes that only ErrorObjects will be thrown, causing tests that throw other values to fail.  The patch removes this restriction.

Comment 6

[Jason Orendorff [:jorendorff]]

Comment on attachment 9027160 [details] [diff] [review]
bug1508672-new-import-syntax-error

Review of attachment 9027160 [details] [diff] [review]:
-----------------------------------------------------------------

Nice!

Comment 7

[Jason Orendorff [:jorendorff]]

Comment on attachment 9027171 [details] [diff] [review]
bug1508672-exception-values

Review of attachment 9027171 [details] [diff] [review]:
-----------------------------------------------------------------

You sure make it look easy. :)

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/761da392dd7a
Enable passing test262 dynamic import tests r=jorendorff
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ee93898e415
Make |new import()| a syntax error r=jorendorff
https://hg.mozilla.org/integration/mozilla-inbound/rev/93445c302752
Fix some minor test262 issues in dynamic import tests r=jorendorff
https://hg.mozilla.org/integration/mozilla-inbound/rev/46ce47bd3368
Finish dynamic module import at a later time in the shell r=jorendorff
https://hg.mozilla.org/integration/mozilla-inbound/rev/9363b52649ac
Dynamically imported modules can throw any value as an exception r=jorendorff

Comment 9

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/761da392dd7a
https://hg.mozilla.org/mozilla-central/rev/0ee93898e415
https://hg.mozilla.org/mozilla-central/rev/93445c302752
https://hg.mozilla.org/mozilla-central/rev/46ce47bd3368
https://hg.mozilla.org/mozilla-central/rev/9363b52649ac