Regression: path.parentPath.get(path.listKey) can clobber a traversal's visitors with those from a separate traversal in the global path cache

[jedwards1211]

Bug Report

• I would like to work on a fix!

Input Code

const {parse} = require('@babel/core')
const generate = require('@babel/generator').default
const traverse = require('@babel/traverse').default

const code = `
import { Foo } from './Foo'
import { Bar } from './Bar'
`

const presets = [
  ['@babel/preset-env', { targets: { node: 'current' } }],
]

const ast = parse(code, {plugins, presets})

traverse(ast, {
  Program(path) {
    path.traverse({
      ImportDeclaration: {
        enter(path) {
          console.log('ENTER', generate(path.node).code)
          if (path.node.source.value === './Bar')
            path.parentPath.get(path.listKey)
        },
        exit(path) {
          console.log('EXIT ', generate(path.node).code)
        }
      }
    })
  }
})

Expected behavior

I haven't bisected to find the version that introduced the regression yet, but with @babel/core, @babel/traverse, and @babel/preset-env 7.4.5, the exit visitor is called for both import statements:

ENTER import { Foo } from './Foo';
EXIT  import { Foo } from './Foo';
ENTER import { Bar } from './Bar';
EXIT  import { Bar } from './Bar';

Current behavior

With these versions:
├─ @babel/core@7.12.10
├─ @babel/preset-env@7.12.11
└─ @babel/traverse@7.12.12

The exit visitor for the second import statement isn't called because path.parentPath.get(path.listKey) has the side effect of replacing path.opts with those of the parent traversal (which only has a Program visitor)

ENTER import { Foo } from './Foo';
EXIT  import { Foo } from './Foo';
ENTER import { Bar } from './Bar';

(missing EXIT import { Bar } from './Bar')

Additional context

babel-plugin-flow-runtime is no longer working after I upgraded @babel/*, and I uncovered this issue debugging it.

[babel-bot]

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

[jedwards1211]

@nicolo-ribaudo okay actually I think the nested traversal isn't even the issue, even if I do a non-nested traversal like traverse(path.node, { ... }) within the Program visitor, because there's an apparently global cache of paths (https://github.com/babel/babel/blob/main/packages/babel-traverse/src/path/index.js#L70) it pulls in information that shouldn't be related in any way at all to the non-nested traversal, and clobbers its options.

Shouldn't each traversal own a separate path cache (or at least each root traversal)? This feels very wrong, how this global cache is causing unexpected effects.

[jedwards1211]

Okay I found the explanation for why cache ownership was moved from the traversal to the instance of @babel/traverse. But it seems it isn't safe for either one to own the cache.

It seems like the only safe design is either to not cache, or if so, each combination of root traversal and @babel/traverse version has a separate cache, instead of a cache owned by a root traversal accidentally getting accessed by different versions of @babel/traverse or a cache owned by @babel/traverse accidentally getting accessed by different root traversals.

[jedwards1211]

This regression was introduced in @babel/traverse 7.12.7 (seems that PR #12302 did it)

[fedeci]

@jedwards1211 I just bisected and it seems to be a regression of #12331.

[jedwards1211]

Huh. I didn't do an actual git bisect, just tried subsequent versions, so i bet you're right

[fedeci]

After further investigation I discovered that the bug was already there since v6 😄 and #12331 just exposed it.

// quick temporary fix
path.parentPath.get(path.listKey, false)

[JLHwung]

I think the behaviour is expected because path.traverse does not visit path itself. In this case, for an ImportDeclaration path, its parentPath is always a Program, whose context are the root context because the Program path is not visited in path.traverse. Now when we call .get("body"): all the immediate children of Program, including the ImportDeclaration, inherit the root context and hence the current context is overridden by root context.

A workaround for

path.parentPath.get(path.listKey)

can be

const siblings = [
  ...path.getAllPrevSiblings(),
  path,
  ...path.getAllNextSiblings(),
];

When parentPath is involved, it is difficult to preserve context as it may run out of the sub-traversal, unless we mark the whole AST as the new context (so plugin is free to visit anywhere), or we provide a getParentPath(context: TraversalContext).

[jedwards1211]

Huh I see. FWIW, I wonder if a different API design would be possible where the context information is passed around instead of being a property on the paths that gets mutated. This issue certainly wasn't a showstopper for me but I did burn a lot of time in a painful debugging session on it.

[JLHwung]

Indeed, I will see if I can take on #12634

[jedwards1211]

@JLHwung oh interesting.

I noticed that visitor is already an argument to path.traverse etc. And this issue involved traversal getting the visitors from path.opts in some cases, and that getting blown away by context changes. So aside from changes in that PR, I wonder if it would be possible for traversal to always use the visitor passed around via function arguments instead of anything related to context?

[nicolo-ribaudo]

The original example and the test in the attached PR both pass on main.

[nicolo-ribaudo]

Fixed by #13813 (found by git bisect), I will add a test.