fix: fix static private field shadowed by local variable

[colinaaa]

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?

[babel-bot]

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

[codesandbox-ci]

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

[nicolo-ribaudo]

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) {
    // ...
  }
}

[colinaaa]

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

[nicolo-ribaudo]

Oh thanks, I didn't realize it

[nicolo-ribaudo]

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

[colinaaa]

Sure!

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

[nicolo-ribaudo]

Thanks!

[JLHwung]

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?

[colinaaa]

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

[JLHwung]

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.

[jridgewell]

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

[nicolo-ribaudo]

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