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]: visitors.merge removes noScope #15767

Open
1 task done
j4k0xb opened this issue Jul 10, 2023 · 2 comments
Open
1 task done

[Bug]: visitors.merge removes noScope #15767

j4k0xb opened this issue Jul 10, 2023 · 2 comments

Comments

@j4k0xb
Copy link

j4k0xb commented Jul 10, 2023

馃捇

  • Would you like to work on a fix?

How are you using Babel?

Programmatic API (babel.transform, babel.parse)

Input code

const parse = require('@babel/parser').parse;
const traverse = require('@babel/traverse').default;
const generate = require('@babel/generator').default;
const visitors = require('@babel/traverse/lib/visitors');

const ast = parse('const a = 1; { const b = 2 };');

const visitor = visitors.merge([
  {
    Program(path) {
      console.log(!!path.scope);
    },
    noScope: true,
  },
  {
    BlockStatement(path) {
      console.log(!!path.scope);
    },
    noScope: true,
  },
]);

traverse(ast, visitor); // should log false

traverse(ast, {}); // some visitor that initializes scope

traverse(ast, visitor); // should log true

Configuration file name

No response

Configuration

No response

Current and expected behavior

Current behaviour (since #15702): noScope of merged visitors gets removed and causes the scope to be crawled anyways and overwrites the old scope.
Expected behaviour (v7.22.6): keep noScope in the merged visitor

I have to use multiple traversals, most use noScope: true to achieve better performance while still accessing the scope/references of a previous traversal.
Another reason is that the AST is sometimes temporarily in a broken state or I manually update references, and don't want the scope to be overwritten.

Environment

System:
OS: Linux 5.15 Manjaro Linux
Binaries:
Node: 18.12.0 - ~/.nvm/versions/node/v18.12.0/bin/node
Yarn: 1.22.19 - /usr/bin/yarn
npm: 8.19.2 - ~/.nvm/versions/node/v18.12.0/bin/npm
npmPackages:
@babel/generator: ^7.22.7 => 7.22.7
@babel/helper-validator-identifier: ^7.22.5 => 7.22.5
@babel/parser: ^7.22.7 => 7.22.7
@babel/template: ^7.22.5 => 7.22.5
@babel/traverse: ^7.22.8 => 7.22.8
@babel/types: ^7.22.5 => 7.22.5
eslint: ^8.44.0 => 8.44.0

Possible solution

Set noScope of the merged visitor if all visitors have it.
Or v7.22.6 behaviour: if at least one has it?

Additional context

No response

@babel-bot
Copy link
Collaborator

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

@JLHwung
Copy link
Contributor

JLHwung commented Jul 10, 2023

The noScope, alongside with some other options:

export type TraverseOptions<S = t.Node> = {
scope?: Scope;
noScope?: boolean;
denylist?: string[];
shouldSkip?: (node: NodePath) => boolean;
} & Visitor<S>;

are ignored in shouldIgnoreKey. We should merge such options in

if (shouldIgnoreKey(nodeType)) continue;

@j4k0xb I have assigned this issue to you since you voiced your intent to work on this issue. Feel free to ask any questions if there are bumps when working on a fix.

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

3 participants