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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug]: Scope bindings missing in AST for the variables which are introduced by babel-plugin-transform-destructuring #14438

Closed
1 task
peey opened this issue Apr 8, 2022 · 5 comments 路 Fixed by #14494
Labels
i: needs triage outdated A closed issue/PR that is archived due to age. Recommended to make a new issue

Comments

@peey
Copy link
Contributor

peey commented Apr 8, 2022

馃捇

  • Would you like to work on a fix?

How are you using Babel?

Programmatic API (babel.transform, babel.parse)

Input code

https://github.com/peey/babel-missing-ast-scope-bindings-repro

Configuration file name

No response

Configuration

No response

Current and expected behavior

Current Behavior

No scope bindings in the AST for variables introduced by @babel/plugin-transform-destructuring.

E.g. in the example

function f() { return {x: 1, y: 2}; }; const {x,y} = f();

The transformed source is

function f() {
  return {
    x: 1,
    y: 2
  };
}

;

const _f = f(),
      x = _f.x,
      y = _f.y;

Each variable has an associated scope binding in the AST, but the variable introduced by the transform (_f) does not.

Expected Behavior

Scope bindings expected to be present

Environment

Binaries:
Node: 16.13.0 - ~/.nvm/versions/node/v16.13.0/bin/node
Yarn: 1.22.17 - ~/.nvm/versions/node/v16.13.0/bin/yarn
npm: 8.1.0 - ~/.nvm/versions/node/v16.13.0/bin/npm
npmPackages:
@babel/core: ^7.17.9 => 7.17.9
@babel/parser: ^7.17.9 => 7.17.9
@babel/plugin-transform-destructuring: ^7.17.7 => 7.17.7
@babel/traverse: ^7.17.9 => 7.17.9
@babel/types: ^7.17.0 => 7.17.0

Possible solution

Perhaps the related piece of source code is the following:?

I was expecting to find templates which introduce the variable, but it seems like the resulting AST is constructed in another way for this transform so I'm not sure.

Additional context

No response

@babel-bot
Copy link
Collaborator

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

@liuxingbaoyu
Copy link
Member

This problem may be difficult to fix.
But if you just want it to work.
Please use originalAst = parse() again

@peey
Copy link
Contributor Author

peey commented Apr 26, 2022

Hi @liuxingbaoyu . I agree that's a good alternative perhaps. But I think if this can be fixed it could be useful to downstream plugins which need scope info (e.g. if some plugin runs after de-structuring, for correct operation it definitely needs correct scope info right?)

I've tried to fix it and made some progress. Please look at the linked PR (very small change - 2 lines in source, ~60 lines in test) in case you have any insights you can share which could help improve it.

peey added a commit to peey/babel that referenced this issue Apr 26, 2022
1. Update tests: some were spurious failures
2. Address failing tests: introduce scope crawling in for statement
   cases
@liuxingbaoyu
Copy link
Member

Yes, I think that makes sense too.

However I prefer to provide options to isolate plugins, which is clearly an issue in many plugins.

@peey
Copy link
Contributor Author

peey commented Apr 28, 2022

What do you mean by isolating plugins in context of this issue?

If you mean they should have clear boundaries and interfaces then I agree. I think this would be a step towards it. Instead of having to worry about "I might have scope bindings if I ran before plugin X but not before plugin Y" this can be a first step towards providing the guarantee that "Each plugin will always receive a proper AST with full information as input, and must produce a proper AST as output". Currently babel-plugin-transform-desturcturing is violating that in that it does recieve valid ASTs but it produces ASTs which are incomplete (e.g. they have AST elements but their bindings have been uninitalized)

Thankfully I was able to fix this issue in the linked PR for babel-plugin-transform-destructuring, but a reviewer had interesting comments about how this approach could perhaps be generalized to all plugins. If that's something you're interested in you can check that out, and maybe that'd be the next step after the PR is merged. In case you do check it out, please also leave a review! The final code is even smaller and simpler thanks to the first reviewer's contribution.

@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 Aug 4, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 4, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
i: needs triage 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.

3 participants