Fix clippy lints behind feature gates and add new CI step all features (#2569)

Many clippy lints usually enforced by `-Dcomplexity` and `-Dcorrectness`
are not caught by CI as they are gated by `features`, like
`runtime-benchmarks`, while the clippy CI job runs with only the default
features for all targets.

This PR also adds a CI step to run clippy with `--all-features` to
ensure the code quality is maintained behind feature gates from now on.

To improve local development, clippy lints are downgraded to warnings,
but they still will result in an error at CI due to the `-Dwarnings`
rustflag.

---------

Co-authored-by: Liam Aharon <liam.aharon@hotmail.com>
This commit is contained in:
Dónal Murray
2023-12-20 16:30:10 +00:00
committed by GitHub
parent 280aa0b573
commit d68868f64a
22 changed files with 68 additions and 91 deletions
+1
View File
@@ -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
check-try-runtime:
stage: check
+6 -5
View File
@@ -484,20 +484,21 @@ 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
+2 -2
View File
@@ -104,7 +104,7 @@ benchmarks! {
// create slash destination account
let lane = LaneId([0, 0, 0, 0]);
let slash_destination = RewardsAccountParams::new(lane, *b"test", RewardsAccountOwner::ThisChain);
T::prepare_rewards_account(slash_destination.clone(), Zero::zero());
T::prepare_rewards_account(slash_destination, Zero::zero());
}: {
crate::Pallet::<T>::slash_and_deregister(&relayer, slash_destination)
}
@@ -121,7 +121,7 @@ benchmarks! {
let account_params =
RewardsAccountParams::new(lane, *b"test", RewardsAccountOwner::ThisChain);
}: {
crate::Pallet::<T>::register_relayer_reward(account_params.clone(), &relayer, One::one());
crate::Pallet::<T>::register_relayer_reward(account_params, &relayer, One::one());
}
verify {
assert_eq!(RelayerRewards::<T>::get(relayer, &account_params), Some(One::one()));
@@ -85,7 +85,7 @@ mod benchmarks {
}
assert!(
OutboundXcmpStatus::<T>::get().iter().find(|p| p.recipient == para).is_none(),
OutboundXcmpStatus::<T>::get().iter().all(|p| p.recipient != para),
"No messages in the channel; therefore removed."
);
}
@@ -56,7 +56,7 @@ mod benchmarks {
.map_err(|_| BenchmarkError::Weightless)?;
#[extrinsic_call]
_(origin as T::RuntimeOrigin, cid.clone(), Some(expire_at.clone()));
_(origin as T::RuntimeOrigin, cid.clone(), Some(expire_at));
assert_eq!(<Announcements<T, I>>::count(), 1);
assert_last_event::<T, I>(
@@ -132,7 +132,7 @@ benchmarks! {
// Ensure that the votes are for the correct session
assert_eq!(vote.session, scenario._session);
// Ensure that there are an expected number of candidates
let header = BenchBuilder::<T>::header(scenario._block_number.clone());
let header = BenchBuilder::<T>::header(scenario._block_number);
// Traverse candidates and assert descriptors are as expected
for (para_id, backing_validators) in vote.backing_validators_per_candidate.iter().enumerate() {
let descriptor = backing_validators.0.descriptor();
@@ -189,7 +189,7 @@ benchmarks! {
// Ensure that the votes are for the correct session
assert_eq!(vote.session, scenario._session);
// Ensure that there are an expected number of candidates
let header = BenchBuilder::<T>::header(scenario._block_number.clone());
let header = BenchBuilder::<T>::header(scenario._block_number);
// Traverse candidates and assert descriptors are as expected
for (para_id, backing_validators)
in vote.backing_validators_per_candidate.iter().enumerate() {
@@ -175,7 +175,7 @@ benchmarks! {
descend_origin {
let mut executor = new_executor::<T>(Default::default());
let who = X2(OnlyChild, OnlyChild);
let instruction = Instruction::DescendOrigin(who.clone());
let instruction = Instruction::DescendOrigin(who);
let xcm = Xcm(vec![instruction]);
} : {
executor.bench_process(xcm)?;
@@ -242,7 +242,7 @@ benchmarks! {
&origin,
assets.clone().into(),
&XcmContext {
origin: Some(origin.clone()),
origin: Some(origin),
message_id: [0; 32],
topic: None,
},
@@ -279,7 +279,7 @@ benchmarks! {
let origin = T::subscribe_origin()?;
let query_id = Default::default();
let max_response_weight = Default::default();
let mut executor = new_executor::<T>(origin.clone());
let mut executor = new_executor::<T>(origin);
let instruction = Instruction::SubscribeVersion { query_id, max_response_weight };
let xcm = Xcm(vec![instruction]);
} : {
@@ -299,14 +299,14 @@ benchmarks! {
query_id,
max_response_weight,
&XcmContext {
origin: Some(origin.clone()),
origin: Some(origin),
message_id: [0; 32],
topic: None,
},
).map_err(|_| "Could not start subscription")?;
assert!(<T::XcmConfig as xcm_executor::Config>::SubscriptionService::is_subscribed(&origin));
let mut executor = new_executor::<T>(origin.clone());
let mut executor = new_executor::<T>(origin);
let instruction = Instruction::UnsubscribeVersion;
let xcm = Xcm(vec![instruction]);
} : {
@@ -538,7 +538,7 @@ benchmarks! {
let mut executor = new_executor::<T>(origin);
let instruction = Instruction::UniversalOrigin(alias.clone());
let instruction = Instruction::UniversalOrigin(alias);
let xcm = Xcm(vec![instruction]);
}: {
executor.bench_process(xcm)?;
@@ -632,13 +632,13 @@ benchmarks! {
let (unlocker, owner, asset) = T::unlockable_asset()?;
let mut executor = new_executor::<T>(unlocker.clone());
let mut executor = new_executor::<T>(unlocker);
// We first place the asset in lock first...
<T::XcmConfig as xcm_executor::Config>::AssetLocker::prepare_lock(
unlocker,
asset.clone(),
owner.clone(),
owner,
)
.map_err(|_| BenchmarkError::Skip)?
.enact()
@@ -658,13 +658,13 @@ benchmarks! {
let (unlocker, owner, asset) = T::unlockable_asset()?;
let mut executor = new_executor::<T>(unlocker.clone());
let mut executor = new_executor::<T>(unlocker);
// We first place the asset in lock first...
<T::XcmConfig as xcm_executor::Config>::AssetLocker::prepare_lock(
unlocker,
asset.clone(),
owner.clone(),
owner,
)
.map_err(|_| BenchmarkError::Skip)?
.enact()
@@ -686,9 +686,9 @@ benchmarks! {
// We first place the asset in lock first...
<T::XcmConfig as xcm_executor::Config>::AssetLocker::prepare_lock(
locker.clone(),
locker,
asset.clone(),
owner.clone(),
owner,
)
.map_err(|_| BenchmarkError::Skip)?
.enact()
@@ -739,7 +739,7 @@ benchmarks! {
let mut executor = new_executor::<T>(origin);
let instruction = Instruction::AliasOrigin(target.clone());
let instruction = Instruction::AliasOrigin(target);
let xcm = Xcm(vec![instruction]);
}: {
executor.bench_process(xcm)?;
@@ -164,7 +164,7 @@ impl EnsureOriginWithArg<RuntimeOrigin, MultiLocation> for ForeignCreators {
#[cfg(feature = "runtime-benchmarks")]
fn try_successful_origin(a: &MultiLocation) -> Result<RuntimeOrigin, ()> {
Ok(pallet_xcm::Origin::Xcm(a.clone()).into())
Ok(pallet_xcm::Origin::Xcm(*a).into())
}
}
+18 -39
View File
@@ -183,7 +183,7 @@ mod benchmarks {
let voter = &members[j as usize];
Alliance::<T, I>::vote(
SystemOrigin::Signed(voter.clone()).into(),
last_hash.clone(),
last_hash,
index,
true,
)?;
@@ -191,12 +191,7 @@ mod benchmarks {
let voter = members[m as usize - 3].clone();
// Voter votes aye without resolving the vote.
Alliance::<T, I>::vote(
SystemOrigin::Signed(voter.clone()).into(),
last_hash.clone(),
index,
true,
)?;
Alliance::<T, I>::vote(SystemOrigin::Signed(voter.clone()).into(), last_hash, index, true)?;
// Voter switches vote to nay, but does not kill the vote, just updates + inserts
let approve = false;
@@ -206,7 +201,7 @@ mod benchmarks {
frame_benchmarking::benchmarking::add_to_whitelist(voter_key.into());
#[extrinsic_call]
_(SystemOrigin::Signed(voter), last_hash.clone(), index, approve);
_(SystemOrigin::Signed(voter), last_hash, index, approve);
//nothing to verify
Ok(())
@@ -255,24 +250,19 @@ mod benchmarks {
let voter = &members[j as usize];
Alliance::<T, I>::vote(
SystemOrigin::Signed(voter.clone()).into(),
last_hash.clone(),
last_hash,
index,
true,
)?;
}
// Voter votes aye without resolving the vote.
Alliance::<T, I>::vote(
SystemOrigin::Signed(voter.clone()).into(),
last_hash.clone(),
index,
true,
)?;
Alliance::<T, I>::vote(SystemOrigin::Signed(voter.clone()).into(), last_hash, index, true)?;
// Voter switches vote to nay, which kills the vote
Alliance::<T, I>::vote(
SystemOrigin::Signed(voter.clone()).into(),
last_hash.clone(),
last_hash,
index,
false,
)?;
@@ -282,7 +272,7 @@ mod benchmarks {
frame_benchmarking::benchmarking::add_to_whitelist(voter_key.into());
#[extrinsic_call]
close(SystemOrigin::Signed(voter), last_hash.clone(), index, Weight::MAX, bytes_in_storage);
close(SystemOrigin::Signed(voter), last_hash, index, Weight::MAX, bytes_in_storage);
assert_eq!(T::ProposalProvider::proposal_of(last_hash), None);
Ok(())
@@ -330,7 +320,7 @@ mod benchmarks {
// approval vote
Alliance::<T, I>::vote(
SystemOrigin::Signed(proposer.clone()).into(),
last_hash.clone(),
last_hash,
index,
false,
)?;
@@ -340,7 +330,7 @@ mod benchmarks {
let voter = &members[j as usize];
Alliance::<T, I>::vote(
SystemOrigin::Signed(voter.clone()).into(),
last_hash.clone(),
last_hash,
index,
false,
)?;
@@ -349,22 +339,17 @@ mod benchmarks {
// Member zero is the first aye
Alliance::<T, I>::vote(
SystemOrigin::Signed(members[0].clone()).into(),
last_hash.clone(),
last_hash,
index,
true,
)?;
let voter = members[1].clone();
// Caller switches vote to aye, which passes the vote
Alliance::<T, I>::vote(
SystemOrigin::Signed(voter.clone()).into(),
last_hash.clone(),
index,
true,
)?;
Alliance::<T, I>::vote(SystemOrigin::Signed(voter.clone()).into(), last_hash, index, true)?;
#[extrinsic_call]
close(SystemOrigin::Signed(voter), last_hash.clone(), index, Weight::MAX, bytes_in_storage);
close(SystemOrigin::Signed(voter), last_hash, index, Weight::MAX, bytes_in_storage);
assert_eq!(T::ProposalProvider::proposal_of(last_hash), None);
Ok(())
@@ -414,7 +399,7 @@ mod benchmarks {
let voter = &members[j as usize];
Alliance::<T, I>::vote(
SystemOrigin::Signed(voter.clone()).into(),
last_hash.clone(),
last_hash,
index,
true,
)?;
@@ -422,7 +407,7 @@ mod benchmarks {
Alliance::<T, I>::vote(
SystemOrigin::Signed(voter.clone()).into(),
last_hash.clone(),
last_hash,
index,
false,
)?;
@@ -430,7 +415,7 @@ mod benchmarks {
System::<T>::set_block_number(BlockNumberFor::<T>::max_value());
#[extrinsic_call]
close(SystemOrigin::Signed(voter), last_hash.clone(), index, Weight::MAX, bytes_in_storage);
close(SystemOrigin::Signed(voter), last_hash, index, Weight::MAX, bytes_in_storage);
// The last proposal is removed.
assert_eq!(T::ProposalProvider::proposal_of(last_hash), None);
@@ -477,7 +462,7 @@ mod benchmarks {
// The prime member votes aye, so abstentions default to aye.
Alliance::<T, I>::vote(
SystemOrigin::Signed(proposer.clone()).into(),
last_hash.clone(),
last_hash,
p - 1,
true, // Vote aye.
)?;
@@ -489,7 +474,7 @@ mod benchmarks {
let voter = &members[j as usize];
Alliance::<T, I>::vote(
SystemOrigin::Signed(voter.clone()).into(),
last_hash.clone(),
last_hash,
index,
false,
)?;
@@ -499,13 +484,7 @@ mod benchmarks {
System::<T>::set_block_number(BlockNumberFor::<T>::max_value());
#[extrinsic_call]
close(
SystemOrigin::Signed(proposer),
last_hash.clone(),
index,
Weight::MAX,
bytes_in_storage,
);
close(SystemOrigin::Signed(proposer), last_hash, index, Weight::MAX, bytes_in_storage);
assert_eq!(T::ProposalProvider::proposal_of(last_hash), None);
Ok(())
+1 -1
View File
@@ -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())
@@ -1749,7 +1749,7 @@ benchmarks! {
.collect::<Vec<BalanceOf<T>>>();
let deposits_bytes: Vec<u8> = deposits.iter().flat_map(|i| i.encode()).collect();
let deposits_len = deposits_bytes.len() as u32;
let deposit_len = value_len.clone();
let deposit_len = value_len;
let callee_offset = value_len + deposits_len;
let code = WasmModule::<T>::from(ModuleDefinition {
memory: Some(ImportedMemory::max::<T>()),
@@ -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;
@@ -317,7 +317,7 @@ where
let (_, old_deposit, storage_module) = state;
// CodeInfoOf::max_encoded_len == OwnerInfoOf::max_encoded_len + 1
// I.e. code info adds up 1 byte per record.
let info_bytes_added = items.clone();
let info_bytes_added = items;
// We removed 1 PrefabWasmModule, and added 1 byte of determinism flag, per contract code.
let storage_removed = storage_module.saturating_sub(info_bytes_added);
// module+code+info - bytes
@@ -65,7 +65,7 @@ fn add_referendum<T: Config>(n: u32) -> (ReferendumIndex, T::Hash, T::Hash) {
0u32.into(),
);
let preimage_hash = note_preimage::<T>();
MetadataOf::<T>::insert(crate::MetadataOwner::Referendum(index), preimage_hash.clone());
MetadataOf::<T>::insert(crate::MetadataOwner::Referendum(index), preimage_hash);
(index, hash, preimage_hash)
}
@@ -172,7 +172,7 @@ mod test {
let hash = H256::repeat_byte(1);
let status = ReferendumStatus {
end: 1u32.into(),
proposal: hash.clone(),
proposal: hash,
threshold: VoteThreshold::SuperMajorityApprove,
delay: 1u32.into(),
tally: Tally { ayes: 1u32.into(), nays: 1u32.into(), turnout: 1u32.into() },
@@ -187,13 +187,10 @@ mod test {
// Case 3: Public proposals
let hash2 = H256::repeat_byte(2);
v0::PublicProps::<T>::put(vec![
(3u32, hash.clone(), 123u64),
(4u32, hash2.clone(), 123u64),
]);
v0::PublicProps::<T>::put(vec![(3u32, hash, 123u64), (4u32, hash2, 123u64)]);
// Case 4: Next external
v0::NextExternal::<T>::put((hash.clone(), VoteThreshold::SuperMajorityApprove));
v0::NextExternal::<T>::put((hash, VoteThreshold::SuperMajorityApprove));
// Migrate.
let state = v1::Migration::<T>::pre_upgrade().unwrap();
@@ -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 {
+4 -4
View File
@@ -190,7 +190,7 @@ benchmarks! {
let recovery_config = RecoveryConfig {
delay_period: DEFAULT_DELAY.into(),
deposit: total_deposit.clone(),
deposit: total_deposit,
friends: bounded_friends.clone(),
threshold: n as u16,
};
@@ -243,7 +243,7 @@ benchmarks! {
let recovery_config = RecoveryConfig {
delay_period: 0u32.into(),
deposit: total_deposit.clone(),
deposit: total_deposit,
friends: bounded_friends.clone(),
threshold: n as u16,
};
@@ -294,7 +294,7 @@ benchmarks! {
let recovery_config = RecoveryConfig {
delay_period: DEFAULT_DELAY.into(),
deposit: total_deposit.clone(),
deposit: total_deposit,
friends: bounded_friends.clone(),
threshold: n as u16,
};
@@ -342,7 +342,7 @@ benchmarks! {
let recovery_config = RecoveryConfig {
delay_period: DEFAULT_DELAY.into(),
deposit: total_deposit.clone(),
deposit: total_deposit,
friends: bounded_friends.clone(),
threshold: n as u16,
};
@@ -260,7 +260,7 @@ mod benchmarks {
// Update metadata
let mut meta = TicketsMeta::<T>::get();
meta.unsorted_tickets_count = tickets_count;
TicketsMeta::<T>::set(meta.clone());
TicketsMeta::<T>::set(meta);
log::debug!(target: LOG_TARGET, "Before sort: {:?}", meta);
#[block]
+3 -3
View File
@@ -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.",
@@ -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());
@@ -362,7 +362,7 @@ mod test {
Some(ScheduledV3Of::<Test> {
maybe_id: Some(vec![i as u8; 320]),
priority: 123,
call: MaybeHashed::Hash(undecodable_hash.clone()),
call: MaybeHashed::Hash(undecodable_hash),
maybe_periodic: Some((4u64, 20)),
origin: root(),
_phantom: PhantomData::<u64>::default(),
+1 -1
View File
@@ -1801,7 +1801,7 @@ impl<T: Config> StakingInterface for Pallet<T> {
) {
let others = exposures
.iter()
.map(|(who, value)| IndividualExposure { who: who.clone(), value: value.clone() })
.map(|(who, value)| IndividualExposure { who: who.clone(), value: *value })
.collect::<Vec<_>>();
let exposure = Exposure { total: Default::default(), own: Default::default(), others };
EraInfo::<T>::set_exposure(*current_era, stash, exposure);
@@ -1637,7 +1637,7 @@ pub(crate) mod remote_tests {
weight_sum +=
StateTrieMigration::<Runtime>::on_initialize(System::<Runtime>::block_number());
root = System::<Runtime>::finalize().state_root().clone();
root = *System::<Runtime>::finalize().state_root();
System::<Runtime>::on_finalize(System::<Runtime>::block_number());
}
(root, weight_sum)
@@ -1687,7 +1687,7 @@ pub(crate) mod remote_tests {
);
loop {
let last_state_root = ext.backend.root().clone();
let last_state_root = *ext.backend.root();
let ((finished, weight), proof) = ext.execute_and_prove(|| {
let weight = run_to_block::<Runtime>(now + One::one()).1;
if StateTrieMigration::<Runtime>::migration_process().finished() {
+2 -2
View File
@@ -431,9 +431,9 @@ benchmarks_instance_pallet! {
let buyer_lookup = T::Lookup::unlookup(buyer.clone());
let price = ItemPrice::<T, I>::from(0u32);
let origin = SystemOrigin::Signed(seller.clone()).into();
Uniques::<T, I>::set_price(origin, collection.clone(), item, Some(price.clone()), Some(buyer_lookup))?;
Uniques::<T, I>::set_price(origin, collection.clone(), item, Some(price), Some(buyer_lookup))?;
T::Currency::make_free_balance_be(&buyer, DepositBalanceOf::<T, I>::max_value());
}: _(SystemOrigin::Signed(buyer.clone()), collection.clone(), item, price.clone())
}: _(SystemOrigin::Signed(buyer.clone()), collection.clone(), item, price)
verify {
assert_last_event::<T, I>(Event::ItemBought {
collection: collection.clone(),
@@ -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)
}
}
}