indexmap-rs / equivalent

Rust traits for key comparison in maps.
Apache License 2.0
18 stars 2 forks source link

Move into std #3

Open jplatte opened 1 year ago

jplatte commented 1 year ago

I think that there could be problems w.r.t. inference, but has moving the traits from this crate into std been considered? I couldn't find any issues on the main rust repo about it.

cuviper commented 1 year ago

I think that there could be problems w.r.t. inference,

FWIW, I noted that in https://github.com/bluss/indexmap/issues/253#issuecomment-1459160166, it actually seems to work better if K and Q are reversed, but I ended up not pursuing that further, in sort of a "worse is better" moment to just get something done. If experience shows that it really is better that way, that could be equivalent v2, or maybe a change to make when importing the idea to the standard library.

but has moving the traits from this crate into std been considered? I couldn't find any issues on the main rust repo about it.

I have not, nor would I likely have time for that, but it may make sense to see if it gets any real community use as an external crate first. Of course there are also users who will ignore it until it's in the standard library. If you're interested, feel free to propose this yourself!

jplatte commented 1 year ago

Yeah, it's a bit annoying having to swap the std HashMap for hashbrown::HashMap just to get this, but I might end up doing it. I will likely also not have time to seriously drive an effort of moving this stuff into std, was mostly curious whether I had missed an existing effort that I could follow.

Thanks!

JakkuSakura commented 8 months ago

it actually seems to work better if K and Q are reversed

It would be easier to implement Equivalent if it's inversed Current:

enum Key {
  K(u32, ExternalKey)
}
impl Equivalent<Key> for (u32, ExternalKey) {
    ..
}

Inversed:

enum Key {
  K(u32, ExternalKey)
}
impl Equivalent<(u32, ExternalKey)> for Key {
    ..
}

Update: disregard this. I forgot there is the orphan rule with relaxed clause.

clarfonthey commented 3 months ago

Just poking in here since this was pointed out by https://github.com/rust-lang/rust/issues/27242#issuecomment-2267133850

As far as crates.io is concerned, only 8 crates directly depend on equivalent, which makes it seem like not many crates have run into a problem where Equivalent works but Borrow doesn't. It would be nice for folks to offer compelling examples of where this trait was useful for them (since I don't doubt they exist), since right now very little data is easily available on how many people are using this.

Like, I think it'd be nice to have this in the standard library, but there's not really a compelling set of data to say it's needed there.

GrigorenkoPV commented 2 months ago

where Equivalent works but Borrow doesn't. It would be nice for folks to offer compelling examples of where this trait was useful for them

https://github.com/droundy/internment/blob/8a5feb354d023a7c69f3889e9e737099ec0e4ea7/src/boxedset.rs#L49-L63

~I think this one can be rewritten using these two traits, but not with std's Borrow and how it is currently used in HashMap.~

Essentially we have HashSet<Box> and we want to do a lookup using &str. This would be possible if we had HashSet, but we need a Box for address stability. Currently this uses the raw_entry API, ~but I think it would be possible to rewrite it using something like~


struct Newtype<T>(Box<T>);

impl<Q, K> Equivalent<K> for Newtype<Q>
where
    Q: Eq + ?Sized,
    K: Borrow<Q> + ?Sized,
{ ... }

type BoxedSet<T> = HashSet<NewType<T>>;

UPD: No, it's not possible. You have to do Equivalent<Newtype<Q>> for K, but the orphan rule won't let you. So yes,

it actually seems to work better if K and Q are reversed,

al8n commented 2 weeks ago

I also want this crate to be moved into std, not only for HashMap, but also the Comparable trait for BTreeMap, the traits in this crate improve the flexibility of lookup comparing the current Borrow trait.

I opened a PR which uses Comparable trait instead of Borrow in crossbeam-skiplist, the flexibility given by the Comparable trait shows that around 10% performance improvement for the lookup.