Add PeekableIterator trait

[wmstack]

View all comments

Tracking issue: #132973

Implementation

pub trait PeekableIterator: Iterator {
  // required method
  fn peek_with<T>(&mut self, func: impl for<'a> FnOnce(Option<&'a Self::Item>) -> T) -> T

  // provided methods
  fn peek_map<T>(&mut self, func: impl for<'a> FnOnce(&'a Self::Item) -> T) -> Option<T>
  fn next_if(&mut self, func: impl FnOnce(&Self::Item) -> bool) -> Option<Self::Item>
  fn next_if_eq<T>(&mut self, expected: &T) -> Option<Self::Item>
  where
    Self::Item: PartialEq<T>,
    T: ?Sized,
}

Todo: Determine which structures need to implement PeekableIterator:

• slice::iter
• str::Chars
• vec::IntoIter
• Peekable<T>

Unresolved Issues:

• Add another trait for iterators that can peek() without mutating?

[rustbot]

r? @tgross35

rustbot has assigned @tgross35.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[theemathas]

Maybe the correct function signature is this?

fn peek<T>(&mut self, callback: impl for<'a> FnOnce(Option<&'a Self::Item>) -> T) -> T

This would mean, for example, that str::Chars would be able to implement this without having a field inside the iterator that contains a char. Instead, it would be able to create a local variable in the peek method that stores the char.

[theemathas]

Suggested change

	// SAFETY: by type invariant, the `end_or_len` field is always
	// non-null for a non-ZST pointee. (This transmute ensures we
	// get `!nonnull` metadata on the load of the field.)
	if ptr == crate::intrinsics::transmute::<$ptr, NonNull<T>>(end_or_len) {
	if ptr.as_ptr() == end_or_len {

[wmstack]

Hmm, I was not able to write it this way as Rust determines the types differ in their mutability. In general, this code was based on the next() method generic over both Iter and IterMut

[tgross35]

What exactly was the error? Seems possible to resolve by adding a $as_ptr metavar that is either as_ptr or as_mut_ptr depending on which one is being implemented (like into_ref).

next is a bit magical, others should try to be simpler if possible. cloning and advancing should be fine, it's cheap.

[tgross35]

Cloning actually wouldn't work for IterMut since it doesn't implement it. But that actually brings up a good point; the safety comments need to say why it is sound to hand out a&mut T.

[tgross35]

This doesn't seem to be tested at all. Please make sure that even unstable API gets helpful examples, aside from checking the implementation they serve as a very useful smoke test for whether or not the API makes sense in the first place.

Additionally, the type-specific impls need tests in library/coretests

View changes since this review

[bluebear94]

@wmstack Are you still open to working on this PR? If not, then I’m willing to take over the work on this.

[wmstack]

@bluebear94 Feel free to pick this up as I’m tied up with coursework at the moment.

[bors]

☔ The latest upstream changes (presumably #147353) made this pull request unmergeable. Please resolve the merge conflicts.

[rustbot]

Some changes occurred in src/tools/cargo

cc @ehuss

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[wmstack]

I have squashed and rebased to the latest main branch.

Seems to be a problem with one of the doctests. In any case the line numbers provided for which assertion failed are concerning as they point at the wrong line numbers.

RUST_BACKTRACE=full ./x test library/core --doc --test-args "iter::traits::peekable::PeekableIterator::peek_with"

I get that the 19th line has an error, and that is an empty line:

The correct line in this instance is line number 24. Maybe the tool omits some spaces or comments, which I don't think is helpful.

[wmstack]

@tgross35 I believe the doctests should all compile but just a few clarifications about the coretests.

• Adding the unstable feature to the relevant file shows:

However the crate root at library/coretests/lib.rs is intentionally left empty. What do you think is the right approach here.

• Should the peekable iterator coretests be in their own file? There is only four trait files and inside library/coretests/iter/traits/iterator.rs there aren't really any other traits being imported they seem to be available through the prelude or something. Is this because they are stabilised?

Edit: Even if I ignore the intentionally empty comment at the crate root, it doesn't really get detected despite being used in a few un-committed tests:

[workingjubilee]

have you tried adding it to https://github.com/rust-lang/rust/blob/main/library/coretests/tests/lib.rs instead?

[workingjubilee]

rustc UX issue: "crate root" file not identified, can be confusing in case of test files. off to file a bug! #157001

[wmstack]

@rustbot ready