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

Private Class Methods Stage 3: Private Accessors #9101

Merged
merged 11 commits into from Jan 21, 2019
Merged

Private Class Methods Stage 3: Private Accessors #9101

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
af5db96
Add accessor loose support
tim-mc Dec 21, 2018
621828a
Add private accessors spec support
tim-mc Dec 24, 2018
9e41394
Fix private dupe name check
tim-mc Dec 27, 2018
5fcbd17
Changes from code review
tim-mc Dec 27, 2018
83c135c
Add duplicated names tests
tim-mc Dec 28, 2018
a5d2172
Add get/set-only tests
tim-mc Dec 28, 2018
f0d6486
Move accessors tests
tim-mc Dec 28, 2018
2498ec2
Split out updates tests
tim-mc Dec 28, 2018
4a328b1
Add helper change tests
tim-mc Dec 28, 2018
d6ccba0
Update test output
tim-mc Jan 4, 2019
62a0ca4
Update test options
tim-mc Jan 7, 2019
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
6 changes: 0 additions & 6 deletions packages/babel-helper-create-class-features-plugin/src/features.js
View file
Open in desktop
Expand Up @@ -72,12 +72,6 @@ export function verifyUsedFeatures(path, file) {
"@babel/plugin-class-features doesn't support class static private methods yet.",
);
}

if (path.node.kind !== "method") {
throw path.buildCodeFrameError(
"@babel/plugin-class-features doesn't support class private accessors yet.",
);
}
}

if (
Expand Down
206 changes: 168 additions & 38 deletions packages/babel-helper-create-class-features-plugin/src/fields.js
View file
Open in desktop
Expand Up @@ -6,16 +6,26 @@ import optimiseCall from "@babel/helper-optimise-call-expression";
export function buildPrivateNamesMap(props) {
const privateNamesMap = new Map();
for (const prop of props) {
if (prop.isPrivate()) {
const isPrivate = prop.isPrivate();
const isMethod = !prop.isProperty();
const isInstance = !prop.node.static;
if (isPrivate) {
Copy link
Member

Choose a reason for hiding this comment

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

isPrivate seems only used here? Could you please inline it?

const { name } = prop.node.key.id;
privateNamesMap.set(name, {
id: prop.scope.generateUidIdentifier(name),
static: !!prop.node.static,
method: prop.isMethod(),
methodId: prop.isMethod()
? prop.scope.generateUidIdentifier(name)
: undefined,
});
const update = privateNamesMap.has(name)
? privateNamesMap.get(name)
: {
id: prop.scope.generateUidIdentifier(name),
static: !isInstance,
method: isMethod,
};
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" && isMethod && isInstance) {
update.methodId = prop.scope.generateUidIdentifier(name);
}
privateNamesMap.set(name, update);
}
}
return privateNamesMap;
Expand All @@ -31,15 +41,19 @@ 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 } = value;
const { id, static: isStatic, method: isMethod, getId, setId } = value;
if (loose) {
initNodes.push(
template.statement.ast`
var ${id} = ${state.addHelper("classPrivateFieldLooseKey")}("${name}")
`,
);
} else if (isMethod && !isStatic) {
initNodes.push(template.statement.ast`var ${id} = new WeakSet();`);
if (getId || setId) {
initNodes.push(template.statement.ast`var ${id} = new WeakMap();`);
} else {
initNodes.push(template.statement.ast`var ${id} = new WeakSet();`);
}
Copy link
Member

Choose a reason for hiding this comment

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

The only difference is WeakMap vs WeakSet. Could we switch to an AST here and just parametrize that?

} else if (!isStatic) {
initNodes.push(template.statement.ast`var ${id} = new WeakMap();`);
Copy link
Member

Choose a reason for hiding this comment

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

See comment above

}
Expand Down Expand Up @@ -121,48 +135,66 @@ const privateNameHandlerSpec = {
static: isStatic,
method: isMethod,
methodId,
getId,
setId,
} = privateNamesMap.get(name);

if (isStatic && !isMethod) {
return t.callExpression(
file.addHelper("classStaticPrivateFieldSpecGet"),
[this.receiver(member), t.cloneNode(classRef), t.cloneNode(id)],
);
} else if (isMethod) {
}
if (isMethod) {
if (getId || setId) {
return t.callExpression(file.addHelper("classPrivateFieldGet"), [
this.receiver(member),
t.cloneNode(id),
]);
}
return t.callExpression(file.addHelper("classPrivateMethodGet"), [
this.receiver(member),
t.cloneNode(id),
t.cloneNode(methodId),
]);
} else {
return t.callExpression(file.addHelper("classPrivateFieldGet"), [
this.receiver(member),
t.cloneNode(id),
]);
}
return t.callExpression(file.addHelper("classPrivateFieldGet"), [
this.receiver(member),
t.cloneNode(id),
]);
},

