Add `attachComment` parser option to disable comment attachment #13229

JLHwung · 2021-04-29T14:17:26Z

Q	A
Minor: New Feature?	Yes
Tests Added + Pass?	Yes
Documentation PR Link	babel/website#2555
Any Dependency Changes?
License	MIT

This PR introduces attachComment: boolean option to @babel/parser. It is true by default, when it is false, Babel parser will skip the comment attachment, which means comments will not be attached to adjacent AST nodes as {leading|inner|trailing}Comments. All the comments are still accessible from the root AST node File.

The option name is borrowed from espree, though espree has removed this option.

On the benchmark many-empty-statements, it achieves 30% performance boost compared to Babel parser 7.13.16

(30% vs Babel parser 7.13.16)

baseline 256 empty statement: 33440 ops/sec ±1.4% (0.03ms)
baseline 512 empty statement: 17045 ops/sec ±1.65% (0.059ms)
baseline 1024 empty statement: 8319 ops/sec ±1.68% (0.12ms)
baseline 2048 empty statement: 3666 ops/sec ±2.19% (0.273ms)
current + attachComment: false 256 empty statement: 44065 ops/sec ±0.93% (0.023ms)
current + attachComment: false 512 empty statement: 20824 ops/sec ±3.15% (0.048ms)
current + attachComment: false 1024 empty statement: 10675 ops/sec ±3.25% (0.094ms)
current + attachComment: false 2048 empty statement: 4409 ops/sec ±6.47% (0.227ms)

The comment attachment is crucial when we print AST back to sources. However, if users are not gonna print AST, disabling attachComment can boost the parser performance. For example, the @babel/eslint-parser can set attachComment to false since the linter does not care comment attachment. We even delete such comments in @babel/eslint-parser.

This PR includes commits from #13227.

Update: I have rebased after #13521 is merged. Here are the new benchmark results vs the latest main.

No comments: (no improvements)

baseline 256 empty statement: 44_500 ops/sec ±1.81% (0.022ms)
baseline 512 empty statement: 23_899 ops/sec ±1.03% (0.042ms)
baseline 1024 empty statement: 11_913 ops/sec ±0.83% (0.084ms)
baseline 2048 empty statement: 6_121 ops/sec ±1.02% (0.163ms)
current + attachComment: false 256 empty statement: 46_872 ops/sec ±2.36% (0.021ms)
current + attachComment: false 512 empty statement: 24_703 ops/sec ±1.78% (0.04ms)
current + attachComment: false 1024 empty statement: 12_327 ops/sec ±1.43% (0.081ms)
current + attachComment: false 2048 empty statement: 6_375 ops/sec ±1.19% (0.157ms)

This is expected because when there are no comments, the new attachment algorithm exits early when state.commentStacks is empty.

Many leading + trailing comments: (30% improvements)

baseline 256 leading comments + 255 trailing comments: 4_141 ops/sec ±71.42% (0.241ms)
baseline 512 leading comments + 511 trailing comments: 3_297 ops/sec ±0.26% (0.303ms)
baseline 1024 leading comments + 1023 trailing comments: 1_591 ops/sec ±0.2% (0.628ms)
baseline 2048 leading comments + 2047 trailing comments: 722 ops/sec ±0.92% (1.386ms)
current + attachComment: false 256 leading comments + 255 trailing comments: 6_088 ops/sec ±59.8% (0.164ms)
current + attachComment: false 512 leading comments + 511 trailing comments: 4_423 ops/sec ±0.5% (0.226ms)
current + attachComment: false 1024 leading comments + 1023 trailing comments: 2_118 ops/sec ±1.47% (0.472ms)
current + attachComment: false 2048 leading comments + 2047 trailing comments: 1_064 ops/sec ±0.87% (0.94ms)

When attachComment is false, we skip creating comment whitespaces and the comment processing, which results to 30% performance boost.

JLHwung · 2021-04-29T14:19:25Z

packages/babel-parser/test/attachComment-false.js

+    options.attachComment = false;
+    return parseExpression(input, options);
+  },
+);


We run parseExpression(#, { attachComment: false }) on expressions fixture as smoke tests. It is faster than running over the complete parser tests.

codesandbox-ci · 2021-04-29T14:22:14Z

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 24b79c8:

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

babel-bot · 2021-04-29T14:22:31Z

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

kaicataldo

Nice! Sorry that this is a bit nitpicky, but thoughts on calling the option attachComments or comentAttachment (to match errorRecovery) to make it clear we're dealing with all comments? Otherwise, this LGTM!

...arser/test/fixtures/comments/attachComment-false/array-expression-trailing-comma/output.json

hzoo

assume it's attachComment since it's named that in esprima? Ok with Kai's suggestion, just need to doc it!

nicolo-ribaudo · 2021-04-29T16:45:46Z

In the future we might want to expand this to be boolean | (commentText: string) => boolean, so that you could for example only attach #__PURE__ annotations or similar things.

hzoo · 2021-04-29T17:16:11Z

Yeah acorn has onComment right https://github.com/acornjs/acorn/blob/7deba41118d6384a2c498c61176b3cf434f69590/acorn/src/options.js#L56-L66

JLHwung · 2021-04-29T19:34:04Z

packages/babel-parser/benchmark/many-empty-statements/attachComment-false.bench.mjs

+function benchCases(name, implementation, options) {
+  for (const length of [256, 512, 1024, 2048]) {
+    suite.add(`${name} ${length} empty statement`, () => {
+      implementation.parse(";".repeat(length), options);


The performance of many-empty-statements is largely depended on Node creation/finalization and the complexity of tokenizer next(). I make it a folder as we can add more benchmark cases here.

aminya · 2021-05-11T07:29:47Z

I wanted to say that I support this option as it is useful in processing the comments. I registered the latest Espree with this feature in espree-attachcomment

I used it in jsdoc2flow package. If Babel adds the support for this feature, I will consider porting my jsdoc to flow compiler to Babel, which also makes it easier for me to make a "jsdoc2typescript" package.

nicolo-ribaudo · 2021-05-11T08:48:25Z

@aminya @babel/parser already attaches comments by default. This option is to make it possible to not attach comments!

aminya · 2021-05-12T00:42:48Z

Oh, my bad. The title was a bit confusing.

kaicataldo

🎉

nicolo-ribaudo · 2021-07-02T22:54:21Z

This probably needs to be redone after #13521 is merged.

nicolo-ribaudo · 2021-08-03T21:58:32Z

@JLHwung Could you rebase?

JLHwung added PR: New Feature 🚀 A type of pull request used for our changelog categories pkg: parser labels Apr 29, 2021

JLHwung commented Apr 29, 2021

View reviewed changes

kaicataldo reviewed Apr 29, 2021

View reviewed changes

hzoo reviewed Apr 29, 2021

View reviewed changes

...arser/test/fixtures/comments/attachComment-false/array-expression-trailing-comma/output.json Outdated Show resolved Hide resolved

hzoo approved these changes Apr 29, 2021

View reviewed changes

JLHwung commented Apr 29, 2021

View reviewed changes

nicolo-ribaudo added this to the v7.15.0 milestone Apr 29, 2021

JLHwung force-pushed the parser-attach-comment-option branch from 6c0bcd4 to 0a37178 Compare April 30, 2021 01:49

jridgewell approved these changes May 11, 2021

View reviewed changes

nicolo-ribaudo added the PR: Ready to be Merged A pull request with already two approvals, but waiting for the next minor release label May 11, 2021

JLHwung changed the title ~~Add attachComment parser option~~ Add attachComment parser option to allow optionally disable comment attachment May 12, 2021

nicolo-ribaudo added the PR: Needs Docs label May 12, 2021

kaicataldo approved these changes May 17, 2021

View reviewed changes

JLHwung force-pushed the parser-attach-comment-option branch from 0a37178 to 5792805 Compare May 17, 2021 17:29

JLHwung force-pushed the parser-attach-comment-option branch 2 times, most recently from 49a120f to 674b637 Compare July 7, 2021 18:56

JLHwung mentioned this pull request Jul 14, 2021

docs: add attachComment babel/website#2555

Merged

JLHwung removed the PR: Needs Docs label Jul 14, 2021

JLHwung force-pushed the parser-attach-comment-option branch from 674b637 to 752fc2b Compare August 3, 2021 22:11

feat: implement attachComment: boolean

ed9324c

JLHwung added 2 commits August 4, 2021 16:46

hoist generated input from benchmark task

93839e4

nit: ensure end is assigned after start

24b79c8

nicolo-ribaudo force-pushed the parser-attach-comment-option branch from 752fc2b to 24b79c8 Compare August 4, 2021 14:46

nicolo-ribaudo merged commit d5b0d9e into babel:main Aug 4, 2021

nicolo-ribaudo deleted the parser-attach-comment-option branch August 4, 2021 15:04

nicolo-ribaudo changed the title ~~Add attachComment parser option to allow optionally disable comment attachment~~ Add attachComment parser option to disable comment attachment Aug 4, 2021

This was referenced Aug 5, 2021

chore(deps-dev): bump @babel/eslint-parser from 7.13.14 to 7.15.0 filecoin-project/slate#863

Merged

chore(deps-dev): bump @babel/core from 7.13.14 to 7.15.0 filecoin-project/slate#862

Closed

fisker mentioned this pull request Aug 6, 2021

Disable comment attach in babel parser prettier/prettier#11313

Closed

4 tasks

github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Nov 4, 2021

github-actions bot locked as resolved and limited conversation to collaborators Nov 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `attachComment` parser option to disable comment attachment #13229

Add `attachComment` parser option to disable comment attachment #13229

JLHwung commented Apr 29, 2021 •

edited

JLHwung Apr 29, 2021

codesandbox-ci bot commented Apr 29, 2021 •

edited

babel-bot commented Apr 29, 2021 •

edited

kaicataldo left a comment

hzoo left a comment

nicolo-ribaudo commented Apr 29, 2021

hzoo commented Apr 29, 2021

JLHwung Apr 29, 2021

aminya commented May 11, 2021 •

edited

nicolo-ribaudo commented May 11, 2021

aminya commented May 12, 2021

kaicataldo left a comment

nicolo-ribaudo commented Jul 2, 2021

nicolo-ribaudo commented Aug 3, 2021

Add attachComment parser option to disable comment attachment #13229

Add attachComment parser option to disable comment attachment #13229

Conversation

JLHwung commented Apr 29, 2021 • edited

JLHwung Apr 29, 2021

Choose a reason for hiding this comment

codesandbox-ci bot commented Apr 29, 2021 • edited

babel-bot commented Apr 29, 2021 • edited

kaicataldo left a comment

Choose a reason for hiding this comment

hzoo left a comment

Choose a reason for hiding this comment

nicolo-ribaudo commented Apr 29, 2021

hzoo commented Apr 29, 2021

JLHwung Apr 29, 2021

Choose a reason for hiding this comment

aminya commented May 11, 2021 • edited

nicolo-ribaudo commented May 11, 2021

aminya commented May 12, 2021

kaicataldo left a comment

Choose a reason for hiding this comment

nicolo-ribaudo commented Jul 2, 2021

nicolo-ribaudo commented Aug 3, 2021

Add `attachComment` parser option to disable comment attachment #13229

Add `attachComment` parser option to disable comment attachment #13229

JLHwung commented Apr 29, 2021 •

edited

codesandbox-ci bot commented Apr 29, 2021 •

edited

babel-bot commented Apr 29, 2021 •

edited

aminya commented May 11, 2021 •

edited