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: Merge FunctionAllocate and FunctionInitialize #1562

Merged
merged 5 commits into from Nov 14, 2019

Conversation

@jmdyck
Copy link
Collaborator

jmdyck commented May 31, 2019

FunctionAllocate and FunctionInitialize are always invoked in tandem. Moreover, the division between them seems somewhat arbitrary. So, merge the two operations into one, called OrdinaryFunctionCreate.

(Yeah, it's got 7 parameters, but I'm pretty sure we can eliminate one of them in a future refactoring.)

The second commit is the main one; the others are prep and cleanup.

(Web IDL and HTML do not reference FunctionAllocate or FunctionInitialize.)

spec.html Outdated
@@ -7454,8 +7445,7 @@ <h1>FunctionCreate ( _kind_, _ParameterList_, _Body_, _Scope_, _Strict_ [ , _pro
1. Set _prototype_ to the intrinsic object %FunctionPrototype%.
1. If _kind_ is not ~Normal~, let _allocKind_ be `"non-constructor"`.
1. Else, let _allocKind_ be `"normal"`.
1. Let _F_ be FunctionAllocate(_prototype_, _Strict_, _allocKind_).
1. Return FunctionInitialize(_F_, _kind_, _ParameterList_, _Body_, _Scope_).
1. Return ! OrdinaryFunctionCreate(_prototype_, _ParameterList_, _Body_, _allocKind_, _kind_, _Strict_, _Scope_).

This comment has been minimized.

Copy link
@ljharb

ljharb May 31, 2019

Member

Is there a reason you didn’t keep the same param order - prototype, strict, allocKind, and then the initialize args?

This comment has been minimized.

Copy link
@jmdyck

jmdyck May 31, 2019

Author Collaborator

The resulting order didn't make sense to me.

This comment has been minimized.

Copy link
@ljharb

ljharb May 31, 2019

Member

At the least, strict seems to me like it should come before body and parameter list, since it determines how it’s parsed.

This comment has been minimized.

Copy link
@jmdyck

jmdyck May 31, 2019

Author Collaborator

Maybe in the real world, but not in spec-world. In general, the spec can't even ask whether some code is strict until after it's been parsed. And anyhow, parsing is well in the past by the time this operation is called. A function's [[Strict]] value is only used to affect the creation of an "arguments" object, and how 'this' gets bound. (I think.)

(Moreover, #1563 eliminates the _Strict_ parameter, so its position in the parameter list may be a moot point.)

@ljharb ljharb requested review from zenparsing and tc39/ecma262-editors May 31, 2019
@jmdyck jmdyck force-pushed the jmdyck:OrdinaryFunctionCreate branch from 7cd5329 to 1dc95e2 Jun 17, 2019
@jmdyck

This comment has been minimized.

Copy link
Collaborator Author

jmdyck commented Jun 17, 2019

(force-pushed to resolve merge conflicts)

@jmdyck jmdyck force-pushed the jmdyck:OrdinaryFunctionCreate branch from 1dc95e2 to b284baf Jul 17, 2019
@jmdyck

This comment has been minimized.

Copy link
Collaborator Author

jmdyck commented Jul 17, 2019

(force-pushed to resolve merge conflicts again)

spec.html Outdated Show resolved Hide resolved
@ljharb
ljharb approved these changes Jul 17, 2019
@ljharb ljharb requested a review from tc39/ecma262-editors Jul 17, 2019
@jmdyck jmdyck force-pushed the jmdyck:OrdinaryFunctionCreate branch from b284baf to c28a7bb Sep 14, 2019
@jmdyck

This comment has been minimized.

Copy link
Collaborator Author

jmdyck commented Sep 14, 2019

(force-pushed to resolve merge conflicts /3)

@ljharb ljharb self-assigned this Sep 14, 2019
@ljharb ljharb requested review from syg and removed request for tc39/ecma262-editors and zenparsing Oct 9, 2019
@jmdyck jmdyck force-pushed the jmdyck:OrdinaryFunctionCreate branch from c28a7bb to a8cac86 Oct 14, 2019
@jmdyck

This comment has been minimized.

Copy link
Collaborator Author

jmdyck commented Oct 14, 2019

(force-pushed to resolve merge conflicts /4)

spec.html Outdated
1. Let _scope_ be _realmF_.[[GlobalEnv]].
1. Perform FunctionInitialize(_F_, ~Normal~, _parameters_, _body_, _scope_).
1. Let _F_ be ! OrdinaryFunctionCreate(_proto_, _parameters_, _body_, _kind_, ~Normal~, _scope_).

This comment has been minimized.

Copy link
@syg

syg Oct 16, 2019

Contributor

Nice cleanup here for _scope_.

spec.html Outdated Show resolved Hide resolved
spec.html Outdated
<h1>FunctionAllocate ( _functionPrototype_, _functionKind_ )</h1>
<p>The abstract operation FunctionAllocate requires the two arguments _functionPrototype_ and _functionKind_. FunctionAllocate performs the following steps:</p>
<emu-clause id="sec-ordinaryfunctioncreate" aoid="OrdinaryFunctionCreate" oldids="sec-functionallocate,sec-functioninitialize">
<h1>OrdinaryFunctionCreate ( _functionPrototype_, _ParameterList_, _Body_, _functionKind_, _kind_, _Scope_ )</h1>

This comment has been minimized.

Copy link
@syg

syg Oct 16, 2019

Contributor

It's unfortunate there's both a _kind_ and a _functionKind_. AFAICT the ~Method~ case of _kind_ doesn't do anything, and _kind_ is only used to determine the this scope lookup mode. I would prefer to pass that directly, i.e. a new _thisMode_ that's ~LexicalThis~ or ~NonLexicalThis~ and have the non-lexical case checked for strictness inside this AO.

@jmdyck WDYT?

This comment has been minimized.

Copy link
@syg

syg Oct 16, 2019

Contributor

Ah, currently _kind_ is still used to determine the allocation kind for non-constructibles. It's all rather circuitous logic in there. Do you plan to layer this on top of #1546, after which my suggestion above makes more sense?

This comment has been minimized.

Copy link
@jmdyck

jmdyck Oct 17, 2019

Author Collaborator

Do you plan to layer this on top of #1546?

Well, if #1546 is accepted, then I'll resolve the resulting merge conflicts here, if that's what you mean.

This comment has been minimized.

Copy link
@jmdyck

jmdyck Oct 24, 2019

Author Collaborator

I would prefer to pass that directly, i.e. a new _thisMode_ that's ~LexicalThis~ or ~NonLexicalThis~

Done! (modulo naming)

and have the non-lexical case checked for strictness inside this AO.

I wasn't sure what you meant by this.

jmdyck added a commit to jmdyck/ecma262 that referenced this pull request Oct 24, 2019
The only effect of OrdinaryFunctionCreate's _kind_ parameter
is on the [[ThisMode]] of the new function.
And although _kind_ has three possible values (~Normal~, ~Method~, ~Arrow~),
the only thing that matters is whether or not it's ~Arrow~.
So replace it with parameter _thisMode_ that has two possible values:
~lexical-this~ and ~nonlexical-this~.

Suggested by @syg in:
tc39#1562 (comment)
@jmdyck jmdyck force-pushed the jmdyck:OrdinaryFunctionCreate branch from a8cac86 to b30238b Oct 24, 2019
@jmdyck

This comment has been minimized.

Copy link
Collaborator Author

jmdyck commented Oct 24, 2019

Force-pushed to resolve merge conflicts, and also to add 2 commits.

spec.html Outdated Show resolved Hide resolved
@ljharb ljharb requested a review from syg Oct 24, 2019
jmdyck added a commit to jmdyck/ecma262 that referenced this pull request Oct 24, 2019
The only effect of OrdinaryFunctionCreate's _kind_ parameter
is on the [[ThisMode]] of the new function.
And although _kind_ has three possible values (~Normal~, ~Method~, ~Arrow~),
the only thing that matters is whether or not it's ~Arrow~.
So replace it with parameter _thisMode_ that has two possible values:
~lexical-this~ and ~non-lexical-this~.

Suggested by @syg in:
tc39#1562 (comment)
@jmdyck jmdyck force-pushed the jmdyck:OrdinaryFunctionCreate branch from b30238b to 324cc1c Oct 24, 2019
Copy link
Member

michaelficarra left a comment

LGTM. This will probably have lots of variable renaming conflicts with #1477.

@michaelficarra michaelficarra requested a review from bakkot Nov 1, 2019
@syg
syg approved these changes Nov 13, 2019
Copy link
Contributor

syg left a comment

lgtm. Nice cleanup!

spec.html Outdated Show resolved Hide resolved
@bakkot
bakkot approved these changes Nov 13, 2019
jmdyck added 5 commits May 30, 2019
FunctionAllocate() always returns a function object
whose [[Realm]] is the current Realm Record.
FunctionAllocate and FunctionInitialize are always invoked in tandem.
Moreover, the division between them seems somewhat arbitrary.
So, merge the two operations into one, called OrdinaryFunctionCreate.
(This seems like a sensible order to me, at least.)
... specifically:
- FunctionCreate
- GeneratorFunctionCreate
- AsyncGeneratorFunctionCreate
- AsyncFunctionCreate

After the refactorings of PR #1546 and the prior commits in this PR,
these have slimmed down to very thin wrappers around OrdinaryFunctionCreate.
The only thing they hide is the choice of prototype;
all their parameters are simply passed through.
I don't think they provide a useful abstraction layer any more.

In fact, I think they're kind of an obstacle.
Any change in the interface of OrdinaryFunctionCreate
would require a corresponding change to each of these operations,
an unnecessary complication and possible inhibitor.

Downstream specs:
- ES Intl doesn't use any of these operations.
- WebIDL doesn't use any of these operations.
- HTML has one call to FunctionCreate.
The only effect of OrdinaryFunctionCreate's _kind_ parameter
is on the [[ThisMode]] of the new function.
And although _kind_ has three possible values (~Normal~, ~Method~, ~Arrow~),
the only thing that matters is whether or not it's ~Arrow~.
So replace it with parameter _thisMode_ that has two possible values:
~lexical-this~ and ~non-lexical-this~.

Suggested in: #1562 (comment)
@ljharb ljharb force-pushed the jmdyck:OrdinaryFunctionCreate branch from 324cc1c to 3899f59 Nov 14, 2019
@ljharb ljharb merged commit 3899f59 into tc39:master Nov 14, 2019
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
netlify/ecma262-snapshots/deploy-preview Deploy preview ready!
Details
@jmdyck

This comment has been minimized.

Copy link
Collaborator Author

jmdyck commented Nov 14, 2019

Yay! Thanks for the merge.

@jmdyck jmdyck deleted the jmdyck:OrdinaryFunctionCreate branch Nov 14, 2019
jmdyck added a commit to jmdyck/html that referenced this pull request Nov 15, 2019
In the ECMAScript spec, the recently merged [PR whatwg#1562](tc39/ecma262#1562)
replaced the `FunctionCreate` operation (and some others)
with the new operation `OrdinaryFunctionCreate`.

This commit makes the necessary changes to HTML's only call to `FunctionCreate`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.