From 6ab82e2a5bd98ead597658887e90e3cb7f9f38be Mon Sep 17 00:00:00 2001 From: Luke Peterson Date: Fri, 2 Oct 2026 02:30:54 -0600 Subject: [PATCH 1/6] Adding a pseudorandom test to exercise the WZ law that passing prune=false followed by prune_path should have identical result to passing prune=true --- src/dense_byte_node.rs | 8 ++- src/line_list_node.rs | 7 +-- src/write_zipper.rs | 117 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 127 insertions(+), 5 deletions(-) diff --git a/src/dense_byte_node.rs b/src/dense_byte_node.rs index 3395e0d7..c95cdf22 100644 --- a/src/dense_byte_node.rs +++ b/src/dense_byte_node.rs @@ -1181,9 +1181,13 @@ impl> TrieNode (Some(&ALL_BYTES[prefix..=prefix]), cf.rec().map(|cf| cf.as_tagged())) } - fn node_remove_unmasked_branches(&mut self, key: &[u8], mask: ByteMask, _prune_limit: usize) { + fn node_remove_unmasked_branches(&mut self, key: &[u8], mask: ByteMask, prune_limit: usize) { if key.len() > 0 { - //We're in a non-existent path below this node + // A one-byte key may name an existing dangling child. With pruning + // enabled, remove it even though it has no branches to mask. + if key.len() == 1 && prune_limit == 0 { + self.node_remove_dangling(key, prune_limit); + } return } // in the future we can use `drain_filter`, but that's experimental diff --git a/src/line_list_node.rs b/src/line_list_node.rs index 6dd77fbe..d031e95d 100644 --- a/src/line_list_node.rs +++ b/src/line_list_node.rs @@ -1958,17 +1958,18 @@ impl TrieNode for LineListNode if key0.len() > key_len { remove_0 = !mask.test_bit(key0[key_len]); } else { - //We can only get here if key0 == key, and the calling code should have descend - // through this node if that key specifies a non-dangling onward link + // An exact empty child is a dangling focus, which can be pruned here. debug_assert!(!self.is_used_child_0() || unsafe{ self.child_in_slot::<0>().is_empty() }); + remove_0 = prune_limit < key_len && self.is_used_child_0() && unsafe{ self.child_in_slot::<0>().is_empty() }; } } if starts_with(key1, key) { if key1.len() > key_len { remove_1 = !mask.test_bit(key1[key_len]); } else { - //See comment above + // See the exact-key case for slot 0. debug_assert!(!self.is_used_child_1() || unsafe{ self.child_in_slot::<1>().is_empty() }); + remove_1 = prune_limit < key_len && self.is_used_child_1() && unsafe{ self.child_in_slot::<1>().is_empty() }; } } self.remove_subtries(remove_0, remove_1, key0_starts_with, prune_limit < key.len(), key.len()); diff --git a/src/write_zipper.rs b/src/write_zipper.rs index 5210220f..98bb3517 100644 --- a/src/write_zipper.rs +++ b/src/write_zipper.rs @@ -5988,6 +5988,123 @@ mod tests { assert_eq!(wz.child_mask(), ByteMask::EMPTY); } + /// Make sure a WriteZipper method with `prune=true` results in exactly the same ending trie state as the same + /// method called with `prune=false` followed by a call to `prune_path` + #[test] + fn write_zipper_prune_flag_test() { + // Keep the minimal regression case easy to recognize. + for flag in [false, true] { + let mut map = PathMap::::new(); + map.create_path([0]); + let mut wz = map.write_zipper(); + wz.descend_to(&[0]); + wz.remove_unmasked_branches(ByteMask::EMPTY, flag); + if !flag { + assert_eq!(wz.prune_path(), 1); + } + assert!( + !wz.path_exists(), + "remove_unmasked_branches with prune={flag} left a dangling focus" + ); + } + + #[cfg(miri)] + const CASE_COUNT: usize = 10; + #[cfg(not(miri))] + const CASE_COUNT: usize = 400; + const OPS: [&str; 8] = [ + "remove_val", "remove_branches", "remove_unmasked_branches", "take_map", + "join_k_path_into", "meet_into", "subtract_into", "join_into_take", + ]; + + use rand::prelude::*; + let mut rng = StdRng::from_seed([53; 32]); + let mut failures = Vec::new(); + 'cases: for case_idx in 0..CASE_COUNT { + let alphabet = rng.random_range(2..=5u8); + let mut map = PathMap::::new(); + let mut source = PathMap::::new(); + let mut paths = Vec::new(); + for path_idx in 0..rng.random_range(1..=12usize) { + let len = if case_idx % 8 == 0 && path_idx == 0 { + rng.random_range(48..=55usize) + } else { + rng.random_range(0..=7usize) + }; + let path: Vec = (0..len).map(|_| rng.random_range(0..alphabet)).collect(); + map.set_val_at(&path, 1u64 << rng.random_range(0..8)); + if rng.random_bool(0.5) { + source.set_val_at(&path, 1u64 << rng.random_range(0..8)); + } + paths.push(path); + } + for _ in 0..rng.random_range(0..=4usize) { + let len = rng.random_range(1..=7usize); + let path: Vec = (0..len).map(|_| rng.random_range(0..alphabet)).collect(); + map.create_path(&path); + paths.push(path); + } + for _ in 0..rng.random_range(0..=4usize) { + let len = rng.random_range(0..=7usize); + let path: Vec = (0..len).map(|_| rng.random_range(0..alphabet)).collect(); + source.set_val_at(&path, 1u64 << rng.random_range(0..8)); + } + let chosen = &paths[rng.random_range(0..paths.len())]; + let mut focus = chosen[..rng.random_range(0..=chosen.len())].to_vec(); + if rng.random_bool(0.2) { focus.push(alphabet); } // An off-trie focus. + let root_len = rng.random_range(0..=focus.len()); + let mask = ByteMask::from_iter((0..=alphabet).filter(|_| rng.random_bool(0.5))); + let k = rng.random_range(0..=4usize); + + for (op_idx, op) in OPS.iter().enumerate() { + let run = |prune: bool| { + let mut dst = map.clone(); + let mut src = source.clone(); + { + let mut wz = dst.write_zipper_at_path(&focus[..root_len]); + wz.descend_to(&focus[root_len..]); + match op_idx { + 0 => { wz.remove_val(prune); }, + 1 => { wz.remove_branches(prune); }, + 2 => { wz.remove_unmasked_branches(mask, prune); }, + 3 => { wz.take_map(prune); }, + 4 => { wz.join_k_path_into(k, prune); }, + 5 => { wz.meet_into(&src.read_zipper_at_path(&focus), prune); }, + 6 => { wz.subtract_into(&src.read_zipper_at_path(&focus), prune); }, + 7 => { + let mut src_wz = src.write_zipper_at_path(&focus[..root_len]); + src_wz.descend_to(&focus[root_len..]); + wz.join_into_take(&mut src_wz, prune); + if !prune { src_wz.prune_path(); } + }, + _ => unreachable!(), + } + if !prune && op_idx != 7 { wz.prune_path(); } + } + assert_valid_trie(dst.root()); + assert_valid_trie(src.root()); + (all_locations(&dst), all_locations(&src)) + }; + if run(true) != run(false) { + failures.push(format!("case={case_idx} op={op} focus={focus:?} root_len={root_len} k={k}")); + if failures.len() == 20 { break 'cases; } + } + } + } + assert!(failures.is_empty(), "{failures:?}"); + } + + #[test] + fn remove_unmasked_prunes_dangling_focus_below_zipper_root() { + let mut map = PathMap::::new(); + map.create_path([0, 1, 2]); + let mut wz = map.write_zipper_at_path(&[0, 1]); + wz.descend_to(&[2]); + wz.remove_unmasked_branches(ByteMask::EMPTY, true); + assert!(!wz.path_exists()); + assert!(map.path_exists_at(&[0, 1])); + } + /// Tests [`ZipperInfallibleSubtries::get_focus`] and [`ZipperInfallibleSubtries::try_borrow_focus`] internal APIs on [`WriteZipperCore`] #[test] fn write_zipper_focus_nodes() { From 213fb653235e4fb2731e665396d0040094aa24ff Mon Sep 17 00:00:00 2001 From: Luke Peterson Date: Fri, 2 Oct 2026 08:06:25 -0600 Subject: [PATCH 2/6] Fixing prune flag inconsistencies; tightening the spec on acceptable LineListNode to disallow dangling sentinels and values from overlapping --- src/dense_byte_node.rs | 7 +- src/line_list_node.rs | 236 +++++++++++++++++++++++++++++------------ src/write_zipper.rs | 32 +++--- 3 files changed, 192 insertions(+), 83 deletions(-) diff --git a/src/dense_byte_node.rs b/src/dense_byte_node.rs index c95cdf22..109d0e73 100644 --- a/src/dense_byte_node.rs +++ b/src/dense_byte_node.rs @@ -185,7 +185,7 @@ impl> ByteNode let cf = unsafe { self.values.get_unchecked_mut(ix) }; let result = cf.take_val(); - if prune_limit == 0 && !cf.has_rec() { + if prune_limit == 0 && cf.rec().map(|rec| rec.as_tagged().node_is_empty()).unwrap_or(true) { self.mask.clear_bit(k); self.values.remove(ix); } @@ -1017,9 +1017,12 @@ impl> TrieNode } true }, - (false, _) => { + (false, false) if prune_limit == 0 => { + self.values.remove(ix); + self.mask.clear_bit(k); false }, + (false, _) => false, } } else { false diff --git a/src/line_list_node.rs b/src/line_list_node.rs index d031e95d..188104d4 100644 --- a/src/line_list_node.rs +++ b/src/line_list_node.rs @@ -1077,29 +1077,44 @@ impl LineListNode { fn factor_prefix(&mut self) where V: Clone + Lattice { let (key0, key1) = self.get_both_keys(); let overlap = find_prefix_overlap(key0, key1); + if overlap == 0 { + return + } //Overlap of 1 is legal if and only if ONE OF the following two conditions are true: // A: slot0 contains a value AND has a 1-byte key (a value at the shared byte, slot1 continuing below it) // B: both slots have a length of 1, and one is a value - let legal_overlap = overlap == 1 && ( - (!self.is_child_ptr::<0>() && key0.len() == 1) || - (!self.is_child_ptr::<1>() && key0.len()==1 && key1.len()==1 )); + if overlap == 1 && key0.len() == 1 { + if self.is_used_value_0() { + // With two one-byte keys, an empty child in slot 1 is + // subsumed by the value in slot 0. + if key1.len() == 1 && self.is_used_child_1() && unsafe{ self.child_in_slot::<1>().is_empty() } { + self.take_payload::<1>(); + } + return + } + if key1.len() == 1 && self.is_used_value_1() { + // The mirror layout has the child in slot 0. + if self.is_used_child_0() && unsafe{ self.child_in_slot::<0>().is_empty() } { + self.take_payload::<0>(); + } + return + } + } //If the overlap is illegal, split the prefix - if overlap > 0 && !legal_overlap { - match merge_guts::(overlap, key0, self, key1, self) { - AlgebraicResult::Element((shared_key, merged_payload)) => { - let mut new_node = Self::new_in(self.alloc.clone()); - unsafe{ new_node.set_payload_owned::<0>(shared_key, merged_payload) }; - *self = new_node; - }, - AlgebraicResult::Identity(mask) => { - debug_assert!(mask & SELF_IDENT > 0); - let mut new_node = Self::new_in(self.alloc.clone()); - unsafe{ new_node.set_payload_owned::<0>(key0, self.clone_payload::<0>().unwrap()) }; - *self = new_node; - }, - AlgebraicResult::None => {} - } + match merge_guts::(overlap, key0, self, key1, self) { + AlgebraicResult::Element((shared_key, merged_payload)) => { + let mut new_node = Self::new_in(self.alloc.clone()); + unsafe{ new_node.set_payload_owned::<0>(shared_key, merged_payload) }; + *self = new_node; + }, + AlgebraicResult::Identity(mask) => { + debug_assert!(mask & SELF_IDENT > 0); + let mut new_node = Self::new_in(self.alloc.clone()); + unsafe{ new_node.set_payload_owned::<0>(key0, self.clone_payload::<0>().unwrap()) }; + *self = new_node; + }, + AlgebraicResult::None => {} } } @@ -1336,6 +1351,12 @@ fn merge_guts<'a, V: Clone + Lattice + Send + Sync, A: Allocator, const ASLOT: u let b_val = unsafe{ b.val_in_slot::() }; return a_val.pjoin(b_val).map(|new_val| (a_key, ValOrChild::Val(new_val))) }, + (true, false) if unsafe{ a.child_in_slot::() }.is_empty() => { + return AlgebraicResult::Element((a_key, b.clone_payload::().unwrap())) + }, + (false, true) if unsafe{ b.child_in_slot::() }.is_empty() => { + return AlgebraicResult::Element((a_key, a.clone_payload::().unwrap())) + }, _ => {} } } @@ -1415,16 +1436,26 @@ fn merge_list_nodes(a: &LineList let (self_key0, self_key1) = a.get_both_keys(); let (other_key0, other_key1) = b.get_both_keys(); + + // A value subsumes a dangling sentinel at the same key. Ignore that + // sentinel in every pairing: otherwise it could also merge with another + // slot of the value's node. + let ignored = [ + a.is_used_child_0() && unsafe{ a.child_in_slot::<0>().is_empty() } && b.contains_val(self_key0), + a.is_used_child_1() && unsafe{ a.child_in_slot::<1>().is_empty() } && b.contains_val(self_key1), + b.is_used_child_0() && unsafe{ b.child_in_slot::<0>().is_empty() } && a.contains_val(other_key0), + b.is_used_child_1() && unsafe{ b.child_in_slot::<1>().is_empty() } && a.contains_val(other_key1), + ]; let mut entries: [MaybeUninit<(&[u8], ValOrChild)>; 4] = [MaybeUninit::uninit(), MaybeUninit::uninit(), MaybeUninit::uninit(), MaybeUninit::uninit()]; let mut entry_cnt = 0; - let mut used: [bool; 4] = [false; 4]; //[self_0, self_1, other_0, other_1] + let mut used = ignored; //[self_0, self_1, other_0, other_1] let mut identity_masks: [u64; 4] = [0; 4]; // Try each pairing in self and other, to see if there is a key-join that can happen // We can assume two keys in the same node can't merge, because they would have already been merged, // and therefore we can also assume that if a key can be merged with one key of a node it can't be // merged with the other - match try_merge::(self_key0, a, other_key0, b) { + match if ignored[0] || ignored[2] { AlgebraicResult::None } else { try_merge::(self_key0, a, other_key0, b) } { AlgebraicResult::Element(joined) => { entries[entry_cnt] = MaybeUninit::new(joined); entry_cnt += 1; @@ -1445,7 +1476,7 @@ fn merge_list_nodes(a: &LineList }, AlgebraicResult::None => { } } - match try_merge::(self_key0, a, other_key1, b) { + match if ignored[0] || ignored[3] { AlgebraicResult::None } else { try_merge::(self_key0, a, other_key1, b) } { AlgebraicResult::Element(joined) => { entries[entry_cnt] = MaybeUninit::new(joined); entry_cnt += 1; @@ -1468,7 +1499,7 @@ fn merge_list_nodes(a: &LineList }, AlgebraicResult::None => {} } - match try_merge::(self_key1, a, other_key0, b) { + match if ignored[1] || ignored[2] { AlgebraicResult::None } else { try_merge::(self_key1, a, other_key0, b) } { AlgebraicResult::Element(joined) => { entries[entry_cnt] = MaybeUninit::new(joined); entry_cnt += 1; @@ -1491,7 +1522,7 @@ fn merge_list_nodes(a: &LineList }, AlgebraicResult::None => {} } - match try_merge::(self_key1, a, other_key1, b) { + match if ignored[1] || ignored[3] { AlgebraicResult::None } else { try_merge::(self_key1, a, other_key1, b) } { AlgebraicResult::Element(joined) => { entries[entry_cnt] = MaybeUninit::new(joined); entry_cnt += 1; @@ -1810,46 +1841,45 @@ impl TrieNode for LineListNode fn node_remove_val(&mut self, key: &[u8], prune_limit: usize) -> Option { //Removing a value is one of the ways a node can be left holding two onward children // under one key, so check the node over on the way out - let result = (|| { - if self.is_used_value_0() { - let node_key_0 = unsafe{ self.key_unchecked::<0>() }; - if node_key_0 == key { - if prune_limit < key.len() { - return Some(self.take_payload::<0>().unwrap().into_val()) - } else { - //If the other slot already keeps this path, then just remove the value - let node_key_1 = unsafe{ self.key_unchecked::<1>() }; - let overlap = find_prefix_overlap(node_key_0, node_key_1); - if node_key_0.len() == overlap { - return Some(self.take_payload::<0>().unwrap().into_val()) - } else { - //Otherwise, turn the value into an empty node - return Some(self.swap_payload::<0>(ValOrChild::Child(TrieNodeODRc::new_empty())).into_val()) - } - } - } + debug_assert!(validate_node(self)); + let (key0, key1) = self.get_both_keys(); + let value0 = self.is_used_value_0() && key0 == key; + let value1 = self.is_used_value_1() && key1 == key; + let can_prune = prune_limit < key.len(); + let check_dangling = can_prune && !value0 && !value1; + let dangling0 = check_dangling && self.is_used_child_0() && key0 == key && unsafe{ self.child_in_slot::<0>().is_empty() }; + let dangling1 = check_dangling && self.is_used_child_1() && key1 == key && unsafe{ self.child_in_slot::<1>().is_empty() }; + + let (result, removed) = if value0 || dangling0 { + if can_prune { + let payload = self.take_payload::<0>().unwrap(); + (match payload { ValOrChild::Val(val) => Some(val), ValOrChild::Child(_) => None }, true) + } else { + let overlap = find_prefix_overlap(key0, key1); + let val = if key0.len() == overlap { + self.take_payload::<0>().unwrap().into_val() + } else { + self.swap_payload::<0>(ValOrChild::Child(TrieNodeODRc::new_empty())).into_val() + }; + (Some(val), true) } - if self.is_used_value_1() { - let node_key_1 = unsafe{ self.key_unchecked::<1>() }; - if node_key_1 == key { - if prune_limit < key.len() { - return Some(self.take_payload::<1>().unwrap().into_val()) - } else { - //If the other slot already keeps this path, then remove the value - let node_key_0 = unsafe{ self.key_unchecked::<0>() }; - let overlap = find_prefix_overlap(node_key_1, node_key_0); - if node_key_1.len() == overlap { - return Some(self.take_payload::<1>().unwrap().into_val()) - } else { - //Otherwise, turn the value into an empty node - return Some(self.swap_payload::<1>(ValOrChild::Child(TrieNodeODRc::new_empty())).into_val()) - } - } - } + } else if value1 || dangling1 { + if can_prune { + let payload = self.take_payload::<1>().unwrap(); + (match payload { ValOrChild::Val(val) => Some(val), ValOrChild::Child(_) => None }, true) + } else { + let overlap = find_prefix_overlap(key1, key0); + let val = if key1.len() == overlap { + self.take_payload::<1>().unwrap().into_val() + } else { + self.swap_payload::<1>(ValOrChild::Child(TrieNodeODRc::new_empty())).into_val() + }; + (Some(val), true) } - None - })(); - if prune_limit > 0 && prune_limit < key.len() && result.is_some() { + } else { + (None, false) + }; + if prune_limit > 0 && can_prune && removed { self.preserve_prune_limit(key, prune_limit); } debug_assert!(validate_node(self)); @@ -1943,8 +1973,9 @@ impl TrieNode for LineListNode let key0_starts_with = starts_with(key0, key); let remove_0 = key0_starts_with && (key0.len() > key_len || self.is_child_ptr::<0>()); let remove_1 = starts_with(key1, key) && (key1.len() > key_len || self.is_child_ptr::<1>()); - self.remove_subtries(remove_0, remove_1, key0_starts_with, prune_limit < key.len(), key.len()); + self.remove_subtries(remove_0, remove_1, key0_starts_with, prune_limit < key.len(), key); if prune_limit > 0 && prune_limit < key_len && (remove_0 || remove_1) { self.preserve_prune_limit(key, prune_limit); } + debug_assert!(validate_node(self)); remove_0 || remove_1 } @@ -1972,7 +2003,7 @@ impl TrieNode for LineListNode remove_1 = prune_limit < key_len && self.is_used_child_1() && unsafe{ self.child_in_slot::<1>().is_empty() }; } } - self.remove_subtries(remove_0, remove_1, key0_starts_with, prune_limit < key.len(), key.len()); + self.remove_subtries(remove_0, remove_1, key0_starts_with, prune_limit < key.len(), key); if prune_limit > 0 && prune_limit < key_len && (remove_0 || remove_1) { self.preserve_prune_limit(key, prune_limit); } } @@ -2603,7 +2634,8 @@ impl TrieNode for LineListNode //Exact match with a path to a child node means take that node let (key0, key1) = self.get_both_keys(); if self.is_used_child_0() && key0 == key { - if prune_limit < key.len() { + if prune_limit < key.len() || + key.len() == 1 && self.is_used_value_1() && key1.len() == 1 && key1[0] == key[0] { return self.take_payload::<0>().map(|payload| payload.into_child()) } else { let child_payload = self.swap_payload::<0>(ValOrChild::Child(TrieNodeODRc::new_empty())); @@ -2611,7 +2643,8 @@ impl TrieNode for LineListNode } } if self.is_used_child_1() && key1 == key { - if prune_limit < key.len() { + if prune_limit < key.len() || + key.len() == 1 && self.is_used_value_0() && key0.len() == 1 && key0[0] == key[0] { return self.take_payload::<1>().map(|payload| payload.into_child()) } else { let child_payload = self.swap_payload::<1>(ValOrChild::Child(TrieNodeODRc::new_empty())); @@ -2636,7 +2669,8 @@ impl TrieNode for LineListNode if key1.len() > key.len() && starts_with(key1, key) { let mut new_node = Self::new_in(self.alloc.clone()); unsafe{ new_node.set_payload_0(&key1[key.len()..], self.is_child_ptr::<1>(), ValOrChildUnion{ _unused: () }) } - new_node.val_or_child0 = if prune_limit < key.len() { + new_node.val_or_child0 = if prune_limit < key.len() || + key.len() == 1 && self.is_used_value_0() && key0.len() == 1 && key0[0] == key[0] { self.take_payload::<1>().unwrap().into() } else { self.shorten_key_len::<1>(key.len()); @@ -2648,6 +2682,7 @@ impl TrieNode for LineListNode None })(); if result.is_some() { self.preserve_prune_limit(key, prune_limit); } + debug_assert!(validate_node(self)); result } @@ -2988,7 +3023,8 @@ impl LineListNode { } /// Part of the implementation of methods the remove subtries from a node - fn remove_subtries(&mut self, remove_0: bool, remove_1: bool, key0_starts_with: bool, prune: bool, key_len: usize) { + fn remove_subtries(&mut self, remove_0: bool, remove_1: bool, key0_starts_with: bool, prune: bool, key: &[u8]) { + let key_len = key.len(); //NOTE: the order here is important because removing slot_0 first might shift the // contents of slot_1, so we much deal with slot_1 first if remove_1 { @@ -3003,6 +3039,16 @@ impl LineListNode { if prune || key_len == 0 { self.take_payload::<0>(); } else { + // We need to prevent a node's trie from being shortened to a sentinel where there + // is already a value at that key + if key_len == 1 && self.is_used_value_1() { + let key1 = unsafe{ self.key_unchecked::<1>() }; + if key1.len() == 1 && key[0] == key1[0] { + self.take_payload::<0>(); + return + } + } + //Shortening key_0 won't ever change the sort order, so it's ok to assume we can stay in slot_0 self.shorten_key_len::<0>(key_len); self.swap_payload::<0>(ValOrChild::Child(TrieNodeODRc::new_empty())); @@ -3061,6 +3107,16 @@ pub(crate) fn validate_node(node: &LineLis panic!() } + // An empty onward link is a dangling sentinel. A value at the same key + // already keeps that path alive, so the sentinel must not be present. + if node.is_used::<1>() && key0 == key1 && ( + node.is_used_child_1() && unsafe{ node.child_in_slot::<1>().is_empty() } || + node.is_used_child_0() && unsafe{ node.child_in_slot::<0>().is_empty() } + ) { + println!("Invalid node - value and dangling sentinel under the same key. {node:?}"); + panic!() + } + // If two unequal keys share a prefix but neither is an ancestor of the // other, that prefix must be factored into an onward child. Otherwise a // virtual focus at the shared prefix spans both slots, while node-at-key @@ -3944,6 +4000,52 @@ mod tests { assert_eq!(by_index.child_mask(), by_path.child_mask()); } + /// Test node operations that might lead to a dangling sentinel overlapping a value, to make sure that + /// invalid state is never created + #[test] + fn value_and_dangling_sentinel_do_not_share_key() { + use crate::PathMap; + use crate::trie_node::assert_valid_trie; + use crate::zipper::*; + + let mut removed_branches = PathMap::::new(); + removed_branches.set_val_at(b"a", 1); + removed_branches.set_val_at(b"ab", 2); + let mut wz = removed_branches.write_zipper(); + wz.descend_to(b"a"); + assert!(wz.remove_branches(false)); + drop(wz); + assert_valid_trie(removed_branches.root()); + assert_eq!(removed_branches.val_at(b"a"), Some(&1)); + assert!(!removed_branches.path_exists_at(b"ab")); + + let mut taken = PathMap::::new(); + taken.set_val_at(b"a", 1); + taken.set_val_at(b"ab", 2); + let mut destination = PathMap::::new(); + destination.write_zipper().join_into_take(&mut taken.write_zipper_at_path(b"a"), true); + assert_valid_trie(taken.root()); + assert_eq!(taken.val_at(b"a"), Some(&1)); + assert!(!taken.path_exists_at(b"ab")); + + let mut dropped = PathMap::::new(); + dropped.set_val_at(b"ab", 1); + dropped.create_path(b"cb"); + dropped.write_zipper().join_k_path_into(1, false); + assert_valid_trie(dropped.root()); + assert_eq!(dropped.val_at(b"b"), Some(&1)); + + let mut dangling = PathMap::::new(); + dangling.create_path(b"a"); + let mut valued = PathMap::::new(); + valued.set_val_at(b"a", 1); + valued.set_val_at(b"ax", 2); + let joined = dangling.join(&valued); + assert_valid_trie(joined.root()); + assert_eq!(joined.val_at(b"a"), Some(&1)); + assert_eq!(joined.val_at(b"ax"), Some(&2)); + } + /// Issue #85: `restrict` panicked when a child-link slot was followed into `other` and /// landed on a dangling byte there (in the child mask, no node behind it). #[test] diff --git a/src/write_zipper.rs b/src/write_zipper.rs index 98bb3517..1472cdde 100644 --- a/src/write_zipper.rs +++ b/src/write_zipper.rs @@ -1474,14 +1474,11 @@ impl <'a, 'path, V: Clone + Send + Sync + Unpin, A: Allocator + 'a> WriteZipperC } let prune_limit = self.node_prune_limit(prune); let mut focus_node = self.focus_stack.top_mut().unwrap(); - if let Some(result) = focus_node.node_remove_val(self.key.node_key(), prune_limit) { - if prune { - self.prune_path_internal(false); - } - Some(result) - } else { - None + let result = focus_node.node_remove_val(self.key.node_key(), prune_limit); + if prune && (result.is_some() || self.focus_stack.top().unwrap().node_is_empty()) { + self.prune_path_internal(false); } + result } /// See [WriteZipper::zipper_head] pub fn zipper_head<'z>(&'z mut self) -> ZipperHead<'z, 'a, V, A> { @@ -2083,6 +2080,11 @@ impl <'a, 'path, V: Clone + Send + Sync + Unpin, A: Allocator + 'a> WriteZipperC } }; + if prune && node_was_none { + // No node operation ran at an already dangling focus. + self.prune_path(); + } + #[cfg(not(feature = "graft_root_vals"))] return node_status; #[cfg(feature = "graft_root_vals")] @@ -2202,6 +2204,11 @@ impl <'a, 'path, V: Clone + Send + Sync + Unpin, A: Allocator + 'a> WriteZipperC } }; + if prune && node_was_none { + // No node operation ran at an already dangling focus. + self.prune_path(); + } + #[cfg(not(feature = "graft_root_vals"))] return node_status; #[cfg(feature = "graft_root_vals")] @@ -2261,14 +2268,11 @@ impl <'a, 'path, V: Clone + Send + Sync + Unpin, A: Allocator + 'a> WriteZipperC if node_key.len() > 0 { let prune_limit = self.node_prune_limit(prune); let mut focus_node = self.focus_stack.top_mut().unwrap(); - if focus_node.node_remove_all_branches(node_key, prune_limit) { - if prune { - self.prune_path_internal(false); - } - true - } else { - false + let removed = focus_node.node_remove_all_branches(node_key, prune_limit); + if prune { + self.prune_path_internal(false); } + removed } else { debug_assert_eq!(self.focus_stack.depth(), 1); if self.focus_stack.top().map(|node| node.node_is_empty()).unwrap_or(false) { From 3dd98b64cb0158fc8899c4d90991f6e28e4c4510 Mon Sep 17 00:00:00 2001 From: Luke Peterson Date: Fri, 2 Oct 2026 09:40:10 -0600 Subject: [PATCH 3/6] Optimizations for join operation, ane making prune benchmarks more regularized (uncorrelated) --- benches/prune.rs | 105 +++++++++++++++--------------------------- src/line_list_node.rs | 65 ++++++++++++++++++++------ 2 files changed, 88 insertions(+), 82 deletions(-) diff --git a/benches/prune.rs b/benches/prune.rs index 595c4b17..c6713ba9 100644 --- a/benches/prune.rs +++ b/benches/prune.rs @@ -6,87 +6,56 @@ fn main() { Divan::from_args().main(); } -fn fixture(path: &[u8]) -> PathMap { - let mut map = PathMap::new(); - map.set_val_at(path, 1); - map -} - -fn run_remove_val(bencher: Bencher, path: &[u8], root_len: usize, prune: bool) { - bencher.with_inputs(|| fixture(path)).bench_local_values(|mut map| { - let mut wz = map.write_zipper_at_path(&path[..root_len]); - wz.descend_to(&path[root_len..]); - black_box(wz.remove_val(prune)); - }); -} - -fn run_remove_branches(bencher: Bencher, path: &[u8], root_len: usize, prune: bool) { - bencher.with_inputs(|| fixture(path)).bench_local_values(|mut map| { - let focus = &path[..path.len() - 1]; - let mut wz = map.write_zipper_at_path(&path[..root_len]); - wz.descend_to(&focus[root_len..]); - black_box(wz.remove_branches(prune)); - }); -} - -fn run_prune_path(bencher: Bencher, path: &[u8], root_len: usize) { +#[divan::bench] +fn prune_path(bencher: Bencher) { bencher.with_inputs(|| { let mut map = PathMap::::new(); - map.create_path(path); + map.create_path(b"abcd"); map }).bench_local_values(|mut map| { - let mut wz = map.write_zipper_at_path(&path[..root_len]); - wz.descend_to(&path[root_len..]); + let mut wz = map.write_zipper_at_path(b"ab"); + wz.descend_to(b"cd"); black_box(wz.prune_path()); }); } -#[divan::bench] -fn prune_path_short_root_at_map_root(bencher: Bencher) { - run_prune_path(bencher, b"abcd", 0); -} - -#[divan::bench] -fn prune_path_short_root_inside_node(bencher: Bencher) { - run_prune_path(bencher, b"abcd", 2); -} - -#[divan::bench] -fn prune_path_long_root_inside_node(bencher: Bencher) { - let path: Vec = (0..100).collect(); - run_prune_path(bencher, &path, 95); -} - -#[divan::bench(args = [false, true])] -fn remove_val_short(bencher: Bencher, prune: bool) { - run_remove_val(bencher, b"abcd", 2, prune); -} - #[divan::bench(args = [false, true])] -fn remove_val_long_root_above_node(bencher: Bencher, prune: bool) { - let path: Vec = (0..100).collect(); - run_remove_val(bencher, &path, 5, prune); -} - -#[divan::bench(args = [false, true])] -fn remove_val_long_root_inside_node(bencher: Bencher, prune: bool) { - let path: Vec = (0..100).collect(); - run_remove_val(bencher, &path, 95, prune); -} - -#[divan::bench(args = [false, true])] -fn remove_branches_short(bencher: Bencher, prune: bool) { - run_remove_branches(bencher, b"abcd", 2, prune); +fn remove_val(bencher: Bencher, prune: bool) { + bencher.with_inputs(|| { + let mut map = PathMap::::new(); + map.set_val_at(b"abcd", 1); + map + }).bench_local_values(|mut map| { + let mut wz = map.write_zipper_at_path(b"ab"); + wz.descend_to(b"cd"); + black_box(wz.remove_val(prune)); + }); } #[divan::bench(args = [false, true])] -fn remove_branches_long_root_above_node(bencher: Bencher, prune: bool) { - let path: Vec = (0..100).collect(); - run_remove_branches(bencher, &path, 5, prune); +fn remove_branches(bencher: Bencher, prune: bool) { + bencher.with_inputs(|| { + let mut map = PathMap::::new(); + map.set_val_at(b"abcd", 1); + map + }).bench_local_values(|mut map| { + let mut wz = map.write_zipper_at_path(b"ab"); + wz.descend_to(b"c"); + black_box(wz.remove_branches(prune)); + }); } #[divan::bench(args = [false, true])] -fn remove_branches_long_root_inside_node(bencher: Bencher, prune: bool) { - let path: Vec = (0..100).collect(); - run_remove_branches(bencher, &path, 95, prune); +fn take_map(bencher: Bencher, prune: bool) { + bencher.with_inputs(|| { + let mut map = PathMap::::new(); + map.set_val_at(b"a", 1); + map.set_val_at(b"ab", 2); + map.set_val_at(b"ac", 3); + map + }).bench_local_values(|mut map| { + let mut wz = map.write_zipper(); + wz.descend_to(b"a"); + black_box(wz.take_map(prune)); + }); } diff --git a/src/line_list_node.rs b/src/line_list_node.rs index 188104d4..3dcfe14c 100644 --- a/src/line_list_node.rs +++ b/src/line_list_node.rs @@ -464,6 +464,16 @@ impl LineListNode { } false } + + /// The other slot can hold a value at this child's key only for a one-byte key. + #[inline] + fn other_slot_has_value_at(&self, key: &[u8]) -> bool { + match SLOT { + 0 => self.is_used_value_1() && key.len() == 1 && self.key_len_1() == 1 && unsafe { self.key_unchecked::<1>()[0] } == key[0], + 1 => self.is_used_value_0() && key.len() == 1 && self.key_len_0() == 1 && unsafe { self.key_unchecked::<0>()[0] } == key[0], + _ => unreachable!(), + } + } fn get_val(&self, key: &[u8]) -> Option<&V> { if self.is_used_value_0() { let node_key_0 = unsafe{ self.key_unchecked::<0>() }; @@ -1343,6 +1353,12 @@ fn merge_guts<'a, V: Clone + Lattice + Send + Sync, A: Allocator, const ASLOT: u return match a_child.pjoin(b_child) { //Two empty children are both just the dangling path AlgebraicResult::None => AlgebraicResult::Identity(SELF_IDENT | COUNTER_IDENT), + // An empty child contributes only its path. If the other + // node also has a value there, its separate value pairing + // already preserves that path. + AlgebraicResult::Identity(mask) if + mask & COUNTER_IDENT > 0 && b.other_slot_has_value_at::(a_key) && a_child.is_empty() || + mask & SELF_IDENT > 0 && a.other_slot_has_value_at::(b_key) && b_child.is_empty() => AlgebraicResult::None, joined => joined.map(|new_child| (a_key, ValOrChild::Child(new_child))), } }, @@ -1374,6 +1390,7 @@ fn merge_guts<'a, V: Clone + Lattice + Send + Sync, A: Allocator, const ASLOT: u AlgebraicResult::Element(joined) => AlgebraicResult::Element((&a_key[0..overlap], ValOrChild::Child(joined))), //`b`'s child already held `a`'s payload, so `b`'s slot is the result AlgebraicResult::Identity(mask) if mask & SELF_IDENT > 0 => AlgebraicResult::Identity(COUNTER_IDENT), + AlgebraicResult::Identity(_) if a.other_slot_has_value_at::(b_key) && b_child.is_empty() => AlgebraicResult::None, AlgebraicResult::Identity(_) => AlgebraicResult::Element((&a_key[0..overlap], ValOrChild::Child(intermediate_node))), AlgebraicResult::None => unreachable!(), //`intermediate_node` is never empty } @@ -1389,6 +1406,7 @@ fn merge_guts<'a, V: Clone + Lattice + Send + Sync, A: Allocator, const ASLOT: u AlgebraicResult::Element(joined) => AlgebraicResult::Element((&a_key[0..overlap], ValOrChild::Child(joined))), //Mirror of the case above: `a`'s slot is the result AlgebraicResult::Identity(mask) if mask & SELF_IDENT > 0 => AlgebraicResult::Identity(SELF_IDENT), + AlgebraicResult::Identity(_) if b.other_slot_has_value_at::(a_key) && a_child.is_empty() => AlgebraicResult::None, AlgebraicResult::Identity(_) => AlgebraicResult::Element((&a_key[0..overlap], ValOrChild::Child(intermediate_node))), AlgebraicResult::None => unreachable!(), //`intermediate_node` is never empty } @@ -1437,25 +1455,16 @@ fn merge_list_nodes(a: &LineList let (self_key0, self_key1) = a.get_both_keys(); let (other_key0, other_key1) = b.get_both_keys(); - // A value subsumes a dangling sentinel at the same key. Ignore that - // sentinel in every pairing: otherwise it could also merge with another - // slot of the value's node. - let ignored = [ - a.is_used_child_0() && unsafe{ a.child_in_slot::<0>().is_empty() } && b.contains_val(self_key0), - a.is_used_child_1() && unsafe{ a.child_in_slot::<1>().is_empty() } && b.contains_val(self_key1), - b.is_used_child_0() && unsafe{ b.child_in_slot::<0>().is_empty() } && a.contains_val(other_key0), - b.is_used_child_1() && unsafe{ b.child_in_slot::<1>().is_empty() } && a.contains_val(other_key1), - ]; let mut entries: [MaybeUninit<(&[u8], ValOrChild)>; 4] = [MaybeUninit::uninit(), MaybeUninit::uninit(), MaybeUninit::uninit(), MaybeUninit::uninit()]; let mut entry_cnt = 0; - let mut used = ignored; //[self_0, self_1, other_0, other_1] + let mut used = [false; 4]; //[self_0, self_1, other_0, other_1] let mut identity_masks: [u64; 4] = [0; 4]; // Try each pairing in self and other, to see if there is a key-join that can happen // We can assume two keys in the same node can't merge, because they would have already been merged, // and therefore we can also assume that if a key can be merged with one key of a node it can't be // merged with the other - match if ignored[0] || ignored[2] { AlgebraicResult::None } else { try_merge::(self_key0, a, other_key0, b) } { + match try_merge::(self_key0, a, other_key0, b) { AlgebraicResult::Element(joined) => { entries[entry_cnt] = MaybeUninit::new(joined); entry_cnt += 1; @@ -1476,7 +1485,7 @@ fn merge_list_nodes(a: &LineList }, AlgebraicResult::None => { } } - match if ignored[0] || ignored[3] { AlgebraicResult::None } else { try_merge::(self_key0, a, other_key1, b) } { + match try_merge::(self_key0, a, other_key1, b) { AlgebraicResult::Element(joined) => { entries[entry_cnt] = MaybeUninit::new(joined); entry_cnt += 1; @@ -1499,7 +1508,7 @@ fn merge_list_nodes(a: &LineList }, AlgebraicResult::None => {} } - match if ignored[1] || ignored[2] { AlgebraicResult::None } else { try_merge::(self_key1, a, other_key0, b) } { + match try_merge::(self_key1, a, other_key0, b) { AlgebraicResult::Element(joined) => { entries[entry_cnt] = MaybeUninit::new(joined); entry_cnt += 1; @@ -1522,7 +1531,7 @@ fn merge_list_nodes(a: &LineList }, AlgebraicResult::None => {} } - match if ignored[1] || ignored[3] { AlgebraicResult::None } else { try_merge::(self_key1, a, other_key1, b) } { + match try_merge::(self_key1, a, other_key1, b) { AlgebraicResult::Element(joined) => { entries[entry_cnt] = MaybeUninit::new(joined); entry_cnt += 1; @@ -4046,6 +4055,34 @@ mod tests { assert_eq!(joined.val_at(b"ax"), Some(&2)); } + #[test] + fn join_dangling_child_with_value_and_descendants() { + use crate::PathMap; + use crate::trie_node::assert_valid_trie; + use crate::zipper::ZipperValuesAt; + + for (key, two_descendants) in [(b'a', false), (b'a', true), (b'z', false), (b'z', true)] { + let other = if key == b'a' { b'z' } else { b'a' }; + let mut dangling = PathMap::::new(); + dangling.create_path([key]); + dangling.set_val_at([other], 4); + + let mut valued = PathMap::::new(); + valued.set_val_at([key], 1); + valued.set_val_at([key, b'x'], 2); + if two_descendants { valued.set_val_at([key, b'y'], 3); } + + for (left, right) in [(&dangling, &valued), (&valued, &dangling)] { + let joined = left.join(right); + assert_valid_trie(joined.root()); + assert_eq!(joined.val_at([key]), Some(&1)); + assert_eq!(joined.val_at([key, b'x']), Some(&2)); + assert_eq!(joined.val_at([key, b'y']), two_descendants.then_some(&3)); + assert_eq!(joined.val_at([other]), Some(&4)); + } + } + } + /// Issue #85: `restrict` panicked when a child-link slot was followed into `other` and /// landed on a dangling byte there (in the child mask, no node behind it). #[test] From 5b461b2b7ff496ecefb6267e83a102a3c579284a Mon Sep 17 00:00:00 2001 From: Luke Peterson Date: Fri, 2 Oct 2026 09:59:03 -0600 Subject: [PATCH 4/6] Updating explanation of prune flag --- pathmap-book/src/1.02.06_zipper_writing.md | 2 +- src/trie_map.rs | 6 +++-- src/write_zipper.rs | 27 ++++++++++++++++++---- 3 files changed, 27 insertions(+), 8 deletions(-) diff --git a/pathmap-book/src/1.02.06_zipper_writing.md b/pathmap-book/src/1.02.06_zipper_writing.md index e3bc6f17..ae0fb04a 100644 --- a/pathmap-book/src/1.02.06_zipper_writing.md +++ b/pathmap-book/src/1.02.06_zipper_writing.md @@ -29,7 +29,7 @@ The [`ZipperWriting`] trait provides methods to modify the trie structure and va - [`remove_unmasked_branches`] selectively removes branches based on a byte mask ## Pruning Behavior -Many operations accept a `prune` parameter that controls whether dangling paths should be automatically cleaned up. When `prune` is `true`, the operation will remove any path segments that become empty, aka "dangling" (no values, and no further downstream branches) as a result of the modification. +Many operations accept a `prune` parameter. With `prune=false`, an emptied focus remains dangling. With `prune=true`, the result is the same as calling the operation with `false` and then [`prune_path`] at the focus; this also handles a pre-existing dangling tip. Pruning from the middle of a path with descendants does nothing. For [`join_into_take`], the flag applies to the source zipper's focus. Automatic pruning helps maintain a compact trie structure by removing unnecessary nodes, but can be counter-productive when you plan to perform additional operations that might reuse those paths. diff --git a/src/trie_map.rs b/src/trie_map.rs index 02dc69f4..37855a2d 100644 --- a/src/trie_map.rs +++ b/src/trie_map.rs @@ -362,7 +362,8 @@ impl PathMap { /// Removes the value at `path` from the map and returns it, or returns `None` if there was no value at `path` /// - /// If `prune` is `true`, the path will be pruned, otherwise it will be left dangling. + /// `prune=false` leaves `path` dangling if emptied; `true` applies [Self::prune_path] + /// afterward, including at a pre-existing dangling tip. See the [path guide](https://pathmap-rs.github.io/1.00.01_basics.html#creating-and-removing-paths). pub fn remove_val_at>(&mut self, path: K, prune: bool) -> Option { let path = path.as_ref(); //NOTE: we're descending the zipper rather than creating it at the path so it will be allowed to @@ -445,7 +446,8 @@ impl PathMap { /// /// Returns `true` if at least one branch was removed. /// - /// If `prune` is `true`, the path will be pruned, otherwise it will be left dangling. + /// `prune=false` leaves `path` dangling if emptied; `true` applies [Self::prune_path] + /// afterward, including at a pre-existing dangling tip. See the [path guide](https://pathmap-rs.github.io/1.00.01_basics.html#creating-and-removing-paths). pub fn remove_branches_at>(&mut self, path: K, prune: bool) -> bool { let path = path.as_ref(); //NOTE: we're descending the zipper rather than creating it at the path so it will be allowed to diff --git a/src/write_zipper.rs b/src/write_zipper.rs index 1472cdde..6043f92f 100644 --- a/src/write_zipper.rs +++ b/src/write_zipper.rs @@ -69,7 +69,8 @@ pub trait ZipperWriting: Wri /// Removes the value at the zipper's focus. Does not affect any onward branches. Returns `Some(val)` /// with the value that was removed, otherwise returns `None` /// - /// Pass `true` to the `prune` argument to automatically remove any dangling path created by this operation. + /// `prune=false` leaves an emptied focus dangling; `true` applies [Self::prune_path] + /// afterward, including at a pre-existing dangling tip. See the [pruning guide](https://pathmap-rs.github.io/1.02.06_zipper_writing.html#pruning-behavior). fn remove_val(&mut self, prune: bool) -> Option; /// Deprecated alias for [ZipperWriting::remove_val] @@ -190,7 +191,8 @@ pub trait ZipperWriting: Wri /// Joins the subtrie below the focus of `src_zipper` into the subtrie below the focus of `self`, /// consuming the subtrie from the `src_zipper` /// - /// Pass `true` to the `prune` argument to automatically remove any dangling path created in `src_zipper`. + /// `prune=false` leaves an emptied source focus dangling; `true` applies + /// `src_zipper.prune_path()` afterward, including at a pre-existing dangling tip. See the [pruning guide](https://pathmap-rs.github.io/1.02.06_zipper_writing.html#pruning-behavior). fn join_into_take + ZipperWriting>(&mut self, src_zipper: &mut Z, prune: bool) -> AlgebraicStatus where V: Lattice; /// Collapses all the paths below the zipper's focus by removing the leading `byte_cnt` bytes from @@ -208,12 +210,18 @@ pub trait ZipperWriting: Wri /// Returns `true` if the focus has at least one downstream continuation, otherwise returns `false`. /// /// NOTE: for legacy reasons, this operation is sometimes called `drop_head` + /// + /// `prune=false` leaves an emptied focus dangling; `true` applies [Self::prune_path] + /// afterward, including at a pre-existing dangling tip. See the [pruning guide](https://pathmap-rs.github.io/1.02.06_zipper_writing.html#pruning-behavior). fn join_k_path_into(&mut self, byte_cnt: usize, prune: bool) -> bool where V: Lattice; /// Collapses all the paths below the zipper's focus by removing the leading `byte_cnt` bytes from /// each path and meets together all of the downstream subtries /// /// Returns `true` if the focus has at least one downstream continuation, otherwise returns `false`. + /// + /// `prune=false` leaves an emptied focus dangling; `true` applies [Self::prune_path] + /// afterward, including at a pre-existing dangling tip. See the [pruning guide](https://pathmap-rs.github.io/1.02.06_zipper_writing.html#pruning-behavior). fn meet_k_path_into(&mut self, byte_cnt: usize, prune: bool) -> bool where V: Lattice; /// Deprecated alias for [ZipperWriting::join_k_path_into] @@ -245,6 +253,9 @@ pub trait ZipperWriting: Wri /// Meets (retains the intersection of) the subtrie below the zipper's focus with the subtrie downstream /// from the focus of `read_zipper` + /// + /// `prune=false` leaves an emptied focus dangling; `true` applies [Self::prune_path] + /// afterward, including at a pre-existing dangling tip. See the [pruning guide](https://pathmap-rs.github.io/1.02.06_zipper_writing.html#pruning-behavior). fn meet_into>(&mut self, read_zipper: &Z, prune: bool) -> AlgebraicStatus where V: Lattice; /// Deprecated alias for [ZipperWriting::meet_into]. May be replaced in the future with a different method @@ -263,6 +274,9 @@ pub trait ZipperWriting: Wri /// Subtracts the subtrie downstream of the focus of `read_zipper` from the subtrie below the `self` zipper's /// focus + /// + /// `prune=false` leaves an emptied focus dangling; `true` applies [Self::prune_path] + /// afterward, including at a pre-existing dangling tip. See the [pruning guide](https://pathmap-rs.github.io/1.02.06_zipper_writing.html#pruning-behavior). fn subtract_into>(&mut self, read_zipper: &Z, prune: bool) -> AlgebraicStatus where V: DistributiveLattice; /// Deprecated alias for [ZipperWriting::subtract_into] @@ -292,7 +306,8 @@ pub trait ZipperWriting: Wri /// Creates a new [PathMap] from the zipper's focus, removing all downstream branches from the zipper /// - /// Pass `true` to the `prune` argument to automatically remove any dangling path created by this operation. + /// `prune=false` leaves an emptied focus dangling; `true` applies [Self::prune_path] + /// afterward, including at a pre-existing dangling tip. See the [pruning guide](https://pathmap-rs.github.io/1.02.06_zipper_writing.html#pruning-behavior). /// /// GOAT: This method's behavior is affected by the `graft_root_vals` feature /// A value at the zipper's focus will not be affected, and will not be included in the resulting map. @@ -302,14 +317,16 @@ pub trait ZipperWriting: Wri /// Removes all branches below the zipper's focus. Does not affect the value if there is one. Returns `true` /// if a branch was removed, otherwise returns `false` /// - /// Pass `true` to the `prune` argument to automatically remove any dangling path created by this operation. + /// `prune=false` leaves an emptied focus dangling; `true` applies [Self::prune_path] + /// afterward, including at a pre-existing dangling tip. See the [pruning guide](https://pathmap-rs.github.io/1.02.06_zipper_writing.html#pruning-behavior). fn remove_branches(&mut self, prune: bool) -> bool; /// Removes multiple branches below the zipper's focus based on the supplied 256-bit `mask` /// /// Key bytes for which the corresponding `mask` bit is `0` will be removed. /// - /// Pass `true` to the `prune` argument to automatically remove any dangling path created by this operation. + /// `prune=false` leaves an emptied focus dangling; `true` applies [Self::prune_path] + /// afterward, including at a pre-existing dangling tip. See the [pruning guide](https://pathmap-rs.github.io/1.02.06_zipper_writing.html#pruning-behavior). fn remove_unmasked_branches(&mut self, mask: ByteMask, prune: bool); /// Creates a dangling path to the current zipper focus. Returns `true` if new path bytes were created, or From c021a5c12c0954c5be231a5c4c47a2abc37a3b0a Mon Sep 17 00:00:00 2001 From: Luke Peterson Date: Fri, 2 Oct 2026 23:03:25 -0600 Subject: [PATCH 5/6] Fixing another prune flag failure, this time in remove_val --- src/line_list_node.rs | 86 +++++++++++++++++++++++++------------------ src/write_zipper.rs | 21 ++++++++++- 2 files changed, 71 insertions(+), 36 deletions(-) diff --git a/src/line_list_node.rs b/src/line_list_node.rs index 3dcfe14c..9c637c79 100644 --- a/src/line_list_node.rs +++ b/src/line_list_node.rs @@ -1851,48 +1851,64 @@ impl TrieNode for LineListNode //Removing a value is one of the ways a node can be left holding two onward children // under one key, so check the node over on the way out debug_assert!(validate_node(self)); - let (key0, key1) = self.get_both_keys(); - let value0 = self.is_used_value_0() && key0 == key; - let value1 = self.is_used_value_1() && key1 == key; - let can_prune = prune_limit < key.len(); - let check_dangling = can_prune && !value0 && !value1; - let dangling0 = check_dangling && self.is_used_child_0() && key0 == key && unsafe{ self.child_in_slot::<0>().is_empty() }; - let dangling1 = check_dangling && self.is_used_child_1() && key1 == key && unsafe{ self.child_in_slot::<1>().is_empty() }; - - let (result, removed) = if value0 || dangling0 { - if can_prune { - let payload = self.take_payload::<0>().unwrap(); - (match payload { ValOrChild::Val(val) => Some(val), ValOrChild::Child(_) => None }, true) - } else { - let overlap = find_prefix_overlap(key0, key1); - let val = if key0.len() == overlap { - self.take_payload::<0>().unwrap().into_val() - } else { - self.swap_payload::<0>(ValOrChild::Child(TrieNodeODRc::new_empty())).into_val() - }; - (Some(val), true) + if prune_limit >= key.len() { + // Valid list-node keys share at most one leading byte. In this + // path we only need to ask whether the other slot keeps the focus. + if self.is_used_value_0() { + let key0 = unsafe{ self.key_unchecked::<0>() }; + if key0 == key { + let other_keeps_path = key0.len() == 1 && self.is_used::<1>() && + unsafe{ self.key_unchecked::<1>() }.first() == key0.first(); + let result = if other_keeps_path { + self.take_payload::<0>().unwrap().into_val() + } else { + self.swap_payload::<0>(ValOrChild::Child(TrieNodeODRc::new_empty())).into_val() + }; + debug_assert!(validate_node(self)); + return Some(result) + } } - } else if value1 || dangling1 { - if can_prune { - let payload = self.take_payload::<1>().unwrap(); - (match payload { ValOrChild::Val(val) => Some(val), ValOrChild::Child(_) => None }, true) - } else { - let overlap = find_prefix_overlap(key1, key0); - let val = if key1.len() == overlap { - self.take_payload::<1>().unwrap().into_val() - } else { - self.swap_payload::<1>(ValOrChild::Child(TrieNodeODRc::new_empty())).into_val() - }; - (Some(val), true) + if self.is_used_value_1() { + let key1 = unsafe{ self.key_unchecked::<1>() }; + if key1 == key { + let other_keeps_path = key1.len() == 1 && + unsafe{ self.key_unchecked::<0>() }.first() == key1.first(); + let result = if other_keeps_path { + self.take_payload::<1>().unwrap().into_val() + } else { + self.swap_payload::<1>(ValOrChild::Child(TrieNodeODRc::new_empty())).into_val() + }; + debug_assert!(validate_node(self)); + return Some(result) + } } + return None + } + + // Prefer a value if both slots have the same key. Compare each + // candidate key once, then consider an empty child only if no value matched. + let (key0, key1) = self.get_both_keys(); + let key0_matches = self.is_used::<0>() && key0 == key; + let slot = if key0_matches && self.is_used_value_0() { + 0 } else { - (None, false) + let key1_matches = self.is_used::<1>() && key1 == key; + if key1_matches && self.is_used_value_1() { + 1 + } else if key0_matches && self.is_used_child_0() && unsafe{ self.child_in_slot::<0>().as_tagged().node_is_empty() } { + 0 + } else if key1_matches && self.is_used_child_1() && unsafe{ self.child_in_slot::<1>().as_tagged().node_is_empty() } { + 1 + } else { + return None + } }; - if prune_limit > 0 && can_prune && removed { + let payload = if slot == 0 { self.take_payload::<0>().unwrap() } else { self.take_payload::<1>().unwrap() }; + if prune_limit > 0 { self.preserve_prune_limit(key, prune_limit); } debug_assert!(validate_node(self)); - result + match payload { ValOrChild::Val(val) => Some(val), ValOrChild::Child(_) => None } } #[inline] diff --git a/src/write_zipper.rs b/src/write_zipper.rs index 6043f92f..2bfddf3a 100644 --- a/src/write_zipper.rs +++ b/src/write_zipper.rs @@ -1492,7 +1492,7 @@ impl <'a, 'path, V: Clone + Send + Sync + Unpin, A: Allocator + 'a> WriteZipperC let prune_limit = self.node_prune_limit(prune); let mut focus_node = self.focus_stack.top_mut().unwrap(); let result = focus_node.node_remove_val(self.key.node_key(), prune_limit); - if prune && (result.is_some() || self.focus_stack.top().unwrap().node_is_empty()) { + if prune { self.prune_path_internal(false); } result @@ -7553,4 +7553,23 @@ mod tests { (vec![0, 0, 0, 0], None), (vec![0, 0, 0, 0, 0], Some(0)), ]); } + + #[test] + fn write_zipper_prune_after_graft_masked() { + let mut map = PathMap::::new(); + map.set_val_at(&[0, 0], 0); + // [] (no value) -> [0] (no value) -> [0, 0] (value 0). + let empty = PathMap::::new(); + let rz = empty.read_zipper(); + let mut wz = map.write_zipper(); + wz.descend_to_byte(0); + // Focus is [0]; the trie still has the value at [0, 0]. + wz.graft_masked_branches(&rz, ByteMask::from_iter([0, 1, 3]), true); + // The empty source replaces the downstream subtrie + // The focus is still [0], but now it should be dangling, with no downstream values or children. + assert!(wz.path_exists(), "graft should leave the focus dangling"); + assert_eq!(wz.remove_val(true), None); + // remove_val has no value to remove, but the focus should be the tip of a dangling path, which should be pruned + assert!(!wz.path_exists(), "remove_val(true) should prune the dangling focus"); + } } From c268e61ed9d0f85f8b562f071e85f90570c43fd3 Mon Sep 17 00:00:00 2001 From: Luke Peterson Date: Fri, 2 Oct 2026 23:34:00 -0600 Subject: [PATCH 6/6] Fixing bug in remove_branches return value --- src/dense_byte_node.rs | 5 +- src/line_list_node.rs | 26 +++++++- src/write_zipper.rs | 136 +++++++++++++++++++++++++++++++++++++++-- 3 files changed, 160 insertions(+), 7 deletions(-) diff --git a/src/dense_byte_node.rs b/src/dense_byte_node.rs index 109d0e73..c80f90f6 100644 --- a/src/dense_byte_node.rs +++ b/src/dense_byte_node.rs @@ -1003,10 +1003,11 @@ impl> TrieNode if self.mask.test_bit(k) { let ix = self.mask.index_of(k) as usize; let cf = unsafe { self.values.get_unchecked_mut(ix) }; + let had_branches = cf.rec().is_some_and(|node| !node.as_tagged().node_is_empty()); match (cf.has_rec(), cf.has_val()) { (true, true) => { cf.set_rec_option(None); - true + had_branches }, (true, false) => { if prune_limit == 0 { @@ -1015,7 +1016,7 @@ impl> TrieNode } else { cf.set_rec_option(None); } - true + had_branches }, (false, false) if prune_limit == 0 => { self.values.remove(ix); diff --git a/src/line_list_node.rs b/src/line_list_node.rs index 9c637c79..aa85da28 100644 --- a/src/line_list_node.rs +++ b/src/line_list_node.rs @@ -1998,10 +1998,12 @@ impl TrieNode for LineListNode let key0_starts_with = starts_with(key0, key); let remove_0 = key0_starts_with && (key0.len() > key_len || self.is_child_ptr::<0>()); let remove_1 = starts_with(key1, key) && (key1.len() > key_len || self.is_child_ptr::<1>()); + let had_branches = (remove_0 && (key0.len() > key_len || !unsafe{ self.child_in_slot::<0>().as_tagged().node_is_empty() })) || + (remove_1 && (key1.len() > key_len || !unsafe{ self.child_in_slot::<1>().as_tagged().node_is_empty() })); self.remove_subtries(remove_0, remove_1, key0_starts_with, prune_limit < key.len(), key); if prune_limit > 0 && prune_limit < key_len && (remove_0 || remove_1) { self.preserve_prune_limit(key, prune_limit); } debug_assert!(validate_node(self)); - remove_0 || remove_1 + had_branches } fn node_remove_unmasked_branches(&mut self, key: &[u8], mask: ByteMask, prune_limit: usize) { @@ -3194,6 +3196,28 @@ mod tests { use crate::alloc::{global_alloc, Allocator, GlobalAlloc}; use super::*; + #[test] + fn remove_branches_at_dangling_child_reports_no_branches() { + for allocated_child in [false, true] { + for prune_limit in [usize::MAX, 0] { + let mut node = LineListNode::::new_in(global_alloc()); + node.node_set_val(b"b", 7).unwrap_or_else(|_| panic!()); + if allocated_child { + let empty = LineListNode::::new_in(global_alloc()); + assert!(node.node_set_branch(b"a", TrieNodeODRc::new_in(empty, global_alloc())).is_ok()); + } else { + assert!(node.node_create_dangling(b"a").is_ok()); + } + assert!(node.node_contains_partial_key(b"a")); + assert_eq!(crate::trie_node::node_count_branches_recursive(node.as_tagged(), b"a"), 0); + + assert!(!node.node_remove_all_branches(b"a", prune_limit), "allocated_child={allocated_child}, prune_limit={prune_limit}"); + assert_eq!(node.node_contains_partial_key(b"a"), prune_limit != 0); + assert_eq!(node.node_get_val(b"b"), Some(&7)); + } + } + } + fn get_recursive<'a, 'b, V: Clone + Send + Sync, A: Allocator + 'b>(key: &'a [u8], node: TaggedNodeRef<'b, V, A>) -> (&'a [u8], TaggedNodeRef<'b, V, A>, usize) { let mut remaining_key = key; let mut child_node = node; diff --git a/src/write_zipper.rs b/src/write_zipper.rs index 2bfddf3a..e1e62aa3 100644 --- a/src/write_zipper.rs +++ b/src/write_zipper.rs @@ -2642,10 +2642,7 @@ impl <'a, 'path, V: Clone + Send + Sync + Unpin, A: Allocator + 'a> WriteZipperC }; let prune_limit = if stopped_at_zipper_root { root_len.saturating_sub(node_key_start + consumed) } else { 0 }; - let removed = container_node.node_remove_all_branches(next_node_key, prune_limit); - - //If we got here, we should have either removed something, or we should be at the top of the zipper - debug_assert!(removed || self.focus_stack.depth()==1); + container_node.node_remove_all_branches(next_node_key, prune_limit); } debug_assert!(temp_path.len() >= root_len); let pruned_bytes = path_buf.len() - temp_path.len(); @@ -7572,4 +7569,135 @@ mod tests { // remove_val has no value to remove, but the focus should be the tip of a dangling path, which should be pruned assert!(!wz.path_exists(), "remove_val(true) should prune the dangling focus"); } + + /// A masked graft can leave an allocated but empty node at the focus. Each + /// prune-aware operation must handle that state, regardless of the shape of + /// the focus and its ancestors. + #[test] + fn write_zipper_prune_flag_after_masked_graft_shapes() { + let empty = PathMap::::new(); + let focus = [10u8, 20]; + let cases: &[(&str, &[u8], &[u8], &[u8], bool)] = &[ + ("single child, list parent", &[0], &[21], &[30], false), + ("two children, list parent", &[0, 1], &[21], &[30], false), + ("four children, dense parent", &[0, 1, 2, 3], &[21, 22, 23], &[30, 31, 32], true), + ]; + let ops = [ + "remove_val", "remove_branches", "remove_unmasked_branches", "take_map", + "meet_into", "subtract_into", + ]; + + for &(shape, children, siblings, root_siblings, dense_parent) in cases { + let mut original = PathMap::::new(); + for &child in children { + original.set_val_at(&[10, 20, child], child as u64); + } + for &sibling in siblings { + original.set_val_at(&[10, sibling], sibling as u64); + } + for &sibling in root_siblings { + original.set_val_at(&[sibling], sibling as u64); + } + + let root = original.root().unwrap().as_tagged(); + let (consumed, parent) = root.node_get_child(&[10]).unwrap(); + assert_eq!(consumed, 1, "{shape}"); + if dense_parent { + assert!(root.as_dense().is_some(), "{shape}: root should be dense"); + assert!(parent.as_tagged().as_dense().is_some(), "{shape}: focus parent should be dense"); + } else { + #[cfg(not(feature = "all_dense_nodes"))] + { + assert!(root.as_list().is_some(), "{shape}: root should be a list"); + assert!(parent.as_tagged().as_list().is_some(), "{shape}: focus parent should be a list"); + } + } + + let mut expected = PathMap::::new(); + for &sibling in siblings { + expected.set_val_at(&[10, sibling], sibling as u64); + } + for &sibling in root_siblings { + expected.set_val_at(&[sibling], sibling as u64); + } + + for root_len in [0, 1] { + for (op_idx, op) in ops.iter().enumerate() { + let run = |prune: bool| { + let mut map = original.clone(); + { + let mut wz = map.write_zipper_at_path(&focus[..root_len]); + wz.descend_to(&focus[root_len..]); + assert!(wz.path_exists(), "{shape}, {op}: missing focus before graft"); + assert_eq!(wz.child_count(), children.len(), "{shape}, {op}: wrong starting shape"); + + wz.graft_masked_branches( + &empty.read_zipper(), + ByteMask::from_iter([0, 1, 2, 3]), + true, + ); + assert!(wz.path_exists(), "{shape}, {op}: graft removed focus"); + assert!(!wz.is_val(), "{shape}, {op}: graft added a value"); + assert_eq!(wz.child_count(), 0, "{shape}, {op}: graft left children"); + + match op_idx { + 0 => { assert_eq!(wz.remove_val(prune), None); }, + 1 => { wz.remove_branches(prune); }, + 2 => { wz.remove_unmasked_branches(ByteMask::EMPTY, prune); }, + 3 => { wz.take_map(prune); }, + 4 => { wz.meet_into(&empty.read_zipper(), prune); }, + 5 => { wz.subtract_into(&empty.read_zipper(), prune); }, + _ => unreachable!(), + } + if !prune { wz.prune_path(); } + assert!(!wz.path_exists(), "{shape}, {op}, prune={prune}: dangling focus survived"); + } + assert_valid_trie(map.root()); + all_locations(&map) + }; + let with_flag = run(true); + assert_eq!(with_flag, run(false), "{shape}, {op}, root_len={root_len}"); + assert_eq!(with_flag, all_locations(&expected), "{shape}, {op}, root_len={root_len}"); + } + } + } + } + + #[test] + fn remove_branches_at_dangling_focus_reports_no_branches() { + for (shape, siblings, allocated_child) in [ + ("list sentinel", &[1u8][..], false), + ("list allocated child", &[1u8][..], true), + ("dense sentinel", &[1u8, 2, 3][..], false), + ("dense allocated child", &[1u8, 2, 3][..], true), + ] { + for prune in [false, true] { + let mut map = PathMap::::new(); + if allocated_child { + map.set_val_at(&[0, 4], 4); + } else { + map.create_path([0]); + } + for &sibling in siblings { + map.set_val_at(&[sibling], sibling as u64); + } + let empty = PathMap::::new(); + let mut wz = map.write_zipper(); + wz.descend_to_byte(0); + if allocated_child { + wz.graft_masked_branches(&empty.read_zipper(), ByteMask::from_iter([4, 5, 6]), true); + } + assert!(wz.path_exists(), "{shape}"); + assert!(!wz.is_val(), "{shape}"); + assert_eq!(wz.child_count(), 0, "{shape}"); + + assert!(!wz.remove_branches(prune), "{shape}, prune={prune}: no branches existed"); + assert_eq!(wz.path_exists(), !prune, "{shape}, prune={prune}"); + drop(wz); + for &sibling in siblings { + assert_eq!(map.val_at(&[sibling]), Some(&(sibling as u64)), "{shape}"); + } + } + } + } }