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
Fix scope of computed method keys #12812
Conversation
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/40995/ |
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit d084ac5:
|
414dd7a
to
448dce5
Compare
if (isKey && path.isMethod()) path = path.parentPath; | ||
if (path && path.isScope()) parent = path; | ||
} while (path && !parent); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Q: Can we call Scope#parent
recursively?
const scopeParent = this._parent()
if (this path key is key && its parentPath is Method) {
return scopeParent.parent;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that would be clearer code. But, if I've understood correctly what you're suggesting, I don't think it can work.
Problem is that the when testing if to skip a scope, we need to know not just where we are, but where we've come from (from inside the key or not?).
The computed key may not be the initial path. e.g.:
const a = 'outside';
const o = {
[
(function f() { return a; })() + ''
]() {
const a = 'inside';
}
};
If we start at the scope of f()
, at that point the parent is not a method, so the check is not triggered at this point. We proceed down to the parent scope (with this._parent()
) and then discover it's a method. But by this point we don't know if we came from inside the key or not - we don't have the previous path - so we don't know whether to skip it or not. To fix this, the method check has to move into _parent()
, at which point _parent()
becomes as complicated as the original.
Here's a couple of other versions. Do you prefer either of them?
get parent() {
let path = this.path;
// eslint-disable-next-line no-constant-condition
while (true) {
// Skip method scope if coming from inside computed key
path =
path.key === "key" && path.parentPath.isMethod()
? path.parentPath.parentPath
: path.parentPath;
if (!path) return undefined;
if (path.isScope()) return path.scope;
}
}
get parent() {
// Skip method scope if coming from inside computed key
let isKey = this.path.key === "key";
const parent = this.path.findParent(p => {
const isScope = p.isScope() && !(isKey && p.isMethod());
isKey = p.key === "key";
return isScope;
});
return parent?.scope;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for a detailed explainer! I slightly prefer the second one but no strong opinion.
2152a14
to
cb2c424
Compare
Added a fix for bug (2) so |
Thanks! Could you also add a test for this (ref: #12790 (comment)) in https://github.com/babel/babel/tree/main/packages/babel-plugin-proposal-optional-chaining/test/fixtures/general? |
@nicolo-ribaudo Test added. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
(I meant to approve ✔️) |
Thank you! I'm sure everyone asks this but... any chance you'll be doing a release soon? |
Maybe Monday but I don't promise. |
I see you kept your non-existent promise! Thanks. |
Partial fix for #12790.
This fixes bug number 1 which @nicolo-ribaudo identified in #12790 (comment).
I don't know when I'll get time to tackle bug number 2, so I thought I'd put in this PR in for starters.
The changes in src/scope/index.ts are to handle the odd case of a function definition embedded within the computed key (see tests).
This also seems to fix the other example of scoping problems mentioned in #12790 (comment)
NB One question: Should the check for a computed key also be done in each iteration of the loop here? I can't find a case where this is necessary but I'm not familiar with how/when Babel may recreate scopes if e.g. nodes are added/replaced so I'm not sure if you can rely on the parent path having done this check already.