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

Fix parens around comment at object-destructuring assignment #1129

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gnprice
Copy link
Contributor

@gnprice gnprice commented Jun 12, 2022

Fixes #575.

We were taking code like the following:

(/**/{foo} = 1)

and wrapping the comment in parens, which is invalid syntax:

(/**/)({foo} = 1)

The symptom appears when using the default parser, but not esprima;
and only when you call the printer directly on the AST node for the
expression, not the whole program.

The bug here was that in needsParens, we check to see if node is
an AssignmentExpression with ObjectPattern, and if so we say that
we do need parens... but we were making that check at the very top
of the function, before we'd checked that node was actually the
thing we wanted to be operating on at all.

If in fact the path currently points to a comment -- as it can
when needsParens gets called by the printer invoked by the
print(commentPath) in printLeadingComment or printTrailingComment
-- then the comment doesn't count as a node, and so getNode will
actually return the parent. If that parent meets the
AssignmentExpression / ObjectPattern condition, then we would
buggily decide that the comment needed parens too.

Fix it by checking that node is the actual top of the stack,
i.e. this.getValue(), before doing anything with node.
Also add a regression test.

We were taking code like the following:

    (/**/{foo} = 1)

and wrapping the comment in parens, which is invalid syntax:

    (/**/)({foo} = 1)

The symptom appears when using the default parser, but not esprima;
and only when you call the printer directly on the AST node for the
expression, not the whole program.

The bug here was that in needsParens, we check to see if `node` is
an AssignmentExpression with ObjectPattern, and if so we say that
we do need parens... but we were making that check at the very top
of the function, before we'd checked that `node` was actually the
thing we wanted to be operating on at all.

If in fact the path currently points to a comment -- as it can
when needsParens gets called by the printer invoked by the
`print(commentPath)` in printLeadingComment or printTrailingComment
-- then the comment doesn't count as a node, and so `getNode` will
actually return the parent.  If that parent meets the
AssignmentExpression / ObjectPattern condition, then we would
buggily decide that the comment needed parens too.

Fix it by checking that `node` is the actual top of the stack,
i.e. `this.getValue()`, before doing anything with `node`.
Also add a regression test.

Fixes: benjamn#575
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

recast generates invalid code when printing destructuring assignment expressions preceded by comments
1 participant