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

Continuation of proposal-class-fields/issues/106 #1

Open
rdking opened this issue Apr 23, 2019 · 17 comments
Open

Continuation of proposal-class-fields/issues/106 #1

rdking opened this issue Apr 23, 2019 · 17 comments

Comments

@rdking
Copy link
Owner

rdking commented Apr 23, 2019

Here, instead of simply bantering about what we need, I'm working towards a proposal to submit, but I need more information from the community. The current goal of this repo is to provide a solution to the issues of using Proxy with respect to hidden data implementations, both within the engine and in user code.

The current discussion surrounds the idea offered by @isiahmeadows and modified by me that adds a resolve(target, receiver, isPrivate, method) method to the Proxy handler.

  • target - (Proxy instance).[[Target]]
  • receiver - Proxy instance
  • isPrivate - true if no handler method could ever be triggered by this access (presumes handler overrides all invariants)
  • method - name of handler method to trigger if a Proxy is returned
  • returns the object to use as the actual receiver of the access attempt

For more history on this conversation...


@mhofman wrote:

Most other implementations of hidden data rely on identity and/or closures Proxy can't see closures even enough to crash because of them. No issue there. For identity based methods, WeakMap's inability to unwrap a Proxy can be remedied by having WeakMap trigger this resolve function, using the result as the key instead of the original object. Hidden data approaches outside of those 2 types won't be affected one way or the other.

This is where I wholly disagree.

Who decides when the resolve handler should be used to test identity? Why WeakMap and not Map, Set or Array.indexOf? Where should it stop?
Also, it probably shouldn't apply to all WeakMap instances as external users (proxy creator or end-user code) may want to associate data with the proxy object but not the target. Do we need to add new constructors for all those objects?

Either you do identity test for proxy objects everywhere, or you do it nowhere. Doing it everywhere is obviously too costly.
What you need is a way to make the hidden data implementation aware of the equivalence between proxy and target objects, so that it doesn't have to rely on hooks into the identity test. My proposal is one approach to accomplish this.

@rdking
Copy link
Owner Author

rdking commented Apr 23, 2019

Who decides when the resolve handler should be used to test identity?

That's a spec issue. My recommendation is for [[Handler]].resolve to be called whenever the Proxy experiences:

  • an invariant called
  • an internal slot accessed other than [[Target]] and [[Handler]]
  • itself being used as a key to an intrinsic container

Why WeakMap and not Map, Set or Array.indexOf?

Who said it should? I only used WeakMap as an example of a highly common use case. Sorry if that didn't come through in my wording. Case 3 above includes but is by no means limited to WeakMap, Map, WeakSet, and Set.

Where should it stop?

That's what we need to figure out. I don't think it should apply to non-key cases as this implies calling resolve any time a Proxy instance is used as a RHS expression. Not only would that greatly diminish performance, it unnecessarily risks breaking existing code.

Also, it probably shouldn't apply to all WeakMap instances as external users (proxy creator or end-user code) may want to associate data with the proxy object but not the target.

Like everything else with Proxy, it's a "call me if I exist" function. Since all Maps and Sets would use the Proxy for its identity, we could allow the method parameter to be "identity" when used as a key. In this way we can identify whether or not the Proxy should appear transparent for those cases.

My proposal is one approach to accomplish this.

The biggest weakness to your proposal is that it requires an object to be specifically constructed to be Proxy safe. The current ES standard leans in the opposite direction, in that you must construct your class in a Proxy-unsafe way for Proxy to have difficulty with it. This is a problem due to all of the existing code. None of that code would be viable using your approach. Further, new developments would take longer trying to take into account potential uses of Proxy by 3rd party developers. The situation becomes even more complex when a developer uses 2 different code sets together. If one is a library using hidden data and the other a monitoring library using Proxy... I'm sure you can see the difficulty.

The approach I'm offering allows developers to remain ignorant of their user's use of Proxy. The responsibility of ensuring a Proxy doesn't break thus falls to the Proxy developer/user. For this kind of problem, a 100% solution isn't possible, but I believe I'm closing in on an 85%.

@mhofman
Copy link

mhofman commented Apr 23, 2019

Why WeakMap and not Map, Set or Array.indexOf?

Who said it should? I only used WeakMap as an example of a highly common use case. Sorry if that didn't come through in my wording. Case 3 above includes but is by no means limited to WeakMap, Map, WeakSet, and Set.

Where should it stop?

That's what we need to figure out. I don't think it should apply to non-key cases as this implies calling resolve any time a Proxy instance is used as a RHS expression. Not only would that greatly diminish performance, it unnecessarily risks breaking existing code.

Sorry, my wording was probably wrong as well. What I meant to convey is that there are a lot of hidden data implementations out there, some predating Map and other containers that use object as keys.

While putting the limit at "anything in the language that uses the object as a container key" would probably solve most modern use-cases, I'm a little worried this creates a dichotomy in the language where in some special cases we resolve the proxy's target for identity, and in some other we don't.

Also, it probably shouldn't apply to all WeakMap instances as external users (proxy creator or end-user code) may want to associate data with the proxy object but not the target.

Like everything else with Proxy, it's a "call me if I exist" function. Since all Maps and Sets would use the Proxy for its identity, we could allow the method parameter to be "identity" when used as a key. In this way we can identify whether or not the Proxy should appear transparent for those cases.

I guess I wasn't clear there either.
Imagine a library that uses WeakMap to hide data.
Now imagine an unrelated library that wraps the first library's objects in Proxy to observe, and needs to keep track of some information for each proxy object it distributes. It distributes different proxy objects to each user, but they all point to the same target. It would like to use WeakMap to associate the user information to their proxy object.
Since resolve has to be in the handler to allow the target implementation to work, how would the proxy creator go about to use the proxy object as a key for its own use?

The biggest weakness to your proposal is that it requires an object to be specifically constructed to be Proxy safe. The current ES standard leans in the opposite direction, in that you must construct your class in a Proxy-unsafe way for Proxy to have difficulty with it. This is a problem due to all of the existing code. None of that code would be viable using your approach.

I don't think I follow.
Only an object that currently breaks with proxies has to be changed to be proxy compatible again. Regular objects without any hidden data can continue to exist as they do today.
For private fields, which are new, it's a matter of having them implement the proposed hook. To avoid authors from having to do that explicitly, maybe this could be done by default.
I.e. a default newProxy hook would automatically copy the [[PrivateFieldValues]] slot from the target to the proxy (when the proxy object is constructed to allow that).

The approach I'm offering allows developers to remain ignorant of their user's use of Proxy. The responsibility of ensuring a Proxy doesn't break thus falls to the Proxy developer/user. For this kind of problem, a 100% solution isn't possible, but I believe I'm closing in on an 85%.

I believe mine mostly allows that as well. Only if they do something that breaks regular proxies do they need to add an explicit hook. The hook however works for all kind of custom hidden data implementations, or potentially other use-cases we haven't thought of yet.

@rdking
Copy link
Owner Author

rdking commented Apr 23, 2019

I'm a little worried this creates a dichotomy in the language where in some special cases we resolve the proxy's target for identity, and in some other we don't.

That's a reasonable concern, and why I seek to keep the rule as consistent as possible. Since only intrinsic objects can make use of an object as a key, it should be clear and reasonable for this rule to only affect intrinsic objects with that capability (Maps and Sets).

Since resolve has to be in the handler to allow the target implementation to work, how would the proxy creator go about to use the proxy object as a key for its own use?

I presume that in your example, it is the Proxy object's identity that would be stored in the WeakMap. The following might be the rule of key evaluation for intrinsic objects under this idea:

  1. Let c be the intrinsic container.
  2. Let k be the key in question.
  3. Let rval be undefined.
  4. If k is a key of c, then
    a. Let rval be c.get(k).
  5. Else if k is an ECMAScript Proxy instance and "resolve" is a key in k.[[Handler]], then
    a. Let tk be k.[[Target]]
    b. Let rk be k.[[Handler]].resolve(tk, k, false, "identity")
    c. Assert(rk !== null)
    d. Assert(Type(rk) == "object")
    e. If (rk !== k) and rk is a key of c, then
    i. Let rval be c.get(rk)
  6. Return rval

The point is, always try for an exact match first. If that fails, then, if the key is a Proxy and resolve doesn't return key, then return based on the resolved key. The fact that the 4th parameter can hold "identity" helps in resolving cases where you may want a Proxy to key differently from its target while still being able to deal with hidden data implementations.

@mhofman
Copy link

mhofman commented Apr 23, 2019

That's an interesting idea for Map.get (and similar), but what about has, set/add and delete?

Should it always use the proxy object's identity, or should it go through the resolve handler as well?
If the former, has might return false, when in reality resolve's return value is actually in the container, so get would return a value. For that reason, let's assume has always goes through resolve.

That leaves the question of set/add and delete.
If they always use the proxy object's identity, then it doesn't match what has and get do: the container could contain resolve's return value, yet a set operation would not override the resolved entry, an add operation would actually add a value instead of not changing the Set, and delete would not remove the entry.
If set/add and delete go through the handler's resolve, then how do you propose it differentiates between the operations of a WeakMap used internally by the target, and the container operations used by the proxy library. Besides a not very pretty scope flag in the proxy library?

I just don't see how you can cleanly mutate a container with a proxy object as key. At which point checking if proxy objects are themselves in containers becomes pointless.

@rdking
Copy link
Owner Author

rdking commented Apr 23, 2019

I just don't see how you can cleanly mutate a container with a proxy object as key. At which point checking if proxy objects are themselves in containers becomes pointless.

It's possible. Just takes a bit of thought.

what about has, set/add and delete?

Has would follow the same logic as get. Makes sense considering the logic for get essentially includes has.

For set, I'm following the notion that for a Proxy p and a target t, p == t but p !== t. So setting will always be according to identity. Without doing that, it would not be possible to set separate data for both the Proxy object and its target. To get around the misdirection problem, if a Proxy is used in a get operation before the first set operation, and the Proxy's target has already been used in a set, then for maps, the Proxy will be assigned as an alias key for the target. That allows the Proxy developer to determine how interfering the Proxy will become. It also ensures that resolve is only needed once.

On the other hand, delete is a lot more nuanced. I'm open to suggestions here.

@rdking
Copy link
Owner Author

rdking commented Apr 24, 2019

For delete I've worked out the following rules:

  1. If the key is an alias, delete the alias.
  2. Else delete the key.
    a. If the key has an alias, convert the alias to an owning key for the data of the deleted key.
    b. Else delete the data associated with the key

Basically, if multiple keys reference the data, delete all keys to delete the data. Otherwise, the remaining keys will still reference the data and the deleted key is free to be used for different data. The same key-aliasing rule can work for Sets as well as long as the has operation is used as the aliasing trigger.

@mbrowne
Copy link

mbrowne commented Apr 25, 2019

I'm not really sold on the original idea here...do proxies really need to have the power to arbitrarily change the target to any object? If sticking with the general approach of a resolve() method, what if you just returned true or false indicating whether or not the operation should tunnel (instead of returning an object)?

I did see @ljharb's comment about multiple targets already being one of the intended use cases for proxies, which is fine for public slots (I'm assuming "public slots" is a valid term—perhaps I should just say public properties), but when internal slots get involved, I suspect there will be a host of potential encapsulation issues and other complexities that would otherwise be avoided.

@mhofman
Copy link

mhofman commented Apr 25, 2019

I'm not sure I follow the alias idea and how it'd work.

I've created a repository to illustrate the following: https://github.com/mhofman/proposal-proxy-resolve-playground

Let's assume a class has hidden data through multiple WeakMap, with the values being primitives (it's the hardest since we can't rely on references to the same object). This is actually what some versions of babel do. Some WeakMap may get a set done in their constructor, so the "key" will be the target. Others may only be done later, so the "key" will be the proxy object. By "key" I mean what the class implementation ends up using as WeakMap.set()'s first argument, not what may potentially be resolved by the handler's trap.

Now let's assume another library that monitors accesses to the above class' instances. It creates a proxy object for each of its users so that it can link each access to the relevant user. Multiple proxy objects may then share the same target. It uses a WeakMap to keep track of its proxy's users.

I've tried to implement your resolve trap suggestion to my best understanding of it, removing some things I don't see as necessary right now. The signature is resolve(target, receiver, privateIdentity) with privateIdentity a boolean indicating that the resolution is for accessing some private data associated to the target, e.g. an internal slot, or the key identity in a Map.

As you can see, I'm not sure how to implement the Map.set and Map.delete operations to work in all circumstances. For now I've just added a useResolve flag to those operations to convey what I believe should end up happening.

@rdking
Copy link
Owner Author

rdking commented Apr 25, 2019

@mbrowne

what if you just returned true or false indicating whether or not the operation should tunnel ... ?

I thought about that a couple of days after my suggested changes in 106. I really don't like the idea of the resolve function being able to return any arbitrary object. At the same time, I have to take into account the intent @ljharb mentioned. What I need to know is whether or not that intent extends to internal slots. If I go by the current functionality, I'd wager that the intent should not be extended that far, and that it's ok to reduce the return value of resolve to a boolean. That would make me a lot happier. If, however, the intent must be extended to internal slots, then we don't have much wiggle room.

@rdking
Copy link
Owner Author

rdking commented Apr 25, 2019

@mhofman

I'm not sure I follow the alias idea and how it'd work.

That you mentioned Babel makes this easier. Babel's implementation of private fields is a good example to draw from. If memory serves, all data stored in a private field WeakMap is wrapped in an object akin to a property descriptor object. If we apply the same concept to this idea, all Maps would have a "container" they create and own for each value added to the Map. As such, I would tend to implement Map something like this:

//The map needs to know how to unwrap a Proxy, hence `Proxy.unwrap`.
class AliasingMap extends Map {
  has(key) {
    return super.has(key) || super.has(UProxy.unwrap(key));
  }
  get(key) {
    let retval;
    if (super.has(key) {
      retval = super.get(key);
    }
    else {
      let pkey = UProxy.unwrap(key);
      if (super.has(pkey)) {
        retval = super.get(pkey);
        super.set(key, retval);
      }
    }
    return retval.value;
  }
  set(key, value) {
    let container = super.get(key) || {};
    container.value = value;
    super.set(key, value};
  }
  /* No need to redefine delete. */
}

@rdking
Copy link
Owner Author

rdking commented Apr 25, 2019

BTW, here's the corresponding Proxy implementation:

let UProxy = (function() {
  let pvt = new WeakMap();
  return class UProxy {
    constructor(target, handler) {
      let rval = new Proxy(target, handler);
      pvt.set(rval, target);
      return rval;
    }

    static unwrap(obj) {
      console.log("Unwrapping Proxy object...");
      return pvt.get(obj);
    }
  }
})();

@mhofman
Copy link

mhofman commented Apr 25, 2019

That you mentioned Babel makes this easier. Babel's implementation of private fields is a good example to draw from. If memory serves, all data stored in a private field WeakMap is wrapped in an object akin to a property descriptor object. If we apply the same concept to this idea, all Maps would have a "container" they create and own for each value added to the Map.

Babel does indeed save a property descriptor in recent versions. I mentioned more as an example.
From the developer's point of view, we do need to allow primitives to be stored as value. If it ends up wrapped inside the container as an object, that's fine, but it needs to be fully transparent for all accesses (including iterators, etc.)
Same goes for aliases. If there are multiple aliased keys, those probably should be hidden and not iterable.

Regarding your code example, I'm even more confused as I thought you were advocating for a resolve trap that would be invoked on Map operations when the proxy is used as a key.
I don't think your unwrap code example would work with the requirement I exposed earlier, as it basically does what a basic resolve would do (use target instead of receiver for private/identity operations). As I've shown before, a simple Map.set with the proxy as key doesn't always work.

Feel free to create a PR on my playground repo if you can make it work.

@rdking
Copy link
Owner Author

rdking commented Apr 25, 2019

Sorry about that. Been typing here while distracted by work. 😜

The unwrap and resolve approaches are a little different, but not by much. Every place where unwrap is called above would be replaced by a call to the resolve handler, followed by a check to see if the receiver was the same as the result. If they're not the same, then proceed as if the result was the return value of unwrap.

@rdking
Copy link
Owner Author

rdking commented Apr 25, 2019

This is the Proxy I mean to give you. This is the one that will have resolve support. I'll work on a PR for your repo later tonight.

let RProxy = (function() {
  const map = new WeakMap;
  return class RProxy {
    constructor(target, handler) {
      let retval = new Proxy(target, new Proxy(handler, {
        get(t, key, r) {
          return function(...args) {
            let [tgt, prop] = args;
            let receiver = retval;
            if ((prop != "resolve") && ("resolve" in tgt)) {
              let isPvt = !(["string", "symbol"].includes(typeof(prop)));
              receiver = tgt.resolve(target, receiver, isPvt, key);
              assert(receiver && (typeof(receiver) == "object"));
            }

            if (key == "get") {
              args[2] = receiver;
            }
            else if (key == "set") {
              args[3] = receiver;
            }
 
            return (handler[key] || Reflect[key])(...args);
          }
        }
      }));
      map.set(retval, target);
      return retval;
    }
  }
})();

@trusktr
Copy link

trusktr commented Dec 16, 2019

Hello! What's the TLDR with private fields and Proxy?

EDIT: is this a better question for the new forums?

@ljharb
Copy link

ljharb commented Dec 16, 2019

@trusktr that's a good place to ask; but the answer is "it's the same as internal slots; they don't tunnel through proxies" (except for Array.isArray, and [[Call]])

@mbrowne
Copy link

mbrowne commented Dec 17, 2019

The proxy issue is also mentioned in the private fields syntax FAQ:

Private fields of a Proxy target aren't accessible from the Proxy itself. For example, the following will throw a TypeError:

class K { #x; get x() { return this.#x; } }
let k = new Proxy(new K, { });
k.x  // TypeError

Read more at
https://github.com/tc39/proposal-class-fields/blob/master/PRIVATE_SYNTAX_FAQ.md#how-do-private-fields-interact-with-proxy

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

No branches or pull requests

5 participants