Add Peekable::until

[apelisse]

Useful to parse since you can read from an iterator until a specific
predicate becomes true, without consuming the positive item.

Following up on #72310 (comment). I've also reused the same "issue", I know nothing about the process.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @joshtriplett (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[apelisse]

cc @jyn514

[apelisse]

Suggested change

	/// true, without consuming the last matching element.
	/// true, without consuming the last, matching element.

[pickfire]

Shouldn't think be take_until? We should also link take_while here for a non-consuming version. This is what I am looking for.

[pickfire]

Suggested change

	}
	}

[pickfire]

cross doc link, we probably don't need this. @jyn514 would be happy if you remove this

[pickfire]

Suggested change

	/// After true is returned, until()'s job is over, and the iterator
	/// After `true` is returned, `until()`'s job is over, and the iterator

[pickfire]

Can we remove flag? And use predicate: Option<P> to indicate that it can still be iterated on?

[pickfire]

skip_while uses fn check most likely to optimized the assembly (size and cache), how does this assembly looks like? @lzutao do you think this will have an impact? SkipWhile

impl<I: Iterator, P> Iterator for SkipWhile<I, P>
where
    P: FnMut(&I::Item) -> bool,
{
    type Item = I::Item;

    #[inline]
    fn next(&mut self) -> Option<I::Item> {
        fn check<'a, T>(
            flag: &'a mut bool,
            pred: &'a mut impl FnMut(&T) -> bool,
        ) -> impl FnMut(&T) -> bool + 'a {
            move |x| {
                if *flag || !pred(x) {
                    *flag = true;
                    true
                } else {
                    false
                }
            }
        }

        let flag = &mut self.flag;
        let pred = &mut self.predicate;
        self.iter.find(check(flag, pred))
    }

[pickfire]

Shouldn't we override try_fold and fold too?

[pickfire]

After working with iterators on fish-manpage-completions for quite a bit, I find this function lacking and we used the one from itertools peeking_take_while. I do think that this will introduce potential footgun for users, I hit quite a few myself too and it is hard to debug and fix, ideally we should write it down somewhere in the docs.

In case someone use peekable on a consuming iterator like SkipWhile, the original iterator will get consumed too, this took me quite a few hours to figure out the bug and fix it. An ASCII diagram to illustrate this

iterator -> | 1 2 3 4 5
iterator.skip_while (!= 2) -> 1 | 2 3 4 5
iterator.skip_while.peekable.take_until (== 3) -> 1 2 | 3 4 5 (note we only iterated 2 here)
The next time we iterate
iterator -> 1 2 | 3 4 5
What we wanted
iterator -> 1 | 2 3 4 5
What should be done
iterator.peekable.skip_while.take_until (not possible right now)

I feel confused myself now too, or maybe this should be discussed more in peekable. The issue I faced is fixed in rust-dc/fish-manpage-completions@efb321b

But the solution is as simple as moving peekable to iterator as early as possible to avoid any side effects.

[apelisse]

Yeah, I didn't know about peeking_take_while before, and that actually solves my problem so I don't have as much interested in doing this. I'm happy to polish this PR if we want to get it merged, but I don't have a strong case to push it now.

[jyn514]

Should this be closed? It sounds like @apelisse has lost interest and I don't know anyone else planning to use this API.

[apelisse]

I'm happy to close, feel free to re-open if needed.

[pickfire]

I do am interested in this since I need it but not that important to me to focus on this right now. It may create more footgun that I need to see into peeking.

[jhpratt]

FWIW I'd be interested in doing whatever changes the libs team desires. The lack of an API like this threw me off when I ran into it, and took a big of debug prints to figure out what was happening.

[jyn514]

@jhpratt sure, feel free to make a new PR with the changes from this one