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

Update coalesce precedence #11017

Merged
merged 3 commits into from Jan 17, 2020
Merged

Update coalesce precedence #11017

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7761d52
refactor: reimplement nullish coalescing precedence tracking
JLHwung Dec 29, 2019
ddc7604
fix: Coalesce has same precedence with LogicalOR
JLHwung Jan 16, 2020
6b2c9e8
fix flow errors
JLHwung Jan 16, 2020
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
61 changes: 22 additions & 39 deletions packages/babel-parser/src/parser/expression.js
View file
Open in desktop
Expand Up @@ -320,7 +320,7 @@ export default class ExpressionParser extends LValParser {
minPrec: number,
noIn: ?boolean,
): N.Expression {
const prec = this.state.type.binop;
let prec = this.state.type.binop;
if (prec != null && (!noIn || !this.match(tt._in))) {
if (prec > minPrec) {
const operator = this.state.value;
Expand All @@ -343,11 +343,17 @@ export default class ExpressionParser extends LValParser {
}

const op = this.state.type;
const logical = op === tt.logicalOR || op === tt.logicalAND;
const coalesce = op === tt.nullishCoalescing;

if (op === tt.pipeline) {
this.expectPlugin("pipelineOperator");
this.state.inPipeline = true;
this.checkPipelineAtInfixOperator(left, leftStartPos);
} else if (coalesce) {
// Handle the precedence of `tt.coalesce` as equal to the range of logical expressions.
// In other words, `node.right` shouldn't contain logical expressions in order to check the mixed error.
prec = ((tt.logicalAND: any): { binop: number }).binop;
}

this.next();
Expand All @@ -369,49 +375,26 @@ export default class ExpressionParser extends LValParser {
}

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

this.finishNode(
node,
logical || coalesce ? "LogicalExpression" : "BinaryExpression",
);
/* 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 => identifier
* 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
* when op is coalesce and nextOp is logical (&&), throw at the pos of nextOp that it can not be mixed.
* Symmetrically it also throws when op is logical and nextOp is coalesce
*/
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`,
);
}
const nextOp = this.state.type;
if (
(coalesce && (nextOp === tt.logicalOR || nextOp === tt.logicalAND)) ||
(logical && nextOp === tt.nullishCoalescing)
) {
throw this.raise(
this.state.start,
`Nullish coalescing operator(??) requires parens when mixing with logical operators`,
);
}

this.finishNode(
node,
op === tt.logicalOR ||
op === tt.logicalAND ||
op === tt.nullishCoalescing
? "LogicalExpression"
: "BinaryExpression",
);

return this.parseExprOp(
node,
leftStartPos,
Expand Down
30 changes: 15 additions & 15 deletions packages/babel-parser/src/tokenizer/types.js
View file
Open in desktop
Expand Up @@ -139,22 +139,22 @@ export const types: { [name: string]: TokenType } = {
tilde: new TokenType("~", { beforeExpr, prefix, startsExpr }),
pipeline: createBinop("|>", 0),
nullishCoalescing: createBinop("??", 1),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JLHwung If you follow the specs the precedence values is wrong :) The '??' should have lowest value.

There are a lot of docs and issue tickets about this, but here you can see it clearly

https://github.com/tc39/proposal-nullish-coalescing/blob/80a589d657c322fd2e152c1936bd9704904b2068/spec.html#L14-L17

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That precedence statement does not follow from the grammar notation in the spec

ShortCircuitExpression[In, Yield, Await]:
  LogicalORExpression[?In, ?Yield, ?Await]
  CoalesceExpression[?In, ?Yield, ?Await]

That statement was updated in tc39/proposal-nullish-coalescing@99ff664 where a NullishExpression was introduced

NullishExpression[In, Yield, Await, Nullish] :
      LogicalORExpression[?In, ?Yield, ?Await, ?Nullish]
      NullishExpression[?In, ?Yield, ?Await, +Nullish] `??` LogicalORExpression[?In, ?Yield, ?Await, +Nullish]

apparently here ?? is lower than || in this notation. But later it was changed to ShortCircuitExpression, where

  1. the RHS of CoalesceExpression is either CoalesceExpression or BitwiseORExpression.
  2. the RHS of LogicalORExpression does not contain CoalesceExpression

So the grammar itself does not conclude that the precedence of coalesce is higher or lower than the logical_or. The only thing we know is that

both coalesce and logical_or are lower than bit_or. (f.1)
logical_or is lower than logical_and. (f.2)

Since the precedence of coalesce and logical_or is meant to solve the error of mixing coalesce and logical operators and now we throw when they are mixed, I don't think the exact precedence of coalesce and logical_or will have any impact as long as the precedence specification satisfies (f.1) and (f.2).

I can think of one advantage if you specify the precedence as coalesce: 1, or: 2, and: 3: you can now check if logical is mixed with coalesce by

(op.binop >> 1) ^ (nextOp.binop >> 1) === 1

This is similar to what is done in seafox but I think here code readability weighs more than performance, unless we do identify that parsing coalesce is the bottleneck.

logicalOR: createBinop("||", 2),
logicalAND: createBinop("&&", 3),
bitwiseOR: createBinop("|", 4),
bitwiseXOR: createBinop("^", 5),
bitwiseAND: createBinop("&", 6),
equality: createBinop("==/!=/===/!==", 7),
relational: createBinop("</>/<=/>=", 8),
bitShift: createBinop("<</>>/>>>", 9),
plusMin: new TokenType("+/-", { beforeExpr, binop: 10, prefix, startsExpr }),
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 }),
// startsExpr: required by v8intrinsic plugin
modulo: new TokenType("%", { beforeExpr, binop: 11, startsExpr }),
star: createBinop("*", 11),
slash: createBinop("/", 11),
modulo: new TokenType("%", { beforeExpr, binop: 10, startsExpr }),
star: createBinop("*", 10),
slash: createBinop("/", 10),
exponent: new TokenType("**", {
beforeExpr,
binop: 12,
binop: 11,
rightAssociative: true,
}),

Expand Down Expand Up @@ -189,8 +189,8 @@ export const types: { [name: string]: TokenType } = {
_null: createKeyword("null", { startsExpr }),
_true: createKeyword("true", { startsExpr }),
_false: createKeyword("false", { startsExpr }),
_in: createKeyword("in", { beforeExpr, binop: 8 }),
_instanceof: createKeyword("instanceof", { beforeExpr, binop: 8 }),
_in: createKeyword("in", { beforeExpr, binop: 7 }),
_instanceof: createKeyword("instanceof", { beforeExpr, binop: 7 }),
_typeof: createKeyword("typeof", { beforeExpr, prefix, startsExpr }),
_void: createKeyword("void", { beforeExpr, prefix, startsExpr }),
_delete: createKeyword("delete", { beforeExpr, prefix, startsExpr }),
Expand Down
2 changes: 1 addition & 1 deletion packages/babel-parser/test/fixtures/core/opts/tokens-true/output.json
View file
Open in desktop
Expand Up @@ -457,7 +457,7 @@
"isAssign": false,
"prefix": true,
"postfix": false,
"binop": 10,
"binop": 9,
"updateContext": null
},
"value": "+",
Expand Down
2 changes: 1 addition & 1 deletion ...parser/test/fixtures/es2020/nullish-coalescing-operator/no-paren-and-nullish/options.json
View file
Open in desktop
@@ -1,4 +1,4 @@
{
"plugins": ["estree"],
"throws": "Nullish coalescing operator(??) requires parens when mixing with logical operators (1:0)"
"throws": "Nullish coalescing operator(??) requires parens when mixing with logical operators (1:7)"
}
2 changes: 1 addition & 1 deletion ...parser/test/fixtures/es2020/nullish-coalescing-operator/no-paren-nullish-and/options.json
View file
Open in desktop
@@ -1,3 +1,3 @@
{
"throws": "Nullish coalescing operator(??) requires parens when mixing with logical operators (1:5)"
"throws": "Nullish coalescing operator(??) requires parens when mixing with logical operators (1:7)"
}
2 changes: 1 addition & 1 deletion ...-parser/test/fixtures/es2020/nullish-coalescing-operator/no-paren-nullish-or/options.json
View file
Open in desktop
@@ -1,3 +1,3 @@
{
"throws": "Nullish coalescing operator(??) requires parens when mixing with logical operators (1:10)"
"throws": "Nullish coalescing operator(??) requires parens when mixing with logical operators (1:12)"
}
2 changes: 1 addition & 1 deletion ...-parser/test/fixtures/es2020/nullish-coalescing-operator/no-paren-or-nullish/options.json
View file
Open in desktop
@@ -1,4 +1,4 @@
{
"plugins": ["estree"],
"throws": "Nullish coalescing operator(??) requires parens when mixing with logical operators (1:0)"
"throws": "Nullish coalescing operator(??) requires parens when mixing with logical operators (1:7)"
}