Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Experimental feature "BTreeMap cursors": CursorMut.peek_prev() broken #111228

Closed
theandi667 opened this issue May 5, 2023 · 1 comment · Fixed by #111238
Closed

Experimental feature "BTreeMap cursors": CursorMut.peek_prev() broken #111228

theandi667 opened this issue May 5, 2023 · 1 comment · Fixed by #111238
Labels
A-collections Area: `std::collection` C-bug Category: This is a bug. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@theandi667
Copy link

theandi667 commented May 5, 2023

While using the experimental new API "BTreeMap cursors" (tracking issue: #107540) I noticed that the CursorMut.peek_prev() API, which is documented to return the last element of the map if the cursor is pointing to the "ghost" non-element, returns the first element of the map instead. Not only does this disagree with the documentation, the immutable Cursor.peek_prev() API does the right thing. Here's a small testcase:

#![feature(btree_cursors)]

fn main() {}

#[cfg(test)]
mod tests {

    use std::collections::BTreeMap;
    use std::ops::Bound;

    #[test]
    fn test_cursor() {
        let map = BTreeMap::from([(1, 1), (2, 2), (3, 3)]);

        let cursor = map.lower_bound(Bound::Excluded(&3));
        assert!(cursor.key().is_none());

        let prev = cursor.peek_prev();
        assert!(matches!(prev, Some((&3, _))));
    }

    #[test]
    fn test_cursor_mut() {
        let mut map = BTreeMap::from([(1, 1), (2, 2), (3, 3)]);

        let mut cursor = map.lower_bound_mut(Bound::Excluded(&3));
        assert!(cursor.key().is_none());

        let prev = cursor.peek_prev();
        // FAILS, prev is Some((&1, &1)) instead.
        assert!(matches!(prev, Some((&3, _))));
    }
}

The CursorMut.peek_prev() API should, as documented, return the last element of the map if the cursor points at the "ghost" non-element.

Instead, this happened: It currently returns the first element of the map.

Meta

rustc --version --verbose:

rustc 1.71.0-nightly (39c6804b9 2023-04-19)
binary: rustc
commit-hash: 39c6804b92aa202369e402525cee329556bc1db0
commit-date: 2023-04-19
host: aarch64-apple-darwin
release: 1.71.0-nightly
LLVM version: 16.0.2
@theandi667 theandi667 added the C-bug Category: This is a bug. label May 5, 2023
@workingjubilee workingjubilee added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label May 5, 2023
@DeltaF1
Copy link

DeltaF1 commented May 5, 2023

It looks like maybe the code for peek_prev in CursorMut was copied from peek_next, and the None case wasn't updated.

unsafe { self.root.reborrow() }
.as_mut()?
.borrow_mut()
.first_leaf_edge()
.next_kv()
.ok()?
.into_kv_valmut()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-collections Area: `std::collection` C-bug Category: This is a bug. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants