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

Refactor await/yield production parameter tracking #10956

Merged
merged 6 commits into from Feb 9, 2020

Conversation

JLHwung
Copy link
Contributor

@JLHwung JLHwung commented Jan 3, 2020

Q                       A
Fixed Issues? Class Field Initializers should not allow YieldExpression
Patch: Bug Fix? Yes
Tests Added + Pass? Yes
License MIT

This PR includes commits from #10947, I will rebase once that PR is merged.

The current parser scoping is meant to track declarations and bindings, it serves well for the purpose of declaration scope. It is also mixed with [Await] and [Yield] production parameter tracks, historically it was okay because when [Await]/[Yield] are changed, it must be when a function declaration/expression is parsed, where a declaration scope is also created.
However, class fields initializers is an exception where a declaration scope is not created but production parameter is changed. Fixing this issue in parser scoping is cumbersome and error-prone: https://github.com/babel/babel/pull/10946/files#diff-a9acee4ca26b68744fe0a1cb5aa03e34R60, therefore I extract the SCOPE_ASYNC and SCOPE_GENERATOR tracking into a dedicate stack fashioned handler, the code is now more intuitive.

I should point out the idea comes out from an offline discussion with @nicolo-ribaudo and I am really appreciated.

Note that after this PR is merged, ultimately isAwaitAllowed

isAwaitAllowed(): boolean {
if (this.scope.inFunction) return this.scope.inAsync;
if (this.options.allowAwaitOutsideFunction) return true;
if (this.hasPlugin("topLevelAwait")) return this.inModule;
return false;
}

should be simplified to

return (this.param.hasAwait || this.options.allowAwaitOutsideFunction)

which is a faithful implementation of the spec text

BindingIdentifier: await
It is a Syntax Error if this production has an [Await] parameter

But we still have trouble on tracking arrow / async arrow expressions. We are try to recover such error in awaitPos when parsing arrow expressions. Unless we are parsing the ambiguous ArrayExpression/ParenthesizedExpression (like what v8 does in https://docs.google.com/document/d/1FAvEp9EUK-G8kHfDIEo_385Hs2SUBCYbJ5H-NnLvq8M/edit ) and record these errors to be fulfilled or rejected later when it becomes unambiguous, we still relies on the scope.inFunction guard.

@JLHwung JLHwung added PR: Spec Compliance 👓 A type of pull request used for our changelog categories pkg: parser labels Jan 3, 2020
@@ -1573,9 +1576,12 @@ export default class StatementParser extends ExpressionParser {
node: N.ClassPrivateProperty,
): N.ClassPrivateProperty {
this.scope.enter(SCOPE_CLASS | SCOPE_SUPER);
// [In] production parameter is tracked in parseMaybeAssign
this.param.enter(PARAM_);
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 am not sure if I can still understand what I have written at https://github.com/babel/babel/pull/10946/files#diff-a9acee4ca26b68744fe0a1cb5aa03e34R60 three months later.

But this change should be more intuitive as it is copied from the spec:

FieldDefinition[Yield, Await]:
  ClassElementName[?Yield, ?Await]  Initializer[In, ~Yield, ~Await]opt

@JLHwung JLHwung force-pushed the refactor-await-yield-tracking branch 2 times, most recently from 2e1be8d to 98ac14f Compare January 3, 2020 23:42
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

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

Apart from some nits, this PR looks good!

What do you think about also moving [+Return] to the new class rather than using this.scope.inFunction? It provides way fewer benefits, but at least we are consistent.

Also, I think that in Babel 8 allowAwaitOutsideFunction and allowReturnOutsideFunction should simply set the corresponding production parameter.

}
this.scope.enter(scopeFlags);
this.scope.enter(SCOPE_PROGRAM);
this.param.enter(paramFlags);
Copy link
Member

Choose a reason for hiding this comment

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

I don't like this name, since param could be anything. What about this.prodp, or better this.prodParam?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well if we don't require prodParam to be as easy to speak as param, I am good with prodParam which is still shorter than the daunting this.productionParameter.

} from "../util/scopeflags";
import {
PARAM_AWAIT,
PARAM_,
Copy link
Member

Choose a reason for hiding this comment

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

Nit: what about PARAM_DEFAULT, or just PARAM? The trailing underscore looks... weird.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well the spec does have [Default] parameter so PARAM_DEFAULT will be confusing as we don't check [Default] yet.

I have come up with names like PARAM_NONE, PARAM_OTHER when authoring but I am afraid they maybe used at the spec one day.

I will go with PARAM then.

@JLHwung
Copy link
Contributor Author

JLHwung commented Jan 4, 2020

What do you think about also moving [+Return] to the new class rather than using this.scope.inFunction? It provides way fewer benefits, but at least we are consistent.

Good idea, I have include it in this PR. Not my usual move but the change is minimal.

I think that in Babel 8 allowAwaitOutsideFunction and allowReturnOutsideFunction should simply set the corresponding production parameter.

Yeah, we are on the same page. I would like to interpret these options as

ScriptBody:
  StatementList[~Yield, +Await, +Return]

ModuleItem:
  StatementListItem[~Yield, +Await, +Return]

Since the all the TypeScript declaration grammars, including NamespaceDeclaration is in the same level as Statement. (Link: https://github.com/microsoft/TypeScript/blob/master/doc/spec.md#112-scripts) allow*OutsideFunction should not change the production parameter of these nodes, though technically TypeScript Spec does not give any specifications of that.

By doing so we can throw

namespace N {
  await 42
}
declare enum E = { P = await 42 };

when allowAwaitOutsideFunction: true, which should be done in Babel 8.

@JLHwung JLHwung force-pushed the refactor-await-yield-tracking branch 2 times, most recently from 5f61655 to 43df227 Compare January 4, 2020 13:55
@@ -1158,7 +1170,7 @@ export default class ExpressionParser extends LValParser {
this.next();
meta = this.createIdentifier(meta, "function");

if (this.scope.inGenerator && this.eat(tt.dot)) {
if (this.prodParam.hasYield && this.eat(tt.dot)) {
Copy link

Choose a reason for hiding this comment

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

2x member access is expensive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, but this.prodParam.hasYield is still faster than scope.inGenerator because we don't have to walkup the scope stacks to find a var scope:

currentVarScope(): IScope {

Copy link

Choose a reason for hiding this comment

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

What about a mutual parser flag instead? state.flag |= HasYield and then
`if (state.flag & HasYield) ...``

Copy link
Member

Choose a reason for hiding this comment

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

Do you think that it would make any difference? I have always assumed that we have way bigger perf problems in our parser, and that those optimizations would only be noticeable after we resolve things like "Every method access has two walk different levels of the prototype chain" and "Almost every function is megamorphic" and "Our AST objects continuously change their shape".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this.prodParam.hasYield is implemented by parser flags. V8 will inline these Get accessors eventually.

The scope stack is used to track nested production params. It is equivalent to do things like

const oldHasYield = this.state.hasYield
this.state.hasYield = true;
parseFunctionBody()
this.state.hasYield = oldHasYield;

The only difference is that the hasYield above is stored implicitly in stack while this.prodParams.hasYield stored explicitly in heap.

Copy link

Choose a reason for hiding this comment

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

@nicolo-ribaudo If you see the whole picture, maybe no :) There are too many bottle necks in this parser such as the lexer, tokens itself etc.

Why not develop a new parser from bottom up? It shouldn't take more than 10 days (estimated) to replicate current Babel parser with working plugins.

Copy link
Member

Choose a reason for hiding this comment

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

If you see the even bigger picture, then it would probably be more valuable to re-implement @babel/traverse which is where Babel spends most of its time 😂

Copy link

Choose a reason for hiding this comment

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

Just looked at @babel/traverse. Should be easy :) Just get rid of the prototype and use functions instead, plus use bitmasks to handle the flow.
The findParent function is one of the root causes I guess for why it's slow :)

@JLHwung JLHwung force-pushed the refactor-await-yield-tracking branch 2 times, most recently from fb67126 to e36aeb3 Compare January 16, 2020 18:38
@nicolo-ribaudo nicolo-ribaudo merged commit 3852969 into babel:master Feb 9, 2020
@nicolo-ribaudo nicolo-ribaudo deleted the refactor-await-yield-tracking branch February 9, 2020 14:31
rajasekarm pushed a commit to rajasekarm/babel that referenced this pull request Feb 17, 2020
* test: add test fixtures

* refactor: track AWAIT and YIELD in separate handler

* fix flow errors

* add flow type annotation to production-parameter

* address review comments

* refactor: track [Return] parameter
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label May 11, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: parser PR: Spec Compliance 👓 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants