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

BABEL_TRANSFORM_ERROR - babel traverse context - cannot read property "removeBinding" of null #12383

Closed
salvoravida opened this issue Nov 22, 2020 · 22 comments · Fixed by #12387
Assignees
Labels
i: bug i: regression outdated A closed issue/PR that is archived due to age. Recommended to make a new issue

Comments

@salvoravida
Copy link

salvoravida commented Nov 22, 2020

Bug Report

EDIT by @nicolo-ribaudo

You can workaround this bug by making sure that @babel/core uses an older @babel/traverse version.
If you are using yarn you can specify "@babel/traverse": "7.12.0" in resolutions, otherwise you can try downgrading it as described by #12383 (comment).


#12331 after this pr

"This is technically a breaking change (we are changing the default value of a parameter in the public API), but I couldn't find anyone online using that parameter and I'm not sure why anyway would not wont it to use the correct context."

is there anyone that do code review, here @babel ?

What do you think about semver?

image

@babel-bot
Copy link
Collaborator

Hey @salvoravida! 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

nicolo-ribaudo commented Nov 23, 2020

The goal of #12331 was to fix problems really similar to the one reported in your stack trace.

Plugins can optimize AST traversal by passing a noScope: true option, which marks the internal scope tracker as not needed and sets the internal scope tracker to null.

