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
Fix parenthesis for nullish coalescing #10269
Conversation
…operator and if any is a logical expression without a paren then throw an error
...ges/babel-parser/test/fixtures/experimental/nullish-coalescing-operator/and-nullish/input.js
Show resolved
Hide resolved
...-parser/test/fixtures/experimental/nullish-coalescing-operator/no-paren-and-nullish/input.js
Show resolved
Hide resolved
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/11251/ |
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.
This is excellent! A few suggestions, but this is almost done.
* then raise an error | ||
*/ | ||
|
||
if (op === tt.logicalOR) { |
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 the binop
for the operators in
babel/packages/babel-parser/src/tokenizer/types.js
Lines 142 to 156 in 4d30379
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), | |
exponent: new TokenType("**", { | |
beforeExpr, | |
binop: 11, |
??
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.
...l-parser/test/fixtures/experimental/nullish-coalescing-operator/no-paren-nullish-or/input.js
Show resolved
Hide resolved
They're now incorrect
…ub.com/vivek12345/babel into fix-parenthesis-for-nullish-coalescing
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 comment
The 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 nullishCoalescing
operator which is as per spec.
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 seems like this incrementing of priority broke this test case
FAIL packages/babel-generator/test/index.js
● generation/typescript › cast as
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.
Why is priority important? It can be the same. a ?? b && c
is an error anyway, regardless of which has higher priority.
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 earlier ||
and ??
had same priority which was not right. ??
should have lower priority then ||
.
Do you think this approach is not right?
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.
See the discussion at #10269 (comment).
...l-parser/test/fixtures/experimental/nullish-coalescing-operator/no-paren-nullish-or/input.js
Show resolved
Hide resolved
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 comment
The reason will be displayed to describe this comment to others. Learn more.
See the discussion at #10269 (comment).
packages/babel-parser/test/fixtures/typescript/cast/as/output.json
Outdated
Show resolved
Hide resolved
… brackets in an ?? && || expression, test cases added
@jridgewell The build failed with this error
Can we restart the build? |
…l operators to be equal added
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.
This is a really great PR for a first time contributor! Thanks for the work!
Thank you @jridgewell for your patience and the kind words. It means a lot. First time contribution to babel has been a delight with your help and support 🎉 |
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.
Thanks!
@babel/babel This is technically a breaking change. How do you think that we should handle it?
Nullish coalescing is only available if you enable |
With this change all the following operations which are invalid will throw error and ask to add parenthesis to them