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: Specify Map/Set forEach more explicitly #2766

Merged
merged 1 commit into from
Aug 10, 2022
Merged

Conversation

bakkot
Copy link
Contributor

@bakkot bakkot commented May 3, 2022

@tabatkins pointed out that Map and Set iterators are specified with explicit indexing, whereas their forEach methods are specified with a "for each element of entries" style loop. The historical reason for that is probably path dependence (explicit indices were required for the iterators prior to the refactoring in #2045), but there is now no particular reason they should be different.

I've chosen to make forEach match the iterators rather than the other way around because I think it makes it clearer that iteration is required to hit elements added to the list during iteration (either between calls to the iterator's next method or by the callback to forEach). We could go the other way, though.

Copy link
Contributor

@gibson042 gibson042 left a comment

Choose a reason for hiding this comment

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

This is good. Are there any other instances of "For each … of <List>" in which the size of the List can change during iteration (that should presumably also be cleaned up)?

spec.html Outdated
Comment on lines 40212 to 40215
1. Let _index_ be 0.
1. Let _entries_ be the List that is _set_.[[SetData]].
1. Let _numEntries_ be the number of elements of _entries_.
1. Repeat, while _index_ &lt; _numEntries_,
Copy link
Contributor

Choose a reason for hiding this comment

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

Consistency nit: it would be nice if all four algorithms declared aliases in the same sequence. And looking at the rest of the spec, it is very common for the iteration alias to be declared immediately before iteration.

Suggested change
1. Let _index_ be 0.
1. Let _entries_ be the List that is _set_.[[SetData]].
1. Let _numEntries_ be the number of elements of _entries_.
1. Repeat, while _index_ &lt; _numEntries_,
1. Let _entries_ be the List that is _set_.[[SetData]].
1. Let _numEntries_ be the number of elements of _entries_.
1. Let _index_ be 0.
1. Repeat, while _index_ &lt; _numEntries_,

spec.html Outdated
@@ -40022,7 +40028,7 @@ <h1>
1. Assert: _kind_ is ~key+value~.
1. Let _result_ be CreateArrayFromList(&laquo; _e_.[[Key]], _e_.[[Value]] &raquo;).
1. Perform ? GeneratorYield(CreateIterResultObject(_result_, *false*)).
1. NOTE: The number of elements in _entries_ may have changed while execution of this abstract operation was paused by Yield.
1. NOTE: The number of elements in _entries_ may have increased while execution of this abstract operation was paused by Yield.
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Choose a reason for hiding this comment

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

While "increased" certainly points to the most important consequence of this, "changed" is still more accurate imo; the number decreasing will also affect the behavior. Might be useful to talk about the order possibly changing as well, like I did in WebIDL:

Note: The size of map, and the order of its entries, might have changed while execution of this abstract operation was paused by Yield.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The number can't decrease, is why I changed this. Elements are never removed from the internal list for a Map or Set.

Choose a reason for hiding this comment

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

Fair, but they can change order which can also be surprising.

Copy link
Member

Choose a reason for hiding this comment

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

How can they?

Choose a reason for hiding this comment

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

You can remove an element from the map and then re-insert it.

Copy link
Member

Choose a reason for hiding this comment

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

That's not really changing order, but you're right that it might be surprising to get the same key twice - but that's still the size increasing.

@ljharb
Copy link
Member

ljharb commented May 4, 2022

Would it be cleaner to come up with a variant of the for each syntax that handles this, rather than effectively converting an iteration into a manual loop?

@bakkot
Copy link
Contributor Author

bakkot commented May 4, 2022

Are there any other instances of "For each … of " in which the size of the List can change during iteration (that should presumably also be cleaned up)?

None that I saw at a quick glance.

Would it be cleaner to come up with a variant of the for each syntax that handles this, rather than effectively converting an iteration into a manual loop?

I can't think of a clean way of doing that.

spec.html Outdated Show resolved Hide resolved
@tabatkins
Copy link

I've chosen to make forEach match the iterators rather than the other way around because I think it makes it clearer that iteration is required to hit elements added to the list during iteration

I also think this is clearer, and I'm about to start writing a tweak to the Infra spec to make map and list iteration better defined and plan to use this "internal index" style as well.

@bakkot bakkot added the editor call to be discussed in the next editor call label Aug 9, 2022
@michaelficarra michaelficarra added editor call to be discussed in the next editor call ready to merge Editors believe this PR needs no further reviews, and is ready to land. and removed editor call to be discussed in the next editor call labels Aug 10, 2022
@bakkot bakkot removed the editor call to be discussed in the next editor call label Aug 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Editors believe this PR needs no further reviews, and is ready to land.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants