Skip to content

Conversation

@forzafedor
Copy link

@forzafedor forzafedor commented Mar 24, 2025

Hello

In this example I had the following problem!

use std::iter::Iterator;

use nom::bytes::complete::tag;
use nom::character::complete::alphanumeric1;
use nom::combinator::iterator;
use nom::sequence::{separated_pair, terminated};
use nom::IResult;


fn main() {

  let data = "key1:value1,key2:value2,";
  let mut nom_it = iterator(
    data,
    terminated(
      separated_pair(alphanumeric1, tag(":"), alphanumeric1),
      tag(","),
    ),
  );

  assert_eq!(Some(("key1", "value1")), nom_it.next());
  assert_eq!(Some(("key2", "value2")), nom_it.next());
  assert_eq!(None, nom_it.next());
  assert_eq!(None, nom_it.next());
  let parser_result: IResult<_, _> = nom_it.finish();
  assert_eq!(Ok(("", ())), parser_result);
}
`thread 'main' panicked at /home/user/nom/src/combinator/mod.rs:912:29:
called `Option::unwrap()` on a `None` value`

This happened because of extra calls to the next() function and
Here the state variable is set to None but is not set in these conditions

I think this behavior is incorrect.
In iterator std::iter.next() additional calls to the next function will return None. I think it would be logical for this iterator to have similar behavior.

I fixed it and added tests for this example!

Please review MR you have time, thank you!

@forzafedor forzafedor requested a review from Geal as a code owner March 24, 2025 15:39
@asibahi
Copy link

asibahi commented Mar 30, 2025

Worth mentioning I removed the panic source in my (bigger scope) iterator PR: #1841

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants