Add a CI workflow to run Clippy

[gendx]

We should run Clippy in one of the GitHub workflows.

[jmichelp]

Just did a quick check. At the moment we're blocked on:

• libtock-rs 0.1.0 which has some clippy errors
• tock which also has some errors (checked by running tools/run_clippy.sh in Tock repository

Issues with Tock kernel are already solved upstream so this one should go away as soon as we bump the commit id. For libtock-rs, either we fork it and include the proper clippy annotations/fixes or we move to a more recent libtock-rs.

[ia0]

Alternatively we may also just run clippy on what we own (with cargo clippy -p ctap2 --features=std or moving in the library directory and running cargo clippy if needed).

[jmichelp]

This won't work. cargo clippy -p ctap2 will still have dependencies to libtock-rs and fail. Same thing applies to libraries/crypto which depends on libtock-rs too.

So at the moment it can only be run against libraries/cbor.

[gendx]

So I guess this is blocked on #95. Once libtock is in sync with upstream, we can see if there are any remaining lints triggering on that front, and if yes discuss it upstream. In the worst case, we'd maintain our own patches to fix the lints.

[jmichelp]

Exactly. The lastest libtock-rs has no clippy errors when run with the default configuration and 1 small issue with src/sensors/ninedof.rs when run with the same configuration as for the kernel. And the fix is really trivial.

[ia0]

cargo clippy -p ctap2 will still have dependencies to libtock-rs and fail.

cargo clippy -p ctap2 --target=thumbv7em-none-eabi yes, but for me cargo clippy -p ctap2 --features=std only gives warning in our code. Maybe the -p option doesn't work when one changes the target.

[gendx]

cargo clippy -p ctap2 will still have dependencies to libtock-rs and fail.

cargo clippy -p ctap2 --target=thumbv7em-none-eabi yes, but for me cargo clippy -p ctap2 --features=std only gives warning in our code. Maybe the -p option doesn't work when one changes the target.

This could be a good first step. However, that's not what I observe on the current HEAD. I only get warnings/errors from libtock-rs.

$ cargo clean
$ cargo clippy -p ctap2 --features=std
[...]
    Checking libtock v0.1.0 (/opensk/third_party/libtock-rs)
warning: unneeded `return` statement
  --> third_party/libtock-rs/src/rng.rs:44:5
   |
44 |     return true;
   |     ^^^^^^^^^^^^ help: remove `return`: `true`
   |
   = 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

[...]

And we ultimately don't want to run Clippy only in --features=std mode, but also consider no_std and the actual target. And of course also try combinations of features like with_ctap2_1.

[ia0]

Weird, I did ./reset.sh && ./setup.sh and now I have the same output as you. Here is the output I used to have:

% cargo clippy -p ctap2 --features=std
warning: Variant name ends with the enum's name
  --> src/ctap/storage.rs:81:5
   |
81 |     AttestationPrivateKey,
   |     ^^^^^^^^^^^^^^^^^^^^^
   |
   = note: `#[warn(clippy::enum_variant_names)]` on by default
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#enum_variant_names

warning: returning the result of a `let` binding from a block
   --> src/usb_ctap_hid.rs:186:5
    |
177 |     let result = recv_with_timeout_detail(buf, timeout_delay);
    |     ---------------------------------------------------------- unnecessary `let` binding
...
186 |     result
    |     ^^^^^^
    |
    = note: `#[warn(clippy::let_and_return)]` on by default
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#let_and_return
help: return the expression directly
    |
177 |     
178 | 
179 |     #[cfg(feature = "verbose")]
180 |     {
181 |         if let Some(SendOrRecvStatus::Received) = result {
182 |             writeln!(Console::new(), "Received packet = {:02x?}", buf as &[u8]).unwrap();
  ...

warning: returning the result of a `let` binding from a block
   --> src/usb_ctap_hid.rs:213:5
    |
204 |     let result = send_or_recv_with_timeout_detail(buf, timeout_delay);
    |     ------------------------------------------------------------------ unnecessary `let` binding
...
213 |     result
    |     ^^^^^^
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#let_and_return
help: return the expression directly
    |
204 |     
205 | 
206 |     #[cfg(feature = "verbose")]
207 |     {
208 |         if let Some(SendOrRecvStatus::Received) = result {
209 |             writeln!(Console::new(), "Received packet = {:02x?}", buf as &[u8]).unwrap();
  ...

warning: large size difference between variants
  --> src/ctap/command.rs:34:5
   |
34 |     AuthenticatorMakeCredential(AuthenticatorMakeCredentialParameters),
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: `#[warn(clippy::large_enum_variant)]` on by default
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#large_enum_variant
help: consider boxing the large fields to reduce the total size of the enum
   |
34 |     AuthenticatorMakeCredential(Box<AuthenticatorMakeCredentialParameters>),
   |                                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

warning: use of `unwrap_or` followed by a function call
   --> src/ctap/command.rs:154:61
    |
154 |                 let list_len = MAX_CREDENTIAL_COUNT_IN_LIST.unwrap_or(exclude_list_vec.len());
    |                                                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|| exclude_list_vec.len())`
    |
    = note: `#[warn(clippy::or_fun_call)]` on by default
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#or_fun_call

warning: use of `unwrap_or` followed by a function call
   --> src/ctap/command.rs:228:61
    |
228 |                 let list_len = MAX_CREDENTIAL_COUNT_IN_LIST.unwrap_or(allow_list_vec.len());
    |                                                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|| allow_list_vec.len())`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#or_fun_call

warning: redundant closure found
   --> src/ctap/command.rs:296:18
    |
296 |             .map(|x| CoseKey(x));
    |                  ^^^^^^^^^^^^^^ help: remove closure as shown: `CoseKey`
    |
    = note: `#[warn(clippy::redundant_closure)]` on by default
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#redundant_closure

warning: large size difference between variants
  --> src/ctap/response.rs:30:5
   |
30 |     AuthenticatorGetNextAssertion(AuthenticatorGetAssertionResponse),
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#large_enum_variant
help: consider boxing the large fields to reduce the total size of the enum
   |
30 |     AuthenticatorGetNextAssertion(Box<AuthenticatorGetAssertionResponse>),
   |                                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

warning: in an `if` condition, avoid complex blocks or closures with blocks; instead, move the block or closure higher and bind it with a `let`
   --> src/ctap/storage.rs:243:53
    |
243 |               && result.as_ref().map_or(false, |cred| {
    |  _____________________________________________________^
244 | |                 cred.cred_protect_policy
245 | |                     == Some(CredentialProtectionPolicy::UserVerificationRequired)
246 | |             })
    | |_____________^
    |
    = note: `#[warn(clippy::block_in_if_condition_stmt)]` on by default
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#block_in_if_condition_stmt

    Finished dev [unoptimized + debuginfo] target(s) in 0.06s

I can't reproduce it now (even when building and testing beforehand).