Add safety comments

agb-hashmap/src/lib.rs

@@ -15,6 +15,8 @@
 #![deny(rustdoc::private_intra_doc_links)]
 #![deny(rustdoc::invalid_html_tags)]
 #![deny(unreachable_pub)]
+#![deny(clippy::missing_safety_doc)]
+#![deny(clippy::undocumented_unsafe_blocks)]
 
 extern crate alloc;
 
@@ -352,11 +354,14 @@ where
         let hash = self.hash(key);
 
         let location = self.nodes.location(key, hash)?;
-        Some(unsafe {
+        Some(
-            self.nodes
+            // SAFETY: we know that a node exists and has a value from the location call above
-                .node_at_unchecked(location)
+            unsafe {
-                .key_value_ref_unchecked()
+                self.nodes
-        })
+                    .node_at_unchecked(location)
+                    .key_value_ref_unchecked()
+            },
+        )
     }
 
     /// Returns a reference to the value corresponding to the key. Returns [`None`] if there is
@@ -403,11 +408,14 @@ where
         let hash = self.hash(key);
 
         let location = self.nodes.location(key, hash)?;
-        Some(unsafe {
+        Some(
-            self.nodes
+            // SAFETY: we know that a node exists and has a value from the location call above
-                .node_at_unchecked_mut(location)
+            unsafe {
-                .value_mut_unchecked()
+                self.nodes
-        })
+                    .node_at_unchecked_mut(location)
+                    .value_mut_unchecked()
+            },
+        )
     }
 
     /// Removes the given key from the map. Returns the current value if it existed, or [`None`]
@@ -561,6 +569,15 @@ pub struct OccupiedEntry<'a, K: 'a, V: 'a, ALLOCATOR: Allocator> {
     location: usize,
 }
 
+impl<'a, K: 'a, V: 'a, ALLOCATOR: Allocator> OccupiedEntry<'a, K, V, ALLOCATOR> {
+    /// # Safety
+    ///
+    /// You must call this with a valid location (one where the entry is defined)
+    unsafe fn new(key: K, map: &'a mut HashMap<K, V, ALLOCATOR>, location: usize) -> Self {
+        Self { key, map, location }
+    }
+}
+
 impl<'a, K: 'a, V: 'a, ALLOCATOR: ClonableAllocator> OccupiedEntry<'a, K, V, ALLOCATOR> {
     /// Gets a reference to the key in the entry.
     pub fn key(&self) -> &K {
@@ -575,6 +592,7 @@ impl<'a, K: 'a, V: 'a, ALLOCATOR: ClonableAllocator> OccupiedEntry<'a, K, V, ALL
 
     /// Gets a reference to the value in the entry.
     pub fn get(&self) -> &V {
+        // SAFETY: This can only be constructed with valid locations
         unsafe {
             self.map
                 .nodes
@@ -590,6 +608,7 @@ impl<'a, K: 'a, V: 'a, ALLOCATOR: ClonableAllocator> OccupiedEntry<'a, K, V, ALL
     ///
     /// [`into_mut`]: Self::into_mut
     pub fn get_mut(&mut self) -> &mut V {
+        // SAFETY: This can only be constructed with valid locations
         unsafe {
             self.map
                 .nodes
@@ -605,6 +624,7 @@ impl<'a, K: 'a, V: 'a, ALLOCATOR: ClonableAllocator> OccupiedEntry<'a, K, V, ALL
     ///
     /// [`get_mut`]: Self::get_mut
     pub fn into_mut(self) -> &'a mut V {
+        // SAFETY: This can only be constructed with valid locations
         unsafe {
             self.map
                 .nodes
@@ -615,6 +635,7 @@ impl<'a, K: 'a, V: 'a, ALLOCATOR: ClonableAllocator> OccupiedEntry<'a, K, V, ALL
 
     /// Sets the value of the entry and returns the entry's old value.
     pub fn insert(&mut self, value: V) -> V {
+        // SAFETY: This can only be constructed with valid locations
         unsafe {
             self.map
                 .nodes
@@ -757,11 +778,10 @@ where
         let location = self.nodes.location(&key, hash);
 
         if let Some(location) = location {
-            Entry::Occupied(OccupiedEntry {
+            Entry::Occupied(
-                key,
+                // SAFETY: location is valid by the call to location above
-                location,
+                unsafe { OccupiedEntry::new(key, self, location) },
-                map: self,
+            )
-            })
         } else {
             Entry::Vacant(VacantEntry { key, map: self })
         }

agb-hashmap/src/node.rs

@@ -40,7 +40,10 @@ impl<K, V> Node<K, V> {
 
     pub(crate) fn value_mut(&mut self) -> Option<&mut V> {
         if self.has_value() {
-            Some(unsafe { self.value_mut_unchecked() })
+            Some(
+                // SAFETY: has a value
+                unsafe { self.value_mut_unchecked() },
+            )
         } else {
             None
         }
@@ -52,7 +55,10 @@ impl<K, V> Node<K, V> {
 
     pub(crate) fn key_ref(&self) -> Option<&K> {
         if self.distance_to_initial_bucket >= 0 {
-            Some(unsafe { self.key.assume_init_ref() })
+            Some(
+                // SAFETY: has a value
+                unsafe { self.key.assume_init_ref() },
+            )
         } else {
             None
         }
@@ -60,7 +66,10 @@ impl<K, V> Node<K, V> {
 
     pub(crate) fn key_value_ref(&self) -> Option<(&K, &V)> {
         if self.has_value() {
-            Some(unsafe { self.key_value_ref_unchecked() })
+            Some(
+                // SAFETY: has a value
+                unsafe { self.key_value_ref_unchecked() },
+            )
         } else {
             None
         }
@@ -72,7 +81,10 @@ impl<K, V> Node<K, V> {
 
     pub(crate) fn key_value_mut(&mut self) -> Option<(&K, &mut V)> {
         if self.has_value() {
-            Some(unsafe { (self.key.assume_init_ref(), self.value.assume_init_mut()) })
+            Some(
+                // SAFETY: has a value
+                unsafe { (self.key.assume_init_ref(), self.value.assume_init_mut()) },
+            )
         } else {
             None
         }
@@ -88,7 +100,10 @@ impl<K, V> Node<K, V> {
             let value = mem::replace(&mut self.value, MaybeUninit::uninit());
             self.distance_to_initial_bucket = -1;
 
-            Some(unsafe { (key.assume_init(), value.assume_init(), self.hash) })
+            Some(
+                // SAFETY: has a value
+                unsafe { (key.assume_init(), value.assume_init(), self.hash) },
+            )
         } else {
             None
         }
@@ -104,6 +119,7 @@ impl<K, V> Node<K, V> {
             let old_key = mem::replace(&mut self.key, MaybeUninit::new(key));
             let old_value = mem::replace(&mut self.value, MaybeUninit::new(value));
 
+            // SAFETY: has a value
             unsafe { (old_key.assume_init(), old_value.assume_init()) }
         } else {
             panic!("Cannot replace an uninitialised node");
@@ -133,8 +149,11 @@ impl<K, V> Node<K, V> {
 impl<K, V> Drop for Node<K, V> {
     fn drop(&mut self) {
         if self.has_value() {
-            unsafe { ptr::drop_in_place(self.key.as_mut_ptr()) };
+            // SAFETY: has a value
-            unsafe { ptr::drop_in_place(self.value.as_mut_ptr()) };
+            unsafe {
+                ptr::drop_in_place(self.key.as_mut_ptr());
+                ptr::drop_in_place(self.value.as_mut_ptr());
+            }
         }
     }
 }
@@ -155,8 +174,14 @@ where
             Self {
                 hash: self.hash,
                 distance_to_initial_bucket: self.distance_to_initial_bucket,
-                key: MaybeUninit::new(unsafe { self.key.assume_init_ref() }.clone()),
+                key: MaybeUninit::new(
-                value: MaybeUninit::new(unsafe { self.value.assume_init_ref() }.clone()),
+                    // SAFETY: has a value
+                    unsafe { self.key.assume_init_ref() }.clone(),
+                ),
+                value: MaybeUninit::new(
+                    // SAFETY: has a value
+                    unsafe { self.value.assume_init_ref() }.clone(),
+                ),
             }
         } else {
             Self {