patina_internal_collections: Move Default trait bound to method level

garybeihl · garybeihl · commit f6e8da633c07 · 2026-01-15T09:18:08.000-05:00
Moves Default requirement from struct definitions to only the methods that
need it (with_capacity, resize). Also fixes UB in both methods by properly
initializing memory with MaybeUninit.
diff --git a/core/patina_internal_collections/src/bst.rs b/core/patina_internal_collections/src/bst.rs
@@ -37,10 +37,6 @@ where
         Bst { storage: Storage::new(), root: Cell::new(core::ptr::null_mut()) }
     }
 
-    /// Creates a new binary tree with a given slice of memory.
-    pub fn with_capacity(slice: &'a mut [u8]) -> Self {
-        Self { storage: Storage::with_capacity(slice), root: Cell::default() }
-    }
 
     /// Returns the number of elements in the tree.
     pub fn len(&self) -> usize {
@@ -600,7 +596,7 @@ where
 
 impl<D> Default for Bst<'_, D>
 where
-    D: SliceKey,
+    D: SliceKey + Default,
 {
     fn default() -> Self {
         Self::new()
@@ -612,17 +608,6 @@ impl<'a, D> Bst<'a, D>
 where
     D: Copy + SliceKey + 'a,
 {
-    /// Replaces the memory of the tree with a new slice, copying the data from the old slice to the new slice.
-    pub fn resize(&mut self, slice: &'a mut [u8]) {
-        let root = { if self.root.get().is_null() { None } else { Some(self.storage.idx(self.root.get())) } };
-
-        self.storage.resize(slice);
-
-        if let Some(idx) = root {
-            self.root.set(self.storage.get_mut(idx).expect("Pointer Exists."));
-        }
-    }
-
     #[cfg(feature = "alloc")]
     #[cfg_attr(docsrs, doc(cfg(feature = "alloc")))]
     #[allow(dead_code)]
@@ -644,6 +629,38 @@ where
         }
     }
 }
+
+/// Methods that require D to be Copy + Default.
+impl<'a, D> Bst<'a, D>
+where
+    D: Copy + SliceKey + Default + 'a,
+{
+    /// Creates a new binary tree with a given slice of memory.
+    ///
+    /// # Trait Requirements
+    ///
+    /// This method requires `D: Default` to initialize nodes. If your type
+    /// doesn't implement Default, you cannot use with_capacity.
+    pub fn with_capacity(slice: &'a mut [u8]) -> Self {
+        Self { storage: Storage::with_capacity(slice), root: Cell::default() }
+    }
+
+    /// Replaces the memory of the tree with a new slice, copying the data from the old slice to the new slice.
+    ///
+    /// # Trait Requirements
+    ///
+    /// This method requires `D: Default` to initialize new nodes. If your type
+    /// doesn't implement Default, you cannot resize the storage.
+    pub fn resize(&mut self, slice: &'a mut [u8]) {
+        let root = { if self.root.get().is_null() { None } else { Some(self.storage.idx(self.root.get())) } };
+
+        self.storage.resize(slice);
+
+        if let Some(idx) = root {
+            self.root.set(self.storage.get_mut(idx).expect("Pointer Exists."));
+        }
+    }
+}
 impl<D> core::fmt::Debug for Bst<'_, D>
 where
     D: SliceKey,
@@ -702,7 +719,7 @@ mod tests {
 
     #[test]
     fn test_get_functions() {
-        #[derive(Debug)]
+        #[derive(Debug, Default, Copy, Clone)]
         struct MyType(usize, usize);
         impl crate::SliceKey for MyType {
             type Key = usize;
diff --git a/core/patina_internal_collections/src/node.rs b/core/patina_internal_collections/src/node.rs
@@ -6,7 +6,7 @@
 //!
 //! SPDX-License-Identifier: Apache-2.0
 //!
-use core::{cell::Cell, mem, ptr::NonNull, slice};
+use core::{cell::Cell, mem, mem::MaybeUninit, ptr::NonNull, slice};
 
 use crate::{Error, Result, SliceKey};
 
@@ -49,28 +49,6 @@ where
         }
     }
 
-    /// Create a new storage container with a slice of memory.
-    pub fn with_capacity(slice: &'a mut [u8]) -> Storage<'a, D> {
-        let storage = Storage {
-            // SAFETY: This is reinterpreting a byte slice as a Node<D> slice.
-            // 1. The alignment is checked implicitly by the slice bounds.
-            // 2. The correct number of Node<D> elements that fit in the byte slice is calculated.
-            // 3. The lifetime ensures the byte slice remains valid for the storage's lifetime
-            data: unsafe {
-                slice::from_raw_parts_mut::<'a, Node<D>>(
-                    slice as *mut [u8] as *mut Node<D>,
-                    slice.len() / mem::size_of::<Node<D>>(),
-                )
-            },
-            length: 0,
-            available: Cell::default(),
-        };
-
-        Self::build_linked_list(storage.data);
-        storage.available.set(storage.data[0].as_mut_ptr());
-        storage
-    }
-
     fn build_linked_list(buffer: &[Node<D>]) {
         let mut node = &buffer[0];
         for next in buffer.iter().skip(1) {
@@ -176,8 +154,55 @@ where
 
 impl<'a, D> Storage<'a, D>
 where
-    D: SliceKey + Copy,
+    D: SliceKey + Copy + Default,
 {
+    /// Create a new storage container with a slice of memory.
+    ///
+    /// # Trait Requirements
+    ///
+    /// This method requires `D: Default` to initialize nodes. If your type
+    /// doesn't implement Default, you cannot use with_capacity.
+    pub fn with_capacity(slice: &'a mut [u8]) -> Storage<'a, D> {
+        // SAFETY: This is reinterpreting a byte slice as a MaybeUninit<Node<D>> slice.
+        // Using MaybeUninit explicitly represents uninitialized memory and avoids undefined
+        // behavior from creating references to uninitialized Node<D>.
+        let uninit_buffer = unsafe {
+            slice::from_raw_parts_mut::<'a, MaybeUninit<Node<D>>>(
+                slice as *mut [u8] as *mut MaybeUninit<Node<D>>,
+                slice.len() / mem::size_of::<Node<D>>(),
+            )
+        };
+
+        // Initialize all nodes with Default values
+        for elem in uninit_buffer.iter_mut() {
+            // SAFETY: We're initializing uninitialized memory using MaybeUninit::write,
+            // which is the safe way to initialize MaybeUninit without reading the old value.
+            elem.write(Node::new(D::default()));
+        }
+
+        // SAFETY: All elements have been initialized in the loop above.
+        // We can now safely convert from MaybeUninit<Node<D>> to Node<D>.
+        let buffer = unsafe {
+            slice::from_raw_parts_mut(
+                uninit_buffer.as_mut_ptr() as *mut Node<D>,
+                uninit_buffer.len()
+            )
+        };
+
+        let storage = Storage {
+            data: buffer,
+            length: 0,
+            available: Cell::default(),
+        };
+
+        if !storage.data.is_empty() {
+            Self::build_linked_list(storage.data);
+            storage.available.set(storage.data[0].as_mut_ptr());
+        }
+
+        storage
+    }
+
     /// Resizes the storage container to a new slice of memory.
     ///
     /// # Panics
@@ -187,18 +212,50 @@ where
     /// # Time Complexity
     ///
     /// O(n)
+    ///
+    /// # Trait Requirements
+    ///
+    /// This method requires `D: Default` to initialize new nodes.
     pub fn resize(&mut self, slice: &'a mut [u8]) {
-        // SAFETY: This is reinterpreting a byte slice as a Node<D> slice.
-        // 1. The alignment is handled by slice casting rules
-        // 2. The correct number of Node<D> elements that fit in the byte slice is calculated
-        // 3. The lifetime 'a ensures the byte slice remains valid for the storage's lifetime
-        let buffer = unsafe {
-            slice::from_raw_parts_mut::<'a, Node<D>>(
-                slice as *mut [u8] as *mut Node<D>,
+        // SAFETY: This is reinterpreting a byte slice as a MaybeUninit<Node<D>> slice.
+        // Using MaybeUninit explicitly represents uninitialized memory and avoids undefined
+        // behavior from creating references to uninitialized Node<D>.
+        let uninit_buffer = unsafe {
+            slice::from_raw_parts_mut::<'a, MaybeUninit<Node<D>>>(
+                slice as *mut [u8] as *mut MaybeUninit<Node<D>>,
                 slice.len() / mem::size_of::<Node<D>>(),
             )
         };
 
+        assert!(uninit_buffer.len() >= self.len());
+
+        // When current capacity is 0, we need to initialize all nodes
+        if self.capacity() == 0 {
+            // Initialize all nodes since we have no existing data
+            for elem in uninit_buffer.iter_mut() {
+                elem.write(Node::new(D::default()));
+            }
+        } else {
+            // Only initialize NEW nodes (beyond self.len())
+            // The first self.len() nodes will have their data copied from old buffer
+            for elem in uninit_buffer[..self.len()].iter_mut() {
+                // Initialize with a temporary value that will be immediately overwritten
+                elem.write(Node::new(D::default()));
+            }
+            // Initialize only the new nodes (beyond current length)
+            for elem in uninit_buffer[self.len()..].iter_mut() {
+                elem.write(Node::new(D::default()));
+            }
+        }
+
+        // SAFETY: All elements have been initialized. Convert to Node<D> slice.
+        let buffer = unsafe {
+            slice::from_raw_parts_mut(
+                uninit_buffer.as_mut_ptr() as *mut Node<D>,
+                uninit_buffer.len()
+            )
+        };
+
         assert!(buffer.len() >= self.len());
 
         // When current capacity is 0, we just need to copy the data and build the available list
diff --git a/core/patina_internal_collections/src/rbt.rs b/core/patina_internal_collections/src/rbt.rs
@@ -39,10 +39,6 @@ where
         Rbt { storage: Storage::new(), root: Cell::new(core::ptr::null_mut()) }
     }
 
-    /// Creates a new binary tree with a given slice of memory.
-    pub fn with_capacity(slice: &'a mut [u8]) -> Self {
-        Rbt { storage: Storage::with_capacity(slice), root: Cell::default() }
-    }
 
     /// Returns the number of elements in the tree.
     pub fn len(&self) -> usize {
@@ -841,8 +837,18 @@ where
 
 impl<'a, D> Rbt<'a, D>
 where
-    D: SliceKey + Copy + 'a,
+    D: SliceKey + Copy + Default + 'a,
 {
+    /// Creates a new binary tree with a given slice of memory.
+    ///
+    /// # Trait Requirements
+    ///
+    /// This method requires `D: Default` to initialize nodes. If your type
+    /// doesn't implement Default, you cannot use with_capacity.
+    pub fn with_capacity(slice: &'a mut [u8]) -> Self {
+        Rbt { storage: Storage::with_capacity(slice), root: Cell::default() }
+    }
+
     /// Replaces the memory of the tree with a new slice, copying the data from the old slice to the new slice.
     pub fn resize(&mut self, slice: &'a mut [u8]) {
         let root = (!self.root.get().is_null()).then(|| self.storage.idx(self.root.get()));
@@ -887,7 +893,7 @@ where
 
 impl<D> core::fmt::Debug for Rbt<'_, D>
 where
-    D: SliceKey,
+    D: SliceKey + Default,
 {
     fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
         f.debug_struct("Rbt")
@@ -1646,7 +1652,7 @@ mod tests {
 
     #[test]
     fn test_get_functions() {
-        #[derive(Debug)]
+        #[derive(Debug, Default, Copy, Clone)]
         struct MyType(usize, usize);
         impl crate::SliceKey for MyType {
             type Key = usize;
