Choice

[jxv]

This is really a review request, not a PR.

[jxv]

It's worth reiterating that both body code for the fn parse_state are* the same.

[Marwes]

Use the ParseError::new function instead. Since the fields are all public it is possible to construct like this but I defined the new function in part to make sure that there is always an error (no empty errors vector).

In this case something like Error::Message("parser choice used with no parsers")

[Marwes]

Added a few comments on your PR. I'm still not sold on the idea of including a choice parser though. While it may be useful in parsec, I think that being forced to box every parser makes it much less useful. If you or someone else can show a compelling use case for using choice over doing p1.or(p2).or(p3) I would be inclined to include it though.

[Marwes]

Instead of having choice_slice and choice_vec, maybe it is possible to only have

fn choice<I>(ps: I) -> Choice<I> where I: Iterator + Clone, Iterator::Item: Parser

By having a Clone bound on iterator I'm thinking that every time the parser is called, the iterator can be cloned and iterated.

[jxv]

Here's my case to use ChoiceSlice: https://github.com/jxv/irc/commit/5d4d82aa2362b5282db5d674b3de3ad3565f461c#diff-9c0b8390b3d65627e2d9c5ad4536cc35R242
Scroll down a bit to let command = ...

I haven't actually got it to compile using the or equivalent because it takes a really long time. After an hour, I'd Ctrl-D it. It compiles within a reasonable amount of time with fewer or's though.*

[jxv]

This gives crashes the compiler:

fn choice<I>(ps: I) -> Choice<I> where I: Iterator + Clone, Iterator::Item: Parser

Either I'm doing something wrong or there's a legit bug.

[Marwes]

Your problem with the compile times is this issue rust-lang/rust#21231. Specializing the or parser before using could work for you but I can't be certain of course. That does seem like a common enough use case though to warrant the choice parsers inclusion.

I noticed you have a bug though in the parser, since it is LL(1) parsers that are created you need to use the try parser to get more look ahead. (Surround each pc::string parser call with try).
Also you can use the map function instead of parser.with(value(...))) which I find slightly clearer

[Marwes]

Anything that crashes the compiler is a bug and the code is actually wrong. It should be:

fn choice<I>(ps: I) -> Choice<I> where I: Iterator + Clone, <I as Iterator>::Item: Parser

EDIT: This seems to be the issue in rust rust-lang/rust#21867

[jxv]

So, okay. It's bug (or two).

Also, thanks for the pc::try tip! Although I don't think the parser.with(value(x)) matters much, it feels clear to me because it somewhat resembles parser >> return x.

Other than the compiler bug with the choice traits, is there anything else which can be improved?

[Marwes]

A couple of things left I think.

I'd argue that ChoiceSlice should hold &mut [P] which would allow you to remove the Clone bound on P in all places.

EDIT: I should mention that the Clone bound is probably fine for the most part since parsers should be really cheap to clone but since no other parser requires it I feel it is a little inconsistent.

I think you got the ChoiceSlice right though it was a little hard to follow the code. I think it may be easier to visualize if you think of the parser as returning three different values you need to deal with.

• Ok(_) => means we are done, just return it (which you have done)
• Err(Consumed(_)) => Since it is a LL(1) parser there is nothing more to be done here either, just return the error
• Err(Empty(_)) => This is the tricky one. If there is no error so far the error needs to be saved, if an Empty error has been saved since later the old and new error needs to be merged together.

I think you got all of these right but a test or two for the parser would be really useful. Only thing that is 'wrong' is that you don't exit early when a consumed error occurs (which is probably why you didn't notice that you needed try in your parsers). Arguably you should only construct the "choice is empty" error if the slice is actually empty, it should avoid an unnecessary allocation.

This ended up a longer response than I expected but choice is probably one of the trickier parsers to get right.

I should probably add some function which helps deal with these cases correctly though, since I know I had to write some extra boilerplate similar to this for a few other parsers (there is the combine method on Consumed but it doesn't work for this case).

[jxv]

Yeah, I think my misunderstanding of the relationship between try and Consumed attributed to the complexity. Thank you for explaning it thoroughly.

In regards to the &mut [P] and Clone for ChoiceSlice, how about using Rc instead?

[Marwes]

How would you use Rc? I don't really see it since if you have a parser inside an Rc you wouldn't be able to get a mutable reference to it.

I have experimented a bit with having parsers take themselves as &self but I didn't feel like it was a good idea since it would force all parsers to take functions as Fn instead of FnMut.

[jxv]

I was thinking that ref-counting might have* reduced the overhead from cloning. But yeah, you'd lose the ability to mutate.

[jxv]

Is this test okay? https://github.com/Marwes/parser-combinators/pull/16/files#diff-b4aea3e418ccdb71239b96952d9cddb6R395

[Marwes]

Looks fine to me, haven't done many tests which are more thorough than that. It might be good to have one call to parse which returns Err but I'd say it is good enough regardless.

[Marwes]

If you feel like you have nothing more to add to this I can merge this after a rebase (I can do the rebase if you want to, just remember to pull in the new master afterwards and work from that).

[jxv]

Yes, you can rebase and merge.

[Marwes]

Rebased and merged in 3bc8739