advance_cursor_while leaves cursor in incorrect positions

[Grimeh]

When using PeekMoreIterator::advance_cursor_while I observed some very odd behaviour that ran counter to my expectations and I believe doesn't match the method name and documentation.

The method documentation reads:

Moves the cursor forward until the predicate is no longer true.

But in practice the cursor is moved forward until the predicate is no longer true and then decremented once.

I believe the call to decrement_cursor should be removed as it leads to unexpected behaviour. Additionally it causes what I believe is a bug in the edge case where the first peeked element fails the predicate. I'll cover both issues with the current implementation.

Cursor points to the last successful element

The method calls decrement_cursor once when the predicate fails, leaving the cursor pointing to the last element where predicate(view) == true. Based on the method name and associated docs I would expect it to point to the next element where predicate(view) == false.

let my_array = [1, 2, 3, 4, 5];
let mut my_iter = my_array.iter().peekmore();
assert_eq!(my_iter.peek(), Some(&&1)); // cursor is 0

my_iter.advance_cursor();
assert_eq!(my_iter.peek(), Some(&&2)); // cursor is 1

my_iter.advance_cursor_while(|element| element.is_some_and(|value| **value < 4));

// intuitively expected behaviour
assert_eq!(my_iter.peek(), Some(&&4)); // cursor is 2, not equal!

// actual behaviour
assert_eq!(my_iter.peek(), Some(&&3)); // equal!

If this is the intended behaviour of the method, fair enough, but the method docs could use some rewording and expanding.

Cursor goes backwards if first element fails predicate

More concerningly, if the first peeked element fails the predicate the cursor is decremented from the value at the time of calling. If cursor > 0 when the method is called, then the value of cursor after the method call is cursor - 1. This is undesirable and leads to subtle bugs in user code (don't ask me how I know 🙂).

let my_array = [1, 2, 3, 4, 5];
let mut my_iter = my_array.iter().peekmore();
assert_eq!(my_iter.peek(), Some(&&1)); // cursor is 0

// advance by three
my_iter.advance_cursor_by(3);
assert_eq!(my_iter.peek(), Some(&&4)); // cursor is 3

// first element tested is 4, fails immediately
my_iter.advance_cursor_while(|element| element.is_some_and(|value| **value < 4));

// expected behaviour
assert_eq!(my_iter.cursor(), 3); // cursor is 2, not equal!
assert_eq!(my_iter.peek(), Some(&&4)); // not equal!

// actual behaviour
assert_eq!(my_iter.cursor(), 2); // equal!
assert_eq!(my_iter.peek(), Some(&&3)); // equal!

As you can see, the value of the cursor is less after the call than it was before the call. From the method name and docs I would never expect this to happen.

Proposed solution

Removing the decrement_cursor call will solve both of these issues and leave advance_cursor_while with what I believe is the intended behaviour based on the existing method documentation.

If the current behaviour is intended, I propose renaming the method to something along the lines of advance_cursor_to_last to communicate that the cursor is advanced to the last matching element (though the meaning of "last" here is a bit ambiguous) and adding a new advance_cursor_while that matches the existing method docs (quoted above).

Either way, the method docs could use expanding to clear up potential confusion.

I've opened a PR with my suggested implementation at #73.

[foresterre]

Thank you for your clear dissecting of, and description of the issue and also for contributing a fix!

[foresterre]

(don't ask me how I know 🙂).

Oof 😢. Sorry about that 😅.

I released your fix as v1.2.1.

I briefly considered releasing it as v2.0.0 to ensure no breakage would happen, but considering that the intent of the method was incorrectly implemented, I believe a patch release is the correct type of version bump.

Thanks again for discovering the bug (/bugs), carefully writing the issue and also providing a fix!

[Grimeh]

I was worried about the change in behaviour and wondered what the best course of action was, but ultimately came to the same conclusion as yourself.

Thanks heaps for releasing the fix so quickly 🙂 time to remove my workarounds!

Glad I could help.