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
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
42 changes: 35 additions & 7 deletions packages/babel-helper-create-class-features-plugin/src/fields.ts
@@ -1,6 +1,6 @@
import { template, traverse, types as t } from "@babel/core";
import type { File } from "@babel/core";
import type { NodePath, Visitor } from "@babel/traverse";
import type { NodePath, Visitor, Scope } from "@babel/traverse";
import ReplaceSupers, {
environmentVisitor,
} from "@babel/helper-replace-supers";
Expand Down Expand Up @@ -58,7 +58,7 @@ export function buildPrivateNamesMap(props: PropPath[]) {
export function buildPrivateNamesNodes(
privateNamesMap: PrivateNamesMap,
privateFieldsAsProperties: boolean,
state,
state: File,
) {
const initNodes: t.Statement[] = [];

Expand Down Expand Up @@ -165,6 +165,7 @@ interface PrivateNameState {
classRef: t.Identifier;
file: File;
noDocumentAll: boolean;
innerBinding?: t.Identifier;
}

const privateNameVisitor = privateNameVisitorFactory<
Expand All @@ -188,9 +189,26 @@ const privateNameVisitor = privateNameVisitorFactory<
},
});

// rename all bindings that shadows innerBinding
function unshadow(
name: string,
scope: Scope,
innerBinding: t.Identifier | undefined,
) {
const binding = scope.getBinding(name);
if (innerBinding && binding && innerBinding !== binding.identifier) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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.

// the classRef has been shadowed, rename the local variable
while (!scope.bindingIdentifierEquals(name, innerBinding)) {
scope.rename(name);
scope = scope.parent;
}
}
}

const privateInVisitor = privateNameVisitorFactory<{
classRef: t.Identifier;
file: File;
innerBinding?: t.Identifier;
}>({
BinaryExpression(path) {
const { operator, left, right } = path.node;
Expand All @@ -204,6 +222,10 @@ const privateInVisitor = privateNameVisitorFactory<{
if (!privateNamesMap.has(name)) return;
if (redeclared && redeclared.includes(name)) return;

// if there are any local variable shadowing classRef, unshadow it
// see #12960
unshadow(this.classRef.name, path.scope, this.innerBinding);

if (privateFieldsAsProperties) {
const { id } = privateNamesMap.get(name);
path.replaceWith(template.expression.ast`
Expand Down Expand Up @@ -255,7 +277,7 @@ const privateNameHandlerSpec: Handler<PrivateNameState & Receiver> & Receiver =
},

get(member) {
const { classRef, privateNamesMap, file } = this;
const { classRef, privateNamesMap, file, innerBinding } = this;
const { name } = (member.node.property as t.PrivateName).id;
const {
id,
Expand All @@ -273,6 +295,10 @@ const privateNameHandlerSpec: Handler<PrivateNameState & Receiver> & Receiver =
? "classStaticPrivateMethodGet"
: "classStaticPrivateFieldSpecGet";

// if there are any local variable shadowing classRef, unshadow it
// see #12960
unshadow(classRef.name, member.scope, innerBinding);

return t.callExpression(file.addHelper(helperName), [
this.receiver(member),
t.cloneNode(classRef),
Expand Down Expand Up @@ -463,8 +489,8 @@ export function transformPrivateNamesUsage(
ref: t.Identifier,
path: NodePath<t.Class>,
privateNamesMap: PrivateNamesMap,
{ privateFieldsAsProperties, noDocumentAll },
state,
{ privateFieldsAsProperties, noDocumentAll, innerBinding },
state: File,
) {
if (!privateNamesMap.size) return;

Expand All @@ -479,12 +505,14 @@ export function transformPrivateNamesUsage(
file: state,
...handler,
noDocumentAll,
innerBinding,
});
body.traverse(privateInVisitor, {
privateNamesMap,
classRef: ref,
file: state,
privateFieldsAsProperties,
innerBinding,
});
}

Expand Down Expand Up @@ -770,7 +798,7 @@ const thisContextVisitor = traverse.visitors.merge([
]);

const innerReferencesVisitor = {
ReferencedIdentifier(path, state) {
ReferencedIdentifier(path: NodePath<t.Identifier>, state) {
if (
path.scope.bindingIdentifierEquals(path.node.name, state.innerBinding)
) {
Expand Down Expand Up @@ -831,7 +859,7 @@ export function buildFieldsInitNodes(
superRef: t.Expression | undefined,
props: PropPath[],
privateNamesMap: PrivateNamesMap,
state,
state: File,
setPublicClassFields: boolean,
privateFieldsAsProperties: boolean,
constantSuper: boolean,
Expand Down
@@ -1,4 +1,5 @@
import { types as t } from "@babel/core";
import type { File } from "@babel/core";
import type { NodePath } from "@babel/traverse";
import nameFunction from "@babel/helper-function-name";
import splitExportDeclaration from "@babel/helper-split-export-declaration";
Expand Down Expand Up @@ -92,7 +93,7 @@ export function createClassFeaturePlugin({
},

visitor: {
Class(path: NodePath<t.Class>, state) {
Class(path: NodePath<t.Class>, state: File) {
if (this.file.get(versionKey) !== version) return;

verifyUsedFeatures(path, this.file);
Expand Down Expand Up @@ -198,6 +199,7 @@ export function createClassFeaturePlugin({
{
privateFieldsAsProperties: privateFieldsAsProperties ?? loose,
noDocumentAll,
innerBinding,
},
state,
);
Expand Down
@@ -0,0 +1,15 @@
class Test {

static #x = 1

static method() {
const Test = 2;
const func = () => {
const Test = 3;
return this.#x + Test;
}
return func() + Test;
}
}

expect(Test.method()).toBe(6)
@@ -0,0 +1,13 @@
class Test {

static #x = 1

static method() {
const Test = 2;
const func = () => {
const Test = 3;
return this.#x + Test;
}
return func() + Test;
}
}
@@ -0,0 +1,3 @@
{
"plugins": ["proposal-class-properties"]
}
@@ -0,0 +1,18 @@
class Test {
static method() {
const _Test2 = 2;

const func = () => {
const _Test = 3;
return babelHelpers.classStaticPrivateFieldSpecGet(this, Test, _x) + _Test;
};

return func() + _Test2;
}

}

var _x = {
writable: true,
value: 1
};
@@ -0,0 +1,15 @@
class Test {

static #x = 1

static method() {
const Test = 2;
const func = () => {
const Test = 3;
return this.#x + Test;
}
return func() + Test;
}
}

expect(Test.method()).toBe(6)
@@ -0,0 +1,13 @@
class Test {

static #x = 1

static method() {
const Test = 2;
const func = () => {
const Test = 3;
return this.#x + Test;
}
return func() + Test;
}
}
@@ -0,0 +1,3 @@
{
"plugins": ["proposal-class-properties"]
}
@@ -0,0 +1,18 @@
class Test {
static method() {
const _Test2 = 2;

const func = () => {
const _Test = 3;
return babelHelpers.classStaticPrivateFieldSpecGet(this, Test, _x) + _Test;
};

return func() + _Test2;
}

}

var _x = {
writable: true,
value: 1
};
@@ -0,0 +1,17 @@
class Test {

static #x = 1

method(other) {
const Test = 2;
const func = () => {
const Test = 3;
return #x in other && Test;
}
return func() + Test;
}
}

const t = new Test();
const t2 = new Test();
expect(t.method(Test)).toBe(5)
@@ -0,0 +1,13 @@
class Test {

static #x = 1

method(other) {
const Test = 2;
const func = () => {
const Test = 3;
return #x in other && Test;
}
return func() + Test;
}
}
@@ -0,0 +1,3 @@
{
"plugins": ["proposal-class-properties"]
}
@@ -0,0 +1,18 @@
class Test {
method(other) {
const _Test2 = 2;

const func = () => {
const _Test = 3;
return other === Test && _Test;
};

return func() + _Test2;
}

}

var _x = {
writable: true,
value: 1
};
@@ -0,0 +1,17 @@
class Test {

static #x = 1

method(other) {
const Test = 2;
const func = () => {
const Test = 3;
return #x in other && Test;
}
return func() + Test;
}
}

const t = new Test();
const t2 = new Test();
expect(t.method(Test)).toBe(5)
@@ -0,0 +1,13 @@
class Test {

static #x = 1

method(other) {
const Test = 2;
const func = () => {
const Test = 3;
return #x in other && Test;
}
return func() + Test;
}
}
@@ -0,0 +1,3 @@
{
"plugins": ["proposal-class-properties"]
}
@@ -0,0 +1,18 @@
class Test {
method(other) {
const _Test2 = 2;

const func = () => {
const _Test = 3;
return other === Test && _Test;
};

return func() + _Test2;
}

}

var _x = {
writable: true,
value: 1
};
@@ -0,0 +1,17 @@
class Test {

static #x = 1

method(other) {
const Test = 2;
const func = () => {
const Test = 3;
return #x in other && Test;
}
return func() + Test;
}
}

const t = new Test();
const t2 = new Test();
expect(t.method(Test)).toBe(5)
@@ -0,0 +1,13 @@
class Test {

static #x = 1

method(other) {
const Test = 2;
const func = () => {
const Test = 3;
return #x in other && Test;
}
return func() + Test;
}
}