Skip to content

Update coalesce precedence#11017

Merged
nicolo-ribaudo merged 3 commits intobabel:masterfrom
JLHwung:update-coalesce-precedence
Jan 17, 2020
Merged

Update coalesce precedence#11017
nicolo-ribaudo merged 3 commits intobabel:masterfrom
JLHwung:update-coalesce-precedence

Conversation

@JLHwung
Copy link
Copy Markdown
Contributor

@JLHwung JLHwung commented Jan 16, 2020

Q                       A
Fixed Issues? Coalesce should have same precedence with LogicalOR
Patch: Bug Fix? Precedence of coalesce is now equal to logical-or.
Tests Added + Pass? Yes
License MIT

The first commit is part of my holiday parser refactor series. The new checking implements as @KFlash suggested in https://github.com/babel/babel/pull/10269/files/40eb40e34b689096d6a5921c63577904a187d4be#r313937603

The second commit updates the precedence to be align with the spec: https://tc39.es/ecma262/#prod-ShortCircuitExpression

@JLHwung JLHwung added PR: Spec Compliance 👓 A type of pull request used for our changelog categories pkg: parser labels Jan 16, 2020
nicolo-ribaudo
nicolo-ribaudo previously approved these changes Jan 16, 2020
@nicolo-ribaudo nicolo-ribaudo dismissed their stale review January 16, 2020 10:17

The Flow failure is related to this PR

@@ -139,22 +139,22 @@ export const types: { [name: string]: TokenType } = {
tilde: new TokenType("~", { beforeExpr, prefix, startsExpr }),
pipeline: createBinop("|>", 0),
nullishCoalescing: createBinop("??", 1),
Copy link
Copy Markdown

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
Copy Markdown
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.

@JLHwung JLHwung force-pushed the update-coalesce-precedence branch from 9c3685f to 87e058e Compare January 16, 2020 15:50
@JLHwung JLHwung force-pushed the update-coalesce-precedence branch from 87e058e to 6b2c9e8 Compare January 17, 2020 14:52
@nicolo-ribaudo nicolo-ribaudo merged commit 45301c5 into babel:master Jan 17, 2020
@nicolo-ribaudo nicolo-ribaudo deleted the update-coalesce-precedence branch January 17, 2020 19:57
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Apr 18, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: parser PR: Spec Compliance 👓 A type of pull request used for our changelog categories

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants