rust-bakery / nom

Rust parser combinator framework
MIT License
9.38k stars 805 forks source link

alt short circuits with Incomplete return #1694

Closed cmleinz closed 1 year ago

cmleinz commented 1 year ago

The below code panics

Panics

use nom::IResult; // 7.1.3

fn main() {
    assert!(parse(b"FOO").is_ok());
}

fn parse(input: &[u8]) -> IResult<&[u8], &[u8]> {
    nom::branch::alt((
        nom::bytes::streaming::tag(b"FOOBAR"),
        nom::bytes::streaming::take(3_usize),  // This could succeed but is never tried
    ))(input)
}

The documentation for alt states: Tests a list of parsers one by one until one succeeds. Perhaps there's good reason for this, but it's unintuitive to me that a result of Err(Incomplete) from a streaming parser should cause an early return from alt when other branches might yet succeed. Perhaps there could be a note in the documentation explaining this?

Thanks for your work on the crate!

epage commented 1 year ago

To make sure I understand, the problem is that for incomplete input, you would expect alt to try another branch, rather than error out?

While I can't speak for Geal, I think this runs counter to how nom is setup for the streaming code path. In streaming, the expectation is that a decision shouldn't be made that will be different based on the amount of input that is available. Or in other words, streaming will fail if it doesn't have a definitive match. Take take_while, it could succeed with any amount of input but instead it doesn't succeed unless it finds a byte that it can't capture.

Geal commented 1 year ago

When working with streaming input, the parser should give the same result, independently of how the data was chunked. So for combinators like alt, if the child parser returned Incomplete when missing data, but could have succeeded on the complete data, then alt has to return Incomplete too instead of deciding to try another branch

cmleinz commented 1 year ago

Thanks I understand your point. I can also use nom::combinator::complete to supplement if I need to.