1670044 - Allow Arbitrary module namespace identifier names

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task, P3)

Description

[Yulia Startsev [:yulia]]

https://github.com/tc39/ecma262/pull/2154

Outcome is currently unclear, we probably want to wait on this one.

Comment 1

[André Bargull [:anba]]

Received consensus in the September meeting and is implemented in JSC (https://bugs.webkit.org/show_bug.cgi?id=217576) and V8 (https://bugs.chromium.org/p/v8/issues/detail?id=10964).

Comment 2

[André Bargull [:anba]]

Attachment: Bug 1670044 - Part 1: Implement "Arbitrary module namespace identifier names" proposal. r=yulia!

The spec change adds a new ModuleExportName production to allow exporting/importing
module bindings using string literals. The strings can contain any characters except
unpaired surrogates (i.e. standalone lead or trail surrogates).

ModuleExportName can be used in import declarations:

• import {ModuleExportName as IdentifierName} from ModuleSpecifier

In export batch declarations:

• export* as ModuleExportName from ModuleSpecifier

When exporting local bindings:

• export {IdentifierName as ModuleExportName}

And when re-exporting bindings:

• export {IdentifierName as ModuleExportName} from ModuleSpecifier
• export {ModuleExportName as IdentifierName} from ModuleSpecifier
• export {ModuleExportName as ModuleExportName} from ModuleSpecifier
• export {ModuleExportName} from ModuleSpecifier

Support for "*" as a ModuleExportName is added in a later part.

Spec change: https://github.com/tc39/ecma262/pull/2154

Depends on D100996

Comment 3

[André Bargull [:anba]]

Attachment: Bug 1670044 - Part 2: Enable test262 tests. r=yulia!

Enable all tests marked with "arbitrary-module-namespace-names".

Depends on D101000

Comment 4

[André Bargull [:anba]]

Attachment: Bug 1670044 - Part 3: Split namedImportsOrNamespaceImport() into namedImports() and namespaceImport(). r=yulia!

Splits the namedImportsOrNamespaceImport() method into namedImports() and
namespaceImport(). namedImportsOrNamespaceImport() didn't contain any shared
code, so it seems better to split it into two separate methods to avoid an
extra nesting level.

Depends on D101007

Comment 5

[André Bargull [:anba]]

Attachment: Bug 1670044 - Part 4: Add PerHandlerParser::processImport(). r=yulia!

Add PerHandlerParser::processImport() similar to the existing processExport()
method in preparation for part 5.

Depends on D101008

Comment 6

[André Bargull [:anba]]

Attachment: Bug 1670044 - Part 5: Move import declaration parsing to GeneralParser. r=yulia!

Import declaration parsing was implemented in Parser instead of
GeneralParser, which required to add access to several other parsing methods
from GeneralParser to Parser. We can avoid this by simply moving import
declaration parsing into GeneralParser.

Depends on D101009

Comment 7

[André Bargull [:anba]]

Attachment: Bug 1670044 - Part 6: Inline ModuleBuilder::appendExportFromEntry. r=yulia!

Inline ModuleBuilder::appendExportFromEntry() in preparation for the next
part. The computeLineAndColumn() call can be moved to the top, because
spec->pn_pos.begin is equal to localNameNode->pn_pos.begin by construction,
cf. FullParseHandler::newImportSpec().

Drive-by changes:

• Use auto* in more places.
• Drop frontend:: which is redundant with using namespace js::frontend.
• Remove the if (exportName) check in ModuleBuilder::appendExportEntry(),
  because exportName is never a nullptr.

Depends on D101010

Comment 8

[André Bargull [:anba]]

Attachment: Bug 1670044 - Part 7: Add separate ImportNamespaceSpec and ExportNamespaceSpec parse nodes. r=yulia!

The approach to use "*" for namespace imports/exports no longer works with
module export names. As an alternative add separate ImportNamespaceSpec and
ExportNamespaceSpec parse nodes. They're currently still implemented as binary
nodes, but the next part will change this.

Depends on D101011

Comment 9

[André Bargull [:anba]]

Attachment: Bug 1670044 - Part 8: Use null for star-imports and exports. r=yulia!

The "Arbitrary module namespace identifier names" spec PR allows to use "" as
a module export name, so we can no longer use that specific string to denote
star-imports/exports. Probably the easiest way to work around this new
restriction is to replace "" with a nullptr string.

Spec change: https://github.com/tc39/ecma262/pull/2155

Depends on D101012

Comment 10

[André Bargull [:anba]]

Attachment: Bug 1670044 - Part 9: Allow "*" for module export names. r=yulia!

We can now remove the "*" restriction from moduleExportName().

Depends on D101013

Comment 11

[André Bargull [:anba]]

Just a heads-up for new syntax. https://bugzilla.mozilla.org/show_bug.cgi?id=1670044#c2 enumerates the possible locations where ModuleExportName (a string literal) can be used in import/export declarations.

Comment 12

[André Bargull [:anba]]

Review ping for https://phabricator.services.mozilla.com/D101011 (and its two successor patches). Not sure if you've seen the pings on Phabricator.

Comment 13

[Ted Campbell [:tcampbell]]

Sorry about that, I missed them. Phabricator was treating them as OR reviewers so it dropped off my dashboard. Marking as "blocked" on UI and adding the "!" in the commit message keeps it active on dashboards. Will take a look..

Comment 14

[Ted Campbell [:tcampbell]]

André, can you fix clang-format issue in part8 and rebase onto current m-c? There seems to be a minor conflict with Bug 1691014 that just merged to central.

Comment 15

[André Bargull [:anba]]

Thanks for the info! I've updated part 8.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/12e200cbc0ea
Part 1: Implement "Arbitrary module namespace identifier names" proposal. r=yulia
https://hg.mozilla.org/integration/autoland/rev/9b99c8d7bf31
Part 2: Enable test262 tests. r=yulia
https://hg.mozilla.org/integration/autoland/rev/94524aabe74b
Part 3: Split namedImportsOrNamespaceImport() into namedImports() and namespaceImport(). r=yulia
https://hg.mozilla.org/integration/autoland/rev/f838ac0b8742
Part 4: Add PerHandlerParser::processImport(). r=yulia
https://hg.mozilla.org/integration/autoland/rev/b0e6682e4886
Part 5: Move import declaration parsing to GeneralParser. r=yulia
https://hg.mozilla.org/integration/autoland/rev/8a4790e525ea
Part 6: Inline ModuleBuilder::appendExportFromEntry. r=yulia,tcampbell
https://hg.mozilla.org/integration/autoland/rev/9241d01321bd
Part 7: Add separate ImportNamespaceSpec and ExportNamespaceSpec parse nodes. r=yulia,tcampbell
https://hg.mozilla.org/integration/autoland/rev/6190127f8259
Part 8: Use null for star-imports and exports. r=yulia,tcampbell
https://hg.mozilla.org/integration/autoland/rev/4184432c8050
Part 9: Allow "*" for module export names. r=yulia

Comment 17

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/12e200cbc0ea
https://hg.mozilla.org/mozilla-central/rev/9b99c8d7bf31
https://hg.mozilla.org/mozilla-central/rev/94524aabe74b
https://hg.mozilla.org/mozilla-central/rev/f838ac0b8742
https://hg.mozilla.org/mozilla-central/rev/b0e6682e4886
https://hg.mozilla.org/mozilla-central/rev/8a4790e525ea
https://hg.mozilla.org/mozilla-central/rev/9241d01321bd
https://hg.mozilla.org/mozilla-central/rev/6190127f8259
https://hg.mozilla.org/mozilla-central/rev/4184432c8050