Editorial: use abstract closures for anonymous functions in more places #2439
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bakkot
force-pushed
the
more-abstract-closures
branch
from
e266aad to
96c0b63
Compare
ljharb
reviewed
Jun 21, 2021
Comment on lines
+39227
to
+39242
| 1. Let _thenFinallyClosure_ be a new Abstract Closure with parameters (_value_) that captures _onFinally_ and _C_ and performs the following steps when called: | ||
| 1. Let _result_ be ? Call(_onFinally_, *undefined*). | ||
| 1. Let _promise_ be ? PromiseResolve(_C_, _result_). | ||
| 1. Let _returnValue_ be a new Abstract Closure with no parameters that captures _value_ and performs the following steps when called: | ||
| 1. Return _value_. | ||
| 1. Let _valueThunk_ be ! CreateBuiltinFunction(_returnValue_, 0, *""*, « »). | ||
| 1. Return ? Invoke(_promise_, *"then"*, « _valueThunk_ »). | ||
| 1. Let _thenFinally_ be ! CreateBuiltinFunction(_thenFinallyClosure_, 1, *""*, « »). | ||
| 1. Let _catchFinallyClosure_ be a new Abstract Closure with parameters (_reason_) that captures _onFinally_ and _C_ and performs the following steps when called: | ||
| 1. Let _result_ be ? Call(_onFinally_, *undefined*). | ||
| 1. Let _promise_ be ? PromiseResolve(_C_, _result_). | ||
| 1. Let _throwReason_ be a new Abstract Closure with no parameters that captures _reason_ and performs the following steps when called: | ||
| 1. Return ThrowCompletion(_reason_). | ||
| 1. Let _thrower_ be ! CreateBuiltinFunction(_throwReason_, 0, *""*, « »). | ||
| 1. Return ? Invoke(_promise_, *"then"*, « _thrower_ »). | ||
| 1. Let _catchFinally_ be ! CreateBuiltinFunction(_catchFinallyClosure_, 1, *""*, « »). |
There was a problem hiding this comment.
fwiw, confirmed these exact changes pass in https://npmjs.com/promise.prototype.finally
syg
approved these changes
Jun 24, 2021
bakkot
force-pushed
the
more-abstract-closures
branch
from
073baa2 to
9eb54b2
Compare
bakkot
added
the
ready to merge
ljharb
force-pushed
the
more-abstract-closures
branch
from
9eb54b2 to
6d3c629
Compare
mathiasbynens
pushed a commit
to mathiasbynens/ecma262
that referenced
this pull request
Oct 18, 2021
mathiasbynens
pushed a commit
to mathiasbynens/ecma262
that referenced
this pull request
Oct 18, 2021
mathiasbynens
pushed a commit
to mathiasbynens/ecma262
that referenced
this pull request
Oct 18, 2021
mathiasbynens
pushed a commit
to mathiasbynens/ecma262
that referenced
this pull request
Oct 18, 2021
mathiasbynens
pushed a commit
to mathiasbynens/ecma262
that referenced
this pull request
Oct 18, 2021
mathiasbynens
pushed a commit
to mathiasbynens/ecma262
that referenced
this pull request
Oct 18, 2021
mathiasbynens
pushed a commit
to mathiasbynens/ecma262
that referenced
this pull request
Oct 18, 2021
mathiasbynens
pushed a commit
to mathiasbynens/ecma262
that referenced
this pull request
Oct 18, 2021
This was referenced Nov 20, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Followup to #2109. Fixes #1894, fixes #933, fixes #1077 (in passing, in the "less handwaving" commit).
There's still six places where I haven't adopted the new style: two (x, x) in CreateResolvingFunctions (used mainly in the Promise constructor), one in PerformPromiseAll, two (x, x) in PerformPromiseAllSettled, and one in PerformPromiseAny.
These remaining cases generally seemed nontrivial enough that I didn't think it worth inlining them. I could, I suppose, replace each with an abstract operation which returns the appropriate abstract closure (or built-in function), but that doesn't seem like it would be much of an improvement.
This is mostly a mechanical transformation. Where there was non-mutable state provided by setting internal slots, I have instead had the abstract closure capture the relevant aliases. I've left mutable state as internal slots, since abstract closures can't write to captured aliases.
One thing of note is that the Default Constructor Functions took a rest parameter, which isn't something we have established syntax for with abstract closures; I decided to mostly mimic what we do in the function constructor and refer to "the List of arguments that was passed to this function by [[Call]] or [[Construct]]" explicitly, rather than introducing syntax for rest parameters in Abstract Closures.