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 <[email protected]>

.gitlab/pipeline/check.yml

@@ -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

Cargo.toml

@@ -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

bridges/modules/relayers/src/benchmarking.rs

@@ -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()));

cumulus/pallets/xcmp-queue/src/benchmarking.rs

@@ -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."
 		);
 	}

cumulus/parachains/pallets/collective-content/src/benchmarking.rs

@@ -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>(

polkadot/runtime/parachains/src/paras_inherent/benchmarking.rs

@@ -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() {

polkadot/xcm/pallet-xcm-benchmarks/src/generic/benchmarking.rs

@@ -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)?;

polkadot/xcm/xcm-simulator/example/src/parachain.rs

@@ -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())
 	}
 }
 

substrate/frame/alliance/src/benchmarking.rs

@@ -183,20 +183,15 @@ mod benchmarks {
 			let voter = &members[j as usize];
 			Alliance::<T, I>::vote(
 				SystemOrigin::Signed(voter.clone()).into(),
-				last_hash.clone(),
+				last_hash,
 				index,
 				true,
 			)?;
 		}
 
 		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,23 +399,23 @@ mod benchmarks {
 			let voter = &members[j as usize];
 			Alliance::<T, I>::vote(
 				SystemOrigin::Signed(voter.clone()).into(),
-				last_hash.clone(),
+				last_hash,
 				index,
 				true,
 			)?;
 		}
 
 		Alliance::<T, I>::vote(
 			SystemOrigin::Signed(voter.clone()).into(),
-			last_hash.clone(),
+			last_hash,
 			index,
 			false,
 		)?;
 
 		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(())

substrate/frame/bounties/src/benchmarking.rs

@@ -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())