Skip to content
This repository was archived by the owner on Apr 2, 2026. It is now read-only.

Add the Set combinator that applies a set of parser without order#850

Merged
zesterer merged 5 commits into
zesterer:mainfrom
peckpeck:set_combinator
Jul 28, 2025
Merged

Add the Set combinator that applies a set of parser without order#850
zesterer merged 5 commits into
zesterer:mainfrom
peckpeck:set_combinator

Conversation

@peckpeck

Copy link
Copy Markdown
Contributor

Add a Set combinator that takes a tuple of parsers and applies all of them in any order.

This permits parsing languages where arguments can be provided in any order.

set((A, B)) parses A then B or B then A depending on which comes first.

Parsers that match without making progress are applied last to make sure they have no better alternative, which allows optional parsers like A.or_not().

@zesterer

zesterer commented Jul 24, 2025

Copy link
Copy Markdown
Owner

Thanks for the PR! I do think there's a lot of potential use for this combinator, and I think the deferred progress trick is especially neat!

I am a little worried about the way in which it can subtly hide O(n^2) parsing behaviour. I assume that the goal is to replace something like

enum Item { A(A), B(B), C(C) }
choose((
    a.map(Item::A),
    b.map(Item::B),
    c.map(Item::C),
))
    .repeated_exactly::<3>()
    .try_map(|items| {
        let (mut a, mut b, mut c) = (None, None, None);
        for x in items {
            match x {
                Item::A(x) => a.replace(x).map(|_| Err(Error::custom("A was repeated"))).unwrap_or(Ok(()))?,
                Item::B(x) => b.replace(x).map(|_| Err(Error::custom("B was repeated"))).unwrap_or(Ok(()))?,
                Item::C(x) => c.replace(x).map(|_| Err(Error::custom("C was repeated"))).unwrap_or(Ok(()))?,
            }
        }
        Ok((a.unwrap(), b.unwrap(), c.unwrap()))
    })

with

set((a, b, c))

Definitely a big win, although expanding it out to even a dozen elements could easily produce hundreds of parser invocations. I'm not sure whether there's a good way to deal with that other than to recommend careful use in the docs.

@zesterer

Copy link
Copy Markdown
Owner

I'm definitely happy to see this merged, although:

  • I think tests should be added (and in particular, for the .or_not() case you mention - that seems potentially fragile to me)
  • A warning in the docs about quadratic behaviour should be added
  • The doc example could be changed a little: parsers like option_a vs option_b actually make this pessimistic behaviour worse, since the parser needs to keep reading all the way to end to find out that it's on the wrong branch. Perhaps a shopping list of apple, banana, carrot might be a more fun illustration.
  • A clarification about the expected behaviour if two set element parsers both match the same input. In the case of choice, we define the result to be the first parser that matches. We probably want to do the same here, but getting the wording right seems a bit more difficult.

@peckpeck

Copy link
Copy Markdown
Contributor Author

Thank you for your quick answer.

It does more than replacing the snippet you wrote, because it allows a, b and c in set((a,b,c)) to be parsers with different output type.

The quadratic behavior is unavoidable, I think. Its impact can be reduced if the user knows a usual ordering of items. But yes, it must be documented.

You're right about other suggestions, I think I will also provide implementations for vecs and arrays in addition to the current tuple one.

@zesterer

Copy link
Copy Markdown
Owner

Great. By the way, if you do a rebase then CI should pass. I really need to get round to pinning the rustc version.

@peckpeck

Copy link
Copy Markdown
Contributor Author

Done, please tell me what you think

@zesterer

Copy link
Copy Markdown
Owner

Looks great. One concern I have is that this parser always Emits its output, which means missing out on one of the big optimisations that chumsky brings to the table, I think go_or_emit should be made generic over M: Mode to achieve this (hopefully other parsers explain how this works, but if not then I'm happy to help out).

Other than that, I'd be happy to merge this!

@peckpeck

Copy link
Copy Markdown
Contributor Author

I tried making it generic over mode earlier, but I couldn't find How, let me try again.

I found how to do it in the tuple case, but for the array case, I need the const { None } syntax to initialize the array without having M::Output<O>: Copy, but this syntax is not available before rust 1.80

In the Vec case, I couldn't find how to map a Vec<M::Output<O>> to a M::Output<Vec<O>>.

@peckpeck

Copy link
Copy Markdown
Contributor Author

Found it, and added some clippy fixes linked to MSRV

@zesterer

Copy link
Copy Markdown
Owner

Ideal, thanks so much! Your work is really appreciated, this is a great addition.

@zesterer
zesterer merged commit 7de553a into zesterer:main Jul 28, 2025
4 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants