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 all 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
44 changes: 37 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,28 @@ const privateNameVisitor = privateNameVisitorFactory<
},
});

// rename all bindings that shadows innerBinding
function unshadow(
name: string,
scope: Scope,
innerBinding: t.Identifier | undefined,
) {
// in some cases, scope.getBinding(name) === undefined
// so we check hasBinding to avoid keeping looping
// see: https://github.com/babel/babel/pull/13656#discussion_r686030715
while (
scope?.hasBinding(name) &&
!scope.bindingIdentifierEquals(name, innerBinding)
) {
scope.rename(name);
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

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 +224,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 +279,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 +297,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 +491,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 +507,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 +800,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 +861,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;
}
}