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.

Sign up for GitHub

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

Jump to bottom

Editorial: Simplify CreateDynamicFunction #2348

Merged
merged 1 commit into from
Apr 2, 2021
Merged

Editorial: Simplify CreateDynamicFunction #2348

merged 1 commit into from
Apr 2, 2021

Conversation

jmdyck
Copy link
Collaborator

@jmdyck jmdyck commented Mar 16, 2021

Formerly, CreateDynamicFunction would parse the text for the parameters and body separately, so we didn't have a complete function decl/expr, so the Early Error rules for the function decl/expr production didn't apply automatically, so we had to apply them "manually".

Instead, parse the text of a complete function expr (which text already exists in _sourceText_!), so that all the relevant Early Error rules apply automatically.

I left a Note for the benefit of anyone who wonders where all the error-checking went, but it might be deemed unnecessary. (No other call to ParseText is accompanied by such a Note.)

This change has a side-benefit in that it will allow a simplification of the definition of "function code" (and strict function code). Currently, it's complicated by this one case (set of cases) where the Parameters and Body (and BindingIdentifier) of a function are not simply the children of a Declaration or Expression. This change will eliminate such cases.

michaelficarra
michaelficarra reviewed Mar 31, 2021
View reviewed changes
spec.html Outdated
1. If _strict_ is *true*, then
1. If BoundNames of _parameters_ contains any duplicate elements, throw a *SyntaxError* exception.
1. Let _expr_ be ParseText(_sourceText_, _exprSym_).
1. NOTE: ParseText enforces all relevant Early Error rules, including those associated with _exprSym_.
Copy link
Member

Choose a reason for hiding this comment

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

Yeah I don't think we need this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added a commit to remove the NOTE step.

michaelficarra
michaelficarra reviewed Mar 31, 2021
View reviewed changes
spec.html Outdated
@@ -24986,21 +24986,21 @@ <h1>CreateDynamicFunction ( _constructor_, _newTarget_, _kind_, _args_ )</h1>
1. Perform ? HostEnsureCanCompileStrings(_callerRealm_, _calleeRealm_).
1. If _newTarget_ is *undefined*, set _newTarget_ to _constructor_.
1. If _kind_ is ~normal~, then
1. Let _goal_ be the grammar symbol |FunctionBody[~Yield, ~Await]|.
1. Let _parameterGoal_ be the grammar symbol |FormalParameters[~Yield, ~Await]|.
1. Let _exprSym_ be the grammar symbol |FunctionExpression|.
Copy link
Member

Choose a reason for hiding this comment

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

Might be easier to use a table for these?

Copy link
Contributor

Choose a reason for hiding this comment

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

Eh, I'm pretty okay with the existing if-else cascade.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think a table for this would be fairly wide. (Its width would be determined by the AsyncGenerator row.)

Copy link
Contributor

Choose a reason for hiding this comment

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

We should perhaps go the other way and inline Table 51 as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I've got another PR planned that will take care of that.

Copy link
Member

Choose a reason for hiding this comment

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

I still kinda prefer the tables for these cases. Maybe talk about it in the editor call?

michaelficarra
michaelficarra approved these changes Mar 31, 2021
View reviewed changes
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 is great!

@ljharb ljharb requested review from syg, bakkot and a team March 31, 2021 18:50
syg
syg approved these changes Mar 31, 2021
View reviewed changes
spec.html Outdated
@@ -24986,21 +24986,21 @@ <h1>CreateDynamicFunction ( _constructor_, _newTarget_, _kind_, _args_ )</h1>
1. Perform ? HostEnsureCanCompileStrings(_callerRealm_, _calleeRealm_).
1. If _newTarget_ is *undefined*, set _newTarget_ to _constructor_.
1. If _kind_ is ~normal~, then
1. Let _goal_ be the grammar symbol |FunctionBody[~Yield, ~Await]|.
1. Let _parameterGoal_ be the grammar symbol |FormalParameters[~Yield, ~Await]|.
1. Let _exprSym_ be the grammar symbol |FunctionExpression|.
Copy link
Contributor

Choose a reason for hiding this comment

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

Eh, I'm pretty okay with the existing if-else cascade.

bakkot
bakkot approved these changes Apr 2, 2021
View reviewed changes
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.

This is greatly improved. I wish I knew why it was originally written with the more convoluted style - I'm worried there is some reason the two aren't equivalent, which necessitates the original complexity, but they really do appear to be the same.

@bakkot bakkot added the ready to merge Editors believe this PR needs no further reviews, and is ready to land. label Apr 2, 2021
@jmdyck @ljharb
Editorial: Simplify CreateDynamicFunction (tc39#2348)
bbaac2f 
Formerly, CreateDynamicFunction would parse the text for the parameters and body *separately*, so we didn't have a complete function decl/expr, so the Early Error rules for the function decl/expr production didn't apply automatically, so we had to apply them "manually".

Instead, parse the text of a complete function expr (which text already exists in _sourceText_!), so that all the relevant Early Error rules apply automatically.
@ljharb ljharb force-pushed the CreateDynamicFunction_parse branch from c162d7a to bbaac2f Compare April 2, 2021 20:05
@ljharb ljharb merged commit bbaac2f into tc39:master Apr 2, 2021
@jmdyck
Copy link
Collaborator Author

jmdyck commented Apr 2, 2021

I wish I knew why it was originally written with the more convoluted style - I'm worried there is some reason the two aren't equivalent, which necessitates the original complexity, but they really do appear to be the same.

For a call to the Function constructor, parsing the parameter-text and body-text separately goes all the way back to the first edition, but that was a reasonable approach then and didn't result in extra complexity. The complexity gradually accreted over successive editions. My guess is, nobody thought to question the original approach.

@michaelficarra michaelficarra added the editor call to be discussed in the next editor call label Apr 2, 2021
@jmdyck jmdyck deleted the CreateDynamicFunction_parse branch April 3, 2021 01:02
jmdyck added a commit to jmdyck/ecma262 that referenced this pull request Apr 3, 2021
@jmdyck
Editorial: Inline "Dynamic Function SourceText Prefixes" table
b35fe87 
... into the if-else cascade in CreateDynamicFunction,
as @bakkot suggested:
tc39#2348 (comment)
@jmdyck jmdyck mentioned this pull request Apr 3, 2021
Merged
@bakkot bakkot mentioned this pull request Apr 5, 2021
Closed
@erights erights mentioned this pull request Apr 5, 2021
Closed
@bakkot bakkot mentioned this pull request Apr 6, 2021
Merged
@michaelficarra michaelficarra removed the editor call to be discussed in the next editor call label Apr 7, 2021
jmdyck added a commit to jmdyck/ecma262 that referenced this pull request Jul 18, 2021
@jmdyck
Editorial: Inline "Dynamic Function SourceText Prefixes" table
ac675ad 
... into the if-else cascade in CreateDynamicFunction,
as @bakkot suggested:
tc39#2348 (comment)
ljharb pushed a commit to jmdyck/ecma262 that referenced this pull request Jul 18, 2021
@jmdyck @ljharb
Editorial: Inline "Dynamic Function SourceText Prefixes" table (tc39#…
6ca50dc 
…2367)

... into the if-else cascade in CreateDynamicFunction, suggested in tc39#2348 (comment)
mathiasbynens pushed a commit to mathiasbynens/ecma262 that referenced this pull request Oct 18, 2021
@jmdyck @mathiasbynens
Editorial: Inline "Dynamic Function SourceText Prefixes" table (tc39#…
3abf56e 
…2367)

... into the if-else cascade in CreateDynamicFunction, suggested in tc39#2348 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaelficarra michaelficarra michaelficarra approved these changes

@bakkot bakkot bakkot approved these changes

@syg syg syg approved these changes

Assignees
No one assigned
Labels
editorial change ready to merge Editors believe this PR needs no further reviews, and is ready to land.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jmdyck @syg @michaelficarra @bakkot @ljharb