tc39 / proposal-setmap-offrom

https://tc39.es/proposal-setmap-offrom/
39 stars 9 forks source link

Allow cloning WeakMaps/WeakSets with `.from` #26

Open bakkot opened 4 years ago

bakkot commented 4 years ago

Currently WeakMap.from(x) requires x to be an iterable, which WeakMaps are not. It might make sense to allow x to be a WeakMap, in which case WeakMap.from(x) would return a copy of x. That's not currently possible in the language, but I am pretty confident it wouldn't violate any important guarantees.

Alternatively, we might say that WeakSet.prototype.union (see https://github.com/tc39/proposal-set-methods/issues/23) is the right place for that ability. I am neutral on that question.

zloirock commented 4 years ago

It looks wrong for me and somehow related to removing of WeakMap.prototype.clear. IIRC one of the problems was a performance problem for implementations that want to implement WeakMaps as internal slots on objects stored as keys.

ljharb commented 4 years ago

They could still do that - by copying the internal slots over, for example.

zloirock commented 4 years ago

In this case, it's also will be a performance problem. But IIRC it's only one of many reasons for removing WeakMap.prototype.clear.

zloirock commented 4 years ago

@ljharb BTW, for example, it's our primitive slot-based WeakMap:

class WeakMap {
  static #ids = Symbol();
  #id = Symbol();
  constructor(init) {
    if (init) for (const el of init) this.set(el[0], el[1]);
  }
  delete(key) {
    if (key[WeakMap.#ids]) return key[WeakMap.#ids].delete(this.#id);
  }
  get(key) {
    if (key[WeakMap.#ids]) return key[WeakMap.#ids].get(this.#id);
  }
  has(key) {
    return !!key[WeakMap.#ids] && key[WeakMap.#ids].has(this.#id);
  }
  set(key, value) {
    if (!hasOwnProperty.call(key, WeakMap.#ids)) {
      Object.defineProperty(key, WeakMap.#ids, { value: new Map() });
    }
    key[WeakMap.#ids].set(this.#id, value);
    return this;
  }
}

How do you implement cloning without performance degradation (at least on cloned weak maps)?

ljharb commented 4 years ago
static from(weakMap) {
  const wm = new WeakMap():
  wm[ids] = weakMap[ids];
  return wm;
}
zloirock commented 4 years ago

And, in this case, any changes on clone affect original, on original - clone (if I understood your idea correctly, since ids is not a weakmap field). It's not a clone, it's just a new reference to old weakmap. If each weakmap will have a set of ids - it will cause performance degradation for clones - at least 2x for the first, 1001x for 1000th.

ljharb commented 4 years ago

Ah, that's true. In that case, there's no way to avoid enumeration for a polyfill, but performance considerations of userland implementations don't tend to hold much weight when designing a feature, historically.

zloirock commented 4 years ago

One more time - it's not about userland, it's about engines implementations which works in this way, see my first comment.

ljharb commented 4 years ago

I'd be happy to hear from engine implementors if that in fact is a concern.

zloirock commented 4 years ago

I don't know how it works in popular engines, but at least it will cause problems for developers of new engines. Also interesting opinions of implementors -)

zloirock commented 4 years ago

Also, it will cause some security issues. For example:

// frozen env where we can't patch WeakSet and methods
// somehow we got access to a weakset
const clone = WeakSet.from(weakset);
// sometime later the `weakset` is cleared or just `object` key removed from it
// and now we have access to the `object`
clone.has(object);
// we could know that the `object` was in the `weakset` in the moment of creating a clone

It's far-fetched, but the idea of WeakMap / WeakSet is that we can't observe keys.

bakkot commented 4 years ago

I agree that this is a capability which was not previous present, but I don't think it's likely to be a security issue. The important guarantee that a WeakMap provides is that having the map does not allow you to access its contents. That guarantee is preserved.

zloirock commented 4 years ago

In this case, making all private fields public in the next version of an abstract language is not a security issue, just a new possibility.

I think that it should solve the committee.

bakkot commented 4 years ago

... what?

zloirock commented 4 years ago
bakkot commented 4 years ago

I don't think this makes keys observable

zloirock commented 4 years ago

It makes keys observable, an example above.

ljharb commented 4 years ago

If you have object, you already know if it's in weakset, so knowing if it's in clone isn't new information.

bakkot commented 4 years ago

The example above does not demonstrate keys being observable.

zloirock commented 4 years ago

@ljharb at the moment of creation clone we haven't access to the object, when we have access to the object it's missed in the weakset. It's observability of a private flag.

zloirock commented 4 years ago

@bakkot one more time - lets the committee solve is this a security issue or not. In my opinion - yes, since I see some places where it can be exploited.

ljharb commented 4 years ago

I see what you're saying, but you also can't know if someone did clone.add(object) or weakset.delete(object) in the "some time after" interim (and if you can know that, there's no new information to be had).

zloirock commented 4 years ago

@ljharb since we create a clone, we have full control over it. About weakset.delete(object) or another operation - yes, but now we have a chance to find it out - that's what I wrote.