Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix clippy lints behind feature gates and introduce new CI step targeting all features #2569

Merged
merged 12 commits into from
Dec 20, 2023
1 change: 1 addition & 0 deletions .gitlab/pipeline/check.yml
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ cargo-clippy:
RUSTFLAGS: "-D warnings"
script:
- SKIP_WASM_BUILD=1 cargo clippy --all-targets --locked --workspace
- SKIP_WASM_BUILD=1 cargo clippy --all-targets --all-features --locked --workspace
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we have a different step for this? Or modify the one we already have?
Or does clippy do some magic and not re-do the work after the first command?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought I'd add it here to hijack the previous job since it has already downloaded the packages and only has to recompile workspace packages which have other features (no other deps). You can actually see it in the clippy job that has run on this PR. Happy to make it a separate step if the CI folks are happy with the extra cost (if there is any - I don't know the ins and outs of forklift).
The features are not purely additive, and can actually remove code from the main build so I don't think we should replace the previous step.


check-try-runtime:
stage: check
Expand Down
12 changes: 7 additions & 5 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -481,25 +481,27 @@ suspicious_double_ref_op = { level = "allow", priority = 2 }

[workspace.lints.clippy]
all = { level = "allow", priority = 0 }
correctness = { level = "deny", priority = 1 }
correctness = { level = "warn", priority = 1 }
complexity = { level = "warn", priority = 1 }
if-same-then-else = { level = "allow", priority = 2 }
complexity = { level = "deny", priority = 1 }
zero-prefixed-literal = { level = "allow", priority = 2 } # 00_1000_000
type_complexity = { level = "allow", priority = 2 } # raison d'etre
nonminimal-bool = { level = "allow", priority = 2 } # maybe
borrowed-box = { level = "allow", priority = 2 } # Reasonable to fix this one
too-many-arguments = { level = "allow", priority = 2 } # (Turning this on would lead to)
needless-lifetimes = { level = "allow", priority = 2 } # generated code
unnecessary_cast = { level = "allow", priority = 2 } # Types may change
identity-op = { level = "allow", priority = 2 } # One case where we do 0 +
useless_conversion = { level = "allow", priority = 2 } # Types may change
unit_arg = { level = "allow", priority = 2 } # styalistic.
option-map-unit-fn = { level = "allow", priority = 2 } # styalistic
bind_instead_of_map = { level = "allow", priority = 2 } # styalistic
unit_arg = { level = "allow", priority = 2 } # stylistic
option-map-unit-fn = { level = "allow", priority = 2 } # stylistic
bind_instead_of_map = { level = "allow", priority = 2 } # stylistic
erasing_op = { level = "allow", priority = 2 } # E.g. 0 * DOLLARS
eq_op = { level = "allow", priority = 2 } # In tests we test equality.
while_immutable_condition = { level = "allow", priority = 2 } # false positives
needless_option_as_deref = { level = "allow", priority = 2 } # false positives
derivable_impls = { level = "allow", priority = 2 } # false positives
clone_on_copy = { level = "allow", priority = 2 } # false positives
seadanda marked this conversation as resolved.
Show resolved Hide resolved
stable_sort_primitive = { level = "allow", priority = 2 } # prefer stable sort
extra-unused-type-parameters = { level = "allow", priority = 2 } # stylistic
default_constructed_unit_structs = { level = "allow", priority = 2 } # stylistic
Expand Down
2 changes: 1 addition & 1 deletion cumulus/pallets/xcmp-queue/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ mod benchmarks {
}

assert!(
OutboundXcmpStatus::<T>::get().iter().find(|p| p.recipient == para).is_none(),
!OutboundXcmpStatus::<T>::get().iter().any(|p| p.recipient == para),
seadanda marked this conversation as resolved.
Show resolved Hide resolved
"No messages in the channel; therefore removed."
);
}
Expand Down
2 changes: 1 addition & 1 deletion substrate/frame/bounties/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,7 @@ benchmarks_instance_pallet! {
);
}
verify {
ensure!(missed_any == false, "Missed some");
ensure!(!missed_any, "Missed some");
if b > 0 {
ensure!(budget_remaining < BalanceOf::<T, I>::max_value(), "Budget not used");
assert_last_event::<T, I>(Event::BountyBecameActive { index: b - 1 }.into())
Expand Down
3 changes: 1 addition & 2 deletions substrate/frame/contracts/src/benchmarking/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2246,13 +2246,12 @@ benchmarks! {
let message_len = message.len() as i32;
let key_type = sp_core::crypto::KeyTypeId(*b"code");
let sig_params = (0..r)
.map(|i| {
.flat_map(|i| {
let pub_key = sp_io::crypto::sr25519_generate(key_type, None);
let sig = sp_io::crypto::sr25519_sign(key_type, &pub_key, &message).expect("Generates signature");
let data: [u8; 96] = [AsRef::<[u8]>::as_ref(&sig), AsRef::<[u8]>::as_ref(&pub_key)].concat().try_into().unwrap();
data
})
.flatten()
.collect::<Vec<_>>();
let sig_params_len = sig_params.len() as i32;

Expand Down
2 changes: 1 addition & 1 deletion substrate/frame/fast-unstake/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ benchmarks! {
fast_unstake_events::<T>().last(),
Some(Event::BatchChecked { .. })
));
assert!(stashes.iter().all(|(s, _)| request.stashes.iter().find(|(ss, _)| ss == s).is_some()));
assert!(stashes.iter().all(|(s, _)| request.stashes.iter().any(|(ss, _)| ss == s)));
}

register_fast_unstake {
Expand Down
4 changes: 2 additions & 2 deletions substrate/frame/scheduler/src/migration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ pub mod v3 {
// Check that no agenda overflows `MaxScheduledPerBlock`.
let max_scheduled_per_block = T::MaxScheduledPerBlock::get() as usize;
for (block_number, agenda) in Agenda::<T>::iter() {
if agenda.iter().cloned().filter_map(|s| s).count() > max_scheduled_per_block {
if agenda.iter().cloned().flatten().count() > max_scheduled_per_block {
log::error!(
target: TARGET,
"Would truncate agenda of block {:?} from {} items to {} items.",
Expand All @@ -119,7 +119,7 @@ pub mod v3 {
// Check that bounding the calls will not overflow `MAX_LENGTH`.
let max_length = T::Preimages::MAX_LENGTH as usize;
for (block_number, agenda) in Agenda::<T>::iter() {
for schedule in agenda.iter().cloned().filter_map(|s| s) {
for schedule in agenda.iter().cloned().flatten() {
match schedule.call {
frame_support::traits::schedule::MaybeHashed::Value(call) => {
let l = call.using_encoded(|c| c.len());
Expand Down
2 changes: 1 addition & 1 deletion substrate/primitives/core/src/paired_crypto.rs
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ pub mod ecdsa_bls377 {
let Ok(right_sig) = sig.0[ecdsa::SIGNATURE_SERIALIZED_SIZE..].try_into() else {
return false
};
bls377::Pair::verify(&right_sig, message.as_ref(), &right_pub)
bls377::Pair::verify(&right_sig, message, &right_pub)
}
}
}
Expand Down