chore: clippy lints #737
chore: clippy lints #737
Conversation
ntn-x2
force-pushed
the
aa/clippy-lints-config
branch
from
1fc7c15
to
db84e55
Compare
ntn-x2
force-pushed
the
aa/clippy-lints-config
branch
from
52cf899
to
be82c3f
Compare
| - --all-features | ||
| cargo-flags: | ||
| # Generic clippy checks for all features | ||
| - --all-targets --all-features |
There was a problem hiding this comment.
This line and the one below maintain the same behaviour as before.
| # Generic clippy checks for no features (catches some missing `no_std`-only lints) | ||
| - --all-targets | ||
| # Clippy lints specifically for all runtime code, excluding all test and binary crates | ||
| - --target wasm32-unknown-unknown --no-default-features --workspace --exclude kilt-parachain --exclude standalone-node --exclude xcm-integration-tests --exclude 'dip-provider*' --exclude 'dip-consumer*' |
There was a problem hiding this comment.
This line is new and it tests all runtime-compatible code.
| @@ -0,0 +1,26 @@ | |||
| # Deployment runtime lints | |||
| [target.'cfg(all(target_arch = "wasm32", not(test)))'] | |||
There was a problem hiding this comment.
These lints apply specifically to runtime code that is not used for tests. If we want to expand the set of lints, some of which might apply to the whole codebase, we can look at https://doc.rust-lang.org/cargo/reference/config.html#build to add more lints.
There was a problem hiding this comment.
Do you think it might be worth moving the config file into a separate crate? Each crate might have different requirements, and specific Clippy warnings may need to be enabled or disabled accordingly. I see, that we maybe introduce a lot of boilerplate code into the repo, but I think it might be worth it to have the flexibility.
ntn-x2
force-pushed
the
aa/clippy-lints-config
branch
from
55d6476
to
86e11f8
Compare
| @@ -50,7 +51,7 @@ use sp_std::marker::PhantomData; | |||
| pub struct WeightInfo<T>(PhantomData<T>); | |||
| impl<T: frame_system::Config> pallet_sudo::WeightInfo for WeightInfo<T> { | |||
|
|
|||
| fn remove_key() -> cumulus_primitives_core::Weight { todo!() } | |||
| fn remove_key() -> cumulus_primitives_core::Weight { Weight::MAX } | |||
There was a problem hiding this comment.
Upss... Could we maybe benchmark the extrinsic?
| impl<T: frame_system::Config> {{pallet}}::WeightInfo for WeightInfo<T> { | ||
| {{#each benchmarks as |benchmark|}} | ||
| {{#each benchmark.comments as |comment|}} | ||
| /// {{comment}} | ||
| {{/each}} | ||
| {{#each benchmark.component_ranges as |range|}} | ||
| /// The range of component `{{range.name}}` is `[{{range.min}}, {{range.max}}]`. | ||
| {{/each}} | ||
| fn {{benchmark.name~}} | ||
| ( | ||
| {{~#each benchmark.components as |c| ~}} | ||
| {{~#if (not c.is_used)}}_{{/if}}{{c.name}}: u32, {{/each~}} |
There was a problem hiding this comment.
Why those fmt changes? I found it previously clearer
| @@ -0,0 +1,26 @@ | |||
| # Deployment runtime lints | |||
| [target.'cfg(all(target_arch = "wasm32", not(test)))'] | |||
There was a problem hiding this comment.
Do you think it might be worth moving the config file into a separate crate? Each crate might have different requirements, and specific Clippy warnings may need to be enabled or disabled accordingly. I see, that we maybe introduce a lot of boilerplate code into the repo, but I think it might be worth it to have the flexibility.
| "-Funsafe_code", | ||
| "-Wclippy::integer_division", | ||
| "-Wclippy::modulo_arithmetic", | ||
| "-Wclippy::print_stderr", |
There was a problem hiding this comment.
We should also maybe add:
https://rust-lang.github.io/rust-clippy/rust-1.74.0/index.html#/branches_sharing_code
https://rust-lang.github.io/rust-clippy/rust-1.74.0/index.html#/todo 😄
https://rust-lang.github.io/rust-clippy/rust-1.74.0/index.html#/unimplemented
https://rust-lang.github.io/rust-clippy/rust-1.74.0/index.html#/manual_let_else
| @@ -484,6 +488,7 @@ pub mod pallet { | |||
| /// `check_releasing_preconditions` as it does not verify all the | |||
| /// preconditions again. | |||
| fn unregister_name(name: &Web3NameOf<T>) -> Result<Web3OwnershipOf<T>, DispatchError> { | |||
| #[allow(clippy::unwrap_used)] | |||
There was a problem hiding this comment.
I see that the checks are already made and the unwrap is safe, but should we maybe not still try to solve that without unwrap?
Fixes https://github.com/KILTprotocol/ticket/issues/3325.
Add some basic lints (can always be expanded for either runtime-specific code or all code via the configurable rustc flags in the new config file). I updated the code to address all places in which the new lints generate an error, including all weight files and weight templates. I also updated the CI check step to check for the specific WASM-targeted, no-std code that will be part of our runtime.
How to test
To check runtime code against the new lint, just run the same command as in the check workflow file:
cargo clippy --locked --target wasm32-unknown-unknown --no-default-features --workspace --exclude kilt-parachain --exclude standalone-node --exclude xcm-integration-tests --exclude 'dip-provider*' --exclude 'dip-consumer*'