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: Add support for Abstract Closures to CreateBuiltinFunction #2109

Conversation

ExE-Boss
Copy link
Contributor

@ExE-Boss ExE-Boss commented Jul 21, 2020

Part of #1894.

I’ve also converted Object.fromEntries to use it, as it’s the only built‑in anonymous function that has no internal slots apart from CreateListIteratorRecord, but built‑in iterators are being converted to use Abstract Closures in #2045.

Depends on:


Preview (#sec-createbuiltinfunction, #sec-object.fromentries)

Copy link
Contributor

@bakkot bakkot left a comment

Choose a reason for hiding this comment

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

Thanks for getting this started! A couple of comments from a first pass.

spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
@ExE-Boss ExE-Boss requested review from bakkot, ljharb and jmdyck July 22, 2020 20:51
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
Copy link
Collaborator

@jmdyck jmdyck left a comment

Choose a reason for hiding this comment

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

It still gives the impression that only the built-ins created via closure get length and name properties, but I gather that PR #2116 will take care of that.

Otherwise, looks okay to me.

@ljharb ljharb requested review from michaelficarra, syg and a team August 10, 2020 05:17
@ljharb ljharb requested a review from a team August 10, 2020 05:18
Copy link
Contributor

@syg syg left a comment

Choose a reason for hiding this comment

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

lgtm with one comment

spec.html Outdated
1. If _realm_ is not present, set _realm_ to the current Realm Record.
1. Assert: _realm_ is a Realm Record.
1. If _prototype_ is not present, set _prototype_ to _realm_.[[Intrinsics]].[[%Function.prototype%]].
1. Let _func_ be a new built-in function object that when called performs the action described by _steps_. The new function object has internal slots whose names are the elements of _internalSlotsList_.
1. Let _func_ be a new built-in function object that when called performs the action described by _algorithm_. The new function object has internal slots whose names are the elements of _internalSlotsList_.
Copy link
Contributor

@syg syg Jan 7, 2021

Choose a reason for hiding this comment

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

I feel like this is lacking clarity a bit for the case with _algorithm_ is an Abstract Closure. Since the definition of ACs explicitly talk about invoking them, perhaps we should have a sentence here that in the case algorithm is an AC, it is invoked with the same arguments in the same order that the built-in function objects was called with, and its return value returned.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Theoretically, algorithm (whether or not it's an AC) is never invoked, it merely specifies the behavior that the new function must exhibit.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough, it is not invoked in the sense of an actual JS function invocation. There's no term we use for the "meta invocation" that's spec-internal calling of an AC. The high order bit for me here was to say something about forwarding of arguments. Perhaps nobody else finds this under-specified and considers "performs the action described" to include the argument forwarding?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually, when I said "invoked", I did mean it in your sense of "meta invocation": algorithm is never meta-invoked. When the new function object is called, it doesn't forward arguments to algorithm, because algorithm isn't in the picture.

E.g., you can imagine that the implementation uses algorithm as a template/blueprint for building the function, and then discards it once the function is created (i.e., by the time this step finishes). algorithm's parameters are a template for the function's parameters, so when the function is called, it will handle its arguments in a way that is consistent with how algorithm handles its parameters.

That's how I read it, anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a fine mental model, I'd still like a sentence about it, like "the function object handles parameters in a way consistent with how algorithm handles parameters"

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps "performs the action described by algorithm using the provided arguments as the values of the corresponding parameters specified by algorithm"?

That doesn't describe how to deal with the case that the function is called with the wrong number of arguments, but that's covered by these paragraphs in ECMAScript Standard Built-in Objects:

Unless otherwise specified in the description of a particular function, if a built-in function or constructor is given fewer arguments than the function is specified to require, the function or constructor shall behave exactly as if it had been given sufficient additional arguments, each such argument being the undefined value. Such missing arguments are considered to be “not present” and may be identified in that manner by specification algorithms. In the description of a particular function, the terms “this value” and “NewTarget” have the meanings given in 10.3.

Unless otherwise specified in the description of a particular function, if a built-in function or constructor described is given more arguments than the function is specified to allow, the extra arguments are evaluated by the call and then ignored by the function. However, an implementation may define implementation specific behaviour relating to such arguments as long as the behaviour is not the throwing of a TypeError exception that is predicated simply on the presence of an extra argument.

Copy link
Contributor

Choose a reason for hiding this comment

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

This wording sgtm.

@ExE-Boss ExE-Boss force-pushed the editorial/create-builtin-function/support-abstract-closures branch from 2c4162c to 1e265b9 Compare February 18, 2021 21:04
spec.html Outdated Show resolved Hide resolved
@ExE-Boss ExE-Boss force-pushed the editorial/create-builtin-function/support-abstract-closures branch 2 times, most recently from 8ed4b63 to 64fc5b2 Compare February 19, 2021 15:25
@ExE-Boss ExE-Boss force-pushed the editorial/create-builtin-function/support-abstract-closures branch from 64fc5b2 to faf5454 Compare March 1, 2021 04:08
@syg syg added editor call to be discussed in the next editor call and removed editor call to be discussed in the next editor call labels Apr 28, 2021
spec.html Outdated Show resolved Hide resolved
@bakkot bakkot added the editor call to be discussed in the next editor call label May 15, 2021
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 otherwise.

@michaelficarra michaelficarra removed the editor call to be discussed in the next editor call label Jun 17, 2021
@ExE-Boss ExE-Boss force-pushed the editorial/create-builtin-function/support-abstract-closures branch from c36adaf to b320097 Compare June 17, 2021 09:41
1. Assert: _realm_ is a Realm Record.
1. If _prototype_ is not present, set _prototype_ to _realm_.[[Intrinsics]].[[%Function.prototype%]].
1. Let _func_ be a new built-in function object that when called performs the action described by _steps_. The new function object has internal slots whose names are the elements of _internalSlotsList_, and an [[InitialName]] internal slot.
1. Let _func_ be a new built-in function object that, when called, performs the action described by _behaviour_. The new function object has internal slots whose names are the elements of _internalSlotsList_, and an [[InitialName]] internal slot.
Copy link
Contributor

@bakkot bakkot Jun 18, 2021

Choose a reason for hiding this comment

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

Suggested change
1. Let _func_ be a new built-in function object that, when called, performs the action described by _behaviour_. The new function object has internal slots whose names are the elements of _internalSlotsList_, and an [[InitialName]] internal slot.
1. Let _func_ be a new built-in function object that, when called, performs the action described by _behaviour_ using the provided arguments as the values of the corresponding parameters specified by _behaviour_. The new function object has internal slots whose names are the elements of _internalSlotsList_, and an [[InitialName]] internal slot.

per discussion.

Edit: pushed a commit doing this.

@bakkot bakkot added the ready to merge Editors believe this PR needs no further reviews, and is ready to land. label Jun 19, 2021
…n` (tc39#2109)

Co-authored-by: ExE Boss <[email protected]>
Co-authored-by: Kevin Gibbons <[email protected]>
Co-authored-by: Michael Dyck <[email protected]>
Co-authored-by: Jordan Harband <[email protected]>
Co-authored-by: Michael Ficarra <[email protected]>
@ljharb ljharb force-pushed the editorial/create-builtin-function/support-abstract-closures branch from 0388051 to fbdf389 Compare June 20, 2021 16:27
@ljharb ljharb merged commit fbdf389 into tc39:master Jun 20, 2021
@ExE-Boss ExE-Boss deleted the editorial/create-builtin-function/support-abstract-closures branch June 21, 2021 10:39
mathiasbynens pushed a commit to mathiasbynens/ecma262 that referenced this pull request Oct 18, 2021
…n` (tc39#2109)

Co-authored-by: ExE Boss <[email protected]>
Co-authored-by: Kevin Gibbons <[email protected]>
Co-authored-by: Michael Dyck <[email protected]>
Co-authored-by: Jordan Harband <[email protected]>
Co-authored-by: Michael Ficarra <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
editorial change 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