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 3 commits
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
17 changes: 16 additions & 1 deletion packages/babel-traverse/src/scope/lib/renamer.ts
View file
Open in desktop
Expand Up @@ -17,7 +17,7 @@ const renameVisitor: Visitor<Renamer> = {
state.binding.identifier,
)
) {
path.skip();
skipAllButComputedMethodKey(path);
}
},

Expand Down Expand Up @@ -142,3 +142,18 @@ export default class Renamer {
}
}
}

function skipAllButComputedMethodKey(path) {
// If the path isn't method with computed key, just skip everything.
if (!path.isMethod() || !path.node.computed) {
path.skip();
return;
}

// So it's a method with a computed key. Make sure to skip every other key the
// traversal would visit.
const keys = t.VISITOR_KEYS[path.type];
for (const key of keys) {
if (key !== "key") path.skipKey(key);
}
}
8 changes: 8 additions & 0 deletions packages/babel-traverse/test/fixtures/rename/method-computed-key-1/input.js
View file
Open in desktop
@@ -0,0 +1,8 @@
let a = "outside";

const obj = {
[a]() {
let a = "inside";
return a;
}
};
3 changes: 3 additions & 0 deletions packages/babel-traverse/test/fixtures/rename/method-computed-key-1/options.json
View file
Open in desktop
@@ -0,0 +1,3 @@
{
"plugins": ["./plugin"]
}
8 changes: 8 additions & 0 deletions packages/babel-traverse/test/fixtures/rename/method-computed-key-1/output.js
View file
Open in desktop
@@ -0,0 +1,8 @@
let z = "outside";
const obj = {
[z]() {
let a = "inside";
return a;
}

};
9 changes: 9 additions & 0 deletions packages/babel-traverse/test/fixtures/rename/method-computed-key-1/plugin.js
View file
Open in desktop
@@ -0,0 +1,9 @@
module.exports = function() {
return {
visitor: {
Program(path) {
path.scope.rename("a", "z");
}
}
};
};
8 changes: 8 additions & 0 deletions packages/babel-traverse/test/fixtures/rename/method-computed-key-2/input.js
View file
Open in desktop
@@ -0,0 +1,8 @@
let a = "outside";

class C {
[a]() {
let a = "inside";
return a;
}
}
3 changes: 3 additions & 0 deletions packages/babel-traverse/test/fixtures/rename/method-computed-key-2/options.json
View file
Open in desktop
@@ -0,0 +1,3 @@
{
"plugins": ["./plugin"]
}
9 changes: 9 additions & 0 deletions packages/babel-traverse/test/fixtures/rename/method-computed-key-2/output.js
View file
Open in desktop
@@ -0,0 +1,9 @@
let z = "outside";

class C {
[z]() {
let a = "inside";
return a;
}

}
9 changes: 9 additions & 0 deletions packages/babel-traverse/test/fixtures/rename/method-computed-key-2/plugin.js
View file
Open in desktop
@@ -0,0 +1,9 @@
module.exports = function() {
return {
visitor: {
Program(path) {
path.scope.rename("a", "z");
}
}
};
};
7 changes: 7 additions & 0 deletions packages/babel-traverse/test/fixtures/rename/method-computed-key-3/input.js
View file
Open in desktop
@@ -0,0 +1,7 @@
let a = "outside";

const obj = {
[a](a = "inside") {
return a;
}
};
3 changes: 3 additions & 0 deletions packages/babel-traverse/test/fixtures/rename/method-computed-key-3/options.json
View file
Open in desktop
@@ -0,0 +1,3 @@
{
"plugins": ["./plugin"]
}
7 changes: 7 additions & 0 deletions packages/babel-traverse/test/fixtures/rename/method-computed-key-3/output.js
View file
Open in desktop
@@ -0,0 +1,7 @@
let z = "outside";
const obj = {
[z](a = "inside") {
return a;
}

};
9 changes: 9 additions & 0 deletions packages/babel-traverse/test/fixtures/rename/method-computed-key-3/plugin.js
View file
Open in desktop
@@ -0,0 +1,9 @@
module.exports = function() {
return {
visitor: {
Program(path) {
path.scope.rename("a", "z");
}
}
};
};
7 changes: 7 additions & 0 deletions packages/babel-traverse/test/fixtures/rename/method-computed-key-4/input.js
View file
Open in desktop
@@ -0,0 +1,7 @@
let a = "outside";

class C {
[a](a = "inside") {
return a;
}
}
3 changes: 3 additions & 0 deletions packages/babel-traverse/test/fixtures/rename/method-computed-key-4/options.json
View file
Open in desktop
@@ -0,0 +1,3 @@
{
"plugins": ["./plugin"]
}
8 changes: 8 additions & 0 deletions packages/babel-traverse/test/fixtures/rename/method-computed-key-4/output.js
View file
Open in desktop
@@ -0,0 +1,8 @@
let z = "outside";

class C {
[z](a = "inside") {
return a;
}

}
9 changes: 9 additions & 0 deletions packages/babel-traverse/test/fixtures/rename/method-computed-key-4/plugin.js
View file
Open in desktop
@@ -0,0 +1,9 @@
module.exports = function() {
return {
visitor: {
Program(path) {
path.scope.rename("a", "z");
}
}
};
};
8 changes: 8 additions & 0 deletions packages/babel-traverse/test/fixtures/rename/method-computed-key-5/input.js
View file
Open in desktop
@@ -0,0 +1,8 @@
let a = "outside";

const obj = {
[(() => a)()]() {
let a = "inside";
return a;
}
};
3 changes: 3 additions & 0 deletions packages/babel-traverse/test/fixtures/rename/method-computed-key-5/options.json
View file
Open in desktop
@@ -0,0 +1,3 @@
{
"plugins": ["./plugin"]
}
8 changes: 8 additions & 0 deletions packages/babel-traverse/test/fixtures/rename/method-computed-key-5/output.js
View file
Open in desktop
@@ -0,0 +1,8 @@
let z = "outside";
const obj = {
[(() => z)()]() {
let a = "inside";
return a;
}

};
9 changes: 9 additions & 0 deletions packages/babel-traverse/test/fixtures/rename/method-computed-key-5/plugin.js
View file
Open in desktop
@@ -0,0 +1,9 @@
module.exports = function() {
return {
visitor: {
Program(path) {
path.scope.rename("a", "z");
}
}
};
};
8 changes: 8 additions & 0 deletions packages/babel-traverse/test/fixtures/rename/method-computed-key-6/input.js
View file
Open in desktop
@@ -0,0 +1,8 @@
let a = "outside";

class C {
[(() => a)()]() {
let a = "inside";
return a;
}
}
3 changes: 3 additions & 0 deletions packages/babel-traverse/test/fixtures/rename/method-computed-key-6/options.json
View file
Open in desktop
@@ -0,0 +1,3 @@
{
"plugins": ["./plugin"]
}
9 changes: 9 additions & 0 deletions packages/babel-traverse/test/fixtures/rename/method-computed-key-6/output.js
View file
Open in desktop
@@ -0,0 +1,9 @@
let z = "outside";

class C {
[(() => z)()]() {
let a = "inside";
return a;
}

}
9 changes: 9 additions & 0 deletions packages/babel-traverse/test/fixtures/rename/method-computed-key-6/plugin.js
View file
Open in desktop
@@ -0,0 +1,9 @@
module.exports = function() {
return {
visitor: {
Program(path) {
path.scope.rename("a", "z");
}
}
};
};
15 changes: 15 additions & 0 deletions packages/babel-traverse/test/fixtures/rename/method-computed-key-7/input.js
View file
Open in desktop
@@ -0,0 +1,15 @@
let a = "outside";

const obj = {
get [
{
get [a]() {
let a = "inside";
return a;
}
}.outside
]() {
let a = "middle";
return a;
}
};
3 changes: 3 additions & 0 deletions packages/babel-traverse/test/fixtures/rename/method-computed-key-7/options.json
View file
Open in desktop
@@ -0,0 +1,3 @@
{
"plugins": ["./plugin"]
}
14 changes: 14 additions & 0 deletions packages/babel-traverse/test/fixtures/rename/method-computed-key-7/output.js
View file
Open in desktop
@@ -0,0 +1,14 @@
let z = "outside";
const obj = {
get [{
get [z]() {
let a = "inside";
return a;
}

}.outside]() {
let a = "middle";
return a;
}

};
9 changes: 9 additions & 0 deletions packages/babel-traverse/test/fixtures/rename/method-computed-key-7/plugin.js
View file
Open in desktop
@@ -0,0 +1,9 @@
module.exports = function() {
return {
visitor: {
Program(path) {
path.scope.rename("a", "z");
}
}
};
};
15 changes: 15 additions & 0 deletions packages/babel-traverse/test/fixtures/rename/method-computed-key-8/input.js
View file
Open in desktop
@@ -0,0 +1,15 @@
let a = "outside";

class C {
static get [
class D {
static get [a]() {
let a = "inside";
return a;
}
}.outside
]() {
let a = "middle";
return a;
}
}
3 changes: 3 additions & 0 deletions packages/babel-traverse/test/fixtures/rename/method-computed-key-8/options.json
View file
Open in desktop
@@ -0,0 +1,3 @@
{
"plugins": ["./plugin"]
}
15 changes: 15 additions & 0 deletions packages/babel-traverse/test/fixtures/rename/method-computed-key-8/output.js
View file
Open in desktop
@@ -0,0 +1,15 @@
let z = "outside";

class C {
static get [class D {
static get [z]() {
let a = "inside";
return a;
}

}.outside]() {
let a = "middle";
return a;
}

}
9 changes: 9 additions & 0 deletions packages/babel-traverse/test/fixtures/rename/method-computed-key-8/plugin.js
View file
Open in desktop
@@ -0,0 +1,9 @@
module.exports = function() {
return {
visitor: {
Program(path) {
path.scope.rename("a", "z");
}
}
};
};