From ef95f4394ca2640280eabc16157bead1f4470c8c Mon Sep 17 00:00:00 2001 From: Igor Malovitsa Date: Wed, 16 Sep 2026 23:24:19 +0000 Subject: [PATCH 1/2] Fix ACTZipper sibling steps and ascend_until off the trie to_sibling answered None for a focus one byte off the trie, so to_next_step gave up instead of moving to the next sibling. ascend_until then checked the landing node rather than the focus for a value or branch, stopping at the wrong ancestor. --- src/arena_compact.rs | 103 ++++++++++++++++++++++++++++++++++++++----- 1 file changed, 92 insertions(+), 11 deletions(-) diff --git a/src/arena_compact.rs b/src/arena_compact.rs index c05f9af5..50d35f67 100644 --- a/src/arena_compact.rs +++ b/src/arena_compact.rs @@ -2668,17 +2668,9 @@ where Storage: AsRef<[u8]> return start_len - self.path.len(); } - match &self.cur_node { - Node::Line(line) => { - if need_value && line.value.is_some() { - return start_len - self.path.len(); - } - } - Node::Branch(node) => { - if need_value && node.value.is_some() { - return start_len - self.path.len(); - } - } + //The deepest real ancestor stops the ascent if the *focus* has a value (when needed) or branches + if (need_value && self.is_val()) || self.child_count() > 1 { + return start_len - self.path.len(); } } while let Some(top_frame) = self.stack.last_mut() { @@ -4232,4 +4224,93 @@ mod tests { assert_eq!(az.val(), None); assert!(!az.path_exists()); } + + /// `ACTZipper` sibling steps from a focus off the trie + #[test] + fn act_zipper_sibling_step_from_an_off_trie_focus() { + use crate::zipper::*; + let mut m = PathMap::::new(); + { let mut w = m.write_zipper(); w.set_val(38); } + m.insert(&[1u8], 5); + m.insert(&[1u8, 0, 2], 22); + m.insert(&[3u8], 7); + let t = ArenaCompactTree::from_zipper(m.read_zipper(), |&v| v); + + //One byte off the trie, with a sibling on either side + let mut az = t.read_zipper_u64(); + az.descend_to(&[2u8]); + assert!(!az.path_exists()); + assert_eq!(az.to_next_sibling_byte(), Some(3)); + assert_eq!(az.path(), &[3u8]); + assert_eq!(az.val(), Some(&7)); + az.ascend(1); + az.descend_to(&[2u8]); + assert_eq!(az.to_prev_sibling_byte(), Some(1)); + assert_eq!(az.path(), &[1u8]); + assert_eq!(az.val(), Some(&5)); + + //No sibling on that side: the zipper stays where it was + let mut az = t.read_zipper_u64(); + az.descend_to(&[0u8]); + assert_eq!(az.to_prev_sibling_byte(), None); + assert_eq!(az.path(), &[0u8]); + assert!(!az.path_exists()); + assert_eq!(az.to_next_sibling_byte(), Some(1)); + + //Two bytes off the trie: the parent is not real, so there is no sibling + let mut az = t.read_zipper_u64(); + az.descend_to(&[2u8, 0]); + assert_eq!(az.to_next_sibling_byte(), None); + assert_eq!(az.path(), &[2u8, 0]); + + //`to_next_step` from an off-trie focus visits what follows it + let mut az = t.read_zipper_u64(); + az.descend_to(&[0u8]); + let mut seen = Vec::new(); + while az.to_next_step() { seen.push(az.path().to_vec()); } + assert_eq!(seen, vec![vec![1u8], vec![1, 0], vec![1, 0, 2], vec![3]]); + } + + /// `ascend_until` / `ascend_until_branch` from a focus off the trie stop at the right ancestor + #[test] + fn act_zipper_ascend_until_from_an_off_trie_focus() { + use crate::zipper::*; + let mut m = PathMap::::new(); + m.insert(&[1u8, 2, 3, 4], 11); //a line under 01, its value at the line's end + m.insert(&[5u8, 2], 22); //a branch at 05, two children, no value + m.insert(&[5u8, 6], 33); + m.insert(&[7u8], 44); //a value at 07, which branches below it as well + m.insert(&[7u8, 8], 55); + m.insert(&[7u8, 9], 66); + let t = ArenaCompactTree::from_zipper(m.read_zipper(), |&v| v); + + let off_trie: [&[u8]; 9] = [&[1, 2, 9], &[1, 2, 3, 9], &[1, 2, 3, 4, 9], &[1, 9, 9], + &[5, 9], &[5, 2, 9], &[7, 9, 9], &[7, 8, 9, 9], &[9]]; + for root in [&[][..], &[1u8], &[1, 2], &[7]] { + for focus in off_trie { + if !focus.starts_with(root) { continue } + let focus = &focus[root.len()..]; + for need_value in [false, true] { + let mut az = t.read_zipper_at_path_u64(root); + let mut pz = m.read_zipper_at_path(root); + assert_eq!(az.descend_to(focus), pz.descend_to(focus)); + assert!(!az.path_exists() && !pz.path_exists(), "{root:?} {focus:?}"); + let (a, p) = if need_value { + (az.ascend_until(), pz.ascend_until()) + } else { + (az.ascend_until_branch(), pz.ascend_until_branch()) + }; + assert_eq!(a, p, "root {root:?} focus {focus:?} need_value {need_value}"); + assert_eq!(az.path(), pz.path(), "root {root:?} focus {focus:?}"); + //Check the focus, then move + assert_eq!(az.path_exists(), pz.path_exists(), "{root:?} {focus:?}"); + assert_eq!(az.val(), pz.val(), "{root:?} {focus:?}"); + assert_eq!(az.child_count(), pz.child_count(), "{root:?} {focus:?}"); + assert_eq!(az.descend_first_byte(), pz.descend_first_byte(), "{root:?} {focus:?}"); + assert_eq!(az.path(), pz.path(), "{root:?} {focus:?} after descend"); + assert_eq!(az.val(), pz.val(), "{root:?} {focus:?} after descend"); + } + } + } + } } From 5bf73e811f91b05158c58bae791df052aeb50de4 Mon Sep 17 00:00:00 2001 From: Luke Peterson Date: Sat, 19 Sep 2026 02:44:24 -0600 Subject: [PATCH 2/2] Factoring arena_compact test to lift it up to ZipperMoving test macro --- src/arena_compact.rs | 42 --------------------- src/zipper.rs | 90 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 90 insertions(+), 42 deletions(-) diff --git a/src/arena_compact.rs b/src/arena_compact.rs index 50d35f67..70397698 100644 --- a/src/arena_compact.rs +++ b/src/arena_compact.rs @@ -4271,46 +4271,4 @@ mod tests { assert_eq!(seen, vec![vec![1u8], vec![1, 0], vec![1, 0, 2], vec![3]]); } - /// `ascend_until` / `ascend_until_branch` from a focus off the trie stop at the right ancestor - #[test] - fn act_zipper_ascend_until_from_an_off_trie_focus() { - use crate::zipper::*; - let mut m = PathMap::::new(); - m.insert(&[1u8, 2, 3, 4], 11); //a line under 01, its value at the line's end - m.insert(&[5u8, 2], 22); //a branch at 05, two children, no value - m.insert(&[5u8, 6], 33); - m.insert(&[7u8], 44); //a value at 07, which branches below it as well - m.insert(&[7u8, 8], 55); - m.insert(&[7u8, 9], 66); - let t = ArenaCompactTree::from_zipper(m.read_zipper(), |&v| v); - - let off_trie: [&[u8]; 9] = [&[1, 2, 9], &[1, 2, 3, 9], &[1, 2, 3, 4, 9], &[1, 9, 9], - &[5, 9], &[5, 2, 9], &[7, 9, 9], &[7, 8, 9, 9], &[9]]; - for root in [&[][..], &[1u8], &[1, 2], &[7]] { - for focus in off_trie { - if !focus.starts_with(root) { continue } - let focus = &focus[root.len()..]; - for need_value in [false, true] { - let mut az = t.read_zipper_at_path_u64(root); - let mut pz = m.read_zipper_at_path(root); - assert_eq!(az.descend_to(focus), pz.descend_to(focus)); - assert!(!az.path_exists() && !pz.path_exists(), "{root:?} {focus:?}"); - let (a, p) = if need_value { - (az.ascend_until(), pz.ascend_until()) - } else { - (az.ascend_until_branch(), pz.ascend_until_branch()) - }; - assert_eq!(a, p, "root {root:?} focus {focus:?} need_value {need_value}"); - assert_eq!(az.path(), pz.path(), "root {root:?} focus {focus:?}"); - //Check the focus, then move - assert_eq!(az.path_exists(), pz.path_exists(), "{root:?} {focus:?}"); - assert_eq!(az.val(), pz.val(), "{root:?} {focus:?}"); - assert_eq!(az.child_count(), pz.child_count(), "{root:?} {focus:?}"); - assert_eq!(az.descend_first_byte(), pz.descend_first_byte(), "{root:?} {focus:?}"); - assert_eq!(az.path(), pz.path(), "{root:?} {focus:?} after descend"); - assert_eq!(az.val(), pz.val(), "{root:?} {focus:?} after descend"); - } - } - } - } } diff --git a/src/zipper.rs b/src/zipper.rs index 1506e55b..fcede695 100644 --- a/src/zipper.rs +++ b/src/zipper.rs @@ -3725,6 +3725,30 @@ pub(crate) mod zipper_moving_tests { crate::zipper::zipper_moving_tests::run_test(&mut temp_store, $make_z, &[], crate::zipper::zipper_moving_tests::zipper_ascend_until_test5) } + #[test] + fn [<$z_name _ascend_until_from_off_trie_root>]() { + let mut temp_store = $read_keys(crate::zipper::zipper_moving_tests::OFF_TRIE_ASCEND_KEYS); + crate::zipper::zipper_moving_tests::run_test(&mut temp_store, $make_z, &[], crate::zipper::zipper_moving_tests::ascend_until_from_off_trie_root) + } + + #[test] + fn [<$z_name _ascend_until_from_off_trie_line_root>]() { + let mut temp_store = $read_keys(crate::zipper::zipper_moving_tests::OFF_TRIE_ASCEND_KEYS); + crate::zipper::zipper_moving_tests::run_test(&mut temp_store, $make_z, &[1], crate::zipper::zipper_moving_tests::ascend_until_from_off_trie_line_root) + } + + #[test] + fn [<$z_name _ascend_until_from_off_trie_mid_line_root>]() { + let mut temp_store = $read_keys(crate::zipper::zipper_moving_tests::OFF_TRIE_ASCEND_KEYS); + crate::zipper::zipper_moving_tests::run_test(&mut temp_store, $make_z, &[1, 2], crate::zipper::zipper_moving_tests::ascend_until_from_off_trie_mid_line_root) + } + + #[test] + fn [<$z_name _ascend_until_from_off_trie_value_branch_root>]() { + let mut temp_store = $read_keys(crate::zipper::zipper_moving_tests::OFF_TRIE_ASCEND_KEYS); + crate::zipper::zipper_moving_tests::run_test(&mut temp_store, $make_z, &[7], crate::zipper::zipper_moving_tests::ascend_until_from_off_trie_value_branch_root) + } + #[test] fn [<$z_name _indexed_zipper_movement1>]() { let mut temp_store = $read_keys(crate::zipper::zipper_moving_tests::ZIPPER_INDEXED_MOVEMENT_TEST1_KEYS); @@ -4046,6 +4070,72 @@ pub(crate) mod zipper_moving_tests { assert!(zipper.path_exists()); } + pub const OFF_TRIE_ASCEND_KEYS: &[&[u8]] = &[ + &[1, 2, 3, 4], &[5, 2], &[5, 6], &[7], &[7, 8], &[7, 9], + ]; + + type OffTrieAscendCase = (&'static [u8], bool, usize, &'static [u8], bool, usize, Option, &'static [u8], bool); + + /// Checks an off-trie ascent and then verifies that the landed focus remains usable. + fn run_off_trie_ascend_cases(mut zipper: Z, cases: &[OffTrieAscendCase]) { + for &(focus, need_value, steps, path, is_val, children, first, after_path, after_is_val) in cases { + zipper.reset(); + zipper.descend_to(focus); + assert!(!zipper.path_exists(), "focus {focus:?}"); + let actual_steps = if need_value { zipper.ascend_until() } else { zipper.ascend_until_branch() }; + assert_eq!(actual_steps, steps, "focus {focus:?}, need_value {need_value}"); + assert_eq!(zipper.path(), path, "focus {focus:?}, need_value {need_value}"); + assert!(zipper.path_exists(), "focus {focus:?}, need_value {need_value}"); + assert_eq!(zipper.is_val(), is_val, "focus {focus:?}, need_value {need_value}"); + assert_eq!(zipper.child_count(), children, "focus {focus:?}, need_value {need_value}"); + assert_eq!(zipper.descend_first_byte(), first, "focus {focus:?}, need_value {need_value}"); + assert_eq!(zipper.path(), after_path, "focus {focus:?}, need_value {need_value}"); + assert_eq!(zipper.is_val(), after_is_val, "focus {focus:?}, need_value {need_value}"); + } + } + + /// Rooted at the map root: 9 off-trie focuses, each tested with both ascent modes. + pub fn ascend_until_from_off_trie_root(zipper: Z) { + run_off_trie_ascend_cases(zipper, &[ + (&[1,2,9],false,3,&[],false,3,Some(1),&[1],false), (&[1,2,9],true,3,&[],false,3,Some(1),&[1],false), + (&[1,2,3,9],false,4,&[],false,3,Some(1),&[1],false), (&[1,2,3,9],true,4,&[],false,3,Some(1),&[1],false), + (&[1,2,3,4,9],false,5,&[],false,3,Some(1),&[1],false), (&[1,2,3,4,9],true,1,&[1,2,3,4],true,0,None,&[1,2,3,4],true), + (&[1,9,9],false,3,&[],false,3,Some(1),&[1],false), (&[1,9,9],true,3,&[],false,3,Some(1),&[1],false), + (&[5,9],false,1,&[5],false,2,Some(2),&[5,2],true), (&[5,9],true,1,&[5],false,2,Some(2),&[5,2],true), + (&[5,2,9],false,2,&[5],false,2,Some(2),&[5,2],true), (&[5,2,9],true,1,&[5,2],true,0,None,&[5,2],true), + (&[7,9,9],false,2,&[7],true,2,Some(8),&[7,8],true), (&[7,9,9],true,1,&[7,9],true,0,None,&[7,9],true), + (&[7,8,9,9],false,3,&[7],true,2,Some(8),&[7,8],true), (&[7,8,9,9],true,2,&[7,8],true,0,None,&[7,8],true), + (&[9],false,1,&[],false,3,Some(1),&[1],false), (&[9],true,1,&[],false,3,Some(1),&[1],false), + ]); + } + + /// Rooted at byte 1, the start of the compressed line. + pub fn ascend_until_from_off_trie_line_root(zipper: Z) { + run_off_trie_ascend_cases(zipper, &[ + (&[2,9],false,2,&[],false,1,Some(2),&[2],false), (&[2,9],true,2,&[],false,1,Some(2),&[2],false), + (&[2,3,9],false,3,&[],false,1,Some(2),&[2],false), (&[2,3,9],true,3,&[],false,1,Some(2),&[2],false), + (&[2,3,4,9],false,4,&[],false,1,Some(2),&[2],false), (&[2,3,4,9],true,1,&[2,3,4],true,0,None,&[2,3,4],true), + (&[9,9],false,2,&[],false,1,Some(2),&[2],false), (&[9,9],true,2,&[],false,1,Some(2),&[2],false), + ]); + } + + /// Rooted partway through the compressed line. + pub fn ascend_until_from_off_trie_mid_line_root(zipper: Z) { + run_off_trie_ascend_cases(zipper, &[ + (&[9],false,1,&[],false,1,Some(3),&[3],false), (&[9],true,1,&[],false,1,Some(3),&[3],false), + (&[3,9],false,2,&[],false,1,Some(3),&[3],false), (&[3,9],true,2,&[],false,1,Some(3),&[3],false), + (&[3,4,9],false,3,&[],false,1,Some(3),&[3],false), (&[3,4,9],true,1,&[3,4],true,0,None,&[3,4],true), + ]); + } + + /// Rooted at byte 7, which is both a value and a branch. + pub fn ascend_until_from_off_trie_value_branch_root(zipper: Z) { + run_off_trie_ascend_cases(zipper, &[ + (&[9,9],false,2,&[],true,2,Some(8),&[8],true), (&[9,9],true,1,&[9],true,0,None,&[9],true), + (&[8,9,9],false,3,&[],true,2,Some(8),&[8],true), (&[8,9,9],true,2,&[8],true,0,None,&[8],true), + ]); + } + pub const ZIPPER_INDEXED_BYTE_TEST1_KEYS: &[&[u8]] = &[b"0", b"1", b"2", b"3", b"4", b"5", b"6"]; pub fn zipper_indexed_bytes_test1(mut zip: Z) {