ACP: Peekable::map_next_if

[kennytm]

Proposal

Problem statement

Similar to Iterator::filter_map, sometimes it is more natural to combine Peekable::next_if and Option::map into a single operation.

Motivating examples or use cases

Consider the following code to unescape octal-escape sequences:

fn unescape_octal(a: &str) -> String {
    let mut it = Peekable::new(a.chars());
    let mut res = String::with_capacity(a.len());
    while let Some(c) = it.next() {
        if c == '\\' {
            let mut codepoint = 0;
            /*------------------------------------------------------*/
            /* read the following [0-7]* and parse into `codepoint` */
            /*------------------------------------------------------*/
            res.push(char::from_u32(codepoint).unwrap());
        } else {
            res.push(c);
        }
    }
    res
}

assert_eq!(unescape_octal(r"ab\377\570c\144e\146"), "abÿŸcdef");

There are currently 2 ways to implement /* read the following [0-7]* and parse into `codepoint` */, each with some small disadvantages:

1. Using .next_if(), but it requires evaluating the condition twice and there is an unnecessary .unwrap():

           while let Some(o) = it.next_if(|c| c.is_digit(8)) {
               codepoint = codepoint * 8 + o.to_digit(8).unwrap();
           }

2. Using .peek(), but the user has to remember to call .next()

           while let Some(o) = it.peek() && let Some(digit) = o.to_digit(8) {
               it.next();
               codepoint = codepoint * 8 + digit;
           }

Solution sketch

The .next_if()'s condition takes an impl FnOnce(&I::Item) -> bool as input, and returns the item on succeed.

We can create a variant which takes an impl FnOnce(&I::Item) -> Option<R> as input, with Some(r) representing "success" (which the item is consumed) and None "failure".

impl<I: Iterator> Peekable<I> {
    pub fn map_next_if_some<R>(&mut self, func: impl FnOnce(&I::Item) -> Option<R>) -> Option<R> {
        let item = self.peek()?;
        let result = func(item)?;
        self.peeked = None; // equivalent to `self.next()`
        Some(result)
    }
}

The motivating example can then be implemented as:

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

Alternatives

• I am not too fond of the name map_next_if_*. I expect better name suggestions.

• One may also provide an alternative interface which takes an owned value:

  pub fn map_next_if_ok<R>(&mut self, func: impl FnOnce(I::Item) -> Result<R, I::Item>) -> Option<R> {
      let unpeek = if let Some(item) = self.next() {
          match func(item) {
              Ok(result) => return Some(result),
              Err(item2) => Some(item2),
          }
      } else {
          None
      };
      self.peeked = Some(unpeek);
      None
  }
  
  // usage:
  while let Some(digit) = it.map_next_if_ok(|c| c.to_digit(8).ok_or(c)) {
      codepoint = codepoint * 8 + digit;
  }

  This allows us to provide an alternative solution to Add iter::Peekable::next_unpeek #557:

  while let Some(s) = parser.map_next_if_ok(|event| match event {
      pulldown_cmark::Event::Text(s) => Ok(s),
      other => Err(other),
  }) {
      do_something(s);
  }

  This is not saying this ACP is supposed to entirely replace .peek_slot() though. For instance the map_next_if approach cannot scale to multiple match arms doing different things like this:

  loop {
      let peek_slot = parser.peek_slot();
      match peek_slot.take() {
          Some(pulldown_cmark::Event::Text(text)) => do_something(text),
          Some(pulldown_cmark::Event::Code(code)) => do_something_else(code), // <-- new branch can be easily added
          other => {
             *peek_slot = other; // <-- still, forgetting to unpeek will be a foot gun.
             break;
          }
      }
  }

  Similar to .peek_slot() the user may unpeek an entirely irrelevant item with Err(custom_value). This is already possible with .peek_mut() (stable since 1.53) so it is not considered a problem.

Links and related work

Some real world example code which can be improved by this ACP (by eliminating some unreachable!() branches or redundant checks)

