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

Add private-property-in-object support #11372

Merged
Merged
@@ -1,9 +1,3 @@
{
"args": [
"src",
"--out-dir",
"lib",
"--copy-files",
"--verbose"
]
"args": ["src", "--out-dir", "lib", "--copy-files", "--verbose"]
}
@@ -1,9 +1,3 @@
{
"args": [
"src",
"--out-dir",
"lib",
"--copy-files",
"--verbose"
]
"args": ["src", "--out-dir", "lib", "--copy-files", "--verbose"]
}
@@ -1,3 +1,9 @@
{
"args": ["--out-file", "script2.js", "--no-comments", "--minified", "script.js"]
"args": [
"--out-file",
"script2.js",
"--no-comments",
"--minified",
"script.js"
]
}
51 changes: 44 additions & 7 deletions packages/babel-helper-create-class-features-plugin/src/features.js
Expand Up @@ -5,6 +5,7 @@ export const FEATURES = Object.freeze({
fields: 1 << 1,
privateMethods: 1 << 2,
decorators: 1 << 3,
privateIn: 1 << 4,
});

// We can't use a symbol because this needs to always be the same, even if
Expand All @@ -28,6 +29,39 @@ export function enableFeature(file, feature, loose) {
file.set(featuresKey, file.get(featuresKey) | feature);
if (loose) file.set(looseKey, file.get(looseKey) | feature);
}

if (
hasFeature(file, FEATURES.fields) &&
hasFeature(file, FEATURES.privateMethods) &&
isLoose(file, FEATURES.fields) !== isLoose(file, FEATURES.privateMethods)
) {
throw new Error(
"'loose' mode configuration must be the same for both @babel/plugin-proposal-class-properties " +
"and @babel/plugin-proposal-private-methods",
);
}

if (
hasFeature(file, FEATURES.fields) &&
hasFeature(file, FEATURES.privateIn) &&
isLoose(file, FEATURES.fields) !== isLoose(file, FEATURES.privateIn)
) {
throw new Error(
"'loose' mode configuration must be the same for both @babel/plugin-proposal-class-properties " +
"and @babel/plugin-proposal-private-property-in-object",
);
}

if (
hasFeature(file, FEATURES.privateMethods) &&
hasFeature(file, FEATURES.privateIn) &&
isLoose(file, FEATURES.privateMethods) !== isLoose(file, FEATURES.privateIn)
) {
throw new Error(
"'loose' mode configuration must be the same for both @babel/plugin-proposal-private-methods " +
"and @babel/plugin-proposal-private-property-in-object",
);
}
}

