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

[estree] attach comments after directives at the end of file #14920

Merged
merged 3 commits into from Sep 13, 2022

Conversation

hegemonic
Copy link
Contributor

@hegemonic hegemonic commented Sep 12, 2022

Q                       A
Fixed Issues? Fixes #14919
Patch: Bug Fix? yes
Major: Breaking Change? no
Minor: New Feature? no
Tests Added + Pass? Yes
Documentation PR Link n/a
Any Dependency Changes? no
License MIT

If a source file contains 1) a directive, followed by 2) a comment, and nothing else, then @babel/parser attaches the comment to the Directive node as a trailing comment.

The ESTree plugin handles Directive nodes by creating an ExpressionStatement node, then transferring information from the Directive node to the ExpressionStatement node. Previously, at the time it transfers information, comments hadn't yet been attached to the Directive node. And even if they had been, the ESTree plugin didn't do anything with comments attached to the Directive node. As a result, the source file's comment wasn't attached to any node in the resulting AST.

With this change, the ESTree plugin generates an AST in which the comment is attached to the ExpressionStatement node as a trailing comment.

Fixes #14919.

…mment, then EOF

If a source file contains 1) a directive, followed by 2) a comment, and nothing else, then `@babel/parser` attaches the comment to the `Directive` node as a trailing comment.

The ESTree plugin handles `Directive` nodes by creating an `ExpressionStatement` node, then transferring information from the `Directive` node to the `ExpressionStatement` node. Previously, at the time it transfers information, comments hadn't yet been attached to the `Directive` node. And even if they had been, the ESTree plugin didn't do anything with comments attached to the `Directive` node. As a result, the source file's comment wasn't attached to any node in the resulting AST.

With this change, the ESTree plugin generates an AST in which the comment is attached to the `ExpressionStatement` node as a trailing comment.
@babel-bot
Copy link
Collaborator

babel-bot commented Sep 12, 2022

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

@liuxingbaoyu liuxingbaoyu added PR: Bug Fix 🐛 A type of pull request used for our changelog categories pkg: parser area: estree labels Sep 12, 2022
Copy link
Contributor

@JLHwung JLHwung left a comment

Choose a reason for hiding this comment

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

Can you add a new test case with estree plugin enabled?

// 1;
"use strict";
// 2;
"use strict";

The comment 1 should be the leading comment of the first ExpressionStatement, and 2 should be the trailing comment of the first ExpressionStatement and the leading comment of the second ExpressionStatement.

@hegemonic
Copy link
Contributor Author

The babel-old-version check is failing, but I don't think this pull request caused it to fail. If you disagree, please let me know.

@liuxingbaoyu
Copy link
Member

Yes, the CI failure has nothing to do with this PR.

Copy link
Contributor

@JLHwung JLHwung left a comment

Choose a reason for hiding this comment

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

Thanks!

@nicolo-ribaudo nicolo-ribaudo changed the title fix: for ESTree, attach comments when a directive is followed by a comment, then EOF [estree] attach comments after directives at the end of file Sep 13, 2022
@nicolo-ribaudo nicolo-ribaudo merged commit 580d730 into babel:main Sep 13, 2022
@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 Dec 14, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: estree outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: parser PR: Bug Fix 🐛 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: ESTree plugin can stop comments from being attached to directives
5 participants