rust-bakery / nom

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

Parser associated types #1626

Closed Geal closed 1 year ago

epage commented 1 year ago

Any hint if this improved type inference at all? Or any other benefit noticed besides dropping the number of generic parameters / phantom data?

coveralls commented 1 year ago

Pull Request Test Coverage Report for Build 3976558656

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details


Changes Missing Coverage Covered Lines Changed/Added Lines %
src/multi/mod.rs 29 32 90.63%
src/internal.rs 3 7 42.86%
<!-- Total: 50 57 87.72% -->
Files with Coverage Reduction New Missed Lines %
benchmarks/benches/json.rs 1 0.0%
benchmarks/benches/json_streaming.rs 1 0.0%
src/multi/mod.rs 1 90.74%
src/internal.rs 38 21.7%
<!-- Total: 41 -->
Totals Coverage Status
Change from base Build 3962072045: 0.03%
Covered Lines: 1554
Relevant Lines: 2501

💛 - Coveralls
Geal commented 1 year ago

so far the main issue with type inference was that I had to add the error type explicitely (F: Parser<I, Error=E> instead of F: Parser<I>) in most combinators. It is possible to remove the error generic type, like I've done for many0, but this makes the type signatures harder to read

epage commented 1 year ago

Just gave this a try in winnow-rs/winnow#220 and it looks like the compiler isn't happy about implementing Parser for u8 and other built-ins because there is nothing to constrain the generic E parameter on that is used for the Error associated type.

impl<I, E> Parser<I> for u8
where
    I: StreamIsPartial,
    I: Stream<Token = u8>,
    E: ParseError<I>,
{
    type Output = u8;
    type Error = E;

    fn parse_next(&mut self, i: I) -> IResult<I, u8, E> {
        crate::bytes::one_of(*self).parse_next(i)
    }
}
error[E0207]: the type parameter `E` is not constrained by the impl trait, self type, or predicates
   --> src/parser.rs:606:9
    |
606 | impl<I, E> Parser<I> for u8
    |         ^ unconstrained type parameter

This would require hard coding the error type which would be unpleasant to then make it seemless with other parts of the API.

I guess only O could be turned into an associated type. That would solve most type inference issues. The inconsistency on output types isn't great though.

epage commented 1 year ago

I found doubling down on PhantomData was another way of resolving the type inference issues: winnow-rs/winnow#222