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

[Bug]: Babel removes parenthesis at place where are required. #16111

Closed
1 task
gabrielgortabns opened this issue Nov 20, 2023 · 23 comments
Closed
1 task

[Bug]: Babel removes parenthesis at place where are required. #16111

gabrielgortabns opened this issue Nov 20, 2023 · 23 comments

Comments

@gabrielgortabns
Copy link

gabrielgortabns commented Nov 20, 2023

💻

  • Would you like to work on a fix?

How are you using Babel?

Programmatic API (babel.transform, babel.parse)

Input code

import something from 'something';

export function myfunction(bool) {
    return /** @type {SomeType} */ (
        bool ? something.a() : something.b()
    )
}

Configuration file name

No response

Configuration

No response

Current and expected behavior

const something = require('something');

exports.myfunction = myfunction;

function myfunction(bool) {
    return; /** @type {SomeType} */ 
        bool ? something.a() : something.b()
}

Environment

System:
OS: Windows 11 10.0.22621
Binaries:
Node: 18.6.0 - C:\Program Files\nodejs\node.EXE
npm: 8.15.0 - C:\Program Files\nodejs\npm.CMD
npmPackages:
@babel/cli: ^7.23.0 => 7.23.0
@babel/core: ^7.23.2 => 7.23.2
@babel/plugin-transform-modules-commonjs: ^7.23.3 => 7.23.3
@babel/preset-env: ^7.23.3 => 7.23.3
@babel/traverse: ^7.23.3 => 7.23.3
eslint: ^8.53.0 => 8.53.0
jest: ^29.7.0 => 29.7.0

Possible solution

No response

Additional context

The issue happens when there is /** @type {SomeType} */ in return statement, or in assigment. Causing either in invalid return, or even if code itself works, in output files, TypeScript throws error because of invalid types (as without parenthesis it doesn't apply type cast on whole expression).

reproduction in comment below:

#16111 (comment)

@babel-bot
Copy link
Collaborator

Hey @GabrielGorta! 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.

@nicolo-ribaudo
Copy link
Member

I'm releasing a patch right now that might fix this issue

@nicolo-ribaudo
Copy link
Member

#16104

@liuxingbaoyu
Copy link
Member

I'm worried this PR might not be helpful.
Also can you provide a way to reproduce it? I can't reproduce it in repl.

repl

@gabrielgortabns
Copy link
Author

Checking tests in PR #16104 and it really seems it will fix the issue.

@liuxingbaoyu
Copy link
Member

liuxingbaoyu commented Nov 20, 2023

That test isn't actually what's being fixed, but I'm concerned it's a regression.

Sorry for not stating this in the PR before.

@kungfooman
Copy link

Isn't this because createParenthesizedExpressions is false by default (for whatever reason, AST vs. CST story)?

I wanted to test this in REPL, but I can't find a way to set {createParenthesizedExpressions: true}. Is there any way to set parse options in REPL?

@liuxingbaoyu
Copy link
Member

Isn't this because createParenthesizedExpressions is false by default (for whatever reason, AST vs. CST story)?

I wanted to test this in REPL, but I can't find a way to set {createParenthesizedExpressions: true}. Is there any way to set parse options in REPL?

No, even this is unexpected when creatingParenthesizedExpressions: false. This is different in AST.

I wanted to test this in REPL, but I can't find a way to set {createParenthesizedExpressions: true}. Is there any way to set parse options in REPL?

Unfortunately there is currently no easy way.

@kungfooman
Copy link

Unfortunately there is currently no easy way.

Too bad, I would also like a "Show AST" option to quickly judge what's going on.

Checking tests in PR #16104 and it really seems it will fix the issue.

Try a single-line type cast comment, it doesn't work: REPL

Output:

function test(bool = false) {
  return /** @type {number} */bool ? 1 : 2;
}

@liuxingbaoyu
Copy link
Member

@kungfooman
Copy link

kungfooman commented Nov 20, 2023

@liuxingbaoyu Thank you, I mean integrated into the PR build, because some plugins or options could manipulate the AST (e.g. createParenthesizedExpressions)

@liuxingbaoyu
Copy link
Member

To be honest, I even doubt whether the plugins have good support for such an option. :)
I personally think that such an AST is more suitable for scenarios where only parser/generator are used.

@babel-bot
Copy link
Collaborator

Hi @GabrielGorta! This issue is missing some important information we'll need to be able to reproduce this issue.

Please understand that we receive a high volume of issues, and there are only a limited number of volunteers that help maintain this project. The easier it is for us to decipher an issue with the info provided, the more likely it is that we'll be able to help.

Please make sure you have the following information documented in this ticket:

  1. Your Babel configuration (typically from .babelrc or babel.config.js)
  2. The current (incorrect) behavior you're seeing
  3. The behavior you expect
  4. A short, self-contained example

Please provide either a link to the problem via the repl, or if the repl is insufficient, a new and minimal repository with instructions on how to build/replicate the issue.

@bjornharrtell
Copy link

This appears to be cause of openlayers/openlayers#15358. A reproduction can be found at https://github.com/bjornharrtell/ol-issue-15358.

@ahocevar
Copy link

ahocevar commented Nov 24, 2023

This appears to be cause of openlayers/openlayers#15358. A reproduction can be found at https://github.com/bjornharrtell/ol-issue-15358.

Looks like this is a regression in @babel/parser. Downgrading to @babel/parser@7.19.0 (by manually editing the lockfile) makes the problem go away:

diff --git a/package-lock.json b/package-lock.json
index c84d63d..3f3903c 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -115,7 +115,7 @@
         "@babel/helper-compilation-targets": "^7.22.15",
         "@babel/helper-module-transforms": "^7.23.3",
         "@babel/helpers": "^7.23.2",
-        "@babel/parser": "^7.23.3",
+        "@babel/parser": "^7.19.0",
         "@babel/template": "^7.22.15",
         "@babel/traverse": "^7.23.3",
         "@babel/types": "^7.23.3",
@@ -500,7 +500,7 @@
       }
     },
     "node_modules/@babel/parser": {
-      "version": "7.23.4",
+      "version": "7.19.0",
       "resolved": "https://registry.npmjs.org/@babel/parser/-/parser-7.23.4.tgz",
       "integrity": "sha512-vf3Xna6UEprW+7t6EtOmFpHNAuxw3xqPZghy+brsnusscJRW5BMUzzHZc5ICjULee81WeUV2jjakG09MDglJXQ==",
       "dev": true,
@@ -1849,7 +1849,7 @@
       "dev": true,
       "dependencies": {
         "@babel/code-frame": "^7.22.13",
-        "@babel/parser": "^7.22.15",
+        "@babel/parser": "^7.19.0",
         "@babel/types": "^7.22.15"
       },
       "engines": {
@@ -1868,7 +1868,7 @@
         "@babel/helper-function-name": "^7.23.0",
         "@babel/helper-hoist-variables": "^7.22.5",
         "@babel/helper-split-export-declaration": "^7.22.6",
-        "@babel/parser": "^7.23.4",
+        "@babel/parser": "^7.19.0",
         "@babel/types": "^7.23.4",
         "debug": "^4.1.0",
         "globals": "^11.1.0"

@gabrielgortabns
Copy link
Author

gabrielgortabns commented Nov 24, 2023

@bjornharrtell

Amazing job! I've just donwgraded the @babel/parser and now it works correctly at least in runtime (e.g. return statement, still type casts are lost). It's exactly same issue. Thank you @ahocevar too, for finding solution.

@kungfooman
Copy link

kungfooman commented Nov 24, 2023

@ahocevar I tried to reproduce the issue and it's kinda confusing:

mkdir babel_paren
cd babel_paren

Create file test.mjs:

import {parse} from '@babel/parser';
import generator from '@babel/generator';
const generate = generator.default;
const code = `
function getResolutionForValueFunction(power) {
  power = power || 2;
  const maxResolution = this.getConstrainedResolution(this.maxResolution_);
  const minResolution = this.minResolution_;
  const max = Math.log(maxResolution / minResolution) / Math.log(power);
  return (
    /**
     * @param {number} value Value.
     * @return {number} Resolution.
     */
    function (value) {
      const resolution = maxResolution / Math.pow(power, value * max);
      return resolution;
    }
  );
}
`;
const ast = parse(code);
const out = generate(ast,
  {
    compact: true,
    //createParenthesizedExpressions: true,
  }
);
console.log(out);

First run:

rm -rf node_modules package.json package-lock.json && npm i @babel/parser@7.23.3 @babel/generator@7.23.3 && node test.mjs
added 12 packages, and audited 13 packages in 737ms

found 0 vulnerabilities
{
  code: 'function getResolutionForValueFunction(power){power=power||2;const maxResolution=this.getConstrainedResolution(this.maxResolution_);const minResolution=this.minResolution_;const max=Math.log(maxResolution/minResolution)/Math.log(power);return(/**\n' +
    '     * @param {number} value Value.\n' +
    '     * @return {number} Resolution.\n' +
    '     */function(value){const resolution=maxResolution/Math.pow(power,value*max);return resolution;});}',
  decodedMap: undefined,
  __mergedMap: [Getter],
  map: [Getter/Setter],
  rawMappings: [Getter/Setter]
}

Nice parentheses, right?

But now run:

npm i @babel/generator && node test.mjs

Result

up to date, audited 13 packages in 439ms

found 0 vulnerabilities
{
  code: 'function getResolutionForValueFunction(power){power=power||2;const maxResolution=this.getConstrainedResolution(this.maxResolution_);const minResolution=this.minResolution_;const max=Math.log(maxResolution/minResolution)/Math.log(power);return/**\n' +
    '     * @param {number} value Value.\n' +
    '     * @return {number} Resolution.\n' +
    '     */function(value){const resolution=maxResolution/Math.pow(power,value*max);return resolution;};}',
  decodedMap: undefined,
  __mergedMap: [Getter],
  map: [Getter/Setter],
  rawMappings: [Getter/Setter]
}

Parentheses are gone! 👀

This only happens when using {compact: true} as generate option.

@liuxingbaoyu
Copy link
Member

liuxingbaoyu commented Nov 24, 2023

@ahocevar @kungfooman
Thank you for your detailed report!
I think there are two problems here.

#16111 (comment)
Regression from @babel/generator@7.23.4, I can reproduce it, will fix it.

And https://github.com/bjornharrtell/ol-issue-15358 is related to @babel/parser.
I get the error, but even if I downgrade @babel/parser it doesn't work.🤔

Suddenly it works fine and I'm investigating. 🤦‍♂️

@liuxingbaoyu
Copy link
Member

I don't understand why downgrading @babel/parer would work and I can't replicate this locally.
The bug in https://github.com/bjornharrtell/ol-issue-15358 disappeared after I downgraded @babel/generator, and it was caused by the merged PR a few days ago.

@bjornharrtell
Copy link

Confirmed that downgrading to @babel/generator@7.23.0 resolves https://github.com/bjornharrtell/ol-issue-15358. Note that downgrading to @babel/generator@7.23.3 does not appear to be enough.

@liuxingbaoyu
Copy link
Member

I downgraded it to @babel/generator@7.23.3 and it worked, this should be fixed soon.

@Doyler123
Copy link

Doyler123 commented Nov 30, 2023

FYI for anyone who might have started seeing the below error in their react application over the last week. This bug is the cause and it is fixed in @babel/generator@7.23.5

Error: Minified React error #152; visit https://reactjs.org/docs/error-decoder.html?invariant=152&args[]=f

@bjornharrtell
Copy link

Confirmed @babel/generator@7.23.5 resolves the issue and a lockfile regen/deps cleanup will bring that for me as transitive dependency.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants