Problems with `renameTo` and block scope

ppershing commented 6 years ago

ES6 Block scoping seems to be broken in renameTo.

Repro:

jscodeshift: 0.5.0
 - babel: 6.26.3
 - babylon: 7.0.0-beta.47
 - flow: 0.73.0
 - recast: 0.14.7

Transform:

module.exports = function transform(file, api, options) {
  const j = api.jscodeshift
  const root = j(file.source)

  let first = true
  root.find(j.VariableDeclarator)
    .filter((path) => {let res = first; first = false; return res}) // select only first declarator
    .forEach((path) => {
      console.log('have path') // check we have exactly one
    })
    .renameTo('baz')

  return root.toSource()
}

Input:

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

Output:

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

Expected output:

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

jods4 commented 3 years ago

I'm writing my own transform and I observe that opening a block like that doesn't create a nested scope in codeshift, which is the reason why renameTo is incorrect here.

Amazed that a correctness bug like this one is still open 2 years later :(

0xdevalias commented 10 months ago

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'd have to look deeper to be sure where exactly the root issue is though.

Potentially related?

It also seems this/similar might be an issue shared across a number of the parsers / scope managers, e.g:

facebook / jscodeshift

Problems with `renameTo` and block scope #263