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

Normative: Set function name and length in CreateBuiltinFunction #2116

Conversation

ExE-Boss
Copy link
Contributor

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

This PR is normative as it defines the order in which the length and name properties are added to built-in functions.

I’ve added name as a parameter, since abstract closures don’t have names, which would make using them to create non‑anonymous functions impossible.

It also preserves compatibility for WebIDL, which uses inline definitions for steps.

Tests: tc39/test262#2921

Unsolved issues:

Blocks:

See also:


Preview (#sec-createbuiltinfunction)

@ExE-Boss ExE-Boss changed the title Normative: Add name and length paremeters to CreateBuiltinFunction Normative: Add name and length paremeters to CreateBuiltinFunction [WIP] Jul 28, 2020
spec.html Outdated Show resolved Hide resolved
@ljharb
Copy link
Member

ljharb commented Jul 28, 2020

I'm confused; the steps themselves should already specify the name and length. Why are they needed here?

@bakkot
Copy link
Contributor

bakkot commented Jul 28, 2020

I'm confused; the steps themselves should already specify the name and length.

They do not.

@ljharb
Copy link
Member

ljharb commented Jul 28, 2020

For example, https://tc39.es/ecma262/#await-fulfilled says "The "length" property of an Await fulfilled function is 1.", and since it says they're anonymous, that means the name is the empty string.

Any "steps" that don't specify these should be fixed to do so.

@bakkot
Copy link
Contributor

bakkot commented Jul 28, 2020

There is prose which specifies these, yes, but not steps. It is odd to explicitly create an object and have steps to set up some of its properties, but have others be described only in prose, which is the current state of affairs.

(The concrete reason to add these as parameters is so that they can be used when passing an abstract closure instead of a sequence of steps, in which case there is no obvious place to put normative prose.)

@ljharb
Copy link
Member

ljharb commented Jul 29, 2020

The length and name of a function defined from steps should imo be determined at the steps' location, and not where the steps happen to be used.

@bakkot
Copy link
Contributor

bakkot commented Jul 29, 2020

I am fine with mentioning it in those locations, but it seems pretty odd to say that's where it should be determined, given that the place the steps are used is the place that sets up internal slots, etc.

(Also, I would like to eventually inline most such steps into the callsites of CreateBuiltInFunction, as in #2109, and this is effectively a prerequisite for that, since after such an inlining there is no longer any other place to put the name and length.)

@ljharb
Copy link
Member

ljharb commented Jul 29, 2020

To me, the steps are what defines the function. Just like how methods define the steps, name, and length in one place, I think these ideally should too.

In other words, I'd prefer to see all these things moved to the steps definition location, rather than moving more of them out to the callsite.

@bakkot bakkot added the editor call to be discussed in the next editor call label Jul 29, 2020
@ExE-Boss ExE-Boss force-pushed the normative/create-builtin-function/add-name-and-length-parameters branch from ce0c88c to 7272289 Compare July 29, 2020 10:28
@ExE-Boss ExE-Boss changed the title Normative: Add name and length paremeters to CreateBuiltinFunction [WIP] Normative: Set function name and length in CreateBuiltinFunction Jul 29, 2020
@ExE-Boss ExE-Boss marked this pull request as ready for review July 29, 2020 10:29
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Per today's editor call, let's make the following change to CreateBuiltinFunction's signature, as well as every callsite of it.

For steps-based functions, they should include (at the CreateBuiltinFunction callsite) steps like the following:

1. Let _length_ be the number of non-optional parameters of _steps_.
1. Let _name_ be the empty String.

Also, in https://tc39.es/ecma262/#sec-ecmascript-standard-built-in-objects, let's change

Each built-in function defined in this specification is created by calling the CreateBuiltinFunction abstract operation (9.3.3).

to be

Each built-in function defined in this specification is created by calling the CreateBuiltinFunction abstract operation (9.3.3). The values of the _name_ and _length_ parameters are the initial values of the _length_ and _name_ properties as discussed below.

spec.html Outdated Show resolved Hide resolved
@ljharb ljharb added needs consensus This needs committee consensus before it can be eligible to be merged. needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 normative change Affects behavior required to correctly evaluate some ECMAScript source text labels Jul 29, 2020
@ExE-Boss
Copy link
Contributor Author

Arguably, the length parameter should be optional, as it can be determined inside CreateBuiltinFunction.

@ExE-Boss ExE-Boss requested review from ljharb and bakkot July 30, 2020 11:44
spec.html Outdated Show resolved Hide resolved
@ExE-Boss ExE-Boss requested a review from ljharb July 30, 2020 21:44
@ljharb ljharb requested review from michaelficarra, syg and a team July 31, 2020 03:35
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.

Sorry for asking for more changes. Reading this again, the language of "Let length be the number of non-optional parameters of steps" is too strange IMO.

If the constraint that @ljharb wants to keep is that the length values be co-located with the steps, I think I'd prefer "Let length be the number of non-optional parameters of a XYZ function as specified below/in XYZ Functions." Basically mirroring the language of "Let steps be the steps of ..."

spec.html Outdated Show resolved Hide resolved
@bakkot bakkot marked this pull request as ready for review January 6, 2021 22:48
@syg
Copy link
Contributor

syg commented Jan 6, 2021

Note to self: figure out if latitude is needed for the web reality of builtin getters and setters and the presence of the get and set prefixes in the function names.

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.

Implementation reality doesn't need extra latitude for accessors, so by my reading this PR is good to go.

spec.html Outdated Show resolved Hide resolved
@bakkot
Copy link
Contributor

bakkot commented Jan 7, 2021

@ExE-Boss This needs a rebase again, after which I believe it will be ready to land. Do you want to take care of it? Otherwise the editors will get to it in a week or two.

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 force-pushed the normative/create-builtin-function/add-name-and-length-parameters branch from bce3f06 to 677e20d Compare January 9, 2021 12:40
@rwaldron rwaldron added has test262 tests and removed needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 labels Jan 29, 2021
@bakkot bakkot added the editor call to be discussed in the next editor call label Feb 3, 2021
@ljharb ljharb requested a review from a team February 3, 2021 22:38
@bakkot bakkot added the es2021 label Feb 9, 2021
@bakkot bakkot added 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 Feb 12, 2021
@ljharb ljharb force-pushed the normative/create-builtin-function/add-name-and-length-parameters branch from 677e20d to 3dd0b48 Compare February 12, 2021 03:20
@ljharb ljharb merged commit 3dd0b48 into tc39:master Feb 12, 2021
@ExE-Boss ExE-Boss deleted the normative/create-builtin-function/add-name-and-length-parameters branch February 12, 2021 06:57
ljharb added a commit that referenced this pull request Feb 12, 2021
Ms2ger added a commit to WebAssembly/spec that referenced this pull request Jun 24, 2021
Ms2ger added a commit to WebAssembly/spec that referenced this pull request Aug 19, 2021
fgmccabe pushed a commit to WebAssembly/js-promise-integration that referenced this pull request Apr 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has consensus This has committee consensus. has test262 tests normative change Affects behavior required to correctly evaluate some ECMAScript source text 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.

None yet

8 participants