From 8a89dec17cb88fc173862e8a3cd5e6f198ae7750 Mon Sep 17 00:00:00 2001 From: Igor Malovitsa Date: Wed, 16 Sep 2026 22:16:37 +0000 Subject: [PATCH 1/3] Fix to_prev_sibling_byte losing the child of a list node location A location with both a value and a child is two same-key slots in a LineListNode. The previous-sibling lookup only checked the first matched slot for the child, so the zipper landed with child_count 0. Check both. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_019R2H8fnco29asY2v3TPbtF --- src/line_list_node.rs | 30 ++++++++++++++++-------------- src/zipper.rs | 21 +++++++++++++++++++++ 2 files changed, 37 insertions(+), 14 deletions(-) diff --git a/src/line_list_node.rs b/src/line_list_node.rs index 6013ef25..e0aa5b7a 100644 --- a/src/line_list_node.rs +++ b/src/line_list_node.rs @@ -2482,21 +2482,23 @@ impl TrieNode for LineListNode && *byte < key[last_key_byte_idx] }) }; - let (sibling_byte, slot) = match key_byte(key1) { - Some(byte) => (byte, 1), - None => match key_byte(key0) { - Some(byte) => (byte, 0), - None => return (None, None), - }, + let sibling_byte = match key_byte(key1).or_else(|| key_byte(key0)) { + Some(byte) => byte, + None => return (None, None), }; - let sib_node = match slot { - 0 if key0.len() == key.len() && self.is_child_ptr::<0>() => { - Some(unsafe { self.child_in_slot::<0>().as_tagged() }) - }, - 1 if key1.len() == key.len() && self.is_child_ptr::<1>() => { - Some(unsafe { self.child_in_slot::<1>().as_tagged() }) - }, - _ => None, + //A value+child location is two same-key slots, so check both for the child + let holds_child = |candidate: &[u8], slot: usize| { + candidate.len() == key.len() + && candidate[last_key_byte_idx] == sibling_byte + && candidate[..last_key_byte_idx] == common_key[..] + && if slot == 0 { self.is_child_ptr::<0>() } else { self.is_child_ptr::<1>() } + }; + let sib_node = if holds_child(key1, 1) { + Some(unsafe { self.child_in_slot::<1>().as_tagged() }) + } else if holds_child(key0, 0) { + Some(unsafe { self.child_in_slot::<0>().as_tagged() }) + } else { + None }; (Some(sibling_byte), sib_node) } diff --git a/src/zipper.rs b/src/zipper.rs index 57a70141..ec0a1dd1 100644 --- a/src/zipper.rs +++ b/src/zipper.rs @@ -6594,6 +6594,27 @@ mod tests { assert_eq!(z.path(), &[3]); } + /// `to_prev_sibling_byte` onto a list-node location holding both a value and a child + #[test] + fn read_zipper_prev_sibling_onto_a_value_and_child_location() { + let mut m = PathMap::::new(); + m.set_val_at(&[2u8, 0], 0); + m.set_val_at(&[2u8, 1], 0); + m.set_val_at(&[2u8], 0); + for start in [&[3u8][..], &[9u8]] { + let mut z = m.read_zipper(); + z.descend_to(start); + assert!(!z.path_exists()); + assert_eq!(z.to_prev_sibling_byte(), Some(2), "from {start:?}"); + assert_eq!(z.path(), &[2u8]); + assert_eq!(z.val(), Some(&0)); + assert_eq!(z.child_count(), 2, "from {start:?}"); + assert_eq!(z.child_mask().iter().collect::>(), vec![0u8, 1]); + assert_eq!(z.descend_first_byte(), Some(0), "from {start:?}"); + assert_eq!(z.path(), &[2u8, 0]); + } + } + #[test] fn read_zipper_prev_sibling_cases() { let m: PathMap<()> = [&[10u8][..], &[20], &[70]].into_iter().collect(); // dense node From b0951f8cb4678919286a6ba8f85f9ed7d22e823a Mon Sep 17 00:00:00 2001 From: Luke Peterson Date: Fri, 18 Sep 2026 23:20:31 -0600 Subject: [PATCH 2/3] Moving a handful of to_prev_sibling_byte tests to the ZipperMoving test macro that is implemented for all zipper types; slightly broadening covered cases --- src/zipper.rs | 67 +++++++++++++++++++++++++++++++++++---------------- 1 file changed, 46 insertions(+), 21 deletions(-) diff --git a/src/zipper.rs b/src/zipper.rs index ec0a1dd1..ba96c579 100644 --- a/src/zipper.rs +++ b/src/zipper.rs @@ -3642,6 +3642,18 @@ pub(crate) mod zipper_moving_tests { crate::zipper::zipper_moving_tests::run_test(&mut temp_store, $make_z, &[], crate::zipper::zipper_moving_tests::zipper_dangling_descend_test) } + #[test] + fn [<$z_name _prev_sibling_sparse_paths>]() { + let mut temp_store = $read_keys(crate::zipper::zipper_moving_tests::ZIPPER_PREV_SIBLING_SPARSE_PATHS_KEYS); + crate::zipper::zipper_moving_tests::run_test(&mut temp_store, $make_z, &[], crate::zipper::zipper_moving_tests::prev_sibling_sparse_paths) + } + + #[test] + fn [<$z_name _prev_sibling_value_and_child_location>]() { + let mut temp_store = $read_keys(crate::zipper::zipper_moving_tests::ZIPPER_PREV_SIBLING_VALUE_AND_CHILD_LOCATION_KEYS); + crate::zipper::zipper_moving_tests::run_test(&mut temp_store, $make_z, &[], crate::zipper::zipper_moving_tests::prev_sibling_value_and_child_location) + } + #[test] fn [<$z_name _zipper_indexed_bytes_test1>]() { let mut temp_store = $read_keys(crate::zipper::zipper_moving_tests::ZIPPER_INDEXED_BYTE_TEST1_KEYS); @@ -3816,6 +3828,40 @@ pub(crate) mod zipper_moving_tests { /// from https://en.wikipedia.org/wiki/Radix_tree#/media/File:Patricia_trie.svg pub const ZIPPER_MOVING_BASIC_TEST_KEYS: &[&[u8]] = &[b"romane", b"romanus", b"romulus", b"rubens", b"ruber", b"rubicon", b"rubicundus", b"rom'i"]; + pub const ZIPPER_PREV_SIBLING_SPARSE_PATHS_KEYS: &[&[u8]] = &[b"AA", b"CCC"]; + + pub const ZIPPER_PREV_SIBLING_VALUE_AND_CHILD_LOCATION_KEYS: &[&[u8]] = &[&[2, 0], &[2, 1], &[2]]; + + pub fn prev_sibling_sparse_paths(mut zipper: Z) { + // An absent root child can have a preceding sibling, while positions + // inside a line have no preceding sibling at their respective depths. + zipper.descend_to(b"C"); + assert_eq!(zipper.to_prev_sibling_byte(), Some(b'A')); + assert_eq!(zipper.path(), b"A"); + zipper.reset(); + zipper.descend_to(b"CC"); + assert_eq!(zipper.to_prev_sibling_byte(), None); + zipper.reset(); + zipper.descend_to(b"CCC"); + assert_eq!(zipper.to_prev_sibling_byte(), None); + } + + /// `to_prev_sibling_byte` onto a location holding both a value and a child subtree. + pub fn prev_sibling_value_and_child_location(mut zipper: Z) { + for start in [&[3u8][..], &[9u8]] { + zipper.reset(); + zipper.descend_to(start); + assert!(!zipper.path_exists()); + assert_eq!(zipper.to_prev_sibling_byte(), Some(2), "from {start:?}"); + assert_eq!(zipper.path(), &[2u8]); + assert!(zipper.is_val()); + assert_eq!(zipper.child_count(), 2, "from {start:?}"); + assert_eq!(zipper.child_mask().iter().collect::>(), vec![0u8, 1]); + assert_eq!(zipper.descend_first_byte(), Some(0), "from {start:?}"); + assert_eq!(zipper.path(), &[2u8, 0]); + } + } + pub fn zipper_moving_basic_test(mut zipper: Z) { fn assert_in_list(val: &[u8], list: &[&[u8]]) { for test_val in list { @@ -6594,27 +6640,6 @@ mod tests { assert_eq!(z.path(), &[3]); } - /// `to_prev_sibling_byte` onto a list-node location holding both a value and a child - #[test] - fn read_zipper_prev_sibling_onto_a_value_and_child_location() { - let mut m = PathMap::::new(); - m.set_val_at(&[2u8, 0], 0); - m.set_val_at(&[2u8, 1], 0); - m.set_val_at(&[2u8], 0); - for start in [&[3u8][..], &[9u8]] { - let mut z = m.read_zipper(); - z.descend_to(start); - assert!(!z.path_exists()); - assert_eq!(z.to_prev_sibling_byte(), Some(2), "from {start:?}"); - assert_eq!(z.path(), &[2u8]); - assert_eq!(z.val(), Some(&0)); - assert_eq!(z.child_count(), 2, "from {start:?}"); - assert_eq!(z.child_mask().iter().collect::>(), vec![0u8, 1]); - assert_eq!(z.descend_first_byte(), Some(0), "from {start:?}"); - assert_eq!(z.path(), &[2u8, 0]); - } - } - #[test] fn read_zipper_prev_sibling_cases() { let m: PathMap<()> = [&[10u8][..], &[20], &[70]].into_iter().collect(); // dense node From 1b3f5ec912e0a7fa235090c0db7fbd4aef783d78 Mon Sep 17 00:00:00 2001 From: Luke Peterson Date: Sat, 19 Sep 2026 00:13:07 -0600 Subject: [PATCH 3/3] Fixing additional bug in ACTZipper sibling movement when focus path is non-existent --- src/arena_compact.rs | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/src/arena_compact.rs b/src/arena_compact.rs index 3914ba8b..183b7bc1 100644 --- a/src/arena_compact.rs +++ b/src/arena_compact.rs @@ -2771,7 +2771,48 @@ where Storage: AsRef<[u8]> descended } + #[cold] + #[inline(never)] + fn to_sibling_from_nonexistent_path(&mut self, next: bool) -> Option { + // A sibling can only exist when the final byte alone is nonexistent and + // its parent is therefore an existing trie position. + if self.invalid != 1 || self.at_root() { + return None; + } + + let cur_byte = *self.path.last().unwrap(); + let (sibling_byte, sibling_idx) = match &self.cur_node { + Node::Line(line) => { + let frame = self.stack.last().unwrap(); + let byte = *self.tree.get_line(line.path).get(frame.node_depth)?; + if (next && byte > cur_byte) || (!next && byte < cur_byte) { + (byte, 0) + } else { + return None; + } + } + Node::Branch(node) => { + let byte = if next { + node.bytemask.next_bit(cur_byte) + } else { + node.bytemask.prev_bit(cur_byte) + }?; + (byte, node.bytemask.index_of(byte) as usize) + } + }; + + self.path.pop(); + self.invalid = 0; + let result = self.descend_indexed_byte(sibling_idx); + debug_assert_eq!(result, Some(sibling_byte)); + result + } + fn to_sibling(&mut self, next: bool) -> Option { + if self.invalid > 0 { + return self.to_sibling_from_nonexistent_path(next); + } + let top_frame = self.stack.last().unwrap(); if self.stack.len() <= 1 || top_frame.node_depth > 0 { // can't move to sibling at root, or along the path