From d769441e75d708f903d342f20b4c79a8960776a5 Mon Sep 17 00:00:00 2001 From: Igor Malovitsa Date: Thu, 17 Sep 2026 03:35:25 +0000 Subject: [PATCH] Fix ProductZipper is_shared at a factor root A secondary factor's root isn't a child of the node above it, so the core zipper's parent lookup unwrapped None. Ask the factor's TrieRef instead. Only report sharing in the last factor, for both ProductZipper and ProductZipperG. In an earlier factor the subtrie below a node continues into the following factors, so the same node reached in different factors is a different subtrie of the product. A cached cata keyed by shared_node_id conflated the two and returned wrong results. --- src/product_zipper.rs | 88 ++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 86 insertions(+), 2 deletions(-) diff --git a/src/product_zipper.rs b/src/product_zipper.rs index 9f2be3e9..10e1b9ea 100644 --- a/src/product_zipper.rs +++ b/src/product_zipper.rs @@ -148,6 +148,20 @@ impl<'factor_z, 'trie, V: Clone + Send + Sync + Unpin, A: Allocator> ProductZipp self.enroll_next_factor(); } } + /// The secondary factor whose root node is the focus, if any. Its node is not a child of the + /// node above it, so the core zipper can't look it up. + fn factor_root(&self) -> Option<&TrieRef<'trie, V, A>> { + match self.factor_paths.last() { + Some(&start) if start == self.depth() => self.secondaries.get(self.factor_paths.len() - 1), + _ => None + } + } + /// `true` once every factor has been entered, i.e. the focus is in the last factor's trie (including + /// its root). See the `ZipperConcrete` impl for why sharing is only reported there. + #[inline] + fn in_last_factor(&self) -> bool { + self.factor_paths.len() == self.secondaries.len() + } /// Internal method to make sure `self.factor_paths` is correct after an ascend method #[inline] fn fix_after_ascend(&mut self) { @@ -361,9 +375,29 @@ impl<'trie, V: Clone + Send + Sync + Unpin + 'trie, A: Allocator + 'trie> Zipper } } +/// Sharing is only reported in the last factor. In an earlier factor, the subtrie below a node continues +/// into the following factors, so the same node reached from a different factor is a different subtrie of +/// the product, and a cache keyed by `shared_node_id` would conflate the two. impl ZipperConcrete for ProductZipper<'_, '_, V, A> { - fn shared_node_id(&self) -> Option { self.z.shared_node_id() } - fn is_shared(&self) -> bool { self.z.is_shared() } + fn shared_node_id(&self) -> Option { + if !self.in_last_factor() { + return None + } + match self.factor_root() { + Some(_) if self.z.is_val() => None, + Some(factor) => factor.shared_node_id(), + None => self.z.shared_node_id(), + } + } + fn is_shared(&self) -> bool { + if !self.in_last_factor() { + return false + } + match self.factor_root() { + Some(factor) => factor.is_shared(), + None => self.z.is_shared(), + } + } } impl<'trie, V: Clone + Send + Sync + Unpin + 'trie, A: Allocator + 'trie> ZipperPathBuffer for ProductZipper<'_, 'trie, V, A> { @@ -525,6 +559,7 @@ impl<'trie, PrimaryZ, SecondaryZ, V> ZipperAbsolutePath fn root_prefix_path(&self) -> &[u8] { self.primary.root_prefix_path() } } +/// Sharing is only reported in the last factor. See the `ZipperConcrete` impl for [ProductZipper] impl<'trie, PrimaryZ, SecondaryZ, V> ZipperConcrete for ProductZipperG<'trie, PrimaryZ, SecondaryZ, V> where @@ -533,6 +568,9 @@ impl<'trie, PrimaryZ, SecondaryZ, V> ZipperConcrete SecondaryZ: ZipperMoving + ZipperPath + ZipperConcrete, { fn shared_node_id(&self) -> Option { + if self.factor_paths.len() < self.secondary.len() { + return None + } if let Some(idx) = self.factor_idx(true) { self.secondary[idx].shared_node_id() } else { @@ -540,6 +578,9 @@ impl<'trie, PrimaryZ, SecondaryZ, V> ZipperConcrete } } fn is_shared(&self) -> bool { + if self.factor_paths.len() < self.secondary.len() { + return false + } if let Some(idx) = self.factor_idx(true) { self.secondary[idx].is_shared() } else { @@ -1975,6 +2016,49 @@ mod tests { |btm: &mut PathMap<()>, path: &[u8]| -> _ { ProductZipperG::new::<[ReadZipperUntracked<()>; 0]>(btm.read_zipper_at_path(path), []) }); + + /// `is_shared` and `shared_node_id` across factor boundaries + #[test] + fn product_zipper_is_shared_across_factors() { + let mut a = PathMap::::new(); + for p in [&[1u8, 2, 1][..], &[1, 2, 1, 0], &[1, 2, 1, 3, 3], &[0], &[2, 2]] { a.set_val_at(p, 7); } + let b = a.clone(); + let mut z = ProductZipper::new(a.read_zipper_at_path(&[1u8, 2, 1]), [b.read_zipper()]); + let mut factor_roots = 0; + while z.to_next_step() { + let _ = (z.is_shared(), z.shared_node_id()); + if z.factor_root().is_some() { + factor_roots += 1; + assert!(z.is_shared(), "{:?}", z.path()); + } + } + assert!(factor_roots > 0); + } + + /// A node shared between factors must not share a `shared_node_id`, because its subtrie in the product + /// differs by factor. Otherwise a cached cata reuses the result from one factor in another. + #[test] + fn product_zipper_cata_cached_across_factors() { + // `s` is grafted in two places, and `b = a.clone()`, so the same node is in both factors + let mut s = PathMap::::new(); + for p in [&[5u8, 6][..], &[5, 7], &[9]] { s.set_val_at(p, 1); } + let mut a = PathMap::::new(); + a.write_zipper_at_path(&[1u8]).graft_map(s.clone()); + a.write_zipper_at_path(&[2u8]).graft_map(s.clone()); + a.set_val_at(&[3u8], 1); + let b = a.clone(); + + let alg = |_: &ByteMask, children: &mut [usize], val: Option<&u64>| children.iter().sum::() + val.is_some() as usize; + let expected = 7 + 7 * 7; + let pz = ProductZipper::new(a.read_zipper(), [b.read_zipper()]); + assert_eq!(pz.into_cata_cached(alg), expected); + let pzg = ProductZipperG::new(a.read_zipper(), [b.read_zipper()]); + assert_eq!(pzg.into_cata_cached(alg), expected); + + // Sharing is still reported in the last factor + let mut z = ProductZipper::new(a.read_zipper(), [b.read_zipper()]); + assert!(z.descend_to_existing(&[3u8, 1]) == 2 && z.is_shared() && z.shared_node_id().is_some()); + } } //POSSIBLE FUTURE DIRECTION: