Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix scope of computed method keys #12812

Merged
merged 4 commits into from Feb 19, 2021
Merged

Fix scope of computed method keys #12812

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
448dce5
Fix scope of computed method keys
overlookmotel Feb 17, 2021
cb2c424
Test for nested computed keys
overlookmotel Feb 17, 2021
fd21c27
Fix scope.rename with computed method keys
overlookmotel Feb 17, 2021
d084ac5
Optional chaining tests
overlookmotel Feb 19, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/babel-traverse/src/path/context.ts
View file
Open in desktop
Expand Up @@ -119,6 +119,10 @@ export function setScope(this: NodePath) {
if (this.opts && this.opts.noScope) return;

let path = this.parentPath;

// Skip method scope if is computed method key
if (this.key === "key" && path.isMethod()) path = path.parentPath;

let target;
while (path && !target) {
if (path.opts && path.opts.noScope) return;
Expand Down
11 changes: 10 additions & 1 deletion packages/babel-traverse/src/scope/index.ts
View file
Open in desktop
Expand Up @@ -359,7 +359,16 @@ export default class Scope {
static contextVariables = ["arguments", "undefined", "Infinity", "NaN"];

get parent() {
const parent = this.path.findParent(p => p.isScope());
let parent,
path = this.path;
do {
// Skip method scope if coming from inside computed key
const isKey = path.key === "key";
path = path.parentPath;
if (isKey && path.isMethod()) path = path.parentPath;
if (path && path.isScope()) parent = path;
} while (path && !parent);

Copy link
Contributor

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;
}

Copy link
Contributor Author

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;
}

Copy link
Contributor

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.

return parent?.scope;
}

Expand Down
68 changes: 68 additions & 0 deletions packages/babel-traverse/test/scope.js
View file
Open in desktop
Expand Up @@ -168,6 +168,74 @@ describe("scope", () => {
});
});

describe("computed method key", () => {
JLHwung marked this conversation as resolved.
Show resolved Hide resolved
describe("should not have visibility of declarations inside method body", () => {
it("when path is computed key", () => {
expect(
getPath(`var a = "outside"; ({ [a]() { let a = "inside" } })`)
.get("body.1.expression.properties.0.key")
.scope.getBinding("a").path.node.init.value,
).toBe("outside");

expect(
getPath(
`var a = "outside"; class foo { [a]() { let a = "inside" } }`,
)
.get("body.1.body.body.0.key")
.scope.getBinding("a").path.node.init.value,
).toBe("outside");
});

it("when path is in nested scope which is computed key", () => {
expect(
getPath(`var a = "outside"; ({ [() => a]() { let a = "inside" } })`)
.get("body.1.expression.properties.0.key.body")
.scope.getBinding("a").path.node.init.value,
).toBe("outside");

expect(
getPath(
`var a = "outside"; class foo { [() => a]() { let a = "inside" } }`,
)
.get("body.1.body.body.0.key.body")
.scope.getBinding("a").path.node.init.value,
).toBe("outside");
});

it("when path is in nested scope within computed key", () => {
expect(
getPath(
`var a = "outside"; ({ [(() => a)() + ""]() { let a = "inside" } })`,
)
.get("body.1.expression.properties.0.key.left.callee.body")
.scope.getBinding("a").path.node.init.value,
).toBe("outside");

expect(
getPath(
`var a = "outside"; class foo { [(() => a)() + ""]() { let a = "inside" } }`,
)
.get("body.1.body.body.0.key.left.callee.body")
.scope.getBinding("a").path.node.init.value,
).toBe("outside");
});
});

it("should not have visibility on parameter bindings", () => {
expect(
getPath(`var a = "outside"; ({ [a](a = "inside") {} })`)
.get("body.1.expression.properties.0.key")
.scope.getBinding("a").path.node.init.value,
).toBe("outside");

expect(
getPath(`var a = "outside"; class foo { [a](a = "inside") {} }`)
.get("body.1.body.body.0.key")
.scope.getBinding("a").path.node.init.value,
).toBe("outside");
});
});

it("variable declaration", function () {
expect(getPath("var foo = null;").scope.getBinding("foo").path.type).toBe(
"VariableDeclarator",
Expand Down