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 parenthesis for nullish coalescing #10269

Merged

Conversation

vivek12345
Copy link
Contributor

@vivek12345 vivek12345 commented Jul 25, 2019

Q                       A
Fixed Issues? Fixes #10260
Patch: Bug Fix? Bug Fix
Major: Breaking Change? Yes
Minor: New Feature? No
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes? NO
License MIT

With this change all the following operations which are invalid will throw error and ask to add parenthesis to them

// will throw syntax error
a ?? b || c

// will throw syntax error
a || b ?? c

// will throw syntax error
a ?? b && c

// will throw syntax error
a && b ?? c

@babel-bot
Copy link
Collaborator

babel-bot commented Jul 25, 2019

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

Copy link
Member

@jridgewell jridgewell left a 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) {
Copy link
Member

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

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,
. (This is actually how the spec is implemented, ?? has lower precedence than || and the rest)

Copy link
Contributor Author

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.

packages/babel-parser/src/parser/expression.js Outdated Show resolved Hide resolved
modulo: createBinop("%", 10),
star: createBinop("*", 10),
slash: createBinop("/", 10),
logicalOR: createBinop("||", 2),
Copy link
Contributor Author

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.

Copy link
Contributor Author

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

Copy link
Member

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.

Copy link
Contributor Author

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?

Copy link
Member

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).

modulo: createBinop("%", 10),
star: createBinop("*", 10),
slash: createBinop("/", 10),
logicalOR: createBinop("||", 2),
Copy link
Member

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/src/tokenizer/types.js Show resolved Hide resolved
@vivek12345
Copy link
Contributor Author

@jridgewell The build failed with this error

ENOMEM: not enough memory, read

Can we restart the build?

packages/babel-generator/test/index.js Show resolved Hide resolved
packages/babel-generator/test/index.js Show resolved Hide resolved
packages/babel-generator/src/node/index.js Show resolved Hide resolved
Copy link
Member

@jridgewell jridgewell left a 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!

@vivek12345
Copy link
Contributor Author

vivek12345 commented Jul 26, 2019

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 🎉

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a 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?

@nicolo-ribaudo nicolo-ribaudo added PR: Spec Compliance 👓 A type of pull request used for our changelog categories Spec: Nullish Coalescing Operator pkg: parser labels Jul 27, 2019
@jridgewell
Copy link
Member

Nullish coalescing is only available if you enable proposal-nullish-coalescing-operator, so I think people are aware that this is still a proposal. The change has an explicit message on how to fix, too. Might be ok to release as a minor with a very bold warning.

@nicolo-ribaudo nicolo-ribaudo merged commit b02e35c into babel:master Sep 6, 2019
@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Dec 6, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Dec 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: parser PR: Ready to be Merged A pull request with already two approvals, but waiting for the next minor release PR: Spec Compliance 👓 A type of pull request used for our changelog categories Spec: Nullish Coalescing Operator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NullishCoalescing forbids mixing with LogicalExpressions without parenthesis
7 participants