https://github.com/rust-lang/rust/blob/8df4a58ac47b778b093652d6190a6f9d54638774/compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs#L1290-L1300

    while let Some(next_expected_idx) = errors.map_next_if_some(|e| match e {
        Error::Missing(&next_expected_idx)
            if next_expected_idx == *missing_idxs.last().unwrap() + 1 =>
        {
            Some(next_expected_idx)
        }
        _ => None,
    }) {
        missing_idxs.push(expected_idx);
    }

https://github.com/max-heller/mdbook-pandoc/blob/197fea7c205ace88a4cda01afa9bd7be983f5b5a/src/preprocess.rs#L1017-L1026

    while let Some(t) = self.parser.map_next_if_ok(|event| match event {
        Event::Text(t) => Ok(t),
        Event::SoftBreak => Ok("\n".into()),
        other => Err(other),
    }) {
        text.push_str(&t);
    }

https://github.com/Rapptz/jimaku/blob/262aa0e370dbf74ea61b624cd9ecbe5415d19f15/src/japanese.rs#L259-L270

fn map_yoon_char(ch: &char) -> Option<char> {
    match *ch {
        'a' => Some('ゃ'),
        'u' => Some('ゅ'),
        'o' => Some('ょ'),
        _ => None,
    }
}
...
if let Some(yoon) = parser.map_next_if_some(map_yoon_char) {
    output.push('に');
    output.push(yoon);
} else {
    output.push(ch);
}

https://github.com/junelife/esp-idf-hal/blob/7db642c37bc2869b7da71e250340c1f56bc26f04/src/spi.rs#L908-L913

    let transactions = core::iter::from_fn(|| {
        spi_operations.map_next_if_ok(|operation| match operation {
            SpiOperation::Transaction(transaction) => Ok(transaction),
            other => Err(other),
        })
    });

https://github.com/parasyte/myn/blob/ba6980a9edd3c13cca6e1d643fae8c43fea730fd/src/ty.rs#L193-L196

    self.map_next_if_ok(|token| match token {
        TokenTree::Group(group) => Ok(group),
        other => Err(other),
    })
    .ok_or_else(|| spanned_error("Expected group", Span::call_site()))

https://github.com/nilehmann/backtracetk/blob/2dfc9d9a554ff8fb672e4cf357dd08d652b30b84/src/lib.rs#L138-L146

    let source_info = lines.map_next_if_ok(|line| match line {
        ParsedLine::BacktraceSource(source_info) => Ok(source_info),
        other => Err(other),
    });

What happens now?

This issue contains an API change proposal (or ACP) and is part of the libs-api team feature lifecycle. Once this issue is filed, the libs-api team will review open proposals as capability becomes available. Current response times do not have a clear estimate, but may be up to several months.

Possible responses

The libs team may respond in various different ways. First, the team will consider the problem (this doesn't require any concrete solution or alternatives to have been proposed):

• We think this problem seems worth solving, and the standard library might be the right place to solve it.
• We think that this probably doesn't belong in the standard library.

Second, if there's a concrete solution:

• We think this specific solution looks roughly right, approved, you or someone else should implement this. (Further review will still happen on the subsequent implementation PR.)
• We're not sure this is the right solution, and the alternatives or other materials don't give us enough information to be sure about that. Here are some questions we have that aren't answered, or rough ideas about alternatives we'd want to see discussed.

[traviscross]

Thanks for the proposal. We discussed this in the libs-api call today and decided to accept the by-value version under the name next_if_map:

impl<I: Iterator> Peekable<I> {
    pub fn next_if_map<R>(
        &mut self,
        f: impl FnOnce(I::Item) -> Result<R, I::Item>,
    ) -> Option<R> {
        let unpeek = if let Some(item) = self.next() {
            match f(item) {
                Ok(result) => return Some(result),
                Err(item) => Some(item),
            }
        } else {
            None
        };
        self.peeked = Some(unpeek);
        None
    }
}

(Regarding the name, it has to have "next" in it, since we're consuming the value. We're putting map at the end by analogy with filter_map.)

We're interested to hear and see examples if there are cases that aren't possible (or are inconvenient) to write with this version but that can be written with the by-ref version.

[kennytm]

rust-lang/rust#143702