From fb5721774956af03aaa95e07b82e8096a012fcf4 Mon Sep 17 00:00:00 2001 From: Amanieu d'Antras Date: Thu, 12 Sep 2024 07:45:03 -0400 Subject: [PATCH 1/2] Make `insert_unique_unchecked` unsafe This is in line with the standard library guarantees that it should be impossible to create an inconsistent `HashMap` with well-defined key types. --- benches/insert_unique_unchecked.rs | 4 +++- src/map.rs | 32 +++++++++++++++++++----------- src/set.rs | 24 ++++++++++++---------- 3 files changed, 37 insertions(+), 23 deletions(-) diff --git a/benches/insert_unique_unchecked.rs b/benches/insert_unique_unchecked.rs index 857ad18e57..cfd69cdb7d 100644 --- a/benches/insert_unique_unchecked.rs +++ b/benches/insert_unique_unchecked.rs @@ -25,7 +25,9 @@ fn insert_unique_unchecked(b: &mut Bencher) { b.iter(|| { let mut m = HashMap::with_capacity(1000); for k in &keys { - m.insert_unique_unchecked(k, k); + unsafe { + m.insert_unique_unchecked(k, k); + } } m }); diff --git a/src/map.rs b/src/map.rs index 7bfd4b20e2..264d500f34 100644 --- a/src/map.rs +++ b/src/map.rs @@ -1761,8 +1761,17 @@ where /// Insert a key-value pair into the map without checking /// if the key already exists in the map. /// + /// This operation is faster than regular insert, because it does not perform + /// lookup before insertion. + /// + /// This operation is useful during initial population of the map. + /// For example, when constructing a map from another map, we know + /// that keys are unique. + /// /// Returns a reference to the key and value just inserted. /// + /// # Safety + /// /// This operation is safe if a key does not exist in the map. /// /// However, if a key exists in the map already, the behavior is unspecified: @@ -1772,12 +1781,9 @@ where /// That said, this operation (and following operations) are guaranteed to /// not violate memory safety. /// - /// This operation is faster than regular insert, because it does not perform - /// lookup before insertion. - /// - /// This operation is useful during initial population of the map. - /// For example, when constructing a map from another map, we know - /// that keys are unique. + /// However this operation is still unsafe because the resulting `HashMap` + /// may be passed to unsafe code which does expect the map to behave + /// correctly, and would could unsoundness as a result. /// /// # Examples /// @@ -1793,10 +1799,12 @@ where /// let mut map2 = HashMap::new(); /// /// for (key, value) in map1.into_iter() { - /// map2.insert_unique_unchecked(key, value); + /// unsafe { + /// map2.insert_unique_unchecked(key, value); + /// } /// } /// - /// let (key, value) = map2.insert_unique_unchecked(4, "d"); + /// let (key, value) = unsafe { map2.insert_unique_unchecked(4, "d") }; /// assert_eq!(key, &4); /// assert_eq!(value, &mut "d"); /// *value = "e"; @@ -1808,7 +1816,7 @@ where /// assert_eq!(map2.len(), 4); /// ``` #[cfg_attr(feature = "inline-more", inline)] - pub fn insert_unique_unchecked(&mut self, k: K, v: V) -> (&K, &mut V) { + pub unsafe fn insert_unique_unchecked(&mut self, k: K, v: V) -> (&K, &mut V) { let hash = make_hash::(&self.hash_builder, &k); let bucket = self .table @@ -3187,7 +3195,7 @@ impl<'a, K, V, S, A: Allocator> IntoIterator for &'a HashMap { /// /// for (key, value) in &map_one { /// println!("Key: {}, Value: {}", key, value); - /// map_two.insert_unique_unchecked(*key, *value); + /// map_two.insert(*key, *value); /// } /// /// assert_eq!(map_one, map_two); @@ -5710,9 +5718,9 @@ mod test_map { #[test] fn test_insert_unique_unchecked() { let mut map = HashMap::new(); - let (k1, v1) = map.insert_unique_unchecked(10, 11); + let (k1, v1) = unsafe { map.insert_unique_unchecked(10, 11) }; assert_eq!((&10, &mut 11), (k1, v1)); - let (k2, v2) = map.insert_unique_unchecked(20, 21); + let (k2, v2) = unsafe { map.insert_unique_unchecked(20, 21) }; assert_eq!((&20, &mut 21), (k2, v2)); assert_eq!(Some(&11), map.get(&10)); assert_eq!(Some(&21), map.get(&20)); diff --git a/src/set.rs b/src/set.rs index 6baa2d63ae..3c59076b1a 100644 --- a/src/set.rs +++ b/src/set.rs @@ -1118,25 +1118,29 @@ where /// Insert a value the set without checking if the value already exists in the set. /// - /// Returns a reference to the value just inserted. + /// This operation is faster than regular insert, because it does not perform + /// lookup before insertion. + /// + /// This operation is useful during initial population of the set. + /// For example, when constructing a set from another set, we know + /// that values are unique. + /// + /// # Safety /// - /// This operation is safe if a value does not exist in the set. + /// This operation is safe if a key does not exist in the set. /// - /// However, if a value exists in the set already, the behavior is unspecified: + /// However, if a key exists in the set already, the behavior is unspecified: /// this operation may panic, loop forever, or any following operation with the set /// may panic, loop forever or return arbitrary result. /// /// That said, this operation (and following operations) are guaranteed to /// not violate memory safety. /// - /// This operation is faster than regular insert, because it does not perform - /// lookup before insertion. - /// - /// This operation is useful during initial population of the set. - /// For example, when constructing a set from another set, we know - /// that values are unique. + /// However this operation is still unsafe because the resulting `HashSet` + /// may be passed to unsafe code which does expect the set to behave + /// correctly, and would could unsoundness as a result. #[cfg_attr(feature = "inline-more", inline)] - pub fn insert_unique_unchecked(&mut self, value: T) -> &T { + pub unsafe fn insert_unique_unchecked(&mut self, value: T) -> &T { self.map.insert_unique_unchecked(value, ()).0 } From e774b7db0299d0f8e92a11461a8553d9931df351 Mon Sep 17 00:00:00 2001 From: Amanieu d'Antras Date: Fri, 20 Sep 2024 12:09:01 +0100 Subject: [PATCH 2/2] Address review comments --- src/map.rs | 2 +- src/set.rs | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/map.rs b/src/map.rs index 264d500f34..69355921ae 100644 --- a/src/map.rs +++ b/src/map.rs @@ -1783,7 +1783,7 @@ where /// /// However this operation is still unsafe because the resulting `HashMap` /// may be passed to unsafe code which does expect the map to behave - /// correctly, and would could unsoundness as a result. + /// correctly, and would cause unsoundness as a result. /// /// # Examples /// diff --git a/src/set.rs b/src/set.rs index 3c59076b1a..2493644555 100644 --- a/src/set.rs +++ b/src/set.rs @@ -1127,9 +1127,9 @@ where /// /// # Safety /// - /// This operation is safe if a key does not exist in the set. + /// This operation is safe if a value does not exist in the set. /// - /// However, if a key exists in the set already, the behavior is unspecified: + /// However, if a value exists in the set already, the behavior is unspecified: /// this operation may panic, loop forever, or any following operation with the set /// may panic, loop forever or return arbitrary result. /// @@ -1138,7 +1138,7 @@ where /// /// However this operation is still unsafe because the resulting `HashSet` /// may be passed to unsafe code which does expect the set to behave - /// correctly, and would could unsoundness as a result. + /// correctly, and would cause unsoundness as a result. #[cfg_attr(feature = "inline-more", inline)] pub unsafe fn insert_unique_unchecked(&mut self, value: T) -> &T { self.map.insert_unique_unchecked(value, ()).0