Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into from

Conversation

cky
Copy link
Contributor

@cky cky commented Dec 9, 2022

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
Copy link
Contributor Author

cky commented Dec 11, 2022

I noticed that @epage had already implemented alternative 2 here, with exactly the same consideration as I had outlined: winnow-rs/winnow@98c5114

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

@cky
Copy link
Contributor Author

cky commented Jan 31, 2023

Given #1633, this patch is redundant.

@cky cky closed this Jan 31, 2023
@cky cky deleted the make-iterator-trait-bounds-match branch January 31, 2023 07:33
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.

None yet

1 participant