From 8071adf7a7b3ad0587e7ea992a0671600f88fae1 Mon Sep 17 00:00:00 2001 From: Igor Malovitsa Date: Wed, 16 Sep 2026 22:18:20 +0000 Subject: [PATCH 1/2] Fix sibling-step debug assert for a focus off the trie With no sibling to move to, to_prev_sibling_byte restores the focus and returns None, then asserted path_exists(), which fails when the focus was never in the trie. Both sibling steps now assert the focus's existence is unchanged instead. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_019R2H8fnco29asY2v3TPbtF --- src/zipper.rs | 74 ++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 73 insertions(+), 1 deletion(-) diff --git a/src/zipper.rs b/src/zipper.rs index ba96c579..1c63c31f 100644 --- a/src/zipper.rs +++ b/src/zipper.rs @@ -416,6 +416,9 @@ pub trait ZipperMoving: Zipper { /// where the index passed is 1 more than the index of the current focus position. fn to_next_sibling_byte(&mut self) -> Option { let cur_byte = self.focus_byte()?; + //The focus may be off the trie + #[cfg(debug_assertions)] + let focus_existed = self.path_exists(); if !self.ascend_byte() { return None } @@ -427,7 +430,10 @@ pub trait ZipperMoving: Zipper { Some(byte) }, None => { + //Restored focus exists iff it did before self.descend_to_byte(cur_byte); + #[cfg(debug_assertions)] + debug_assert_eq!(self.path_exists(), focus_existed); None } } @@ -442,6 +448,9 @@ pub trait ZipperMoving: Zipper { /// where the index passed is 1 less than the index of the current focus position. fn to_prev_sibling_byte(&mut self) -> Option { let cur_byte = self.focus_byte()?; + //The focus may be off the trie + #[cfg(debug_assertions)] + let focus_existed = self.path_exists(); if !self.ascend_byte() { return None } @@ -453,8 +462,10 @@ pub trait ZipperMoving: Zipper { Some(byte) }, None => { + //Restored focus exists iff it did before self.descend_to_byte(cur_byte); - debug_assert!(self.path_exists()); + #[cfg(debug_assertions)] + debug_assert_eq!(self.path_exists(), focus_existed); None } } @@ -6447,6 +6458,67 @@ mod tests { assert_eq!(z.to_prev_sibling_byte(), None); } + /// Sibling steps from a focus that is not in the trie + #[test] + fn sibling_step_from_a_focus_that_does_not_exist() { + //Empty map + let mut empty = PathMap::::new(); + let mut wz = empty.write_zipper(); + wz.descend_to(&[0u8]); + assert!(!wz.path_exists()); + assert_eq!(wz.to_prev_sibling_byte(), None); + assert_eq!(wz.path(), &[0u8]); + assert!(!wz.path_exists()); + assert_eq!(wz.to_next_sibling_byte(), None); + assert_eq!(wz.path(), &[0u8]); + drop(wz); + + let mut map = PathMap::::new(); + map.insert(&[1u8, 3], 13); + map.insert(&[1u8, 5], 15); + map.insert(&[7u8], 7); + + //Missing focus under an existing parent: siblings come from the parent + for (byte, prev, next) in [(2u8, None, Some(3u8)), (4, Some(3), Some(5)), (6, Some(5), None)] { + let mut wz = map.write_zipper_at_path(&[1u8]); + wz.descend_to(&[byte]); + assert!(!wz.path_exists(), "byte {byte}"); + assert_eq!(wz.to_prev_sibling_byte(), prev, "byte {byte}"); + assert_eq!(wz.path_exists(), prev.is_some(), "byte {byte}"); + assert_eq!(wz.path(), &[prev.unwrap_or(byte)], "byte {byte}"); + drop(wz); + + let mut wz = map.write_zipper_at_path(&[1u8]); + wz.descend_to(&[byte]); + assert_eq!(wz.to_next_sibling_byte(), next, "byte {byte}"); + drop(wz); + + //The read zipper's native impls must agree with the write zipper's default impls + let mut rz = map.read_zipper_at_path(&[1u8]); + rz.descend_to(&[byte]); + assert!(!rz.path_exists(), "byte {byte}"); + assert_eq!(rz.to_prev_sibling_byte(), prev, "byte {byte}"); + let mut rz = map.read_zipper_at_path(&[1u8]); + rz.descend_to(&[byte]); + assert_eq!(rz.to_next_sibling_byte(), next, "byte {byte}"); + } + + //Missing parent: no siblings, focus unchanged + let mut wz = map.write_zipper(); + wz.descend_to(&[9u8, 9]); + assert!(!wz.path_exists()); + assert_eq!(wz.to_prev_sibling_byte(), None); + assert_eq!(wz.path(), &[9u8, 9]); + assert_eq!(wz.to_next_sibling_byte(), None); + assert_eq!(wz.path(), &[9u8, 9]); + assert!(!wz.path_exists()); + //...and the zipper is still usable afterwards + wz.ascend(2); + assert_eq!(wz.to_next_sibling_byte(), None); + wz.descend_to(&[7u8]); + assert_eq!(wz.val(), Some(&7)); + } + /// Tests iteration behavior of to_next_val implementations, comparing the default impl /// against the native imple, and a third run that interleaves calls to each #[test] From a3f2858d591e16c354ef17900d0f8f0bd49230af Mon Sep 17 00:00:00 2001 From: Luke Peterson Date: Sat, 19 Sep 2026 01:28:13 -0600 Subject: [PATCH 2/2] Simplifying default impls for to_next and to_prev by getting rid of over-zealous debug_asserts. Lifting test up to macro. --- src/zipper.rs | 134 +++++++++++++++++++++++--------------------------- 1 file changed, 61 insertions(+), 73 deletions(-) diff --git a/src/zipper.rs b/src/zipper.rs index 1c63c31f..1e2dc9d4 100644 --- a/src/zipper.rs +++ b/src/zipper.rs @@ -416,9 +416,6 @@ pub trait ZipperMoving: Zipper { /// where the index passed is 1 more than the index of the current focus position. fn to_next_sibling_byte(&mut self) -> Option { let cur_byte = self.focus_byte()?; - //The focus may be off the trie - #[cfg(debug_assertions)] - let focus_existed = self.path_exists(); if !self.ascend_byte() { return None } @@ -430,10 +427,7 @@ pub trait ZipperMoving: Zipper { Some(byte) }, None => { - //Restored focus exists iff it did before self.descend_to_byte(cur_byte); - #[cfg(debug_assertions)] - debug_assert_eq!(self.path_exists(), focus_existed); None } } @@ -448,9 +442,6 @@ pub trait ZipperMoving: Zipper { /// where the index passed is 1 less than the index of the current focus position. fn to_prev_sibling_byte(&mut self) -> Option { let cur_byte = self.focus_byte()?; - //The focus may be off the trie - #[cfg(debug_assertions)] - let focus_existed = self.path_exists(); if !self.ascend_byte() { return None } @@ -462,10 +453,7 @@ pub trait ZipperMoving: Zipper { Some(byte) }, None => { - //Restored focus exists iff it did before self.descend_to_byte(cur_byte); - #[cfg(debug_assertions)] - debug_assert_eq!(self.path_exists(), focus_existed); None } } @@ -3665,6 +3653,18 @@ pub(crate) mod zipper_moving_tests { 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 _sibling_step_from_a_focus_that_does_not_exist_empty>]() { + let mut temp_store = $read_keys(&[]); + crate::zipper::zipper_moving_tests::run_test(&mut temp_store, $make_z, &[], crate::zipper::zipper_moving_tests::sibling_step_from_a_focus_that_does_not_exist_empty) + } + + #[test] + fn [<$z_name _sibling_step_from_a_focus_that_does_not_exist>]() { + let mut temp_store = $read_keys(crate::zipper::zipper_moving_tests::SIBLING_STEP_FROM_A_FOCUS_THAT_DOES_NOT_EXIST_KEYS); + crate::zipper::zipper_moving_tests::run_test(&mut temp_store, $make_z, &[], crate::zipper::zipper_moving_tests::sibling_step_from_a_focus_that_does_not_exist) + } + #[test] fn [<$z_name _zipper_indexed_bytes_test1>]() { let mut temp_store = $read_keys(crate::zipper::zipper_moving_tests::ZIPPER_INDEXED_BYTE_TEST1_KEYS); @@ -3991,6 +3991,55 @@ pub(crate) mod zipper_moving_tests { assert_eq!(zip.path(), b"bb"); } + /// Sibling steps from a focus that is not in the trie + pub fn sibling_step_from_a_focus_that_does_not_exist_empty(mut zipper: Z) { + zipper.descend_to(&[0]); + assert!(!zipper.path_exists()); + assert_eq!(zipper.to_prev_sibling_byte(), None); + assert_eq!(zipper.path(), &[0]); + assert!(!zipper.path_exists()); + assert_eq!(zipper.to_next_sibling_byte(), None); + assert_eq!(zipper.path(), &[0]); + assert!(!zipper.path_exists()); + } + + pub const SIBLING_STEP_FROM_A_FOCUS_THAT_DOES_NOT_EXIST_KEYS: &[&[u8]] = + &[&[1, 3], &[1, 5], &[7]]; + + /// Sibling steps from a missing focus use the parent\'s siblings when present, and otherwise + /// preserve the missing focus. + pub fn sibling_step_from_a_focus_that_does_not_exist(mut zipper: Z) { + //Missing focus under an existing parent: siblings come from the parent + for (byte, prev, next) in [(2, None, Some(3)), (4, Some(3), Some(5)), (6, Some(5), None)] { + zipper.reset(); + zipper.descend_to(&[1, byte]); + assert!(!zipper.path_exists(), "byte {byte}"); + assert_eq!(zipper.to_prev_sibling_byte(), prev, "byte {byte}"); + assert_eq!(zipper.path_exists(), prev.is_some(), "byte {byte}"); + assert_eq!(zipper.path(), &[1, prev.unwrap_or(byte)], "byte {byte}"); + + zipper.reset(); + zipper.descend_to(&[1, byte]); + assert_eq!(zipper.to_next_sibling_byte(), next, "byte {byte}"); + assert_eq!(zipper.path_exists(), next.is_some(), "byte {byte}"); + assert_eq!(zipper.path(), &[1, next.unwrap_or(byte)], "byte {byte}"); + } + + //Missing parent: no siblings, focus unchanged, and the zipper remains usable + zipper.reset(); + zipper.descend_to(&[9, 9]); + assert!(!zipper.path_exists()); + assert_eq!(zipper.to_prev_sibling_byte(), None); + assert_eq!(zipper.path(), &[9, 9]); + assert_eq!(zipper.to_next_sibling_byte(), None); + assert_eq!(zipper.path(), &[9, 9]); + assert!(!zipper.path_exists()); + zipper.ascend(2); + assert_eq!(zipper.to_next_sibling_byte(), None); + zipper.descend_to(&[7]); + assert!(zipper.path_exists()); + } + 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) { @@ -6458,67 +6507,6 @@ mod tests { assert_eq!(z.to_prev_sibling_byte(), None); } - /// Sibling steps from a focus that is not in the trie - #[test] - fn sibling_step_from_a_focus_that_does_not_exist() { - //Empty map - let mut empty = PathMap::::new(); - let mut wz = empty.write_zipper(); - wz.descend_to(&[0u8]); - assert!(!wz.path_exists()); - assert_eq!(wz.to_prev_sibling_byte(), None); - assert_eq!(wz.path(), &[0u8]); - assert!(!wz.path_exists()); - assert_eq!(wz.to_next_sibling_byte(), None); - assert_eq!(wz.path(), &[0u8]); - drop(wz); - - let mut map = PathMap::::new(); - map.insert(&[1u8, 3], 13); - map.insert(&[1u8, 5], 15); - map.insert(&[7u8], 7); - - //Missing focus under an existing parent: siblings come from the parent - for (byte, prev, next) in [(2u8, None, Some(3u8)), (4, Some(3), Some(5)), (6, Some(5), None)] { - let mut wz = map.write_zipper_at_path(&[1u8]); - wz.descend_to(&[byte]); - assert!(!wz.path_exists(), "byte {byte}"); - assert_eq!(wz.to_prev_sibling_byte(), prev, "byte {byte}"); - assert_eq!(wz.path_exists(), prev.is_some(), "byte {byte}"); - assert_eq!(wz.path(), &[prev.unwrap_or(byte)], "byte {byte}"); - drop(wz); - - let mut wz = map.write_zipper_at_path(&[1u8]); - wz.descend_to(&[byte]); - assert_eq!(wz.to_next_sibling_byte(), next, "byte {byte}"); - drop(wz); - - //The read zipper's native impls must agree with the write zipper's default impls - let mut rz = map.read_zipper_at_path(&[1u8]); - rz.descend_to(&[byte]); - assert!(!rz.path_exists(), "byte {byte}"); - assert_eq!(rz.to_prev_sibling_byte(), prev, "byte {byte}"); - let mut rz = map.read_zipper_at_path(&[1u8]); - rz.descend_to(&[byte]); - assert_eq!(rz.to_next_sibling_byte(), next, "byte {byte}"); - } - - //Missing parent: no siblings, focus unchanged - let mut wz = map.write_zipper(); - wz.descend_to(&[9u8, 9]); - assert!(!wz.path_exists()); - assert_eq!(wz.to_prev_sibling_byte(), None); - assert_eq!(wz.path(), &[9u8, 9]); - assert_eq!(wz.to_next_sibling_byte(), None); - assert_eq!(wz.path(), &[9u8, 9]); - assert!(!wz.path_exists()); - //...and the zipper is still usable afterwards - wz.ascend(2); - assert_eq!(wz.to_next_sibling_byte(), None); - wz.descend_to(&[7u8]); - assert_eq!(wz.val(), Some(&7)); - } - /// Tests iteration behavior of to_next_val implementations, comparing the default impl /// against the native imple, and a third run that interleaves calls to each #[test]