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.

Sign up for GitHub

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

Jump to bottom

Fix nested classes reference private fields #11405

Merged
merged 5 commits into from Apr 14, 2020
Merged

Fix nested classes reference private fields #11405

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
808445b
Fix nested classes reference private fields
jridgewell Apr 11, 2020
995a6be
Process only visible private fields when redeclaring
jridgewell Apr 12, 2020
d450792
Comments
jridgewell Apr 12, 2020
27a09f1
Skip class traversal if there are no private fields
jridgewell Apr 12, 2020
3a673dc
Handle redeclared private field in computed key
jridgewell Apr 14, 2020
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
50 changes: 33 additions & 17 deletions packages/babel-helper-create-class-features-plugin/src/fields.js
View file
Open in desktop
Expand Up @@ -7,35 +7,41 @@ import optimiseCall from "@babel/helper-optimise-call-expression";

import * as ts from "./typescript";

export function buildPrivateNamesMap(props) {
const privateNamesMap = new Map();
export function buildPrivateNamesMap(privateNamesMap, props, depth) {
for (const prop of props) {
const isPrivate = prop.isPrivate();
const isMethod = !prop.isProperty();
const isInstance = !prop.node.static;
if (isPrivate) {
if (prop.isPrivate()) {
const isMethod = !prop.isProperty();
const isInstance = !prop.node.static;
const { name } = prop.node.key.id;
const update = privateNamesMap.has(name)
? privateNamesMap.get(name)
: {
id: prop.scope.generateUidIdentifier(name),
static: !isInstance,
method: isMethod,
};

let update = privateNamesMap.get(name);
if (!update || update.depth !== depth) {
update = {
id: prop.scope.generateUidIdentifier(name),
static: !isInstance,
method: isMethod,
depth,
};
privateNamesMap.set(name, update);
}

if (prop.node.kind === "get") {
update.getId = prop.scope.generateUidIdentifier(`get_${name}`);
} else if (prop.node.kind === "set") {
update.setId = prop.scope.generateUidIdentifier(`set_${name}`);
} else if (prop.node.kind === "method") {
update.methodId = prop.scope.generateUidIdentifier(name);
}
privateNamesMap.set(name, update);
}
}
return privateNamesMap;
}

export function buildPrivateNamesNodes(privateNamesMap, loose, state) {
export function buildPrivateNamesNodes(
privateNamesMap,
loose,
state,
privateDepth,
) {
const initNodes = [];

for (const [name, value] of privateNamesMap) {
Expand All @@ -45,7 +51,16 @@ export function buildPrivateNamesNodes(privateNamesMap, loose, state) {
// In spec mode, only instance fields need a "private name" initializer
// because static fields are directly assigned to a variable in the
// buildPrivateStaticFieldInitSpec function.
const { id, static: isStatic, method: isMethod, getId, setId } = value;
const {
id,
static: isStatic,
method: isMethod,
getId,
setId,
depth,
} = value;
if (depth !== privateDepth) continue;

const isAccessor = getId || setId;
if (loose) {
initNodes.push(
Expand Down Expand Up @@ -263,6 +278,7 @@ export function transformPrivateNamesUsage(
loose,
state,
) {
if (!privateNamesMap.size) return;
const body = path.get("body");

if (loose) {
Expand Down
245 changes: 132 additions & 113 deletions packages/babel-helper-create-class-features-plugin/src/index.js
View file
Open in desktop
Expand Up @@ -30,6 +30,8 @@ export { FEATURES, injectInitialization };
const version = pkg.version.split(".").reduce((v, x) => v * 1e5 + +x, 0);
const versionKey = "@babel/plugin-class-features/version";

const privateNamesStack = [new Map()];
Copy link
Member

Choose a reason for hiding this comment

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

When using a WeakSet, please put this in the createClassFeaturePluign function so that when the plugin finishes running it can be garbage-collected, even if the AST was still retained.


export function createClassFeaturePlugin({
name,
feature,
Expand All @@ -49,141 +51,158 @@ export function createClassFeaturePlugin({
},

visitor: {
Class(path, state) {
if (this.file.get(versionKey) !== version) return;
Class: {
enter(path, state) {
if (this.file.get(versionKey) !== version) return;

verifyUsedFeatures(path, this.file);

const loose = isLoose(this.file, feature);
verifyUsedFeatures(path, this.file);

let constructor;
let isDecorated = hasOwnDecorators(path.node);
const props = [];
const elements = [];
const computedPaths = [];
const privateNames = new Set();
const body = path.get("body");
const loose = isLoose(this.file, feature);
const privateNamesMap = new Map(
privateNamesStack[privateNamesStack.length - 1],
Copy link
Member

Choose a reason for hiding this comment

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

Since nodes can be re-queued and skipped, it's not guaranteed that they are visited in-order.

Instead of a stack, I think we could use a WeakMap from class nodes to privateNamesMap, and get the parent class using path.findParent(p => p.isClass()).

);
privateNamesStack.push(privateNamesMap);

for (const path of body.get("body")) {
verifyUsedFeatures(path, this.file);
let constructor;
let isDecorated = hasOwnDecorators(path.node);
const props = [];
const elements = [];
const computedPaths = [];
const privateNames = new Set();
const body = path.get("body");

if (path.node.computed) {
computedPaths.push(path);
}
for (const path of body.get("body")) {
verifyUsedFeatures(path, this.file);

if (path.isPrivate()) {
const { name } = path.node.key.id;
const getName = `get ${name}`;
const setName = `set ${name}`;

if (path.node.kind === "get") {
if (
privateNames.has(getName) ||
(privateNames.has(name) && !privateNames.has(setName))
) {
throw path.buildCodeFrameError("Duplicate private field");
}
if (path.node.computed) {
computedPaths.push(path);
}

privateNames.add(getName).add(name);
} else if (path.node.kind === "set") {
if (
privateNames.has(setName) ||
(privateNames.has(name) && !privateNames.has(getName))
) {
throw path.buildCodeFrameError("Duplicate private field");
if (path.isPrivate()) {
const { name } = path.node.key.id;
const getName = `get ${name}`;
const setName = `set ${name}`;

if (path.node.kind === "get") {
if (
privateNames.has(getName) ||
(privateNames.has(name) && !privateNames.has(setName))
) {
throw path.buildCodeFrameError("Duplicate private field");
}

privateNames.add(getName).add(name);
} else if (path.node.kind === "set") {
if (
privateNames.has(setName) ||
(privateNames.has(name) && !privateNames.has(getName))
) {
throw path.buildCodeFrameError("Duplicate private field");
}

privateNames.add(setName).add(name);
} else {
if (
(privateNames.has(name) &&
!privateNames.has(getName) &&
!privateNames.has(setName)) ||
(privateNames.has(name) &&
(privateNames.has(getName) || privateNames.has(setName)))
) {
throw path.buildCodeFrameError("Duplicate private field");
}

privateNames.add(name);
}
}

privateNames.add(setName).add(name);
if (path.isClassMethod({ kind: "constructor" })) {
constructor = path;
} else {
if (
(privateNames.has(name) &&
!privateNames.has(getName) &&
!privateNames.has(setName)) ||
(privateNames.has(name) &&
(privateNames.has(getName) || privateNames.has(setName)))
) {
throw path.buildCodeFrameError("Duplicate private field");
elements.push(path);
if (path.isProperty() || path.isPrivate()) {
props.push(path);
}

privateNames.add(name);
}

if (!isDecorated) isDecorated = hasOwnDecorators(path.node);
}

if (path.isClassMethod({ kind: "constructor" })) {
constructor = path;
if (!props.length && !isDecorated) return;

let ref;

if (path.isClassExpression() || !path.node.id) {
nameFunction(path);
ref = path.scope.generateUidIdentifier("class");
} else {
elements.push(path);
if (path.isProperty() || path.isPrivate()) {
props.push(path);
}
ref = path.node.id;
}

if (!isDecorated) isDecorated = hasOwnDecorators(path.node);
}
// NODE: These three functions don't support decorators yet,
// but verifyUsedFeatures throws if there are both
// decorators and private fields.
const depth = privateNamesStack.length;
buildPrivateNamesMap(privateNamesMap, props, depth);
const privateNamesNodes = buildPrivateNamesNodes(
privateNamesMap,
loose,
state,
depth,
);

if (!props.length && !isDecorated) return;
transformPrivateNamesUsage(ref, path, privateNamesMap, loose, state);

let ref;
let keysNodes, staticNodes, instanceNodes, wrapClass;

if (path.isClassExpression() || !path.node.id) {
nameFunction(path);
ref = path.scope.generateUidIdentifier("class");
} else {
ref = path.node.id;
}
if (isDecorated) {
staticNodes = keysNodes = [];
({ instanceNodes, wrapClass } = buildDecoratedClass(
ref,
path,
elements,
this.file,
));
} else {
keysNodes = extractComputedKeys(
ref,
path,
computedPaths,
this.file,
);
({ staticNodes, instanceNodes, wrapClass } = buildFieldsInitNodes(
ref,
path.node.superClass,
props,
privateNamesMap,
state,
loose,
));
}

// NODE: These three functions don't support decorators yet,
// but verifyUsedFeatures throws if there are both
// decorators and private fields.
const privateNamesMap = buildPrivateNamesMap(props);
const privateNamesNodes = buildPrivateNamesNodes(
privateNamesMap,
loose,
state,
);

transformPrivateNamesUsage(ref, path, privateNamesMap, loose, state);

let keysNodes, staticNodes, instanceNodes, wrapClass;

if (isDecorated) {
staticNodes = keysNodes = [];
({ instanceNodes, wrapClass } = buildDecoratedClass(
ref,
path,
elements,
this.file,
));
} else {
keysNodes = extractComputedKeys(ref, path, computedPaths, this.file);
({ staticNodes, instanceNodes, wrapClass } = buildFieldsInitNodes(
ref,
path.node.superClass,
props,
privateNamesMap,
state,
loose,
));
}
if (instanceNodes.length > 0) {
injectInitialization(
path,
constructor,
instanceNodes,
(referenceVisitor, state) => {
if (isDecorated) return;
for (const prop of props) {
if (prop.node.static) continue;
prop.traverse(referenceVisitor, state);
}
},
);
}

if (instanceNodes.length > 0) {
injectInitialization(
path,
constructor,
instanceNodes,
(referenceVisitor, state) => {
if (isDecorated) return;
for (const prop of props) {
if (prop.node.static) continue;
prop.traverse(referenceVisitor, state);
}
},
);
}
path = wrapClass(path);
path.insertBefore(keysNodes);
path.insertAfter([...privateNamesNodes, ...staticNodes]);
},

path = wrapClass(path);
path.insertBefore(keysNodes);
path.insertAfter([...privateNamesNodes, ...staticNodes]);
exit() {
privateNamesStack.pop();
},
},

PrivateName(path) {
Expand Down
18 changes: 18 additions & 0 deletions ...posal-class-properties/test/fixtures/private-loose/nested-class-other-redeclared/input.js
View file
Open in desktop
@@ -0,0 +1,18 @@
class Foo {
#foo = 1;
#bar = 1;

test() {
class Nested {
#bar = 2;

test() {
this.#foo;
this.#bar;
}
}

this.#foo;
this.#bar;
}
}