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: consistency around List iteration #2152

Merged
merged 2 commits into from
Sep 2, 2020
Merged

Editorial: consistency around List iteration #2152

merged 2 commits into from
Sep 2, 2020

Conversation

bakkot
Copy link
Contributor

@bakkot bakkot commented Aug 23, 2020

Every place that an algorithm talks about iterating a List it now says 1. For each element _name_ of _list_, do, rather than saying "in" or "that is a member of" or "from" or whatever. Sometimes an actual type is used in place of "element".

I also added a sentence to the description of the List type that explicitly says Lists are iterated in order, and removed the few places in the spec text which explicitly specified that the order of iteration was the order of the List. (Most places just assume it.)

Marked as a draft until I get feedback on this and, if we are in favor of these changes, add a lint rule to ecmarkup to enforce this style where possible. Edit: ready to go. The first commit bumps ecmarkup, which fails linting, and the second commit changes the list iteration style and then passes linting. They should be landed in reverse order.

See also #2142.

@ljharb ljharb marked this pull request as draft August 24, 2020 03:07
@jmdyck
Copy link
Collaborator

jmdyck commented Aug 24, 2020

Was there ever the idea that, if a "For each" didn't state an explicit order, then the order didn't matter (i.e., either the order made no observable difference, or it did but the different behaviors were all deemed valid)? Looking at past editions, I don't think so, but it's hard to be sure.

In cases where different iteration orders would result in different behaviors, does test262 require "List order"?

spec.html Outdated
@@ -14455,7 +14456,7 @@ <h1>Runtime Semantics: Evaluation</h1>
1. If _importMeta_ is ~empty~, then
1. Set _importMeta_ to ! OrdinaryObjectCreate(*null*).
1. Let _importMetaValues_ be ! HostGetImportMetaProperties(_module_).
1. For each Record { [[Key]], [[Value]] } _p_ that is an element of _importMetaValues_, do
1. For each Record { [[Key]], [[Value]] } _p_ of _importMetaValues_, do
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
1. For each Record { [[Key]], [[Value]] } _p_ of _importMetaValues_, do
1. For each Record _p_ of _importMetaValues_, do

I don't think mentioning the fields adds anything here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since these are anonymous Records, the fields are essentially part of the type. I'd lightly prefer to keep them.

Copy link
Member

Choose a reason for hiding this comment

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

could we make them proper named records instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could, but this and the host-defined AO which provides this list are the only two places which talk about this particular kind of record, so I think adding another layer of indirection there would hurt rather than help clarity.

spec.html Outdated
@@ -39350,7 +39351,7 @@ <h1>FinalizationRegistry.prototype.unregister ( _unregisterToken_ )</h1>
1. Perform ? RequireInternalSlot(_finalizationRegistry_, [[Cells]]).
1. If Type(_unregisterToken_) is not Object, throw a *TypeError* exception.
1. Let _removed_ be *false*.
1. For each Record { [[WeakRefTarget]], [[HeldValue]], [[UnregisterToken]] } _cell_ that is an element of _finalizationRegistry_.[[Cells]], do
1. For each Record { [[WeakRefTarget]], [[HeldValue]], [[UnregisterToken]] } _cell_ of _finalizationRegistry_.[[Cells]], do
Copy link
Member

Choose a reason for hiding this comment

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

Same here.

Suggested change
1. For each Record { [[WeakRefTarget]], [[HeldValue]], [[UnregisterToken]] } _cell_ of _finalizationRegistry_.[[Cells]], do
1. For each Record _cell_ of _finalizationRegistry_.[[Cells]], do

Copy link
Member

@michaelficarra michaelficarra left a comment

Choose a reason for hiding this comment

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

LGTM. You should also look into instances of iteration that empty the list, such as this one: #2122 (review). We can normalise those and probably convert many of them to this form if they don't use the list afterward. There's also some that do indexing manually that we could probably convert.

@bakkot
Copy link
Contributor Author

bakkot commented Aug 24, 2020

You should also look into instances of iteration that empty the list

Good catch, will do.

There's also some that do indexing manually that we could probably convert.

Yes:

I'll address the latter three.

@michaelficarra
Copy link
Member

Be careful, the CreateDynamicFunction one doesn't cover the whole list.

@michaelficarra michaelficarra added editor call to be discussed in the next editor call and removed editor call to be discussed in the next editor call labels Aug 25, 2020
@bakkot
Copy link
Contributor Author

bakkot commented Aug 27, 2020

@michaelficarra I added a pair of commits converting those two styles of iteration into the standard style.

There's still an instance of the first style in ForInIteratorPrototype.next step 7.b, but that one is relying on the list mutation to persist state across invocations, so it shouldn't change.

I also left CreateDynamicFunction alone, so the only instances of the second style I changed were in String.fromCharCode and String.fromCodePoint.

@bakkot bakkot marked this pull request as ready for review September 2, 2020 03:43
@ljharb ljharb self-assigned this Sep 2, 2020
@ljharb ljharb merged commit 8dfc62e into master Sep 2, 2020
@ljharb ljharb deleted the for-each-order branch September 2, 2020 05:27
michaelficarra added a commit to ryanjduffy/ecma262 that referenced this pull request Sep 2, 2020
anba added a commit to anba/ecma402 that referenced this pull request Feb 25, 2021
gibson042 added a commit to gibson042/ecma402 that referenced this pull request Apr 12, 2023
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.

4 participants