Skip to content

Commit

Permalink
fix: fix static private field shadowed by local variable (#13656)
Browse files Browse the repository at this point in the history
* fix: fix static private field shadowed by local variable

currently throw an error, maybe we could generate correct code

fix #12960

* feat: rename local variable and add test cases

* feat: add unshadow to privateIn visitor

also add test cases

* test: add reference to shadowed variable

* refactor: apply suggested changes

simplify logic and add comments
  • Loading branch information
colinaaa committed Aug 30, 2021
1 parent a54f041 commit 313ecb5
Show file tree
Hide file tree
Showing 22 changed files with 291 additions and 8 deletions.
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);
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;
}
}

0 comments on commit 313ecb5

Please sign in to comment.