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: Define required internal slots in CreateBuiltinFunction steps #2115

Conversation

ExE-Boss
Copy link
Contributor

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

The way CreateBuiltinFunction is currently specified would’ve resulted in it not having the [[Prototype]], [[Extensible]], [[Realm]] and [[ScriptOrModule]] internal slots unless they were explicitly passed using CreateBuiltinFunction(steps, « [[Prototype]], [[Extensible]], [[Realm]], [[ScriptOrModule]] »).

I’ve also made it optional in preparation for converting many of the built‑in functions to use abstract closures after #2109.

See also:


Preview (#sec-createbuiltinfunction)

@ExE-Boss ExE-Boss force-pushed the editorial/create-builtin-function/fix-internal-slots-list-param branch 3 times, most recently from f622968 to 464b597 Compare July 28, 2020 11:08
@anba
Copy link
Contributor

anba commented Jul 28, 2020

Built-in functions can be implemented as ECMAScript function objects, in which case they have even more internal slots. If they are not implemented as ECMAScript function objects, but instead as implementation provided function exotic objects, they are already described as required to have these slots:

If a built-in function object is implemented as an exotic object it must have the ordinary object behaviour specified in 9.1. All such function exotic objects also have [[Prototype]], [[Extensible]], [[Realm]], and [[ScriptOrModule]] internal slots.

@ExE-Boss
Copy link
Contributor Author

they are already described as required to have these slots:

This makes that requirement explicit, rather than implicit.

@anba
Copy link
Contributor

anba commented Jul 28, 2020

But it gives the impression that only [[Prototype]], [[Extensible]], [[Realm]], and [[ScriptOrModule]] are needed, which isn't true for ECMAScript function objects. If this needs to be explicit, it should have two branches to cover the case if the built-in function is an ECMAScript function object or an exotic object. And in each branch the appropriate slots should be initialised. (For example right now the spec also doesn't cover how the other internal slots for a built-in function which is an ECMAScript function object are initialised. This is all left to be defined in an actual implementation.)

ljharb
ljharb previously requested changes Jul 28, 2020
spec.html Outdated Show resolved Hide resolved
@ljharb
Copy link
Member

ljharb commented Jul 28, 2020

The requirement is already explicit - it's just not in this location.

@ExE-Boss ExE-Boss force-pushed the editorial/create-builtin-function/fix-internal-slots-list-param branch 3 times, most recently from 750835c to 429cc47 Compare August 6, 2020 18:12
@ExE-Boss ExE-Boss requested a review from ljharb August 6, 2020 18:12
spec.html Outdated
Comment on lines 9059 to 9061
1. If the built-in function object is implemented using ECMAScript code, then
1. Let _internalSlotsList_ be the internal slots listed in <emu-xref href="#table-27"></emu-xref>.
1. Otherwise, let _internalSlotsList_ be &laquo; [[Prototype]], [[Extensible]], [[Realm]], [[ScriptOrModule]] &raquo;.
Copy link
Member

Choose a reason for hiding this comment

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

This also seems unnecessary - func is "a new built-in function object", so it automatically has these slots with no further spec text.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I’d like to convert CreateBuiltinFunction to use MakeBasicObject if possible.

spec.html Outdated
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. Set _func_.[[Realm]] to _realm_.
1. Set _func_.[[Prototype]] to _prototype_.
1. If the built-in function object is implemented using ECMAScript code, then
Copy link
Contributor

Choose a reason for hiding this comment

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

I am against this change. Switching on implementation details in normative prose seems bad to me.

Copy link
Collaborator

Choose a reason for hiding this comment

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

In the current spec, I think the only obvious example of switching on an implementation detail is:

  • If an implementation-defined debugging facility is available and enabled, ....

For "host", there's:

  • If the host requires use of an exotic object to serve as realm's global object, ...
  • If the host requires that the *this* binding in realm's global scope return an object other than the global object, ...
  • If <various conditions> and ! HostHasSourceTextAvailable(_func_) is *true*, ...

That's all I could find.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@bakkot:

Switching on implementation details in normative prose seems bad to me.

9.3 Built-in Function Objects says that if you implement a built-in function as an exotic function, then it must satisfy some extra requirements (that run-of-the-mill exotic functions don't have to). So that is normative prose that is switching on an implementation 'detail'. The proposed text that you were objecting to seems comparable, just in algorithmic form.

@ExE-Boss ExE-Boss changed the title Editorial: Fix internalSlotsList parameter of CreateBuiltinFunction Editorial: Define required internal slots in CreateBuiltinFunction steps Aug 14, 2020
@ExE-Boss ExE-Boss force-pushed the editorial/create-builtin-function/fix-internal-slots-list-param branch from 429cc47 to 0d4fa01 Compare August 31, 2020 14:47
@ExE-Boss ExE-Boss requested review from ljharb, bakkot and jmdyck August 31, 2020 14:57
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated
1. Let _internalSlotsList_ be the internal slots specified in <emu-xref href="#sec-built-in-function-objects"></emu-xref>.
1. Append [[InitialName]] to _internalSlotsList_.
1. Append each element of _additionalInternalSlotsList_ to _internalSlotsList_.
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_.
Copy link
Collaborator

Choose a reason for hiding this comment

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

As @anba pointed out: if _func_ is an ECMAScript (i.e. ordinary) function object, it must have all the internal slots listed here, but this operation doesn't mention most of them or say how they're initialized. Of course, that's been true since CreateBuiltinFunction was introduced, but this PR appears to be trying for completeness. I think it's somewhat misleading in its current state.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I’m not sure how I should specify that given @bakkot’s review in #2115 (comment).

Copy link
Collaborator

Choose a reason for hiding this comment

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

If @bakkot continues to object, then I think the solution is for CreateBuiltinFunction to not be as specific about slots as you'd like. E.g., you could ditch the construction of _internalSlotsList_ and then say something like: The new function object has the internal slots required by either (9.2) or (9.3), as appropriate, and also has internal slots whose names are the elements of _additionalInternalSlotsList_.

(It should be possible to replace "either (9.2) or (9.3), as appropriate" with just "(9.3)", since 9.3 deals with all built-in functions, and covers the ordinary/exotic implementation choice. But I think, as 9.3 currently stands, that would be a little misleading, as readers would only look at the slots explicitly mentioned in 9.3 and not consider those included by reference to 9.2.)

@ExE-Boss ExE-Boss force-pushed the editorial/create-builtin-function/fix-internal-slots-list-param branch from 0d4fa01 to b12b3c1 Compare September 9, 2020 07:03
@ExE-Boss ExE-Boss requested a review from jmdyck September 9, 2020 07:05
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
@jmdyck
Copy link
Collaborator

jmdyck commented Jun 12, 2021

I've been requested to review this, and I'm willing to do so, but I think the merge conflicts should be resolved first. In particular, note that #2190 dropped the [[ScriptOrModule]] slot for exotic built-ins.

@ljharb ljharb force-pushed the editorial/create-builtin-function/fix-internal-slots-list-param branch from c6bfede to 1436088 Compare June 12, 2021 20:29
@ljharb
Copy link
Member

ljharb commented Jun 12, 2021

@jmdyck i've rebased it; feel free to take a look.

jmdyck
jmdyck previously requested changes Jun 13, 2021
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 turned out my comments wouldn't have fit very well into the "suggested change" boxes, so instead I filed ExE-Boss#1 against this PR's branch.

@jmdyck
Copy link
Collaborator

jmdyck commented Jun 13, 2021

It's still the case (as it has been since CreateBuiltinFunction was introduced) that if the function being created is an ordinary function object, CreateBuiltinFunction doesn't say how most of its internal slots are initialized. Without trying to 'fix' that shortcoming, we could maybe at least acknowledge it in a Note. E.g.:

NOTE: If func is an ECMAScript function object, it has many other internal slots that must be initialized, but this operation does not address how that happens.

With or without that, I think this PR is somewhat clearer than the status quo about internal slots of built-ins.

@ljharb ljharb force-pushed the master branch 3 times, most recently from 3d0c24c to 7a79833 Compare June 29, 2021 02:21
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.

This looks good, but it will need to be rebased (due to #545) to have the proper header format for CreateBuiltinFunction.

@bakkot bakkot force-pushed the editorial/create-builtin-function/fix-internal-slots-list-param branch from e09a329 to a1d076f Compare July 22, 2021 06:35
@bakkot
Copy link
Contributor

bakkot commented Jul 22, 2021

Done.

@bakkot bakkot added the ready to merge Editors believe this PR needs no further reviews, and is ready to land. label Jul 22, 2021
@ljharb ljharb force-pushed the editorial/create-builtin-function/fix-internal-slots-list-param branch from a1d076f to e7610a8 Compare July 22, 2021 17:48
@ljharb ljharb dismissed jmdyck’s stale review July 22, 2021 17:48

changes addressed

@ljharb ljharb removed their request for review July 22, 2021 17:48
@ljharb ljharb dismissed their stale review July 22, 2021 17:49

changes addressed

@ljharb ljharb merged commit e7610a8 into tc39:master Jul 22, 2021
@ExE-Boss ExE-Boss deleted the editorial/create-builtin-function/fix-internal-slots-list-param branch July 23, 2021 07:23
mathiasbynens pushed a commit to mathiasbynens/ecma262 that referenced this pull request Oct 18, 2021
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