Fix parenthesis for nullish coalescing (#10269)

* ♻️ added condition to check for left and right of nullish coalescing operator and if any is a logical expression without a paren then throw an error

* 🐛 bugs fixed and test cases updated for babel parser

* ♻️ code comments added

* 🐛 spell error rectified

* ♻️ failing test updated

* 🐛 push tests after make build

* Skip nullish-coalescing flow precedence tests

They're now incorrect

* ♻️ error message updated, binop priority of other logical operators +1 from ?? and

* ♻️ increaed the binOp for in and instanceOf, added logic to print the brackets in an ?? && || expression, test cases added

* 🐛 failing test fixed and comments updated

* ♻️ new lines added between tests

* ♻️ basic tests for checking the binOp of instanceOf, in and relational operators to be equal added

* ♻️ new lines added in between tests
This commit is contained in:
Vivek Nayyar 2019-09-06 22:35:44 +07:00 committed by Nicolò Ribaudo
parent 3e8a5c5e28
commit b02e35c19a
26 changed files with 282 additions and 142 deletions

View File

@ -97,5 +97,18 @@ export function needsParens(node, parent, printStack) {
if (isOrHasCallExpression(node)) return true; if (isOrHasCallExpression(node)) return true;
} }
/* 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); return find(expandedParens, node, parent, printStack);
} }

View File

@ -2,3 +2,4 @@ foo ||bar;
(x => x)|| bar; (x => x)|| bar;
(function a(x){return x;})|| 2; (function a(x){return x;})|| 2;
0||(function(){return alpha;}); 0||(function(){return alpha;});
a ?? (b || c);

View File

@ -0,0 +1,3 @@
{
"plugins": ["nullishCoalescingOperator"]
}

View File

@ -8,3 +8,5 @@ foo || bar;
0 || function () { 0 || function () {
return alpha; return alpha;
}; };
a ?? (b || c);

View File

@ -298,6 +298,31 @@ describe("generation", function() {
}); });
describe("programmatic generation", function() { describe("programmatic generation", function() {
it("should add parenthesis when NullishCoalescing is used along with ||", function() {
// 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`);
});
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)`);
});
it("numeric member expression", function() { it("numeric member expression", function() {
// Should not generate `0.foo` // Should not generate `0.foo`
const mem = t.memberExpression( const mem = t.memberExpression(

View File

@ -380,6 +380,39 @@ export default class ExpressionParser extends LValParser {
node.right = this.parseExprOpRightExpr(op, prec, noIn); 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`,
);
}
}
this.finishNode( this.finishNode(
node, node,
op === tt.logicalOR || op === tt.logicalOR ||

View File

@ -139,21 +139,21 @@ export const types: { [name: string]: TokenType } = {
tilde: new TokenType("~", { beforeExpr, prefix, startsExpr }), tilde: new TokenType("~", { beforeExpr, prefix, startsExpr }),
pipeline: createBinop("|>", 0), pipeline: createBinop("|>", 0),
nullishCoalescing: createBinop("??", 1), nullishCoalescing: createBinop("??", 1),
logicalOR: createBinop("||", 1), logicalOR: createBinop("||", 2),
logicalAND: createBinop("&&", 2), logicalAND: createBinop("&&", 3),
bitwiseOR: createBinop("|", 3), bitwiseOR: createBinop("|", 4),
bitwiseXOR: createBinop("^", 4), bitwiseXOR: createBinop("^", 5),
bitwiseAND: createBinop("&", 5), bitwiseAND: createBinop("&", 6),
equality: createBinop("==/!=/===/!==", 6), equality: createBinop("==/!=/===/!==", 7),
relational: createBinop("</>/<=/>=", 7), relational: createBinop("</>/<=/>=", 8),
bitShift: createBinop("<</>>/>>>", 8), bitShift: createBinop("<</>>/>>>", 9),
plusMin: new TokenType("+/-", { beforeExpr, binop: 9, prefix, startsExpr }), plusMin: new TokenType("+/-", { beforeExpr, binop: 10, prefix, startsExpr }),
modulo: createBinop("%", 10), modulo: createBinop("%", 11),
star: createBinop("*", 10), star: createBinop("*", 11),
slash: createBinop("/", 10), slash: createBinop("/", 11),
exponent: new TokenType("**", { exponent: new TokenType("**", {
beforeExpr, beforeExpr,
binop: 11, binop: 12,
rightAssociative: true, rightAssociative: true,
}), }),
@ -188,8 +188,8 @@ export const types: { [name: string]: TokenType } = {
_null: createKeyword("null", { startsExpr }), _null: createKeyword("null", { startsExpr }),
_true: createKeyword("true", { startsExpr }), _true: createKeyword("true", { startsExpr }),
_false: createKeyword("false", { startsExpr }), _false: createKeyword("false", { startsExpr }),
_in: createKeyword("in", { beforeExpr, binop: 7 }), _in: createKeyword("in", { beforeExpr, binop: 8 }),
_instanceof: createKeyword("instanceof", { beforeExpr, binop: 7 }), _instanceof: createKeyword("instanceof", { beforeExpr, binop: 8 }),
_typeof: createKeyword("typeof", { beforeExpr, prefix, startsExpr }), _typeof: createKeyword("typeof", { beforeExpr, prefix, startsExpr }),
_void: createKeyword("void", { beforeExpr, prefix, startsExpr }), _void: createKeyword("void", { beforeExpr, prefix, startsExpr }),
_delete: createKeyword("delete", { beforeExpr, prefix, startsExpr }), _delete: createKeyword("delete", { beforeExpr, prefix, startsExpr }),

View File

@ -457,7 +457,7 @@
"isAssign": false, "isAssign": false,
"prefix": true, "prefix": true,
"postfix": false, "postfix": false,
"binop": 9, "binop": 10,
"updateContext": null "updateContext": null
}, },
"value": "+", "value": "+",

View File

@ -1,7 +1,7 @@
{ {
"type": "File", "type": "File",
"start": 0, "start": 0,
"end": 12, "end": 14,
"loc": { "loc": {
"start": { "start": {
"line": 1, "line": 1,
@ -9,13 +9,13 @@
}, },
"end": { "end": {
"line": 1, "line": 1,
"column": 12 "column": 14
} }
}, },
"program": { "program": {
"type": "Program", "type": "Program",
"start": 0, "start": 0,
"end": 12, "end": 14,
"loc": { "loc": {
"start": { "start": {
"line": 1, "line": 1,
@ -23,7 +23,7 @@
}, },
"end": { "end": {
"line": 1, "line": 1,
"column": 12 "column": 14
} }
}, },
"sourceType": "script", "sourceType": "script",
@ -32,7 +32,7 @@
{ {
"type": "ExpressionStatement", "type": "ExpressionStatement",
"start": 0, "start": 0,
"end": 12, "end": 14,
"loc": { "loc": {
"start": { "start": {
"line": 1, "line": 1,
@ -40,13 +40,13 @@
}, },
"end": { "end": {
"line": 1, "line": 1,
"column": 12 "column": 14
} }
}, },
"expression": { "expression": {
"type": "LogicalExpression", "type": "LogicalExpression",
"start": 0, "start": 0,
"end": 11, "end": 13,
"loc": { "loc": {
"start": { "start": {
"line": 1, "line": 1,
@ -54,35 +54,35 @@
}, },
"end": { "end": {
"line": 1, "line": 1,
"column": 11 "column": 13
} }
}, },
"left": { "left": {
"type": "LogicalExpression", "type": "LogicalExpression",
"start": 0, "start": 1,
"end": 6, "end": 7,
"loc": { "loc": {
"start": { "start": {
"line": 1, "line": 1,
"column": 0 "column": 1
}, },
"end": { "end": {
"line": 1, "line": 1,
"column": 6 "column": 7
} }
}, },
"left": { "left": {
"type": "Identifier", "type": "Identifier",
"start": 0, "start": 1,
"end": 1, "end": 2,
"loc": { "loc": {
"start": { "start": {
"line": 1, "line": 1,
"column": 0 "column": 1
}, },
"end": { "end": {
"line": 1, "line": 1,
"column": 1 "column": 2
}, },
"identifierName": "a" "identifierName": "a"
}, },
@ -91,35 +91,39 @@
"operator": "&&", "operator": "&&",
"right": { "right": {
"type": "Identifier", "type": "Identifier",
"start": 5, "start": 6,
"end": 6, "end": 7,
"loc": { "loc": {
"start": { "start": {
"line": 1, "line": 1,
"column": 5 "column": 6
}, },
"end": { "end": {
"line": 1, "line": 1,
"column": 6 "column": 7
}, },
"identifierName": "b" "identifierName": "b"
}, },
"name": "b" "name": "b"
},
"extra": {
"parenthesized": true,
"parenStart": 0
} }
}, },
"operator": "??", "operator": "??",
"right": { "right": {
"type": "Identifier", "type": "Identifier",
"start": 10, "start": 12,
"end": 11, "end": 13,
"loc": { "loc": {
"start": { "start": {
"line": 1, "line": 1,
"column": 10 "column": 12
}, },
"end": { "end": {
"line": 1, "line": 1,
"column": 11 "column": 13
}, },
"identifierName": "c" "identifierName": "c"
}, },

View File

@ -0,0 +1,7 @@
{
"plugins": [
"nullishCoalescingOperator",
"estree"
],
"throws": "Nullish coalescing operator(??) requires parens when mixing with logical operators (1:0)"
}

View File

@ -0,0 +1,6 @@
{
"plugins": [
"nullishCoalescingOperator"
],
"throws": "Nullish coalescing operator(??) requires parens when mixing with logical operators (1:5)"
}

View File

@ -0,0 +1,6 @@
{
"plugins": [
"nullishCoalescingOperator"
],
"throws": "Nullish coalescing operator(??) requires parens when mixing with logical operators (1:10)"
}

View File

@ -0,0 +1,7 @@
{
"plugins": [
"nullishCoalescingOperator",
"estree"
],
"throws": "Nullish coalescing operator(??) requires parens when mixing with logical operators (1:0)"
}

View File

@ -1,7 +1,7 @@
{ {
"type": "File", "type": "File",
"start": 0, "start": 0,
"end": 12, "end": 14,
"loc": { "loc": {
"start": { "start": {
"line": 1, "line": 1,
@ -9,13 +9,13 @@
}, },
"end": { "end": {
"line": 1, "line": 1,
"column": 12 "column": 14
} }
}, },
"program": { "program": {
"type": "Program", "type": "Program",
"start": 0, "start": 0,
"end": 12, "end": 14,
"loc": { "loc": {
"start": { "start": {
"line": 1, "line": 1,
@ -23,7 +23,7 @@
}, },
"end": { "end": {
"line": 1, "line": 1,
"column": 12 "column": 14
} }
}, },
"sourceType": "script", "sourceType": "script",
@ -32,7 +32,7 @@
{ {
"type": "ExpressionStatement", "type": "ExpressionStatement",
"start": 0, "start": 0,
"end": 12, "end": 14,
"loc": { "loc": {
"start": { "start": {
"line": 1, "line": 1,
@ -40,13 +40,13 @@
}, },
"end": { "end": {
"line": 1, "line": 1,
"column": 12 "column": 14
} }
}, },
"expression": { "expression": {
"type": "LogicalExpression", "type": "LogicalExpression",
"start": 0, "start": 0,
"end": 11, "end": 13,
"loc": { "loc": {
"start": { "start": {
"line": 1, "line": 1,
@ -54,7 +54,7 @@
}, },
"end": { "end": {
"line": 1, "line": 1,
"column": 11 "column": 13
} }
}, },
"left": { "left": {
@ -77,30 +77,30 @@
"operator": "??", "operator": "??",
"right": { "right": {
"type": "LogicalExpression", "type": "LogicalExpression",
"start": 5, "start": 6,
"end": 11, "end": 12,
"loc": { "loc": {
"start": { "start": {
"line": 1, "line": 1,
"column": 5 "column": 6
}, },
"end": { "end": {
"line": 1, "line": 1,
"column": 11 "column": 12
} }
}, },
"left": { "left": {
"type": "Identifier", "type": "Identifier",
"start": 5, "start": 6,
"end": 6, "end": 7,
"loc": { "loc": {
"start": { "start": {
"line": 1, "line": 1,
"column": 5 "column": 6
}, },
"end": { "end": {
"line": 1, "line": 1,
"column": 6 "column": 7
}, },
"identifierName": "b" "identifierName": "b"
}, },
@ -109,20 +109,24 @@
"operator": "&&", "operator": "&&",
"right": { "right": {
"type": "Identifier", "type": "Identifier",
"start": 10, "start": 11,
"end": 11, "end": 12,
"loc": { "loc": {
"start": { "start": {
"line": 1, "line": 1,
"column": 10 "column": 11
}, },
"end": { "end": {
"line": 1, "line": 1,
"column": 11 "column": 12
}, },
"identifierName": "c" "identifierName": "c"
}, },
"name": "c" "name": "c"
},
"extra": {
"parenthesized": true,
"parenStart": 5
} }
} }
} }

View File

@ -1,7 +1,7 @@
{ {
"type": "File", "type": "File",
"start": 0, "start": 0,
"end": 12, "end": 14,
"loc": { "loc": {
"start": { "start": {
"line": 1, "line": 1,
@ -9,13 +9,13 @@
}, },
"end": { "end": {
"line": 1, "line": 1,
"column": 12 "column": 14
} }
}, },
"program": { "program": {
"type": "Program", "type": "Program",
"start": 0, "start": 0,
"end": 12, "end": 14,
"loc": { "loc": {
"start": { "start": {
"line": 1, "line": 1,
@ -23,7 +23,7 @@
}, },
"end": { "end": {
"line": 1, "line": 1,
"column": 12 "column": 14
} }
}, },
"sourceType": "script", "sourceType": "script",
@ -32,7 +32,7 @@
{ {
"type": "ExpressionStatement", "type": "ExpressionStatement",
"start": 0, "start": 0,
"end": 12, "end": 14,
"loc": { "loc": {
"start": { "start": {
"line": 1, "line": 1,
@ -40,13 +40,13 @@
}, },
"end": { "end": {
"line": 1, "line": 1,
"column": 12 "column": 14
} }
}, },
"expression": { "expression": {
"type": "LogicalExpression", "type": "LogicalExpression",
"start": 0, "start": 0,
"end": 11, "end": 13,
"loc": { "loc": {
"start": { "start": {
"line": 1, "line": 1,
@ -54,21 +54,7 @@
}, },
"end": { "end": {
"line": 1, "line": 1,
"column": 11 "column": 13
}
},
"left": {
"type": "LogicalExpression",
"start": 0,
"end": 6,
"loc": {
"start": {
"line": 1,
"column": 0
},
"end": {
"line": 1,
"column": 6
} }
}, },
"left": { "left": {
@ -90,40 +76,58 @@
}, },
"operator": "??", "operator": "??",
"right": { "right": {
"type": "Identifier", "type": "LogicalExpression",
"start": 5, "start": 6,
"end": 6, "end": 12,
"loc": { "loc": {
"start": { "start": {
"line": 1, "line": 1,
"column": 5 "column": 6
}, },
"end": { "end": {
"line": 1,
"column": 12
}
},
"left": {
"type": "Identifier",
"start": 6,
"end": 7,
"loc": {
"start": {
"line": 1, "line": 1,
"column": 6 "column": 6
}, },
"end": {
"line": 1,
"column": 7
},
"identifierName": "b" "identifierName": "b"
}, },
"name": "b" "name": "b"
}
}, },
"operator": "||", "operator": "||",
"right": { "right": {
"type": "Identifier", "type": "Identifier",
"start": 10, "start": 11,
"end": 11, "end": 12,
"loc": { "loc": {
"start": { "start": {
"line": 1, "line": 1,
"column": 10 "column": 11
}, },
"end": { "end": {
"line": 1, "line": 1,
"column": 11 "column": 12
}, },
"identifierName": "c" "identifierName": "c"
}, },
"name": "c" "name": "c"
},
"extra": {
"parenthesized": true,
"parenStart": 5
}
} }
} }
} }

View File

@ -1,7 +1,7 @@
{ {
"type": "File", "type": "File",
"start": 0, "start": 0,
"end": 12, "end": 14,
"loc": { "loc": {
"start": { "start": {
"line": 1, "line": 1,
@ -9,13 +9,13 @@
}, },
"end": { "end": {
"line": 1, "line": 1,
"column": 12 "column": 14
} }
}, },
"program": { "program": {
"type": "Program", "type": "Program",
"start": 0, "start": 0,
"end": 12, "end": 14,
"loc": { "loc": {
"start": { "start": {
"line": 1, "line": 1,
@ -23,7 +23,7 @@
}, },
"end": { "end": {
"line": 1, "line": 1,
"column": 12 "column": 14
} }
}, },
"sourceType": "script", "sourceType": "script",
@ -32,7 +32,7 @@
{ {
"type": "ExpressionStatement", "type": "ExpressionStatement",
"start": 0, "start": 0,
"end": 12, "end": 14,
"loc": { "loc": {
"start": { "start": {
"line": 1, "line": 1,
@ -40,13 +40,13 @@
}, },
"end": { "end": {
"line": 1, "line": 1,
"column": 12 "column": 14
} }
}, },
"expression": { "expression": {
"type": "LogicalExpression", "type": "LogicalExpression",
"start": 0, "start": 0,
"end": 11, "end": 13,
"loc": { "loc": {
"start": { "start": {
"line": 1, "line": 1,
@ -54,35 +54,35 @@
}, },
"end": { "end": {
"line": 1, "line": 1,
"column": 11 "column": 13
} }
}, },
"left": { "left": {
"type": "LogicalExpression", "type": "LogicalExpression",
"start": 0, "start": 1,
"end": 6, "end": 7,
"loc": { "loc": {
"start": { "start": {
"line": 1, "line": 1,
"column": 0 "column": 1
}, },
"end": { "end": {
"line": 1, "line": 1,
"column": 6 "column": 7
} }
}, },
"left": { "left": {
"type": "Identifier", "type": "Identifier",
"start": 0, "start": 1,
"end": 1, "end": 2,
"loc": { "loc": {
"start": { "start": {
"line": 1, "line": 1,
"column": 0 "column": 1
}, },
"end": { "end": {
"line": 1, "line": 1,
"column": 1 "column": 2
}, },
"identifierName": "a" "identifierName": "a"
}, },
@ -91,35 +91,39 @@
"operator": "||", "operator": "||",
"right": { "right": {
"type": "Identifier", "type": "Identifier",
"start": 5, "start": 6,
"end": 6, "end": 7,
"loc": { "loc": {
"start": { "start": {
"line": 1, "line": 1,
"column": 5 "column": 6
}, },
"end": { "end": {
"line": 1, "line": 1,
"column": 6 "column": 7
}, },
"identifierName": "b" "identifierName": "b"
}, },
"name": "b" "name": "b"
},
"extra": {
"parenthesized": true,
"parenStart": 0
} }
}, },
"operator": "??", "operator": "??",
"right": { "right": {
"type": "Identifier", "type": "Identifier",
"start": 10, "start": 12,
"end": 11, "end": 13,
"loc": { "loc": {
"start": { "start": {
"line": 1, "line": 1,
"column": 10 "column": 12
}, },
"end": { "end": {
"line": 1, "line": 1,
"column": 11 "column": 13
}, },
"identifierName": "c" "identifierName": "c"
}, },

View File

@ -0,0 +1,15 @@
import { types } from "../../../src/tokenizer/types";
describe("token types", () => {
it("should check if the binOp for relational === in", () => {
expect(types.relational.binop).toEqual(types._in.binop);
});
it("should check if the binOp for relational === instanceOf", () => {
expect(types.relational.binop).toEqual(types._instanceof.binop);
});
it("should check if the binOp for in === instanceOf", () => {
expect(types._in.binop).toEqual(types._instanceof.binop);
});
});

View File

@ -29,3 +29,5 @@ class_method_kinds/polymorphic_getter.js
ES6/modules/migrated_0020.js ES6/modules/migrated_0020.js
export_import_reserved_words/migrated_0003.js export_import_reserved_words/migrated_0003.js
export_statements/export_trailing_comma.js export_statements/export_trailing_comma.js
nullish_coalescing/precedence_and.js
nullish_coalescing/precedence_or.js