Fix parenthesis for nullish coalescing by vivek12345 · Pull Request #10269 · babel/babel

vivek12345 · 2019-07-25T21:35:45Z

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

…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

...-parser/test/fixtures/experimental/nullish-coalescing-operator/no-paren-and-nullish/input.js

babel-bot · 2019-07-25T22:13:13Z

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

jridgewell

This is excellent! A few suggestions, but this is almost done.

jridgewell · 2019-07-26T03:01:20Z

packages/babel-parser/src/parser/expression.js

+         * then raise an error
+         */
+
+        if (op === tt.logicalOR) {


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,

. (This is actually how the spec is implemented, ?? has lower precedence than || and the rest)

Thank you @jridgewell. This change has been made.

packages/babel-parser/src/parser/expression.js

...l-parser/test/fixtures/experimental/nullish-coalescing-operator/no-paren-nullish-or/input.js

They're now incorrect

…1 from ?? and

…ub.com/vivek12345/babel into fix-parenthesis-for-nullish-coalescing

vivek12345 · 2019-07-26T13:28:32Z

packages/babel-parser/src/tokenizer/types.js

-  modulo: createBinop("%", 10),
-  star: createBinop("*", 10),
-  slash: createBinop("/", 10),
+  logicalOR: createBinop("||", 2),


priority of other operators incremented by +1 so that they have more priority over the nullishCoalescing operator which is as per spec.

It seems like this incrementing of priority broke this test case

FAIL packages/babel-generator/test/index.js ● generation/typescript › cast as

Why is priority important? It can be the same. a ?? b && c is an error anyway, regardless of which has higher priority.

@nicolo-ribaudo earlier || and ?? had same priority which was not right. ?? should have lower priority then ||.
Do you think this approach is not right?

See the discussion at #10269 (comment).

...l-parser/test/fixtures/experimental/nullish-coalescing-operator/no-paren-nullish-or/input.js

jridgewell · 2019-07-26T19:25:55Z

packages/babel-parser/src/tokenizer/types.js

-  modulo: createBinop("%", 10),
-  star: createBinop("*", 10),
-  slash: createBinop("/", 10),
+  logicalOR: createBinop("||", 2),


See the discussion at #10269 (comment).

packages/babel-parser/src/tokenizer/types.js

packages/babel-parser/test/fixtures/typescript/cast/as/output.json

… brackets in an ?? && || expression, test cases added

vivek12345 · 2019-07-26T20:11:49Z

@jridgewell The build failed with this error

ENOMEM: not enough memory, read

Can we restart the build?

packages/babel-generator/test/index.js

packages/babel-generator/src/node/index.js

…l operators to be equal added

jridgewell

This is a really great PR for a first time contributor! Thanks for the work!

packages/babel-parser/test/unit/tokenizer/types.js

vivek12345 · 2019-07-26T21:40:54Z

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 🎉

nicolo-ribaudo

Thanks!

@babel/babel This is technically a breaking change. How do you think that we should handle it?

jridgewell · 2019-07-27T21:25:12Z

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.

KFlash · 2019-08-03T04:42:09Z

packages/babel-parser/src/parser/expression.js

+              node.right.start,
+              `Nullish coalescing operator(??) requires parens when mixing with logical operators`,
+            );
+          }


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

It would only work for a && b ?? c, but not for a ?? b && c since in the second case the precedence is a ?? (b && c)

@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

nicolo-ribaudo · 2019-08-14T15:17:18Z

Regarding #10269 (review), I think that we should merge it since

Most people would probably have already used parentheses, to improve readability
Prettier did already add the now required parenthesis

existentialism

👍

montogeek · 2019-09-03T09:50:06Z

What is left to be able to merge it?

nicolo-ribaudo · 2019-09-03T15:15:16Z

We are going to release a minor version with this patch soon!

vivek12345 added 3 commits July 24, 2019 14:23

♻️ added condition to check for left and right of nullish coalescing …

2ea36b6

…operator and if any is a logical expression without a paren then throw an error

🐛 bugs fixed and test cases updated for babel parser

2ec028c

♻️ code comments added

82a6ac2

vivek12345 commented Jul 25, 2019

View reviewed changes

...ges/babel-parser/test/fixtures/experimental/nullish-coalescing-operator/and-nullish/input.js Show resolved Hide resolved

vivek12345 commented Jul 25, 2019

View reviewed changes

...-parser/test/fixtures/experimental/nullish-coalescing-operator/no-paren-and-nullish/input.js Show resolved Hide resolved

vivek12345 added 3 commits July 26, 2019 03:18

🐛 spell error rectified

823d998

♻️ failing test updated

a7037dd

🐛 push tests after make build

