From 1e3b1c3f7ea377db48696e9ddc5c81423e87d34b Mon Sep 17 00:00:00 2001 From: Igor Malovitsa Date: Wed, 16 Sep 2026 23:25:42 +0000 Subject: [PATCH] Fix join_into overwriting a destination that holds the source Joining with a TinyRefNode swapped operands without inverting the identity mask, so an unchanged destination reported COUNTER_IDENT and join_into replaced it with the source. merge_guts and the integer pjoin also under-reported identities, giving Element for unchanged joins. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_019R2H8fnco29asY2v3TPbtF --- src/dense_byte_node.rs | 6 +++++- src/line_list_node.rs | 36 ++++++++++++++++--------------- src/ring.rs | 16 +++++++++----- src/write_zipper.rs | 48 ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 83 insertions(+), 23 deletions(-) diff --git a/src/dense_byte_node.rs b/src/dense_byte_node.rs index 65568d9c..2b69e152 100644 --- a/src/dense_byte_node.rs +++ b/src/dense_byte_node.rs @@ -1323,8 +1323,12 @@ impl> TrieNode self.pjoin(other_byte_node).map(|new_node| TrieNodeODRc::new_in(new_node, self.alloc.clone())) }, TINY_REF_NODE_TAG => { + //Expand the tiny node and keep `self` on the left, so the identity mask stays ours let tiny_node = unsafe{ other.as_tiny_unchecked() }; - tiny_node.pjoin_dyn(self.as_tagged()) + match tiny_node.into_full() { + Some(full_node) => self.pjoin_dyn(full_node.as_tagged()), + None => AlgebraicResult::Identity(SELF_IDENT), + } } EMPTY_NODE_TAG => { AlgebraicResult::Identity(SELF_IDENT) diff --git a/src/line_list_node.rs b/src/line_list_node.rs index 6013ef25..6cd8afac 100644 --- a/src/line_list_node.rs +++ b/src/line_list_node.rs @@ -1345,14 +1345,13 @@ fn merge_guts<'a, V: Clone + Lattice + Send + Sync, A: Allocator, const ASLOT: u unsafe{ intermediate_node.set_payload_owned::<0>(&a_key[overlap..], a_payload); } debug_assert!(validate_node(&intermediate_node)); let intermediate_node = TrieNodeODRc::new_in(intermediate_node, a.alloc.clone()); - let joined = b_child.pjoin(&intermediate_node).unwrap_or_else(|which_arg| { - match which_arg { - 0 => b_child.clone(), - 1 => intermediate_node, - _ => unreachable!() - } - }, || panic!()); - return AlgebraicResult::Element((&a_key[0..overlap], ValOrChild::Child(joined))) + return match b_child.pjoin(&intermediate_node) { + 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(_) => AlgebraicResult::Element((&a_key[0..overlap], ValOrChild::Child(intermediate_node))), + AlgebraicResult::None => unreachable!(), //`intermediate_node` is never empty + } } if a_key_len == overlap && a.is_child_ptr::() && b_key_len > overlap { let a_child = unsafe{ a.child_in_slot::() }; @@ -1361,14 +1360,13 @@ fn merge_guts<'a, V: Clone + Lattice + Send + Sync, A: Allocator, const ASLOT: u unsafe{ intermediate_node.set_payload_owned::<0>(&b_key[overlap..], b_payload); } debug_assert!(validate_node(&intermediate_node)); let intermediate_node = TrieNodeODRc::new_in(intermediate_node, a.alloc.clone()); - let joined = a_child.pjoin(&intermediate_node).unwrap_or_else(|which_arg| { - match which_arg { - 0 => a_child.clone(), - 1 => intermediate_node, - _ => unreachable!() - } - }, || panic!()); - return AlgebraicResult::Element((&a_key[0..overlap], ValOrChild::Child(joined))) + return match a_child.pjoin(&intermediate_node) { + 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(_) => AlgebraicResult::Element((&a_key[0..overlap], ValOrChild::Child(intermediate_node))), + AlgebraicResult::None => unreachable!(), //`intermediate_node` is never empty + } } //If we have overlapping initial bytes that can be joined together, make a new prefix node @@ -2649,8 +2647,12 @@ impl TrieNode for LineListNode } }, TINY_REF_NODE_TAG => { + //Expand the tiny node and keep `self` on the left (see DenseByteNode::pjoin_dyn) let tiny_node = unsafe{ other.as_tiny_unchecked() }; - tiny_node.pjoin_dyn(self.as_tagged()) + match tiny_node.into_full() { + Some(full_node) => self.pjoin_dyn(full_node.as_tagged()), + None => AlgebraicResult::Identity(SELF_IDENT), + } } EMPTY_NODE_TAG => { AlgebraicResult::Identity(SELF_IDENT) diff --git a/src/ring.rs b/src/ring.rs index 4b9b1d45..100edaed 100644 --- a/src/ring.rs +++ b/src/ring.rs @@ -851,15 +851,21 @@ impl Lattice for () { fn pmeet(&self, _other: &Self) -> AlgebraicResult { AlgebraicResult::Identity(SELF_IDENT | COUNTER_IDENT) } } +/// Left-biased join; equal values are also `other`'s identity +#[inline] +fn left_biased_pjoin(a: &T, b: &T) -> AlgebraicResult { + if a == b { AlgebraicResult::Identity(SELF_IDENT | COUNTER_IDENT) } else { AlgebraicResult::Identity(SELF_IDENT) } +} + //GOAT trash impl Lattice for usize { - fn pjoin(&self, _other: &usize) -> AlgebraicResult { AlgebraicResult::Identity(SELF_IDENT) } + fn pjoin(&self, other: &usize) -> AlgebraicResult { left_biased_pjoin(self, other) } fn pmeet(&self, _other: &usize) -> AlgebraicResult { AlgebraicResult::Identity(SELF_IDENT) } } //GOAT trash impl Lattice for u64 { - fn pjoin(&self, _other: &u64) -> AlgebraicResult { AlgebraicResult::Identity(SELF_IDENT) } + fn pjoin(&self, other: &u64) -> AlgebraicResult { left_biased_pjoin(self, other) } fn pmeet(&self, _other: &u64) -> AlgebraicResult { AlgebraicResult::Identity(SELF_IDENT) } } @@ -873,13 +879,13 @@ impl DistributiveLattice for u64 { //GOAT trash impl Lattice for u32 { - fn pjoin(&self, _other: &u32) -> AlgebraicResult { AlgebraicResult::Identity(SELF_IDENT) } + fn pjoin(&self, other: &u32) -> AlgebraicResult { left_biased_pjoin(self, other) } fn pmeet(&self, _other: &u32) -> AlgebraicResult { AlgebraicResult::Identity(SELF_IDENT) } } //GOAT trash impl Lattice for u16 { - fn pjoin(&self, _other: &u16) -> AlgebraicResult { AlgebraicResult::Identity(SELF_IDENT) } + fn pjoin(&self, other: &u16) -> AlgebraicResult { left_biased_pjoin(self, other) } fn pmeet(&self, _other: &u16) -> AlgebraicResult { AlgebraicResult::Identity(SELF_IDENT) } } @@ -893,7 +899,7 @@ impl DistributiveLattice for u16 { //GOAT trash impl Lattice for u8 { - fn pjoin(&self, _other: &u8) -> AlgebraicResult { AlgebraicResult::Identity(SELF_IDENT) } + fn pjoin(&self, other: &u8) -> AlgebraicResult { left_biased_pjoin(self, other) } fn pmeet(&self, _other: &u8) -> AlgebraicResult { AlgebraicResult::Identity(SELF_IDENT) } } diff --git a/src/write_zipper.rs b/src/write_zipper.rs index 0892e475..8e68fce9 100644 --- a/src/write_zipper.rs +++ b/src/write_zipper.rs @@ -6672,4 +6672,52 @@ mod tests { } assert_eq!(keys(&m), ["cx", "cy", "d"]); } + + /// `join_into` from a source focus partway into a line node (a `TinyRefNode`) + #[test] + fn write_zipper_join_into_mid_key_source_keeps_destination() { + 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 src = mk(&[(&[0, 0, 0], 7)]); + + //Dense destination + let mut dst = mk(&[(&[0], 7), (&[1], 1), (&[2], 2), (&[3], 3)]); + let before = vals(&dst); + let st = { let mut wz = dst.write_zipper(); let mut rz = src.read_zipper(); rz.descend_to(&[0, 0]); wz.join_into(&rz) }; + assert_eq!(st, AlgebraicStatus::Identity); + assert_eq!(vals(&dst), before); + + //List destination + let mut dst = mk(&[(&[0], 7), (&[0, 0], 0)]); + let before = vals(&dst); + let st = { let mut wz = dst.write_zipper(); let mut rz = src.read_zipper(); rz.descend_to(&[0, 0]); wz.join_into(&rz) }; + assert_eq!(st, AlgebraicStatus::Identity); + assert_eq!(vals(&dst), before); + + //And a join that does add something still says so, with the destination intact + let mut dst = mk(&[(&[1], 1), (&[2], 2), (&[3], 3)]); + let st = { let mut wz = dst.write_zipper(); let mut rz = src.read_zipper(); rz.descend_to(&[0, 0]); wz.join_into(&rz) }; + assert_eq!(st, AlgebraicStatus::Element); + assert_eq!(vals(&dst), vec![(vec![0], 7), (vec![1], 1), (vec![2], 2), (vec![3], 3)]); + } + + /// `join_into` of a source already contained under a destination child is `Identity` + #[test] + fn write_zipper_join_into_contained_under_child_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, 0], 0), (&[0, 1], 0)]); + let before = vals(&dst); + let src = mk(&[(&[0, 0], 0)]); + let st = { let mut wz = dst.write_zipper(); wz.join_into(&src.read_zipper()) }; + assert_eq!(st, AlgebraicStatus::Identity); + assert_eq!(vals(&dst), before); + + //The mirror image: the source holds the child, the destination the longer key + let mut dst = mk(&[(&[0, 0], 0)]); + let src = mk(&[(&[0, 0], 0), (&[0, 1], 0)]); + let st = { let mut wz = dst.write_zipper(); wz.join_into(&src.read_zipper()) }; + assert_eq!(st, AlgebraicStatus::Element); + assert_eq!(vals(&dst), vals(&src)); + } }