set(member, value) {
const { classRef, privateNamesMap, file } = this;
const { name } = member.node.property.id;
const { id, static: isStatic, method: isMethod } = privateNamesMap.get(
name,
);
const {
id,
static: isStatic,
method: isMethod,
setId,
} = privateNamesMap.get(name);

if (isStatic && !isMethod) {
return t.callExpression(
file.addHelper("classStaticPrivateFieldSpecSet"),
[this.receiver(member), t.cloneNode(classRef), t.cloneNode(id), value],
);
} else if (isMethod) {
}
if (isMethod) {
if (setId) {
return t.callExpression(file.addHelper("classPrivateFieldSet"), [
this.receiver(member),
t.cloneNode(id),
value,
]);
}
return t.callExpression(file.addHelper("classPrivateMethodSet"), []);
} else {
return t.callExpression(file.addHelper("classPrivateFieldSet"), [
this.receiver(member),
t.cloneNode(id),
value,
]);
}
return t.callExpression(file.addHelper("classPrivateFieldSet"), [
this.receiver(member),
t.cloneNode(id),
value,
]);
},

call(member, args) {
Expand Down Expand Up @@ -255,21 +287,91 @@ function buildPrivateStaticFieldInitSpec(prop, privateNamesMap) {
}

function buildPrivateMethodInitLoose(ref, prop, privateNamesMap) {
const { methodId, id } = privateNamesMap.get(prop.node.key.id.name);
const privateName = privateNamesMap.get(prop.node.key.id.name);
const { methodId, id, getId, setId, initAdded } = privateName;
if (initAdded) return;

if (methodId) {
return template.statement.ast`
Object.defineProperty(${ref}, ${id}, {
// configurable is false by default
// enumerable is false by default
// writable is false by default
value: ${methodId.name}
});
`;
}

return template.statement.ast`
Object.defineProperty(${ref}, ${id}, {
// configurable is false by default
// enumerable is false by default
// writable is false by default
value: ${methodId.name}
if (getId || setId) {
privateNamesMap.set(prop.node.key.id.name, {
...privateName,
initAdded: true,
});
`;

if (getId && setId) {
return template.statement.ast`
Object.defineProperty(${ref}, ${id}, {
// configurable is false by default
// enumerable is false by default
// writable is false by default
get: ${getId.name},
set: ${setId.name}
});
`;
} else if (getId && !setId) {
return template.statement.ast`
Object.defineProperty(${ref}, ${id}, {
// configurable is false by default
// enumerable is false by default
// writable is false by default
get: ${getId.name}
});
`;
} else if (!getId && setId) {
return template.statement.ast`
Object.defineProperty(${ref}, ${id}, {
// configurable is false by default
// enumerable is false by default
// writable is false by default
set: ${setId.name}
});
`;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

This looks quite verbose to me, could we switch it to building an AST and parametrizing the set/get? Also, I believe we have a few helpers that we could use here.

}

function buildPrivateInstanceMethodInitSpec(ref, prop, privateNamesMap) {
const { id } = privateNamesMap.get(prop.node.key.id.name);
const privateName = privateNamesMap.get(prop.node.key.id.name);
const { id, getId, setId, initAdded } = privateName;
if (initAdded) return;

if (getId || setId) {
privateNamesMap.set(prop.node.key.id.name, {
...privateName,
initAdded: true,
});

if (getId && setId) {
return template.statement.ast`
${id}.set(${ref}, {
get: ${getId.name},
set: ${setId.name}
});
`;
} else if (getId && !setId) {
return template.statement.ast`
${id}.set(${ref}, {
get: ${getId.name}
});
`;
} else if (!getId && setId) {
return template.statement.ast`
${id}.set(${ref}, {
set: ${setId.name}
});
`;
}
}
return template.statement.ast`${id}.add(${ref})`;
}

Expand Down Expand Up @@ -300,9 +402,37 @@ function buildPublicFieldInitSpec(ref, prop, state) {
}

function buildPrivateInstanceMethodDeclaration(prop, privateNamesMap) {
const { methodId } = privateNamesMap.get(prop.node.key.id.name);
const privateName = privateNamesMap.get(prop.node.key.id.name);
const {
methodId,
getId,
setId,
getterDeclared,
setterDeclared,
} = privateName;
const { params, body } = prop.node;
const methodValue = t.functionExpression(methodId, params, body);
const isGetter = getId && !getterDeclared && params.length === 0;
const isSetter = setId && !setterDeclared && params.length > 0;

if (isGetter) {
privateNamesMap.set(prop.node.key.id.name, {
...privateName,
getterDeclared: true,
});
return t.variableDeclaration("var", [
t.variableDeclarator(getId, methodValue),
]);
}
if (isSetter) {
privateNamesMap.set(prop.node.key.id.name, {
...privateName,
setterDeclared: true,
});
return t.variableDeclaration("var", [
t.variableDeclarator(setId, methodValue),
]);
}

return t.variableDeclaration("var", [
t.variableDeclarator(methodId, methodValue),
Expand Down Expand Up @@ -404,7 +534,7 @@ export function buildFieldsInitNodes(

return {
staticNodes,
instanceNodes,
instanceNodes: instanceNodes.filter(Boolean),
wrapClass(path) {
for (const prop of props) {
prop.remove();
Expand Down
34 changes: 31 additions & 3 deletions packages/babel-helper-create-class-features-plugin/src/index.js
View file
Open in desktop
Expand Up @@ -75,11 +75,39 @@ export function createClassFeaturePlugin({

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

if (privateNames.has(name)) {
throw path.buildCodeFrameError("Duplicate private field");
privateNames.add(name);
}
privateNames.add(name);
}

if (path.isClassMethod({ kind: "constructor" })) {
Expand Down
24 changes: 17 additions & 7 deletions packages/babel-helpers/src/helpers.js
View file
Open in desktop
Expand Up @@ -1048,7 +1048,11 @@ helpers.classPrivateFieldGet = helper("7.0.0-beta.0")`
if (!privateMap.has(receiver)) {
throw new TypeError("attempted to get private field on non-instance");
}
return privateMap.get(receiver).value;
var descriptor = privateMap.get(receiver);
if (descriptor.get) {
return descriptor.get.call(receiver);
}
return descriptor.value;
}
`;

Expand All @@ -1058,13 +1062,19 @@ helpers.classPrivateFieldSet = helper("7.0.0-beta.0")`
throw new TypeError("attempted to set private field on non-instance");
}
var descriptor = privateMap.get(receiver);
if (!descriptor.writable) {
// This should only throw in strict mode, but class bodies are
// always strict and private fields can only be used inside
// class bodies.
throw new TypeError("attempted to set read only private field");
if (descriptor.set) {
descriptor.set.call(receiver, value);
} else {
if (!descriptor.writable) {
// This should only throw in strict mode, but class bodies are
// always strict and private fields can only be used inside
// class bodies.
throw new TypeError("attempted to set read only private field");
}

descriptor.value = value;
}
descriptor.value = value;

return value;
}
`;
Expand Down