From c1ee6ee337f33a9e1eb8002eeadbf95add5fa3c4 Mon Sep 17 00:00:00 2001 From: Urgau Date: Wed, 25 Sep 2024 11:02:30 +0200 Subject: [PATCH] Change signature of `get_many_mut` APIs by 1) panicking on overlapping keys and 2) returning an array of Option rather than an Option of array. And address a potential future UB by only using raw pointers rust-lang/unsafe-code-guidelines#346 a --- src/map.rs | 150 ++++++++++++++++++++++++++++++++----------------- src/raw/mod.rs | 45 ++++++++------- src/table.rs | 53 +++++++++++------ 3 files changed, 157 insertions(+), 91 deletions(-) diff --git a/src/map.rs b/src/map.rs index 1e794ca4f..1c2cc4458 100644 --- a/src/map.rs +++ b/src/map.rs @@ -1467,8 +1467,11 @@ where /// Attempts to get mutable references to `N` values in the map at once. /// /// Returns an array of length `N` with the results of each query. For soundness, at most one - /// mutable reference will be returned to any value. `None` will be returned if any of the - /// keys are duplicates or missing. + /// mutable reference will be returned to any value. `None` will be used if the key is missing. + /// + /// # Panics + /// + /// Panics if any keys are overlapping. /// /// # Examples /// @@ -1481,33 +1484,46 @@ where /// libraries.insert("Herzogin-Anna-Amalia-Bibliothek".to_string(), 1691); /// libraries.insert("Library of Congress".to_string(), 1800); /// + /// // Get Athenæum and Bodleian Library + /// let [Some(a), Some(b)] = libraries.get_many_mut([ + /// "Athenæum", + /// "Bodleian Library", + /// ]) else { panic!() }; + /// + /// // Assert values of Athenæum and Library of Congress /// let got = libraries.get_many_mut([ /// "Athenæum", /// "Library of Congress", /// ]); /// assert_eq!( /// got, - /// Some([ - /// &mut 1807, - /// &mut 1800, - /// ]), + /// [ + /// Some(&mut 1807), + /// Some(&mut 1800), + /// ], /// ); /// /// // Missing keys result in None /// let got = libraries.get_many_mut([ - /// "Athenæum", + /// "Athenæum1", /// "New York Public Library", /// ]); - /// assert_eq!(got, None); + /// assert_eq!(got, [None, None]); + /// ``` + /// + /// ```should_panic + /// use hashbrown::HashMap; + /// + /// let mut libraries = HashMap::new(); + /// libraries.insert("Athenæum".to_string(), 1807); /// - /// // Duplicate keys result in None + /// // Duplicate keys panic! /// let got = libraries.get_many_mut([ /// "Athenæum", /// "Athenæum", /// ]); - /// assert_eq!(got, None); /// ``` - pub fn get_many_mut(&mut self, ks: [&Q; N]) -> Option<[&'_ mut V; N]> + pub fn get_many_mut(&mut self, ks: [&Q; N]) -> [Option<&'_ mut V>; N] where Q: Hash + Equivalent + ?Sized, { @@ -1517,8 +1533,8 @@ where /// Attempts to get mutable references to `N` values in the map at once, without validating that /// the values are unique. /// - /// Returns an array of length `N` with the results of each query. `None` will be returned if - /// any of the keys are missing. + /// Returns an array of length `N` with the results of each query. `None` will be used if + /// the key is missing. /// /// For a safe alternative see [`get_many_mut`](`HashMap::get_many_mut`). /// @@ -1540,29 +1556,37 @@ where /// libraries.insert("Herzogin-Anna-Amalia-Bibliothek".to_string(), 1691); /// libraries.insert("Library of Congress".to_string(), 1800); /// - /// let got = libraries.get_many_mut([ + /// // SAFETY: The keys do not overlap. + /// let [Some(a), Some(b)] = (unsafe { libraries.get_many_unchecked_mut([ + /// "Athenæum", + /// "Bodleian Library", + /// ]) }) else { panic!() }; + /// + /// // SAFETY: The keys do not overlap. + /// let got = unsafe { libraries.get_many_unchecked_mut([ /// "Athenæum", /// "Library of Congress", - /// ]); + /// ]) }; /// assert_eq!( /// got, - /// Some([ - /// &mut 1807, - /// &mut 1800, - /// ]), + /// [ + /// Some(&mut 1807), + /// Some(&mut 1800), + /// ], /// ); /// - /// // Missing keys result in None - /// let got = libraries.get_many_mut([ + /// // SAFETY: The keys do not overlap. + /// let got = unsafe { libraries.get_many_unchecked_mut([ /// "Athenæum", /// "New York Public Library", - /// ]); - /// assert_eq!(got, None); + /// ]) }; + /// // Missing keys result in None + /// assert_eq!(got, [Some(&mut 1807), None]); /// ``` pub unsafe fn get_many_unchecked_mut( &mut self, ks: [&Q; N], - ) -> Option<[&'_ mut V; N]> + ) -> [Option<&'_ mut V>; N] where Q: Hash + Equivalent + ?Sized, { @@ -1574,8 +1598,11 @@ where /// references to the corresponding keys. /// /// Returns an array of length `N` with the results of each query. For soundness, at most one - /// mutable reference will be returned to any value. `None` will be returned if any of the keys - /// are duplicates or missing. + /// mutable reference will be returned to any value. `None` will be used if the key is missing. + /// + /// # Panics + /// + /// Panics if any keys are overlapping. /// /// # Examples /// @@ -1594,30 +1621,37 @@ where /// ]); /// assert_eq!( /// got, - /// Some([ - /// (&"Bodleian Library".to_string(), &mut 1602), - /// (&"Herzogin-Anna-Amalia-Bibliothek".to_string(), &mut 1691), - /// ]), + /// [ + /// Some((&"Bodleian Library".to_string(), &mut 1602)), + /// Some((&"Herzogin-Anna-Amalia-Bibliothek".to_string(), &mut 1691)), + /// ], /// ); /// // Missing keys result in None /// let got = libraries.get_many_key_value_mut([ /// "Bodleian Library", /// "Gewandhaus", /// ]); - /// assert_eq!(got, None); + /// assert_eq!(got, [Some((&"Bodleian Library".to_string(), &mut 1602)), None]); + /// ``` /// - /// // Duplicate keys result in None + /// ```should_panic + /// use hashbrown::HashMap; + /// + /// let mut libraries = HashMap::new(); + /// libraries.insert("Bodleian Library".to_string(), 1602); + /// libraries.insert("Herzogin-Anna-Amalia-Bibliothek".to_string(), 1691); + /// + /// // Duplicate keys result in panic! /// let got = libraries.get_many_key_value_mut([ /// "Bodleian Library", /// "Herzogin-Anna-Amalia-Bibliothek", /// "Herzogin-Anna-Amalia-Bibliothek", /// ]); - /// assert_eq!(got, None); /// ``` pub fn get_many_key_value_mut( &mut self, ks: [&Q; N], - ) -> Option<[(&'_ K, &'_ mut V); N]> + ) -> [Option<(&'_ K, &'_ mut V)>; N] where Q: Hash + Equivalent + ?Sized, { @@ -1657,22 +1691,28 @@ where /// ]); /// assert_eq!( /// got, - /// Some([ - /// (&"Bodleian Library".to_string(), &mut 1602), - /// (&"Herzogin-Anna-Amalia-Bibliothek".to_string(), &mut 1691), - /// ]), + /// [ + /// Some((&"Bodleian Library".to_string(), &mut 1602)), + /// Some((&"Herzogin-Anna-Amalia-Bibliothek".to_string(), &mut 1691)), + /// ], /// ); /// // Missing keys result in None /// let got = libraries.get_many_key_value_mut([ /// "Bodleian Library", /// "Gewandhaus", /// ]); - /// assert_eq!(got, None); + /// assert_eq!( + /// got, + /// [ + /// Some((&"Bodleian Library".to_string(), &mut 1602)), + /// None, + /// ], + /// ); /// ``` pub unsafe fn get_many_key_value_unchecked_mut( &mut self, ks: [&Q; N], - ) -> Option<[(&'_ K, &'_ mut V); N]> + ) -> [Option<(&'_ K, &'_ mut V)>; N] where Q: Hash + Equivalent + ?Sized, { @@ -1680,7 +1720,7 @@ where .map(|res| res.map(|(k, v)| (&*k, v))) } - fn get_many_mut_inner(&mut self, ks: [&Q; N]) -> Option<[&'_ mut (K, V); N]> + fn get_many_mut_inner(&mut self, ks: [&Q; N]) -> [Option<&'_ mut (K, V)>; N] where Q: Hash + Equivalent + ?Sized, { @@ -1692,7 +1732,7 @@ where unsafe fn get_many_unchecked_mut_inner( &mut self, ks: [&Q; N], - ) -> Option<[&'_ mut (K, V); N]> + ) -> [Option<&'_ mut (K, V)>; N] where Q: Hash + Equivalent + ?Sized, { @@ -5937,7 +5977,7 @@ mod test_map { } #[test] - fn test_get_each_mut() { + fn test_get_many_mut() { let mut map = HashMap::new(); map.insert("foo".to_owned(), 0); map.insert("bar".to_owned(), 10); @@ -5945,25 +5985,31 @@ mod test_map { map.insert("qux".to_owned(), 30); let xs = map.get_many_mut(["foo", "qux"]); - assert_eq!(xs, Some([&mut 0, &mut 30])); + assert_eq!(xs, [Some(&mut 0), Some(&mut 30)]); let xs = map.get_many_mut(["foo", "dud"]); - assert_eq!(xs, None); - - let xs = map.get_many_mut(["foo", "foo"]); - assert_eq!(xs, None); + assert_eq!(xs, [Some(&mut 0), None]); let ys = map.get_many_key_value_mut(["bar", "baz"]); assert_eq!( ys, - Some([(&"bar".to_owned(), &mut 10), (&"baz".to_owned(), &mut 20),]), + [ + Some((&"bar".to_owned(), &mut 10)), + Some((&"baz".to_owned(), &mut 20)) + ], ); let ys = map.get_many_key_value_mut(["bar", "dip"]); - assert_eq!(ys, None); + assert_eq!(ys, [Some((&"bar".to_string(), &mut 10)), None]); + } + + #[test] + #[should_panic = "duplicate keys found"] + fn test_get_many_mut_duplicate() { + let mut map = HashMap::new(); + map.insert("foo".to_owned(), 0); - let ys = map.get_many_key_value_mut(["baz", "baz"]); - assert_eq!(ys, None); + let _xs = map.get_many_mut(["foo", "foo"]); } #[test] diff --git a/src/raw/mod.rs b/src/raw/mod.rs index 2c6392181..495557ad4 100644 --- a/src/raw/mod.rs +++ b/src/raw/mod.rs @@ -1,10 +1,10 @@ use crate::alloc::alloc::{handle_alloc_error, Layout}; use crate::scopeguard::{guard, ScopeGuard}; use crate::TryReserveError; +use core::array; use core::iter::FusedIterator; use core::marker::PhantomData; use core::mem; -use core::mem::MaybeUninit; use core::ptr::NonNull; use core::{hint, ptr}; @@ -484,6 +484,13 @@ impl Bucket { } } + /// Acquires the underlying non-null pointer `*mut T` to `data`. + #[inline] + fn as_non_null(&self) -> NonNull { + // SAFETY: `self.ptr` is already a `NonNull` + unsafe { NonNull::new_unchecked(self.as_ptr()) } + } + /// Create a new [`Bucket`] that is offset from the `self` by the given /// `offset`. The pointer calculation is performed by calculating the /// offset from `self` pointer (convenience for `self.ptr.as_ptr().sub(offset)`). @@ -1291,20 +1298,19 @@ impl RawTable { &mut self, hashes: [u64; N], eq: impl FnMut(usize, &T) -> bool, - ) -> Option<[&'_ mut T; N]> { + ) -> [Option<&'_ mut T>; N] { unsafe { - let ptrs = self.get_many_mut_pointers(hashes, eq)?; + let ptrs = self.get_many_mut_pointers(hashes, eq); - for (i, &cur) in ptrs.iter().enumerate() { - if ptrs[..i].iter().any(|&prev| ptr::eq::(prev, cur)) { - return None; + for (i, cur) in ptrs.iter().enumerate() { + if cur.is_some() && ptrs[..i].contains(cur) { + panic!("duplicate keys found"); } } // All bucket are distinct from all previous buckets so we're clear to return the result // of the lookup. - // TODO use `MaybeUninit::array_assume_init` here instead once that's stable. - Some(mem::transmute_copy(&ptrs)) + ptrs.map(|ptr| ptr.map(|mut ptr| ptr.as_mut())) } } @@ -1312,27 +1318,20 @@ impl RawTable { &mut self, hashes: [u64; N], eq: impl FnMut(usize, &T) -> bool, - ) -> Option<[&'_ mut T; N]> { - let ptrs = self.get_many_mut_pointers(hashes, eq)?; - Some(mem::transmute_copy(&ptrs)) + ) -> [Option<&'_ mut T>; N] { + let ptrs = self.get_many_mut_pointers(hashes, eq); + ptrs.map(|ptr| ptr.map(|mut ptr| ptr.as_mut())) } unsafe fn get_many_mut_pointers( &mut self, hashes: [u64; N], mut eq: impl FnMut(usize, &T) -> bool, - ) -> Option<[*mut T; N]> { - // TODO use `MaybeUninit::uninit_array` here instead once that's stable. - let mut outs: MaybeUninit<[*mut T; N]> = MaybeUninit::uninit(); - let outs_ptr = outs.as_mut_ptr(); - - for (i, &hash) in hashes.iter().enumerate() { - let cur = self.find(hash, |k| eq(i, k))?; - *(*outs_ptr).get_unchecked_mut(i) = cur.as_mut(); - } - - // TODO use `MaybeUninit::array_assume_init` here instead once that's stable. - Some(outs.assume_init()) + ) -> [Option>; N] { + array::from_fn(|i| { + self.find(hashes[i], |k| eq(i, k)) + .map(|cur| cur.as_non_null()) + }) } /// Returns the number of elements the map can hold without reallocating. diff --git a/src/table.rs b/src/table.rs index 8f9530404..d442a0149 100644 --- a/src/table.rs +++ b/src/table.rs @@ -966,8 +966,11 @@ where /// the `i`th key to be looked up. /// /// Returns an array of length `N` with the results of each query. For soundness, at most one - /// mutable reference will be returned to any value. `None` will be returned if any of the - /// keys are duplicates or missing. + /// mutable reference will be returned to any value. `None` will be used if the key is missing. + /// + /// # Panics + /// + /// Panics if any keys are overlapping. /// /// # Examples /// @@ -994,29 +997,52 @@ where /// let got = libraries.get_many_mut(keys.map(|k| hasher(&k)), |i, val| keys[i] == val.0); /// assert_eq!( /// got, - /// Some([&mut ("Athenæum", 1807), &mut ("Library of Congress", 1800),]), + /// [Some(&mut ("Athenæum", 1807)), Some(&mut ("Library of Congress", 1800))], /// ); /// /// // Missing keys result in None /// let keys = ["Athenæum", "New York Public Library"]; /// let got = libraries.get_many_mut(keys.map(|k| hasher(&k)), |i, val| keys[i] == val.0); - /// assert_eq!(got, None); + /// assert_eq!(got, [Some(&mut ("Athenæum", 1807)), None]); + /// # } + /// # fn main() { + /// # #[cfg(feature = "nightly")] + /// # test() + /// # } + /// ``` + /// + /// ```should_panic + /// # #[cfg(feature = "nightly")] + /// # fn test() { + /// # use hashbrown::{HashTable, DefaultHashBuilder}; + /// # use std::hash::BuildHasher; + /// + /// let mut libraries: HashTable<(&str, u32)> = HashTable::new(); + /// let hasher = DefaultHashBuilder::default(); + /// let hasher = |val: &_| hasher.hash_one(val); + /// for (k, v) in [ + /// ("Athenæum", 1807), + /// ("Library of Congress", 1800), + /// ] { + /// libraries.insert_unique(hasher(&k), (k, v), |(k, _)| hasher(&k)); + /// } /// - /// // Duplicate keys result in None + /// // Duplicate keys result in a panic! /// let keys = ["Athenæum", "Athenæum"]; /// let got = libraries.get_many_mut(keys.map(|k| hasher(&k)), |i, val| keys[i] == val.0); - /// assert_eq!(got, None); /// # } /// # fn main() { /// # #[cfg(feature = "nightly")] - /// # test() + /// # test(); + /// # #[cfg(not(feature = "nightly"))] + /// # panic!(); /// # } /// ``` pub fn get_many_mut( &mut self, hashes: [u64; N], eq: impl FnMut(usize, &T) -> bool, - ) -> Option<[&'_ mut T; N]> { + ) -> [Option<&'_ mut T>; N] { self.raw.get_many_mut(hashes, eq) } @@ -1063,18 +1089,13 @@ where /// let got = libraries.get_many_mut(keys.map(|k| hasher(&k)), |i, val| keys[i] == val.0); /// assert_eq!( /// got, - /// Some([&mut ("Athenæum", 1807), &mut ("Library of Congress", 1800),]), + /// [Some(&mut ("Athenæum", 1807)), Some(&mut ("Library of Congress", 1800))], /// ); /// /// // Missing keys result in None /// let keys = ["Athenæum", "New York Public Library"]; /// let got = libraries.get_many_mut(keys.map(|k| hasher(&k)), |i, val| keys[i] == val.0); - /// assert_eq!(got, None); - /// - /// // Duplicate keys result in None - /// let keys = ["Athenæum", "Athenæum"]; - /// let got = libraries.get_many_mut(keys.map(|k| hasher(&k)), |i, val| keys[i] == val.0); - /// assert_eq!(got, None); + /// assert_eq!(got, [Some(&mut ("Athenæum", 1807)), None]); /// # } /// # fn main() { /// # #[cfg(feature = "nightly")] @@ -1085,7 +1106,7 @@ where &mut self, hashes: [u64; N], eq: impl FnMut(usize, &T) -> bool, - ) -> Option<[&'_ mut T; N]> { + ) -> [Option<&'_ mut T>; N] { self.raw.get_many_unchecked_mut(hashes, eq) }