Skip to content

bugfix!: over advancing in Cursor::advance_skipping_shared#162

Open
ParkMyCar wants to merge 2 commits into
jneem:mainfrom
ParkMyCar:fix/bug20-diff-eq
Open

bugfix!: over advancing in Cursor::advance_skipping_shared#162
ParkMyCar wants to merge 2 commits into
jneem:mainfrom
ParkMyCar:fix/bug20-diff-eq

Conversation

@ParkMyCar

Copy link
Copy Markdown

Bug

When Cursor::advance_skipping_shared finds equal leaves from the two cursors being compared, the index 0 element of the next non-shared leaf is skipped.

The buggy behavior that causes this issue is:

  1. find a shared leaf between the two cursors
  2. set the leavs of both Cursors to None and update the respective stacks to the next non-shared leaf
  3. call .next() on both cursors, now referencing index 0 of the non-shared leaves
  4. go back to the top of the loop, skipped_any == false and the leaves are not shared
  5. call .next() on both cursors again, advancing both cursors to index 1 (the bug)

Summary

The first commit of this PR is a failing test that exhibits the bug, the second commit are the requires changes to fix it.

AI disclosure: I didn't see any references to AI usage in your code of conduct, but wanted to callout that I used Claude to help me root cause the issue and implement a fix. I reviewed the change in the context of the codebase, and to the best of my knowledge this is the right fix, but wanted to be transparent in my use of AI :)

Fixes #161

@jneem

jneem commented Jun 21, 2026

Copy link
Copy Markdown
Owner

Thanks for the PR. I'm a little bothered by how complicated the looping becomes. For example, the following pattern is much simpler and seems pretty much the same:

        while let (Some(this), Some(that)) = (self.leaf, other.leaf) {
            // TODO: in edition 2024, this can be part of the loop condition
            if !std::ptr::eq(this.1, that.1) {
                break;
            }
            // <snip>
            self.next();
            other.next();
        }
        if !skipped_any {
            self.next();
            other.next();
        }

But the bigger thing that's bothering me actually predates this patch: why do the existing None checks treat self and other differently? Why can't other end earlier, and so other.leaf becomes None before self.leaf?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

OrdMap::diff silently drops ~1/8 of single-key value updates

2 participants