0fb1916

jridgewell reviewed Jul 26, 2019

View reviewed changes

jridgewell and others added 3 commits July 25, 2019 23:16

Skip nullish-coalescing flow precedence tests

3260bef

They're now incorrect

♻️ error message updated, binop priority of other logical operators +…

6a0de8b

…1 from ?? and

Merge branch 'fix-parenthesis-for-nullish-coalescing' of https://gith…

5655417

…ub.com/vivek12345/babel into fix-parenthesis-for-nullish-coalescing

vivek12345 commented Jul 26, 2019

View reviewed changes

jridgewell reviewed Jul 26, 2019

View reviewed changes

vivek12345 added 2 commits July 27, 2019 01:27

♻️ increaed the binOp for in and instanceOf, added logic to print the…

d9ea03c

… brackets in an ?? && || expression, test cases added

🐛 failing test fixed and comments updated

6918e86

vivek12345 commented Jul 26, 2019

View reviewed changes

packages/babel-generator/test/index.js Show resolved Hide resolved

jridgewell approved these changes Jul 26, 2019

View reviewed changes

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

vivek12345 added 2 commits July 27, 2019 02:25

♻️ new lines added between tests

417cd40

♻️ basic tests for checking the binOp of instanceOf, in and relationa…

4a2d372

…l operators to be equal added

jridgewell approved these changes Jul 26, 2019

View reviewed changes

packages/babel-parser/test/unit/tokenizer/types.js Show resolved Hide resolved

♻️ new lines added in between tests

40eb40e

nicolo-ribaudo approved these changes Jul 27, 2019

View reviewed changes

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

existentialism mentioned this pull request Jul 27, 2019

Add optional chaining and nullish coalescing operators support facebook/create-react-app#7438

Merged

KFlash reviewed Aug 3, 2019

View reviewed changes

nicolo-ribaudo added this to the v7.6.0 milestone Aug 14, 2019

existentialism approved these changes Aug 28, 2019

View reviewed changes

montogeek approved these changes Sep 3, 2019

View reviewed changes

nicolo-ribaudo added the PR: Ready to be Merged A pull request with already two approvals, but waiting for the next minor release label Sep 3, 2019

nicolo-ribaudo merged commit b02e35c into babel:master Sep 6, 2019

snyk-bot mentioned this pull request Oct 24, 2019

[Snyk] Upgrade @babel/runtime from 7.4.4 to 7.6.3 Matthelonianxl/drizzle#9

Open

snyk-bot mentioned this pull request Nov 1, 2019

[Snyk] Upgrade @babel/core from 7.2.2 to 7.6.4 nbv1982/material-ui#5

Open

jpangelle mentioned this pull request Nov 7, 2019

Add support for optional chaining and nullary coalescing facebook/create-react-app#7431

Closed

snyk-bot mentioned this pull request Nov 12, 2019

[Snyk] Upgrade @babel/runtime from 7.1.5 to 7.6.3 ajesse11x/Parse-SDK-JS#1

Open

This was referenced Nov 28, 2019

[Snyk] Upgrade @babel/runtime from 7.0.0-beta.42 to 7.7.2 ali8889/mist#4

Open

[Snyk] Upgrade @babel/runtime from 7.5.5 to 7.7.4 dyna-dot/CyberChef#3

Open

snyk-bot mentioned this pull request Dec 5, 2019

[Snyk] Upgrade @babel/runtime from 7.3.1 to 7.7.4 dyna-dot/bit-javascript#5

Open

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 bot locked as resolved and limited conversation to collaborators Dec 6, 2019

	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,

Uh oh!

Conversation

vivek12345 commented Jul 25, 2019 • edited by nicolo-ribaudo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

babel-bot commented Jul 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jridgewell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vivek12345 Jul 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

vivek12345 commented Jul 26, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jridgewell left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vivek12345 commented Jul 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolo-ribaudo left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jridgewell commented Jul 27, 2019

Uh oh!

KFlash Aug 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KFlash Aug 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolo-ribaudo commented Aug 14, 2019

Uh oh!

existentialism left a comment

Choose a reason for hiding this comment

Uh oh!

montogeek commented Sep 3, 2019

Uh oh!

nicolo-ribaudo commented Sep 3, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

vivek12345 commented Jul 25, 2019 •

edited by nicolo-ribaudo

Loading

babel-bot commented Jul 25, 2019 •

edited

Loading

vivek12345 Jul 26, 2019 •

edited

Loading

vivek12345 commented Jul 26, 2019 •

edited

Loading

nicolo-ribaudo left a comment •

edited

Loading

KFlash Aug 3, 2019 •

edited

Loading

KFlash Aug 14, 2019 •

edited

Loading