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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Regression: path.parentPath.get(path.listKey) can clobber a traversal's visitors with those from a separate traversal in the global path cache #12570
Comments
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." |
@nicolo-ribaudo okay actually I think the nested traversal isn't even the issue, even if I do a non-nested traversal like 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. |
As mentioned on the task https://phabricator.babeljs.io/T7179 having this cache on the AST leads to all sorts of portability and reuse bugs. This moves the cache into a clearable WeakMap which will fix the following: 1. Moving the AST between different babel versions or tools will not lead into sharing potentially outdated cached information 2. `.clear()` can be called on the cache by a plugin to clear potentially outdated information. This is helpful when implementing two seperate pipelines that should not share information. I think the next step (which is harder, I tried) is to isolate cache and make it live on a transform or pipeline level state (like the `hub`). The reason it is hard is because the `babel-traverse` main API -- although requires the state object to be passed -- not many callers do. To fix this we should release a patch version that warns about this and fix all the internal callers. Next couple of releases we can start throwing when no state is passed (or we can create our own state).
Okay I found the explanation for why cache ownership was moved from the traversal to the instance of It seems like the only safe design is either to not cache, or if so, each combination of root traversal and |
This regression was introduced in @babel/traverse 7.12.7 (seems that PR #12302 did it) |
@jedwards1211 I just bisected and it seems to be a regression of #12331. |
Huh. I didn't do an actual git bisect, just tried subsequent versions, so i bet you're right |
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) |
I think the behaviour is expected because A workaround for path.parentPath.get(path.listKey) can be const siblings = [
...path.getAllPrevSiblings(),
path,
...path.getAllNextSiblings(),
]; When |
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. |
Indeed, I will see if I can take on #12634 |
@JLHwung oh interesting. I noticed that |
The original example and the test in the attached PR both pass on |
Fixed by #13813 (found by |
Bug Report
Input 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:
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 replacingpath.opts
with those of the parent traversal (which only has aProgram
visitor)(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.The text was updated successfully, but these errors were encountered: