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 9 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 |
---|---|---|
|
@@ -139,21 +139,21 @@ export const types: { [name: string]: TokenType } = { | |
tilde: new TokenType("~", { beforeExpr, prefix, startsExpr }), | ||
pipeline: createBinop("|>", 0), | ||
nullishCoalescing: createBinop("??", 1), | ||
logicalOR: createBinop("||", 1), | ||
logicalAND: createBinop("&&", 2), | ||
bitwiseOR: createBinop("|", 3), | ||
bitwiseXOR: createBinop("^", 4), | ||
bitwiseAND: createBinop("&", 5), | ||
equality: createBinop("==/!=/===/!==", 6), | ||
relational: createBinop("</>/<=/>=", 7), | ||
bitShift: createBinop("<</>>/>>>", 8), | ||
plusMin: new TokenType("+/-", { beforeExpr, binop: 9, prefix, startsExpr }), | ||
modulo: createBinop("%", 10), | ||
star: createBinop("*", 10), | ||
slash: createBinop("/", 10), | ||
logicalOR: createBinop("||", 2), | ||
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. priority of other operators incremented by +1 so that they have more priority over the 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 seems like this incrementing of priority broke this test case
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. Why is priority important? It can be the same. 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 earlier 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. See the discussion at #10269 (comment). |
||
logicalAND: createBinop("&&", 3), | ||
bitwiseOR: createBinop("|", 4), | ||
bitwiseXOR: createBinop("^", 5), | ||
bitwiseAND: createBinop("&", 6), | ||
equality: createBinop("==/!=/===/!==", 7), | ||
relational: createBinop("</>/<=/>=", 8), | ||
jridgewell marked this conversation as resolved.
Show resolved
Hide resolved
|
||
bitShift: createBinop("<</>>/>>>", 9), | ||
plusMin: new TokenType("+/-", { beforeExpr, binop: 10, prefix, startsExpr }), | ||
modulo: createBinop("%", 11), | ||
star: createBinop("*", 11), | ||
slash: createBinop("/", 11), | ||
exponent: new TokenType("**", { | ||
beforeExpr, | ||
binop: 11, | ||
binop: 12, | ||
rightAssociative: true, | ||
}), | ||
|
||
|
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": "Nullish coalescing operator(??) requires parens when mixing with logical operators (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": "Nullish coalescing operator(??) requires parens when mixing with logical operators (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": "Nullish coalescing operator(??) requires parens when mixing with logical operators (1:10)" | ||
} |
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": "Nullish coalescing operator(??) requires parens when mixing with logical operators (1:0)" | ||
} |
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.
@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 comment
The reason will be displayed to describe this comment to others. Learn more.
It would only work for
a && b ?? c
, but not fora ?? b && c
since in the second case the precedence isa ?? (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.
It would only work for
a && b ?? c
, but not fora ?? b && c
since in the second case the precedence isa ?? (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.
@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