shnewto / bnf

Parse BNF grammar definitions
MIT License
256 stars 22 forks source link

Update to Rust edition 2021 #90

Closed CrockAgile closed 2 years ago

CrockAgile commented 2 years ago

Changes

This attempts a non-breaking change of updating BNF's Rust edition (read more about editions).

The quick summary of editions is:

The current edition used by BNF is Rust 2015, which is the default. But it is good practice to upgrade crates when possible, because it unlocks future language features.

For example, one improvement made in Rust 2018 edition was the removal of redundant extern crate uses.

// rust 2015
extern crate rand;

use rand::thread_rng;

// rust 2018
use rand::thread_rng;

Issues

I don't know if this is a new issue from these changes, or running tests on new hardware, but I did encounter one flaky test:

fn recursion_limit() {
  let grammar: Result<Grammar, _> = "<nonterm> ::= <nonterm>".parse();
  assert!(grammar.is_ok(), "{:?} should be ok", grammar);
  let sentence = grammar.unwrap().generate();
  assert!(sentence.is_err(), "{:?} should be err", sentence);
  match sentence {
      Err(e) => match e {
          Error::RecursionLimit(_) => (),
          e => panic!("should should be Error::RecursionLimit: {:?}", e),
      },
      Ok(s) => panic!("should should be Error::RecursionLimit: {}", s),
  }
}

This test would fail sometimes, and pass others. 🤷‍♂️

codecov[bot] commented 2 years ago

Codecov Report

Merging #90 (b68f1cc) into main (6c9ebc1) will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##             main      #90   +/-   ##
=======================================
  Coverage   91.94%   91.94%           
=======================================
  Files          10       10           
  Lines         869      869           
=======================================
  Hits          799      799           
  Misses         70       70           
Impacted Files Coverage Δ
src/error.rs 74.57% <ø> (ø)
src/expression.rs 95.48% <ø> (ø)
src/grammar.rs 90.09% <ø> (ø)
src/parsers.rs 100.00% <ø> (ø)
src/production.rs 88.54% <ø> (ø)
src/term.rs 93.13% <ø> (ø)
tests/display.rs 100.00% <ø> (ø)
tests/from_str.rs 100.00% <ø> (ø)
tests/grammar.rs 75.00% <ø> (ø)
tests/iterate.rs 97.82% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 6c9ebc1...b68f1cc. Read the comment docs.

shnewto commented 2 years ago

I can revisit that recursion limit test, the whole method of "maybe too much recursion so monitor the stack and bail if we need to" logic is something I've been daydreaming about fixing since i implemented it 😅 . But don't think reimplementing that needs to be a blocker for this. The only thing I'd like to be somewhat confident about is that using the generate logic isn't also flakey now / can cause panics

CrockAgile commented 2 years ago

The only thing I'd like to be somewhat confident about is that using the generate logic isn't also flakey now / can cause panics

anything beyond running the tests I could do to get that confidence? seems to have passed on all platforms in CI, but I got a few stack explosion panics running locally

shnewto commented 2 years ago

Ah thanks for that effort! I kinda want to poke at it and see if I can reproduce on my end, so far I haven't seen it 🤔 Don't sweat me waiting on anything from you though, these updates are awesome. It's on me now, I'll try not to let this linger too much longer before making a call on the panic thing 🙏