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

Allow compiling #foo in obj without compiling private fields #13172

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
Expand Up @@ -20,7 +20,7 @@ import {
isLoose,
} from "./features";

export { FEATURES, injectInitialization };
export { FEATURES, enableFeature, injectInitialization };

// Note: Versions are represented as an integer. e.g. 7.1.5 is represented
// as 70000100005. This method is easier than using a semver-parsing
Expand Down
Expand Up @@ -17,8 +17,10 @@
"babel-plugin"
],
"dependencies": {
"@babel/helper-compilation-targets": "workspace:^7.12.17",
"@babel/helper-create-class-features-plugin": "workspace:^7.13.0",
"@babel/helper-plugin-utils": "workspace:^7.13.0"
"@babel/helper-plugin-utils": "workspace:^7.13.0",
"@babel/plugin-syntax-private-property-in-object": "workspace:^7.13.0"
},
"peerDependencies": {
"@babel/core": "^7.0.0-0"
Expand Down
161 changes: 148 additions & 13 deletions packages/babel-plugin-proposal-private-property-in-object/src/index.js
@@ -1,23 +1,158 @@
/* eslint-disable @babel/development/plugin-name */

import { declare } from "@babel/helper-plugin-utils";
import syntaxPlugin from "@babel/plugin-syntax-private-property-in-object";
import {
createClassFeaturePlugin,
enableFeature,
FEATURES,
injectInitialization as injectConstructorInit,
} from "@babel/helper-create-class-features-plugin";

export default declare((api, options) => {
api.assertVersion(7);
export default declare(({ assertVersion, types: t, template }, { loose }) => {
assertVersion(7);

// NOTE: When using the class fields or private methods plugins,
// they will also take care of '#priv in obj' checks when visiting
// the ClassExpression or ClassDeclaration nodes.
// The visitor of this plugin is only effective when not compiling
// private fields and methods.

const classWeakSets = new WeakMap();
const fieldsWeakSets = new WeakMap();

function unshadow(name, targetScope, scope) {
while (scope !== targetScope) {
if (scope.hasOwnBinding(name)) scope.rename(name);
scope = scope.parent;
}
}

function injectToFieldInit(fieldPath, expr, before = false) {
if (fieldPath.node.value) {
if (before) {
fieldPath.get("value").insertBefore(expr);
} else {
fieldPath.get("value").insertAfter(expr);
}
} else {
fieldPath.set("value", t.unaryExpression("void", expr));
}
}

function injectInitialization(classPath, init) {
let firstFieldPath;
let consturctorPath;

for (const el of classPath.get("body.body")) {
if (
(el.isClassProperty() || el.isClassPrivateProperty()) &&
!el.node.static
) {
firstFieldPath = el;
break;
}
if (!consturctorPath && el.isClassMethod({ kind: "constructor" })) {
consturctorPath = el;
}
}

if (firstFieldPath) {
injectToFieldInit(firstFieldPath, init, true);
} else {
injectConstructorInit(classPath, consturctorPath, [
t.expressionStatement(init),
]);
}
}

function getWeakSetId(weakSets, outerClass, reference, name = "", inject) {
let id = classWeakSets.get(reference.node);

if (!id) {
id = outerClass.scope.generateUidIdentifier(`${name || ""} brandCheck`);
classWeakSets.set(reference.node, id);

inject(reference, template.expression.ast`${t.cloneNode(id)}.add(this)`);

outerClass.insertBefore(template.ast`var ${id} = new WeakSet()`);
}

return t.cloneNode(id);
}

return {
name: "proposal-private-property-in-object",
inherits: syntaxPlugin,
pre() {
// Enable this in @babel/helper-create-class-features-plugin, so that it
// can be handled by the private fields and methods transform.
enableFeature(this.file, FEATURES.privateIn, loose);
},
visitor: {
BinaryExpression(path) {
const { node } = path;
if (node.operator !== "in") return;
if (!t.isPrivateName(node.left)) return;

const { name } = node.left.id;

let privateElement;
const outerClass = path.findParent(path => {
if (!path.isClass()) return false;

privateElement = path
.get("body.body")
.find(({ node }) => t.isPrivate(node) && node.key.id.name === name);

return !!privateElement;
});

if (outerClass.parentPath.scope.path.isPattern()) {
outerClass.replaceWith(template.ast`(() => ${outerClass.node})()`);
// The injected class will be queued and eventually transformed when visited
return;
}

if (privateElement.isMethod()) {
if (privateElement.node.static) {
if (outerClass.node.id) {
unshadow(outerClass.node.id.name, outerClass.scope, path.scope);
} else {
outerClass.set("id", path.scope.generateUidIdentifier("class"));
}
path.replaceWith(
template.expression.ast`
${t.cloneNode(outerClass.node.id)} === ${path.node.right}
`,
);
} else {
const id = getWeakSetId(
classWeakSets,
outerClass,
outerClass,
outerClass.node.id?.name,
injectInitialization,
);

return createClassFeaturePlugin({
name: "proposal-class-properties",
path.replaceWith(
template.expression.ast`${id}.has(${path.node.right})`,
);
}
} else {
// Private fields might not all be initialized: see the 'halfConstructed'
// example at https://v8.dev/features/private-brand-checks.

api,
feature: FEATURES.privateIn,
loose: options.loose,
const id = getWeakSetId(
fieldsWeakSets,
outerClass,
privateElement,
privateElement.node.key.id.name,
injectToFieldInit,
);

manipulateOptions(opts, parserOpts) {
parserOpts.plugins.push("privateIn");
path.replaceWith(
template.expression.ast`${id}.has(${path.node.right})`,
);
}
},
},
});
};
});
@@ -0,0 +1,7 @@
class Foo {
get #foo() {}

test(other) {
return #foo in other;
}
}
@@ -0,0 +1,14 @@
var _FooBrandCheck = new WeakSet();
Copy link
Contributor

@JLHwung JLHwung Apr 19, 2021

Choose a reason for hiding this comment

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

We can also place FooBrandCheck to a private static field, e.g.

class Foo {
  private static #_FooBrandCheck = new WeakSet();
  constructor() {
    Foo.#_FooBrandCheck.add(this);
  }

  get #foo() {}

  test(other) {
    return Foo.#_FooBrandCheck.has(other);
  }
}

so we avoid the edge cases when FooBrandCheck might be injected to an incorrect scope, e.g. when a class is in the param initializer.

// <-- FooBrandCheck will be injected to here
(x = class { #foo; test (other) { return #foo in other } }) => {}

Copy link
Member Author

Choose a reason for hiding this comment

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

Ohh I love this idea

Copy link
Contributor

Choose a reason for hiding this comment

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

I wish class access were not stage-2. The class binding can be overwritten. Guess we may have to stick to temporary variable.


class Foo {
constructor() {
_FooBrandCheck.add(this);
}

get #foo() {}

test(other) {
return _FooBrandCheck.has(other);
Copy link
Contributor

Choose a reason for hiding this comment

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

Q: Is _FooBrandCheck.has(other) equivalent to other instanceof Foo?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, it's different when other is a subclass of Foo

Copy link
Contributor

Choose a reason for hiding this comment

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

If Bar, a subclass of Foo, has super() in its constructor, does it imply #foo in bar is true where bar is an instance of Bar?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh I was confusing this test with the private fields one. instanceof is different because it checks the prototype:

class Base {
  constructor() {
    return {};
  }
}

class Foo extends Base {
  #foo;

  static check(obj) {
    return #foo in obj;
  }
}

new Foo() instanceof Foo; // false
Foo.check(new Foo); // true
class Foo {
  #foo;

  static check(obj) {
    return #foo in obj;
  }
}

let foo = new Foo();
foo.__proto__ = {};
foo instanceof Foo; // false
Foo.check(foo); // true
class Foo {
  #foo;

  static check(obj) {
    return #foo in obj;
  }
}

let foo = Object.create(Foo.prototype);
foo instanceof Foo; // true
Foo.check(foo); // false

If Bar, a subclass of Foo, has super() in its constructor, does it imply #foo in bar is true where bar is an instance of Bar?

If by "bar is an instance of Bar" you mean bar instanceof Bar then no, there isn't an implication in any direction.

}

}
@@ -0,0 +1,4 @@
(x = class {
#foo;
test(other) { return #foo in other }
}) => {}
@@ -0,0 +1,12 @@
(x = (() => {
var _fooBrandCheck;

return _fooBrandCheck = new WeakSet(), class {
#foo = void _fooBrandCheck.add(this);

test(other) {
return _fooBrandCheck.has(other);
}

};
})()) => {};
@@ -0,0 +1,9 @@
function fn() {
return new class {
#priv;

method(obj) {
return #priv in obj;
}
}
}
@@ -0,0 +1,12 @@
function fn() {
var _privBrandCheck;

return new (_privBrandCheck = new WeakSet(), class {
#priv = void _privBrandCheck.add(this);

method(obj) {
return _privBrandCheck.has(obj);
}

})();
}
@@ -0,0 +1,9 @@
function fn() {
return new class {
static #priv;

method(obj) {
return #priv in obj;
}
}
}
@@ -0,0 +1,12 @@
function fn() {
var _privBrandCheck;

return new (_privBrandCheck = new WeakSet(), class {
static #priv = void _privBrandCheck.add(this);

method(obj) {
return _privBrandCheck.has(obj);
}

})();
}
@@ -0,0 +1,7 @@
class Foo {
#foo = 1;

test(other) {
return #foo in other;
}
}
@@ -0,0 +1,12 @@
var _temp;

var _fooBrandCheck = new WeakSet();

class Foo {
#foo = (_temp = 1, _fooBrandCheck.add(this), _temp);

test(other) {
return _fooBrandCheck.has(other);
}

}
@@ -0,0 +1,28 @@
let hasW, hasX, hasY, hasZ;
let halfConstructed;

class F {
m() {
hasW = #w in this;
hasX = #x in this;
hasY = #y in this;
hasZ = #z in this;
}
get #w() {}
#x = 0;
#y = (() => {
halfConstructed = this;
throw "error";
})();
#z() {}
}

try {
new F();
} catch {}
halfConstructed.m();

expect(hasW).toBe(true);
expect(hasX).toBe(true);
expect(hasY).toBe(false);
expect(hasZ).toBe(true);
@@ -0,0 +1,14 @@
class F {
m() {
#w in this;
#x in this;
#y in this;
#z in this;
}
get #w() {}
#x = 0;
#y = (() => {
throw 'error';
})();
#z() {}
}
@@ -0,0 +1,3 @@
{
"minNodeVersion": "14.0.0"
}
@@ -0,0 +1,29 @@
var _temp, _temp2;

var _FBrandCheck = new WeakSet();

var _xBrandCheck = new WeakSet();

var _yBrandCheck = new WeakSet();

class F {
m() {
_FBrandCheck.has(this);

_xBrandCheck.has(this);

_yBrandCheck.has(this);

_FBrandCheck.has(this);
}

get #w() {}

#x = (_temp = (_FBrandCheck.add(this), 0), _xBrandCheck.add(this), _temp);
#y = (_temp2 = (() => {
throw 'error';
})(), _yBrandCheck.add(this), _temp2);

#z() {}

}