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 parenthesis for nullish coalescing #10269
Changes from 6 commits
2ea36b6
2ec028c
82a6ac2
823d998
a7037dd
0fb1916
3260bef
6a0de8b
5655417
d9ea03c
6918e86
417cd40
4a2d372
40eb40e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -380,6 +380,58 @@ export default class ExpressionParser extends LValParser { | |
|
||
node.right = this.parseExprOpRightExpr(op, prec, noIn); | ||
|
||
/* this check is needed because for operations like | ||
* a ?? b || c | ||
* a ?? b => This is considered as a logical expression in the ast tree | ||
* c => Identifier | ||
* so we need to check, only for || cases that if left is logical expression without paren | ||
* then raise an error | ||
*/ | ||
|
||
if (op === tt.logicalOR) { | ||
if ( | ||
left.type === "LogicalExpression" && | ||
left.operator === "??" && | ||
!(left.extra && left.extra.parenthesized) | ||
) { | ||
throw this.raise( | ||
left.start, | ||
`Wrap left hand side of the expression in parenthesis`, | ||
); | ||
} | ||
} | ||
/* this check is for all && and ?? operators | ||
* a ?? b && c for this example | ||
* b && c => This is considered as a logical expression in the ast tree | ||
* a => Identifier | ||
* so for ?? operator we need to check in this case the right expression | ||
* second case a && b ?? c | ||
* here a && b => This is considered as a logical expression in the ast tree | ||
* c => identifer | ||
* so now here for ?? operator we need to check the left expression | ||
*/ | ||
if (op === tt.nullishCoalescing) { | ||
if ( | ||
left.type === "LogicalExpression" && | ||
left.operator !== "??" && | ||
!(left.extra && left.extra.parenthesized) | ||
) { | ||
throw this.raise( | ||
left.start, | ||
`Wrap left hand side of the expression in parenthesis`, | ||
jridgewell marked this conversation as resolved.
Show resolved
Hide resolved
|
||
); | ||
} else if ( | ||
node.right.type === "LogicalExpression" && | ||
node.right.operator !== "??" && | ||
!(node.right.extra && node.right.extra.parenthesized) | ||
) { | ||
throw this.raise( | ||
node.right.start, | ||
`Wrap right hand side of the expression in parenthesis.`, | ||
); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jridgewell @vivek12345 Can I ask why you are checking the AST node property? It should be enough to use current token as a func arg and validate against it. That way you 1) improve performance 2) skip duplicate code 3) simplify This will also improve the performance. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would only work for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would only work for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @nicolo-ribaudo I'm confused because it will work for both lhs and rhs. I implemented it that way I described. See here . And it works very well to if you try it in the REPL |
||
} | ||
|
||
this.finishNode( | ||
node, | ||
op === tt.logicalOR || | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
a && b ?? c; | ||
(a && b) ?? c; | ||
jridgewell marked this conversation as resolved.
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
c && d ?? e; | ||
jridgewell marked this conversation as resolved.
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
{ | ||
"plugins": [ | ||
"nullishCoalescingOperator", | ||
"estree" | ||
], | ||
"throws": "Wrap left hand side of the expression in parenthesis (1:0)" | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
a ?? b && c; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
{ | ||
"plugins": [ | ||
"nullishCoalescingOperator" | ||
], | ||
"throws": "Wrap right hand side of the expression in parenthesis. (1:5)" | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
e ?? f ?? g || h; | ||
jridgewell marked this conversation as resolved.
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
{ | ||
"plugins": [ | ||
"nullishCoalescingOperator" | ||
], | ||
"throws": "Wrap left hand side of the expression in parenthesis (1:0)" | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
h || i ?? j; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
{ | ||
"plugins": [ | ||
"nullishCoalescingOperator", | ||
"estree" | ||
], | ||
"throws": "Wrap left hand side of the expression in parenthesis (1:0)" | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
a ?? b && c; | ||
a ?? (b && c); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
a ?? b || c; | ||
a ?? (b || c); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can actually avoid this by lowering the precedence of the
??
operator compared to||
. You can do that by raising thebinop
for the operators inbabel/packages/babel-parser/src/tokenizer/types.js
Lines 142 to 156 in 4d30379
??
has lower precedence than||
and the rest)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @jridgewell. This change has been made.