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

Should CompositeKey have value-like equality? #2

Open
acutmore opened this issue Dec 15, 2023 · 12 comments
Open

Should CompositeKey have value-like equality? #2

acutmore opened this issue Dec 15, 2023 · 12 comments

Comments

@acutmore
Copy link
Owner

acutmore commented Dec 15, 2023

Should CompositeKey have value-like equality?

Yes:

This would look something like this:

const obj = {};
const a = CompositeKey(obj, 1, 2);
const b = CompositeKey(obj, 1, 2);
assert(typeof a === "object");
assert(Reflect.ownkeys(a).length === 0); // or maybe only `Symbl.toStringTag` like ESM namespaces
assert(a === b):
assert(Reflect.getPrototypeOf(a) === null);

No:

This would look something like this:

const obj = {};
const a = new CompositeKey(obj, 1, 2);
const b = new CompositeKey(obj, 1, 2);

// same as above:
assert(typeof a === "object");
assert(Reflect.ownkeys(a).length === 0); // or maybe only `Symbl.toStringTag` like ESM namespaces

// different:
assert(a !== b);
assert(Reflect.getPrototypeOf(a) === CompositeKey.prototype);
assert(CompositeKey.equal(a, b));

Pros/Cons

Value like semantics

  • Pro: Maps and Sets don't need to beware of CompositeKey and treat it differently from other objects
  • Con: To avoid tying the equality of the CK to a realm it would need to have a null prototype.
  • Con: Effectively forces implementations to do global interning at the point in time when CKs are created, delaying until === can add overhead to every === call.
  • Con: May enforce that at least one constituent has a lifetime (CompositeKey(1, 2) throws).
  • Pro? Could avoid new

Unique Object

  • Pro: Allows more freedom of when work is done when it comes to computing hashes and equality.
  • Pro: CKs can have a prototype, which may be useful for adding methods/symbols in the future.
  • Pro: No requirement for at least one lifetime bearing constituent (new CompositeKey(1, 2) allowed)
  • Con: CKs would be a special type of object when returned by Map and Set with a keyby function.
    • more work to virtualise.
  • Con? Would most likely need to be created with new
This was referenced Dec 15, 2023
@acutmore
Copy link
Owner Author

Updated to reflect that value-like-semantics are likely to come with a requirement that at least one constituent one the key has a lifetime - like https://github.com/tc39/proposal-richer-keys

So CompositeKey(1, 2, 3) would throw, with a work around being CompositeKey(Object, 1, 2, 3).

The reason for this is that otherwise CompositeKeys could make it too easy to create memory leaks. And while CompositeKey(Object, 1, 2, 3) also has the same risk there has been somewhat more of an explicit opt-in to the CK having an extended, potentially eternal, lifetime.

@shicks
Copy link
Collaborator

shicks commented Dec 18, 2023

@acutmore in #2 (comment):

Updated to reflect that value-like-semantics are likely to come with a requirement that at least one constituent one the key has a lifetime - like https://github.com/tc39/proposal-richer-keys

Note that Bradley's proposal was to have two different kinds of keys: compositeKey that had lifetime and compositeSymbol that didn't (but required only primitive components). I think this leaves us with two different versions of the value-like semantics option:

Value-like semantics, no analogue to compositeSymbol

  • Pro: Straightforward and ergonomic (modulo needing a lifetime object)
  • Pro: Enables composite WeakMap keys
  • Con: Requires a hacky workaround in the (most-common?) case of primitive-only composites; most developers would likely cargo-cult a global object here and end up leaking memory anyway

Value-like semantics on CompositeKey, non-value semantics on CompositeSymbol

  • Pro: Enables composite WeakMap keys
  • Con: Exposes confusing implementation details that most users likely won't understand
  • Con: Foot-gun w.r.t. different === behavior

@nicolo-ribaudo
Copy link
Collaborator

Con: Requires a hacky workaround in the (most-common?) case of primitive-only composites;

An option could be to throw not when creating a primitives-only composite key, but when trying to use it in a weakmap.

@ljharb
Copy link
Collaborator

ljharb commented Dec 18, 2023

That seems like a much better choice.

@mhofman
Copy link
Collaborator

mhofman commented Dec 18, 2023

An option could be to throw not when creating a primitives-only composite key, but when trying to use it in a weakmap.

That would be very surprising, if even Web compatible, to have typeof ck === 'object' and not be able to use it a WM key.

IMO, this constraint really leads to the unique object semantics. It's unfortunate to require user code explicitly use a new equality predicate, but I don't see how we can do otherwise if we want to use an object.

There is still the possibility of overloading symbols I guess, since those are not universally acceptable as unique keys. But the mental overload of having even more kinds of symbols might be too high for authors as well.

@ljharb
Copy link
Collaborator

ljharb commented Dec 18, 2023

@mhofman that's already the case with typeof x === 'symbol', so I don't think it would be surprising. Whether it's web compatible is certainly a different question.

@mhofman
Copy link
Collaborator

mhofman commented Dec 18, 2023

I don't understand. Today all type 'object' are usable as WM key. Making some unusable would be very surprising.

@nicolo-ribaudo
Copy link
Collaborator

Regarding web compatibility, no website currently uses CompositeKey and thus their behavior in weakmaps does not risk breaking existing websites.

@mhofman
Copy link
Collaborator

mhofman commented Dec 19, 2023

Given how composable the ecosystem is, I don't think that's a fair assessment. There are definitely libraries out there that will assume an object they are passed as argument can be used as a WeakMap key. We've had the same discussion regarding R&T

@nicolo-ribaudo
Copy link
Collaborator

And exactly as it was for R&T, that problem increases adoption friction but doesn't break existing websites.

There is no library today that would stop working (breaking some app/website) if browsers implement a CompositeKey that cannot be used as a weakmap key. However, developers of those website cannot start using CompositeKey with those libraries until they implement support for it.

@bakkot
Copy link

bakkot commented Jan 31, 2024

Making === work is much better, I think. Specializing Map and Set is annoying - what if I want to use an array with .includes instead? Or something like the uniqueBy proposal? Either all of these things need to be specialized for CompositeKey, or we make === work.

Is the global intern table that bad? I've done it in userland. If the concern is side-channels, we could instead expose a makeCompositeKey constructor which gives you a fresh constructor with its own intern table, which would still work for >99% of use cases I think.

Con: May enforce that at least one constituent has a lifetime (CompositeKey(1, 2) throws).

Say more about why? Is it just so that you can garbage-collect CompositeKey objects even if they're still used as a key in a WeakMaps? I think having a documented limitation that CompositeKeys can be collected if you have no strong references to them would be fine. Occasionally surprising, but it's hard to see how this break any of the intended use cases.

@acutmore
Copy link
Owner Author

Is the global intern table that bad

The pollyfill in this repo uses an intern-trie. It's not a problem to implement - can be done in userland.

For an API like CompositeKey.equal(key(1, 2, 3, ...N), key(2, 3, ...N)) this can return that they are not equal on checking the first value. Interning the keys at creation time could add significant overhead for such short lived keys. "Most objects die young" and all that.

I completely agree that === semantics simplify things massively, as all the existing places that check equality would "just work". And that is a significant win.

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

6 participants