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: fix static private field shadowed by local variable #13656

Merged
merged 5 commits into from Aug 30, 2021

Conversation

@colinaaa
Copy link
Contributor

@colinaaa colinaaa commented Aug 8, 2021

Q                       A
Fixed Issues? Fixes #12960
Patch: Bug Fix? 👍
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? 👍
Documentation PR Link
Any Dependency Changes?
License MIT

use innerBinding created by #13429, and check whether the classRef for private fields equals the innerBinding.

currently throw an error, maybe we could generate correct code

Q: any idea how could we generate correct code?
Should we rename the local variable?
Or rename the class?
Or capture a separate reference in a temp variable before it's shadowed?

currently throw an error, maybe we could generate correct code

fix babel#12960
@babel-bot
Copy link
Collaborator

@babel-bot babel-bot commented Aug 8, 2021

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

@codesandbox
Copy link

@codesandbox codesandbox bot commented Aug 8, 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 68decd2:

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

@@ -273,6 +274,13 @@ const privateNameHandlerSpec: Handler<PrivateNameState & Receiver> & Receiver =
? "classStaticPrivateMethodGet"
: "classStaticPrivateFieldSpecGet";

const binding = member.scope.getBinding(classRef.name);
if (innerBinding && binding && !(binding.identifier === innerBinding)) {
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo Aug 9, 2021

Nit:

Suggested change
if (innerBinding && binding && !(binding.identifier === innerBinding)) {
if (innerBinding && binding && binding.identifier !== innerBinding) {

Also, binding should always be defined (because if there isn't a conflicting variable, it's innerBinding). It would be better to assert it, rather than checking it:

if (innerBinding) {
  if (!binding) throw new Error("Internal Babel error: binding should be defined");
  if (binding.identifier !== innerBinding) {
    // ...
  }
}

Copy link
Contributor Author

@colinaaa colinaaa Aug 10, 2021

Infect, binding could be undefined.

e.g: for a ClassExpression, classRef is generated by path.scope.generateUidIdentifier("class"); and would be inserted after our transformation.

const cls = class Test {
  static #x = 1
  method() {
    const Test = 1;
    return this.#x;
  }
}

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo Aug 10, 2021

Oh thanks, I didn't realize it

throw binding.path.buildCodeFrameError(
`Shadowing class ${classRef.name} with private property`,
);
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo Aug 9, 2021

Q: any idea how could we generate correct code?
Should we rename the local variable?

Yeah, I'd rename the local variable:

path.scope.rename(classRef.name);

Copy link
Contributor Author

@colinaaa colinaaa Aug 10, 2021

Sure!

I renamed all the local variables that shadowing the classRef including those in parent scopes.

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Thanks!

innerBinding: t.Identifier | undefined,
) {
const binding = scope.getBinding(name);
if (innerBinding && binding && innerBinding !== binding.identifier) {
Copy link
Contributor

@JLHwung JLHwung Aug 13, 2021

The if branch is an inlined version of Scope#bindingIdentifierEquals, guess we can just put

while (!scope.bindingIdentifierEquals(name, innerBinding)) {
      scope.rename(name);
      scope = scope.parent;
    }

here?

Copy link
Contributor Author

@colinaaa colinaaa Aug 14, 2021

Sounds great! And all tests passed.

But I got a question: there are some cases that scope.bindingIdentifier(name) === undefined and innerBinding !== undefined which means scope.bindingIdentifierEquals(name, innerBinding) will always returns false. This will cause the while loop runs all the way alone to the top level scope and renaming all the variables.

The renaming and looping here are useless, and maybe cause performance problem.

So maybe we change it to:

  while (
    scope?.hasBinding(name) &&
    !scope.bindingIdentifierEquals(name, innerBinding)
  ) {
    scope.rename(name);
    scope = scope.parent;
  }

Copy link
Contributor

@JLHwung JLHwung Aug 18, 2021

there are some cases that scope.bindingIdentifier(name) === undefined and innerBinding !== undefined

If the scope is the parent scope of the class where innerBinding is defined, then scope.bindingIdentifier(classRef.name) is surely undefined, unless defined otherwise. We could exit the loop after scope becomes the class scope.

simplify logic and add comments
scope?.hasBinding(name) &&
!scope.bindingIdentifierEquals(name, innerBinding)
) {
scope.rename(name);
Copy link
Member

@jridgewell jridgewell Aug 18, 2021

Can renaming ever fail? I can't think of a case myself.

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo Aug 30, 2021

No, it always succeeds (but it might be observable since it sometimes changes the .name property of functions).

@nicolo-ribaudo nicolo-ribaudo merged commit 313ecb5 into babel:main Aug 30, 2021
25 of 27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants