stabilize Peekable::next_if_map (#![feature(peekable_next_if_map)])

[jdonszelmann]

Stabilization report

Summary

#![feature(peekable_next_if_map)] is a variation of next_if on peekable iterators that can transform the peeked item. This creates a way to take ownership of the next item in an iterator when some condition holds, but put the item back when the condition doesn't hold. This pattern would otherwise have needed unwraps in many cases.

Tracking issue

What is stabilized

impl<I: Iterator> Peekable<I> {
    pub fn next_if_map<R>(
        &mut self,
        f: impl FnOnce(I::Item) -> Result<R, I::Item>,
    ) -> Option<R> {
        ..
    }
}

Example usage adapted from the ACP:

let mut it = Peekable::new("123".chars());

while let Some(digit) = it.next_if_map(|c| c.to_digit(8)) {
    codepoint = codepoint * 8 + digit;
}

Nightly use

At the moment, this feature is barely used in nightly, though I've found multiple good uses for it in my own projects, hence my pushing for stabilization. It makes the kind of patterns used in recursive descent parsing super concise and maybe with its stabilization it will find more use.

Test coverage

Besides a quite comprehensive doctest, this feature is tested (including panicking in the closure) here:

rust/library/coretests/tests/iter/adapters/peekable.rs

Lines 275 to 359 in c880acd

	#[test]
	fn test_peekable_next_if_map_mutation() {
	fn collatz((mut num, mut len): (u64, u32)) -> Result<u32, (u64, u32)> {
	let jump = num.trailing_zeros();
	num >>= jump;
	len += jump;
	if num == 1 { Ok(len) } else { Err((3 * num + 1, len + 1)) }
	}
	let mut iter = once((3, 0)).peekable();
	assert_eq!(iter.peek(), Some(&(3, 0)));
	assert_eq!(iter.next_if_map(collatz), None);
	assert_eq!(iter.peek(), Some(&(10, 1)));
	assert_eq!(iter.next_if_map(collatz), None);
	assert_eq!(iter.peek(), Some(&(16, 3)));
	assert_eq!(iter.next_if_map(collatz), Some(7));
	assert_eq!(iter.peek(), None);
	assert_eq!(iter.next_if_map(collatz), None);
	}
	#[test]
	#[cfg_attr(not(panic = "unwind"), ignore = "test requires unwinding support")]
	fn test_peekable_next_if_map_panic() {
	use core::cell::Cell;
	use std::panic::{AssertUnwindSafe, catch_unwind};
	struct BitsetOnDrop<'a> {
	value: u32,
	cell: &'a Cell<u32>,
	}
	impl<'a> Drop for BitsetOnDrop<'a> {
	fn drop(&mut self) {
	self.cell.update(|v| v | self.value);
	}
	}
	let cell = &Cell::new(0);
	let mut it = [
	BitsetOnDrop { value: 1, cell },
	BitsetOnDrop { value: 2, cell },
	BitsetOnDrop { value: 4, cell },
	BitsetOnDrop { value: 8, cell },
	]
	.into_iter()
	.peekable();
	// sanity check, .peek() won't consume the value, .next() will transfer ownership.
	let item = it.peek().unwrap();
	assert_eq!(item.value, 1);
	assert_eq!(cell.get(), 0);
	let item = it.next().unwrap();
	assert_eq!(item.value, 1);
	assert_eq!(cell.get(), 0);
	drop(item);
	assert_eq!(cell.get(), 1);
	// next_if_map returning Ok should transfer the value out.
	let item = it.next_if_map(Ok).unwrap();
	assert_eq!(item.value, 2);
	assert_eq!(cell.get(), 1);
	drop(item);
	assert_eq!(cell.get(), 3);
	// next_if_map returning Err should not drop anything.
	assert_eq!(it.next_if_map::<()>(Err), None);
	assert_eq!(cell.get(), 3);
	assert_eq!(it.peek().unwrap().value, 4);
	assert_eq!(cell.get(), 3);
	// next_if_map panicking should consume and drop the item.
	let result = catch_unwind({
	let mut it = AssertUnwindSafe(&mut it);
	move || it.next_if_map::<()>(|_| panic!())
	});
	assert!(result.is_err());
	assert_eq!(cell.get(), 7);
	assert_eq!(it.next().unwrap().value, 8);
	assert_eq!(cell.get(), 15);
	assert!(it.peek().is_none());
	// next_if_map should *not* execute the closure if the iterator is exhausted.
	assert!(it.next_if_map::<()>(|_| panic!()).is_none());
	assert!(it.peek().is_none());
	assert_eq!(cell.get(), 15);
	}

History

• ACP: ACP: Peekable::map_next_if libs-team#613 accepted with ACP: Peekable::map_next_if libs-team#613 (comment)
• implementation: core: add Peekable::next_if_map #143725 with tests, and no issues reported since july.

Acknowledgments

ACP, implementation and tracking issue for this feature all by @kennytm <3

[rustbot]

r? @joboet

rustbot has assigned @joboet.
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

[joboet]

r? libs-api

[ais523]

IIRC stabilisation reports are often looking for personal experiences with an API?

In my case, I haven't been using Peekable::next_if_map because a) I didn't realise it existed and b) it's nightly-only. Howver, I did really need it for a program I was writing, to the extent that I considered implementing it myself at one point, and so I have experience with the type of code that would use this and what the alternatives look like.

Here's an example of the sort of code I ended up writing, due to not having next_if_map available (Nybble is a project-specific type that represents a hexadecimal digit):

while let Some(nybble) = iter.next_if(|b| b.is_ascii_hexdigit()) {
    payload.push(Nybble::from_hexdigit(nybble as char).expect(
        "ASCII hexdigits are valid nybbles"));
}

but this really would be much better written as

while let Some(nybble) = iter.next_if_map(|b| Nybble::from_hexdigit(*b as char)) {
    payload.push(nybble);
}

which avoids the double check. (In this particular case there are no TOCTOU issues because nothing can change whether a particular byte is a valid ASCII hex digit, but checking a condition twice is a code smell and occasionally a security vulnerability, so it is generally considered good style to check only once and next_if_map makes that possible.)

The existing API isn't ideal for my use-case, though – the previous code-block doesn't actually match the signature of the next_if_map which is being proposed for stabilisation, so if I were using the version that's currently being proposed for stabilisation, I would have had to write it like this instead:

while let Some(nybble) = iter.next_if_map(|b| Nybble::from_hexdigit(b as char).ok_or(b)) {
    payload.push(nybble);
}

which is still cleaner than the code without next_if_map, but a bit uglier than the version that copies out of the peek slot. Note that the "example usage" in the stabilisation report does the same thing (char::to_digit() returns an Option<u32>, not a Result<u32, char>) – either the example needs updating or the API needs changing.

The API proposed for stabilisation also makes it possible to change the element in the iterator's peek slot (because nothing forces the Err of a returned result to be the same I::Item that was passed in), which is a bit weird.

I guess the correct API depends on how often Peekable is used with non-Copy types. In my experience, Peekable is mostly used for parsers, which can generally cheaply copy the tokens that they're parsing, but perhaps other people have other uses that make the more general API more useful.

[jdonszelmann]

fair enough, and thanks for your thoughts. I couldn't find that many uses on github yet, so these kinds of experiences help <3.

I personally used it a bunch too now, specifically for parsers as I mentioned. I had some kinds of tokens that contained contents: like strings containing a reference to the contents of the string. Matching on those tokens and getting the contents out is impossible to do as neatly as with this API. In fact, the entire pattern became a single line :)