meh / rust-terminfo

Terminal information for Rust.
Other
40 stars 15 forks source link

Upgrade to Nom 7 #25

Closed andersk closed 1 year ago

andersk commented 1 year ago

Nom 5 and 6 give build warnings in current Rust nightly that will become errors in future Rust versions:

$ cargo build
   Compiling terminfo v0.7.5 (/home/anders/rust/rust-terminfo)
    Finished dev [unoptimized + debuginfo] target(s) in 1.83s
warning: the following packages contain code that will be rejected by a future version of Rust: nom v5.1.2
note: to see what the problems were, use the option `--future-incompat-report`, or run `cargo report future-incompatibilities --id 417`

The actual warnings are many copies of

> warning: trailing semicolon in macro used in expression position
>    --> /home/anders/.cargo/registry/src/github.com-1ecc6299db9ec823/nom-5.1.2/src/branch/macros.rs:520:90
>     |
> 520 | ...option::Option::None), $($rest)*);
>     |                                     ^
>     |
>    ::: /home/anders/.cargo/registry/src/github.com-1ecc6299db9ec823/nom-5.1.2/src/branch/mod.rs:263:1
>     |
> 263 | permutation_trait!(FnA A, FnB B, FnC C, FnD D, FnE E, FnF F, FnG G, FnH H, FnI I, FnJ J, FnK K, FnL L, FnM M, FnN N, FnO O, FnP P, FnQ Q, FnR R, FnS S, FnT T, FnU ...
>     | --------------------------------------------------------------------------------------------------------------------------------------------------------------------- in this macro invocation
>     |
>     = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
>     = note: for more information, see issue #79813 <https://github.com/rust-lang/rust/issues/79813>
>     = note: `#[allow(semicolon_in_expressions_from_macros)]` on by default
>     = note: this warning originates in the macro `permutation_init` which comes from the expansion of the macro `permutation_trait` (in Nightly builds, run with -Z macro-backtrace for more info)
wez commented 1 year ago

I have used nom in the past, but it was a while ago. I've quickly skimmed this and it seems fine.

The nature of most parsing code is that it tends to be hard to really get a good sense of where the corner cases might be and that you live or die by your test cases/coverage when it comes to having confidence in this sort of change.

I haven't looked at the tests in this crate in enough detail to have an opinion on the level of confidence myself, so my disposition is: if @andersk feels like there's an area that might need some extra scrutiny before merging we should look into it, but otherwise, if you're both thinking that the tests exercise enough of the parser, then let's merge it!

andersk commented 1 year ago

If there are corner cases to which extra attention might be due, they’d be in EOF handling.

Nom has streaming and complete versions of many primitive combinators, where the streaming version throws an Incomplete error on EOF, while the complete version treats EOF as a regular success or failure. I went with the streaming versions, because that’s what the old macros expanded to:

The one exception was that I used nom::bytes::complete::take_till to match the behavior of our custom take_until_or_eof! macro. take_until or take_while don’t do what we want, nor do streaming versions. Fortunately, this detail is well covered by tests.

I noted that our Incomplete handler just panics like regular failures, so it’s possible that switching everything to the complete versions would have been more idiomatic and avoided the need for the complete!nom::combinator::complete combinator. But that would have demanded more careful scrutiny, so we can leave it to a future refactor.

TL;DR: I’m pretty confident this is fine as a one-to-one transliteration.

SabrinaJewson commented 1 year ago

@wez What’s the plan on merging this? I’d be happy to, so if you read this please go ahead.

a1ien commented 1 year ago

Will be nice if you make new 0.7.6 release.

str4d commented 1 year ago

It can't be an 0.7.6 release because this PR made changes to the public API (e.g. terminfo::parser::compiled::parse exposes nom 5 in the public API). This will need to be released in an 0.8.0.