From c30392c46cf5026443c825b05771b5eb513d95e5 Mon Sep 17 00:00:00 2001 From: Igor Malovitsa Date: Wed, 16 Sep 2026 23:17:49 +0000 Subject: [PATCH 1/2] Fix dense restrict status and dangling paths The dense restrict required both masks to match before reporting Identity, though only the destination's branches matter. It also kept the flag set when it dropped a value beside a kept child, or an unvalidated dangling path, so the caller kept the destination unchanged. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_019R2H8fnco29asY2v3TPbtF --- src/dense_byte_node.rs | 11 ++++- src/write_zipper.rs | 94 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 104 insertions(+), 1 deletion(-) diff --git a/src/dense_byte_node.rs b/src/dense_byte_node.rs index 65568d9c..1c3a1fb5 100644 --- a/src/dense_byte_node.rs +++ b/src/dense_byte_node.rs @@ -501,6 +501,11 @@ impl> ByteNode new_node.values.push(cf.clone()); } else { + //An unvalidated value is dropped, so this is a change + if cf.val().is_some() { + is_identity = false; + } + //If there is an onward link in the CF and other node, continue the restriction recursively if let Some(self_child) = cf.rec() { let other_child = other.get_node_at_key(&[key_byte]); @@ -527,6 +532,9 @@ impl> ByteNode is_identity = false; } } + } else { + //No value or child: an unvalidated dangling path, also dropped + is_identity = false; } } } else { @@ -2355,7 +2363,8 @@ impl> ByteNode // Iterate the overlap mask directly. Slot indexes are recovered with // prefix popcounts in each dense-mask word. let mut mm: ByteMask = self.mask & other.mask; - let mut is_identity = self.mask == mm && other.mask == mm; + //Restrict is non-commutative: only `self`'s branches matter for identity + let mut is_identity = self.mask == mm; let mmc = [mm.0[0].count_ones(), mm.0[1].count_ones(), mm.0[2].count_ones(), mm.0[3].count_ones()]; diff --git a/src/write_zipper.rs b/src/write_zipper.rs index 0892e475..6e024691 100644 --- a/src/write_zipper.rs +++ b/src/write_zipper.rs @@ -6672,4 +6672,98 @@ mod tests { } assert_eq!(keys(&m), ["cx", "cy", "d"]); } + + /// Dense `restrict` is `Identity` even when the source has extra branches + #[test] + fn write_zipper_restrict_wider_source_is_identity() { + fn mk(ps: &[(&[u8], u64)]) -> PathMap { let mut m = PathMap::new(); for (p, v) in ps { m.set_val_at(p, *v); } m } + fn vals(m: &PathMap) -> Vec<(Vec, u64)> { m.iter().map(|(k, v)| (k.to_vec(), *v)).collect() } + + let mut dst = mk(&[(&[0], 1), (&[1], 2), (&[2], 3)]); + let before = vals(&dst); + let src = mk(&[(&[0], 0), (&[1], 0), (&[2], 0), (&[3], 0), (&[4], 0)]); + let st = { let mut wz = dst.write_zipper(); wz.restrict(&src.read_zipper()) }; + assert_eq!(st, AlgebraicStatus::Identity); + assert_eq!(vals(&dst), before); + + //A restriction that really does drop a branch still reports it + let mut dst = mk(&[(&[0], 1), (&[1], 2), (&[2], 3)]); + let src = mk(&[(&[0], 0), (&[1], 0), (&[3], 0), (&[4], 0)]); + let st = { let mut wz = dst.write_zipper(); wz.restrict(&src.read_zipper()) }; + assert_eq!(st, AlgebraicStatus::Element); + assert_eq!(vals(&dst), vec![(vec![0], 1), (vec![1], 2)]); + } + + /// Dense `restrict` against a list node drops a value beside a kept child + #[test] + fn write_zipper_restrict_drops_value_beside_kept_child() { + fn mk(ps: &[(&[u8], u64)]) -> PathMap { let mut m = PathMap::new(); for (p, v) in ps { m.set_val_at(p, *v); } m } + fn vals(m: &PathMap) -> Vec<(Vec, u64)> { m.iter().map(|(k, v)| (k.to_vec(), *v)).collect() } + + //Dense root with a value and a child at [0] + let mut dst = mk(&[(&[0], 7), (&[0, 0], 1), (&[1], 2), (&[2], 3)]); + let filter = mk(&[(&[0], 0), (&[0, 0], 0), (&[5], 0), (&[6], 0)]); + { let mut wz = dst.write_zipper(); wz.meet_into(&filter.read_zipper(), false); } + assert_eq!(vals(&dst), vec![(vec![0], 7), (vec![0, 0], 1)]); + + //`src` has no value at `[0]`, so `[0]` is not kept, while `[0, 0]` is + let src = mk(&[(&[0, 0], 0)]); + let st = { let mut wz = dst.write_zipper(); wz.restrict(&src.read_zipper()) }; + assert_eq!(vals(&dst), vec![(vec![0, 0], 1)]); + assert_eq!(st, AlgebraicStatus::Element); + } + + /// Dense `restrict` drops an unvalidated dangling path + #[test] + fn write_zipper_restrict_drops_dangling_branch() { + fn mk(ps: &[(&[u8], u64)]) -> PathMap { let mut m = PathMap::new(); for (p, v) in ps { m.set_val_at(p, *v); } m } + fn vals(m: &PathMap) -> Vec<(Vec, u64)> { m.iter().map(|(k, v)| (k.to_vec(), *v)).collect() } + + //Dense root with [0] dangling + let mut dst = mk(&[(&[0], 1), (&[1], 2), (&[2], 3), (&[3], 4)]); + let filter = mk(&[(&[0], 1), (&[1], 2)]); + { let mut wz = dst.write_zipper(); wz.meet_into(&filter.read_zipper(), false); } + dst.remove_val_at(&[0u8], false); + assert_eq!(dst.path_exists_at(&[0u8]), true, "[0] should be left dangling"); + assert_eq!(vals(&dst), vec![(vec![1], 2)]); + + //`src` has no value at [0], so the dangling [0] goes + let src = mk(&[(&[0, 9], 0), (&[1], 0)]); + let st = { let mut wz = dst.write_zipper(); wz.restrict(&src.read_zipper()) }; + assert_eq!(st, AlgebraicStatus::Element); + assert_eq!(dst.path_exists_at(&[0u8]), false); + assert_eq!(vals(&dst), vec![(vec![1], 2)]); + + //A dangling path that *is* validated stays: `src` has a value at [0]. + let mut dst = mk(&[(&[0], 1), (&[1], 2), (&[2], 3), (&[3], 4)]); + { let mut wz = dst.write_zipper(); wz.meet_into(&filter.read_zipper(), false); } + dst.remove_val_at(&[0u8], false); + let src = mk(&[(&[0], 0), (&[1], 0)]); + let st = { let mut wz = dst.write_zipper(); wz.restrict(&src.read_zipper()) }; + assert_eq!(st, AlgebraicStatus::Identity); + assert_eq!(dst.path_exists_at(&[0u8]), true); + assert_eq!(vals(&dst), vec![(vec![1], 2)]); + + //Fuzzer reproducer + let mut map0 = PathMap::::new(); + map0.set_val_at(&[0u8], 0); + let mut map1 = PathMap::::new(); + map1.set_val_at(&[0u8, 0, 0, 0], 0); + map1.set_val_at(&[1u8], 0); + { + let mut wz = map0.write_zipper_at_path(&[]); + let mut rz = map1.read_zipper_at_path(&[]); + wz.join_into(&rz); + wz.descend_first_byte(); + rz.to_next_val(); + wz.subtract_into(&rz, false); + rz.to_next_val(); + wz.meet_into(&rz, false); + } + assert_eq!(map0.path_exists_at(&[0u8]), true, "meet_into leaves [0] dangling"); + let st = { let mut wz = map0.write_zipper(); wz.restrict(&map1.read_zipper()) }; + assert_eq!(st, AlgebraicStatus::Element); + assert_eq!(map0.path_exists_at(&[0u8]), false); + assert_eq!(vals(&map0), vec![(vec![1], 0)]); + } } From a388b3a2262d4acb27b389d0bbe25d69b7142a8b Mon Sep 17 00:00:00 2001 From: Luke Peterson Date: Sat, 19 Sep 2026 07:24:12 -0600 Subject: [PATCH 2/2] Changes only to comments. The prior fixes were good, but the vocabulary deviated from what the rest of the project uses --- src/dense_byte_node.rs | 4 ++-- src/write_zipper.rs | 34 +++++++++++++++++----------------- 2 files changed, 19 insertions(+), 19 deletions(-) diff --git a/src/dense_byte_node.rs b/src/dense_byte_node.rs index fbab32fc..140c0144 100644 --- a/src/dense_byte_node.rs +++ b/src/dense_byte_node.rs @@ -505,7 +505,7 @@ impl> ByteNode new_node.values.push(cf.clone()); } else { - //An unvalidated value is dropped, so this is a change + //Dropping this value changes the destination if cf.val().is_some() { is_identity = false; } @@ -537,7 +537,7 @@ impl> ByteNode } } } else { - //No value or child: an unvalidated dangling path, also dropped + //The target slot is dangling and the restrictor has no value here: drop it is_identity = false; } } diff --git a/src/write_zipper.rs b/src/write_zipper.rs index e8ade221..4647adb1 100644 --- a/src/write_zipper.rs +++ b/src/write_zipper.rs @@ -6753,28 +6753,28 @@ mod tests { assert_eq!(keys(&m), ["cx", "cy", "d"]); } - /// Dense `restrict` is `Identity` even when the source has extra branches + /// Dense `restrict` is `Identity` even when the restrictor has extra branches #[test] - fn write_zipper_restrict_wider_source_is_identity() { + fn write_zipper_restrict_wider_restrictor_is_identity() { fn mk(ps: &[(&[u8], u64)]) -> PathMap { let mut m = PathMap::new(); for (p, v) in ps { m.set_val_at(p, *v); } m } fn vals(m: &PathMap) -> Vec<(Vec, u64)> { m.iter().map(|(k, v)| (k.to_vec(), *v)).collect() } let mut dst = mk(&[(&[0], 1), (&[1], 2), (&[2], 3)]); let before = vals(&dst); - let src = mk(&[(&[0], 0), (&[1], 0), (&[2], 0), (&[3], 0), (&[4], 0)]); - let st = { let mut wz = dst.write_zipper(); wz.restrict(&src.read_zipper()) }; + let restrictor = mk(&[(&[0], 0), (&[1], 0), (&[2], 0), (&[3], 0), (&[4], 0)]); + let st = { let mut wz = dst.write_zipper(); wz.restrict(&restrictor.read_zipper()) }; assert_eq!(st, AlgebraicStatus::Identity); assert_eq!(vals(&dst), before); //A restriction that really does drop a branch still reports it let mut dst = mk(&[(&[0], 1), (&[1], 2), (&[2], 3)]); - let src = mk(&[(&[0], 0), (&[1], 0), (&[3], 0), (&[4], 0)]); - let st = { let mut wz = dst.write_zipper(); wz.restrict(&src.read_zipper()) }; + let restrictor = mk(&[(&[0], 0), (&[1], 0), (&[3], 0), (&[4], 0)]); + let st = { let mut wz = dst.write_zipper(); wz.restrict(&restrictor.read_zipper()) }; assert_eq!(st, AlgebraicStatus::Element); assert_eq!(vals(&dst), vec![(vec![0], 1), (vec![1], 2)]); } - /// Dense `restrict` against a list node drops a value beside a kept child + /// Dense `restrict` against a list node drops a value in the same node as a kept child #[test] fn write_zipper_restrict_drops_value_beside_kept_child() { fn mk(ps: &[(&[u8], u64)]) -> PathMap { let mut m = PathMap::new(); for (p, v) in ps { m.set_val_at(p, *v); } m } @@ -6786,14 +6786,14 @@ mod tests { { let mut wz = dst.write_zipper(); wz.meet_into(&filter.read_zipper(), false); } assert_eq!(vals(&dst), vec![(vec![0], 7), (vec![0, 0], 1)]); - //`src` has no value at `[0]`, so `[0]` is not kept, while `[0, 0]` is - let src = mk(&[(&[0, 0], 0)]); - let st = { let mut wz = dst.write_zipper(); wz.restrict(&src.read_zipper()) }; + //The restrictor has no value at `[0]`, so `[0]` is not kept, while `[0, 0]` is + let restrictor = mk(&[(&[0, 0], 0)]); + let st = { let mut wz = dst.write_zipper(); wz.restrict(&restrictor.read_zipper()) }; assert_eq!(vals(&dst), vec![(vec![0, 0], 1)]); assert_eq!(st, AlgebraicStatus::Element); } - /// Dense `restrict` drops an unvalidated dangling path + /// Dense `restrict` drops a dangling path not kept by the restrictor #[test] fn write_zipper_restrict_drops_dangling_branch() { fn mk(ps: &[(&[u8], u64)]) -> PathMap { let mut m = PathMap::new(); for (p, v) in ps { m.set_val_at(p, *v); } m } @@ -6807,19 +6807,19 @@ mod tests { assert_eq!(dst.path_exists_at(&[0u8]), true, "[0] should be left dangling"); assert_eq!(vals(&dst), vec![(vec![1], 2)]); - //`src` has no value at [0], so the dangling [0] goes - let src = mk(&[(&[0, 9], 0), (&[1], 0)]); - let st = { let mut wz = dst.write_zipper(); wz.restrict(&src.read_zipper()) }; + //The restrictor has no value at [0], so the dangling [0] goes + let restrictor = mk(&[(&[0, 9], 0), (&[1], 0)]); + let st = { let mut wz = dst.write_zipper(); wz.restrict(&restrictor.read_zipper()) }; assert_eq!(st, AlgebraicStatus::Element); assert_eq!(dst.path_exists_at(&[0u8]), false); assert_eq!(vals(&dst), vec![(vec![1], 2)]); - //A dangling path that *is* validated stays: `src` has a value at [0]. + //A dangling path that is kept stays: the restrictor has a value at [0]. let mut dst = mk(&[(&[0], 1), (&[1], 2), (&[2], 3), (&[3], 4)]); { let mut wz = dst.write_zipper(); wz.meet_into(&filter.read_zipper(), false); } dst.remove_val_at(&[0u8], false); - let src = mk(&[(&[0], 0), (&[1], 0)]); - let st = { let mut wz = dst.write_zipper(); wz.restrict(&src.read_zipper()) }; + let restrictor = mk(&[(&[0], 0), (&[1], 0)]); + let st = { let mut wz = dst.write_zipper(); wz.restrict(&restrictor.read_zipper()) }; assert_eq!(st, AlgebraicStatus::Identity); assert_eq!(dst.path_exists_at(&[0u8]), true); assert_eq!(vals(&dst), vec![(vec![1], 2)]);