rust-bakery / nom

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

Make `nom::combinator::iterator`'s trait bounds match `impl Iterator for &mut ParserIterator`'s #1584

Closed cky closed 1 year ago

cky commented 1 year ago

A ParserIterator's primary function is to be an iterator. I'd go so far as to say that a ParserIterator that can't function as an iterator is broken.

Currently, nom::combinator::iterator expects a Parser<Input, Output, Error> for its f parameter, but the returned ParserIterator object is not actually usable as an iterator unless f also conforms to FnMut(Input) -> IResult<Input, Output, Error>, because of the constraints on impl Iterator for &mut ParserIterator.

In particular, passing in a (type-erased) impl Parser<Input, Output, Error> results in an unusable iterator, even if its underlying (unerased) type actually conforms to FnMut(Input) -> IResult<Input, Output, Error>.

So, to avoid confusion for callers, let's constrain the f parameter to what's actually expected by the impl Iterator.


Alternatives

Of course, ideally, the constraints on impl Iterator for &mut ParserIterator would be loosened to accept a Parser<Input, Output, Error> instead. However, I don't know of any current way to do that without breaking compatibility:

  1. The most ideal solution would be to give Parser an associated Output type. But you can't do that without affecting all callers.
  2. Another possibility is to add an Output type parameter to ParserIterator. But then callers have to be adjusted to specify ParserIterator with 4 type parameters instead of 3, so that also breaks compatibility.
cky commented 1 year ago

I noticed that @epage had already implemented alternative 2 here, with exactly the same consideration as I had outlined: https://github.com/epage/nom-experimental/commit/98c51143d649b6c5c44eb06df042bf4721301889

Given the precedent, I'm happy to retract this PR if that's the solution approach that we'd like to go with.

cky commented 1 year ago

Given #1633, this patch is redundant.