Skip to content

Restore traversal context after enter / traverse#13813

Merged
nicolo-ribaudo merged 7 commits intobabel:mainfrom
JLHwung:fix-13801
Oct 11, 2021
Merged

Restore traversal context after enter / traverse#13813
nicolo-ribaudo merged 7 commits intobabel:mainfrom
JLHwung:fix-13801

Conversation

@JLHwung
Copy link
Copy Markdown
Contributor

@JLHwung JLHwung commented Oct 4, 2021 •
edited
Loading

Q                       A
Fixed Issues? Fixes #13801
Patch: Bug Fix? Y
Major: Breaking Change? Maybe.
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

In this PR we restores the context after call("enter") and traverse.node. The current AST operations we provided did not guarantee that the traversal context is perserved, let alone users may even change the associated context. The traversal context is crucial to sub-traverse in order to isolate visitors / traverse states from the root traverse.

See also #13801 (comment) for how the leaked traversal context causes confusing errors.

I add a benchmark case to access the performance impact of this PR. The benchmark is a worst case scenario because context is changed in every node visitor. Compared to 7.15.4, Babel traverse in this PR is 10% slower. In the real world most context change happen in sub traverse, preserving context will ensure global visitors are not invoked in sub traverse so the real world performance should be better than this.

Benchmark Results

baseline 256 empty statements: 6_354 ops/sec ±1.76% (0.157ms)
baseline 512 empty statements: 3_287 ops/sec ±0.83% (0.304ms)
baseline 1024 empty statements: 1_615 ops/sec ±0.92% (0.619ms)
baseline 2048 empty statements: 795 ops/sec ±1.91% (1.259ms)
baseline mutating context 256 empty statements: 6_570 ops/sec ±0.94% (0.152ms)
baseline mutating context 512 empty statements: 3_237 ops/sec ±1.4% (0.309ms)
baseline mutating context 1024 empty statements: 1_635 ops/sec ±0.91% (0.612ms)
baseline mutating context 2048 empty statements: 811 ops/sec ±1.16% (1.233ms)
current 256 empty statements: 5_151 ops/sec ±19.24% (0.194ms)
current 512 empty statements: 2_843 ops/sec ±1.74% (0.352ms)
current 1024 empty statements: 1_424 ops/sec ±0.96% (0.702ms)
current 2048 empty statements: 706 ops/sec ±2.6% (1.417ms)
current mutating context 256 empty statements: 6_270 ops/sec ±0.62% (0.159ms)
current mutating context 512 empty statements: 3_121 ops/sec ±0.98% (0.32ms)
current mutating context 1024 empty statements: 1_553 ops/sec ±0.91% (0.644ms)
current mutating context 2048 empty statements: 758 ops/sec ±1.76% (1.318ms)

@JLHwung JLHwung added PR: Bug Fix 🐛 A type of pull request used for our changelog categories pkg: traverse labels Oct 4, 2021
@babel-bot
Copy link
Copy Markdown
Collaborator

babel-bot commented Oct 4, 2021 •
edited
Loading

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/49081/

@codesandbox-ci
Copy link
Copy Markdown

codesandbox-ci bot commented Oct 4, 2021 •
edited
Loading

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit f11e12d:

Sandbox Source
babel-repl-custom-plugin Configuration
babel-plugin-multi-config Configuration

JLHwung
JLHwung commented Oct 4, 2021
View reviewed changes
packages/babel-traverse/src/path/context.ts
// a requeued node (e.g. by .replaceWith()) that is then marked
// with .skip().
if (this.shouldSkip || this.call("enter") || this.shouldSkip) {
if (this.shouldSkip || this.call("enter")) {
Copy link
Copy Markdown
Contributor Author

@JLHwung JLHwung Oct 4, 2021

Choose a reason for hiding this comment

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

If this.enter("enter") is false, either fns is empty (no visitors), or all the visitors does not change the traverse flags. If fns is empty, then this.shouldSkip would not be changed at all. If all the visitors do not change traversal flags, then this.shouldSkip should equals to this.shouldSkip before enter hook is called. Therefore the second this.shouldSkip check is redundant.

@nicolo-ribaudo
Copy link
Copy Markdown
Member

nicolo-ribaudo commented Oct 4, 2021

Does this also fix #12631?

@JLHwung
Copy link
Copy Markdown
Contributor Author

JLHwung commented Oct 5, 2021

Does this also fix #12631?

Thanks for the reminder. I check out the reproduction repo: The traversal context does change after .call("enter"). After I apply the fix to the next-bundled babel.js, the context is correctly restored, but the error d(...) is not a function can still be reproduced.

danieltroger
danieltroger reviewed Oct 6, 2021
View reviewed changes
...ages/babel-plugin-proposal-async-generator-functions/test/fixtures/regression/13801/input.js
@@ -0,0 +1,11 @@
async function main() {
async () => {
Copy link
Copy Markdown

@danieltroger danieltroger Oct 6, 2021

Choose a reason for hiding this comment

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

Is it intentional that this function is no longer invoked?

Copy link
Copy Markdown
Contributor Author

@JLHwung JLHwung Oct 6, 2021

Choose a reason for hiding this comment

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

Kind of. Personally I prefer the minimal AST structure that can reproduce the bug. In this case we are not testing the runtime semantics so the simpler the input code is, the easier to reason about what is going on. You can verify that the original code example also works on the PR REPL.

Copy link
Copy Markdown

@danieltroger danieltroger Oct 6, 2021

Choose a reason for hiding this comment

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

Very epic thanks for explaining

@JLHwung JLHwung marked this pull request as ready for review October 6, 2021 15:01
@nicolo-ribaudo
Copy link
Copy Markdown
Member

nicolo-ribaudo commented Oct 6, 2021

Could we also benchmark, for example, compiling https://github.com/babel/babel/blob/main/packages/babel-parser/src/parser/expression.js with @babel/preset-env + @babel/preset-flow to see the real world perf diff?

@JLHwung
Copy link
Copy Markdown
Contributor Author

JLHwung commented Oct 7, 2021

I added a real-world benchmark, the current PR is 3% (not statistically significant) slower than main branch. The minimal sample size for benchmark is 100.

baseline babel-parser-expression.txt: 5.93 ops/sec ±2.97% (169ms)
current babel-parser-expression.txt: 5.78 ops/sec ±3.51% (173ms)

@nicolo-ribaudo nicolo-ribaudo requested a review from hzoo October 8, 2021 20:03
@nicolo-ribaudo nicolo-ribaudo merged commit 21eeb8e into babel:main Oct 11, 2021
@nicolo-ribaudo nicolo-ribaudo deleted the fix-13801 branch October 11, 2021 17:04
@nicolo-ribaudo nicolo-ribaudo mentioned this pull request Oct 11, 2021
Merged
@snyk-bot snyk-bot mentioned this pull request Dec 14, 2021
Closed
@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 Jan 11, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@existentialism existentialism existentialism approved these changes

@nicolo-ribaudo nicolo-ribaudo nicolo-ribaudo approved these changes

@hzoo hzoo Awaiting requested review from hzoo

+1 more reviewer

@danieltroger danieltroger danieltroger left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: traverse PR: Bug Fix 🐛 A type of pull request used for our changelog categories

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug]: babel crashes when destructuring, after using for await inside of async IIFE

5 participants

@JLHwung @babel-bot @nicolo-ribaudo @existentialism @danieltroger