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: