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.

Sign up for GitHub

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

Jump to bottom

Implement export * as ns from module syntax #5779

Merged
merged 1 commit into from
Oct 31, 2018
Merged

Implement export * as ns from module syntax #5779

merged 1 commit into from
Oct 31, 2018

Conversation

rhuanjl
Copy link
Collaborator

@rhuanjl rhuanjl commented Oct 13, 2018

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: Normative: Add export * as ns from "mod” to Export production and Module Semantic 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

@rhuanjl rhuanjl force-pushed the exportNamespace branch 2 times, most recently from be13504 to fa7b2f7 Compare October 13, 2018 23:18
@rhuanjl rhuanjl mentioned this pull request Oct 14, 2018
Merged
jackhorton
jackhorton approved these changes Oct 15, 2018
View reviewed changes
Copy link
Contributor

@jackhorton jackhorton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

cc @pleath @boingoing

lib/Parser/Parse.cpp
@@ -2357,12 +2357,12 @@ void Parser::ParseNamedImportOrExportClause(ModuleImportOrExportEntryList* impor
// If we are parsing an import statement, the token after 'as' must be a BindingIdentifier.
if (!isExportClause)
{
ChkCurTokNoScan(tkID, ERRsyntax);
ChkCurTokNoScan(tkID, ERRValidIfFollowedBy, _u("'as'"), _u("an identifier."));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hurray for better error messages!

lib/Parser/Parse.h
@@ -1086,11 +1087,11 @@ class Parser
void AddToNodeList(ParseNode ** ppnodeList, ParseNode *** pppnodeLast, ParseNode * pnodeAdd);
void AddToNodeListEscapedUse(ParseNode ** ppnodeList, ParseNode *** pppnodeLast, ParseNode * pnodeAdd);

void ChkCurTokNoScan(int tk, int wErr)
void ChkCurTokNoScan(int tk, int wErr, LPCWSTR stringOne = _u(""), LPCWSTR stringTwo = _u(""))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I would prefer if new code only used char16, those old Windows-style ALLCAPSTYPENAMES are pretty inscrutable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These seem to be typedef'd to const char16_t (on xplat at least) but the windows CI failed if I used that here, wanting instead const wchar - not sure if I can use that on xplat.

If I was going to change here to avoid problems I'd probably need to change it up the chain i.e. in Error() and ParseExceptionObject (admittedly I wrote the code in those two places in #5761)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh interesting. this code may be old enough then that it still expects there to be a difference between wchar types. If thats the case, then theres no issue.

lib/Runtime/Language/ModuleNamespace.cpp Outdated
// special case for export * as ns
if (moduleNameRecord.bindingName == Js::PropertyIds::star_)
{
*value = static_cast <Var> (moduleNameRecord.module->GetNamespace());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: spacing around template arguments

lib/Runtime/Language/SourceTextModuleRecord.cpp Outdated
{
// ambiguous; don't need to search further
// export * as someName from "foo"
*exportRecord = childModuleRecord->GetNamespaceNameRecord();
return true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it may be easier to understand control flow here if the else conditions were eliminated (I know they were here already and you were following the same style). Since each if ends in a return, we can eliminate them altogether.

test/es6module/bug_issue_5777.js Show resolved Hide resolved
@jdalton jdalton mentioned this pull request Oct 15, 2018
Merged
@rhuanjl rhuanjl force-pushed the exportNamespace branch 2 times, most recently from 37e796a to 4fa03d3 Compare October 26, 2018 07:27
@rhuanjl
Copy link
Collaborator Author

rhuanjl commented Oct 30, 2018

Is there anything more I should do on this?

boingoing
boingoing approved these changes Oct 30, 2018
View reviewed changes
Copy link
Contributor

@boingoing boingoing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, please rebase and I'll help test and pull this one in.

@rhuanjl
Implement export * as ns from module syntax
0e1e761 
Also fix chakra-core#5777
And minor syntax error message improvement
@chakrabot chakrabot merged commit 0e1e761 into chakra-core:master Oct 31, 2018
chakrabot pushed a commit that referenced this pull request Oct 31, 2018
@boingoing
[MERGE #5779 @rhuanjl] Implement export * as ns from module syntax
ce09d0e 
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
@rhuanjl rhuanjl deleted the exportNamespace branch October 31, 2018 06:22
chakrabot pushed a commit that referenced this pull request Oct 31, 2018
@boingoing
[MERGE #5780 @rhuanjl] Fix module-namespace indirect exports
ff490af 
Merge pull request #5780 from rhuanjl:indirectExports

Saw the solution to this whilst working on #5779

@digitalinfinity conscious the issue for this (5501) was assigned to you, was intending to leave but saw the fix whilst looking at the code for something else. Let me know if this PR is unwanted and I will close it.

This fixes two cases where a module-namespace object could have undefined indirect exports:
1. `export {a as b} from "thisModule";` - due to it not being in the localExports list but then being marked as a local export when processing the indirectExports
2. `export {default as otherName} from "anyModule";` - due to seemingly unnecessary special handling of `default`

Test262 note, this fix introduces passes for 6 es6 module tests, it is also necessary for several Dynamic Import tests to pass - and for some `export * as ns` tests. The 6 es6 tests that this fixes are:
test/language/module-code/namespace/internals/get-own-property-str-found-init.js
test/language/module-code/namespace/internals/get-str-initialize.js
test/language/module-code/namespace/internals/get-str-found-init.js
test/language/module-code/namespace/internals/get-str-update.js
test/language/module-code/namespace/internals/has-property-str-found-uninit.js
test/language/module-code/namespace/internals/has-property-str-found-init.js

fix #5501
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jackhorton jackhorton jackhorton approved these changes

@fatcerberus fatcerberus fatcerberus left review comments

@boingoing boingoing boingoing approved these changes

Assignees

@boingoing boingoing

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

BUG: Module with duplicate export name should always be a syntax error Implement export-ns-from syntax.
5 participants
@rhuanjl @boingoing @fatcerberus @jackhorton @chakrabot