Fix: ProductZipper crashes - #135
Merged
luketpeterson merged 7 commits intoSep 25, 2026
Merged
luketpeterson merged 7 commits into
luketpeterson merged 7 commits into
Conversation
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.
With nothing below the base and no sibling, k_path_default_internal never reached its exit and spun. It also let k = 0 step sideways from the base. Stop when back at the base.
A factor was entered below a primary path that doesn't exist, and a sibling step at depth 0 dropped the factor record although the core zipper can't leave the factor there. Later moves then saw factor roots the zipper didn't know about, and is_shared unwrapped None.
imlvts
force-pushed
the
fix/product-zipper-factor-bookkeeping-root
branch
3 times, most recently
from
September 23, 2026 15:10
1b71605 to
2c47b5b
Compare
| #[inline] | ||
| fn ensure_descend_next_factor(&mut self) { | ||
| if self.factor_paths.len() < self.secondaries.len() && self.z.child_count() == 0 { | ||
| if self.factor_paths.len() < self.secondaries.len() && self.z.child_count() == 0 && self.z.path_exists() { |
There was a problem hiding this comment.
Hmm, this used to be there, but it cost performance and it wasn't clear why this was needed.
Collaborator
There was a problem hiding this comment.
Added the does_not_enter_factor_at_nonexistent_path test to demonstrate why the check is needed. Hopefully cost of the check is minimal because it's only paid when stepping across factors.
# Conflicts: # src/zipper.rs
…er PR that was already merged
Fixing to_next_sibling_byte and to_prev_sibling_byte on Generic and Dependent PZs when the zipper is at the root Adding test to demonstrate the need for an exists check in the Concrete PZ when descending across factors
…rvative fix, but it leaves a lot of sharing on the table. This will be revisited in Adam-Vandervorst#136
Collaborator
|
Ready to merge this. Ended up doing something conservative for is_shared & shared_node_id which is valid but leaves a lot of sharing on the table. Will be revisited in #136 Merging. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix the default k-path walk looping forever at a leaf
With nothing below the base and no sibling, k_path_default_internal never
reached its exit and spun. It also let k = 0 step sideways from the base.
Stop when back at the base.
Fix ProductZipper factor bookkeeping at the root
A factor was entered below a primary path that doesn't exist, and a
sibling step at depth 0 dropped the factor record although the core
zipper can't leave the factor there. Later moves then saw factor roots
the zipper didn't know about, and is_shared unwrapped None.