zesterer / chumsky

Write expressive, high-performance parsers with ease.
https://crates.io/crates/chumsky
MIT License
3.64k stars 155 forks source link

Confusing `Clone` requirement error when using recursive parser #666

Closed koshell closed 2 months ago

koshell commented 2 months ago

I'm trying to build a lexer for Lua. I'm having some very confusing issues dealing with recursive parsers. Currently I'm trying to implement this (ebnf):

exp ::=  "nil" 
         | "false" 
         | "true" 
         | Number 
         | String 
         | "..." 
         | function 
         | prefixexp 
         | tableconstructor 
         | exp binop exp 
         | unop exp

The tokens that don't require recursion appear to work fine. However when implementing recursion for unop exp I keep getting a very confusing error. Confusingly the error happens even when I'm not actually using the parser recursively. (Struct and function definitions at the end.)

This is the code (I'm not using recursive() so I could isolate the error):

fn expr<'t>() -> impl Parser<'t, &'t str, Vec<TokenSpan<'t>>> {
    let mut expr = Recursive::declare();

    let _expr = choice((
        text::keyword("nil").map_with(|_, e| (Token::Nil.with_span(e.span()).to_vec())),
        just("...").map_with(|_, e| (Token::Ellipsis.with_span(e.span()).to_vec())),
        bool().map(|token| (token.to_vec())),
        // number,
        // string().map(|token| (token.to_vec())),
        // function,
        // prefixexp,
        // tableconstructor,
        // unop().padded().then(expr).padded().,
    ));

    expr.define(_expr); // <== ERROR HERE

    return expr;
}
The error message (it's a lot) ```text error[E0277]: the trait bound `chumsky::combinator::Map>, lexer::tokens::TokenSpan<'_>, {closure@src\lexer\lua51.rs:110:20: 110:27}>: std::clone::Clone` is not satisfied --> src\lexer\lua51.rs:119:17 | 119 | expr.define(_expr); | ------ ^^^^^ the trait `std::clone::Clone` is not implemented for `chumsky::combinator::Map>, lexer::tokens::TokenSpan<'_>, {closure@src\lexer\lua51.rs:110:20: 110:27}>` | | | required by a bound introduced by this call | = note: required for `chumsky::combinator::Map>, lexer::tokens::TokenSpan<'_>, {closure@src\lexer\lua51.rs:110:20: 110:27}>` to implement `std::clone::Clone` = note: required because it appears within the type `(MapWith::Str> + Clone, &str, {closure@lua51.rs:108:39}>, MapWith, ..., ...>, ...)` = note: required for `Choice<(MapWith::Str> + Clone, &str, {closure@lua51.rs:108:39}>, MapWith, ..., ...>, ...)>` to implement `std::clone::Clone` note: required by a bound in `chumsky::recursive::Recursive::>::define` --> C:\Users\Zaiju\AppData\Local\Rust\cargo\registry\src\index.crates.io-6f17d22bba15001f\chumsky-1.0.0-alpha.7\src\recursive.rs:137:44 | 137 | pub fn define + Clone + MaybeSync + 'a + 'b>(&mut self, parser: P) { | ^^^^^ required by this bound in `Recursive::>::define` = note: the full name for the type has been written to 'B:\Projects\In Development\lua_parser\target\debug\deps\lua_parser-e4f40600767d3161.long-type-9460391373123907900.txt' = note: consider using `--verbose` to print the full type name to the console = note: the full name for the type has been written to 'B:\Projects\In Development\lua_parser\target\debug\deps\lua_parser-e4f40600767d3161.long-type-12725192073381237434.txt' = note: consider using `--verbose` to print the full type name to the console help: consider borrowing here | 119 | expr.define(&_expr); | + For more information about this error, try `rustc --explain E0277`. ```

I've been having some difficulty working out what is causing this issue. There are a lot of opaque types and closures which make the type signature a mess. My thoughts:

With that in mind I'm wondering if it's one of the intermediate types that isn't Clone. Specifically I noticed MapExtra (used in Parser.map_with(f: F)) isn't Clone. Could that be the issues? I didn't think it was capturing MapExtra since it's passed as a variable to the closure, but this isn't an area of Rust I'm terribly familiar with.

Wrapping the whole thing in Rc::new does solve the issue:

// ...
expr.define(Rc::new(_expr)); // <== no error
// ...

I'll use this as a workaround for now while I build up the rest of the tokens. Please let me know if there is a more idiomatic way to resolve this.

Struct and function definitions:

Token ```rust #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub(crate) enum Token<'t> { Boolean(bool), // ... snip ... Ellipsis, Nil, } impl<'t> Token<'t> { pub(crate) const fn with_span(self, span: SimpleSpan) -> TokenSpan<'t> { TokenSpan::from(self, span) } } ```
TokenSpan ```rust #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub(crate) struct TokenSpan<'t>(Token<'t>, SimpleSpan); impl<'t> TokenSpan<'t> { pub fn to_vec(self) -> Vec { vec![self] } pub const fn from(token: Token<'t>, span: SimpleSpan) -> Self { Self(token, span) } } ```
bool() ```rust fn bool<'t>() -> impl Parser<'t, &'t str, TokenSpan<'t>> { choice(( text::keyword("true").map_with(|_, e| Token::Boolean(true).with_span(e.span())), text::keyword("false").map_with(|_, e| Token::Boolean(false).with_span(e.span())), )) } ```
wackbyte commented 2 months ago

Does the bool function return impl Parser<...> or impl Parser<...> + Clone?

koshell commented 2 months ago

Does the bool function return impl Parser<...> or impl Parser<...> + Clone?

... That is a good point, currently its signature is fn bool<'t>() -> impl Parser<'t, &'t str, TokenSpan<'t>> (copied more or less verbatim from some of them doc). I had kind of forgotten that I'm passing around Traits and not Types. I'll try adding + Clone and see if that resolves it.

koshell commented 2 months ago

Yep, instantly solved. Confusing error message had short-circuited my brain and I had forgotten to think about the traits I'm passing around.

I'll make some kind of wrapper struct so I can return a concrete type and hopefully get more helpful errors next time. Thanks for the help @wackbyte.

zesterer commented 2 months ago

I've been wondering for a while whether Parser should implicitly have Clone as a supertrait. Technically it's unnecessary and a greater restriction placed upon the user, but in practice it's probably a usability win in 99% of cases: you're perhaps the dozenth person I've seen with a similar issue.

I also can't really see a use-case for a !Clone parser either: it would need to be non-recursive and also have a reason to consume itself, which no parser method requires anyway.