Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Normative: Add `export * as ns from "mod”` to Export production and Module Semantic #1174

Open
wants to merge 1 commit into
base: master
from

Conversation

@spectranaut
Copy link

commented Apr 17, 2018

This PR adds export * as ns from "mod" updates from tc39/proposal-export-ns-from to the spec as part of the July 27, 2017 notes. This PR replaces and expands this PR: #1005

To see an updated diff of the new spec changes, you can look here: https://spectranaut.github.io/proposal-export-ns-from

Tests partially complete.

Closes #1005.

@spectranaut spectranaut changed the title Add `export * as ns from "mod”` to Export production and Module Semantic Normative: Add `export * as ns from "mod”` to Export production and Module Semantic Apr 17, 2018

spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
@spectranaut

This comment has been minimized.

Copy link
Author

commented Apr 18, 2018

I updated table 38 based on the modifications made for export * as namespace but would like if someone review/read for clarity and word choice - I feel awkward editing editorial text: https://spectranaut.github.io/proposal-export-ns-from/#table-38

@ljharb ljharb requested review from jdalton, bterlson and bmeck Apr 18, 2018

@leobalter

This comment has been minimized.

Copy link
Member

commented Apr 18, 2018

Looks good to me, if you want to bikeshed over the new additions:

- A List of ExportEntry records derived from the code of this module that correspond to reexported imports that occur within the module or direct exports from an export * as namespace.
+ A List of ExportEntry records derived from the code of this module that correspond to reexported imports that occur within the module or exports from export * as namespace declarations.
- A List of ExportEntry records derived from the code of this module that correspond to export * declarations (but not export * as namespace declarations) that occur within the module.
+ A List of ExportEntry records derived from the code of this module that correspond to export * declarations that occur within the module except from export * as namespace declarations.
@spectranaut

This comment has been minimized.

Copy link
Author

commented Apr 20, 2018

@ljharb -- you might want to "re-review", I changed how export * as namespace declarations ResolveBinding records are made. I removed the [[isNamespace]] I added and instead the [[BindingName]] is "*namespace*" in these scenarios (as there is actually no binding for the namespace). I tried to update more editorial text to make it clear.

you can see the updated pretty diff here: https://spectranaut.github.io/proposal-export-ns-from/#sec-module-namespace-exotic-objects

@ljharb
ljharb approved these changes Apr 27, 2018
@ljharb

This comment has been minimized.

Copy link
Member

commented Jun 28, 2018

@spectranaut would you mind rebasing this? it'd also be great if we could add this to the agenda for july, now that its tests are merged.

@hzoo hzoo referenced this pull request Jul 25, 2018
@leobalter

This comment has been minimized.

Copy link
Member

commented Sep 27, 2018

Expressing that we have consensus to move forward on this feature but we are waiting on vms to implement this feature before it's merged. Similar to the stage 3 process. Test262 tests are already available, and I'll be happy to point them.

@spectranaut

This comment has been minimized.

Copy link
Author

commented Oct 5, 2018

Rebased, @ljharb :)

@ljharb ljharb requested a review from tc39/ecma262-editors Oct 24, 2018

