From eb3aa7ea2ea59d2d5114c59793ee86537bf68e13 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timoth=C3=A9e=20Haudebourg?= Date: Fri, 22 Jul 2022 13:49:19 +0200 Subject: [PATCH 1/6] Define `Equivalent` trait. --- src/lib.rs | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/src/lib.rs b/src/lib.rs index bc1c971303..659aa325e2 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -117,6 +117,39 @@ pub mod hash_set { pub use crate::map::HashMap; pub use crate::set::HashSet; +/// Key equivalence trait. +/// +/// This trait defines the function used to compare the input value with the +/// map keys (or set values) during a lookup operation sur as [`HashMap::get`] +/// or [`HashSet::contains`]. +/// It is provided with a blanket implementation based on the +/// [`Borrow`](core::borrow::Borrow) trait. +/// +/// # Correctness +/// +/// Equivalent values must hash to the same value. +pub trait Equivalent { + /// Checks if this value is equivalent to the given key. + /// + /// Returns `true` if oth values are equivalent, and `false` otherwise. + /// + /// # Correctness + /// + /// When this function returns `true`, both `self` and `key` must hash to + /// the same value. + fn equivalent(&self, key: &K) -> bool; +} + +impl Equivalent for Q +where + Q: Eq, + K: core::borrow::Borrow, +{ + fn equivalent(&self, key: &K) -> bool { + self == key.borrow() + } +} + /// The error type for `try_reserve` methods. #[derive(Clone, PartialEq, Eq, Debug)] pub enum TryReserveError { From 75a9ef979b7502ce123631d5af77b65ac6131911 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timoth=C3=A9e=20Haudebourg?= Date: Fri, 22 Jul 2022 13:49:41 +0200 Subject: [PATCH 2/6] Use `Equivalent` trait in `HashMap`. --- src/map.rs | 129 +++++++++++++++++++++-------------------------------- 1 file changed, 52 insertions(+), 77 deletions(-) diff --git a/src/map.rs b/src/map.rs index a5d3ccb97e..020e7ee895 100644 --- a/src/map.rs +++ b/src/map.rs @@ -1,5 +1,5 @@ use crate::raw::{Allocator, Bucket, Global, RawDrain, RawIntoIter, RawIter, RawTable}; -use crate::TryReserveError; +use crate::{Equivalent, TryReserveError}; use core::borrow::Borrow; use core::fmt::{self, Debug}; use core::hash::{BuildHasher, Hash}; @@ -209,13 +209,12 @@ impl Clone for HashMap(hash_builder: &S) -> impl Fn(&(Q, V)) -> u64 + '_ +pub(crate) fn make_hasher(hash_builder: &S) -> impl Fn(&(Q, V)) -> u64 + '_ where - K: Borrow, Q: Hash, S: BuildHasher, { - move |val| make_hash::(hash_builder, &val.0) + move |val| make_hash::(hash_builder, &val.0) } /// Ensures that a single closure type across uses of this which, in turn prevents multiple @@ -223,10 +222,9 @@ where #[cfg_attr(feature = "inline-more", inline)] fn equivalent_key(k: &Q) -> impl Fn(&(K, V)) -> bool + '_ where - K: Borrow, - Q: ?Sized + Eq, + Q: ?Sized + Equivalent, { - move |x| k.eq(x.0.borrow()) + move |x| k.equivalent(&x.0) } /// Ensures that a single closure type across uses of this which, in turn prevents multiple @@ -234,17 +232,15 @@ where #[cfg_attr(feature = "inline-more", inline)] fn equivalent(k: &Q) -> impl Fn(&K) -> bool + '_ where - K: Borrow, - Q: ?Sized + Eq, + Q: ?Sized + Equivalent, { - move |x| k.eq(x.borrow()) + move |x| k.equivalent(x) } #[cfg(not(feature = "nightly"))] #[cfg_attr(feature = "inline-more", inline)] -pub(crate) fn make_hash(hash_builder: &S, val: &Q) -> u64 +pub(crate) fn make_hash(hash_builder: &S, val: &Q) -> u64 where - K: Borrow, Q: Hash + ?Sized, S: BuildHasher, { @@ -1012,7 +1008,7 @@ where #[cfg_attr(feature = "inline-more", inline)] pub fn reserve(&mut self, additional: usize) { self.table - .reserve(additional, make_hasher::(&self.hash_builder)); + .reserve(additional, make_hasher::<_, V, S>(&self.hash_builder)); } /// Tries to reserve capacity for at least `additional` more elements to be inserted @@ -1062,7 +1058,7 @@ where #[cfg_attr(feature = "inline-more", inline)] pub fn try_reserve(&mut self, additional: usize) -> Result<(), TryReserveError> { self.table - .try_reserve(additional, make_hasher::(&self.hash_builder)) + .try_reserve(additional, make_hasher::<_, V, S>(&self.hash_builder)) } /// Shrinks the capacity of the map as much as possible. It will drop @@ -1084,7 +1080,7 @@ where #[cfg_attr(feature = "inline-more", inline)] pub fn shrink_to_fit(&mut self) { self.table - .shrink_to(0, make_hasher::(&self.hash_builder)); + .shrink_to(0, make_hasher::<_, V, S>(&self.hash_builder)); } /// Shrinks the capacity of the map with a lower limit. It will drop @@ -1113,7 +1109,7 @@ where #[cfg_attr(feature = "inline-more", inline)] pub fn shrink_to(&mut self, min_capacity: usize) { self.table - .shrink_to(min_capacity, make_hasher::(&self.hash_builder)); + .shrink_to(min_capacity, make_hasher::<_, V, S>(&self.hash_builder)); } /// Gets the given key's corresponding entry in the map for in-place manipulation. @@ -1174,10 +1170,9 @@ where #[cfg_attr(feature = "inline-more", inline)] pub fn entry_ref<'a, 'b, Q: ?Sized>(&'a mut self, key: &'b Q) -> EntryRef<'a, 'b, K, Q, V, S, A> where - K: Borrow, - Q: Hash + Eq, + Q: Hash + Equivalent, { - let hash = make_hash::(&self.hash_builder, key); + let hash = make_hash::(&self.hash_builder, key); if let Some(elem) = self.table.find(hash, equivalent_key(key)) { EntryRef::Occupied(OccupiedEntryRef { hash, @@ -1216,8 +1211,7 @@ where #[inline] pub fn get(&self, k: &Q) -> Option<&V> where - K: Borrow, - Q: Hash + Eq, + Q: Hash + Equivalent, { // Avoid `Option::map` because it bloats LLVM IR. match self.get_inner(k) { @@ -1248,8 +1242,7 @@ where #[inline] pub fn get_key_value(&self, k: &Q) -> Option<(&K, &V)> where - K: Borrow, - Q: Hash + Eq, + Q: Hash + Equivalent, { // Avoid `Option::map` because it bloats LLVM IR. match self.get_inner(k) { @@ -1261,13 +1254,12 @@ where #[inline] fn get_inner(&self, k: &Q) -> Option<&(K, V)> where - K: Borrow, - Q: Hash + Eq, + Q: Hash + Equivalent, { if self.table.is_empty() { None } else { - let hash = make_hash::(&self.hash_builder, k); + let hash = make_hash::(&self.hash_builder, k); self.table.get(hash, equivalent_key(k)) } } @@ -1298,8 +1290,7 @@ where #[inline] pub fn get_key_value_mut(&mut self, k: &Q) -> Option<(&K, &mut V)> where - K: Borrow, - Q: Hash + Eq, + Q: Hash + Equivalent, { // Avoid `Option::map` because it bloats LLVM IR. match self.get_inner_mut(k) { @@ -1330,8 +1321,7 @@ where #[cfg_attr(feature = "inline-more", inline)] pub fn contains_key(&self, k: &Q) -> bool where - K: Borrow, - Q: Hash + Eq, + Q: Hash + Equivalent, { self.get_inner(k).is_some() } @@ -1362,8 +1352,7 @@ where #[cfg_attr(feature = "inline-more", inline)] pub fn get_mut(&mut self, k: &Q) -> Option<&mut V> where - K: Borrow, - Q: Hash + Eq, + Q: Hash + Equivalent, { // Avoid `Option::map` because it bloats LLVM IR. match self.get_inner_mut(k) { @@ -1375,13 +1364,12 @@ where #[inline] fn get_inner_mut(&mut self, k: &Q) -> Option<&mut (K, V)> where - K: Borrow, - Q: Hash + Eq, + Q: Hash + Equivalent, { if self.table.is_empty() { None } else { - let hash = make_hash::(&self.hash_builder, k); + let hash = make_hash::(&self.hash_builder, k); self.table.get_mut(hash, equivalent_key(k)) } } @@ -1431,8 +1419,7 @@ where /// ``` pub fn get_many_mut(&mut self, ks: [&Q; N]) -> Option<[&'_ mut V; N]> where - K: Borrow, - Q: Hash + Eq, + Q: Hash + Equivalent, { self.get_many_mut_inner(ks).map(|res| res.map(|(_, v)| v)) } @@ -1487,8 +1474,7 @@ where ks: [&Q; N], ) -> Option<[&'_ mut V; N]> where - K: Borrow, - Q: Hash + Eq, + Q: Hash + Equivalent, { self.get_many_unchecked_mut_inner(ks) .map(|res| res.map(|(_, v)| v)) @@ -1543,8 +1529,7 @@ where ks: [&Q; N], ) -> Option<[(&'_ K, &'_ mut V); N]> where - K: Borrow, - Q: Hash + Eq, + Q: Hash + Equivalent, { self.get_many_mut_inner(ks) .map(|res| res.map(|(k, v)| (&*k, v))) @@ -1599,8 +1584,7 @@ where ks: [&Q; N], ) -> Option<[(&'_ K, &'_ mut V); N]> where - K: Borrow, - Q: Hash + Eq, + Q: Hash + Equivalent, { self.get_many_unchecked_mut_inner(ks) .map(|res| res.map(|(k, v)| (&*k, v))) @@ -1611,12 +1595,11 @@ where ks: [&Q; N], ) -> Option<[&'_ mut (K, V); N]> where - K: Borrow, - Q: Hash + Eq, + Q: Hash + Equivalent, { let hashes = self.build_hashes_inner(ks); self.table - .get_many_mut(hashes, |i, (k, _)| ks[i].eq(k.borrow())) + .get_many_mut(hashes, |i, (k, _)| ks[i].equivalent(k)) } unsafe fn get_many_unchecked_mut_inner( @@ -1624,22 +1607,20 @@ where ks: [&Q; N], ) -> Option<[&'_ mut (K, V); N]> where - K: Borrow, - Q: Hash + Eq, + Q: Hash + Equivalent, { let hashes = self.build_hashes_inner(ks); self.table - .get_many_unchecked_mut(hashes, |i, (k, _)| ks[i].eq(k.borrow())) + .get_many_unchecked_mut(hashes, |i, (k, _)| ks[i].equivalent(k)) } fn build_hashes_inner(&self, ks: [&Q; N]) -> [u64; N] where - K: Borrow, - Q: Hash + Eq, + Q: Hash + Equivalent, { let mut hashes = [0_u64; N]; for i in 0..N { - hashes[i] = make_hash::(&self.hash_builder, ks[i]); + hashes[i] = make_hash::(&self.hash_builder, ks[i]); } hashes } @@ -1677,7 +1658,7 @@ where Some(mem::replace(item, v)) } else { self.table - .insert(hash, (k, v), make_hasher::(&self.hash_builder)); + .insert(hash, (k, v), make_hasher::<_, V, S>(&self.hash_builder)); None } } @@ -1736,7 +1717,7 @@ where let hash = make_insert_hash::(&self.hash_builder, &k); let bucket = self .table - .insert(hash, (k, v), make_hasher::(&self.hash_builder)); + .insert(hash, (k, v), make_hasher::<_, V, S>(&self.hash_builder)); let (k_ref, v_ref) = unsafe { bucket.as_mut() }; (k_ref, v_ref) } @@ -1812,8 +1793,7 @@ where #[cfg_attr(feature = "inline-more", inline)] pub fn remove(&mut self, k: &Q) -> Option where - K: Borrow, - Q: Hash + Eq, + Q: Hash + Equivalent, { // Avoid `Option::map` because it bloats LLVM IR. match self.remove_entry(k) { @@ -1853,10 +1833,9 @@ where #[cfg_attr(feature = "inline-more", inline)] pub fn remove_entry(&mut self, k: &Q) -> Option<(K, V)> where - K: Borrow, - Q: Hash + Eq, + Q: Hash + Equivalent, { - let hash = make_hash::(&self.hash_builder, k); + let hash = make_hash::(&self.hash_builder, k); self.table.remove_entry(hash, equivalent_key(k)) } } @@ -2140,8 +2119,8 @@ where impl Index<&Q> for HashMap where - K: Eq + Hash + Borrow, - Q: Eq + Hash, + K: Eq + Hash, + Q: Hash + Equivalent, S: BuildHasher, A: Allocator + Clone, { @@ -3103,10 +3082,9 @@ impl<'a, K, V, S, A: Allocator + Clone> RawEntryBuilderMut<'a, K, V, S, A> { pub fn from_key(self, k: &Q) -> RawEntryMut<'a, K, V, S, A> where S: BuildHasher, - K: Borrow, - Q: Hash + Eq, + Q: Hash + Equivalent, { - let hash = make_hash::(&self.map.hash_builder, k); + let hash = make_hash::(&self.map.hash_builder, k); self.from_key_hashed_nocheck(hash, k) } @@ -3136,8 +3114,7 @@ impl<'a, K, V, S, A: Allocator + Clone> RawEntryBuilderMut<'a, K, V, S, A> { #[allow(clippy::wrong_self_convention)] pub fn from_key_hashed_nocheck(self, hash: u64, k: &Q) -> RawEntryMut<'a, K, V, S, A> where - K: Borrow, - Q: Eq, + Q: Equivalent, { self.from_hash(hash, equivalent(k)) } @@ -3211,10 +3188,9 @@ impl<'a, K, V, S, A: Allocator + Clone> RawEntryBuilder<'a, K, V, S, A> { pub fn from_key(self, k: &Q) -> Option<(&'a K, &'a V)> where S: BuildHasher, - K: Borrow, - Q: Hash + Eq, + Q: Hash + Equivalent, { - let hash = make_hash::(&self.map.hash_builder, k); + let hash = make_hash::(&self.map.hash_builder, k); self.from_key_hashed_nocheck(hash, k) } @@ -3242,8 +3218,7 @@ impl<'a, K, V, S, A: Allocator + Clone> RawEntryBuilder<'a, K, V, S, A> { #[allow(clippy::wrong_self_convention)] pub fn from_key_hashed_nocheck(self, hash: u64, k: &Q) -> Option<(&'a K, &'a V)> where - K: Borrow, - Q: Eq, + Q: Equivalent, { self.from_hash(hash, equivalent(k)) } @@ -3950,7 +3925,7 @@ impl<'a, K, V, S, A: Allocator + Clone> RawVacantEntryMut<'a, K, V, S, A> { let &mut (ref mut k, ref mut v) = self.table.insert_entry( hash, (key, value), - make_hasher::(self.hash_builder), + make_hasher::<_, V, S>(self.hash_builder), ); (k, v) } @@ -4018,7 +3993,7 @@ impl<'a, K, V, S, A: Allocator + Clone> RawVacantEntryMut<'a, K, V, S, A> { let elem = self.table.insert( hash, (key, value), - make_hasher::(self.hash_builder), + make_hasher::<_, V, S>(self.hash_builder), ); RawOccupiedEntryMut { elem, @@ -5567,7 +5542,7 @@ impl<'a, K, V, S, A: Allocator + Clone> VacantEntry<'a, K, V, S, A> { let entry = table.insert_entry( self.hash, (self.key, value), - make_hasher::(&self.table.hash_builder), + make_hasher::<_, V, S>(&self.table.hash_builder), ); &mut entry.1 } @@ -5581,7 +5556,7 @@ impl<'a, K, V, S, A: Allocator + Clone> VacantEntry<'a, K, V, S, A> { let elem = self.table.table.insert( self.hash, (self.key, value), - make_hasher::(&self.table.hash_builder), + make_hasher::<_, V, S>(&self.table.hash_builder), ); OccupiedEntry { hash: self.hash, @@ -6305,7 +6280,7 @@ impl<'a, 'b, K, Q: ?Sized, V, S, A: Allocator + Clone> VacantEntryRef<'a, 'b, K, let entry = table.insert_entry( self.hash, (self.key.into_owned(), value), - make_hasher::(&self.table.hash_builder), + make_hasher::<_, V, S>(&self.table.hash_builder), ); &mut entry.1 } @@ -6319,7 +6294,7 @@ impl<'a, 'b, K, Q: ?Sized, V, S, A: Allocator + Clone> VacantEntryRef<'a, 'b, K, let elem = self.table.table.insert( self.hash, (self.key.into_owned(), value), - make_hasher::(&self.table.hash_builder), + make_hasher::<_, V, S>(&self.table.hash_builder), ); OccupiedEntryRef { hash: self.hash, From 49f5c8bc7ca91228b5c9a57bc313088d593a1818 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timoth=C3=A9e=20Haudebourg?= Date: Fri, 22 Jul 2022 13:58:49 +0200 Subject: [PATCH 3/6] Use `Equivalent` trait in `HashSet`. --- src/set.rs | 21 +++++++-------------- 1 file changed, 7 insertions(+), 14 deletions(-) diff --git a/src/set.rs b/src/set.rs index 2a4dcea52c..c3e14affe1 100644 --- a/src/set.rs +++ b/src/set.rs @@ -1,6 +1,5 @@ -use crate::TryReserveError; +use crate::{Equivalent, TryReserveError}; use alloc::borrow::ToOwned; -use core::borrow::Borrow; use core::fmt; use core::hash::{BuildHasher, Hash}; use core::iter::{Chain, FromIterator, FusedIterator}; @@ -773,8 +772,7 @@ where #[cfg_attr(feature = "inline-more", inline)] pub fn contains(&self, value: &Q) -> bool where - T: Borrow, - Q: Hash + Eq, + Q: Hash + Equivalent, { self.map.contains_key(value) } @@ -800,8 +798,7 @@ where #[cfg_attr(feature = "inline-more", inline)] pub fn get(&self, value: &Q) -> Option<&T> where - T: Borrow, - Q: Hash + Eq, + Q: Hash + Equivalent, { // Avoid `Option::map` because it bloats LLVM IR. match self.map.get_key_value(value) { @@ -856,8 +853,7 @@ where #[inline] pub fn get_or_insert_owned(&mut self, value: &Q) -> &T where - T: Borrow, - Q: Hash + Eq + ToOwned, + Q: Hash + Equivalent + ToOwned, { // Although the raw entry gives us `&mut T`, we only return `&T` to be consistent with // `get`. Key mutation is "raw" because you're not supposed to affect `Eq` or `Hash`. @@ -889,8 +885,7 @@ where #[cfg_attr(feature = "inline-more", inline)] pub fn get_or_insert_with(&mut self, value: &Q, f: F) -> &T where - T: Borrow, - Q: Hash + Eq, + Q: Hash + Equivalent, F: FnOnce(&Q) -> T, { // Although the raw entry gives us `&mut T`, we only return `&T` to be consistent with @@ -1106,8 +1101,7 @@ where #[cfg_attr(feature = "inline-more", inline)] pub fn remove(&mut self, value: &Q) -> bool where - T: Borrow, - Q: Hash + Eq, + Q: Hash + Equivalent, { self.map.remove(value).is_some() } @@ -1133,8 +1127,7 @@ where #[cfg_attr(feature = "inline-more", inline)] pub fn take(&mut self, value: &Q) -> Option where - T: Borrow, - Q: Hash + Eq, + Q: Hash + Equivalent, { // Avoid `Option::map` because it bloats LLVM IR. match self.map.remove_entry(value) { From a115a7d6d421e88aec3fee6d29167d52afd4243c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timoth=C3=A9e=20Haudebourg?= Date: Fri, 22 Jul 2022 14:11:36 +0200 Subject: [PATCH 4/6] Fix `test_into_iter_refresh` & `make_hash`. --- src/map.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/map.rs b/src/map.rs index 020e7ee895..6a1e18c23b 100644 --- a/src/map.rs +++ b/src/map.rs @@ -252,9 +252,8 @@ where #[cfg(feature = "nightly")] #[cfg_attr(feature = "inline-more", inline)] -pub(crate) fn make_hash(hash_builder: &S, val: &Q) -> u64 +pub(crate) fn make_hash(hash_builder: &S, val: &Q) -> u64 where - K: Borrow, Q: Hash + ?Sized, S: BuildHasher, { @@ -8255,7 +8254,7 @@ mod test_map { let e = map.table.insert( hash_value, (i, 2 * i), - super::make_hasher::(&hash_builder), + super::make_hasher::<_, usize, _>(&hash_builder), ); it.reflect_insert(&e); if let Some(p) = removed.iter().position(|e| e == &(i, 2 * i)) { From 9fd585606d1ce39fa39c32ea008f880650526456 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timoth=C3=A9e=20Haudebourg?= Date: Fri, 22 Jul 2022 14:17:24 +0200 Subject: [PATCH 5/6] Fix typos in `Equivalent` documentation. --- src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 659aa325e2..e43165dd63 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -120,7 +120,7 @@ pub use crate::set::HashSet; /// Key equivalence trait. /// /// This trait defines the function used to compare the input value with the -/// map keys (or set values) during a lookup operation sur as [`HashMap::get`] +/// map keys (or set values) during a lookup operation such as [`HashMap::get`] /// or [`HashSet::contains`]. /// It is provided with a blanket implementation based on the /// [`Borrow`](core::borrow::Borrow) trait. @@ -131,7 +131,7 @@ pub use crate::set::HashSet; pub trait Equivalent { /// Checks if this value is equivalent to the given key. /// - /// Returns `true` if oth values are equivalent, and `false` otherwise. + /// Returns `true` if both values are equivalent, and `false` otherwise. /// /// # Correctness /// From f9df82035a780d257f760b9baea08a383139b2f7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timoth=C3=A9e=20Haudebourg?= Date: Thu, 1 Sep 2022 12:06:58 +0200 Subject: [PATCH 6/6] Added tests for the `Equivalent` trait. --- tests/equivalent_trait.rs | 53 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) create mode 100644 tests/equivalent_trait.rs diff --git a/tests/equivalent_trait.rs b/tests/equivalent_trait.rs new file mode 100644 index 0000000000..713dddd53c --- /dev/null +++ b/tests/equivalent_trait.rs @@ -0,0 +1,53 @@ +use hashbrown::Equivalent; +use hashbrown::HashMap; + +use std::hash::Hash; + +#[derive(Debug, Hash)] +pub struct Pair(pub A, pub B); + +impl PartialEq<(A, B)> for Pair +where + C: PartialEq, + D: PartialEq, +{ + fn eq(&self, rhs: &(A, B)) -> bool { + self.0 == rhs.0 && self.1 == rhs.1 + } +} + +impl Equivalent for Pair +where + Pair: PartialEq, + A: Hash + Eq, + B: Hash + Eq, +{ + fn equivalent(&self, other: &X) -> bool { + *self == *other + } +} + +#[test] +fn test_lookup() { + let s = String::from; + let mut map = HashMap::new(); + map.insert((s("a"), s("b")), 1); + map.insert((s("a"), s("x")), 2); + + assert!(map.contains_key(&Pair("a", "b"))); + assert!(!map.contains_key(&Pair("b", "a"))); +} + +#[test] +fn test_string_str() { + let s = String::from; + let mut map = HashMap::new(); + map.insert(s("a"), 1); + map.insert(s("b"), 2); + map.insert(s("x"), 3); + map.insert(s("y"), 4); + + assert!(map.contains_key("a")); + assert!(!map.contains_key("z")); + assert_eq!(map.remove("b"), Some(2)); +}