zesterer / chumsky

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

Empty choice panics when trying to parse. #668

Open DaniD3v opened 2 months ago

DaniD3v commented 2 months ago
struct ParsableType;

fn parser() -> impl Parser<char, ParsableType, Error = ParserError> {
    let parsers: [BoxedParser<'_, char, ParsableType, Simple<char>>; 0] = [];
    choice(parsers)
}

#[test]
fn this_panics() {
    let _ = parser().parse("");
}
---- parser::bin_ops::this_panics stdout ----
thread 'parser::bin_ops::this_panics' panicked at /home/notyou/.cargo/registry/src/index.crates.io-6f17d22bba15001f/chumsky-0.9.3/src/primitive.rs:945:30:
called `Option::unwrap()` on a `None` value

Having an empty choice is reasonable and useful. Consider having a Vec<Option<impl Parser>> and then using .into_iter().flatten().

If you don't agree there should at least be an error message telling the user what went wrong.

here's the full backtrace:

---- parser::bin_ops::this_panics stdout ----
thread 'parser::bin_ops::this_panics' panicked at /home/notyou/.cargo/registry/src/index.crates.io-6f17d22bba15001f/chumsky-0.9.3/src/primitive.rs:945:30:
called `Option::unwrap()` on a `None` value
stack backtrace:
   0: rust_begin_unwind
             at /rustc/bd53aa3bf7a24a70d763182303bd75e5fc51a9af/library/std/src/panicking.rs:665:5
   1: core::panicking::panic_fmt
             at /rustc/bd53aa3bf7a24a70d763182303bd75e5fc51a9af/library/core/src/panicking.rs:74:14
   2: core::panicking::panic
             at /rustc/bd53aa3bf7a24a70d763182303bd75e5fc51a9af/library/core/src/panicking.rs:148:5
   3: core::option::unwrap_failed
             at /rustc/bd53aa3bf7a24a70d763182303bd75e5fc51a9af/library/core/src/option.rs:2015:5
   4: core::option::Option<T>::unwrap
             at /rustc/bd53aa3bf7a24a70d763182303bd75e5fc51a9af/library/core/src/option.rs:965:21
   5: <chumsky::primitive::Choice<[A; N],E> as chumsky::Parser<I,O>>::parse_inner
             at /home/notyou/.cargo/registry/src/index.crates.io-6f17d22bba15001f/chumsky-0.9.3/src/primitive.rs:945:30
   6: chumsky::parse_recovery_inner
             at /home/notyou/.cargo/registry/src/index.crates.io-6f17d22bba15001f/chumsky-0.9.3/src/lib.rs:110:29
   7: chumsky::Parser::parse_recovery
             at /home/notyou/.cargo/registry/src/index.crates.io-6f17d22bba15001f/chumsky-0.9.3/src/lib.rs:201:9
   8: chumsky::Parser::parse
             at /home/notyou/.cargo/registry/src/index.crates.io-6f17d22bba15001f/chumsky-0.9.3/src/lib.rs:241:32
   9: greybolt::parser::bin_ops::this_panics
             at ./src/parser/bin_ops.rs:94:13
  10: greybolt::parser::bin_ops::this_panics::{{closure}}
             at ./src/parser/bin_ops.rs:93:17
  11: core::ops::function::FnOnce::call_once
             at /rustc/bd53aa3bf7a24a70d763182303bd75e5fc51a9af/library/core/src/ops/function.rs:250:5
  12: core::ops::function::FnOnce::call_once
             at /rustc/bd53aa3bf7a24a70d763182303bd75e5fc51a9af/library/core/src/ops/function.rs:250:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
DaniD3v commented 2 months ago

I just found out that this is already fixed on master. We should probably backport the fix to 0.9

zesterer commented 2 months ago

I agree, this should 'just work'. Monoid laws, etc. I'll look into backporting this over the next few days if it's a problem for you. That said, I'd recommend using 1.0 given that's where all the new dev work is happening.

DaniD3v commented 2 months ago

I agree, this should 'just work'. Monoid laws, etc. I'll look into backporting this over the next few days if it's a problem for you. That said, I'd recommend using 1.0 given that's where all the new dev work is happening.

I already migrated to 1.0 yesterday. For the 0.9 users I have this little workaround.

if (parsers.len()) > 0 {
    choice(parsers).boxed()
} else {
    empty().not().boxed()
}

What do you think about just releasing a 0.10 version? I think we're still quite far from 1.0 but the changes since 0.9 are already pretty significant. I can imagine it's also pretty bothersome to maintain 2 branches all the time.

zesterer commented 2 months ago

I'm happy to accept PRs to the 0.9 branch and perform releases for it. I wouldn't consider this to be a breaking change but a fix, too.