Clippy check step for PRs

[CrockAgile]

Clippy has a lot of best practices, but BNF is in violation of a lot of them. Some would be breaking changes (like removing the non std::str::FromStr from_str public functions). Other changes require updating the parsers to nom 5 best practices.

Once that is done, would just need to revert previous attempt to re-enable clippy CI.

    Checking bnf v0.2.6
warning: use of deprecated item 'ws': whitespace parsing only works with macros and will not be updated anymore
  --> src/parsers.rs:6:1
   |
6  | / named!(pub prod_lhs< &[u8], Term >,
7  | |     do_parse!(
8  | |             nt: delimited!(char!('<'), take_until!(">"), ws!(char!('>'))) >>
9  | |             _ret: ws!(tag!("::=")) >>
10 | |             (Term::Nonterminal(String::from_utf8_lossy(nt).into_owned()))
11 | |     )
12 | | );
   | |__^
   |
   = note: `#[warn(deprecated)]` on by default
   = note: this warning originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)

warning: use of deprecated item 'ws': whitespace parsing only works with macros and will not be updated anymore
  --> src/parsers.rs:6:1
   |
6  | / named!(pub prod_lhs< &[u8], Term >,
7  | |     do_parse!(
8  | |             nt: delimited!(char!('<'), take_until!(">"), ws!(char!('>'))) >>
9  | |             _ret: ws!(tag!("::=")) >>
10 | |             (Term::Nonterminal(String::from_utf8_lossy(nt).into_owned()))
11 | |     )
12 | | );
   | |__^
   |
   = note: this warning originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)

warning: use of deprecated item 'ws': whitespace parsing only works with macros and will not be updated anymore
  --> src/parsers.rs:14:1
   |
14 | / named!(pub terminal< &[u8], Term >,
15 | |     do_parse!(
16 | |         t: alt!(
17 | |             delimited!(char!('"'), take_until!("\""), ws!(char!('"'))) |
...  |
21 | |     )
22 | | );
   | |__^
   |
   = note: this warning originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)

warning: use of deprecated item 'ws': whitespace parsing only works with macros and will not be updated anymore
  --> src/parsers.rs:14:1
   |
14 | / named!(pub terminal< &[u8], Term >,
15 | |     do_parse!(
16 | |         t: alt!(
17 | |             delimited!(char!('"'), take_until!("\""), ws!(char!('"'))) |
...  |
21 | |     )
22 | | );
   | |__^
   |
   = note: this warning originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)

warning: use of deprecated item 'ws': whitespace parsing only works with macros and will not be updated anymore
  --> src/parsers.rs:24:1
   |
24 | / named!(pub nonterminal< &[u8], Term >,
25 | |     do_parse!(
26 | |         nt: complete!(delimited!(char!('<'), take_until!(">"), ws!(char!('>')))) >>
27 | |         ws!(not!(complete!(tag!("::=")))) >>
28 | |         (Term::Nonterminal(String::from_utf8_lossy(nt).into_owned()))
29 | |     )
30 | | );
   | |__^
   |
   = note: this warning originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)

warning: use of deprecated item 'ws': whitespace parsing only works with macros and will not be updated anymore
  --> src/parsers.rs:24:1
   |
24 | / named!(pub nonterminal< &[u8], Term >,
25 | |     do_parse!(
26 | |         nt: complete!(delimited!(char!('<'), take_until!(">"), ws!(char!('>')))) >>
27 | |         ws!(not!(complete!(tag!("::=")))) >>
28 | |         (Term::Nonterminal(String::from_utf8_lossy(nt).into_owned()))
29 | |     )
30 | | );
   | |__^
   |
   = note: this warning originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)

warning: use of deprecated item 'ws': whitespace parsing only works with macros and will not be updated anymore
  --> src/parsers.rs:42:1
   |
42 | / named!(pub expression_next,
43 | |     do_parse!(
44 | |         ws!(char!('|')) >>
45 | |         ret: recognize!(peek!(complete!(expression))) >>
46 | |         (ret)
47 | |     )
48 | | );
   | |__^
   |
   = note: this warning originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)

warning: use of deprecated item 'ws': whitespace parsing only works with macros and will not be updated anymore
  --> src/parsers.rs:50:1
   |
50 | / named!(pub expression< &[u8], Expression >,
51 | |     do_parse!(
52 | |         peek!(term) >>
53 | |         terms: many1!(complete!(term)) >>
...  |
63 | |     )
64 | | );
   | |__^
   |
   = note: this warning originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)

warning: use of deprecated item 'ws': whitespace parsing only works with macros and will not be updated anymore
  --> src/parsers.rs:74:1
   |
74 | / named!(pub production< &[u8], Production >,
75 | |     do_parse!(
76 | |         lhs: ws!(prod_lhs) >>
77 | |         rhs: many1!(complete!(expression)) >>
...  |
86 | |     )
87 | | );
   | |__^
   |
   = note: this warning originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)

