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

Editorial: stop using strings for special values #2155

Closed
wants to merge 3 commits into from

Conversation

ljharb
Copy link
Member

@ljharb ljharb commented Aug 26, 2020

This was extracted from #2154 so it could land separately.

@ljharb ljharb requested review from syg, michaelficarra, bmeck, bakkot and a team August 26, 2020 23:21
ljharb pushed a commit to ljharb/ecma262 that referenced this pull request Aug 26, 2020
This was extracted from tc39#2154 so it could land separately.
spec.html Outdated Show resolved Hide resolved
ljharb pushed a commit to ljharb/ecma262 that referenced this pull request Aug 26, 2020
This was extracted from tc39#2154 so it could land separately.
ljharb pushed a commit to ljharb/ecma262 that referenced this pull request Aug 26, 2020
This was extracted from tc39#2154 so it could land separately.
spec.html Outdated Show resolved Hide resolved
ljharb added a commit to ljharb/ecma262 that referenced this pull request Aug 26, 2020
This was extracted from tc39#2154 so it could land separately.

Co-authored-by: Jordan Harband <ljharb@gmail.com>
@ljharb ljharb requested a review from devsnek August 26, 2020 23:40
spec.html Outdated Show resolved Hide resolved
ljharb added a commit to ljharb/ecma262 that referenced this pull request Sep 2, 2020
This was extracted from tc39#2154 so it could land separately.

Co-authored-by: Jordan Harband <ljharb@gmail.com>
ljharb added a commit to ljharb/ecma262 that referenced this pull request Sep 2, 2020
This was extracted from tc39#2154 so it could land separately.

Co-authored-by: Jordan Harband <ljharb@gmail.com>
ljharb added a commit to ljharb/ecma262 that referenced this pull request Sep 2, 2020
This was extracted from tc39#2154 so it could land separately.

Co-authored-by: Jordan Harband <ljharb@gmail.com>
ljharb added a commit to ljharb/ecma262 that referenced this pull request Sep 2, 2020
This was extracted from tc39#2154 so it could land separately.

Co-authored-by: Jordan Harband <ljharb@gmail.com>
@bakkot
Copy link
Contributor

bakkot commented Sep 8, 2020

There's another special string: "*", used in ImportEntry Records to indicate that the import request is for the target module's namespace object.

spec.html Outdated
@@ -23816,7 +23816,7 @@ <h1>InitializeEnvironment ( ) Concrete Method</h1>
1. Let _module_ be this Source Text Module Record.
1. For each ExportEntry Record _e_ of _module_.[[IndirectExportEntries]], do
1. Let _resolution_ be ? _module_.ResolveExport(_e_.[[ExportName]]).
1. If _resolution_ is *null* or *"ambiguous"*, throw a *SyntaxError* exception.
1. If _resolution_ is *null* or ~ambiguous~ throw a *SyntaxError* exception.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
1. If _resolution_ is *null* or ~ambiguous~ throw a *SyntaxError* exception.
1. If _resolution_ is *null* or ~ambiguous~, throw a *SyntaxError* exception.

spec.html Outdated
@@ -12165,7 +12165,7 @@ <h1>Runtime Semantics: BindingInitialization</h1>
<h1>Runtime Semantics: InitializeBoundName ( _name_, _value_, _environment_ )</h1>
<p>The abstract operation InitializeBoundName takes arguments _name_, _value_, and _environment_. It performs the following steps when called:</p>
<emu-alg>
1. Assert: Type(_name_) is String.
1. Assert: Either Type(_name_) is String, or _name_ is ~default~.
1. If _environment_ is not *undefined*, then
1. Perform _environment_.InitializeBinding(_name_, _value_).
Copy link
Contributor

Choose a reason for hiding this comment

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

I am pretty sure we shouldn't actually be attempting to initialize a binding named ~default~.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed in 870e5f7

@ljharb
Copy link
Member Author

ljharb commented Sep 8, 2020

Handled the ImportEntry Records string in 158950a.

@@ -24636,7 +24635,7 @@ <h1>Runtime Semantics: Evaluation</h1>
<emu-alg>
1. Let _value_ be ? BindingClassDeclarationEvaluation of |ClassDeclaration|.
1. Let _className_ be the sole element of BoundNames of |ClassDeclaration|.
1. If _className_ is *"\*default\*"*, then
1. If _className_ is ~default~, then
Copy link
Contributor

@bakkot bakkot Sep 8, 2020

Choose a reason for hiding this comment

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

I don't actually understand why we're making/initializing a binding here at all. Possibly some interaction with module instantiation I'm missing? Anyone know what's up with that?

@@ -20352,10 +20351,10 @@ <h1>Static Semantics: BoundNames</h1>
</emu-alg>
<emu-grammar>GeneratorDeclaration : `function` `*` `(` FormalParameters `)` `{` GeneratorBody `}`</emu-grammar>
<emu-alg>
1. Return &laquo; *"\*default\*"* &raquo;.
1. Return &laquo; ~default~ &raquo;.
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this still ends up making a binding named ~default~ in InitializeEnvironment of Source Text Module Records, step 23.

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Feb 10, 2021
…tcampbell

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: tc39/ecma262#2155

Differential Revision: https://phabricator.services.mozilla.com/D101013
jamienicol pushed a commit to jamienicol/gecko that referenced this pull request Feb 11, 2021
…tcampbell

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: tc39/ecma262#2155

Differential Revision: https://phabricator.services.mozilla.com/D101013
@ljharb ljharb force-pushed the master branch 3 times, most recently from 3d0c24c to 7a79833 Compare June 29, 2021 02:21
</td>
<td>
The name under which the desired binding is exported by the module identified by [[ModuleRequest]]. *null* if the |ExportDeclaration| does not have a |ModuleSpecifier|. *"\*"* indicates that the export request is for all exported bindings.
The name under which the desired binding is exported by the module identified by [[ModuleRequest]]. *null* if the |ExportDeclaration| does not have a |ModuleSpecifier|. ~*~ indicates that the export request is for all exported bindings.
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to use something like ~all~ for this.

@syg syg added the editor call to be discussed in the next editor call label Oct 25, 2021
@ljharb ljharb closed this in #2566 Nov 5, 2021
@bakkot bakkot removed the editor call to be discussed in the next editor call label Nov 10, 2021
@ljharb ljharb deleted the enumb branch January 26, 2022 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants