Add attachComment parser option to disable comment attachment#13229
Add attachComment parser option to disable comment attachment#13229nicolo-ribaudo merged 3 commits intobabel:mainfrom
attachComment parser option to disable comment attachment#13229Conversation
| options.attachComment = false; | ||
| return parseExpression(input, options); | ||
| }, | ||
| ); |
There was a problem hiding this comment.
We run parseExpression(#, { attachComment: false }) on expressions fixture as smoke tests. It is faster than running over the complete parser tests.
|
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:
|
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/47913/ |
kaicataldo
left a comment
There was a problem hiding this comment.
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
Outdated
Show resolved
Hide resolved
hzoo
left a comment
There was a problem hiding this comment.
assume it's attachComment since it's named that in esprima? Ok with Kai's suggestion, just need to doc it!
|
In the future we might want to expand this to be |
|
Yeah acorn has onComment right https://github.com/acornjs/acorn/blob/7deba41118d6384a2c498c61176b3cf434f69590/acorn/src/options.js#L56-L66 |
| function benchCases(name, implementation, options) { | ||
| for (const length of [256, 512, 1024, 2048]) { | ||
| suite.add(`${name} ${length} empty statement`, () => { | ||
| implementation.parse(";".repeat(length), options); |
There was a problem hiding this comment.
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.
6c0bcd4 to
0a37178
Compare
|
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. |
|
@aminya |
|
Oh, my bad. The title was a bit confusing. |
attachComment parser optionattachComment parser option to allow optionally disable comment attachment
0a37178 to
5792805
Compare
|
This probably needs to be redone after #13521 is merged. |
49a120f to
674b637
Compare
|
@JLHwung Could you rebase? |
674b637 to
752fc2b
Compare
752fc2b to
24b79c8
Compare
attachComment parser option to allow optionally disable comment attachmentattachComment parser option to disable comment attachment
This PR introduces
attachComment: booleanoption to@babel/parser. It istrueby default, when it isfalse, 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 nodeFile.The option name is borrowed from
espree, thoughespreehas 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)
The comment attachment is crucial when we print AST back to sources. However, if users are not gonna print AST, disabling
attachCommentcan boost the parser performance. For example, the@babel/eslint-parsercan setattachCommenttofalsesince 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)
This is expected because when there are no comments, the new attachment algorithm exits early when
state.commentStacksis empty.Many leading + trailing comments: (30% improvements)
When
attachCommentisfalse, we skip creating comment whitespaces and the comment processing, which results to 30% performance boost.