Skip to content
This repository was archived by the owner on Jan 25, 2022. It is now read-only.

Another approach to brand-check #230

Closed
Igmat opened this issue Mar 23, 2019 · 24 comments
Closed

Another approach to brand-check #230

Igmat opened this issue Mar 23, 2019 · 24 comments

Comments

@Igmat
Copy link

Igmat commented Mar 23, 2019

Right now this code:

class A {
  #priv = 1;
  
  method() {
    console.log(this.#priv);
  }
}

desugars to this:

"use strict";

function _instanceof(left, right) { if (right != null && typeof Symbol !== "undefined" && right[Symbol.hasInstance]) { return right[Symbol.hasInstance](left); } else { return left instanceof right; } }

function _classCallCheck(instance, Constructor) { if (!_instanceof(instance, Constructor)) { throw new TypeError("Cannot call a class as a function"); } }

function _defineProperties(target, props) { for (var i = 0; i < props.length; i++) { var descriptor = props[i]; descriptor.enumerable = descriptor.enumerable || false; descriptor.configurable = true; if ("value" in descriptor) descriptor.writable = true; Object.defineProperty(target, descriptor.key, descriptor); } }

function _createClass(Constructor, protoProps, staticProps) { if (protoProps) _defineProperties(Constructor.prototype, protoProps); if (staticProps) _defineProperties(Constructor, staticProps); return Constructor; }

function _classPrivateFieldGet(receiver, privateMap) { if (!privateMap.has(receiver)) { throw new TypeError("attempted to get private field on non-instance"); } var descriptor = privateMap.get(receiver); if (descriptor.get) { return descriptor.get.call(receiver); } return descriptor.value; }

var A =
/*#__PURE__*/
function () {
  function A() {
    _classCallCheck(this, A);

    _priv.set(this, {
      writable: true,
      value: 1
    });
  }

  _createClass(A, [{
    key: "method",
    value: function method() {
      console.log(_classPrivateFieldGet(this, _priv));
    }
  }]);

  return A;
}();

var _priv = new WeakMap();

As we know, it makes wrapping proxies to fail (you can check it in #227 (comment)).

The reason why it fails is brand-check semantic that is implemented here:

function _classPrivateFieldGet(receiver, privateMap) {
  if (!privateMap.has(receiver)) {
    throw new TypeError("attempted to get private field on non-instance"); 
  }
  var descriptor = privateMap.get(receiver);
  if (descriptor.get) {
    return descriptor.get.call(receiver); 
  }
  return descriptor.value; 
}

The idea of such brand-check is that it guarantees that constructor was executed, which means that all privates were installed to instance.
I want to propose change to this brand-check, which will guarantee that constructor was executed, but won't dissmiss wrapping proxies.

Consider following desugarred code:

function _classPrivateFieldGet(receiver, prop, brand) {
  if (!Object.hasOwnProperty.apply(receiver, brand)) {
    throw new TypeError("attempted to get private field on non-instance"); 
  }
  return receiver[prop];
}

var A =
/*#__PURE__*/
function () {
  var _brand = Symbol.private();
  var _priv = Symbol.private();
  function A() {
    _classCallCheck(this, A);

    _defineProperty(this, _brand, true);
    _defineProperty(this, _priv, 1);
  }

  _createClass(A, [{
    key: "method",
    value: function method() {
      console.log(_classPrivateFieldGet(this, _priv, _brand));
    }
  }]);

  return A;
}();

It uses Symbol.private (as in this proposal, which could be implemented by simple pollyfill (like here)) instead of WeakMap, but efectively solves problem committee has mentioned by providing proxy-safe brand-check mechanism. Does it make any sense for you?

UPD.: code samples are updated to cover the case mentioned by @nicolo-ribaudo in #230 (comment)

@nicolo-ribaudo
Copy link
Member

With that desugaring, the brand check can be easily worked around:

class A {
  #foo;
  static assertA(obj) {
    obj.#foo;
  }
}

var notAnA = {};
A.assertA(notAnA); // throws

hack(notAnA, A, notAnA => {
  // here brand checks are spoiled
  A.assertA(notAnA); // doesn't throw
});

function hack(obj, Class, cb) {
  var proto = obj.__proto__;
  obj.__proto__ = new Class;
  cb(obj); // obj has the brand symbol in his prototype now
  obj.__proto__ = proto;
}

@Igmat
Copy link
Author

Igmat commented Mar 23, 2019

function _classPrivateFieldGet(receiver, prop, brand) {
  if (!Object.hasOwnProperty.apply(receiver, brand) || !receiver[brand]) {
    throw new TypeError("attempted to get private field on non-instance"); 
  }
  return receiver[prop];
}

It'll solve your case.

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Mar 23, 2019

Did you mean this? If so, I agree that it would solve the proxy issue but I don't know exactly what branding guarantees the committee want.

function _classPrivateFieldGet(receiver, prop, brand) {
  if (!Object.hasOwnProperty.apply(receiver, brand)) {
    throw new TypeError("attempted to get private field on non-instance"); 
  }
  return receiver[prop];
}

Btw, from a spec point pow this can be implemented without private symbols. The spec for PrivateFieldFind would look like this:

PrivateFieldFind (P, O)

1. Assert: P is a Private Name.
2. Assert: O is an object with a [[PrivateFieldValues]] internal slot.
3. For each element entry in O.[[PrivateFieldValues]],
    a. If entry.[[PrivateName]] is P, return entry.
4. If O has a [[ProxyTarget]] internal slot,
    a. Return PrivateFieldFind(O.[[ProxyTarget]])
5. Return empty.

@trusktr
Copy link

trusktr commented Mar 24, 2019

receiver[brand]
receiver[prop]

What about going back to WeakMap for prop, and using WeakMap for brand too? EDIT: I suppose it doesn't matter much, as long as it detectable by user code.

@trusktr
Copy link

trusktr commented Mar 24, 2019

What about mixins? Seems the brand needs to be hoisted to module scope, so that we can do the following (without needing a language-level mixin feature):

let count = 0

const AMixin = (Base = Object) => {
    return class A extends Base {
        #priv = ++count
        
        getOther(other) {
            retur other.#priv
        }
    }
}

const Foo = AMixin(class {})
const Bar = AMixin(class {})

const foo = new Foo
const bar = new Foo

// intuitively:
console.assert(foo.getOther(bar) === 2)

Otherwise each application of the mixin will generate a class with a different brand, although obviously in the example it is clear that the user needs the branding across instances of the same definition in source of the class, right?

Am I understanding branding correctly, or is my mixin question really about something else?

@nicolo-ribaudo
Copy link
Member

@trusktr That is unrelated to this issue, and it has been discussed at #60.

@trusktr
Copy link

trusktr commented Mar 25, 2019

@nicolo-ribaudo Thanks. Where's the best resource to learn what exactly "branding" means?

@littledan
Copy link
Member

I think the "branding" discussion is a bit of a distraction, and the WeakMap semantics are independently motivated. See these slides for an explanation.

We've discussed "branding" alternatives in this repository as well as in TC39 at length. Closing this issue, as I don't think there's much new to cover here.

@trusktr
Copy link

trusktr commented Mar 25, 2019

@littledan Thank for that 👍. Not sure where to ask this, but the slides say that

class H {
  #x;
  constructor(x) {
    this.#x = x;
  }
  get x() { return this.#x; }
}

const obj = new H();
const proxy = new Proxy(obj, {});
proxy.x

is a TypeError. Doesn't that violate hard privacy? Shouldn't proxy.x return undefined if we want hard privacy?

Why did we decide semantics should closely follow internal slot behavior? IMO it is a better goal to make a language feature that community likes, and not necessarily emulate some feature most people currently don't understand (let alone care about). What I mean is, if private fields don't behave like those Date examples... so what??? I'd rather have an awesome feature, for my classes, which isn't constrained by the goal of emulating an existing other feature of some builtin class.

@Igmat
Copy link
Author

Igmat commented Mar 25, 2019

@littledan I've already seen those slides - and created this issue having your arguments in mind.

I'll quote design goals you mentioned there, with my comments regarding applying of this brand-check proposal:

  1. Class controls who sees private (👍 refactoring private) TRUE
  2. Analogous to public (👍 refactoring public ⇔ private) TRUE
  3. Analogous to internal slots (👍 polyfilling built-ins) HALF-TRUE, Proxy interaction is an exception
  4. Keep the mental model simple (👍 learning) TRUE
  5. Class controls how private operates (👍 predictable) TRUE
  6. Optimizable as much as public (👍 fast) TRUE

This proposal aims to solve Proxy issue by making 2 changes:

  1. Tunnel privates to target
  2. Change brand-check as shown above.

The only guarantee provided by current brand-check mechanism is that instance is built via specific constructor chain. My proposal guarantees exactly the same, but also solves Proxy issue, so I don't understand why do you dismiss it?

Do you have any specific cases which isn't covered by this proposal?

P.S.

Symbol.private was mentioned only because it's easier to show brand-check semantic with it, but it's achievable without it.

P.P.S.

Could you, please, re-open this issue, since IMO it worth discussion.

@rdking
Copy link

rdking commented Mar 25, 2019

@littledan @Igmat Here's a pure question:

Why must a private implementation be analogous to internal slots when internal slots have undesirable features? Is the analogy only in as far as they are essentially object instance properties hidden from public code?

@ljharb
Copy link
Member

ljharb commented Mar 26, 2019

What makes them undesirable? Since they’re how the builtins behave, one argument might be that things that behave differently are bucking convention.

@rdking
Copy link

rdking commented Mar 26, 2019

By that logic, using internal slots as an implementation of class-instance private data is bucking convention for class-instance private data. That's undesirable. In the best of all cases, private members are no different than public members except that they cannot be accessed by anything not a part of the class definition. Anything beyond that is undesirable.

How internal slots work in ES is perfectly fine for internal slots. But for private fields, internal slots have "features" that get in the way of several use cases.

@trusktr
Copy link

trusktr commented Mar 29, 2019

@ljharb Do you think about internal slots while you code in JavaScript? You don't. No one does, unless in rare cases like if you're an engine author.

Adding a feature just to emulate internal slots is only a limitation being imposed for no good reason other than "it works the same". Providing people with features based on how they use JavaScript and what they want rather than emulating the engine is a better path to take.

Most people don't care about the engine implementation (except for knowing what triggers performance pitfalls), they just want to write code, and if you can let them do that with less limitation, then that would be better.

99.9999% (or maybe 100%) of people don't think about internal slots when they are writing JavaScript.

In the best of all cases, private members are no different than public members except that they cannot be accessed by anything not a part of the class definition.

Very well said!

@ljharb
Copy link
Member

ljharb commented Mar 29, 2019

Yes, I do, constantly; that's why i use Array.isArray to detect arrays and don't do a ducktype check for .map or whatever method I plan to use. I would argue that many more people than you think DO consider the "core essence" of something - its brand, ie, "what it actually is" and not just "what it does" or "what things it has". That's also what typeof offers (since it partially operates based on internal slots).

Your claim is overly strong; I'd say anyone using typeof is thinking about internal slots whether they realize it or not.

@rdking
Copy link

rdking commented Mar 29, 2019

@ljharb I'm going to try to re-word what @trusktr was trying to say.

Do developers think about internal slots when coding? Usually not. Why? They don't care how the features are implemented. They only care about the observable semantics, i.e. how they can get the language to do what they want. That doesn't imply that they aren't thinking about features that use internal slots, but rather that the feature could be implemented in some other way and the developer won't care, so long as the result is the same.

Using Array.isArray as an example: developers know that if they verify their arrays using Array.isArray, then all of the methods of Array.prototype will work without throwing an inexplicable error. It will also confirm an array sourced from a different realm. It doesn't matter to most developers that the reason some of the Array methods throw is because of a lack of internal slots on non-arrays.

@ljharb
Copy link
Member

ljharb commented Mar 29, 2019

They don’t actually know that - both because isArray doesn’t tell them that it inherits from Array.prototype; and because none (probably one, but off the top of my head i can’t recall any) of the array methods throw such an error. isArray tells you it’s an exotic object with a magic length property (ie, it’s a brand check alone)

I’m certainly sure most people don’t know what an internal slot is nor know their semantics, but that doesn’t mean they’re not thinking about the concept.

@rdking
Copy link

rdking commented Mar 30, 2019

@ljharb

isArray doesn’t tell them that it inherits from Array.prototype

Red herring. When I said:

developers know that if they verify their arrays using Array.isArray, then all of the methods of Array.prototype will work...

I meant that any call of the Array method with the verified object as the context will work. It doesn't matter if the object inherits from Array.prototype.

none (probably one, but off the top of my head i can’t recall any) of the array methods throw such an error.

There is at least one. I've run into it before, but I'd have to do a little research to remind myself which ones. So my point stand as written. That's what the developer knows about Array.isArray. If I remember correctly, it was written because cross-realm arrays would fail when used with certain Array.prototype methods.

I’m certainly sure most people don’t know what an internal slot is nor know their semantics, but that doesn’t mean they’re not thinking about the concept.

While potentially true, it's laughably irrelevant. Its only developers like us that bother to fuss around thinking about the inner workings of the engine. Compared to the entire ES community, I'd wager that less than 1% ever bothers to think about the concept of internal slots. And although they've existed for quite some time, before these private-fields discussions, I'd wager that even less ever knew the concept existed.

@trusktr
Copy link

trusktr commented Apr 6, 2019

@rdking Thanks for perfectly re-wording what I was saying.

@ljharb That reasoning doesn't make sense. Just because I use JavaScript doesn't mean I automatically think about internal slots.

So, just because someone uses a computer means they think about transistors? That makes no sense.


A private class member feature does not need to be implemented around internal slot semantics. It needs to be implemented around what the user of the language will experience.

@ljharb
Copy link
Member

ljharb commented Apr 6, 2019

@trusktr again, i'm not saying people necessarily have the word "internal slot" in their head, or know what it is. But I am saying that devs have an innate sense that certain objects have an identity - something not directly observable that makes them what they are. For example, that an array isn't just something that inherits from Array.prototype, a regex isn't just something with a test method and a pattern property, etc.

@trusktr
Copy link

trusktr commented Apr 6, 2019

I am saying that devs have an innate sense that certain objects have an identity

Yes, but even that doesn't apply in most cases.

And with regards to a private-member feature, what you're talking has nothing to do with it. I mean, identifying an object is one thing, but what someone can do with private members is another thing.

I still don't understand how "brand check" applies to the average developer's concerns, let alone if I even understand it.

What I do know is that features in this spec don't work as I'd like them to when making my own classes, of which I know the identities of (and I've never had to deal with cross-realm issues, honestly, because in all those cases I've mostly used message passing in which deserialization yields correct-realm objects, etc).

@rdking
Copy link

rdking commented Apr 6, 2019

@ljharb

I am saying that devs have an innate sense that certain objects have an identity - something not directly observable that makes them what they are.

Based on my memory of when I first was learning JS, devs have knowledge (not sense) that certain object are native entities for the language and not something created in the language. They recognize that these things have capabilities and properties not exposed to the language. And that's why they're not reproducible within the language except by the provided means. The concept of "identity" is only loosely relevant to that understanding. Nowhere in that does the concept of "internal slots" have a place.

This makes all of what you posted a red herring argument. The truth is that if ES once again provided the capability of uniquely identifying the function that is running at position N in the call stack, then secure, fully encapsulated, hard private object members could easily be implemented in ES code. No internal slots or per-instance WeakMaps required. The only issue to follow that would be providing an ergonomic syntax to reduce the complexity of use.

@trusktr
Copy link

trusktr commented Apr 6, 2019

Source position can also give the identity of anything in the engine, cross-realm or not. What's the reason we aren't using call stacks to determine ability to access private members, if the engine already has that structure in place?

if ES once again provided the capability of uniquely identifying the function that is running at position N in the call stack

The engine doesn't have to expose that in JS land, but can still use it to validate private member access. (Note, even in non-strict mode, the JS utilities for that have flaws that prevent private member access from being blocked in all necessary cases, but these problems don't exist inside the JS engine)

@trusktr
Copy link

trusktr commented Apr 6, 2019

@ljharb By the way, here's a poll, "When you code in JavasScript, do you think about internal slots?":

https://twitter.com/trusktr/status/1114419022366269442

I should've configured the poll to last for a week, but I accidentally left it at 24 hours.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants