From 4a75dec5df88eed3883f4191b5f2d24f55a78c14 Mon Sep 17 00:00:00 2001 From: Tomasz Polaczyk Date: Mon, 21 Oct 2024 12:51:41 +0200 Subject: [PATCH 1/3] Use bool::then instead of then_some with function calls --- cumulus/polkadot-omni-node/lib/src/command.rs | 16 +++++++++------- polkadot/cli/src/command.rs | 10 ++++++---- .../src/collator_side/validators_buffer.rs | 2 +- substrate/bin/node/cli/src/service.rs | 10 ++++++---- substrate/client/network/src/discovery.rs | 2 +- .../client/network/sync/src/strategy/state.rs | 2 +- .../client/network/sync/src/strategy/warp.rs | 2 +- substrate/frame/contracts/proc-macro/src/lib.rs | 2 +- .../src/pallet/expand/pallet_struct.rs | 2 +- substrate/frame/support/src/migrations.rs | 4 ++-- templates/parachain/node/src/command.rs | 16 +++++++++------- 11 files changed, 38 insertions(+), 30 deletions(-) diff --git a/cumulus/polkadot-omni-node/lib/src/command.rs b/cumulus/polkadot-omni-node/lib/src/command.rs index 350dcfee1cdb..fe935a03cc95 100644 --- a/cumulus/polkadot-omni-node/lib/src/command.rs +++ b/cumulus/polkadot-omni-node/lib/src/command.rs @@ -266,13 +266,15 @@ pub fn run(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 = diff --git a/polkadot/cli/src/command.rs b/polkadot/cli/src/command.rs index d124c8fb7eb7..7c904e6658e7 100644 --- a/polkadot/cli/src/command.rs +++ b/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(); diff --git a/polkadot/node/network/collator-protocol/src/collator_side/validators_buffer.rs b/polkadot/node/network/collator-protocol/src/collator_side/validators_buffer.rs index fbb3ff4328a5..35202fc96299 100644 --- a/polkadot/node/network/collator-protocol/src/collator_side/validators_buffer.rs +++ b/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() { diff --git a/substrate/bin/node/cli/src/service.rs b/substrate/bin/node/cli/src/service.rs index 00658b361df9..602c0c5c5a61 100644 --- a/substrate/bin/node/cli/src/service.rs +++ b/substrate/bin/node/cli/src/service.rs @@ -419,10 +419,12 @@ pub fn new_full_base::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 { diff --git a/substrate/client/network/src/discovery.rs b/substrate/client/network/src/discovery.rs index 86c66c22701c..49e0797c126c 100644 --- a/substrate/client/network/src/discovery.rs +++ b/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) { diff --git a/substrate/client/network/sync/src/strategy/state.rs b/substrate/client/network/sync/src/strategy/state.rs index a04ab8be4fea..d69ab3e2d535 100644 --- a/substrate/client/network/sync/src/strategy/state.rs +++ b/substrate/client/network/sync/src/strategy/state.rs @@ -173,7 +173,7 @@ impl StateStrategy { peer_id: PeerId, announce: &BlockAnnounce, ) -> Option<(B::Hash, NumberFor)> { - 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) { diff --git a/substrate/client/network/sync/src/strategy/warp.rs b/substrate/client/network/sync/src/strategy/warp.rs index cce6a93caf43..0c71dd3c6aee 100644 --- a/substrate/client/network/sync/src/strategy/warp.rs +++ b/substrate/client/network/sync/src/strategy/warp.rs @@ -301,7 +301,7 @@ where peer_id: PeerId, announce: &BlockAnnounce, ) -> Option<(B::Hash, NumberFor)> { - 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) { diff --git a/substrate/frame/contracts/proc-macro/src/lib.rs b/substrate/frame/contracts/proc-macro/src/lib.rs index 84ea7de00a2f..4aba1d24dbd5 100644 --- a/substrate/frame/contracts/proc-macro/src/lib.rs +++ b/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! { diff --git a/substrate/frame/support/procedural/src/pallet/expand/pallet_struct.rs b/substrate/frame/support/procedural/src/pallet/expand/pallet_struct.rs index c6166ff45b19..79bf33a828e2 100644 --- a/substrate/frame/support/procedural/src/pallet/expand/pallet_struct.rs +++ b/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 = 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![ diff --git a/substrate/frame/support/src/migrations.rs b/substrate/frame/support/src/migrations.rs index 905d6143e4f1..3fdf8d6edc95 100644 --- a/substrate/frame/support/src/migrations.rs +++ b/substrate/frame/support/src/migrations.rs @@ -825,14 +825,14 @@ impl SteppedMigrations for T { fn nth_id(n: u32) -> Option> { 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> { // 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") } diff --git a/templates/parachain/node/src/command.rs b/templates/parachain/node/src/command.rs index 938bda837e0d..5d9308aed154 100644 --- a/templates/parachain/node/src/command.rs +++ b/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) From bd6aea834a864118ab94fbe2a04441d8e16e1bcf Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Mon, 21 Oct 2024 15:53:24 +0100 Subject: [PATCH 2/3] prdoc Signed-off-by: Oliver Tale-Yazdi --- prdoc/pr_6156.prdoc | 40 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) create mode 100644 prdoc/pr_6156.prdoc diff --git a/prdoc/pr_6156.prdoc b/prdoc/pr_6156.prdoc new file mode 100644 index 000000000000..d570a4c1219d --- /dev/null +++ b/prdoc/pr_6156.prdoc @@ -0,0 +1,40 @@ +title: "Use bool::then instead of then_some with function calls" +doc: +- audience: Node Dev + description: |- + 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. +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 From 7de77842749d79575e8fde47cab4a7247edd15e8 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Mon, 21 Oct 2024 15:55:02 +0100 Subject: [PATCH 3/3] shorter doc Signed-off-by: Oliver Tale-Yazdi --- prdoc/pr_6156.prdoc | 19 +------------------ 1 file changed, 1 insertion(+), 18 deletions(-) diff --git a/prdoc/pr_6156.prdoc b/prdoc/pr_6156.prdoc index d570a4c1219d..d20324a83a2f 100644 --- a/prdoc/pr_6156.prdoc +++ b/prdoc/pr_6156.prdoc @@ -2,25 +2,8 @@ title: "Use bool::then instead of then_some with function calls" doc: - audience: Node Dev description: |- - 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. + Fix misusage of `bool::then_some`. - 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. crates: - name: polkadot-omni-node-lib bump: patch