Scoping issue

[pionxzh]

The current identifier renaming is not 100% accurate. By inspecting the unpacking snapshot, you can tell that some variable was wrongly renamed to export or require during the unpacking process. Mostly because ast-types are giving us wrong scope information, and it's no longer well-maintained. We need to either patch it or find an alternative.

• Add test cases to address the scoping correctness
• Known issue: Block Scope is not correct (Problems with renameTo and block scope facebook/jscodeshift#263)

The best solution would be to fix it in the upstream.
Let's track the progress at facebook/jscodeshift#500

[0xdevalias]

I assume the issue here is related to ast-type's parsing/handling of Scope/etc?

• https://github.com/benjamn/ast-types#scope

For my own background knowledge/context, it seems that jscodeshift is basically a wrapper around recast, which relies on ast-types:

• https://github.com/facebook/jscodeshift

  • jscodeshift is a reference to the wrapper around recast and provides a jQuery-like API to navigate and transform the AST

  • The transform file can let jscodeshift know with which parser to parse the source files (and features like templates).

    To do that, the transform module can export parser, which can either be one of the strings "babel", "babylon", "flow", "ts", or "tsx", or it can be a parser object that is compatible with recast and follows the estree spec.

  • Recast itself relies heavily on ast-types which defines methods to traverse the AST, access node fields and build new nodes. ast-types wraps every AST node into a path object. Paths contain meta-information and helper methods to process AST nodes.

I wonder if the root issue lies in recast, or ast-types? Potentially related:

• getBindings doesn't detect a closure's free variables benjamn/recast#397
• Implement Scope.prototype.rename(oldName, newName) benjamn/ast-types#133
• path.scope not accurate for const and let benjamn/ast-types#154
• Scope of a var in catch is catchclause benjamn/ast-types#188
• Scope.js can't find object-deconstruction declarations benjamn/ast-types#256
• Variables declared in object destructuring aren't collected as bindings when using babel parser benjamn/ast-types#360
• Missing catch binding breaks scope API benjamn/ast-types#944
• Added support for block scoping benjamn/ast-types#455

---

I haven't looked closely at the code here and how it's used, but would something like eslint-scope help?

• https://github.com/eslint/eslint-scope

  • ESLint Scope is the ECMAScript scope analyzer used in ESLint. It is a fork of escope

    • https://github.com/estools/escope

      • Escope (escope) is ECMAScript scope analyzer extracted from esmangle project.

Looking at the linked issue (facebook/jscodeshift#263), it seems that a 'raw' block will lead to scope issues currently.

Example code that breaks:

const x = 42
{
  const x = 47
  console.log(x)
}
console.log(x)

Reviewing eslint-scope / escope sounds like they may also have similar bugs:

• escope
  • Add ES6 Support estools/escope#33
  • Try blocks not identified as scope level estools/escope#57
  • Block-level functions and web extensions estools/escope#73
  • let declaration treated as var declaration estools/escope#119
  • Function declarations attached to block scope? estools/escope#126
• eslint-scope
  • Proposal: "function-default-params" scope eslint/eslint-scope#34

---

I was playing around with some PoC code with eslint-scope this morning, that you can see on the following REPL:

• https://replit.com/@0xdevalias/Rewriting-JavaScript-Variables-via-AST-Examples#espree-eslint-scope_2.js

Running espree-eslint-scope_2.js with the breaking example code from facebook/jscodeshift#263 (see above) gives me the following output:

$ node espree-eslint-scope_2.js

-= SCOPE INFO =-
Scope: type=global (block.type=Program) block.id?.name=undefined implicit=x,x,console,x,console,x
Variable: x [
  { type: 'Variable', kind: 'const' },
  { type: 'Variable', kind: 'const' }
] References: 4

-= ANALYSIS RESULT =-
{
  "type": "global",
  "implicitGlobals": [
    "x",
    "x",
    "console",
    "x",
    "console",
    "x"
  ],
  "identifiers": [
    {
      "name": "x",
      "types": [
        {
          "type": "Variable",
          "kind": "const"
        },
        {
          "type": "Variable",
          "kind": "const"
        }
      ]
    }
  ],
  "childScopes": []
}

I think this may mean that eslint-scope may also be incorrectly handling the scope of these variables; as I would expect there to be both the global scope, and the block scope.

It seems ChatGPT agrees with that:

Details

Me:

Given the following javascript code, how many scopes should there be, and which should be defined in which scope?

const x = 42
{
  const x = 47
  console.log(x)
}
console.log(x)

ChatGPT:

In the provided JavaScript code, there are two scopes involved:

1. Global Scope: The first const x = 42; is defined in the global scope. Variables declared in the global scope can be accessed and modified throughout the entire script.

2. Block Scope: The { const x = 47; console.log(x); } part defines a block scope. The const x = 47; within this block is a separate variable that only exists within this block scope. It's important to note that const and let in JavaScript are block-scoped, meaning they are only accessible within the block they are defined in.

Here's a breakdown of what's defined in each scope:

• Global Scope:

  • const x = 42;

• Block Scope:

  • const x = 47; (This is a different x than the one in the global scope.)

When the console.log(x); inside the block is executed, it outputs 47, which is the value of x in the block scope. When the console.log(x); outside the block is executed, it outputs 42, which is the value of x in the global scope.

The two x variables, although they share the same name, are distinct and exist in separate scopes.

I've opened a bug for this on eslint-scope, but unless that gets fixed, it doesn't seem like it will be a good solution for the needs here:

• Bug: eslint-scope fails to create new Scope for block element eslint/eslint-scope#110

Edit: Seems I was wrong about that, see my below comment for updates:

• Scoping issue #32 (comment)

---

These tools may also be useful for checking how escope handles things:

• https://mazurov.github.io/eslevels-demo/
• https://mazurov.github.io/escope-demo/

[0xdevalias]

I just did another test in the REPL, this time just using @babel/parser / @babel/traverse directly (not wrapped with jscodeshift / recast, so not using ast-types):

• https://replit.com/@0xdevalias/Rewriting-JavaScript-Variables-via-AST-Examples#babel-scopes.js

PoC Code

const parser = require('@babel/parser');
const traverse = require('@babel/traverse').default;

const code = `
const x = 42;
{
  const x = 47;
  console.log(x);
}
console.log(x);
`;

const ast = parser.parse(code, {
  sourceType: 'module',
  // plugins: [
  //   // Add any necessary plugins
  // ],
});

const scopes = new Set();

function collectScopes(path) {
  if (path.scope && !scopes.has(path.scope)) {
    scopes.add(path.scope);
    path.scope.parent && collectScopes(path.findParent((p) => p.scope));
  }
}

traverse(ast, {
  enter(path) {
    collectScopes(path);
  },
});

console.log("\n-= Scopes and Bindings =-\n");
scopes.forEach((scope) => {
  console.group(
    `Scope (uid=${scope.uid}, path.type=${scope.path.type})`
  );
  console.log(
    'Bindings:',
    JSON.stringify(Object.keys(scope.bindings), null, 2)
  );
  // console.log(scope);
  console.groupEnd();
  console.log();
});

This seems to have correctly output the 2 scopes as expected:

$ node babel-scopes.js 

-= Scopes and Bindings =-

Scope (uid=0, path.type=Program)
  Bindings: [
    "x"
  ]

Scope (uid=1, path.type=BlockStatement)
  Bindings: [
    "x"
  ]

[0xdevalias]

I think this may mean that eslint-scope may also be incorrectly handling the scope of these variables; as I would expect there to be both the global scope, and the block scope.

I've opened a bug for this on eslint-scope, but unless that gets fixed, it doesn't seem like it will be a good solution for the needs here:

• Bug: eslint-scope fails to create new Scope for block element eslint/eslint-scope#110

It seems I was just using eslint-scope wrong, and not parsing the ecmaScript: 6 option to it (Ref)

There are some useful docs links and discoveries in my comment on the above issue:

• Bug: eslint-scope fails to create new Scope for block element eslint/eslint-scope#110 (comment)
• https://eslint.org/docs/latest/extend/scope-manager-interface

And I also updated my REPL to fix the code and make it work properly now:

• https://replit.com/@0xdevalias/Rewriting-JavaScript-Variables-via-AST-Examples

With the very basic minimal example being:

const espree = require('espree');
const eslintScope = require('eslint-scope');

const exampleJSCode = `
const x = 42
{
  const x = 47
  console.log(x)
}
console.log(x)
`

const commonParserOptions = {
  ecmaVersion: 2020,
  sourceType: 'module', // script, module, commonjs
}
  
// Parse the JavaScript code into an AST with range information
const ast = espree.parse(jsCode, {
  ...commonParserOptions,
  range: true  // Include range information
});

// Analyze the scopes in the AST
// See the .analyze options in the source for more details
//   https://github.com/eslint/eslint-scope/blob/957748e7fb741dd23f521af0c124ce6da0848997/lib/index.js#L111-L131
// See the following for more details on the ScopeManager interface:
//  https://eslint.org/docs/latest/extend/scope-manager-interface
//  https://github.com/eslint/eslint-scope/blob/main/lib/scope-manager.js
const scopeManager = eslintScope.analyze(
  ast,
  {
    ...commonParserOptions,
    nodejsScope: false,
  }
);

console.log('ScopeManager.scopes=', scopeManager.scopes)

See the ScopeManger interface docs (Ref, potentially not up to date?) for more info on what objects/functions/etc eslint-scope provides.

[pionxzh]

Babel and eslint are handling it correctly; the issue is from ast-types. The reason why I didn't use them is to maintain only one ast tool been used. We can either fix ast-types ourselves, and potentially PR back to the upstream. Using Babel or other tools for scoping is doable, but it will be hard to maintain, and there will be some performance punishment. I will check some references you provided this weekend to see what can we do.

[0xdevalias]

Babel and eslint are handling it correctly; the issue is from ast-types

@pionxzh Yup. I just wanted to test/ensure that the other suggestions I was making actually handled it properly.

---

The reason why I didn't use them is to maintain only one ast tool been used.

@pionxzh Yeah, that definitely makes sense. I wasn't suggesting them so much in a 'use multiple ast tools' way. More in a 'if we need to replace the current choice, what are some alternatives that handle it properly'

---

We can either fix ast-types ourselves, and potentially PR back to the upstream.

@pionxzh nods Part of the reason I was looking at some of the alternatives is that the upstream jscodeshift issue you linked seems to suggest that recast and ast-types are pretty unmaintained; and even that issue hasn't had any meaningful updates in more than a year, which makes me think jscodeshift is also equally unmaintained:

• Bringing jscodeshift up to date facebook/jscodeshift#500

  • The biggest issue is with recast. This library hasn't really had a lot of maintenance for the last couple of years, and there's something like 150+ issues and 40+ pull requests waiting to be merged. It seems like 80% of the issues that are logged against jscodeshift are actually recast issues. In order to fix the jscodeshift's outstanding issues, either recast itself needs to fix them or jscodeshift will need to adopt/create its own fork of recast to solve them.

  • What can be said about recast can probably also be said to a lesser degree about ast-types

  • Bringing jscodeshift up to date facebook/jscodeshift#500 (comment)

    • The biggest challenge facing jscodeshift (other than the documentation that everyone keeps complaining about) is 50-80% of its logged bugs are actually bugs in the recast and ast-types libraries, both of which have a substantial number of bugs and outstanding pull requests, and both of which tend to be updated infrequently. This means there are whole classes of bugs that jscodeshift as a project can't do anything about. At some point, unless something changes with those projects, it's probably going to be necessary to fork both those libraries, put in PR's to upstream back to them, and hope that they'll someday be merged.

• Bringing jscodeshift up to date facebook/jscodeshift#500 (comment)

  • Since the last update on this issue was in August 2022, I just wanted to check in and see what the latest state of things is on this?

    If jscodeshift is currently tightly tied to a 'rarely maintained' recast / ast-types; without much ability to fix bugs caused by those tools, should we also consider jscodeshift to be in a 'rarely maintained' state? And if so, are there any suggestions as to what the best 'modern alternative' would be?

So my thinking was more in the space of "if the current underlying tools are unmaintained and buggy, what are the 'best modern alternative' choices we can use; and particularly ones that are quite heavily used by others, and so are likely to stay maintained/updated.

---

Using Babel or other tools for scoping is doable, but it will be hard to maintain, and there will be some performance punishment.

@pionxzh When you say hard to maintain/performance punishment; do you just mean if we were using multiple AST tools at the same time? Or do you mean by switching to one of those at all?

---

I will check some references you provided this weekend to see what can we do.

@pionxzh Sounds good :)

[pionxzh]

I mean it would be hard to maintain with multiple AST tools.

[0xdevalias]

Yup, makes sense. I'd definitely also recommend sticking to 1 for that reason.

[0xdevalias]

I'm curious, what benefits does jscodeshift (with the babylon / babel parser) / recast / ast-types give us compared to just using @babel/parser / @babel/traverse directly?

I asked ChatGPT, and it seemed to suggest that there is a 'higher level API' from jscodeshift; and that recast focuses on maintaining the original formatting/spacing of source code being modified (which since I believe we pretty print with prettier anyway, is probably irrelevant to the needs of this project)

I'm curious whether that 'higher level API' from jscodeshift actually simplifies things much compared to using babel's libraries directly?

ChatGPT Response

Question:

A javascript AST project I am looking at is using jscodeshift with the babel / babylon parser. jscodeshift uses recast, which uses ast-types.

What benefits would jscodeshift / recast / ast-types give us compared to just using @babel/parser / @babel/traverse directly

Response:

Using jscodeshift, recast, and ast-types in conjunction with the Babel/Babylon parser offers several benefits compared to using @babel/parser and @babel/traverse directly. Here's an overview:

jscodeshift

1. Higher-Level API: jscodeshift provides a more fluent and high-level API for transforming code. It's designed to make writing codemods (code modifications) simpler and more intuitive.

2. Integration with Recast: jscodeshift uses recast internally, which helps in retaining the original formatting and style of the code as much as possible. This is particularly useful when you want to apply transformations without altering the code's original format.

3. Community and Ecosystem: jscodeshift has a good community and ecosystem around it, with many predefined codemods and transformations that you can leverage.

Recast

1. Code Formatting Preservation: Recast's primary feature is to parse your code and then print it back out while preserving the original formatting as much as possible. This is crucial when working on large codebases where consistency in formatting is vital.

2. Compatibility with Various Parsers: Recast is designed to be parser-agnostic, so it can work with different parsers like Esprima, Babel, and others. This flexibility allows you to choose the parser that best suits your needs.

3. AST Manipulation: Although @babel/traverse is powerful for AST traversal, recast adds an extra layer of utility in terms of manipulating the AST and reprinting it in a way that respects the original code's layout.

ast-types

1. AST Node Type Definitions: ast-types is used by recast for defining and validating the structure of AST nodes. This helps in ensuring that the transformations you apply are structurally sound and conform to the expected AST node types.

2. Fluent API for AST Construction: ast-types provides a fluent API for constructing AST nodes, making it easier to generate new parts of the AST in your transformations.

Comparison with @babel/parser and @babel/traverse

• Direct AST Manipulation: @babel/parser and @babel/traverse offer more direct and low-level manipulation of the AST. While this gives you more control, it also requires a deeper understanding of ASTs and might involve more boilerplate code for common transformations.

• Formatting Considerations: When using Babel tools directly, you often have to handle formatting separately (using tools like Prettier), as they don't inherently preserve the original code's formatting.

• Use Case Specificity: If your primary goal is to write transformation scripts or codemods with a focus on maintaining code style and formatting, jscodeshift with recast and ast-types is a more suitable choice. For more general-purpose AST analysis and manipulation, @babel/parser and @babel/traverse might be adequate.

In conclusion, the choice between these tools depends on your specific use cases, the importance of preserving code formatting, and the level of abstraction you prefer for AST manipulation. For more straightforward AST parsing and traversal, Babel's tools might suffice. For more complex transformations, particularly where code style preservation is crucial, jscodeshift combined with recast and ast-types offers a more specialized solution.

---

Edit: For my own reference, here are the code locations currently referencing jscodeshift, recast and/or ast-types:

• https://github.com/search?q=repo%3Apionxzh%2Fwakaru+%28jscodeshift+OR+recast+OR+ast-types%29&type=code
  • https://github.com/pionxzh/wakaru/blob/main/packages/unpacker/src/unpack.ts#L16
    • @wakaru/unpack using jscodeshift with babylon parser
  • https://github.com/pionxzh/wakaru/blob/main/packages/unminify/src/index.ts#L20
    • @wakaru/unminify using jscodeshift with babylon parser
  • etc

I also note that there is a @wakaru/ast-utils package that could also potentially be used to centralise some 'leaky abstractions' from these libs if we were to refactor:

• https://github.com/pionxzh/wakaru/tree/main/packages/ast-utils

And it might also potentially make sense to refactor some of these utils into that ast-utils package as well maybe?

• https://github.com/pionxzh/wakaru/tree/main/packages/unminify/src/utils

Edit 2: Raised a new issue for the 'refactor into @wakaru/ast-utils' potential mentioned above:

• refactor 'leaky abstractions' related to ASTs + other AST utils into @wakaru/ast-utils #65

[pionxzh]

jscodeshift provides a query-like API that is easier to find and manipulate specific AST nodes than babel/traverse 's traversal API. Adopting another tool (eg. babel/traverse) basically means rewriting everything, which is the thing that I do not want to spend time on. Currently the scoping issue is only happening on some edge cases. It's still acceptable for me. That's why I prefer to "patch" it instead of using a totally different tool.

[0xdevalias]

jscodeshift provides a query-like API that is easier to find and manipulate specific AST nodes than babel/traverse's traversal API.

nods that makes sense, and I suspected it was likely going to be something like that, but I wanted to confirm my assumptions (given that the current jscodeshift -> @babel/parser -> recast -> ast-types adds a lot of extra libraries/points of potential bugs/etc, particularly given jscodeshift/recast/ast-types are basically unmaintained currently)

I was basically thinking about what the 'main advantages' are that are being gained from each of the AST related libs; as that then makes it easier to think about whether there is a 'better' solution that still meets those needs/constraints.

[pionxzh]

I think jscodeshift's README provides a quite detailed description of each part of the components. 🤔

[0xdevalias]

I think jscodeshift's README provides a quite detailed description of each part of the components. 🤔

@pionxzh Yeah, in terms of jscodeshift. But I wanted to know what your personal reasoning for wanting to stick with it specifically was, and whether you cared about some of the 'strengths' that came along with the underlying libs (such as recast's focus on not modifying unrelated whitespace/etc; which seems irrelevant for wakaru since it prettifiers the end code anyway); which you answered for me above (simpler API + don't want to have to recode existing stuff for a new API)