function hasFeature(file, feature) {
Expand Down Expand Up @@ -69,14 +103,17 @@ export function verifyUsedFeatures(path, file) {
}

if (
hasFeature(file, FEATURES.privateMethods) &&
hasFeature(file, FEATURES.fields) &&
isLoose(file, FEATURES.privateMethods) !== isLoose(file, FEATURES.fields)
path.isPrivateName() &&
path.parentPath.isBinaryExpression({
operator: "in",
left: path.node,
})
) {
throw path.buildCodeFrameError(
"'loose' mode configuration must be the same for both @babel/plugin-proposal-class-properties " +
"and @babel/plugin-proposal-private-methods",
);
if (!hasFeature(file, FEATURES.privateIn)) {
throw path.buildCodeFrameError(
"Private property in checks are not enabled.",
);
}
}

if (path.isProperty()) {
Expand Down
135 changes: 88 additions & 47 deletions packages/babel-helper-create-class-features-plugin/src/fields.js
Expand Up @@ -70,69 +70,104 @@ export function buildPrivateNamesNodes(privateNamesMap, loose, state) {
// Traverses the class scope, handling private name references. If an inner
// class redeclares the same private name, it will hand off traversal to the
// restricted visitor (which doesn't traverse the inner class's inner scope).
const privateNameVisitor = {
function privateNameVisitorFactory(visitor) {
const privateNameVisitor = {
...visitor,

Class(path) {
const { privateNamesMap } = this;
const body = path.get("body.body");

const visiblePrivateNames = new Map(privateNamesMap);
const redeclared = [];
for (const prop of body) {
if (!prop.isPrivate()) continue;
const { name } = prop.node.key.id;
visiblePrivateNames.delete(name);
redeclared.push(name);
}

// If the class doesn't redeclare any private fields, we can continue with
// our overall traversal.
if (!redeclared.length) {
return;
}

// This class redeclares some private field. We need to process the outer
// environment with access to all the outer privates, then we can process
// the inner environment with only the still-visible outer privates.
path.get("body").traverse(nestedVisitor, {
...this,
redeclared,
});
path.traverse(privateNameVisitor, {
...this,
privateNamesMap: visiblePrivateNames,
});

// We'll eventually hit this class node again with the overall Class
// Features visitor, which'll process the redeclared privates.
path.skipKey("body");
},
};

// Traverses the outer portion of a class, without touching the class's inner
// scope, for private names.
const nestedVisitor = traverse.visitors.merge([
{
...visitor,
},
environmentVisitor,
]);

return privateNameVisitor;
}

const privateNameVisitor = privateNameVisitorFactory({
PrivateName(path) {
const { privateNamesMap } = this;
const { privateNamesMap, redeclared } = this;
const { node, parentPath } = path;

if (!parentPath.isMemberExpression({ property: node })) return;
if (!privateNamesMap.has(node.id.name)) return;

const { name } = node.id;
if (!privateNamesMap.has(name)) return;
if (redeclared && redeclared.includes(name)) return;

this.handle(parentPath);
},
});

Class(path) {
const { privateNamesMap } = this;
const body = path.get("body.body");
const privateInVisitor = privateNameVisitorFactory({
BinaryExpression(path) {
const { operator, left, right } = path.node;
if (operator !== "in") return;
if (!path.get("left").isPrivateName()) return;

const visiblePrivateNames = new Map(privateNamesMap);
const redeclared = [];
for (const prop of body) {
if (!prop.isPrivate()) continue;
const { name } = prop.node.key.id;
visiblePrivateNames.delete(name);
redeclared.push(name);
}
const { loose, privateNamesMap, redeclared } = this;
const { name } = left.id;

if (!privateNamesMap.has(name)) return;
if (redeclared && redeclared.includes(name)) return;

// If the class doesn't redeclare any private fields, we can continue with
// our overall traversal.
if (!redeclared.length) {
if (loose) {
const { id } = privateNamesMap.get(name);
path.replaceWith(template.expression.ast`
Object.prototype.hasOwnProperty.call(${right}, ${id})
`);
return;
}

// This class redeclares some private field. We need to process the outer
// environment with access to all the outer privates, then we can process
// the inner environment with only the still-visible outer privates.
path.get("body").traverse(privateNameNestedVisitor, {
...this,
redeclared,
});
path.traverse(privateNameVisitor, {
...this,
privateNamesMap: visiblePrivateNames,
});
const { id, static: isStatic } = privateNamesMap.get(name);
jridgewell marked this conversation as resolved.
Show resolved Hide resolved

// We'll eventually hit this class node again with the overall Class
// Features visitor, which'll process the redeclared privates.
path.skipKey("body");
},
};
if (isStatic) {
path.replaceWith(template.expression.ast`${right} === ${this.classRef}`);
return;
}

// Traverses the outer portion of a class, without touching the class's inner
// scope, for private names.
const privateNameNestedVisitor = traverse.visitors.merge([
{
PrivateName(path) {
const { redeclared } = this;
const { name } = path.node.id;
if (redeclared.includes(name)) path.skip();
},
},
{
PrivateName: privateNameVisitor.PrivateName,
path.replaceWith(template.expression.ast`${id}.has(${right})`);
Copy link
Member

Choose a reason for hiding this comment

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

just curious; do these field WeakMaps have WeakMap.prototype.has copied onto them? or can i intercept babel private fields, even in spec mode, by replacing WeakMap.prototype.has?

Copy link
Member Author

Choose a reason for hiding this comment

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

We do not secure the weak
maps, so this will do a prototype reference. Note that even get and set calls go through the prototype, too. We can't ensure the order of our evaluation, so the WeakMap could have already been patched by the time we reach the class definition.

Copy link
Member

Choose a reason for hiding this comment

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

Of course - you can't protect against first-run code, but to be spec compliant you'd have to at least protect against later code. Object.defineProperties(id, Object.getOwnPropertyDescriptors(WeakMap.prototype)) at id creation would address that.

Copy link
Member

Choose a reason for hiding this comment

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

Currently, all of our output assumes that builtins haven't been modified.

Copy link
Member

Choose a reason for hiding this comment

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

oof, ok - it's totally fine to assume they haven't been modified at module evaluation time (you don't really have a choice) but it's not fine to assume they haven't been modified later. that should be fixed asap (but not in this PR, obviously)

},
environmentVisitor,
]);
});

const privateNameHandlerSpec = {
memoise(member, count) {
Expand Down Expand Up @@ -306,6 +341,12 @@ export function transformPrivateNamesUsage(
...privateNameHandlerSpec,
});
}
body.traverse(privateInVisitor, {
privateNamesMap,
classRef: ref,
file: state,
loose,
});
}

function buildPrivateFieldInitLoose(ref, prop, privateNamesMap) {
Expand Down
@@ -1,9 +1,7 @@
{
"presets": [
["typescript"]
],
"presets": [["typescript"]],
"plugins": [
["proposal-decorators", { "decoratorsBeforeExport": true }],
["proposal-class-properties"]
]
}
}
21 changes: 21 additions & 0 deletions packages/babel-parser/src/parser/expression.js
Expand Up @@ -27,6 +27,7 @@ import {
isReservedWord,
isStrictReservedWord,
isStrictBindReservedWord,
isIdentifierStart,
} from "../util/identifier";
import type { Pos, Position } from "../util/location";
import * as charCodes from "charcodes";
Expand Down Expand Up @@ -1138,6 +1139,26 @@ export default class ExpressionParser extends LValParser {
this.registerTopicReference();
return this.finishNode(node, "PipelinePrimaryTopicReference");
}

const nextCh = this.input.codePointAt(this.state.end);
if (isIdentifierStart(nextCh) || nextCh === charCodes.backslash) {
const start = this.state.start;
// $FlowIgnore It'll either parse a PrivateName or throw.
node = (this.parseMaybePrivateName(true): N.PrivateName);
if (this.match(tt._in)) {
this.expectPlugin("privateIn");
this.classScope.usePrivateName(node.id.name, node.start);
} else if (this.hasPlugin("privateIn")) {
this.raise(
this.state.start,
Errors.PrivateInExpectedIn,
node.id.name,
);
} else {
throw this.unexpected(start);
}
return node;
}
}
// fall through
default:
Expand Down
2 changes: 2 additions & 0 deletions packages/babel-parser/src/parser/location.js
Expand Up @@ -125,6 +125,8 @@ export const Errors = Object.freeze({
"Topic reference was used in a lexical context without topic binding",
PrimaryTopicRequiresSmartPipeline:
"Primary Topic Reference found but pipelineOperator not passed 'smart' for 'proposal' option.",
PrivateInExpectedIn:
"Private names are only allowed in property accesses (`obj.#%0`) or in `in` expressions (`#%0 in obj`)",
PrivateNameRedeclaration: "Duplicate private name #%0",
RecordExpressionBarIncorrectEndSyntaxType:
"Record expressions ending with '|}' are only allowed when the 'syntaxType' option of the 'recordAndTuple' plugin is set to 'bar'",
Expand Down
@@ -1,3 +1,3 @@
{
"throws": "Unexpected token, expected \";\" (1:12)"
}
}
@@ -1,5 +1,3 @@
{
"plugins": [
["pipelineOperator", { "proposal": "minimal" }]
]
"plugins": [["pipelineOperator", { "proposal": "minimal" }]]
}
@@ -0,0 +1,7 @@
class Point {
#x = 1;
#y = 2;
static isPoint(obj) {
return #x in obj && #y in obj;
}
}
@@ -0,0 +1,6 @@
{
"plugins": [
"classPrivateProperties"
],
"throws": "This experimental syntax requires enabling the parser plugin: 'privateIn' (5:14)"
}
@@ -1,4 +1,7 @@
{
"throws": "Unexpected token (3:3)",
"plugins": ["classProperties", "classPrivateMethods"]
}
"plugins": [
"classProperties",
"classPrivateMethods"
]
}
@@ -1,4 +1,6 @@
{
"throws": "Unexpected token (4:11)",
"plugins": ["classPrivateProperties"]
"plugins": [
"classPrivateProperties"
]
}
@@ -1,4 +1,11 @@
{
"plugins": [["pipelineOperator", { "proposal": "smart" }]],
"plugins": [
[
"pipelineOperator",
{
"proposal": "smart"
}
]
],
"throws": "Unexpected token (1:4)"
}
}
@@ -0,0 +1,6 @@
class Foo {
#x = 1;
test() {
#x + 1;
}
}