winnow-rs / winnow

Making parsing a breeze
https://docs.rs/winnow
Other
572 stars 44 forks source link

`map` with both Output and Stream #231

Open martinohmann opened 1 year ago

martinohmann commented 1 year ago

Please complete the following tasks

winnow version

v0.4.1

Describe your use case

See the discussion thread https://github.com/winnow-rs/winnow/discussions/230#discussioncomment-5620269

The general idea is to get access to the state of a winnow::stream::Stateful input in a map-like function so that it can be updated while mapping a parser's output.

Right now, only simple use cases are supported, like the one shown in the Stateful type docs.

Describe the solution you'd like

A method on the Parser trait that allows to modify the state in a map-like operation.

Ideally something like map_with_state that would both provide the output of the previous parser, and a reference to the state inside the Stateful so that it could be updated.

There was another idea in https://github.com/winnow-rs/winnow/discussions/230#discussioncomment-5620269 to generalize this to not only support the Stateful input type, but any input type by having something like map_with_input to expose a reference to the whole input, not just the state in a Stateful.

Alternatives, if applicable

No response

Additional Context

No response

martinohmann commented 1 year ago

@epage feel free to rename the issue to something better, I was struggling to find a good title.

martinohmann commented 1 year ago

I was thinking about it a bit more and checked the existing API of the Parser trait. We could design this in a way that better integrates with what already exist. Taking recognize/with_recognized and span/with_span as examples, we could:

Then you could simply access this via a map call. This might be enough to both support the Stateful use case and the use case mentioned in https://github.com/winnow-rs/winnow/discussions/230#discussioncomment-5620026.

I can try to compile a draft PR for this approach in the next few days and play around with it a bit to see if this would actually work in the way I envision.

martinohmann commented 1 year ago

I'm also not too nailed down on the naming yet. Maybe output_stream/with_output_stream would be a better fit to make it more obvious what's contained in the returned Stream.

martinohmann commented 1 year ago

Another name for this could be recognize_stream/with_recognized_stream since its behaviour is identical to recognize with the difference that it returns a Stream instead of Stream::Slice.

epage commented 11 months ago

chumsky consolidated a of their various map calls by having a type that carries all the information, see zesterer/chumsky#537

I don't think we should do that for the common cases but it provides a general form for complex cases.

The main difference with the proposal here is we'd want to also include Checkpoint from before the start of the parse operation. Naming will be difficult. Likely some minor API considerations that'd need to be taken into account.

epage commented 2 months ago

I wonder if we could use "tacit trait specialization", like axum and bevy, to allow a broad selection of function signatures to all work with one combinator.

https://willcrichton.net/notes/defeating-coherence-rust/

martinohmann commented 2 months ago

Do you envision something like map that could work as map(|output| ...) and map(|(input, output)| ...) using this TTS pattern?

I'm a little bit torn about this because on one hand it improves flexibility, but on the other hand it'll make it harder to reason about what certain APIs expect from the user.

I'm always a bit scared of APIs that involve a lot of generic type parameters -- especially when these are imposed by one or more traits -- because it sometimes makes it really hard to understand which input types satisfy the contract and which don't.

I'd probably take the stance of having more methods that are less generic but serve a distinct purpose rather than cramming too much into a single API.