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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
13 changes: 13 additions & 0 deletions packages/babel-generator/src/node/index.js
Expand Up @@ -97,5 +97,18 @@ export function needsParens(node, parent, printStack) {
if (isOrHasCallExpression(node)) return true;
}
jridgewell marked this conversation as resolved.
Show resolved Hide resolved

/* this check is for NullishCoalescing being used with LogicalOperators like && and ||
* For example when someone creates an ast programmaticaly like this
* t.logicalExpression(
* "??",
* t.logicalExpression("||", t.identifier("a"), t.identifier("b")),
* t.identifier("c"),
* );
* In the example above the AST is equivalent to writing a || b ?? c
* This is incorrect because NullishCoalescing when used with LogicalExpressions should have parenthesis
* The correct syntax is (a || b) ?? c, that is why we need parenthesis in this case
*/
if (t.isLogicalExpression(node) && parent.operator === "??") return true;

return find(expandedParens, node, parent, printStack);
}
@@ -1,4 +1,5 @@
foo ||bar;
(x => x)|| bar;
(function a(x){return x;})|| 2;
0||(function(){return alpha;});
0||(function(){return alpha;});
a ?? (b || c);
@@ -0,0 +1,3 @@
{
"plugins": ["nullishCoalescingOperator"]
}
Expand Up @@ -7,4 +7,6 @@ foo || bar;

0 || function () {
return alpha;
};
};

a ?? (b || c);
25 changes: 25 additions & 0 deletions packages/babel-generator/test/index.js
Expand Up @@ -298,6 +298,31 @@ describe("generation", function() {
});

describe("programmatic generation", function() {
it("should add parenthesis when NullishCoalescing is used along with ||", function() {
jridgewell marked this conversation as resolved.
Show resolved Hide resolved
// https://github.com/babel/babel/issues/10260
const nullishCoalesc = t.logicalExpression(
"??",
t.logicalExpression("||", t.identifier("a"), t.identifier("b")),
t.identifier("c"),
);
const output = generate(nullishCoalesc).code;
expect(output).toBe(`(a || b) ?? c`);
});
jridgewell marked this conversation as resolved.
Show resolved Hide resolved

it("should add parenthesis when NullishCoalesing is used with &&", function() {
const nullishCoalesc = t.logicalExpression(
"??",
t.identifier("a"),
t.logicalExpression(
"&&",
t.identifier("b"),
t.logicalExpression("&&", t.identifier("c"), t.identifier("d")),
),
);
const output = generate(nullishCoalesc).code;
expect(output).toBe(`a ?? (b && c && d)`);
});
jridgewell marked this conversation as resolved.
Show resolved Hide resolved

it("numeric member expression", function() {
// Should not generate `0.foo`
const mem = t.memberExpression(
Expand Down
33 changes: 33 additions & 0 deletions packages/babel-parser/src/parser/expression.js
Expand Up @@ -380,6 +380,39 @@ export default class ExpressionParser extends LValParser {

node.right = this.parseExprOpRightExpr(op, prec, noIn);

/* this check is for all ?? 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 to have parenthesis
* 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 to have parenthesis
* if the parenthesis is missing we raise an error and throw it
*/
if (op === tt.nullishCoalescing) {
if (
left.type === "LogicalExpression" &&
left.operator !== "??" &&
!(left.extra && left.extra.parenthesized)
) {
throw this.raise(
left.start,
`Nullish coalescing operator(??) requires parens when mixing with logical operators`,
);
} else if (
node.right.type === "LogicalExpression" &&
node.right.operator !== "??" &&
!(node.right.extra && node.right.extra.parenthesized)
) {
throw this.raise(
node.right.start,
`Nullish coalescing operator(??) requires parens when mixing with logical operators`,
);
}
Copy link

@KFlash KFlash Aug 3, 2019

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

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
30 changes: 15 additions & 15 deletions packages/babel-parser/src/tokenizer/types.js
Expand Up @@ -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),
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).

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,
}),

Expand Down Expand Up @@ -188,8 +188,8 @@ export const types: { [name: string]: TokenType } = {
_null: createKeyword("null", { startsExpr }),
_true: createKeyword("true", { startsExpr }),
_false: createKeyword("false", { startsExpr }),
_in: createKeyword("in", { beforeExpr, binop: 7 }),
_instanceof: createKeyword("instanceof", { beforeExpr, binop: 7 }),
_in: createKeyword("in", { beforeExpr, binop: 8 }),
_instanceof: createKeyword("instanceof", { beforeExpr, binop: 8 }),
_typeof: createKeyword("typeof", { beforeExpr, prefix, startsExpr }),
_void: createKeyword("void", { beforeExpr, prefix, startsExpr }),
_delete: createKeyword("delete", { beforeExpr, prefix, startsExpr }),
Expand Down
Expand Up @@ -457,7 +457,7 @@
"isAssign": false,
"prefix": true,
"postfix": false,
"binop": 9,
"binop": 10,
"updateContext": null
},
"value": "+",
Expand Down
@@ -1 +1 @@
a && b ?? c;
(a && b) ?? c;
jridgewell marked this conversation as resolved.
Show resolved Hide resolved
@@ -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
@@ -0,0 +1 @@
c && d ?? e;
jridgewell marked this conversation as resolved.
Show resolved Hide resolved
@@ -0,0 +1,7 @@
{
"plugins": [
"nullishCoalescingOperator",
"estree"
],
"throws": "Nullish coalescing operator(??) requires parens when mixing with logical operators (1:0)"
}
@@ -0,0 +1 @@
a ?? b && c;
@@ -0,0 +1,6 @@
{
"plugins": [
"nullishCoalescingOperator"
],
"throws": "Nullish coalescing operator(??) requires parens when mixing with logical operators (1:5)"
}
@@ -0,0 +1 @@
e ?? f ?? g || h;
jridgewell marked this conversation as resolved.
Show resolved Hide resolved
@@ -0,0 +1,6 @@
{
"plugins": [
"nullishCoalescingOperator"
],
"throws": "Nullish coalescing operator(??) requires parens when mixing with logical operators (1:10)"
}
@@ -0,0 +1 @@
h || i ?? j;
@@ -0,0 +1,7 @@
{
"plugins": [
"nullishCoalescingOperator",
"estree"
],
"throws": "Nullish coalescing operator(??) requires parens when mixing with logical operators (1:0)"
}
@@ -1 +1 @@
a ?? b && c;
a ?? (b && c);