Use bool::then instead of then_some with function calls (paritytech#6156

)

I noticed that hardware benchmarks are being run even though we pass the
--no-hardware-benchmarks cli flag. After some debugging, the cause is an
incorrect usage of the `then_some` method.

From [std
docs](https://doc.rust-lang.org/std/primitive.bool.html#method.then_some):

> Arguments passed to then_some are eagerly evaluated; if you are
passing the result of a function call, it is recommended to use
[then](https://doc.rust-lang.org/std/primitive.bool.html#method.then),
which is lazily evaluated.

```rust
let mut a = 0;
let mut function_with_side_effects = || { a += 1; };

true.then_some(function_with_side_effects());
false.then_some(function_with_side_effects());

// `a` is incremented twice because the value passed to `then_some` is
// evaluated eagerly.
assert_eq!(a, 2);
```

This PR fixes all the similar usages of the `then_some` method across
the codebase.

polkadot address: 138eUqXvUYT3o4GdbnWQfGRzM8yDWh5Q2eFrFULL7RAXzdWD

---------

Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Co-authored-by: Shawn Tabrizi <[email protected]>
Co-authored-by: Oliver Tale-Yazdi <[email protected]>

cumulus/polkadot-omni-node/lib/src/command.rs

@@ -266,13 +266,15 @@ pub fn run<CliConfig: crate::cli::CliConfig>(cmd_config: RunConfig) -> Result<()
 				}
 
 				let hwbench = (!cli.no_hardware_benchmarks)
-					.then_some(config.database.path().map(|database_path| {
-						let _ = std::fs::create_dir_all(database_path);
-						sc_sysinfo::gather_hwbench(
-							Some(database_path),
-							&SUBSTRATE_REFERENCE_HARDWARE,
-						)
-					}))
+					.then(|| {
+						config.database.path().map(|database_path| {
+							let _ = std::fs::create_dir_all(database_path);
+							sc_sysinfo::gather_hwbench(
+								Some(database_path),
+								&SUBSTRATE_REFERENCE_HARDWARE,
+							)
+						})
+					})
 					.flatten();
 
 				let parachain_account =

polkadot/cli/src/command.rs

@@ -203,10 +203,12 @@ where
 
 	runner.run_node_until_exit(move |config| async move {
 		let hwbench = (!cli.run.no_hardware_benchmarks)
-			.then_some(config.database.path().map(|database_path| {
-				let _ = std::fs::create_dir_all(&database_path);
-				sc_sysinfo::gather_hwbench(Some(database_path), &SUBSTRATE_REFERENCE_HARDWARE)
-			}))
+			.then(|| {
+				config.database.path().map(|database_path| {
+					let _ = std::fs::create_dir_all(&database_path);
+					sc_sysinfo::gather_hwbench(Some(database_path), &SUBSTRATE_REFERENCE_HARDWARE)
+				})
+			})
 			.flatten();
 
 		let database_source = config.database.clone();

polkadot/node/network/collator-protocol/src/collator_side/validators_buffer.rs

@@ -110,7 +110,7 @@ impl ValidatorGroupsBuffer {
 			.validators
 			.iter()
 			.enumerate()
-			.filter_map(|(idx, authority_id)| bits[idx].then_some(authority_id.clone()))
+			.filter_map(|(idx, authority_id)| bits[idx].then(|| authority_id.clone()))
 			.collect();
 
 		if let Some(last_group) = self.group_infos.iter().last() {

prdoc/pr_6156.prdoc

@@ -0,0 +1,23 @@
+title: "Use bool::then instead of then_some with function calls"
+doc:
+- audience: Node Dev
+  description: |-
+    Fix misusage of `bool::then_some`.
+
+crates:
+- name: polkadot-omni-node-lib
+  bump: patch
+- name: polkadot-cli
+  bump: patch
+- name: polkadot-collator-protocol
+  bump: patch
+- name: sc-network
+  bump: patch
+- name: sc-network-sync
+  bump: patch
+- name: pallet-contracts-proc-macro
+  bump: patch
+- name: frame-support-procedural
+  bump: patch
+- name: frame-support
+  bump: patch

substrate/bin/node/cli/src/service.rs

@@ -420,10 +420,12 @@ pub fn new_full_base<N: NetworkBackend<Block, <Block as BlockT>::Hash>>(
 	let enable_offchain_worker = config.offchain_worker.enabled;
 
 	let hwbench = (!disable_hardware_benchmarks)
-		.then_some(config.database.path().map(|database_path| {
-			let _ = std::fs::create_dir_all(&database_path);
-			sc_sysinfo::gather_hwbench(Some(database_path), &SUBSTRATE_REFERENCE_HARDWARE)
-		}))
+		.then(|| {
+			config.database.path().map(|database_path| {
+				let _ = std::fs::create_dir_all(&database_path);
+				sc_sysinfo::gather_hwbench(Some(database_path), &SUBSTRATE_REFERENCE_HARDWARE)
+			})
+		})
 		.flatten();
 
 	let sc_service::PartialComponents {

substrate/client/network/src/discovery.rs

@@ -648,7 +648,7 @@ impl NetworkBehaviour for DiscoveryBehaviour {
 		let mut list: LinkedHashSet<_> = self
 			.permanent_addresses
 			.iter()
-			.filter_map(|(p, a)| (*p == peer_id).then_some(a.clone()))
+			.filter_map(|(p, a)| (*p == peer_id).then(|| a.clone()))
 			.collect();
 
 		if let Some(ephemeral_addresses) = self.ephemeral_addresses.get(&peer_id) {

substrate/client/network/sync/src/strategy/state.rs

@@ -173,7 +173,7 @@ impl<B: BlockT> StateStrategy<B> {
 		peer_id: PeerId,
 		announce: &BlockAnnounce<B::Header>,
 	) -> Option<(B::Hash, NumberFor<B>)> {
-		is_best.then_some({
+		is_best.then(|| {
 			let best_number = *announce.header.number();
 			let best_hash = announce.header.hash();
 			if let Some(ref mut peer) = self.peers.get_mut(&peer_id) {

substrate/client/network/sync/src/strategy/warp.rs

@@ -301,7 +301,7 @@ where
 		peer_id: PeerId,
 		announce: &BlockAnnounce<B::Header>,
 	) -> Option<(B::Hash, NumberFor<B>)> {
-		is_best.then_some({
+		is_best.then(|| {
 			let best_number = *announce.header.number();
 			let best_hash = announce.header.hash();
 			if let Some(ref mut peer) = self.peers.get_mut(&peer_id) {

substrate/frame/contracts/proc-macro/src/lib.rs

@@ -522,7 +522,7 @@ fn expand_docs(def: &EnvDef) -> TokenStream2 {
 ///    `expand_impls()`).
 fn expand_env(def: &EnvDef, docs: bool) -> TokenStream2 {
 	let impls = expand_impls(def);
-	let docs = docs.then_some(expand_docs(def)).unwrap_or(TokenStream2::new());
+	let docs = docs.then(|| expand_docs(def)).unwrap_or(TokenStream2::new());
 	let stable_api_count = def.host_funcs.iter().filter(|f| f.is_stable).count();
 
 	quote! {

substrate/frame/support/procedural/src/pallet/expand/pallet_struct.rs

@@ -171,7 +171,7 @@ pub fn expand_pallet_struct(def: &mut Def) -> proc_macro2::TokenStream {
 	let whitelisted_storage_idents: Vec<syn::Ident> = def
 		.storages
 		.iter()
-		.filter_map(|s| s.whitelisted.then_some(s.ident.clone()))
+		.filter_map(|s| s.whitelisted.then(|| s.ident.clone()))
 		.collect();
 
 	let whitelisted_storage_keys_impl = quote::quote![

substrate/frame/support/src/migrations.rs

@@ -825,14 +825,14 @@ impl<T: SteppedMigration> SteppedMigrations for T {
 
 	fn nth_id(n: u32) -> Option<Vec<u8>> {
 		n.is_zero()
-			.then_some(T::id().encode())
+			.then(|| T::id().encode())
 			.defensive_proof("nth_id should only be called with n==0")
 	}
 
 	fn nth_max_steps(n: u32) -> Option<Option<u32>> {
 		// It should be generally fine to call with n>0, but the code should not attempt to.
 		n.is_zero()
-			.then_some(T::max_steps())
+			.then(|| T::max_steps())
 			.defensive_proof("nth_max_steps should only be called with n==0")
 	}
 

templates/parachain/node/src/command.rs

@@ -220,13 +220,15 @@ pub fn run() -> Result<()> {
 
 			runner.run_node_until_exit(|config| async move {
 				let hwbench = (!cli.no_hardware_benchmarks)
-					.then_some(config.database.path().map(|database_path| {
-						let _ = std::fs::create_dir_all(database_path);
-						sc_sysinfo::gather_hwbench(
-							Some(database_path),
-							&SUBSTRATE_REFERENCE_HARDWARE,
-						)
-					}))
+					.then(|| {
+						config.database.path().map(|database_path| {
+							let _ = std::fs::create_dir_all(database_path);
+							sc_sysinfo::gather_hwbench(
+								Some(database_path),
+								&SUBSTRATE_REFERENCE_HARDWARE,
+							)
+						})
+					})
 					.flatten();
 
 				let para_id = chain_spec::Extensions::try_get(&*config.chain_spec)