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.

Sign up for GitHub

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

Jump to bottom

Fix parenthesis for nullish coalescing #10269

Merged
merged 14 commits into from Sep 6, 2019
Merged

Fix parenthesis for nullish coalescing #10269

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
2ea36b6
:recycle: added condition to check for left and right of nullish coal…
vivek12345 Jul 24, 2019
2ec028c
:bug: bugs fixed and test cases updated for babel parser
vivek12345 Jul 25, 2019
82a6ac2
:recycle: code comments added
vivek12345 Jul 25, 2019
823d998
:bug: spell error rectified
vivek12345 Jul 25, 2019
a7037dd
:recycle: failing test updated
vivek12345 Jul 25, 2019
0fb1916
:bug: push tests after make build
vivek12345 Jul 25, 2019
3260bef
Skip nullish-coalescing flow precedence tests
jridgewell Jul 26, 2019
6a0de8b
:recycle: error message updated, binop priority of other logical oper…
vivek12345 Jul 26, 2019
5655417
Merge branch 'fix-parenthesis-for-nullish-coalescing' of https://gith…
vivek12345 Jul 26, 2019
d9ea03c
:recycle: increaed the binOp for in and instanceOf, added logic to pr…
vivek12345 Jul 26, 2019
6918e86
:bug: failing test fixed and comments updated
vivek12345 Jul 26, 2019
417cd40
:recycle: new lines added between tests
vivek12345 Jul 26, 2019
4a2d372
:recycle: basic tests for checking the binOp of instanceOf, in and re…
vivek12345 Jul 26, 2019
40eb40e
:recycle: new lines added in between tests
vivek12345 Jul 26, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
52 changes: 52 additions & 0 deletions packages/babel-parser/src/parser/expression.js
View file
Open in desktop
Expand Up @@ -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) {
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

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)

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.

if (
left.type === "LogicalExpression" &&
left.operator === "??" &&
!(left.extra && left.extra.parenthesized)
) {
throw this.raise(
left.start,
`Wrap left hand side of the expression in parentheses`,
);
}
}
/* 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 parentheses`,
);
} 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 parentheses.`,
);
}
Copy link

@KFlash KFlash Aug 3, 2019 •
edited

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.

Copy link
Member

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 for a ?? b && c since in the second case the precedence is a ?? (b && c)

Copy link
Member

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 for a ?? b && c since in the second case the precedence is a ?? (b && c)

Copy link

@KFlash KFlash Aug 14, 2019 •
edited

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

}

this.finishNode(
node,
op === tt.logicalOR ||
Expand Down
2 changes: 1 addition & 1 deletion .../babel-parser/test/fixtures/experimental/nullish-coalescing-operator/and-nullish/input.js
View file
Open in desktop
@@ -1 +1 @@
a && b ?? c;
(a && b) ?? c;
jridgewell marked this conversation as resolved.
Show resolved Hide resolved
52 changes: 28 additions & 24 deletions ...bel-parser/test/fixtures/experimental/nullish-coalescing-operator/and-nullish/output.json
View file
Open in desktop
@@ -1,29 +1,29 @@
{
"type": "File",
"start": 0,
"end": 12,
"end": 14,
"loc": {
"start": {
"line": 1,
"column": 0
},
"end": {
"line": 1,
"column": 12
"column": 14
}
},
"program": {
"type": "Program",
"start": 0,
"end": 12,
"end": 14,
"loc": {
"start": {
"line": 1,
"column": 0
},
"end": {
"line": 1,
"column": 12
"column": 14
}
},
"sourceType": "script",
Expand All @@ -32,57 +32,57 @@
{
"type": "ExpressionStatement",
"start": 0,
"end": 12,
"end": 14,
"loc": {
"start": {
"line": 1,
"column": 0
},
"end": {
"line": 1,
"column": 12
"column": 14
}
},
"expression": {
"type": "LogicalExpression",
"start": 0,
"end": 11,
"end": 13,
"loc": {
"start": {
"line": 1,
"column": 0
},
"end": {
"line": 1,
"column": 11
"column": 13
}
},
"left": {
"type": "LogicalExpression",
"start": 0,
"end": 6,
"start": 1,
"end": 7,
"loc": {
"start": {
"line": 1,
"column": 0
"column": 1
},
"end": {
"line": 1,
"column": 6
"column": 7
}
},
"left": {
"type": "Identifier",
"start": 0,
"end": 1,
"start": 1,
"end": 2,
"loc": {
"start": {
"line": 1,
"column": 0
"column": 1
},
"end": {
"line": 1,
"column": 1
"column": 2
},
"identifierName": "a"
},
Expand All @@ -91,35 +91,39 @@
"operator": "&&",
"right": {
"type": "Identifier",
"start": 5,
"end": 6,
"start": 6,
"end": 7,
"loc": {
"start": {
"line": 1,
"column": 5
"column": 6
},
"end": {
"line": 1,
"column": 6
"column": 7
},
"identifierName": "b"
},
"name": "b"
},
"extra": {
"parenthesized": true,
"parenStart": 0
}
},
"operator": "??",
"right": {
"type": "Identifier",
"start": 10,
"end": 11,
"start": 12,
"end": 13,
"loc": {
"start": {
"line": 1,
"column": 10
"column": 12
},
"end": {
"line": 1,
"column": 11
"column": 13
},
"identifierName": "c"
},
Expand Down
1 change: 1 addition & 0 deletions ...rser/test/fixtures/experimental/nullish-coalescing-operator/no-paren-and-nullish/input.js
View file
Open in desktop
@@ -0,0 +1 @@
c && d ?? e;
jridgewell marked this conversation as resolved.
Show resolved Hide resolved
7 changes: 7 additions & 0 deletions .../test/fixtures/experimental/nullish-coalescing-operator/no-paren-and-nullish/options.json
View file
Open in desktop
@@ -0,0 +1,7 @@
{
"plugins": [
"nullishCoalescingOperator",
"estree"
],
"throws": "Wrap left hand side of the expression in parentheses (1:0)"
}
1 change: 1 addition & 0 deletions ...rser/test/fixtures/experimental/nullish-coalescing-operator/no-paren-nullish-and/input.js
View file
Open in desktop
@@ -0,0 +1 @@
a ?? b && c;
6 changes: 6 additions & 0 deletions .../test/fixtures/experimental/nullish-coalescing-operator/no-paren-nullish-and/options.json
View file
Open in desktop
@@ -0,0 +1,6 @@
{
"plugins": [
"nullishCoalescingOperator"
],
"throws": "Wrap right hand side of the expression in parentheses. (1:5)"
}
1 change: 1 addition & 0 deletions ...arser/test/fixtures/experimental/nullish-coalescing-operator/no-paren-nullish-or/input.js
View file
Open in desktop
@@ -0,0 +1 @@
e ?? f ?? g || h;
jridgewell marked this conversation as resolved.
Show resolved Hide resolved
6 changes: 6 additions & 0 deletions ...r/test/fixtures/experimental/nullish-coalescing-operator/no-paren-nullish-or/options.json
View file
Open in desktop
@@ -0,0 +1,6 @@
{
"plugins": [
"nullishCoalescingOperator"
],
"throws": "Wrap left hand side of the expression in parentheses (1:0)"
}
1 change: 1 addition & 0 deletions ...arser/test/fixtures/experimental/nullish-coalescing-operator/no-paren-or-nullish/input.js
View file
Open in desktop
@@ -0,0 +1 @@
h || i ?? j;
7 changes: 7 additions & 0 deletions ...r/test/fixtures/experimental/nullish-coalescing-operator/no-paren-or-nullish/options.json
View file
Open in desktop
@@ -0,0 +1,7 @@
{
"plugins": [
"nullishCoalescingOperator",
"estree"
],
"throws": "Wrap left hand side of the expression in parentheses (1:0)"
}
2 changes: 1 addition & 1 deletion .../babel-parser/test/fixtures/experimental/nullish-coalescing-operator/nullish-and/input.js
View file
Open in desktop
@@ -1 +1 @@
a ?? b && c;
a ?? (b && c);
44 changes: 24 additions & 20 deletions ...bel-parser/test/fixtures/experimental/nullish-coalescing-operator/nullish-and/output.json
View file
Open in desktop
@@ -1,29 +1,29 @@
{
"type": "File",
"start": 0,
"end": 12,
"end": 14,
"loc": {
"start": {
"line": 1,
"column": 0
},
"end": {
"line": 1,
"column": 12
"column": 14
}
},
"program": {
"type": "Program",
"start": 0,
"end": 12,
"end": 14,
"loc": {
"start": {
"line": 1,
"column": 0
},
"end": {
"line": 1,
"column": 12
"column": 14
}
},
"sourceType": "script",
Expand All @@ -32,29 +32,29 @@
{
"type": "ExpressionStatement",
"start": 0,
"end": 12,
"end": 14,
"loc": {
"start": {
"line": 1,
"column": 0
},
"end": {
"line": 1,
"column": 12
"column": 14
}
},
"expression": {
"type": "LogicalExpression",
"start": 0,
"end": 11,
"end": 13,
"loc": {
"start": {
"line": 1,
"column": 0
},
"end": {
"line": 1,
"column": 11
"column": 13
}
},
"left": {
Expand All @@ -77,30 +77,30 @@
"operator": "??",
"right": {
"type": "LogicalExpression",
"start": 5,
"end": 11,
"start": 6,
"end": 12,
"loc": {
"start": {
"line": 1,
"column": 5
"column": 6
},
"end": {
"line": 1,
"column": 11
"column": 12
}
},
"left": {
"type": "Identifier",
"start": 5,
"end": 6,
"start": 6,
"end": 7,
"loc": {
"start": {
"line": 1,
"column": 5
"column": 6
},
"end": {
"line": 1,
"column": 6
"column": 7
},
"identifierName": "b"
},
Expand All @@ -109,20 +109,24 @@
"operator": "&&",
"right": {
"type": "Identifier",
"start": 10,
"end": 11,
"start": 11,
"end": 12,
"loc": {
"start": {
"line": 1,
"column": 10
"column": 11
},
"end": {
"line": 1,
"column": 11
"column": 12
},
"identifierName": "c"
},
"name": "c"
},
"extra": {
"parenthesized": true,
"parenStart": 5
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion ...s/babel-parser/test/fixtures/experimental/nullish-coalescing-operator/nullish-or/input.js
View file
Open in desktop
@@ -1 +1 @@
a ?? b || c;
a ?? (b || c);