warning: use of deprecated item 'ws': whitespace parsing only works with macros and will not be updated anymore
  --> src/parsers.rs:74:1
   |
74 | / named!(pub production< &[u8], Production >,
75 | |     do_parse!(
76 | |         lhs: ws!(prod_lhs) >>
77 | |         rhs: many1!(complete!(expression)) >>
...  |
86 | |     )
87 | | );
   | |__^
   |
   = note: this warning originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)

warning: unneeded return statement
   --> src/grammar.rs:115:9
    |
115 |         return Ok(result);
    |         ^^^^^^^^^^^^^^^^^^ help: remove `return`: `Ok(result)`
    |
    = note: `#[warn(clippy::needless_return)]` on by default
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_return

warning: useless use of `format!`
  --> src/error.rs:55:32
   |
55 |             Needed::Unknown => format!("Data error: insufficient size, expectation unknown"),
   |                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using .to_string(): `"Data error: insufficient size, expectation unknown".to_string()`
   |
   = note: `#[warn(clippy::useless_format)]` on by default
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#useless_format

warning: you should consider deriving a `Default` implementation for `expression::Expression`
  --> src/expression.rs:16:5
   |
16 | /     pub fn new() -> Expression {
17 | |         Expression { terms: vec![] }
18 | |     }
   | |_____^
   |
   = note: `#[warn(clippy::new_without_default)]` on by default
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#new_without_default
help: try this
   |
10 | #[derive(Default)]
   |

warning: defining a method called `from_str` on this type; consider implementing the `std::str::FromStr` trait or choosing a less ambiguous name
  --> src/expression.rs:26:5
   |
26 | /     pub fn from_str(s: &str) -> Result<Self, Error> {
27 | |         match parsers::expression_complete(s.as_bytes()) {
28 | |             Result::Ok((_, o)) => Ok(o),
29 | |             Result::Err(e) => Err(Error::from(e)),
30 | |         }
31 | |     }
   | |_____^
   |
   = note: `#[warn(clippy::should_implement_trait)]` on by default
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#should_implement_trait

warning: you should consider deriving a `Default` implementation for `grammar::Grammar`
  --> src/grammar.rs:20:5
   |
20 | /     pub fn new() -> Grammar {
21 | |         Grammar {
22 | |             productions: vec![],
23 | |         }
24 | |     }
   | |_____^
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#new_without_default
help: try this
   |
14 | #[derive(Default)]
   |

warning: defining a method called `from_str` on this type; consider implementing the `std::str::FromStr` trait or choosing a less ambiguous name
  --> src/grammar.rs:32:5
   |
32 | /     pub fn from_str(s: &str) -> Result<Self, Error> {
33 | |         match parsers::grammar_complete(s.as_bytes()) {
34 | |             Result::Ok((_, o)) => Ok(o),
35 | |             Result::Err(e) => Err(Error::from(e)),
36 | |         }
37 | |     }
   | |_____^
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#should_implement_trait

warning: writing `&String` instead of `&str` involves a new object where a slice will do.
  --> src/grammar.rs:74:31
   |
74 |     fn traverse(&self, ident: &String, rng: &mut StdRng) -> Result<String, Error> {
   |                               ^^^^^^^
   |
   = note: `#[warn(clippy::ptr_arg)]` on by default
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#ptr_arg
help: change this to
   |
74 |     fn traverse(&self, ident: &str, rng: &mut StdRng) -> Result<String, Error> {
   |                               ^^^^
help: change `ident.clone()` to
   |
86 |         let nonterm = Term::Nonterminal(ident.to_string());
   |                                         ^^^^^^^^^^^^^^^^^

error: using `clone` on a double-reference; this will copy the reference instead of cloning the inner type
  --> src/grammar.rs:99:37
   |
99 |             Some(e) => expression = e.clone(),
   |                                     ^^^^^^^^^
   |
   = note: `#[deny(clippy::clone_double_ref)]` on by default
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#clone_double_ref
help: try dereferencing it
   |
99 |             Some(e) => expression = &(*e).clone(),
   |                                     ^^^^^^^^^^^^^
help: or try being explicit about what type to clone
   |
99 |             Some(e) => expression = &expression::Expression::clone(e),
   |                                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[CrockAgile]

Might also be nice to expand the clippy check to make automatic PR suggestions 🙀

[CrockAgile]

I think maybe this has already been done. Is that right @shnewto ?

[shnewto]

ah yeah totally! tho it'd still be cool to have clippy give autofeedback on PRs, maybe that could be a help wanted new issue 😄

[CrockAgile]

isn't it already giving autofeedback on PRs? I could swear I got some clippy comments on the Earley parser PR 🤔

[shnewto]

maybe! tho there's no reference to clippy in the github actions...

[CrockAgile]

oh right! that was coverage comments. ignore me 🙊

okay so this issue still open and valid