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

Parentheses removed falsely when transforming nullish-coalescing-operator #10966

Closed
dword-design opened this issue Jan 6, 2020 · 10 comments · Fixed by #11014
Closed

Parentheses removed falsely when transforming nullish-coalescing-operator #10966

dword-design opened this issue Jan 6, 2020 · 10 comments · Fixed by #11014

Comments

@dword-design
Copy link
Contributor

@dword-design dword-design commented Jan 6, 2020

Bug Report

Current Behavior
When transforming an AST containing a nullish coalescing operator, parentheses are removed falsely, leading to different behavior.

Input Code

const babel = require('@babel/core')
const { readFileSync, writeFileSync } = require('fs')

const code = readFileSync('src/index.js')
// src/index.js:
// const foo = 'test'
// console.log((foo ?? '') == '')

const babelConfig = {
  plugins: [
    '@babel/plugin-proposal-nullish-coalescing-operator',
  ],
}

// this works as expected
const ast = babel.parseSync(code, babelConfig)

// this removes the parentheses
const { code: transformedCode } = babel.transformFromAstSync(ast, code)

writeFileSync('dist/index.js', transformedCode)
// dist/index.js:
// const foo = 'test';
// console.log(foo ?? '' == '');

const babelRegister = require('@babel/register')
babelRegister(babelConfig)

require('./dist')
// outputs 'test' instead of false

See also:
https://github.com/dword-design/test-babel-missing-parentheses

Expected behavior/code
The resulting file should still contain the parentheses.

Environment

  • Babel version(s): 7.7.7
  • Node/npm version: Node 11.15.0/npm 6.7.0
  • OS: macOS 10.14.6
  • Monorepo: no
  • How you are using Babel: core, register
@babel-bot

This comment has been minimized.

Copy link
Collaborator

@babel-bot babel-bot commented Jan 6, 2020

Hey @dword-design! We really appreciate you taking the time to report an issue. The collaborators on this project attempt to help as many people as possible, but we're a limited number of volunteers, so it's possible this won't be addressed swiftly.

If you need any help, or just have general Babel or JavaScript questions, we have a vibrant Slack community that typically always has someone willing to help. You can sign-up here for an invite."

@sidntrivedi012

This comment has been minimized.

Copy link
Contributor

@sidntrivedi012 sidntrivedi012 commented Jan 8, 2020

Hey, I would like to work on it. Thanks.

@sidntrivedi012

This comment has been minimized.

Copy link
Contributor

@sidntrivedi012 sidntrivedi012 commented Jan 12, 2020

Hey, was working on this issue. Needed some help in that how do we decide precedence in https://github.com/babel/babel/blob/master/packages/babel-generator/src/node/parentheses.js ?

Also, how to find which function in parentheses.js implements operations on ?? operator ?

@nicolo-ribaudo

This comment has been minimized.

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented Jan 12, 2020

Its precedence should be the same as ?? or lower.

In the spec, they are represented at the same level: https://tc39.es/ecma262/#sec-binary-logical-operators

ShortCircuitExpression:
  LogicalORExpression
  CoalesceExpression

Also, how to find which function in parentheses.js implements operations on ?? operator ?

??, && and || are represented using a LogicalExpression node: you can see it on ASTExplorer. demo
You need to create a LogicalExpression function, which must return true when the node needs parentheses.

@sidntrivedi012

This comment has been minimized.

Copy link
Contributor

@sidntrivedi012 sidntrivedi012 commented Jan 13, 2020

Ok, working on it. Just wanted to ask if this is causing this issue? https://github.com/babel/babel/blob/master/packages/babel-generator/src/node/parentheses.js#L114-L126

@nicolo-ribaudo

This comment has been minimized.

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented Jan 13, 2020

Oh good catch! It seems that the problem is there.
Probably Binary is an alias for BinaryExpression and LogicalExpression.

@nicolo-ribaudo

This comment has been minimized.

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented Jan 13, 2020

But removing that isLogicalEexpression test would still leave some cases to fix.

@sidntrivedi012

This comment has been minimized.

Copy link
Contributor

@sidntrivedi012 sidntrivedi012 commented Jan 13, 2020

@nicolo-ribaudo so, should I remove the isLogicalExpression condition ?

@sidntrivedi012

This comment has been minimized.

Copy link
Contributor

@sidntrivedi012 sidntrivedi012 commented Jan 13, 2020

@nicolo-ribaudo

This comment has been minimized.

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented Jan 13, 2020

Oh right, we are already handling cases of mixed ?? and ||/&&. Then assigning the correct precedence might be enough.
Btw, if you have other questions please open a draft PR so that it's easier to see what you are doing!

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