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
Refactor await/yield production parameter tracking #10956
Conversation
@@ -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_); |
There was a problem hiding this comment.
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
2e1be8d
to
98ac14f
Compare
There was a problem hiding this 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); |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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_, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Good idea, I have include it in this PR. Not my usual move but the change is minimal.
Yeah, we are on the same page. I would like to interpret these options as
Since the all the TypeScript declaration grammars, including By doing so we can throw namespace N {
await 42
}
declare enum E = { P = await 42 }; when |
5f61655
to
43df227
Compare
@@ -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)) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
babel/packages/babel-parser/src/util/scope.js
Line 209 in 09cb427
currentVarScope(): IScope { |
There was a problem hiding this comment.
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) ...``
There was a problem hiding this comment.
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".
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 😂
There was a problem hiding this comment.
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 :)
fb67126
to
e36aeb3
Compare
e36aeb3
to
8102648
Compare
* 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
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
andSCOPE_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
babel/packages/babel-parser/src/parser/expression.js
Lines 2186 to 2191 in 9f832c2
should be simplified to
which is a faithful implementation of the spec text
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 ambiguousArrayExpression/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 thescope.inFunction
guard.