acutmore / proposal-keyby

1 stars 1 forks source link

CompositeKey and WeakMaps #3

Open acutmore opened 10 months ago

acutmore commented 10 months ago

Should CompositeKey be allowed as a WeakMap key, and if so what would the semantics be? (also depends on #2).

And should there be a keyBy option when constructing new WeakMap([], { keyBy: ... }) (and WeakSet).

ljharb commented 10 months ago

If #2 is yes, then yes, else no.

mhofman commented 10 months ago

Why not allow in both case? If composite key is an object, it would be very surprising to not allow as WM key.

First, feature-wise, I want composite weak keys. Building tries of WeakMap is extremely annoying.

If #2 is no (unique object), I expect to have a similar keyBy option for WeakMap to have the ability of creating composite weak keys. However without keyBy being used, CompositeKey remains just another object, and should be allowed as a regular WM key.

ljharb commented 10 months ago

oh, true, ofc as an object it must be allowed - but you don't have to do anything for that except not actively disallow it.

shicks commented 10 months ago

I think the starting point for this question is that WeakMap should have as similar semantics to Map as possible. It's worth expanding on the options here explicitly in terms of the choices in #2.

If CompositeKey(1, 2) === CompositeKey(1, 2)

Global interning makes this question trivial: treating them as ordinary objects in WeakMap just works.

If CompositeKey(1, 2) !== CompositeKey(1, 2)

The most obvious choice here is that using a CompositeKey as an ordinary key (without keyBy) should do the same thing as Map (which is to treat two equivalent keys as different objects) and tie the entry's lifetime to the specific object. The main question this leaves is how to handle a keyBy'd composite key with only primitive components. I see four remotely reasonable options:

  1. Don't support keyBy in WeakMap at all. This would be disappointing as @mhofman (and others) have expressed that we'd like to have weak composite keys.
  2. Prevent creating primitive-only composite keys. This is unsatisfying because one of the main reasons to not intern was to lift this restriction. See my discussion of gotchas when requiring a mandatory object component. I suspect this would lead to widespread memory leakage, though likely to be significantly less than if all created keys were interned unconditionally.
  3. Throw when adding the primitive-only composite key. This would be surprising because it's the first instance of an typeof x === 'object' not being allowed in a WeakMap.
  4. Allow primitive-only composite keys in WeakMap with an understanding that it will never clean itself up and thus will always leak (an alternative would be that it immediately cleans itself up, but I believe option 3 above to be strictly preferable to this).
mhofman commented 10 months ago

3. Throw when adding the primitive-only composite key. This would be surprising because it's the first instance of an typeof x === 'object' not being allowed in a WeakMap.

I don't think that's accurate. Given that the composite key would be returned by a new keyBy option, it's entirely acceptable for valid values to be more restrictive than what is acceptable for the key value.

For Map I actually advocated in #1 that only primitives or composite keys should be valid keyBy return values. For WeakMap, only non registered symbols and composite keys containing at least an object or a non-registered symbol would be valid (aka the "weakable" subset of primitive and all composite keys).

As such I believe 3. here is the obvious choice if we go for unique object semantics for Composite Keys. Because of the WeakMap restrictions, I don't believe that value-like object semantics is viable. We can still explore a 3rd option, which is to have a new type of symbol to represent composite keys (which would have unique or registered-like WeakMap behavior depending on the constituents of the composite key)

shicks commented 10 months ago

That's a good point - I was conflating the situation with the interned one where not using keyBy would make it more surprising.

Would this symbol-based option be (roughly) analogous to extending Symbol.for to take an arbitrary list of arguments, and it would return a symbol that's allowed as a WeakMap key if and only if there's an object somewhere in the list? VMs would presumably then have freedom to represent these symbols however they wanted internally. This seems like a hybrid between the interning option and the object semantics. Is it worth splitting into its own issue to investigate further?

mhofman commented 10 months ago

Would this symbol-based option be (roughly) analogous to extending Symbol.for to take an arbitrary list of arguments

Maybe? For once there would be no keyFor equivalent to get back at the constituents.

This seems like a hybrid between the interning option and the object semantics.

I'm not sure. We still have to decide up front if the "equal" symbols would be ===. But at least where WM is concerned, it would not be surprising to have some values not being valid keys, unlike for typeof 'object'.

Is it worth splitting into its own issue to investigate further?

Yeah I can open an issue.