Bug 160343

Summary: [ES6] Module binding can be exported by multiple names
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: JavaScriptCoreAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, keith_miller, mark.lam, msaboff, saam
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Yusuke Suzuki 2016-07-29 09:53:41 PDT
Module can export the same local binding by using multiple names.


var value = 42;


export { value };
export { value as value2 };
Comment 1 Yusuke Suzuki 2016-07-30 12:57:25 PDT
Created attachment 284952 [details]
Patch
Comment 2 Yusuke Suzuki 2016-07-30 13:12:36 PDT
Created attachment 284953 [details]
Patch
Comment 3 Saam Barati 2016-07-30 16:19:12 PDT
Comment on attachment 284953 [details]
Patch

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

r=me

> Source/JavaScriptCore/parser/Parser.cpp:2793
> +template <class TreeBuilder> typename TreeBuilder::ExportSpecifier Parser<LexerType>::parseExportSpecifier(TreeBuilder& context, Vector<std::pair<const Identifier*, const Identifier*>>& maybeExportedLocalNames, bool& hasKeywordForLocalBindings)

Why is this called "maybeExportedLocalNames" instead of just "exportedLocalNames".
It looks like we iterate the list later and export all the names.

> Source/JavaScriptCore/tests/modules/aliased-names/main.js:10
> +export { change }

Maybe it's worth adding a test that should fail when the program is something like:
export {foo as bar, baz as bar}
Comment 4 Yusuke Suzuki 2016-07-31 00:02:23 PDT
Comment on attachment 284953 [details]
Patch

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

Thanks!

>> Source/JavaScriptCore/parser/Parser.cpp:2793
>> +template <class TreeBuilder> typename TreeBuilder::ExportSpecifier Parser<LexerType>::parseExportSpecifier(TreeBuilder& context, Vector<std::pair<const Identifier*, const Identifier*>>& maybeExportedLocalNames, bool& hasKeywordForLocalBindings)
> 
> Why is this called "maybeExportedLocalNames" instead of just "exportedLocalNames".
> It looks like we iterate the list later and export all the names.

This is because maybeExportedLocalNames could be non-local names.
Here, we need to handle two syntax.

1.
export { a, b, c };

2.
export { a, b, c } from "other-module";

In the first case, a, b, c should be local bindings. So it is OK.
However, in the second case, a, b, c is directly exported from the "other-module" and there are no local bindings in the current module (And we must not create such local binding in the current module). So in this case, these collected names are not exportedLocalNames.


We cannot determine which the current parsing syntax is ((1) OR (2) ?) until we encounter the `from "other-module"` part. So when parsing export specifiers (here), these collected names are `maybe` exported local names.

>> Source/JavaScriptCore/tests/modules/aliased-names/main.js:10
>> +export { change }
> 
> Maybe it's worth adding a test that should fail when the program is something like:
> export {foo as bar, baz as bar}

Sounds nice!
Comment 5 Yusuke Suzuki 2016-07-31 00:04:49 PDT
Created attachment 284962 [details]
Patch
Comment 6 Yusuke Suzuki 2016-07-31 00:05:18 PDT
Comment on attachment 284962 [details]
Patch

Oops, accidentally cleared r+. I'll land manually.
Comment 7 Yusuke Suzuki 2016-07-31 00:06:18 PDT
Committed r203953: <http://trac.webkit.org/changeset/203953>