rust-bakery / nom

Rust parser combinator framework
MIT License
9.18k stars 792 forks source link

FlatMap does not properly implement Parser #1731

Open hsfzxjy opened 4 months ago

hsfzxjy commented 4 months ago

Example test case:

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=38ea81309c5b6c70c5ded853cbb30a34

use nom::character::complete::anychar;
use nom::Parser;

fn main() {
    anychar.flat_map(|_| anychar).parse("ab").unwrap();
}

https://github.com/rust-bakery/nom/blob/869f8972a4383b13cf89574fda28cb7dbfd56517/src/internal.rs#L262

https://github.com/rust-bakery/nom/blob/869f8972a4383b13cf89574fda28cb7dbfd56517/src/internal.rs#L364

Seems like a mismatch between above lines (notice Fn vs FnMut), making the flat_map unusable.

However I saw it fixed in current main branch

https://github.com/rust-bakery/nom/blob/e87c7da9fa2ddb943306369f6c6d9e914256edbc/src/internal.rs#L673

Will this fix also be ported to 7.x?

gl-yziquel commented 1 month ago

I'm not sure, but it seems to me that backporting such fixes would break semantic versioning.

I myself just upgraded from 7.1.3 to 8.0.0-alpha2 because I really needed the more flexible typing of 8 for everything flatmap related. (Related to on-the-fly parsing of function declarations w/r arguments for a lambda-calculus dialect.)

7.1.3 IMO has a broken flat_map implementation: the return type should be a Parser, and not an FnMut, especially in a context where it is so hard to give explicit names for anonymous closure types in rust.

I'd rather push for a 8.0.0 release or pre-release available on crates.io rather than a backport. Because of semver.

gl-yziquel commented 1 month ago

Oh ! 8.0.0-alpha2 is usable !! Great !