The problem (fixed by #12331) was that sometimes an old value of noScope was used, and if a plugin run with noScope: true and then a different plugin run with noScope: false, the second one didn't get a scope tracker instance causing errors such as Cannot read property removeBinding of null.

The "technically a breaking change" introduced by that PR is that now the noScope option is not ignored, so if a plugin sets noScope: true but then tries to use the scope tracker it will now throw an error (usually noScope: true was ignored before that PR). So the actual breaking change that I know of (and that I knew of when I wrote that description) is... respecting the value explicitly set by an option.

I'm thus surprised by this error, but yeah we should fix the regression.

However, we'll need your help to do it. When opening a new issue in this repository, GitHub gives you a form similar to this one:

Schermata da 2020-11-23 02-21-57

As you can see, there are some predefined sections:

  • Current behavior: ✔️ (this is the stack trace you posted)
  • Input Code: ❌
  • Expected behavior: ✔️ (not crashing)
  • Babel Configuration: ❌

Without the missing information (marked with ❌), we cannot fix this issue.

  • If we just revert that PR, we don't have any guarantee that it actually fixes the error.
  • If we are lucky and it fixes the error, we still cannot write a test for it. This makes it quite likely that we'll break it again when fixing in another way the bug that Use the correct context when re-using a cached NodePath #12331 was originally resolving.

I understand that depending on your employer's policy it might be impossible for you to just give us the full contents of CustomDatePicker.js. You can comment out pieces of that file until the error goes away, and then just sharing the minimal portion of code needed to reproduce the crash.


PS. (I'm replying here to #12331 (comment))

You are breaking many production CI build

I strongly recommend using a lockfile package-lock.json or yarn.lock to avoid automatic dependencies upgrades. In this case your CI is failing, but in general "automatically downloading new code and executing it" is not a good idea from a security perspective.

@ehoogeveen
Copy link

ehoogeveen commented Nov 23, 2020

I think babel-plugin-minify-simplify is triggering this error (whether the fault is with the plugin or babel itself I don't know). It can be seen when minifying bowser v1.9.4 for example.

To reproduce:

  1. Using node.js LTS v14.15.1 with npm version 6.14.8, set up a clean node project.
  2. Run npm install --save-dev @babel/cli @babel/core babel-preset-minify bowser@1.9.4
  3. Run npx babel --presets=babel-preset-minify node_modules/bowser/bowser.js

@salvoravida

This comment has been minimized.

@stefaniapedrazzi
Copy link

stefaniapedrazzi commented Nov 23, 2020

We are also having issues due to this change and our CI is failing since then.

As far as I understand, but maybe it is wrong, there are conflicts with the babel minify presets that we solved by using the --presets "@babel/preset-env" starting option.
Unfortunately this trick is no longer working and it throws the error mentioned in the issue description.

I understand that it could take a while to check and fix it.
But it would be very welcome if you could suggest a workaround.

@ehoogeveen
Copy link

A workaround that mostly worked for me was to disable the minify-simplify plugin by adding ["minify", { simplify: false }] to the preset options. Unfortunately I then got a "Maximum call stack size exceeded" error when minifying d3 v4.13.0 (but I haven't investigated that yet).

@nicolo-ribaudo
Copy link
Member

Thanks @ehoogeveen for the example and workaround! I will investigate a fix today, in the meantime you can add @babel/traverse@7.12.0 to your devDependencies, uninstall @babel/core and reinstall it (so that it uses the older @babel/traverse version).

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Nov 23, 2020

I trimmed down the reproduction example to just two plugins: babel-plugin-transform-merge-sibling-variables and babel-plugin-minify-simplify. As an alternative to ["minify", { simplify: false }], a third workaround could be ["minify", { mergeVars: false }].

EDIT: In case someone is interested, this is the minimal amount of code to trigger the bug:

function compareVersions() {
  var precision = 0;
  var chunks = precision;
  while (precision) {}
}

and the problem uncovered by #12331 is that they try to do two different things with the chunks variable. transform-merge-sibling-vairables tries to generate

function compareVersions() {
  var precision = 0,
      chunks = precision;

  while (precision) {}
}

while minify-simplify tries to generate

function compareVersions() {
  var precision = 0;

  for (var chunks = precision; precision;);
}

Somehow one of them is getting a stale AST node causing problems (but this is not a bug with babel-minify, since @babel/traverse should prevent it).

@nicolo-ribaudo nicolo-ribaudo self-assigned this Nov 23, 2020
@w3nda
Copy link

w3nda commented Nov 23, 2020

Having our CI build failing as well with the very same issue, without explicitly using any of the packages you mentioned here ...
is there any ETA for this issue?

I can hardly tell which workaround could be used in my case, here are the babel packages we use

"@babel/cli": "^7.0.0"
"@babel/core": "^7.0.0"
"@babel/plugin-proposal-class-properties": "^7.0.0"
"@babel/plugin-proposal-object-rest-spread": "^7.0.0"
"@babel/plugin-syntax-dynamic-import": "^7.0.0"
"@babel/plugin-transform-modules-commonjs": "^7.0.0"
"@babel/plugin-transform-runtime": "^7.0.0"
"@babel/preset-env": "^7.0.0"
"@babel/preset-react": "^7.0.0"

And a snippet from our failing build -

 Module build failed (from ./node_modules/babel-loader/lib/index.js):

ERR! TypeError: [OMITTED]: Cannot read property 'removeBinding' of null

ERR!     at Object.keys.forEach.name ([OMITTED]/node_modules/@babel/traverse/lib/path/removal.js:45:52)

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Nov 23, 2020

I'll open a PR today, hoping for a patch release today.

@w3nda You can try #12383 (comment), which is a generic workaround and not specific to babel-minify.

@nicolo-ribaudo
Copy link
Member

I managed to reproduce the bug even reverting #12331: that PR made this bug more common, but didn't cause it.

@nicolo-ribaudo
Copy link
Member

Can the people who see this bug in their project try to replace the getSibling function in node_modules/@babel/core/node_modules/@babel/traverse/lib/path/family.js and/or node_modules/@babel/traverse/lib/path/family.js with this, and see if they still their build still crashes? I have identified the bug and fixed the example provided in #12383 (comment), but I want to make sure I'm not missing something.

  function getSibling(key) {
    return _index.default.get({
      parentPath: this.parentPath,
      parent: this.parent,
      container: this.container,
      listKey: this.listKey,
      key: key
-   });
+   }).setContext(this.context);
  }

@ehoogeveen
Copy link

With that change applied, the removeBinding errors are gone.

However I do still get the following error when minifying sortablejs@1.10.2 (also new with babel 7.12.7):

RangeError: node_modules/sortablejs/Sortable.js: Maximum call stack size exceeded
    at NodePath._resolve (node_modules/@babel/traverse/lib/path/introspection.js:310:18)
    at NodePath.resolve (node_modules/@babel/traverse/lib/path/introspection.js:307:15)
    at NodePath._resolve (node_modules/@babel/traverse/lib/path/introspection.js:317:31)
    at NodePath.resolve (node_modules/@babel/traverse/lib/path/introspection.js:307:15)
    at node_modules/@babel/traverse/lib/path/inference/inferer-reference.js:72:27
    at Array.filter (<anonymous>)
    at getConstantViolationsBefore (node_modules/@babel/traverse/lib/path/inference/inferer-reference.js:71:21)
    at getTypeAnnotationBindingConstantViolations (node_modules/@babel/traverse/lib/path/inference/inferer-reference.js:36:28)
    at NodePath._default (node_modules/@babel/traverse/lib/path/inference/inferer-reference.js:22:14)
    at NodePath._getTypeAnnotation (node_modules/@babel/traverse/lib/path/inference/index.js:59:20) {
  code: 'BABEL_TRANSFORM_ERROR'

I also got that error without the fix when disabling minify-simplify. I'm not sure the stack is entirely consistent; it might be hitting the limit at different points (or staying just beneath it in some cases) - without the fix but with minify-simplify disabled, I got the error while minifying d3@4.13.0:

RangeError: output/js/ext/full/d3.js: Maximum call stack size exceeded
    at NodePath.setScope (node_modules/@babel/traverse/lib/path/context.js:121:18)
    at NodePath.setContext (node_modules/@babel/traverse/lib/path/context.js:149:8)
    at NodePath._getKey (node_modules/@babel/traverse/lib/path/family.js:212:8)
    at NodePath.get (node_modules/@babel/traverse/lib/path/family.js:186:17)
    at NodePath._resolve (node_modules/@babel/traverse/lib/path/introspection.js:316:14)
    at NodePath.resolve (node_modules/@babel/traverse/lib/path/introspection.js:307:15)
    at node_modules/@babel/traverse/lib/path/inference/inferer-reference.js:72:27
    at Array.filter (<anonymous>)
    at getConstantViolationsBefore (node_modules/@babel/traverse/lib/path/inference/inferer-reference.js:71:21)
    at getTypeAnnotationBindingConstantViolations (node_modules/@babel/traverse/lib/path/inference/inferer-reference.js:36:28) {
  code: 'BABEL_TRANSFORM_ERROR'
}

Tested on Windows 10 x64.

@stefaniapedrazzi
Copy link

Can the people who see this bug in their project try to replace the getSibling function in node_modules/@babel/core/node_modules/@babel/traverse/lib/path/family.js and/or node_modules/@babel/traverse/lib/path/family.js with this, and see if they still their build still crashes?

I confirm that patching getSibling in node_modules/@babel/core/node_modules/@babel/traverse/lib/path/family.js fixes the issue in my project.

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Nov 23, 2020

@ehoogeveen Can you share that d3.js file? I tried with npx babel --presets=babel-preset-minify node_modules/d3/build/d3.js (which is this file) but I cannot reproduce the error. EDIT: I see it now.

@ehoogeveen
Copy link

I was able to work around the stack overflow by calling the babel cli using node --stack-size=1536 node_modules/@babel/cli/bin/babel --presets=babel-preset-minify node_modules/sortablejs/Sortable.js (for the sortablejs example), so it isn't infinite recursion.

@mhbarros
Copy link

Thanks @ehoogeveen for the example and workaround! I will investigate a fix today, in the meantime you can add @babel/traverse@7.12.0 to your devDependencies, uninstall @babel/core and reinstall it (so that it uses the older @babel/traverse version).

Thanks, it worked just fine. I also updated my package-lock.json with these values and changed my CI system to use the command "npm ci" instead of "npm install", to always pull this version until i manually update (thanks @nicolo-ribaudo for the tip!)

@nicolo-ribaudo
Copy link
Member

I'm keeping this open until it's released, in case someone else has the same problem.

@brunomptorres
Copy link

I'm keeping this open until it's released, in case someone else has the same problem.

@nicolo-ribaudo any idea on when this is getting released?

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Nov 23, 2020

Right after #12390 and #12389 are merged, because they fix other regressions with this release.

@nicolo-ribaudo
Copy link
Member

Released as 7.12.8

@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 Feb 23, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
i: bug i: regression outdated A closed issue/PR that is archived due to age. Recommended to make a new issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants