Make clippy _a little_ more annoying (#10570)

* Clippy: +complexity

* Update client/cli/src/arg_enums.rs

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

* Update bin/node/inspect/src/lib.rs

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

* Update primitives/keystore/src/testing.rs

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

* Update frame/elections/src/lib.rs

Co-authored-by: Keith Yeung <kungfukeith11@gmail.com>

* Update primitives/npos-elections/fuzzer/src/reduce.rs

Co-authored-by: Keith Yeung <kungfukeith11@gmail.com>

* Incorporating feedback

* No need for Ok

* Additional

* Needed slice

* Wigy's suggestions on less derefs

* fix count

* reverting changes brought in by option_map_unit_fn

* add --all-targets

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
Co-authored-by: Keith Yeung <kungfukeith11@gmail.com>

.cargo/config.toml

@@ -10,5 +10,24 @@ rustflags = [
   "-Aclippy::all",
   "-Dclippy::correctness",
   "-Aclippy::if-same-then-else",
-  "-Aclippy::clone-double-ref"
+  "-Aclippy::clone-double-ref",
+  "-Dclippy::complexity",
+  "-Aclippy::clone_on_copy",             # Too common
+  "-Aclippy::needless_lifetimes",        # Backward compat?
+  "-Aclippy::zero-prefixed-literal",     # 00_1000_000
+  "-Aclippy::type_complexity",           # raison d'etre
+  "-Aclippy::nonminimal-bool",           # maybe
+  "-Aclippy::borrowed-box",              # Reasonable to fix this one
+  "-Aclippy::too-many-arguments",        # (Turning this on would lead to)
+  "-Aclippy::unnecessary_cast",          # Types may change
+  "-Aclippy::identity-op",               # One case where we do 0 + 
+  "-Aclippy::useless_conversion",        # Types may change
+  "-Aclippy::unit_arg",                  # styalistic.
+  "-Aclippy::option-map-unit-fn",        # styalistic
+  "-Aclippy::bind_instead_of_map",       # styalistic 
+  "-Aclippy::erasing_op",                # E.g. 0 * DOLLARS
+  "-Aclippy::eq_op",                     # In tests we test equality.
+  "-Aclippy::while_immutable_condition", # false positives
+  "-Aclippy::needless_option_as_deref",  # false positives
+  "-Aclippy::derivable_impls",           # false positives
 ]

.gitlab-ci.yml

@@ -350,7 +350,7 @@ cargo-clippy:
   <<:                              *docker-env
   <<:                              *test-refs
   script:
-    - SKIP_WASM_BUILD=1 env -u RUSTFLAGS cargo +nightly clippy
+    - SKIP_WASM_BUILD=1 env -u RUSTFLAGS cargo +nightly clippy --all-targets
 
 cargo-check-benches:
   stage:                           test

bin/node-template/node/src/service.rs

@@ -60,7 +60,7 @@ pub fn new_partial(
 	ServiceError,
 > {
 	if config.keystore_remote.is_some() {
-		return Err(ServiceError::Other(format!("Remote Keystores are not supported.")))
+		return Err(ServiceError::Other("Remote Keystores are not supported.".into()))
 	}
 
 	let telemetry = config

bin/node/inspect/src/lib.rs

@@ -261,7 +261,7 @@ impl<Hash: FromStr + Debug, Number: FromStr + Debug> FromStr for ExtrinsicAddres
 
 		let index = it
 			.next()
-			.ok_or_else(|| format!("Extrinsic index missing: example \"5:0\""))?
+			.ok_or("Extrinsic index missing: example \"5:0\"")?
 			.parse()
 			.map_err(|e| format!("Invalid index format: {}", e))?;
 

client/chain-spec/src/chain_spec.rs

@@ -223,7 +223,7 @@ impl<G, E> ChainSpec<G, E> {
 
 	/// Network protocol id.
 	pub fn protocol_id(&self) -> Option<&str> {
-		self.client_spec.protocol_id.as_ref().map(String::as_str)
+		self.client_spec.protocol_id.as_deref()
 	}
 
 	/// Additional loosly-typed properties of the chain.

client/cli/src/arg_enums.rs

@@ -51,7 +51,7 @@ impl std::str::FromStr for WasmExecutionMethod {
 			}
 			#[cfg(not(feature = "wasmtime"))]
 			{
-				Err(format!("`Compiled` variant requires the `wasmtime` feature to be enabled"))
+				Err("`Compiled` variant requires the `wasmtime` feature to be enabled".into())
 			}
 		} else {
 			Err(format!("Unknown variant `{}`, known variants: {:?}", s, Self::variants()))

client/cli/src/commands/sign.rs

@@ -70,7 +70,7 @@ fn sign<P: sp_core::Pair>(
 	message: Vec<u8>,
 ) -> error::Result<String> {
 	let pair = utils::pair_from_suri::<P>(suri, password)?;
-	Ok(format!("{}", hex::encode(pair.sign(&message))))
+	Ok(hex::encode(pair.sign(&message)))
 }
 
 #[cfg(test)]

client/cli/src/commands/verify.rs

@@ -57,7 +57,7 @@ impl VerifyCmd {
 		let message = utils::read_message(self.message.as_ref(), self.hex)?;
 		let sig_data = utils::decode_hex(&self.sig)?;
 		let uri = utils::read_uri(self.uri.as_ref())?;
-		let uri = if uri.starts_with("0x") { &uri[2..] } else { &uri };
+		let uri = if let Some(uri) = uri.strip_prefix("0x") { uri } else { &uri };
 
 		with_crypto_scheme!(self.crypto_scheme.scheme, verify(sig_data, message, uri))
 	}

client/cli/src/params/mod.rs

@@ -79,8 +79,8 @@ impl FromStr for BlockNumberOrHash {
 	type Err = String;
 
 	fn from_str(block_number: &str) -> Result<Self, Self::Err> {
-		if block_number.starts_with("0x") {
-			if let Some(pos) = &block_number[2..].chars().position(|c| !c.is_ascii_hexdigit()) {
+		if let Some(rest) = block_number.strip_prefix("0x") {
+			if let Some(pos) = rest.chars().position(|c| !c.is_ascii_hexdigit()) {
 				Err(format!(
 					"Expected block hash, found illegal hex character at position: {}",
 					2 + pos,

client/consensus/common/src/block_import.rs

@@ -287,9 +287,9 @@ impl<Block: BlockT, Transaction> BlockImportParams<Block, Transaction> {
 	pub fn take_intermediate<T: 'static>(&mut self, key: &[u8]) -> Result<Box<T>, Error> {
 		let (k, v) = self.intermediates.remove_entry(key).ok_or(Error::NoIntermediate)?;
 
-		v.downcast::<T>().or_else(|v| {
+		v.downcast::<T>().map_err(|v| {
 			self.intermediates.insert(k, v);
-			Err(Error::InvalidIntermediate)
+			Error::InvalidIntermediate
 		})
 	}
 

client/consensus/manual-seal/src/consensus/babe.rs

@@ -279,7 +279,7 @@ where
 		// a quick check to see if we're in the authorities
 		let epoch = self.epoch(parent, slot)?;
 		let (authority, _) = self.authorities.first().expect("authorities is non-emptyp; qed");
-		let has_authority = epoch.authorities.iter().find(|(id, _)| *id == *authority).is_some();
+		let has_authority = epoch.authorities.iter().any(|(id, _)| *id == *authority);
 
 		if !has_authority {
 			log::info!(target: "manual-seal", "authority not found");

client/consensus/manual-seal/src/lib.rs

@@ -173,7 +173,7 @@ pub async fn run_manual_seal<B, BI, CB, E, C, TP, SC, CS, CIDP>(
 					env: &mut env,
 					select_chain: &select_chain,
 					block_import: &mut block_import,
-					consensus_data_provider: consensus_data_provider.as_ref().map(|p| &**p),
+					consensus_data_provider: consensus_data_provider.as_deref(),
 					pool: pool.clone(),
 					client: client.clone(),
 					create_inherent_data_providers: &create_inherent_data_providers,
@@ -408,8 +408,8 @@ mod tests {
 		})
 		.await
 		.unwrap();
-		// assert that the background task returns ok
-		assert_eq!(rx.await.unwrap().unwrap(), ());
+		// check that the background task returns ok:
+		rx.await.unwrap().unwrap();
 	}
 
 	#[tokio::test]

client/db/src/bench.rs

@@ -672,7 +672,6 @@ mod test {
 			assert_eq!(rw_tracker.1, 0);
 			assert_eq!(rw_tracker.2, 2);
 			assert_eq!(rw_tracker.3, 0);
-			drop(rw_tracker);
 			bench_state.wipe().unwrap();
 		}
 	}

client/db/src/lib.rs

@@ -1993,7 +1993,7 @@ impl<Block: BlockT> sc_client_api::backend::Backend<Block> for Backend<Block> {
 		});
 		let database_cache = MemorySize::from_bytes(0);
 		let state_cache =
-			MemorySize::from_bytes((*&self.shared_cache).read().used_storage_cache_size());
+			MemorySize::from_bytes(self.shared_cache.read().used_storage_cache_size());
 		let state_db = self.storage.state_db.memory_info();
 
 		Some(UsageInfo {
@@ -2452,7 +2452,7 @@ pub(crate) mod tests {
 			let storage = vec![(vec![1, 3, 5], None), (vec![5, 5, 5], Some(vec![4, 5, 6]))];
 
 			let (root, overlay) = op.old_state.storage_root(
-				storage.iter().map(|(k, v)| (&k[..], v.as_ref().map(|v| &v[..]))),
+				storage.iter().map(|(k, v)| (k.as_slice(), v.as_ref().map(|v| &v[..]))),
 				state_version,
 			);
 			op.update_db_storage(overlay).unwrap();
@@ -3000,7 +3000,7 @@ pub(crate) mod tests {
 			let storage = vec![(b"test".to_vec(), Some(b"test2".to_vec()))];
 
 			let (root, overlay) = op.old_state.storage_root(
-				storage.iter().map(|(k, v)| (&k[..], v.as_ref().map(|v| &v[..]))),
+				storage.iter().map(|(k, v)| (k.as_slice(), v.as_ref().map(|v| &v[..]))),
 				state_version,
 			);
 			op.update_db_storage(overlay).unwrap();

client/db/src/offchain.rs

@@ -92,7 +92,7 @@ impl sp_core::offchain::OffchainStorage for LocalStorage {
 		{
 			let _key_guard = key_lock.lock();
 			let val = self.db.get(columns::OFFCHAIN, &key);
-			is_set = val.as_ref().map(|x| &**x) == old_value;
+			is_set = val.as_deref() == old_value;
 
 			if is_set {
 				self.set(prefix, item_key, new_value)

client/executor/src/integration_tests/mod.rs

@@ -126,7 +126,7 @@ fn call_in_wasm<E: Externalities>(
 	let executor =
 		crate::WasmExecutor::<HostFunctions>::new(execution_method, Some(1024), 8, None, 2);
 	executor.uncached_call(
-		RuntimeBlob::uncompress_if_needed(&wasm_binary_unwrap()[..]).unwrap(),
+		RuntimeBlob::uncompress_if_needed(wasm_binary_unwrap()).unwrap(),
 		ext,
 		true,
 		function,
@@ -479,7 +479,7 @@ fn should_trap_when_heap_exhausted(wasm_method: WasmExecutionMethod) {
 
 	let err = executor
 		.uncached_call(
-			RuntimeBlob::uncompress_if_needed(&wasm_binary_unwrap()[..]).unwrap(),
+			RuntimeBlob::uncompress_if_needed(wasm_binary_unwrap()).unwrap(),
 			&mut ext.ext(),
 			true,
 			"test_exhaust_heap",
@@ -491,7 +491,7 @@ fn should_trap_when_heap_exhausted(wasm_method: WasmExecutionMethod) {
 }
 
 fn mk_test_runtime(wasm_method: WasmExecutionMethod, pages: u64) -> Arc<dyn WasmModule> {
-	let blob = RuntimeBlob::uncompress_if_needed(&wasm_binary_unwrap()[..])
+	let blob = RuntimeBlob::uncompress_if_needed(wasm_binary_unwrap())
 		.expect("failed to create a runtime blob out of test runtime");
 
 	crate::wasm_runtime::create_wasm_runtime_with_code::<HostFunctions>(
@@ -597,7 +597,7 @@ fn parallel_execution(wasm_method: WasmExecutionMethod) {
 				assert_eq!(
 					executor
 						.uncached_call(
-							RuntimeBlob::uncompress_if_needed(&wasm_binary_unwrap()[..]).unwrap(),
+							RuntimeBlob::uncompress_if_needed(wasm_binary_unwrap()).unwrap(),
 							&mut ext,
 							true,
 							"test_twox_128",
@@ -691,7 +691,7 @@ fn panic_in_spawned_instance_panics_on_joining_its_result(wasm_method: WasmExecu
 	let error_result =
 		call_in_wasm("test_panic_in_spawned", &[], wasm_method, &mut ext).unwrap_err();
 
-	assert!(format!("{}", error_result).contains("Spawned task"));
+	assert!(error_result.contains("Spawned task"));
 }
 
 test_wasm_execution!(memory_is_cleared_between_invocations);

client/executor/src/lib.rs

@@ -82,7 +82,7 @@ mod tests {
 		);
 		let res = executor
 			.uncached_call(
-				RuntimeBlob::uncompress_if_needed(&wasm_binary_unwrap()[..]).unwrap(),
+				RuntimeBlob::uncompress_if_needed(wasm_binary_unwrap()).unwrap(),
 				&mut ext,
 				true,
 				"test_empty_return",

client/executor/src/wasm_runtime.rs

@@ -77,7 +77,7 @@ struct VersionedRuntime {
 
 impl VersionedRuntime {
 	/// Run the given closure `f` with an instance of this runtime.
-	fn with_instance<'c, R, F>(&self, ext: &mut dyn Externalities, f: F) -> Result<R, Error>
+	fn with_instance<R, F>(&self, ext: &mut dyn Externalities, f: F) -> Result<R, Error>
 	where
 		F: FnOnce(
 			&Arc<dyn WasmModule>,

client/executor/wasmtime/src/host.rs

@@ -195,15 +195,15 @@ impl<'a> Sandbox for HostContext<'a> {
 		&mut self,
 		instance_id: u32,
 		export_name: &str,
-		args: &[u8],
+		mut args: &[u8],
 		return_val: Pointer<u8>,
 		return_val_len: u32,
 		state: u32,
 	) -> sp_wasm_interface::Result<u32> {
 		trace!(target: "sp-sandbox", "invoke, instance_idx={}", instance_id);
 
 		// Deserialize arguments and convert them into wasmi types.
-		let args = Vec::<sp_wasm_interface::Value>::decode(&mut &args[..])
+		let args = Vec::<sp_wasm_interface::Value>::decode(&mut args)
 			.map_err(|_| "Can't decode serialized arguments for the invocation")?
 			.into_iter()
 			.map(Into::into)

client/executor/wasmtime/src/tests.rs

@@ -313,7 +313,7 @@ fn test_max_memory_pages() {
 #[test]
 fn test_instances_without_reuse_are_not_leaked() {
 	let runtime = crate::create_runtime::<HostFunctions>(
-		RuntimeBlob::uncompress_if_needed(&wasm_binary_unwrap()[..]).unwrap(),
+		RuntimeBlob::uncompress_if_needed(wasm_binary_unwrap()).unwrap(),
 		crate::Config {
 			heap_pages: 2048,
 			max_memory_size: None,

client/finality-grandpa/src/communication/gossip.rs

@@ -615,17 +615,14 @@ impl<N: Ord> Peers<N> {
 		// `LUCKY_PEERS / 2` is filled we start allocating to the second stage set.
 		let half_lucky = LUCKY_PEERS / 2;
 		let one_and_a_half_lucky = LUCKY_PEERS + half_lucky;
-		let mut n_authorities_added = 0;
-		for peer_id in shuffled_authorities {
+		for (n_authorities_added, peer_id) in shuffled_authorities.enumerate() {
 			if n_authorities_added < half_lucky {
 				first_stage_peers.insert(*peer_id);
 			} else if n_authorities_added < one_and_a_half_lucky {
 				second_stage_peers.insert(*peer_id);
 			} else {
 				break
 			}
-
-			n_authorities_added += 1;
 		}
 
 		// fill up first and second sets with remaining peers (either full or authorities)

client/finality-grandpa/src/lib.rs

@@ -267,7 +267,7 @@ pub struct Config {
 
 impl Config {
 	fn name(&self) -> &str {
-		self.name.as_ref().map(|s| s.as_str()).unwrap_or("<unknown>")
+		self.name.as_deref().unwrap_or("<unknown>")
 	}
 }
 

client/rpc/src/state/state_full.rs

@@ -441,10 +441,7 @@ where
 		keys: Option<Vec<StorageKey>>,
 	) {
 		let keys = Into::<Option<Vec<_>>>::into(keys);
-		let stream = match self
-			.client
-			.storage_changes_notification_stream(keys.as_ref().map(|x| &**x), None)
-		{
+		let stream = match self.client.storage_changes_notification_stream(keys.as_deref(), None) {
 			Ok(stream) => stream,
 			Err(err) => {
 				let _ = subscriber.reject(client_err(err).into());

client/rpc/src/state/tests.rs

@@ -32,7 +32,7 @@ use substrate_test_runtime_client::{prelude::*, runtime};
 const STORAGE_KEY: &[u8] = b"child";
 
 fn prefixed_storage_key() -> PrefixedStorageKey {
-	let child_info = ChildInfo::new_default(&STORAGE_KEY[..]);
+	let child_info = ChildInfo::new_default(STORAGE_KEY);
 	child_info.prefixed_storage_key()
 }
 

client/rpc/src/system/tests.rs

@@ -354,26 +354,26 @@ fn test_add_reset_log_filter() {
 	};
 
 	// Initiate logs loop in child process
-	child_in.write(b"\n").unwrap();
+	child_in.write_all(b"\n").unwrap();
 	assert!(read_line().contains(EXPECTED_BEFORE_ADD));
 
 	// Initiate add directive & reload in child process
-	child_in.write(b"add_reload\n").unwrap();
+	child_in.write_all(b"add_reload\n").unwrap();
 	assert!(read_line().contains(EXPECTED_BEFORE_ADD));
 	assert!(read_line().contains(EXPECTED_AFTER_ADD));
 
 	// Check that increasing the max log level works
-	child_in.write(b"add_trace\n").unwrap();
+	child_in.write_all(b"add_trace\n").unwrap();
 	assert!(read_line().contains(EXPECTED_WITH_TRACE));
 	assert!(read_line().contains(EXPECTED_BEFORE_ADD));
 	assert!(read_line().contains(EXPECTED_AFTER_ADD));
 
 	// Initiate logs filter reset in child process
-	child_in.write(b"reset\n").unwrap();
+	child_in.write_all(b"reset\n").unwrap();
 	assert!(read_line().contains(EXPECTED_BEFORE_ADD));
 
 	// Return from child process
-	child_in.write(b"exit\n").unwrap();
+	child_in.write_all(b"exit\n").unwrap();
 	assert!(child_process.wait().expect("Error waiting for child process").success());
 
 	// Check for EOF

client/service/src/builder.rs

@@ -363,7 +363,7 @@ where
 		spawn_handle,
 		config.clone(),
 	)?;
-	Ok(crate::client::Client::new(
+	crate::client::Client::new(
 		backend,
 		executor,
 		genesis_storage,
@@ -373,7 +373,7 @@ where
 		prometheus_registry,
 		telemetry,
 		config,
-	)?)
+	)
 }
 
 /// Parameters to pass into `build`.

client/service/src/client/call_executor.rs

@@ -266,9 +266,7 @@ where
 					&runtime_code,
 					self.spawn_handle.clone(),
 				)
-				.with_storage_transaction_cache(
-					storage_transaction_cache.as_mut().map(|c| &mut **c),
-				)
+				.with_storage_transaction_cache(storage_transaction_cache.as_deref_mut())
 				.set_parent_hash(at_hash);
 				state_machine.execute_using_consensus_failure_handler(
 					execution_manager,