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.

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

Merged
merged 4 commits into from Feb 19, 2021

Conversation

overlookmotel
Copy link
Contributor

@overlookmotel overlookmotel commented Feb 17, 2021

Q                       A
Fixed Issues? Partial fix for #12790
Patch: Bug Fix? Yes
Major: Breaking Change? No
Minor: New Feature? No
Tests Added + Pass? Yes
Documentation PR Link n/a
Any Dependency Changes? No
License MIT

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.

@babel-bot
Copy link
Collaborator

babel-bot commented Feb 17, 2021

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/40995/

@codesandbox-ci
Copy link

codesandbox-ci bot commented Feb 17, 2021

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:

Sandbox Source
babel-repl-custom-plugin Configuration
babel-plugin-multi-config Configuration

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.

@overlookmotel
Copy link
Contributor Author

Added a fix for bug (2) so scope.rename() now treats computed method keys correctly.

@nicolo-ribaudo
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo added pkg: traverse (scope) PR: Bug Fix 🐛 A type of pull request used for our changelog categories labels Feb 17, 2021
@overlookmotel
Copy link
Contributor Author

@nicolo-ribaudo Test added.

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@nicolo-ribaudo nicolo-ribaudo merged commit 792672e into babel:main Feb 19, 2021
@nicolo-ribaudo
Copy link
Member

(I meant to approve ✔️)

@overlookmotel
Copy link
Contributor Author

Thank you!

I'm sure everyone asks this but... any chance you'll be doing a release soon?

@overlookmotel overlookmotel deleted the computed-key-scope branch February 20, 2021 01:02
@nicolo-ribaudo
Copy link
Member

Maybe Monday but I don't promise.

@overlookmotel
Copy link
Contributor Author

I see you kept your non-existent promise! Thanks.

This was referenced Mar 11, 2021
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label May 26, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: traverse (scope) PR: Bug Fix 🐛 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants