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

NullishCoalescing forbids mixing with LogicalExpressions without parenthesis #10260

Closed
jridgewell opened this issue Jul 24, 2019 · 19 comments · Fixed by #10269

Comments

@jridgewell
Copy link
Member

commented Jul 24, 2019

Bug Report

Current Behavior
The nullish coalescing operator (??) currently allows mixing with the other logical operators (|| and &&) without parenthesis. This is invalid per the latest specs, parenthesis are required to mix them:

// Currently valid but should be a syntax error
a ?? b || c

// Currently valid but should be a syntax error
a || b ?? c

// Currently valid but should be a syntax error
a ?? b && c

// Currently valid but should be a syntax error
a && b ?? c

https://babeljs.io/repl/#?babili=false&browsers=Safari%2011&build=&builtIns=false&spec=false&loose=false&code_lz=PTAEGEFcCdoUwHYBcA2BPUA3AhiglgCagBGkSoAzgBYD2kKRxco2lay2AHqHLDdAChWAfmElQAHwmgAxgIEgIMeMnRZchEmUq16jZqwrskXHn0Gsp40bPmKosRKgw58jbdToMSBth2680PxCoDbEoABkEbYKYA4qzupuWuSeej4sfiYB5iFR1mJyQA&debug=false&forceAllTransforms=false&shippedProposals=false&circleciRepo=&evaluate=true&fileSize=false&timeTravel=false&sourceType=module&lineWrap=true&presets=&prettier=true&targets=&version=7.5.5&externalPlugins=%40babel%2Fplugin-proposal-nullish-coalescing-operator%407.4.4

Expected behavior/code
These should each throw a syntax error when parsing. It requires parens to disambiguate the intended grouping.

// Valid according to spec
a ?? (b || c)

// Valid according to spec
(a ?? b) || c

// Valid according to spec
(a || b) ?? c

// Valid according to spec
a || (b ?? c)

// Valid according to spec
a ?? (b && c)

// Valid according to spec
(a ?? b) && c

// Valid according to spec
(a && b) ?? c

// Valid according to spec
a && (b ?? c)

Babel Configuration (.babelrc, package.json, cli command)

{
  "plugins": ["proposal-nullish-coalescing-operator"]
}

Environment

  • Babel version(s): 7

Possible Solution
These should throw during parsing. Somewhere around

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

Additionally, when printing the AST, the nullish coalescing operator should print parenthesis around the left/right hand side if they are the other logical operators. Somewhere around

AssignmentExpression as LogicalExpression,
.

@vivek12345

This comment has been minimized.

Copy link
Contributor

commented Jul 24, 2019

Can someone who has not contributed previously to Babel take this up?

@jridgewell

This comment has been minimized.

Copy link
Member Author

commented Jul 24, 2019

Yes!

@dhruvdutt

This comment has been minimized.

Copy link

commented Jul 24, 2019

Can I pick this up?

@vivek12345

This comment has been minimized.

Copy link
Contributor

commented Jul 24, 2019

@dhruvdutt I have already started looking into it.

@jridgewell

This comment has been minimized.

Copy link
Member Author

commented Jul 24, 2019

I think this goes to @vivek12345 first, but you're welcome to collaborate with them on a PR?

@vivek12345

This comment has been minimized.

Copy link
Contributor

commented Jul 24, 2019

@jridgewell Should I be checking in the parser if A) if op === tt.nullishCoalescing and B) check if we have other logical operators around it and if yes then throw raise an error.
Am I thinking in the right direction here?

@jridgewell

This comment has been minimized.

Copy link
Member Author

commented Jul 24, 2019

You're on the right track, but we only raise an error if the other operators are unparenthesized. (a || b) ?? c is legal. Also note that we need to raise an error for both a ?? b || c and a || b ?? c.

@vivek12345

This comment has been minimized.

Copy link
Contributor

commented Jul 24, 2019

Understood. Also a ?? b ?? c this would be valid right? This won't need a parenthesis around.

@vivek12345

This comment has been minimized.

Copy link
Contributor

commented Jul 24, 2019

@jridgewell while printing the AST with parenthesis, I see AssignmentExpression being exported as LogicalExpression and inside AssignmentExpression I see a check for parenthesis needed and then accordingly it is adding a new token ( before left node and then a ) after right.

In our case this would slightly change and we will have to check if we need this parenthesis or not and then also check where it should be placed: - right or left.

So the current paren condition inside AssignmentExpression would not suffice our requirement.

Am I right in understanding this?

@vivek12345

This comment has been minimized.

Copy link
Contributor

commented Jul 24, 2019

Also I am trying to understand why would I need this change in the printing AST part. If I am not allowing an expression like a ?? b && c without the parenthesis and only allowing it with the parenthesis like this a ?? ( b && c ) so will babel-generate not automatically use the token ( specified in the expression while constructing the tree.

Why do we have to explicitly add it?

@jridgewell

This comment has been minimized.

Copy link
Member Author

commented Jul 24, 2019

Also a ?? b ?? c this would be valid right? This won't need a parenthesis around.

Correct.

I see AssignmentExpression being exported as LogicalExpression and inside AssignmentExpression I see a check for parenthesis needed and then accordingly it is adding a new token ( before left node and then a ) after right.

It's probably time we made an explicit function for LogicalExpression, since it's not more complicated.

In our case this would slightly change and we will have to check if we need this parenthesis or not and then also check where it should be placed: - right or left.

Correct.

Why do we have to explicitly add it?

Because it's possible to construct the AST by hand, and our printer should be smart enough to know that parenthesis are required:

See https://astexplorer.net/#/gist/a896ac99208d75fac59b9924facbdf7e/20c49836c532c2db184df733f79d76d69d0cb476

@vivek12345

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2019

@jridgewell I am a bit stuck at it now. My initial approach that I thought was to check

if (op === tt.nullishCoalescing) {
    if (
        left.type === "LogicalExpression" &&
        left.operator !== "??" &&
        !(left.extra && left.extra.parenthesized)
    ) {
        throw this.raise(
            `Wrap left hand side of the expression in parentheses`,
        );
    }

and then do the same check for node.right but it seems the evaluation is left to right so even if right has a logical expression like let's say
a ?? c || d

Here a ?? c is considered as left and d is considered as right.

I assumed that a would be considered asleft and c || d would be a logical expression and with no paren the right one and then I could just say wrap the right hand side in parenthesis.
This assumption was not right and I am confused as to do I have to do some recursive checks here?

Can you help me a bit here?

@vivek12345

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2019

For something like this

a ?? b && c || d

What should be the right parenthesis ordering?

@vivek12345

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2019

Okay I have made some improvement.
@jridgewell Do you think this is in the right direction?

 // for || we need to check only left as in a ?? b || c, a ?? b is considered a logical expression starting from left and the c the identiier
 if (op === tt.logicalOR) {
    if (
      left.type === "LogicalExpression" &&
      left.operator === "??" &&
      !(left.extra && left.extra.parenthesized)
    ) {
        throw this.raise(
          left.start,
          `Wrap left hand side of the expression in parentheses`,
        );
     }
 }
 // for ?? we need to check both left and right
if (op === tt.nullishCoalescing) {
  if (
    left.type === "LogicalExpression" &&
    left.operator !== "??" &&
    !(left.extra && left.extra.parenthesized)
  ) {
    throw this.raise(
      left.start,
      `Wrap left hand side of the expression in parentheses`,
    );
  } else if (
    node.right.type === "LogicalExpression" &&
    node.right.operator !== "??" &&
    !(node.right.extra && node.right.extra.parenthesized)
  ) {
    throw this.raise(
      node.right.start,
      `Wrap right hand side of the expression in parentheses.`,
    );
  }
}
@jridgewell

This comment has been minimized.

Copy link
Member Author

commented Jul 25, 2019

Can you open a PR (even if it’s not working 100%)? It’ll be easier for me to review and comment that way.

@vivek12345

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2019

@jridgewell the repl is ready. Circle CI passed but CI failed due to JOB=babel-parser-flow-tests. Not really sure on why that failed.

@tanhauhau

This comment has been minimized.

Copy link
Member

commented Jul 26, 2019

@vivek12345 the flow parser test case failed.

we borrowed the test case for flow parser from here.

for example in the ci log, it mentioned babel throw and error for the nullish_coalescing/precedence_and test case, but flow parsed the code into this ast, a passed case for them, but apparently this MR change will break this test case.

if according to the spec, they should fix their test case, we should raised an issue to them.

temporarily, we can whitelist the test case here for test cases that has discrepancy with flow's parser

@jridgewell

This comment has been minimized.

Copy link
Member Author

commented Jul 26, 2019

I pushed a commit to whitelist the tests for now.

@vivek12345

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2019

@tanhauhau and @jridgewell I have raised a bug on flow parser for the same issue.
facebook/flow#7972

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.