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

feat: Auto-detect whether to use complete/streaming parser #28

Merged
merged 14 commits into from
Dec 13, 2022
Merged

Conversation

epage
Copy link
Collaborator

@epage epage commented Nov 30, 2022

Previously in #6, I was transitioning parsers from returning FnMut(I) -> IResult<I, O, E> to structs that implement Parser so we could implement two unique parse traits, one for complete and one for streaming parsing. That ran into a series of problems, including not-as-great type inference.

This PR takes a different approach by making the type of parser a property of the Input, using const-generics. We can then specialize the implementation based on a const STREAMING: bool.

Potential downsides

  • Intentionally mixing complete and streaming parsers due to complications with the error type
  • Extra line noise for streaming users
    • I'm feeling like this will be acceptable as most of the noise is felt in tests and streaming is likely a less common case
  • Output types not carrying Streaming<T> type
    • For nom::bytes, I automatically convert to the Complete type but that might not always be the most accurate.
    • Unspecialzied functions, like recognize, can't be specialized without adding a new trait bound
    • We might want a dedicated trait for this as it feels incorrect to use InputIsStreaming::Complete for something unrelated to Complete vs Streaming and to use that trait in non-specialized places

Steps

  • Create context-sensitive streaming/complete parsers
  • Migrate all code off of streaming/complete-specific parsers
  • Deprecate streaming/complete-specific parsers
  • Ensure Streaming documentation is sufficient
  • Resolve anything else returning Incomplete (e.g. length_data)
  • Constrain nom::combinator::complete to streaming (move to trait, deprecating old version?)
    • As shown by length_value, complete can be useful in InputIsStreaming agnostic code.

Fixes rust-bakery/nom#1535

@coveralls
Copy link

coveralls commented Nov 30, 2022

Pull Request Test Coverage Report for Build 3688759797

  • 551 of 974 (56.57%) changed or added relevant lines in 16 files are covered.
  • 19 unchanged lines in 7 files lost coverage.
  • Overall coverage decreased (-2.6%) to 58.408%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/combinator/mod.rs 6 7 85.71%
src/bits/mod.rs 9 12 75.0%
src/number/complete.rs 2 5 40.0%
src/number/streaming.rs 2 5 40.0%
src/parser.rs 2 5 40.0%
src/bits/streaming.rs 26 32 81.25%
src/bytes/mod.rs 52 60 86.67%
src/bits/complete.rs 25 34 73.53%
src/character/complete.rs 58 68 85.29%
src/character/streaming.rs 44 67 65.67%
Files with Coverage Reduction New Missed Lines %
src/bytes/streaming.rs 1 50.92%
src/number/complete.rs 1 59.73%
src/character/streaming.rs 2 73.01%
src/number/streaming.rs 2 60.68%
src/bytes/complete.rs 3 66.86%
src/parser.rs 4 27.06%
src/lib.rs 6 0%
Totals Coverage Status
Change from base Build 3688567867: -2.6%
Covered Lines: 1754
Relevant Lines: 3003

💛 - Coveralls

@epage epage changed the title feat: Auto-detect whether to use complete/streaming parser WIP: feat: Auto-detect whether to use complete/streaming parser Nov 30, 2022
@epage epage marked this pull request as draft November 30, 2022 19:01
@epage epage force-pushed the port branch 6 times, most recently from eda142c to 26bd98f Compare December 6, 2022 21:19
@epage epage force-pushed the port branch 2 times, most recently from d240a9f to bb6b08c Compare December 9, 2022 18:46
This will allow wrapping the `Input` with types to carry more metadata
while still getting the needed output type.
BREAKING CHANGE: Some parsers added a new trait bound:
`Input: IntoOutput`
Features
- Conversions between methods
- Able to impl for one or the other mode
- Able to impl for both modes with specialization
Considering streaming parsers will need to deconstruct the error
anyways, we shouldn't be offering them an easy path that can make them
miss a case except when their testing is sufficient.
These were more entangled, so I didn't bother to do them by type.
@epage epage force-pushed the port branch 2 times, most recently from f790be8 to 39741a7 Compare December 13, 2022 18:58
Before, `length_data` and `length_value` were streaming-only (without
advertising it).  This now uses `InputIsStreaming` to specialize the
implementation.

`IntoOutput::from_output` reads weird but its good enough for now.  I
could see running into issues with this with recoverable errors but the
case we are using it atm doesn't care about them.
@epage epage marked this pull request as ready for review December 13, 2022 19:38
@epage epage changed the title WIP: feat: Auto-detect whether to use complete/streaming parser feat: Auto-detect whether to use complete/streaming parser Dec 13, 2022
@epage epage merged commit 28be52b into main Dec 13, 2022
@epage epage deleted the port branch December 13, 2022 19:43
epage added a commit that referenced this pull request Dec 16, 2022
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.

Allow types to be both stream and complete parsers
2 participants