Skip to content

Fix parentheses removal in nullish-coalescing operation#11014

Merged
nicolo-ribaudo merged 4 commits intobabel:masterfrom
sidntrivedi:fix-paranthesis-removal-nullish
Jan 15, 2020
Merged

Fix parentheses removal in nullish-coalescing operation#11014
nicolo-ribaudo merged 4 commits intobabel:masterfrom
sidntrivedi:fix-paranthesis-removal-nullish

Conversation

@sidntrivedi
Copy link
Copy Markdown
Contributor

@sidntrivedi sidntrivedi commented Jan 15, 2020

Q                       A
Fixed Issues? Fixes #10966
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

@sidntrivedi
Copy link
Copy Markdown
Contributor Author

sidntrivedi commented Jan 15, 2020

@nicolo-ribaudo Made a draft PR for working on #10966 . I tried adding precedence to ?? operator in the list but no success.

@sidntrivedi
Copy link
Copy Markdown
Contributor Author

@nicolo-ribaudo Its working. The problem was that I was not running make build or make watch for compiling the changes made to parentheses.js.

Also, I have assigned precedence 0 to it since https://tc39.es/ecma262/#sec-binary-logical-operators mentions Logical OR and ?? at same precedence. PTAL. Thanks. 🙂

@@ -2,6 +2,7 @@ import * as t from "@babel/types";

const PRECEDENCE = {
"||": 0,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

While it's unobservable now, ?? should actually be the lowest precedence of all tokens. Meaning we'll need to increase every other token's precedence.

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.

Ok, so I am assigning a precedence of 0 to ?? and will increment that of others by 1. BTW, I am still not clear as to how to get to know the precedence that we follow here.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Oh, actually, you're right. SortCircuitExpression is both CoalesceExpression and LogicalORExpression, so they have the same precedence. I was remember when we had the spec using grammar flags, and there it was CoalesceExpression -> LogicalORExpression.

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.

Oh, I had already made the changes. 😅 Reverting them.

@@ -0,0 +1,2 @@
const foo = 'test';
console.log(foo ?? '' == ''); No newline at end of file
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Don't forget to update the test

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.

Oops. My bad. Making the changes.

@existentialism existentialism added PR: Bug Fix 🐛 A type of pull request used for our changelog categories pkg: generator Spec: Nullish Coalescing Operator labels Jan 15, 2020
@sidntrivedi sidntrivedi force-pushed the fix-paranthesis-removal-nullish branch from 38bf8f0 to 70da59b Compare January 15, 2020 20:10
@sidntrivedi
Copy link
Copy Markdown
Contributor Author

@jridgewell Made the changes. PTAL. Thanks.

Copy link
Copy Markdown
Contributor

@JLHwung JLHwung left a comment

Choose a reason for hiding this comment

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

Thanks!

@existentialism existentialism changed the title [DRAFT]Fix paranthesis removal in nullish-coalescing operation Fix parentheses removal in nullish-coalescing operation Jan 15, 2020
Copy link
Copy Markdown
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

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

Thanks for this PR!

BTW, I am still not clear as to how to get to know the precedence that we follow here.

You can check the correct precedence in the spec.
Consider this example (directly from the https://tc39.es/ecma262).

AdditiveExpression:
   MultiplicativeExpression
   AdditiveExpression + MultiplicativeExpression
   AdditiveExpression - MultiplicativeExpression

MultiplicativeExpression:
  ExponentiationExpression
  MultiplicativeExpression MultiplicativeOperator ExponentiationExpression

MultiplicativeOperator: one of
  * / %

ExponentiationExpression:
  UnaryExpression
  UpdateExpression  **  ExponentiationExpression

You have to trust me about this: the spec starts "parsing" from AdditiveExpression. If you want, you can look for the Expression production in the spec and try to understand why.

Now, suppose that we are trying to parse the following expression: a * b + c + d * e.

We first look for an AdditiveExpression. It can start with another AdditiveExpression, but it will ultimately go down and look for a MultiplicativeExpression.
We start parsing the nested MultiplicativeExpression, and go down to ExponentiationExpression, which then goes down to UnaryExpression. Trust me or try to read the spec, but both UnaryExpression and UpdateExpression go down to Identfier and parse a.

At this point, we start going "up" and go back to ExponentiationExpression: we already have something matched by either UnaryExpression or UpdateExpression. We check if there is **: there isn't, so we "return" a to MultiplicativeExpression. Then, MultiplicativeExpression either parses one of *, /, or %, or "returns up" to AdditiveExpression. There is the * character, so it consumes it and then parser another ExponentiationExpression: we now have a * b.

Now, we have a MultiplicativeExpression: it can either be the left operand of another MultiplicativeExpression, or "return up" to AdditiveExpression. We don't have one of *, /, or % anymore, so it returns.

The MultiplicativeExpression is now used as the left operand of the AdditiveExpression, because there is the + token. We have MultiplicativeExpression + ((a * b) + ), and need to look for its right operand which is specified as a MultiplicativeExpression.

We go down and parse the c identifier, and then go back in the grammar productions up to MultiplicativeExpression. We don't have one of * / %, so it "returns" c as the right operand of the AdditiveExpression: we now have (a * b) + c.

This AdditiveExpression can either be finished or be the left operand of another AdditiveExpression. The parser finds the + character, so it falls into the second case. We now have ((a * b) + c) +, and start parsing a MultiplicativeExpression: following the same logic as before, the MultiplicativeExpression is (d * e).

(d * e) is returned to the main AdditiveExpression: we now have ((a * b) + c) + (d * e).

You can see that * has an higher precedence than +, because when there aren't parentheses * is parsed exactly as if it was wrapped.

I also copied the ExponentiationExpression production from the spec because it gives us the opportunity to see another property of binary operators: their associativity. In this case, AdditiveExpression and MultiplicativeExpression are left-associative, while ExponentiationExpression is right-associative.
I leave to you to understand why 😉

@nicolo-ribaudo nicolo-ribaudo merged commit 6ad7e19 into babel:master Jan 15, 2020
@sidntrivedi
Copy link
Copy Markdown
Contributor Author

@nicolo-ribaudo Thanks a lot for such a detailed explanation. I was confused earlier but not now. And yes, I fully trust your words, no need to read the spec. 😉

@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 17, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 17, 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: generator PR: Bug Fix 🐛 A type of pull request used for our changelog categories Spec: Nullish Coalescing Operator

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Parentheses removed falsely when transforming nullish-coalescing-operator

5 participants