From c27aecf2fadb17ca7461da45cf18c7d203a14127 Mon Sep 17 00:00:00 2001 From: Declan Kelly Date: Mon, 4 May 2026 16:14:01 -0700 Subject: [PATCH 1/2] Replace inner node empty constructor with builder **Description** - Move the `from_header` and `from_prefix` inner node constructor functions into a separate crate-private trait. - Add a `::builder` method to the public inner node trait which creates an instance of a type-state inner node builder that only gives access to the contained inner node once it has more than one child. - Update existing ops to use the builder function instead of empty constructor - Delete the empty constructor function - Rework a ton of test code to use the builder **Motivation** For a while there has been an invariant that every inner node has at least one child, but this was not really made explicit anywhere or enforced on construction. This builder type makes it straightforward to construct appropriate inner nodes. There is still a way to bypass it for code inside the crate, where you could write: ```rust N::from_header(Header::from_prefix(&[], 0)) ``` which is a functionally empty. I think this change is still an overall benefit, despite the gap. **Testing Done** Updated a bunch of tests, they all pass. --- benches/node/match_prefix.rs | 20 +- benches/node/min_max.rs | 10 +- benches/tree/iter.rs | 16 +- src/raw/operations/insert.rs | 149 ++++--- src/raw/operations/lookup/tests.rs | 188 ++++----- src/raw/representation.rs | 362 +++++++++++++----- src/raw/representation/header.rs | 10 - src/raw/representation/inner_node_direct.rs | 60 ++- src/raw/representation/inner_node_indirect.rs | 70 ++-- src/raw/representation/inner_node_sorted.rs | 154 ++++---- src/raw/representation/pointers.rs | 21 +- src/raw/visitor/well_formed.rs | 70 ++-- 12 files changed, 644 insertions(+), 486 deletions(-) diff --git a/benches/node/match_prefix.rs b/benches/node/match_prefix.rs index 11e704f8..176bde18 100644 --- a/benches/node/match_prefix.rs +++ b/benches/node/match_prefix.rs @@ -26,18 +26,22 @@ fn bench(c: &mut Criterion) { ]; let p0 = &[0, 0, 0, 0, 0, 0, 0, 0]; - let mut node48_small = InnerNode48::, usize, 16>::from_prefix(p0, p0.len()); - let mut node256_small = InnerNodeDirect::, usize, 16>::from_prefix(p0, p0.len()); - node48_small.write_child(99, leaf_opaque); - node256_small.write_child(99, leaf_opaque); + let node48_small = InnerNode48::, usize, 16>::builder(p0, p0.len()) + .write_child(99, leaf_opaque) + .build(); + let node256_small = InnerNodeDirect::, usize, 16>::builder(p0, p0.len()) + .write_child(99, leaf_opaque) + .build(); let p1 = &[ 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, ]; - let mut node48_large = InnerNode48::, usize, 16>::from_prefix(p1, p1.len()); - let mut node256_large = InnerNodeDirect::, usize, 16>::from_prefix(p1, p1.len()); - node48_large.write_child(99, leaf_opaque); - node256_large.write_child(99, leaf_opaque); + let node48_large = InnerNode48::, usize, 16>::builder(p1, p1.len()) + .write_child(99, leaf_opaque) + .build(); + let node256_large = InnerNodeDirect::, usize, 16>::builder(p1, p1.len()) + .write_child(99, leaf_opaque) + .build(); macro_rules! generate_benches { (single_bench $match_func:ident $b:ident $node:ident $key:ident $($current_depth:literal)?) => { diff --git a/benches/node/min_max.rs b/benches/node/min_max.rs index 805a880d..1bdd3b27 100644 --- a/benches/node/min_max.rs +++ b/benches/node/min_max.rs @@ -12,16 +12,18 @@ fn bench(c: &mut Criterion) { let nodes48: Vec<_> = (0..count) .map(|i| { let idx = i * skip; - let mut node = InnerNode48::::empty(); - node.write_child(idx, dangling_opaque); + let node = InnerNode48::::builder(&[], 0) + .write_child(idx, dangling_opaque) + .build(); (idx, node) }) .collect(); let nodes256: Vec<_> = (0..count) .map(|i| { let idx = i * skip; - let mut node = InnerNodeDirect::::empty(); - node.write_child(idx, dangling_opaque); + let node = InnerNodeDirect::::builder(&[], 0) + .write_child(idx, dangling_opaque) + .build(); (idx, node) }) .collect(); diff --git a/benches/tree/iter.rs b/benches/tree/iter.rs index 86a2eb84..cb9a87be 100644 --- a/benches/tree/iter.rs +++ b/benches/tree/iter.rs @@ -11,6 +11,13 @@ fn iter_node>( ty: &str, sizes: &[u16], ) { + for (idx, size) in sizes.iter().enumerate() { + assert!( + *size > 0, + "size {size} in index {idx} must be greater than zero" + ); + } + let mut rng = StdRng::seed_from_u64(69420); let bytes: Vec<_> = (0..=255u8).collect(); @@ -20,11 +27,14 @@ fn iter_node>( let mut group = c.benchmark_group(format!("iter_node/{ty}")); for size in sizes { - let mut node = N::empty(); - for key in bytes.choose_multiple(&mut rng, *size as usize) { - node.write_child(*key, dangling_opaque) + let mut iter = bytes.choose_multiple(&mut rng, *size as usize); + let mut builder = N::builder(&[], 0).write_child(*iter.next().unwrap(), dangling_opaque); + for key in iter { + builder = builder.write_child(*key, dangling_opaque); } + let node = builder.build(); + group.bench_function(format!("{size}").as_str(), |b| { b.iter(|| { node.iter().for_each(|(k, n)| { diff --git a/src/raw/operations/insert.rs b/src/raw/operations/insert.rs index e4ef5354..8f3ab6e6 100644 --- a/src/raw/operations/insert.rs +++ b/src/raw/operations/insert.rs @@ -354,50 +354,60 @@ impl InsertPoint { NodePtr::allocate_node_ptr(LeafNode::with_no_siblings(key, value), alloc); let new_leaf_pointer_opaque = new_leaf_pointer.to_opaque(); - let mut new_n4 = { - // SAFETY: The lifetime of the header reference is bounded to this block and no - // current mutation happens. Also, we know this is an inner node pointer because - // of the specific insert case + let n4_builder = { + // SAFETY: The `header` pointer is bounded to this block and must be pointing to + // an existing inner node. let header = unsafe { mismatched_inner_node_ptr.header_ref_unchecked() }; // prefix mismatch, need to split prefix into two separate nodes and take the // common prefix into a new parent node let prefix = header.read_prefix(); let prefix = &prefix[..prefix.len().min(mismatch.matched_bytes)]; - InnerNode4::from_prefix(prefix, mismatch.matched_bytes) + InnerNode4::builder(prefix, mismatch.matched_bytes) }; - unsafe { - // SAFETY: This is a new node 4 so it's empty and we have - // space for writing new children. We also check the order - // of the keys before writing - if mismatch.prefix_byte < key_byte { - new_n4 - .write_child_unchecked(mismatch.prefix_byte, mismatched_inner_node_ptr); - new_n4.write_child_unchecked(key_byte, new_leaf_pointer_opaque); + let new_n4 = if mismatch.prefix_byte < key_byte { + // SAFETY: The `write_child_unchecked` are safe because we know the builder + // starts empty and we're adding 2 children (out of a capacity of 4) + // in the correct sorted order (given by the if-else condition). + let new_n4 = unsafe { + n4_builder + .write_child_unchecked(mismatch.prefix_byte, mismatched_inner_node_ptr) + .write_child_unchecked(key_byte, new_leaf_pointer_opaque) + .build() + }; - // SAFETY: There are no concurrent modifications, the `apply` safety doc - // covers this - let previous_leaf_ptr = maximum_unchecked(mismatched_inner_node_ptr); + // SAFETY: There are no concurrent modifications, the `apply` safety doc + // covers this + let previous_leaf_ptr = unsafe { maximum_unchecked(mismatched_inner_node_ptr) }; - // SAFETY: There is no concurrent modification of the new leaf node, the - // the `apply` function. - LeafNode::insert_after(new_leaf_pointer, previous_leaf_ptr); - } else { - new_n4.write_child_unchecked(key_byte, new_leaf_pointer_opaque); - new_n4 - .write_child_unchecked(mismatch.prefix_byte, mismatched_inner_node_ptr); + // SAFETY: There is no concurrent modification of the new leaf node, the + // the `apply` function. + unsafe { LeafNode::insert_after(new_leaf_pointer, previous_leaf_ptr) }; - // SAFETY: There are no concurrent modifications, the `apply` safety doc - // covers this - let next_leaf_ptr = minimum_unchecked(mismatched_inner_node_ptr); + new_n4 + } else { + // SAFETY: The `write_child_unchecked` are safe because we know the builder + // starts empty and we're adding 2 children (out of a capacity of 4) + // in the correct sorted order (given by the if-else condition). + let new_n4 = unsafe { + n4_builder + .write_child_unchecked(key_byte, new_leaf_pointer_opaque) + .write_child_unchecked(mismatch.prefix_byte, mismatched_inner_node_ptr) + .build() + }; - // SAFETY: There is no concurrent modification of the new leaf node, the - // existing leaf node, or its siblings because of the safety requirements of - // the `apply` function. - LeafNode::insert_before(new_leaf_pointer, next_leaf_ptr); - } - } + // SAFETY: There are no concurrent modifications, the `apply` safety doc + // covers this + let next_leaf_ptr = unsafe { minimum_unchecked(mismatched_inner_node_ptr) }; + + // SAFETY: There is no concurrent modification of the new leaf node, the + // existing leaf node, or its siblings because of the safety requirements of + // the `apply` function. + unsafe { LeafNode::insert_before(new_leaf_pointer, next_leaf_ptr) }; + + new_n4 + }; { // Scope header mutation so that the mutable reference is held for the minimum @@ -481,44 +491,61 @@ impl InsertPoint { core::hint::assert_unchecked(key_bytes_used <= new_key_bytes_used); } - let mut new_n4 = InnerNode4::from_prefix( + let leaf_node_key_byte = leaf_bytes[new_key_bytes_used]; + let new_leaf_node_key_byte = key_bytes[new_key_bytes_used]; + + // Build the node with the existing leaf as the first child. This + // exhausts the `key_bytes` borrow of `key` so that `key` can be + // moved into the new leaf below. + let n4_builder = InnerNode4::builder( &key_bytes[key_bytes_used..new_key_bytes_used], new_key_bytes_used - key_bytes_used, ); - let leaf_node_key_byte = leaf_bytes[new_key_bytes_used]; - let new_leaf_node_key_byte = key_bytes[new_key_bytes_used]; let new_leaf_node_pointer = NodePtr::allocate_node_ptr(LeafNode::with_no_siblings(key, value), alloc); - unsafe { - // SAFETY: This is a new node 4 so it's empty and we have - // space for writing new children. We also check the order - // of the keys before writing - if leaf_node_key_byte < new_leaf_node_key_byte { - new_n4.write_child_unchecked(leaf_node_key_byte, leaf_node_ptr.to_opaque()); - new_n4.write_child_unchecked( - new_leaf_node_key_byte, - new_leaf_node_pointer.to_opaque(), - ); + let new_n4 = if leaf_node_key_byte < new_leaf_node_key_byte { + // SAFETY: The `write_child_unchecked` are safe because we know the builder + // starts empty and we're adding 2 children (out of a capacity of 4) + // in the correct sorted order (given by the if-else condition). + let new_n4 = unsafe { + n4_builder + .write_child_unchecked(leaf_node_key_byte, leaf_node_ptr.to_opaque()) + .write_child_unchecked( + new_leaf_node_key_byte, + new_leaf_node_pointer.to_opaque(), + ) + .build() + }; - // SAFETY: There is no concurrent modification of the new leaf node, the - // existing leaf node, or its siblings because of the safety requirements of - // the `apply` function. - LeafNode::insert_after(new_leaf_node_pointer, leaf_node_ptr); - } else { - new_n4.write_child_unchecked( - new_leaf_node_key_byte, - new_leaf_node_pointer.to_opaque(), - ); - new_n4.write_child_unchecked(leaf_node_key_byte, leaf_node_ptr.to_opaque()); + // SAFETY: There is no concurrent modification of the new leaf node, the + // existing leaf node, or its siblings because of the safety requirements of + // the `apply` function. + unsafe { LeafNode::insert_after(new_leaf_node_pointer, leaf_node_ptr) }; - // SAFETY: There is no concurrent modification of the new leaf node, the - // existing leaf node, or its siblings because of the safety requirements of - // the `apply` function. - LeafNode::insert_before(new_leaf_node_pointer, leaf_node_ptr); - } - } + new_n4 + } else { + // SAFETY: The `write_child_unchecked` are safe because we know the builder + // starts empty and we're adding 2 children (out of a capacity of 4) + // in the correct sorted order (given by the if-else condition). + let new_n4 = unsafe { + n4_builder + .write_child_unchecked( + new_leaf_node_key_byte, + new_leaf_node_pointer.to_opaque(), + ) + .write_child_unchecked(leaf_node_key_byte, leaf_node_ptr.to_opaque()) + .build() + }; + + // SAFETY: There is no concurrent modification of the new leaf node, the + // existing leaf node, or its siblings because of the safety requirements of + // the `apply` function. + unsafe { LeafNode::insert_before(new_leaf_node_pointer, leaf_node_ptr) }; + + new_n4 + }; ( NodePtr::allocate_node_ptr(new_n4, alloc).to_opaque(), diff --git a/src/raw/operations/lookup/tests.rs b/src/raw/operations/lookup/tests.rs index c3f479c9..b62cccce 100644 --- a/src/raw/operations/lookup/tests.rs +++ b/src/raw/operations/lookup/tests.rs @@ -18,11 +18,11 @@ fn lookup_on_non_copy_leaf() { let l1_ptr: OpaqueNodePtr, String, 16> = NodePtr::from(&mut l1).to_opaque(); let l2_ptr = NodePtr::from(&mut l2).to_opaque(); - let mut inner_node = InnerNode4::from_prefix(&[1, 2], 2); - - // Update inner node prefix and child slots - inner_node.write_child(3, l1_ptr); - inner_node.write_child(4, l2_ptr); + let mut inner_node = InnerNode4::builder(&[1, 2], 2) + // Update inner node prefix and child slots + .write_child(3, l1_ptr) + .write_child(4, l2_ptr) + .build(); let root = NodePtr::from(&mut inner_node).to_opaque(); @@ -69,13 +69,13 @@ fn lookup_on_full_node4() { let l3_ptr = NodePtr::from(&mut l3).to_opaque(); let l4_ptr = NodePtr::from(&mut l4).to_opaque(); - let mut inner_node = InnerNode4::from_prefix(&[1, 2], 2); - - // Update inner node prefix and child slots - inner_node.write_child(1, l1_ptr); - inner_node.write_child(2, l2_ptr); - inner_node.write_child(3, l3_ptr); - inner_node.write_child(4, l4_ptr); + let mut inner_node = InnerNode4::builder(&[1, 2], 2) + // Update inner node prefix and child slots + .write_child(1, l1_ptr) + .write_child(2, l2_ptr) + .write_child(3, l3_ptr) + .write_child(4, l4_ptr) + .build(); let root = NodePtr::from(&mut inner_node).to_opaque(); @@ -119,36 +119,6 @@ fn lookup_on_full_node4() { } } -#[test] -fn lookup_on_empty_nodes() { - let mut n4 = InnerNode4::, (), 16>::empty(); - let mut n16 = InnerNode16::empty(); - let mut n48 = InnerNode48::empty(); - let mut n256 = InnerNodeDirect::empty(); - - let roots = vec![ - NodePtr::from(&mut n4).to_opaque(), - NodePtr::from(&mut n16).to_opaque(), - NodePtr::from(&mut n48).to_opaque(), - NodePtr::from(&mut n256).to_opaque(), - ]; - - for root in roots { - // SAFETY: All the `search` calls are safe because there are no leaves in this - // tree. - unsafe { - assert!(search_unchecked(root, [1, 2, 1].as_ref()).is_none()); - assert!(search_unchecked(root, [1, 2, 2].as_ref()).is_none()); - assert!(search_unchecked(root, [1, 2, 3].as_ref()).is_none()); - assert!(search_unchecked(root, [1, 2, 4].as_ref()).is_none()); - assert!(search_unchecked(root, [].as_ref()).is_none()); - assert!(search_unchecked(root, [1, 2].as_ref()).is_none()); - assert!(search_unchecked(root, [1, 2, 10].as_ref()).is_none()); - assert!(search_unchecked(root, [0, 2, 1].as_ref()).is_none()); - } - } -} - #[test] fn lookup_on_node16() { let mut l1 = LeafNode::with_no_siblings(Box::from([1, 2, 1]), 121); @@ -161,13 +131,13 @@ fn lookup_on_node16() { let l3_ptr = NodePtr::from(&mut l3).to_opaque(); let l4_ptr = NodePtr::from(&mut l4).to_opaque(); - let mut inner_node = InnerNode16::from_prefix(&[1, 2], 2); - - // Update inner node prefix and child slots - inner_node.write_child(1, l1_ptr); - inner_node.write_child(2, l2_ptr); - inner_node.write_child(3, l3_ptr); - inner_node.write_child(4, l4_ptr); + let mut inner_node = InnerNode16::builder(&[1, 2], 2) + // Update inner node prefix and child slots + .write_child(1, l1_ptr) + .write_child(2, l2_ptr) + .write_child(3, l3_ptr) + .write_child(4, l4_ptr) + .build(); let root = NodePtr::from(&mut inner_node).to_opaque(); @@ -223,13 +193,13 @@ fn lookup_on_node48() { let l3_ptr = NodePtr::from(&mut l3).to_opaque(); let l4_ptr = NodePtr::from(&mut l4).to_opaque(); - let mut inner_node = InnerNode48::from_prefix(&[1, 2], 2); - - // Update inner node prefix and child slots - inner_node.write_child(1, l1_ptr); - inner_node.write_child(2, l2_ptr); - inner_node.write_child(3, l3_ptr); - inner_node.write_child(4, l4_ptr); + let mut inner_node = InnerNode48::builder(&[1, 2], 2) + // Update inner node prefix and child slots + .write_child(1, l1_ptr) + .write_child(2, l2_ptr) + .write_child(3, l3_ptr) + .write_child(4, l4_ptr) + .build(); let root = NodePtr::from(&mut inner_node).to_opaque(); @@ -285,13 +255,13 @@ fn lookup_on_node256() { let l3_ptr = NodePtr::from(&mut l3).to_opaque(); let l4_ptr = NodePtr::from(&mut l4).to_opaque(); - let mut inner_node = InnerNodeDirect::from_prefix(&[1, 2], 2); - - // Update inner node prefix and child slots - inner_node.write_child(1, l1_ptr); - inner_node.write_child(2, l2_ptr); - inner_node.write_child(3, l3_ptr); - inner_node.write_child(4, l4_ptr); + let mut inner_node = InnerNodeDirect::builder(&[1, 2], 2) + // Update inner node prefix and child slots + .write_child(1, l1_ptr) + .write_child(2, l2_ptr) + .write_child(3, l3_ptr) + .write_child(4, l4_ptr) + .build(); let root = NodePtr::from(&mut inner_node).to_opaque(); @@ -376,22 +346,23 @@ fn lookup_on_n16_n4_layer_tree() { let l3_ptr = NodePtr::from(&mut l3).to_opaque(); let l4_ptr = NodePtr::from(&mut l4).to_opaque(); - let mut n4_left = InnerNode4::from_prefix(&[5, 6], 2); - let mut n4_right = InnerNode4::from_prefix(&[7, 8], 2); - let mut n16 = InnerNode16::from_prefix(&[1, 2], 2); - - // Update inner node prefix and child slots - n4_left.write_child(1, l1_ptr); - n4_left.write_child(2, l2_ptr); + let mut n4_left = InnerNode4::builder(&[5, 6], 2) + .write_child(1, l1_ptr) + .write_child(2, l2_ptr) + .build(); - n4_right.write_child(3, l3_ptr); - n4_right.write_child(4, l4_ptr); + let mut n4_right = InnerNode4::builder(&[7, 8], 2) + .write_child(3, l3_ptr) + .write_child(4, l4_ptr) + .build(); let n4_left_ptr = NodePtr::from(&mut n4_left).to_opaque(); let n4_right_ptr = NodePtr::from(&mut n4_right).to_opaque(); - n16.write_child(3, n4_left_ptr); - n16.write_child(4, n4_right_ptr); + let mut n16 = InnerNode16::builder(&[1, 2], 2) + .write_child(3, n4_left_ptr) + .write_child(4, n4_right_ptr) + .build(); let root = NodePtr::from(&mut n16).to_opaque(); @@ -456,22 +427,23 @@ fn lookup_on_n48_n4_layer_tree() { let l3_ptr = NodePtr::from(&mut l3).to_opaque(); let l4_ptr = NodePtr::from(&mut l4).to_opaque(); - let mut n4_left = InnerNode4::from_prefix(&[5, 6], 2); - let mut n4_right = InnerNode4::from_prefix(&[7, 8], 2); - let mut n48 = InnerNode4::from_prefix(&[1, 2], 2); + let mut n4_left = InnerNode4::builder(&[5, 6], 2) + .write_child(1, l1_ptr) + .write_child(2, l2_ptr) + .build(); - // Update inner node prefix and child slots - n4_left.write_child(1, l1_ptr); - n4_left.write_child(2, l2_ptr); - - n4_right.write_child(3, l3_ptr); - n4_right.write_child(4, l4_ptr); + let mut n4_right = InnerNode4::builder(&[7, 8], 2) + .write_child(3, l3_ptr) + .write_child(4, l4_ptr) + .build(); let n4_left_ptr = NodePtr::from(&mut n4_left).to_opaque(); let n4_right_ptr = NodePtr::from(&mut n4_right).to_opaque(); - n48.write_child(3, n4_left_ptr); - n48.write_child(4, n4_right_ptr); + let mut n48 = InnerNode4::builder(&[1, 2], 2) + .write_child(3, n4_left_ptr) + .write_child(4, n4_right_ptr) + .build(); let root = NodePtr::from(&mut n48).to_opaque(); @@ -536,22 +508,23 @@ fn lookup_on_n256_n4_layer_tree() { let l3_ptr = NodePtr::from(&mut l3).to_opaque(); let l4_ptr = NodePtr::from(&mut l4).to_opaque(); - let mut n4_left = InnerNode4::from_prefix(&[5, 6], 2); - let mut n4_right = InnerNode4::from_prefix(&[7, 8], 2); - let mut n256 = InnerNodeDirect::from_prefix(&[1, 2], 2); - - // Update inner node prefix and child slots - n4_left.write_child(1, l1_ptr); - n4_left.write_child(2, l2_ptr); + let mut n4_left = InnerNode4::builder(&[5, 6], 2) + .write_child(1, l1_ptr) + .write_child(2, l2_ptr) + .build(); - n4_right.write_child(3, l3_ptr); - n4_right.write_child(4, l4_ptr); + let mut n4_right = InnerNode4::builder(&[7, 8], 2) + .write_child(3, l3_ptr) + .write_child(4, l4_ptr) + .build(); let n4_left_ptr = NodePtr::from(&mut n4_left).to_opaque(); let n4_right_ptr = NodePtr::from(&mut n4_right).to_opaque(); - n256.write_child(3, n4_left_ptr); - n256.write_child(4, n4_right_ptr); + let mut n256 = InnerNodeDirect::builder(&[1, 2], 2) + .write_child(3, n4_left_ptr) + .write_child(4, n4_right_ptr) + .build(); let root = NodePtr::from(&mut n256).to_opaque(); @@ -616,22 +589,23 @@ fn lookup_on_n4_n4_layer_tree() { let l3_ptr = NodePtr::from(&mut l3).to_opaque(); let l4_ptr = NodePtr::from(&mut l4).to_opaque(); - let mut n4_left = InnerNode4::from_prefix(&[5, 6], 2); - let mut n4_right = InnerNode4::from_prefix(&[7, 8], 2); - let mut n4 = InnerNode4::from_prefix(&[1, 2], 2); - - // Update inner node prefix and child slots - n4_left.write_child(1, l1_ptr); - n4_left.write_child(2, l2_ptr); + let mut n4_left = InnerNode4::builder(&[5, 6], 2) + .write_child(1, l1_ptr) + .write_child(2, l2_ptr) + .build(); - n4_right.write_child(3, l3_ptr); - n4_right.write_child(4, l4_ptr); + let mut n4_right = InnerNode4::builder(&[7, 8], 2) + .write_child(3, l3_ptr) + .write_child(4, l4_ptr) + .build(); let n4_left_ptr = NodePtr::from(&mut n4_left).to_opaque(); let n4_right_ptr = NodePtr::from(&mut n4_right).to_opaque(); - n4.write_child(3, n4_left_ptr); - n4.write_child(4, n4_right_ptr); + let mut n4 = InnerNode4::builder(&[1, 2], 2) + .write_child(3, n4_left_ptr) + .write_child(4, n4_right_ptr) + .build(); let root = NodePtr::from(&mut n4).to_opaque(); diff --git a/src/raw/representation.rs b/src/raw/representation.rs index 2936b7e1..519c4a44 100644 --- a/src/raw/representation.rs +++ b/src/raw/representation.rs @@ -4,6 +4,7 @@ use core::{ cmp::Ordering, fmt, iter::FusedIterator, + marker::PhantomData, ops::{Bound, RangeBounds, RangeInclusive}, }; @@ -93,6 +94,56 @@ pub(crate) mod private { } impl Sealed for super::InnerNodeDirect {} impl Sealed for super::LeafNode {} + + /// Internal construction trait for inner nodes. + /// + /// This is `pub(crate)` so external users cannot access `from_header`, + /// `empty`, or `from_prefix` directly — use [`super::InnerNodeBuilder`] + /// from the public API instead. + pub trait InnerNodeInit: Sealed + Sized { + fn from_header(header: super::Header) -> Self; + + fn from_prefix(prefix: &[u8], prefix_len: usize) -> Self { + Self::from_header(super::Header::new(prefix, prefix_len)) + } + } + + impl InnerNodeInit + for super::InnerNodeSorted + { + fn from_header(header: super::Header) -> Self { + use core::mem::MaybeUninit; + Self { + header, + child_pointers: [MaybeUninit::uninit(); SIZE], + keys: [0; SIZE], + } + } + } + + impl InnerNodeInit + for super::InnerNodeIndirect + { + fn from_header(header: super::Header) -> Self { + use core::mem::MaybeUninit; + super::InnerNodeIndirect { + header, + child_indices: [None; 256], + child_pointers: [MaybeUninit::uninit(); SIZE], + } + } + } + + impl InnerNodeInit + for super::InnerNodeDirect + { + fn from_header(header: super::Header) -> Self { + super::InnerNodeDirect { + header, + child_pointers: [None; 256], + } + } + } } /// All nodes which contain a runtime tag that validates their type. @@ -202,7 +253,9 @@ pub struct OptimisticMismatch { /// /// All structures that implement this trait must be `repr(C)` and have a /// [`Header`] as the first field of the struct. -pub unsafe trait InnerNodeCommon: Sized { +pub unsafe trait InnerNodeCommon: + Sized + private::InnerNodeInit +{ /// The type of the iterator over all children of the inner node type Iter<'a>: Iterator)> + DoubleEndedIterator @@ -210,27 +263,21 @@ pub unsafe trait InnerNodeCommon: Sized { where Self: 'a; - /// Create an empty [`InnerNode`], with no children and no prefix - #[inline] - fn empty() -> Self { - Self::from_header(Header::empty()) - } - - /// Create a new [`InnerNode`] using `prefix` as the node prefix and - /// `prefix_len` as the node prefix length. + /// Create a builder for constructing an inner node with at least one child. /// - /// This is done because when a prefix mismatch happens - /// the length of the mismatch can be greater or equal to - /// prefix size, since we search for the first child of the - /// node to recreate the prefix, that's why we don't use - /// `prefix.len()` as the node prefix length - fn from_prefix(prefix: &[u8], prefix_len: usize) -> Self { - Self::from_header(Header::new(prefix, prefix_len)) + /// The builder uses the typestate pattern to ensure + /// [`InnerNodeBuilder::build`] cannot be called until at least one + /// child has been added via [`InnerNodeBuilder::write_child`]. + fn builder( + prefix: &[u8], + prefix_len: usize, + ) -> InnerNodeBuilder { + InnerNodeBuilder { + node: >::from_prefix(prefix, prefix_len), + _state: PhantomData, + } } - /// Create a new [`InnerNode`] using a `Header` - fn from_header(header: Header) -> Self; - /// Get the `Header` from the [`InnerNode`] fn header(&self) -> &Header; @@ -507,6 +554,107 @@ pub trait InnerNode: } } +/// Marker type for [`InnerNodeBuilder`]: no children have been added yet. +pub struct NoChild; + +/// Marker type for [`InnerNodeBuilder`]: at least one child has been added. +pub struct HasChild; + +/// Typestate builder for inner nodes that enforces the non-empty invariant. +/// +/// The only way to call [`build`][InnerNodeBuilder::build] is to first add at +/// least one child via +/// [`write_child`][InnerNodeBuilder::write_child]. This is checked at +/// compile time via the `S` typestate parameter. +/// +/// Obtain a builder via [`InnerNodeCommon::builder`]. +#[expect(clippy::type_complexity)] +pub struct InnerNodeBuilder { + node: N, + _state: PhantomData (K, V, S)>, +} + +impl InnerNodeBuilder +where + N: InnerNodeCommon, +{ + /// Add the first child, transitioning the builder to the [`HasChild`] state + /// and enabling [`build`][InnerNodeBuilder::build]. + pub fn write_child( + mut self, + key_byte: u8, + child: OpaqueNodePtr, + ) -> InnerNodeBuilder { + self.node.write_child(key_byte, child); + InnerNodeBuilder { + node: self.node, + _state: PhantomData, + } + } +} + +impl + InnerNodeBuilder, NoChild> +{ + /// Add the first child to the node without bounds check or order. + /// + /// This function transitions the build to the [`HasChild`] state and + /// enabling [`build`][InnerNodeBuilder::build]. + /// + /// # Safety + /// - This functions assumes that the write is gonna be inbound (i.e the + /// check for a full node is done previously to the call of this function) + pub unsafe fn write_child_unchecked( + mut self, + key_byte: u8, + child: OpaqueNodePtr, + ) -> InnerNodeBuilder, HasChild> { + // SAFETY: Covered by function safety requirements + unsafe { self.node.write_child_unchecked(key_byte, child) }; + InnerNodeBuilder { + node: self.node, + _state: PhantomData, + } + } +} + +impl InnerNodeBuilder +where + N: InnerNodeCommon, +{ + /// Add another child to the node being built. + pub fn write_child(mut self, key_byte: u8, child: OpaqueNodePtr) -> Self { + self.node.write_child(key_byte, child); + self + } + + /// Consume the builder and return the finished inner node. + /// + /// Only callable after at least one child has been added. + pub fn build(self) -> N { + self.node + } +} + +impl + InnerNodeBuilder, HasChild> +{ + /// Add a child to the node without bounds check or order. + /// + /// # Safety + /// - This functions assumes that the write is gonna be inbound (i.e the + /// check for a full node is done previously to the call of this function) + pub unsafe fn write_child_unchecked( + mut self, + key_byte: u8, + child: OpaqueNodePtr, + ) -> Self { + // SAFETY: Covered by function safety requirements + unsafe { self.node.write_child_unchecked(key_byte, child) }; + self + } +} + /// This type represents the contents of an [`InnerNode`] prefix, either read /// directly from the prefix or fetched from a leaf node descendant. /// @@ -744,10 +892,21 @@ mod tests { mem::align_of::() ); - let n4 = InnerNode4::, (), 16>::empty(); - let n16 = InnerNode4::, (), 16>::empty(); - let n48 = InnerNode4::, (), 16>::empty(); - let n256 = InnerNode4::, (), 16>::empty(); + let mut leaf = LeafNode::, (), 16>::with_no_siblings(vec![].into(), ()); + let leaf_ptr = NodePtr::from(&mut leaf).to_opaque(); + + let n4 = InnerNode4::, (), 16>::builder(&[], 0) + .write_child(0, leaf_ptr) + .build(); + let n16 = InnerNode4::, (), 16>::builder(&[], 0) + .write_child(0, leaf_ptr) + .build(); + let n48 = InnerNode4::, (), 16>::builder(&[], 0) + .write_child(0, leaf_ptr) + .build(); + let n256 = InnerNode4::, (), 16>::builder(&[], 0) + .write_child(0, leaf_ptr) + .build(); let n4_ptr = const_addr(&n4 as *const InnerNode4, (), 16>); let n16_ptr = const_addr(&n16 as *const InnerNode4, (), 16>); @@ -762,92 +921,94 @@ mod tests { assert!(n256_ptr.trailing_zeros() >= 3); } - pub(crate) fn inner_node_write_child_test( - mut node: impl InnerNode, Value = ()>, - num_children: usize, - ) { - let mut leaves = Vec::with_capacity(num_children); - for _ in 0..num_children { - leaves.push(LeafNode::with_no_siblings(vec![].into(), ())); - } + pub(crate) fn inner_node_write_child_test(num_children: usize) + where + N: InnerNode, Value = ()>, + { + let mut leaves: Vec, (), PREFIX_LEN>> = (0..num_children) + .map(|_| LeafNode::with_no_siblings(vec![].into(), ())) + .collect(); + + let leaf_pointers: Vec, (), PREFIX_LEN>> = leaves + .iter_mut() + .map(|leaf| NodePtr::from(leaf).to_opaque()) + .collect(); + + let mut node = N::builder(&[], 0).write_child(0, leaf_pointers[0]).build(); assert!(!node.is_full()); - { - let leaf_pointers = leaves - .iter_mut() - .map(|leaf| NodePtr::from(leaf).to_opaque()) - .collect::>(); - - for (idx, leaf_pointer) in leaf_pointers.iter().copied().enumerate() { - node.write_child(u8::try_from(idx).unwrap(), leaf_pointer); - } - for (idx, leaf_pointer) in leaf_pointers.into_iter().enumerate() { - assert_eq!( - node.lookup_child(u8::try_from(idx).unwrap()), - Some(leaf_pointer) - ); - } + for (idx, leaf_pointer) in leaf_pointers[1..].iter().copied().enumerate() { + node.write_child(u8::try_from(idx + 1).unwrap(), leaf_pointer); + } + + for (idx, leaf_pointer) in leaf_pointers.iter().copied().enumerate() { + assert_eq!( + node.lookup_child(u8::try_from(idx).unwrap()), + Some(leaf_pointer) + ); } assert!(node.is_full()); } - pub fn inner_node_remove_child_test( - mut node: impl InnerNode, Value = ()>, - num_children: usize, - ) { - let mut leaves = Vec::with_capacity(num_children); - for _ in 0..num_children { - leaves.push(LeafNode::with_no_siblings(vec![].into(), ())); - } + pub fn inner_node_remove_child_test(num_children: usize) + where + N: InnerNode, Value = ()>, + { + let mut leaves: Vec, (), PREFIX_LEN>> = (0..num_children) + .map(|_| LeafNode::with_no_siblings(vec![].into(), ())) + .collect(); + + let leaf_pointers: Vec, (), PREFIX_LEN>> = leaves + .iter_mut() + .map(|leaf| NodePtr::from(leaf).to_opaque()) + .collect(); + + let mut node = N::builder(&[], 0).write_child(0, leaf_pointers[0]).build(); assert!(!node.is_full()); - { - let leaf_pointers = leaves - .iter_mut() - .map(|leaf| NodePtr::from(leaf).to_opaque()) - .collect::>(); - - for (idx, leaf_pointer) in leaf_pointers.iter().copied().enumerate() { - node.write_child(u8::try_from(idx).unwrap(), leaf_pointer); - } - for (idx, leaf_pointer) in leaf_pointers.iter().copied().enumerate() { - assert_eq!( - node.lookup_child(u8::try_from(idx).unwrap()), - Some(leaf_pointer) - ); - } + for (idx, leaf_pointer) in leaf_pointers[1..].iter().copied().enumerate() { + node.write_child(u8::try_from(idx + 1).unwrap(), leaf_pointer); + } - for (idx, leaf_pointer) in leaf_pointers.iter().copied().enumerate() { - assert_eq!( - node.remove_child(u8::try_from(idx).unwrap()), - Some(leaf_pointer) - ); + for (idx, leaf_pointer) in leaf_pointers.iter().copied().enumerate() { + assert_eq!( + node.lookup_child(u8::try_from(idx).unwrap()), + Some(leaf_pointer) + ); + } - assert_eq!(node.lookup_child(u8::try_from(idx).unwrap()), None); - } + for (idx, leaf_pointer) in leaf_pointers.iter().copied().enumerate() { + assert_eq!( + node.remove_child(u8::try_from(idx).unwrap()), + Some(leaf_pointer) + ); + + assert_eq!(node.lookup_child(u8::try_from(idx).unwrap()), None); } + assert!(!node.is_full()); } - pub(crate) fn inner_node_shrink_test( - mut node: impl InnerNode, Value = ()>, - num_children: usize, - ) { - let mut leaves = Vec::with_capacity(num_children); - for _ in 0..num_children { - leaves.push(LeafNode::with_no_siblings(vec![].into(), ())); - } + pub(crate) fn inner_node_shrink_test(num_children: usize) + where + N: InnerNode, Value = ()>, + { + let mut leaves: Vec, (), PREFIX_LEN>> = (0..num_children) + .map(|_| LeafNode::with_no_siblings(vec![].into(), ())) + .collect(); - let leaf_pointers = leaves + let leaf_pointers: Vec, (), PREFIX_LEN>> = leaves .iter_mut() .map(|leaf| NodePtr::from(leaf).to_opaque()) - .collect::>(); + .collect(); - for (idx, leaf_pointer) in leaf_pointers.iter().copied().enumerate() { - node.write_child(u8::try_from(idx).unwrap(), leaf_pointer); + let mut node = N::builder(&[], 0).write_child(0, leaf_pointers[0]).build(); + + for (idx, leaf_pointer) in leaf_pointers[1..].iter().copied().enumerate() { + node.write_child(u8::try_from(idx + 1).unwrap(), leaf_pointer); } let shrunk_node = node.shrink(); @@ -860,27 +1021,28 @@ mod tests { } } - pub(crate) fn inner_node_min_max_test( - mut node: impl InnerNode, Value = ()>, - num_children: usize, - ) { + pub(crate) fn inner_node_min_max_test(num_children: usize) + where + N: InnerNode, Value = ()>, + { assert!( num_children >= 2, "test harness must specify more than 1 child" ); - let mut leaves = Vec::with_capacity(num_children); - for _ in 0..num_children { - leaves.push(LeafNode::with_no_siblings(vec![].into(), ())); - } + let mut leaves: Vec, (), PREFIX_LEN>> = (0..num_children) + .map(|_| LeafNode::with_no_siblings(vec![].into(), ())) + .collect(); - let leaf_pointers = leaves + let leaf_pointers: Vec, (), PREFIX_LEN>> = leaves .iter_mut() .map(|leaf| NodePtr::from(leaf).to_opaque()) - .collect::>(); + .collect(); - for (idx, leaf_pointer) in leaf_pointers.iter().copied().enumerate() { - node.write_child(u8::try_from(idx).unwrap(), leaf_pointer); + let mut node = N::builder(&[], 0).write_child(0, leaf_pointers[0]).build(); + + for (idx, leaf_pointer) in leaf_pointers[1..].iter().copied().enumerate() { + node.write_child(u8::try_from(idx + 1).unwrap(), leaf_pointer); } assert_eq!(node.header().num_children(), num_children); diff --git a/src/raw/representation/header.rs b/src/raw/representation/header.rs index 49b10144..529cd476 100644 --- a/src/raw/representation/header.rs +++ b/src/raw/representation/header.rs @@ -52,16 +52,6 @@ impl Header { header } - /// Create a new `Header` for an empty node. - #[inline] - pub fn empty() -> Self { - Self { - num_children: 0, - prefix_len: 0, - prefix: [0; PREFIX_LEN], - } - } - /// Read the initialized portion of the prefix present in the header. #[inline] pub fn read_prefix(&self) -> &[u8] { diff --git a/src/raw/representation/inner_node_direct.rs b/src/raw/representation/inner_node_direct.rs index 670d3385..507de5e3 100644 --- a/src/raw/representation/inner_node_direct.rs +++ b/src/raw/representation/inner_node_direct.rs @@ -114,13 +114,6 @@ unsafe impl InnerNodeCommon &self.header } - fn from_header(header: Header) -> Self { - InnerNodeDirect { - header, - child_pointers: [None; 256], - } - } - fn lookup_child(&self, key_fragment: u8) -> Option> { self.child_pointers[usize::from(key_fragment)] } @@ -368,7 +361,6 @@ mod tests { #[test] fn lookup() { - let mut n = InnerNodeDirect::, (), 16>::empty(); let mut l1 = LeafNode::with_no_siblings(Box::from([]), ()); let mut l2 = LeafNode::with_no_siblings(Box::from([]), ()); let mut l3 = LeafNode::with_no_siblings(Box::from([]), ()); @@ -376,56 +368,55 @@ mod tests { let l2_ptr = NodePtr::from(&mut l2).to_opaque(); let l3_ptr = NodePtr::from(&mut l3).to_opaque(); - assert!(n.lookup_child(123).is_none()); - - n.header.inc_num_children(); - n.header.inc_num_children(); - n.header.inc_num_children(); - - n.child_pointers[1] = Some(l1_ptr); - n.child_pointers[123] = Some(l2_ptr); - n.child_pointers[3] = Some(l3_ptr); + let n = InnerNodeDirect::, (), 16>::builder(&[], 0) + .write_child(1, l1_ptr) + .write_child(123, l2_ptr) + .write_child(3, l3_ptr) + .build(); assert_eq!(n.lookup_child(123), Some(l2_ptr)); } #[test] fn write_child() { - inner_node_write_child_test(InnerNodeDirect::<_, _, 16>::empty(), 256) + inner_node_write_child_test::<16, InnerNodeDirect, (), 16>>(256) } #[test] fn remove_child() { - inner_node_remove_child_test(InnerNodeDirect::<_, _, 16>::empty(), 256) + inner_node_remove_child_test::<16, InnerNodeDirect, (), 16>>(256) } #[test] #[should_panic = "unable to grow a Node256, something went wrong!"] fn grow() { - let n = InnerNodeDirect::, (), 16>::empty(); + let mut leaf = LeafNode::, (), 16>::with_no_siblings(vec![].into(), ()); + let leaf_ptr = NodePtr::from(&mut leaf).to_opaque(); + let n = InnerNodeDirect::, (), 16>::builder(&[], 0) + .write_child(0, leaf_ptr) + .build(); n.grow(); } #[test] fn shrink() { - inner_node_shrink_test(InnerNodeDirect::<_, _, 16>::empty(), 48); + inner_node_shrink_test::<16, InnerNodeDirect, (), 16>>(48); } #[test] #[should_panic = "Cannot shrink a InnerNodeDirect when it has more than 48 children. Currently \ has [49] children."] fn shrink_too_many_children_panic() { - inner_node_shrink_test(InnerNodeDirect::<_, _, 16>::empty(), 49); + inner_node_shrink_test::<16, InnerNodeDirect, (), 16>>(49); } #[test] fn min_max() { - inner_node_min_max_test(InnerNodeDirect::<_, _, 16>::empty(), 256); + inner_node_min_max_test::<16, InnerNodeDirect, (), 16>>(256); } fn fixture() -> FixtureReturn, (), 16>, 4> { - let mut n256 = InnerNodeDirect::empty(); let mut l1 = LeafNode::with_no_siblings(vec![].into(), ()); let mut l2 = LeafNode::with_no_siblings(vec![].into(), ()); let mut l3 = LeafNode::with_no_siblings(vec![].into(), ()); @@ -435,10 +426,12 @@ mod tests { let l3_ptr = NodePtr::from(&mut l3).to_opaque(); let l4_ptr = NodePtr::from(&mut l4).to_opaque(); - n256.write_child(3, l1_ptr); - n256.write_child(255, l2_ptr); - n256.write_child(0u8, l3_ptr); - n256.write_child(85, l4_ptr); + let n256 = InnerNodeDirect::builder(&[], 0) + .write_child(3, l1_ptr) + .write_child(255, l2_ptr) + .write_child(0u8, l3_ptr) + .write_child(85, l4_ptr) + .build(); (n256, [l1, l2, l3, l4], [l1_ptr, l2_ptr, l3_ptr, l4_ptr]) } @@ -524,7 +517,6 @@ mod tests { } fn fixture_empty_edges() -> FixtureReturn, (), 16>, 4> { - let mut n4 = InnerNodeDirect::empty(); let mut l1 = LeafNode::with_no_siblings(vec![].into(), ()); let mut l2 = LeafNode::with_no_siblings(vec![].into(), ()); let mut l3 = LeafNode::with_no_siblings(vec![].into(), ()); @@ -534,10 +526,12 @@ mod tests { let l3_ptr = NodePtr::from(&mut l3).to_opaque(); let l4_ptr = NodePtr::from(&mut l4).to_opaque(); - n4.write_child(3, l1_ptr); - n4.write_child(254, l2_ptr); - n4.write_child(2u8, l3_ptr); - n4.write_child(85, l4_ptr); + let n4 = InnerNodeDirect::builder(&[], 0) + .write_child(3, l1_ptr) + .write_child(254, l2_ptr) + .write_child(2u8, l3_ptr) + .write_child(85, l4_ptr) + .build(); (n4, [l1, l2, l3, l4], [l1_ptr, l2_ptr, l3_ptr, l4_ptr]) } diff --git a/src/raw/representation/inner_node_indirect.rs b/src/raw/representation/inner_node_indirect.rs index 5cfa204c..01fee42b 100644 --- a/src/raw/representation/inner_node_indirect.rs +++ b/src/raw/representation/inner_node_indirect.rs @@ -174,14 +174,6 @@ unsafe impl InnerNodeCommon) -> Self { - InnerNodeIndirect { - header, - child_indices: [None; 256], - child_pointers: [MaybeUninit::uninit(); SIZE], - } - } - fn lookup_child(&self, key_fragment: u8) -> Option> { let index = &self.child_indices[usize::from(key_fragment)]; let child_pointers = self.initialized_child_pointers(); @@ -535,7 +527,6 @@ mod tests { #[test] fn lookup() { - let mut n = InnerNode48::, (), 16>::empty(); let mut l1 = LeafNode::with_no_siblings(Box::from([]), ()); let mut l2 = LeafNode::with_no_siblings(Box::from([]), ()); let mut l3 = LeafNode::with_no_siblings(Box::from([]), ()); @@ -543,42 +534,33 @@ mod tests { let l2_ptr = NodePtr::from(&mut l2).to_opaque(); let l3_ptr = NodePtr::from(&mut l3).to_opaque(); - assert!(n.lookup_child(123).is_none()); - - n.header.inc_num_children(); - n.header.inc_num_children(); - n.header.inc_num_children(); - - n.child_indices[1] = Some(2usize.try_into().unwrap()); - n.child_indices[3] = Some(0usize.try_into().unwrap()); - n.child_indices[123] = Some(1usize.try_into().unwrap()); - - n.child_pointers[0].write(l1_ptr); - n.child_pointers[1].write(l2_ptr); - n.child_pointers[2].write(l3_ptr); + let n = InnerNode48::, (), 16>::builder(&[], 0) + .write_child(1, l3_ptr) + .write_child(3, l1_ptr) + .write_child(123, l2_ptr) + .build(); assert_eq!(n.lookup_child(123), Some(l2_ptr)); } #[test] fn write_child() { - inner_node_write_child_test(InnerNode48::<_, _, 16>::empty(), 48) + inner_node_write_child_test::<16, InnerNode48, (), 16>>(48) } #[test] fn remove_child() { - inner_node_remove_child_test(InnerNode48::<_, _, 16>::empty(), 48) + inner_node_remove_child_test::<16, InnerNode48, (), 16>>(48) } #[test] #[should_panic] fn write_child_full_panic() { - inner_node_write_child_test(InnerNode48::<_, _, 16>::empty(), 49); + inner_node_write_child_test::<16, InnerNode48, (), 16>>(49); } #[test] fn grow() { - let mut n48 = InnerNode48::, (), 16>::empty(); let mut l1 = LeafNode::with_no_siblings(vec![].into(), ()); let mut l2 = LeafNode::with_no_siblings(vec![].into(), ()); let mut l3 = LeafNode::with_no_siblings(vec![].into(), ()); @@ -586,9 +568,11 @@ mod tests { let l2_ptr = NodePtr::from(&mut l2).to_opaque(); let l3_ptr = NodePtr::from(&mut l3).to_opaque(); - n48.write_child(3, l1_ptr); - n48.write_child(123, l2_ptr); - n48.write_child(1, l3_ptr); + let n48 = InnerNode48::, (), 16>::builder(&[], 0) + .write_child(3, l1_ptr) + .write_child(123, l2_ptr) + .write_child(1, l3_ptr) + .build(); let n256 = n48.grow(); @@ -600,23 +584,22 @@ mod tests { #[test] fn shrink() { - inner_node_shrink_test(InnerNode48::<_, _, 16>::empty(), 16); + inner_node_shrink_test::<16, InnerNode48, (), 16>>(16); } #[test] #[should_panic = "Cannot shrink a InnerNodeIndirect when it has more than 16 children. \ Currently has [17] children."] fn shrink_too_many_children_panic() { - inner_node_shrink_test(InnerNode48::<_, _, 16>::empty(), 17); + inner_node_shrink_test::<16, InnerNode48, (), 16>>(17); } #[test] fn min_max() { - inner_node_min_max_test(InnerNode48::<_, _, 16>::empty(), 48); + inner_node_min_max_test::<16, InnerNode48, (), 16>>(48); } fn fixture() -> FixtureReturn, (), 16>, 4> { - let mut n4 = InnerNode48::empty(); let mut l1 = LeafNode::with_no_siblings(vec![].into(), ()); let mut l2 = LeafNode::with_no_siblings(vec![].into(), ()); let mut l3 = LeafNode::with_no_siblings(vec![].into(), ()); @@ -626,10 +609,12 @@ mod tests { let l3_ptr = NodePtr::from(&mut l3).to_opaque(); let l4_ptr = NodePtr::from(&mut l4).to_opaque(); - n4.write_child(3, l1_ptr); - n4.write_child(255, l2_ptr); - n4.write_child(0u8, l3_ptr); - n4.write_child(85, l4_ptr); + let n4 = InnerNode48::builder(&[], 0) + .write_child(3, l1_ptr) + .write_child(255, l2_ptr) + .write_child(0u8, l3_ptr) + .write_child(85, l4_ptr) + .build(); (n4, [l1, l2, l3, l4], [l1_ptr, l2_ptr, l3_ptr, l4_ptr]) } @@ -715,7 +700,6 @@ mod tests { } fn fixture_empty_edges() -> FixtureReturn, (), 16>, 4> { - let mut n4 = InnerNode48::empty(); let mut l1 = LeafNode::with_no_siblings(vec![].into(), ()); let mut l2 = LeafNode::with_no_siblings(vec![].into(), ()); let mut l3 = LeafNode::with_no_siblings(vec![].into(), ()); @@ -725,10 +709,12 @@ mod tests { let l3_ptr = NodePtr::from(&mut l3).to_opaque(); let l4_ptr = NodePtr::from(&mut l4).to_opaque(); - n4.write_child(3, l1_ptr); - n4.write_child(254, l2_ptr); - n4.write_child(2u8, l3_ptr); - n4.write_child(85, l4_ptr); + let n4 = InnerNode48::builder(&[], 0) + .write_child(3, l1_ptr) + .write_child(254, l2_ptr) + .write_child(2u8, l3_ptr) + .write_child(85, l4_ptr) + .build(); (n4, [l1, l2, l3, l4], [l1_ptr, l2_ptr, l3_ptr, l4_ptr]) } diff --git a/src/raw/representation/inner_node_sorted.rs b/src/raw/representation/inner_node_sorted.rs index 77f78deb..8f12522d 100644 --- a/src/raw/representation/inner_node_sorted.rs +++ b/src/raw/representation/inner_node_sorted.rs @@ -366,14 +366,6 @@ unsafe impl InnerNodeCommon) -> Self { - Self { - header, - child_pointers: [MaybeUninit::uninit(); SIZE], - keys: [0; SIZE], - } - } - fn lookup_child(&self, key_fragment: u8) -> Option> { let idx = self.lookup_child_index(key_fragment)?; unsafe { @@ -618,7 +610,6 @@ mod tests { #[test] fn node4_lookup() { - let mut n = InnerNode4::, (), 16>::empty(); let mut l1 = LeafNode::with_no_siblings(vec![].into(), ()); let mut l2 = LeafNode::with_no_siblings(vec![].into(), ()); let mut l3 = LeafNode::with_no_siblings(vec![].into(), ()); @@ -626,42 +617,33 @@ mod tests { let l2_ptr = NodePtr::from(&mut l2).to_opaque(); let l3_ptr = NodePtr::from(&mut l3).to_opaque(); - assert!(n.lookup_child(123).is_none()); - - n.header.inc_num_children(); - n.header.inc_num_children(); - n.header.inc_num_children(); - - n.keys[0] = 3; - n.keys[1] = 123; - n.keys[2] = 1; - - n.child_pointers[0].write(l1_ptr); - n.child_pointers[1].write(l2_ptr); - n.child_pointers[2].write(l3_ptr); + let n = InnerNode4::, (), 16>::builder(&[], 0) + .write_child(3, l1_ptr) + .write_child(123, l2_ptr) + .write_child(1, l3_ptr) + .build(); assert_eq!(n.lookup_child(123), Some(l2_ptr)); } #[test] fn node4_write_child() { - inner_node_write_child_test(InnerNode4::<_, _, 16>::empty(), 4) + inner_node_write_child_test::<16, InnerNode4, (), 16>>(4) } #[test] fn node4_remove_child() { - inner_node_remove_child_test(InnerNode4::<_, _, 16>::empty(), 4) + inner_node_remove_child_test::<16, InnerNode4, (), 16>>(4) } #[test] #[should_panic] fn node4_write_child_full_panic() { - inner_node_write_child_test(InnerNode4::<_, _, 16>::empty(), 5); + inner_node_write_child_test::<16, InnerNode4, (), 16>>(5); } #[test] fn node4_grow() { - let mut n4 = InnerNode4::, (), 16>::empty(); let mut l1 = LeafNode::with_no_siblings(vec![].into(), ()); let mut l2 = LeafNode::with_no_siblings(vec![].into(), ()); let mut l3 = LeafNode::with_no_siblings(vec![].into(), ()); @@ -669,9 +651,11 @@ mod tests { let l2_ptr = NodePtr::from(&mut l2).to_opaque(); let l3_ptr = NodePtr::from(&mut l3).to_opaque(); - n4.write_child(3, l1_ptr); - n4.write_child(123, l2_ptr); - n4.write_child(1, l3_ptr); + let n4 = InnerNode4::, (), 16>::builder(&[], 0) + .write_child(3, l1_ptr) + .write_child(123, l2_ptr) + .write_child(1, l3_ptr) + .build(); let n16 = n4.grow(); @@ -684,19 +668,22 @@ mod tests { #[test] #[should_panic = "unable to shrink a Node4, something went wrong!"] fn node4_shrink() { - let n4 = InnerNode4::, (), 16>::empty(); + let mut leaf = LeafNode::, (), 16>::with_no_siblings(vec![].into(), ()); + let leaf_ptr = NodePtr::from(&mut leaf).to_opaque(); + let n4 = InnerNode4::, (), 16>::builder(&[], 0) + .write_child(0, leaf_ptr) + .build(); n4.shrink(); } #[test] fn node4_min_max() { - inner_node_min_max_test(InnerNode4::<_, _, 16>::empty(), 4); + inner_node_min_max_test::<16, InnerNode4, (), 16>>(4); } #[test] fn node16_lookup() { - let mut n = InnerNode16::, (), 16>::empty(); let mut l1 = LeafNode::with_no_siblings(Box::from([]), ()); let mut l2 = LeafNode::with_no_siblings(Box::from([]), ()); let mut l3 = LeafNode::with_no_siblings(Box::from([]), ()); @@ -704,43 +691,34 @@ mod tests { let l2_ptr = NodePtr::from(&mut l2).to_opaque(); let l3_ptr = NodePtr::from(&mut l3).to_opaque(); - assert!(n.lookup_child(123).is_none()); - - n.header.inc_num_children(); - n.header.inc_num_children(); - n.header.inc_num_children(); - - n.keys[0] = 3; - n.keys[1] = 123; - n.keys[2] = 1; - - n.child_pointers[0].write(l1_ptr); - n.child_pointers[1].write(l2_ptr); - n.child_pointers[2].write(l3_ptr); + let n = InnerNode16::, (), 16>::builder(&[], 0) + .write_child(3, l1_ptr) + .write_child(123, l2_ptr) + .write_child(1, l3_ptr) + .build(); assert_eq!(n.lookup_child(123), Some(l2_ptr)); } #[test] fn node16_write_child() { - inner_node_write_child_test(InnerNode16::<_, _, 16>::empty(), 16) + inner_node_write_child_test::<16, InnerNode16, (), 16>>(16) } #[test] fn node16_remove_child() { - inner_node_remove_child_test(InnerNode16::<_, _, 16>::empty(), 16) + inner_node_remove_child_test::<16, InnerNode16, (), 16>>(16) } #[test] #[should_panic] fn node16_write_child_full_panic() { - inner_node_write_child_test(InnerNode16::<_, _, 16>::empty(), 17); + inner_node_write_child_test::<16, InnerNode16, (), 16>>(17); } #[test] #[should_panic = "Node must be full to grow to node 48"] fn node16_grow_panic() { - let mut n16 = InnerNode16::, (), 16>::empty(); let mut l1 = LeafNode::with_no_siblings(vec![].into(), ()); let mut l2 = LeafNode::with_no_siblings(vec![].into(), ()); let mut l3 = LeafNode::with_no_siblings(vec![].into(), ()); @@ -748,22 +726,30 @@ mod tests { let l2_ptr = NodePtr::from(&mut l2).to_opaque(); let l3_ptr = NodePtr::from(&mut l3).to_opaque(); - n16.write_child(3, l1_ptr); - n16.write_child(123, l2_ptr); - n16.write_child(1, l3_ptr); + let n16 = InnerNode16::, (), 16>::builder(&[], 0) + .write_child(3, l1_ptr) + .write_child(123, l2_ptr) + .write_child(1, l3_ptr) + .build(); let _n48 = n16.grow(); } #[test] fn node16_grow() { - let mut n16 = InnerNode16::, (), 16>::empty(); - let mut v = Vec::new(); - for i in 0..16 { - let mut l = LeafNode::with_no_siblings(vec![].into(), ()); - let l_ptr = NodePtr::from(&mut l).to_opaque(); - v.push(l_ptr); - n16.write_child(i * 2, l_ptr); + let mut leaves: Vec, (), 16>> = (0..16) + .map(|_| LeafNode::with_no_siblings(vec![].into(), ())) + .collect(); + let v: Vec<_> = leaves + .iter_mut() + .map(|l| NodePtr::from(l).to_opaque()) + .collect(); + + let mut n16 = InnerNode16::, (), 16>::builder(&[], 0) + .write_child(0, v[0]) + .build(); + for i in 1..16u8 { + n16.write_child(i * 2, v[usize::from(i)]); } let n48 = n16.grow(); @@ -775,23 +761,22 @@ mod tests { #[test] fn node16_shrink() { - inner_node_shrink_test(InnerNode16::<_, _, 16>::empty(), 4); + inner_node_shrink_test::<16, InnerNode16, (), 16>>(4); } #[test] #[should_panic = "Cannot change InnerNodeSorted<16> to size 4 when it has more than 4 \ children. Currently has [5] children."] fn node16_shrink_too_many_children_panic() { - inner_node_shrink_test(InnerNode16::<_, _, 16>::empty(), 5); + inner_node_shrink_test::<16, InnerNode16, (), 16>>(5); } #[test] fn node16_min_max() { - inner_node_min_max_test(InnerNode16::<_, _, 16>::empty(), 16); + inner_node_min_max_test::<16, InnerNode16, (), 16>>(16); } fn node4_fixture() -> FixtureReturn, (), 16>, 4> { - let mut n4 = InnerNode4::empty(); let mut l1 = LeafNode::with_no_siblings(vec![].into(), ()); let mut l2 = LeafNode::with_no_siblings(vec![].into(), ()); let mut l3 = LeafNode::with_no_siblings(vec![].into(), ()); @@ -801,10 +786,12 @@ mod tests { let l3_ptr = NodePtr::from(&mut l3).to_opaque(); let l4_ptr = NodePtr::from(&mut l4).to_opaque(); - n4.write_child(3, l1_ptr); - n4.write_child(255, l2_ptr); - n4.write_child(0u8, l3_ptr); - n4.write_child(85, l4_ptr); + let n4 = InnerNode4::builder(&[], 0) + .write_child(3, l1_ptr) + .write_child(255, l2_ptr) + .write_child(0u8, l3_ptr) + .write_child(85, l4_ptr) + .build(); (n4, [l1, l2, l3, l4], [l1_ptr, l2_ptr, l3_ptr, l4_ptr]) } @@ -885,7 +872,6 @@ mod tests { } fn node4_fixture_empty_edges() -> FixtureReturn, (), 16>, 4> { - let mut n4 = InnerNode4::empty(); let mut l1 = LeafNode::with_no_siblings(vec![].into(), ()); let mut l2 = LeafNode::with_no_siblings(vec![].into(), ()); let mut l3 = LeafNode::with_no_siblings(vec![].into(), ()); @@ -895,10 +881,12 @@ mod tests { let l3_ptr = NodePtr::from(&mut l3).to_opaque(); let l4_ptr = NodePtr::from(&mut l4).to_opaque(); - n4.write_child(3, l1_ptr); - n4.write_child(254, l2_ptr); - n4.write_child(2u8, l3_ptr); - n4.write_child(85, l4_ptr); + let n4 = InnerNode4::builder(&[], 0) + .write_child(3, l1_ptr) + .write_child(254, l2_ptr) + .write_child(2u8, l3_ptr) + .write_child(85, l4_ptr) + .build(); (n4, [l1, l2, l3, l4], [l1_ptr, l2_ptr, l3_ptr, l4_ptr]) } @@ -1009,7 +997,6 @@ mod tests { } fn node16_fixture() -> FixtureReturn, (), 16>, 4> { - let mut n16 = InnerNode16::empty(); let mut l1 = LeafNode::with_no_siblings(vec![].into(), ()); let mut l2 = LeafNode::with_no_siblings(vec![].into(), ()); let mut l3 = LeafNode::with_no_siblings(vec![].into(), ()); @@ -1019,10 +1006,12 @@ mod tests { let l3_ptr = NodePtr::from(&mut l3).to_opaque(); let l4_ptr = NodePtr::from(&mut l4).to_opaque(); - n16.write_child(3, l1_ptr); - n16.write_child(255, l2_ptr); - n16.write_child(0u8, l3_ptr); - n16.write_child(85, l4_ptr); + let n16 = InnerNode16::builder(&[], 0) + .write_child(3, l1_ptr) + .write_child(255, l2_ptr) + .write_child(0u8, l3_ptr) + .write_child(85, l4_ptr) + .build(); (n16, [l1, l2, l3, l4], [l1_ptr, l2_ptr, l3_ptr, l4_ptr]) } @@ -1108,7 +1097,6 @@ mod tests { } fn node16_fixture_empty_edges() -> FixtureReturn, (), 16>, 4> { - let mut n16 = InnerNode16::empty(); let mut l1 = LeafNode::with_no_siblings(vec![].into(), ()); let mut l2 = LeafNode::with_no_siblings(vec![].into(), ()); let mut l3 = LeafNode::with_no_siblings(vec![].into(), ()); @@ -1118,10 +1106,12 @@ mod tests { let l3_ptr = NodePtr::from(&mut l3).to_opaque(); let l4_ptr = NodePtr::from(&mut l4).to_opaque(); - n16.write_child(3, l1_ptr); - n16.write_child(254, l2_ptr); - n16.write_child(2u8, l3_ptr); - n16.write_child(85, l4_ptr); + let n16 = InnerNode16::builder(&[], 0) + .write_child(3, l1_ptr) + .write_child(254, l2_ptr) + .write_child(2u8, l3_ptr) + .write_child(85, l4_ptr) + .build(); (n16, [l1, l2, l3, l4], [l1_ptr, l2_ptr, l3_ptr, l4_ptr]) } diff --git a/src/raw/representation/pointers.rs b/src/raw/representation/pointers.rs index 983bf273..a3a5762e 100644 --- a/src/raw/representation/pointers.rs +++ b/src/raw/representation/pointers.rs @@ -602,14 +602,25 @@ mod tests { use alloc::boxed::Box; use super::*; - use crate::raw::InnerNodeCommon; + use crate::raw::{InnerNodeCommon, LeafNode}; #[test] fn opaque_node_ptr_is_correct() { - let mut n4 = InnerNode4::, usize, 16>::empty(); - let mut n16 = InnerNode16::, usize, 16>::empty(); - let mut n48 = InnerNode48::, usize, 16>::empty(); - let mut n256 = InnerNodeDirect::, usize, 16>::empty(); + let mut leaf = LeafNode::, usize, 16>::with_no_siblings(vec![].into(), 0); + let leaf_ptr: OpaqueNodePtr, usize, 16> = NodePtr::from(&mut leaf).to_opaque(); + + let mut n4 = InnerNode4::, usize, 16>::builder(&[], 0) + .write_child(0, leaf_ptr) + .build(); + let mut n16 = InnerNode16::, usize, 16>::builder(&[], 0) + .write_child(0, leaf_ptr) + .build(); + let mut n48 = InnerNode48::, usize, 16>::builder(&[], 0) + .write_child(0, leaf_ptr) + .build(); + let mut n256 = InnerNodeDirect::, usize, 16>::builder(&[], 0) + .write_child(0, leaf_ptr) + .build(); let n4_ptr = NodePtr::from(&mut n4).to_opaque(); let n16_ptr = NodePtr::from(&mut n16).to_opaque(); diff --git a/src/raw/visitor/well_formed.rs b/src/raw/visitor/well_formed.rs index 0d105ea8..0762e9b1 100644 --- a/src/raw/visitor/well_formed.rs +++ b/src/raw/visitor/well_formed.rs @@ -676,34 +676,40 @@ mod tests { let l2_ptr = NodePtr::allocate_node_ptr(l2, &Global); let l3_ptr = NodePtr::allocate_node_ptr(l3, &Global); - let n4_left = InnerNode4::from_prefix(&[5, 6], 2); - let n4_right = InnerNode4::from_prefix(&[7, 8], 2); - let n16 = InnerNode16::from_prefix(&[1, 2], 2); - + // Build n4_left with first child, then allocate + let n4_left = InnerNode4::builder(&[5, 6], 2) + .write_child(1, l1_ptr.to_opaque()) + .build(); let n4_left_ptr = NodePtr::allocate_node_ptr(n4_left, &Global); + + // Build n4_right with first child (l3), then allocate; loop child added later + let n4_right = InnerNode4::builder(&[7, 8], 2) + .write_child(3, l3_ptr.to_opaque()) + .build(); let n4_right_ptr = NodePtr::allocate_node_ptr(n4_right, &Global); + // Build n16 with first child (n4_left), then allocate; n4_right added later + let n16 = InnerNode16::builder(&[1, 2], 2) + .write_child(3, n4_left_ptr.to_opaque()) + .build(); + // construct root early let root = NodePtr::allocate_node_ptr(n16, &Global); { let n4_left = unsafe { n4_left_ptr.as_mut() }; - // Update inner node prefix and child slots - n4_left.write_child(1, l1_ptr.to_opaque()); + // Add remaining child n4_left.write_child(2, l2_ptr.to_opaque()); } { let n4_right = unsafe { n4_right_ptr.as_mut() }; - - n4_right.write_child(3, l3_ptr.to_opaque()); // replace normal l4 pointer with loop back to root n4_right.write_child(4, root.to_opaque()); } { let n16 = unsafe { root.as_mut() }; - n16.write_child(3, n4_left_ptr.to_opaque()); n16.write_child(4, n4_right_ptr.to_opaque()); } @@ -759,22 +765,23 @@ mod tests { let l3_ptr = NodePtr::from(&mut l3).to_opaque(); let l4_ptr = NodePtr::from(&mut l4).to_opaque(); - let mut n4_left = InnerNode4::from_prefix(&[5, 6], 2); - let mut n4_right = InnerNode4::from_prefix(&[7, 8], 2); - let mut n16 = InnerNode16::from_prefix(&[1, 2], 2); - - // Update inner node prefix and child slots - n4_left.write_child(1, l1_ptr); - n4_left.write_child(2, l2_ptr); + let mut n4_left = InnerNode4::builder(&[5, 6], 2) + .write_child(1, l1_ptr) + .write_child(2, l2_ptr) + .build(); - n4_right.write_child(3, l3_ptr); - n4_right.write_child(4, l4_ptr); + let mut n4_right = InnerNode4::builder(&[7, 8], 2) + .write_child(3, l3_ptr) + .write_child(4, l4_ptr) + .build(); let n4_left_ptr = NodePtr::from(&mut n4_left).to_opaque(); let n4_right_ptr = NodePtr::from(&mut n4_right).to_opaque(); - n16.write_child(3, n4_left_ptr); - n16.write_child(4, n4_right_ptr); + let mut n16 = InnerNode16::builder(&[1, 2], 2) + .write_child(3, n4_left_ptr) + .write_child(4, n4_right_ptr) + .build(); let root = NodePtr::from(&mut n16).to_opaque(); @@ -808,22 +815,23 @@ mod tests { let l3_ptr = NodePtr::from(&mut l3).to_opaque(); let l4_ptr = NodePtr::from(&mut l4).to_opaque(); - let mut n4_left = InnerNode4::from_prefix(&[5, 6], 2); - let mut n4_right = InnerNode4::from_prefix(&[7, 8], 2); - let mut n16 = InnerNode16::from_prefix(&[1, 2], 2); - - // Update inner node prefix and child slots - n4_left.write_child(1, l1_ptr); - n4_left.write_child(2, l2_ptr); + let mut n4_left = InnerNode4::builder(&[5, 6], 2) + .write_child(1, l1_ptr) + .write_child(2, l2_ptr) + .build(); - n4_right.write_child(3, l3_ptr); - n4_right.write_child(4, l4_ptr); + let mut n4_right = InnerNode4::builder(&[7, 8], 2) + .write_child(3, l3_ptr) + .write_child(4, l4_ptr) + .build(); let n4_left_ptr = NodePtr::from(&mut n4_left).to_opaque(); let n4_right_ptr = NodePtr::from(&mut n4_right).to_opaque(); - n16.write_child(3, n4_left_ptr); - n16.write_child(4, n4_right_ptr); + let mut n16 = InnerNode16::builder(&[1, 2], 2) + .write_child(3, n4_left_ptr) + .write_child(4, n4_right_ptr) + .build(); let root = NodePtr::from(&mut n16).to_opaque(); From c101a193469f87f6d386f4ddd1531209c1b30fed Mon Sep 17 00:00:00 2001 From: Declan Kelly Date: Sat, 2 May 2026 14:21:26 -0700 Subject: [PATCH 2/2] Update changelog for recent range bug --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8599c009..8dfec205 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,9 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ### Changed - Implement `FusedIterator` for `TreeMap` iterators `IntoKeys` and `IntoValues`. +### Fixed + - `TreeMap::range` iterator had bugs in a couple of places, specifically around determining bounds when the initial lookup terminated in an inner node. I added some better fuzz coverage for this iterator that helped with finding these issues. + ## [0.5.0] - 2026-04-16 ### Added