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

Dynamic Access of Private FIelds #104

Open
robbiespeed opened this issue Aug 19, 2017 · 16 comments
Open

Dynamic Access of Private FIelds #104

robbiespeed opened this issue Aug 19, 2017 · 16 comments

Comments

@robbiespeed
Copy link

robbiespeed commented Aug 19, 2017

It's stated in the FAQ that this.#x !== this['#x'] this makes sense in the same way that this.x.y !== this['x.y'], however that means we currently have no way of dynamic access.

That answer also gives the argument that dynamic access "is contrary to the notion of 'private'". This is not true at all, of course private fields shouldn't be dynamically accessed outside of the class body, but inside there are many reasons it can be valuable.

Not Possible Without Dynamic Access

I want to be clear here, I'm not advocating that private fields should be settable or retrievable from outside the class body, unless the library maintainer explicitly adds such feature.

  • Observer pattern
    If I library were to have set(obj, key, value) and observe(obj, key, callback) methods there would be no way to internally observe or set private fields, this would necessitate anyone using such library to have all fields public.

  • Intentional Escape Hatch For Debug and Tests

Possible Solutions

I would like to open this up to discussion, I don't quite know myself the best way to make this possible, or all the concerns people have about introducing such a feature.

Just to get things started though one possibility might be a new Private Key type. A example might look something like:

class Cat {
  #hunger = 0;
  constructor () {
    if (ENV.DEBUG_PRIVATE) {
      this.__getPrivate = (privateKey) => this.#[privateKey];
      // Set uses the observable `set` method
      this.__setPrivate = (privateKey, value) => set(this, #[privateKey], value);
      
      // If not using a observer library
      // this.__setPrivate = (privateKey, value) => this.#[privateKey] = value;
    }
    observe(this, #['hunger'], () => {
      if (this.#hunger > 10) {
        this.growl();
      }
    })
    setInterval(() => {
      set(this, #['hunger'], this.#hunger + 1);
    }, 1000);
  }
  growl () {
    console.log('Meeooow');
  }
  eat (food) {
    set(this, #['hunger'], this.#hunger - food.value);
  }
}

const cat = new Cat();
// Private fields still directly innaccessible from outside class body
cat.#['hunger']; // throw error or return undefined
cat.#['hunger'] = 2; // throw error
set(cat, #['hunger'], 2); // throw error

cat.__getPrivate('hunger'); // return 0
cat.__setPrivate('hunger', 10); // return 10
// log > Meeooow
cat.eat({ name: 'fish', value: 3 });
cat.__getPrivate('hunger'); // return 7

Open to better ideas for the syntax

@robbiespeed
Copy link
Author

@wycats because the observer pattern is critical to ember, I'm wondering if you had any thoughts on this.

@ljharb
Copy link
Member

ljharb commented Aug 19, 2017

If you want dynamic access, then store a Map in a single private field, and you can use anything you like as the key.

I don't see why enabling direct dynamic access of private fields is desired or cleaner, given that this is an easy option

@bakkot
Copy link
Contributor

bakkot commented Aug 20, 2017

You may be interested in the decorators proposal, which has an integration with this proposal here. I think it would let you accomplish approximately what you've laid out here with a single class decorator, and I think is a better route for this sort of metaprogramming. (It also includes a PrivateName type.)

@robbiespeed
Copy link
Author

@bakkot
I've been looking forward to decorators moving ahead to stage 3. I have a couple issues with the example of observing field changes the way described in unified class features though.

  • it adds the extra burden of explicitly describing observable fields
  • slows property access by requiring the definition of a getter

The second issue is the biggest in my eyes since property access is everywhere, and last time I did the perf test using getters and setters added a significant penalty. Getters and setters have their place, just not when it comes to property observation IMO.

@ljharb

If you want dynamic access, then store a Map in a single private field, and you can use anything you like as the key.

That is a decent workaround, for the case of observable fields, would not work for debugging private fields though.

I don't see why enabling direct dynamic access of private fields is desired or cleaner, given that this is an easy option

It's not necessarily always easy, sure from the ground it is. If your modifying existing code that uses private fields into a single map, that's going to be a real pain.

It's desired because it allows more flexible development, and from where I can see doesn't present any cons that weren't already there.

@rdking
Copy link

rdking commented Aug 21, 2017

@ljharb

I don't see why enabling direct dynamic access of private fields is desired or cleaner...

The same argument could be made for adding private fields since there are other solutions (like using WeakMaps) that are no different than your suggestion of using a Map. Just about any argument you could make against using a WeakMap to store private data would also have an analog for using a Map to make for dynamic access of private fields.

@bakkot
One of the main advantages of dynamic field access is still lost with decorators where private fields are concerned. The ability to iterate through the list of private fields in an object still can't be done from a decorator method since private names are not valid in member accessing [].

@ljharb
Copy link
Member

ljharb commented Aug 21, 2017

There are indeed reasons why WeakMaps are insufficient for private fields; none of them apply to what this issue is asking for.

@bmeck
Copy link
Member

bmeck commented Aug 21, 2017

The desire for hard privacy is very much real and mandatory to prevent various things like those that come up somewhat often with people potentially misusing/relying on internal APIs in Node (an example from just a few days ago nodejs/CTC#164 ). I have no desire for soft private state when Symbols already fulfill that use case.

@robbiespeed
Copy link
Author

@bmeck dynamic access does not equate to soft privacy.

Private fields (soft or hard) will also not remove the necessity of having internal api's that are actually public, with the expectation that they are for internal use only. It is already possible in some scenarios to use something like rollup to remove access to internal apis.

@bmeck
Copy link
Member

bmeck commented Aug 21, 2017

Maybe I am not understanding then:

I want to be clear here, I'm not advocating that private fields should be settable or retrievable from outside the class body, unless the library maintainer explicitly adds such feature.

Observer pattern
If I library were to have set(obj, key, value) and observe(obj, key, callback) methods there would be no way to internally observe or set private fields, this would necessitate anyone using such library to have all fields public.

Intentional Escape Hatch For Debug and Tests

Appeared to be stating the intent here is to allow soft privacy by exposing the private fields for the 2 use cases mentioned. Is that incorrect?

I am not concerned as much about exposing things via public get/set mechanisms as that is possible today by creating stuff similar to your example above.

@robbiespeed
Copy link
Author

robbiespeed commented Aug 21, 2017

Appeared to be stating the intent here is to allow soft privacy by exposing the private fields for the 2 use cases mentioned. Is that incorrect?

Only the second is intended to allow soft privacy, as opt in, and primarily for library maintainers with the expectation that such if statement would be striped from production libraries.

@bmeck
Copy link
Member

bmeck commented Aug 21, 2017

@robbiespeed One option available today is to use helpers similar to what you had above without needing to reify a PrivateName type which can be hard to lock down (see tc39/proposal-decorators#5):

class Cat {
  #hunger = 0;
  constructor () {
    if (ENV.DEBUG_PRIVATE) {
      // Get uses the observable `get` method to virtualize storage
      this.__getPrivate = (privateKey) => {
         // last argument specifies location
         if (privateKey === 'hunger') return get(this, 'hunger', 'private');
      };
      // if not using an observer library
      this.__getPrivate = (privateKey) => {
         if (privateKey === 'hunger') return this.#hunger;
      };
      // Set uses the observable `set` method
      this.__setPrivate = (privateKey, value) => {
         // last argument specifies location
         if (privateKey === 'hunger') return set(this, 'hunger', value, 'private');
      };
      
      // If not using a observer library
      this.__setPrivate = (privateKey, value) => {
         // last argument specifies location
         if (privateKey === 'hunger') return this.#hunger = value;
      };
    }
    // last argument specifies location
    observe(this, 'hunger', () => {
      if (this.__getPrivate('hunger') > 10) {
        this.growl();
      }
    }, 'private')
    setInterval(() => {
      this.__setPrivate('hunger', this.__getPrivate('hunger') + 1);
    }, 1000);
  }
  growl () {
    console.log('Meeooow');
  }
  eat (food) {
    this.__setPrivate('hunger', this.__getPrivate('hunger') - food.value);
  }
}

const cat = new Cat();
// Private fields still directly innaccessible from outside class body
cat.#hunger; // throw error
cat.#hunger = 2; // throw error

cat.__getPrivate('hunger'); // return 0
cat.__setPrivate('hunger', 10); // return 10
// log > Meeooow
cat.eat({ name: 'fish', value: 3 });
cat.__getPrivate('hunger'); // return 7

Edit: everything should be using __ methods if observable lib wants to be able to hook into it properly. Still has a problem of not being able to observe the initializer.

@bakkot
Copy link
Contributor

bakkot commented Aug 21, 2017

@robbiespeed, I wouldn't generalize from performance characteristics of regular properties to performance characteristics of private fields. Among other things, private fields are a lot more static. Also, I'm pretty sure a class-level decorator would allow you to decorate every field without needing to explicitly mark which ones were observed (though I cannot imagine finding this to be good programming practice).

Anyway, if it's just for debugging, you might as well just use eval, or any number of slightly more verbose patterns. I don't think it's worth adding complexity to the language to support that particular use case.

class Foo {
  #a; #b;
  constructor() {
    if (debug) this.debug = name => eval(`this.#${name}`);
  }
}

@robbiespeed
Copy link
Author

@bmeck
I'm not sure I understand your suggestion, and perhaps this is on me for not explaining the example more thoroughly, but it was the intention that ENV.DEBUG_PRIVATE === false should not break the public api of the Cat, in your example everything breaks unless ENV.DEBUG_PRIVATE === true. Also increases complexity immensely as number of private fields grow.

I think the issue you linked (in particular this comment), has given me a better understanding of your concerns. I can't see anything specific about dynamic private fields that would cause leakage of node internals, unless a leak was explicitly defined.

@bakkot
Yes I was speaking there about performance of regular properties (can't test private fields). The argument will still likely hold true for private fields though, as defining a getter / setter means issuing a function call that then does a lookup vs. just doing a lookup.

I think you are right a class level decorator could work for libraries that need to declare fields as observable.

I honestly had not thought of eval as an option. My initial reaction was that eval shouldn't be able to access private fields, but I guess it is "safe" since any passed reference of eval would instead run in global scope. While it works for access, I can't see of a way to make it work for setting private fields.

@bakkot
Copy link
Contributor

bakkot commented Aug 22, 2017

@robbiespeed

as defining a getter / setter means issuing a function call that then does a lookup vs. just doing a lookup

OK, I'm confused. I thought the whole thing you were asking for was to be able to do a function call when writing to or reading from a field?

My initial reaction was that eval shouldn't be able to access private fields

Mine too, but we talked about this a while ago. Committee consensus was that eval should work, which after the discussion I agreed with.

While it works for access, I can't see of a way to make it work for setting private fields.

What's wrong with

class Foo {
  #a; #b;
  constructor() {
    if (debug) this.debugSet = (name, val) => eval(`this.#${name} = val`);
  }
}

?

@robbiespeed
Copy link
Author

@bakkot

What's wrong with

My apologies, nothing wrong with that.

OK, I'm confused. I thought the whole thing you were asking for was to be able to do a function call when writing to or reading from a field?

Writing to yes, reading from no. I don't want added access time, but added write time is fine, hence the use of the set method.
Really what I'd like to see is some sort of PrivateKey type that can be created only within a class, but can be passed around internally and externally. In order to run operations on private fields of an instance, but only when you have a reference to one of the PrivateKeys. There are likely more use cases, the escape hatch and observers where just the only 2 I could think of. People would of course have to be very careful about where they pass a PrivateKey, but it could prove useful.

@littledan
Copy link
Member

I see a number of solutions presented to the filer's problem in this thread; it seems like there's not an extremely strong case for adding another feature to the proposal.

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