Normative: provide default prototype and realm values for CreateBuiltinFunction

[brabalan]

To address #1030

This is my first pull request to the spec, please let me know if I'm doing it wrong.

[littledan]

I like the idea of this change. I agree with you that the spec text is a little bit vague in this respect. However, I have some small suggestions for improvements in the way you've phrased things, all minor editorial nits:

• We usually put optional arguments in square brackets, like in man pages. However, these are not denoted that way. Also, the second argument is left not optional, while the first is optional, which we tend not to do (since you provide arguments from left to right).
• I'm not sure "what the current realm record is" when reading spec text definitions of functions. It's a sort of level thing--there's a realm being constructed, but is it really the "current' one at that point? Maybe you could say instead, the realm under construction?
• There's no actual call of this abstract algorithm with the incomplete argument list, just the "as if" wording. Maybe, rather than phrasing this as default arguments, there could be a more precise argument list provided in that paragraph at the bottom instead.

[brabalan]

@littledan My motivation is to understand what to do in step 2 of https://tc39.github.io/ecma262/#sec-proxy.revocable. In that call to this abstract algorithm, there is no prototype nor realm defined. An alternate fix is to find all the calls to the abstract algorithm and make sure they always specify the prototype and realm.

Regarding "the current realm record", it is defined here https://tc39.github.io/ecma262/#current-realm: it's the realm of the running execution context.

[littledan]

Oh, I misunderstood and was trying to explain how to clarify a different step, where I thought the current realm was more vague, in CreateIntrinsics. However, it seems that CreateIntrinsics does have a direct call to CreateBuiltinFunction, and the line at the end of CreateBuiltinFunction describing that it's called in this case doesn't serve a normative function.

For the case in Proxy.revokable, could this be handled by replacing step 2 with a direct call to CreateBuiltinFunction with the appropriate arguments? Otherwise, the semantics make sense to me with respect to Proxy.revocable.

cc @verwaest @domenic , is this how you interpreted the realm for Proxy.revocable?

[brabalan]

@littledan Using CreateBuiltinFunction in Proxy.revokable is indeed a solution. Should I amend this pull request or create a new one?

[littledan]

@brabalan I'd amend the current PR.

[brabalan]

Done. I also seized the occasion to make clear the internal slot we need.

[littledan]

Great fix--I think it makes the specification much more clear.

[ljharb]

LGTM pending comment

[ljharb]

Each argument comma needs a space after it.

[brabalan]

@ljharb fixed

[anba]

I'd inline %FunctionPrototype% in the call to CreateBuiltinFunction, cf. MakeArgGetter and MakeArgSetter.

[anba]

What is special about Proxy.revokable that it needs to call CreateBuiltinFunction explicitly whereas any other built-in function created in algorithm steps doesn't need to call it? See CreateListIteratorRecord, CreateResolvingFunctions, NewPromiseCapability, PerformPromiseAll, AsyncFunctionAwait.

[allenwb]

The root problem here is in clause 17 where it says:

Unless otherwise specified, each built-in function defined in this specification is created as if by calling the CreateBuiltinFunction abstract operation (9.3.3).

The intent of that line was to eliminate the need to have to explicitly show the determination of the CreateBuildinFunction arguments every place we need to create one. But the text has too much ambiguity WRT the arguments passed to CreateBuildinFunction. I'd restate it as:

Unless otherwise specified, each built-in function defined in this specification is created as if by calling the CreateBuiltinFunction abstract operation (9.3.3) using the steps and internal slots (if any) provided by the specification of the function. Unless otherwise specified the realm argument is the current realm and the prototype argument is the %FunctionPrototype% of that realm.

Note that "current realm" is defined in https://tc39.github.io/ecma262/#sec-execution-contexts

With the above addition the current language in step 2 of Proxy.revoke and all other similar built-in function creation steps should be fine.

[brabalan]

@allenwb I agree, this is very similar to what I initially proposed (in English instead of providing default values in the steps). I like that solution because it fixes everything once and for all.

Should I amend the pull request to reflect this? (I'm new to this, I don't know what the process should be.)

[littledan]

Sorry for getting things off-track here with that suggestion! I missed the call in section 17. Is that section normative text? I don't really understand what that spec text is doing--aren't the same built-in function objects provided by 8.2.2 CreateIntrinsics (which is already explicit about its realm argument, though maybe it could use some more clarity for the prototype)?

Minor: Do we prefer to use functional notation or general sentences when calling abstract algorithms like CreateBuiltinFunction from paragraphs? I'm not against using sentences (it seems to work just fine for web specs) but was suggesting functional notation above since it seems to be used most of the time in this document.

[jmdyck]

I don't really understand what that spec text [in section 17] is doing--aren't the same built-in function objects provided by 8.2.2 CreateIntrinsic [...]?

8.2.2 CreateIntrinsics covers the built-in functions that are in or reachable from Table 7: 'Well-Known Intrinsic Objects', but that isn't (quite) every built-in function defined in the spec.

Of that remainder, some are created by explicit calls to CreateBuiltinFunction (in MakeArgGetter and MakeArgSetter), and the rest are created by wording such as
Let _foo_ be a new built-in function object as defined in <section>.
(See the list from @anba upthread.) It's for the latter that the wording in 17 is needed.

(If we converted the latter to explicit calls to CreateBuiltinFunction (see old bug 4080), I don't think we'd need the wording in 17 or 9.3.3, though we could say something similar in a note. I should maybe submit a PR.)

[littledan]

To answer @anba 's question about what makes Proxy.revocable different from those other cases--nothing, just that I didn't remember those cases and so didn't bring them up. The current logic was a little hard for me to trace, especially with a lot of places that create built-in function objects somehow just linking to "function object". If these aren't changed to call CreateBuiltinFunction directly, maybe they should at least each link to either section 17 or 9.3.3.

[littledan]

@jmdyck

If we converted the latter to explicit calls to CreateBuiltinFunction (see old bug 4080), I don't think we'd need the wording in 17 or 9.3.3, though we could say something similar in a note. I should maybe submit a PR.

+100 for this change

[allenwb]

The reason for the paragraph in clause 17 (and all of clause 17 is indeed normative) the need to repetitively use a 4 step sequence such as that proposed in #1044 (review)

The primary purpose of all of clause 17 is to define such short cuts.

[littledan]

@allenwb I see. Would it be acceptable to somehow link from the places where it applies to the relevant part of section 17? Or, as a maybe-too-explicit alternative, what if we used a "spec macro" the way @domenic is refactoring Await in the async iteration spec ?

[allenwb]

well, the macro is essentially "a new built-in function object as defined in". Clause 17 is the definition of that macro.

Note that clause 17 applies to every function defined in clauses 18-26, unless otherwise specified.

[jmdyck]

the need to repetitively use a 4 step sequence such as that proposed in #1044 (review)

As @anba pointed out above, the prototype can be specified inline, so it'd only be a 3-step sequence.

And we could arrange that realm is an optional arg, defaulting to the current Realm Record, reducing it to a 2-step sequence. (We could also arrange that prototype is optional, defaulting to %FunctionPrototype%.)

We could get get it down to 1 step, but only by introducing some new notation, which probably isn't worth it.

[jmdyck]

Note that clause 17 applies to every function defined in clauses 18-26, unless otherwise specified.

Yes, except almost all of them are otherwise specified, in CreateIntrinsics.

[brabalan]

To summarize, if I understand correctly, we have two options:

• explicitly use CreateBuiltinFunction providing explicit arguments
• amend clause 17 to give default values for realm and prototype

The former is more precise and formal, but requires much more work to find out all the places that require the change. How do we choose which option to take?

[jmdyck]

[explicitly using CreateBuiltinFunction] requires much more work to find out all the places that require the change.

The only algorithm steps that require the change are those using the phrase "a new built-in function object as defined in". There are 8 such occurrences, and @anba has already listed where they occur.

Are there any built-in functions defined in the spec that are not created by some algorithm step? I've scanned through the spec and I don't think so. How else could they be created?

[littledan]

@jmdyck Well, seems like ES5.1 didn't have all this explicit CreateBuiltinFunction/CreateIntrinsics logic, so that's a way. Hats off to @allenwb for making things more concrete in ES2015. In this bug, we're just discussing a minor tweak on something that was already significantly improved in that iteration.

[jmdyck]

When I said "How else could they be created?", I didn't mean "How could the spec be (re)organized to create built-in functions differently?", but rather "Is there any other wording/mechanism that the current spec is using to create built-in functions?" (I.e., basically just rephrasing the first sentence of that para.)

(This question is important to determining, after certain changes, whether the para in clause 17 is needed any more. We could make the changes and yet retain the clause 17 para "just in case", but that would be unsatisfying.)

[brabalan]

I changed all the occurrences listed by @anba to explicitly call CreateBuiltinFunction.

[littledan]

@bterlson What do you think of this patch?

[brabalan]

Is there anything I can do to help move this forward?

[jmdyck]

You missed a case in CreateResolvingFunctions ("Let reject be a new built-in function object ...").

You need to resolve conflicts (rebase against ecma262's current master).
When you do, you'll find there are two new cases, in Promise.prototype.finally.

You may need to wait a while for this to be merged. I gather @bterlson is focusing on the stage 4 normative PRs (I think there are two left: #1048 and #1066) so that they can get into the 2018 snapshot. After that, he'll have time to look at editorial PRs (which have a later snapshot-deadline).

[brabalan]

@jmdyck Thank you for the feedback. I merged the changes from master and added the missed cases.

[jmdyck]

The fact that you merged from master (rather than rebasing to it) confused my limited git-fu (when I tried to pull your changes onto one of my branches, for checking). But I eventually got it via some cherry-picks.

The net effect looks good to me. (@bterlson, I recommend pulling it in as a single commit.)

[brabalan]

@jmdyck Sorry for the merge, I thought one should never rebase a published branch. Is it different for pull requests?

[jmdyck]

I thought one should never rebase a published branch.

Hm, yes, there is that injunction. But as you suggest, I think it is different for pull requests, since it's unlikely that anyone would use a PR branch as a base for subsequent work.

For my PR branches, I rebase-to-master all the time (have done for years), and nobody's complained.

But we should ask the editors (@bterlson, @ljharb): when a PR's submitter resolves conflicts with master, do you have a preference between rebasing or merging? CONTRIBUTING.md doesn't say.

[ljharb]

Merge commits should be avoided; always rebase and force push a PR branch.

[brabalan]

Rebased and squashed.

[littledan]

Good to have this guidance; I've been debating for myself about this. Let's make sure to document this as our team's practice in CONTRIBUTING.md or someplace similar.

[jmdyck]

Let's make sure to document this as our team's practice in CONTRIBUTING.md or someplace similar.

See #1099.

[bterlson]

This looks good, thanks everyone for the detailed review! Sorry for being delinquent.