chakrabot pushed a commit to microsoft/ChakraCore that referenced this pull request Oct 31, 2018
[MERGE #5779 @rhuanjl] Implement export * as ns from module syntax
Merge pull request #5779 from rhuanjl:exportNamespace

This PR does the following:
1. implements support for `export * as ns from "module"` syntax a normative change PR to ecma262 which has tc39 consensus
See spec diff: https://spectranaut.github.io/proposal-export-ns-from/
See normative PR here: tc39/ecma262#1174 (comment)
This is placed behind a flag but set to default enabled
2. Adds some relevant tests
3. Fixes a bug where duplicate exports weren't always a syntax error (implementing this feature correctly without fixing this bug would have been awkward)
4. Some drive by syntax error message improvement for duplicate exports and alias'd exports
    - "Syntax error: Syntax error" -> "Syntax error: Duplicate export of name '%s'"
    - "Syntax error: Syntax error" -> "Syntax error: 'as' is only valid if followed by an identifier."

There are unfortunately some remaining related test262 failures due to #5778 and #5501

closes #5759
fixes #5777
@mathiasbynens

This comment has been minimized.

Copy link
Member

commented Oct 31, 2018

This is shipping in V8 v7.2 / Chrome 72 as well. v8/v8@4c0b56a

@aMoniker

This comment has been minimized.

Copy link

commented Oct 31, 2018

Which VMs are left before this can be merged?

@ljharb

This comment has been minimized.

Copy link
Member

commented Oct 31, 2018

@aMoniker it would need consensus, which can only happen at TC39 meetings. The next is at the end of November.

KFlash added a commit to cherow/cherow that referenced this pull request Jan 30, 2019

@ljharb ljharb requested review from zenparsing and removed request for bterlson and bmeck Feb 28, 2019

@ljharb ljharb self-assigned this Mar 26, 2019

@ljharb ljharb force-pushed the bocoup:export-star-as-ns branch from a6cbfac to 7caea60 Mar 26, 2019

ljharb added a commit to bocoup/ecma262 that referenced this pull request Mar 26, 2019
spec.html Outdated Show resolved Hide resolved

@ljharb ljharb requested a review from tc39/ecma262-editors Mar 27, 2019

spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Show resolved Hide resolved

@ljharb ljharb removed their assignment May 2, 2019

@bmeck bmeck referenced this pull request May 21, 2019

@ljharb ljharb requested review from anba and waldemarhorwat Jun 22, 2019

@TimothyGu
Copy link
Member

left a comment

Some editorials.

spec.html Outdated
1. Let _importedModule_ be ? HostResolveImportedModule(_module_, _e_.[[ModuleRequest]]).
1. Return _importedModule_.ResolveExport(_e_.[[ImportName]], _resolveSet_).
1. If _e_.[[ImportName]] is `"*"`, then
1. NOTE: _module_ does not provide the direct binding for this export.

This comment has been minimized.

Copy link
@TimothyGu

TimothyGu Jun 23, 2019

Member

Maybe this should be an assert as well for symmetry?

This comment has been minimized.

Copy link
@spectranaut

spectranaut Jun 24, 2019

Author

Sounds good.

spec.html Outdated Show resolved Hide resolved
spec.html Outdated
<emu-alg>
1. Return the ExportedNames of |ExportClause|.
1. Return the ExportedNames of NamedExports.

This comment has been minimized.

Copy link
@TimothyGu

TimothyGu Jun 23, 2019

Member

Same here: production chaining should take care of this.

This comment has been minimized.

Copy link
@spectranaut

spectranaut Jun 24, 2019

Author

Same question. Is it reasonable to leave it in for clarity? Otherwise I agree it's unnecessary.

spec.html Outdated
<emu-grammar>ExportDeclaration : `export` `*` FromClause `;`</emu-grammar>
<emu-grammar>ExportDeclaration : `export` ExportFromClause FromClause `;`</emu-grammar>
<emu-alg>
1. Return the ExportedNames of ExportFromClause.

This comment has been minimized.

Copy link
@TimothyGu

TimothyGu Jun 23, 2019

Member
Suggested change
1. Return the ExportedNames of ExportFromClause.
1. Return the ExportedNames of |ExportFromClause|.
spec.html Outdated
@@ -21955,7 +21957,7 @@ <h1>Source Text Module Records</h1>
List of ExportEntry Records
</td>
<td>
A List of ExportEntry records derived from the code of this module that correspond to reexported imports that occur within the module.
A List of ExportEntry records derived from the code of this module that correspond to reexported imports that occur within the module or exports from export * as namespace declarations.

This comment has been minimized.

Copy link
@TimothyGu

TimothyGu Jun 23, 2019

Member
Suggested change
A List of ExportEntry records derived from the code of this module that correspond to reexported imports that occur within the module or exports from export * as namespace declarations.
A List of ExportEntry records derived from the code of this module that correspond to reexported imports that occur within the module or exports from `export * as namespace` declarations.
spec.html Outdated
@@ -21966,7 +21968,7 @@ <h1>Source Text Module Records</h1>
List of ExportEntry Records
</td>
<td>
A List of ExportEntry records derived from the code of this module that correspond to export * declarations that occur within the module.
A List of ExportEntry records derived from the code of this module that correspond to export * declarations that occur within the module, not including export * as namespace declarations.

This comment has been minimized.

Copy link
@TimothyGu

TimothyGu Jun 23, 2019

Member
Suggested change
A List of ExportEntry records derived from the code of this module that correspond to export * declarations that occur within the module, not including export * as namespace declarations.
A List of ExportEntry records derived from the code of this module that correspond to `export *` declarations that occur within the module, not including `export * as namespace` declarations.
spec.html Outdated
NamedExports
NameSpaceExport

NameSpaceExport:

This comment has been minimized.

Copy link
@jmdyck

jmdyck Jun 27, 2019

Collaborator
Suggested change
NameSpaceExport:
NameSpaceExport :

(Insert space before colon.)

Also, the text uses "namespace" as a single word, so the "S" should not be capitalized. (Later: Ah, I guess it's for symmetry with 'NameSpaceImport'. Sigh.)

But instead of those two, it seems there's no need for the extra nonterminal + production. Just define:

ExportFromClause :
    `*`
    `*` `as` IdentifierName
    NamedExports

(In fact, that's the production that ExportEntriesForModule assumes.)

spec.html Outdated
1. Let _entry_ be the ExportEntry Record {[[ModuleRequest]]: _module_, [[ImportName]]: `"*"`, [[LocalName]]: *null*, [[ExportName]]: *null* }.
1. Return a new List containing _entry_.
</emu-alg>
<emu-grammar>ExportFromClause : `*` as IdentifierName</emu-grammar>

This comment has been minimized.

Copy link
@jmdyck

jmdyck Jun 27, 2019

Collaborator
Suggested change
<emu-grammar>ExportFromClause : `*` as IdentifierName</emu-grammar>
<emu-grammar>ExportFromClause : `*` `as` IdentifierName</emu-grammar>

(Need to backtick the "as".)

spec.html Outdated
`export` ExportClause FromClause `;`
`export` ExportClause `;`
`export` ExportFromClause FromClause `;`
`export` NamedExports `;`
`export` VariableStatement[~Yield, ~Await]
`export` Declaration[~Yield, ~Await]
`export` `default` HoistableDeclaration[~Yield, ~Await, +Default]
`export` `default` ClassDeclaration[~Yield, ~Await, +Default]
`export` `default` [lookahead &lt;! {`function`, `async` [no |LineTerminator| here] `function`, `class`}] AssignmentExpression[+In, ~Yield, ~Await] `;`

This comment has been minimized.

Copy link
@jmdyck

jmdyck Jun 27, 2019

Collaborator

The new productions (for ExportFromClause and NameSpaceExport) need to be referenced from Annex A.

<emu-grammar>ExportClause : `{` `}`</emu-grammar>
<emu-grammar>ExportFromClause : `*`</emu-grammar>
<emu-alg>
1. Let _entry_ be the ExportEntry Record {[[ModuleRequest]]: _module_, [[ImportName]]: `"*"`, [[LocalName]]: *null*, [[ExportName]]: *null* }.

This comment has been minimized.

Copy link
@jmdyck

jmdyck Jun 27, 2019

Collaborator
Suggested change
1. Let _entry_ be the ExportEntry Record {[[ModuleRequest]]: _module_, [[ImportName]]: `"*"`, [[LocalName]]: *null*, [[ExportName]]: *null* }.
1. Let _entry_ be the ExportEntry Record { [[ModuleRequest]]: _module_, [[ImportName]]: `"*"`, [[LocalName]]: *null*, [[ExportName]]: *null* }.

(insert space after left brace)

spec.html Outdated
</emu-alg>
<emu-grammar>ExportFromClause : `*` as IdentifierName</emu-grammar>
<emu-alg>
1. Let exportName be the StringValue of IdentifierName.

This comment has been minimized.

Copy link
@jmdyck

jmdyck Jun 27, 2019

Collaborator
Suggested change
1. Let exportName be the StringValue of IdentifierName.
1. Let _exportName_ be the StringValue of |IdentifierName|.

(Insert underscores around "exportName" and bars around "IdentifierName".)

spec.html Outdated
<emu-grammar>ExportFromClause : `*` as IdentifierName</emu-grammar>
<emu-alg>
1. Let exportName be the StringValue of IdentifierName.
1. Let _entry_ be the ExportEntry Record {[[ModuleRequest]]: _module_, [[ImportName]]: `"*"`, [[LocalName]]: *null*, [[ExportName]]: _exportName_ }.

This comment has been minimized.

Copy link
@jmdyck

jmdyck Jun 27, 2019

Collaborator
Suggested change
1. Let _entry_ be the ExportEntry Record {[[ModuleRequest]]: _module_, [[ImportName]]: `"*"`, [[LocalName]]: *null*, [[ExportName]]: _exportName_ }.
1. Let _entry_ be the ExportEntry Record { [[ModuleRequest]]: _module_, [[ImportName]]: `"*"`, [[LocalName]]: *null*, [[ExportName]]: _exportName_ }.

(Insert space after left brace.)

spec.html Outdated
1. Return _importedModule_.ResolveExport(_e_.[[ImportName]], _resolveSet_).
1. If _e_.[[ImportName]] is `"*"`, then
1. Assert: _module_ does not provide the direct binding for this export.
1. Return ResolvedBinding Record {[[Module]]: _importedModule_, [[BindingName]]: `"*namespace*"`}.

This comment has been minimized.

Copy link
@jmdyck

jmdyck Jun 27, 2019

Collaborator
Suggested change
1. Return ResolvedBinding Record {[[Module]]: _importedModule_, [[BindingName]]: `"*namespace*"`}.
1. Return ResolvedBinding Record { [[Module]]: _importedModule_, [[BindingName]]: `"*namespace*"` }.

(Insert space after left brace and before right brace.)

spec.html Outdated
`export` `*` FromClause `;`
`export` ExportClause FromClause `;`
`export` ExportClause `;`
`export` ExportFromClause FromClause `;`

This comment has been minimized.

Copy link
@jmdyck

jmdyck Jun 27, 2019

Collaborator

To me, "ExportFromClause" seems like the wrong name. The nonterminal 'FromClause' is so called because it's a clause that starts with the keyword "from". The nonterminal "ExportFromClause" seems like it should have a similar story, but doesn't.

Maybe "ReExports"? Or I suppose "ExportClause" is justified on the basis of symmetry with the Import productions. (In any case, renaming the current "ExportClause" to "NamedExports" makes sense to me.)

Alternatively, just inline the new production:

ExportDeclaration :
    `export` `*` FromClause `;`
    `export` `*` `as` IdentifierName FromClause `;`
    `export` NamedExports FromClause `;`
    ...

This comment has been minimized.

Copy link
@zenparsing

zenparsing Jun 27, 2019

Member

Yes, I was thinking along the same lines. I would prefer the inlining solution, unless there's some other reason for the additional non-terminal?

Normative: Add `export * as ns from "mod”` (#1174)
Co-authored-by: John-David Dalton <john.david.dalton@gmail.com>
Co-authored-by: Valerie R Young <valerie@bocoup.com>

@ljharb ljharb force-pushed the bocoup:export-star-as-ns branch from b0576ec to a072613 Aug 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.