Refactor: Remove unused variable assignments for fallible functions (#8247)

# Description
This PR resolves issue #8236.

I recently graduated from PBA, and during one of our sessions,
[@shawntabrizi](https://github.com/shawntabrizi) pointed out an
important issue related to error handling in Rust.

When using let _ = some_fallible_function();, if the result is not
followed by a ?, the error is silently swallowed without any warning or
compiler feedback.

In contrast, if we don’t use let _ = and forget to add a ?, the compiler
will correctly emit a warning or error — helping the developer catch the
issue early.

This behavior can easily lead to bugs going unnoticed and makes error
handling less reliable, especially for beginners following examples.


## Integration
This PR introduces no functional or logical changes, and therefore can
safely be integrated into existing downstream projects without
additional adjustments.

From my point of view, this issue can be classified as something like
`I4-Silent`.

## Review Notes

I went through all occurrences of `let _ =` with a fallible function.
Some of them return values tagged as `#[must_use]`. In these cases, I
retained the underscore operator intentionally _(see:
`polkadot/node/core/av-store/src/lib.rs` lines: 1099 & 1108,
`polkadot/xcm/xcm-builder/src/currency_adapter.rs` line: 217,
`substrate/frame/contracts/src/wasm/mod.rs` line: 360,
`substrate/frame/revive/src/wasm/mod.rs` line: 307)_.

Co-authored-by: Shawn Tabrizi <[email protected]>
Co-authored-by: Bastian Köcher <[email protected]>

Author: 3 people

bridges/modules/xcm-bridge-hub-router/src/lib.rs

@@ -167,7 +167,7 @@ pub mod pallet {
 			bridge_id: H256,
 			is_congested: bool,
 		) -> DispatchResult {
-			let _ = T::BridgeHubOrigin::ensure_origin(origin)?;
+			T::BridgeHubOrigin::ensure_origin(origin)?;
 
 			log::info!(
 				target: LOG_TARGET,
@@ -405,7 +405,7 @@ impl<T: Config<I>, I: 'static> SendXcm for Pallet<T, I> {
 				// to avoid losing funds).
 				let destination_version = T::DestinationVersion::get_version_for(&dest_clone)
 					.ok_or(SendError::DestinationUnsupported)?;
-				let _ = VersionedXcm::from(xcm_to_dest_clone)
+				VersionedXcm::from(xcm_to_dest_clone)
 					.into_version(destination_version)
 					.map_err(|()| SendError::DestinationUnsupported)?;
 

cumulus/primitives/utility/src/tests/swap_first.rs

@@ -411,7 +411,7 @@ pub mod mock {
 				Ok(c) => c,
 				Err(_) => return Err((credit_in, DispatchError::Unavailable)),
 			};
-			let _ = Fungibles::resolve(&pool_account, credit_in)
+			Fungibles::resolve(&pool_account, credit_in)
 				.map_err(|c| (c, DispatchError::Unavailable))?;
 			Ok(credit_out)
 		}
@@ -439,7 +439,7 @@ pub mod mock {
 				Err(_) => return Err((credit_in, DispatchError::Unavailable)),
 			};
 			let (credit_in, change) = credit_in.split(amount_out);
-			let _ = Fungibles::resolve(&pool_account, credit_in)
+			Fungibles::resolve(&pool_account, credit_in)
 				.map_err(|c| (c, DispatchError::Unavailable))?;
 			Ok((credit_out, change))
 		}

docs/sdk/src/reference_docs/frame_origin.rs

@@ -241,7 +241,7 @@ pub mod pallet_with_external_origin {
 	#[pallet::call]
 	impl<T: Config> Pallet<T> {
 		pub fn externally_checked_ext(origin: OriginFor<T>) -> DispatchResult {
-			let _ = T::ExternalOrigin::ensure_origin(origin)?;
+			T::ExternalOrigin::ensure_origin(origin)?;
 			todo!();
 		}
 	}

polkadot/node/core/approval-voting/src/ops.rs

@@ -108,7 +108,7 @@ pub fn canonicalize(
 		overlay_db.delete_blocks_at_height(i);
 
 		for b in at_height {
-			let _ = visit_and_remove_block_entry(b, overlay_db, &mut visited_candidates)?;
+			visit_and_remove_block_entry(b, overlay_db, &mut visited_candidates)?;
 		}
 	}
 
@@ -219,7 +219,7 @@ pub fn add_block_entry(
 		let mut blocks_at_height = store.load_blocks_at_height(&number)?;
 		if blocks_at_height.contains(&entry.block_hash()) {
 			// seems we already have a block entry for this block. nothing to do here.
-			return Ok(Vec::new())
+			return Ok(Vec::new());
 		}
 
 		blocks_at_height.push(entry.block_hash());
@@ -372,7 +372,7 @@ pub fn revert_to(
 			if child_entry.parent_hash() != hash {
 				return Err(SubsystemError::Context(
 					"revert below last finalized block or corrupted storage".to_string(),
-				))
+				));
 			}
 
 			(children, children_height)

polkadot/runtime/common/src/slots/mod.rs

@@ -210,7 +210,7 @@ pub mod pallet {
 		#[pallet::call_index(2)]
 		#[pallet::weight(T::WeightInfo::trigger_onboard())]
 		pub fn trigger_onboard(origin: OriginFor<T>, para: ParaId) -> DispatchResult {
-			let _ = ensure_signed(origin)?;
+			ensure_signed(origin)?;
 			let leases = Leases::<T>::get(para);
 			match leases.first() {
 				// If the first element in leases is present, then it has a lease!

polkadot/runtime/parachains/src/hrmp.rs

@@ -788,7 +788,7 @@ pub mod pallet {
 				(config.hrmp_sender_deposit, config.hrmp_recipient_deposit)
 			};
 
-			let _ = HrmpChannels::<T>::mutate(&channel_id, |channel| -> DispatchResult {
+			HrmpChannels::<T>::mutate(&channel_id, |channel| -> DispatchResult {
 				if let Some(ref mut channel) = channel {
 					let current_sender_deposit = channel.sender_deposit;
 					let current_recipient_deposit = channel.recipient_deposit;

polkadot/runtime/rococo/src/impls.rs

@@ -169,7 +169,7 @@ where
 		]);
 
 		// send
-		let _ = <pallet_xcm::Pallet<Runtime>>::send(
+		<pallet_xcm::Pallet<Runtime>>::send(
 			RawOrigin::Root.into(),
 			Box::new(VersionedLocation::from(destination)),
 			Box::new(VersionedXcm::from(program)),

polkadot/runtime/westend/src/impls.rs

@@ -169,7 +169,7 @@ where
 		]);
 
 		// send
-		let _ = <pallet_xcm::Pallet<Runtime>>::send(
+		<pallet_xcm::Pallet<Runtime>>::send(
 			RawOrigin::Root.into(),
 			Box::new(VersionedLocation::from(destination)),
 			Box::new(VersionedXcm::from(program)),

polkadot/xcm/xcm-builder/src/universal_exports.rs

@@ -529,7 +529,7 @@ impl<
 			message.0.insert(0, DescendOrigin(bridge_instance));
 		}
 
-		let _ = send_xcm::<Router>(dest, message).map_err(|_| DispatchBlobError::RoutingError)?;
+		send_xcm::<Router>(dest, message).map_err(|_| DispatchBlobError::RoutingError)?;
 		Ok(())
 	}
 }

substrate/bin/utils/chain-spec-builder/src/lib.rs

@@ -294,7 +294,7 @@ impl ChainSpecBuilder {
 			},
 			ChainSpecBuilderCmd::Verify(VerifyCmd { ref input_chain_spec }) => {
 				let chain_spec = ChainSpec::from_json_file(input_chain_spec.clone())?;
-				let _ = serde_json::from_str::<serde_json::Value>(&chain_spec.as_json(true)?)
+				serde_json::from_str::<serde_json::Value>(&chain_spec.as_json(true)?)
 					.map_err(|e| format!("Conversion to json failed: {e}"))?;
 			},
 			ChainSpecBuilderCmd::ListPresets(ListPresetsCmd { runtime }) => {

substrate/client/consensus/grandpa/src/communication/mod.rs

@@ -988,7 +988,7 @@ fn check_catch_up<Block: BlockT>(
 	)?;
 
 	// check signatures on all contained precommits.
-	let _ = check_signatures::<Block, _>(
+	check_signatures::<Block, _>(
 		msg.precommits.iter().map(|vote| {
 			(
 				finality_grandpa::Message::Precommit(vote.precommit.clone()),

substrate/client/executor/wasmtime/src/tests.rs

@@ -349,7 +349,7 @@ fn test_max_memory_pages(
 
 		let runtime = builder.build();
 		let mut instance = runtime.new_instance().unwrap();
-		let _ = instance.call_export("main", &[])?;
+		instance.call_export("main", &[])?;
 		Ok(())
 	}
 

substrate/client/network/sync/src/engine.rs

@@ -112,7 +112,7 @@ struct Metrics {
 
 impl Metrics {
 	fn register(r: &Registry, major_syncing: Arc<AtomicBool>) -> Result<Self, PrometheusError> {
-		let _ = MajorSyncingGauge::register(r, major_syncing)?;
+		MajorSyncingGauge::register(r, major_syncing)?;
 		Ok(Self {
 			peers: {
 				let g = Gauge::new("substrate_sync_peers", "Number of peers we sync with")?;

substrate/client/service/src/client/client.rs

@@ -1372,7 +1372,7 @@ where
 	) -> sp_blockchain::Result<(KeyValueStates, usize)> {
 		let mut db = sp_state_machine::MemoryDB::<HashingFor<Block>>::new(&[]);
 		// Compact encoding
-		let _ = sp_trie::decode_compact::<sp_state_machine::LayoutV0<HashingFor<Block>>, _, _>(
+		sp_trie::decode_compact::<sp_state_machine::LayoutV0<HashingFor<Block>>, _, _>(
 			&mut db,
 			proof.iter_compact_encoded_nodes(),
 			Some(&root),

substrate/frame/asset-conversion/ops/src/lib.rs

@@ -154,7 +154,7 @@ pub mod pallet {
 			asset1: Box<T::AssetKind>,
 			asset2: Box<T::AssetKind>,
 		) -> DispatchResultWithPostInfo {
-			let _ = ensure_signed(origin)?;
+			ensure_signed(origin)?;
 
 			let pool_id = T::PoolLocator::pool_id(&asset1, &asset2)
 				.map_err(|_| Error::<T>::InvalidAssetPair)?;

substrate/frame/assets/src/functions.rs

@@ -815,37 +815,36 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 	) -> Result<u32, DispatchError> {
 		let mut dead_accounts: Vec<T::AccountId> = vec![];
 		let mut remaining_accounts = 0;
-		let _ =
-			Asset::<T, I>::try_mutate_exists(&id, |maybe_details| -> Result<(), DispatchError> {
-				let mut details = maybe_details.as_mut().ok_or(Error::<T, I>::Unknown)?;
-				// Should only destroy accounts while the asset is in a destroying state
-				ensure!(details.status == AssetStatus::Destroying, Error::<T, I>::IncorrectStatus);
+		Asset::<T, I>::try_mutate_exists(&id, |maybe_details| -> Result<(), DispatchError> {
+			let mut details = maybe_details.as_mut().ok_or(Error::<T, I>::Unknown)?;
+			// Should only destroy accounts while the asset is in a destroying state
+			ensure!(details.status == AssetStatus::Destroying, Error::<T, I>::IncorrectStatus);
 
-				for (i, (who, mut v)) in Account::<T, I>::iter_prefix(&id).enumerate() {
-					if Self::ensure_account_can_die(id.clone(), &who).is_err() {
-						continue
-					}
-					// unreserve the existence deposit if any
-					if let Some((depositor, deposit)) = v.reason.take_deposit_from() {
-						T::Currency::unreserve(&depositor, deposit);
-					} else if let Some(deposit) = v.reason.take_deposit() {
-						T::Currency::unreserve(&who, deposit);
-					}
-					if let Remove = Self::dead_account(&who, &mut details, &v.reason, false) {
-						Account::<T, I>::remove(&id, &who);
-						dead_accounts.push(who);
-					} else {
-						// deposit may have been released, need to update `Account`
-						Account::<T, I>::insert(&id, &who, v);
-						defensive!("destroy did not result in dead account?!");
-					}
-					if i + 1 >= (max_items as usize) {
-						break
-					}
+			for (i, (who, mut v)) in Account::<T, I>::iter_prefix(&id).enumerate() {
+				if Self::ensure_account_can_die(id.clone(), &who).is_err() {
+					continue
 				}
-				remaining_accounts = details.accounts;
-				Ok(())
-			})?;
+				// unreserve the existence deposit if any
+				if let Some((depositor, deposit)) = v.reason.take_deposit_from() {
+					T::Currency::unreserve(&depositor, deposit);
+				} else if let Some(deposit) = v.reason.take_deposit() {
+					T::Currency::unreserve(&who, deposit);
+				}
+				if let Remove = Self::dead_account(&who, &mut details, &v.reason, false) {
+					Account::<T, I>::remove(&id, &who);
+					dead_accounts.push(who);
+				} else {
+					// deposit may have been released, need to update `Account`
+					Account::<T, I>::insert(&id, &who, v);
+					defensive!("destroy did not result in dead account?!");
+				}
+				if i + 1 >= (max_items as usize) {
+					break
+				}
+			}
+			remaining_accounts = details.accounts;
+			Ok(())
+		})?;
 
 		for who in &dead_accounts {
 			T::Freezer::died(id.clone(), &who);
@@ -869,7 +868,7 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 		max_items: u32,
 	) -> Result<u32, DispatchError> {
 		let mut removed_approvals = 0;
-		let _ = Asset::<T, I>::try_mutate_exists(
+		Asset::<T, I>::try_mutate_exists(
 			id.clone(),
 			|maybe_details| -> Result<(), DispatchError> {
 				let details = maybe_details.as_mut().ok_or(Error::<T, I>::Unknown)?;

substrate/frame/assets/src/lib.rs

@@ -844,7 +844,7 @@ pub mod pallet {
 			origin: OriginFor<T>,
 			id: T::AssetIdParameter,
 		) -> DispatchResultWithPostInfo {
-			let _ = ensure_signed(origin)?;
+			ensure_signed(origin)?;
 			let id: T::AssetId = id.into();
 			let removed_accounts = Self::do_destroy_accounts(id, T::RemoveItemsLimit::get())?;
 			Ok(Some(T::WeightInfo::destroy_accounts(removed_accounts)).into())
@@ -868,7 +868,7 @@ pub mod pallet {
 			origin: OriginFor<T>,
 			id: T::AssetIdParameter,
 		) -> DispatchResultWithPostInfo {
-			let _ = ensure_signed(origin)?;
+			ensure_signed(origin)?;
 			let id: T::AssetId = id.into();
 			let removed_approvals = Self::do_destroy_approvals(id, T::RemoveItemsLimit::get())?;
 			Ok(Some(T::WeightInfo::destroy_approvals(removed_approvals)).into())
@@ -886,7 +886,7 @@ pub mod pallet {
 		/// Each successful call emits the `Event::Destroyed` event.
 		#[pallet::call_index(5)]
 		pub fn finish_destroy(origin: OriginFor<T>, id: T::AssetIdParameter) -> DispatchResult {
-			let _ = ensure_signed(origin)?;
+			ensure_signed(origin)?;
 			let id: T::AssetId = id.into();
 			Self::do_finish_destroy(id)
 		}
@@ -944,7 +944,7 @@ pub mod pallet {
 			let id: T::AssetId = id.into();
 
 			let f = DebitFlags { keep_alive: false, best_effort: true };
-			let _ = Self::do_burn(id, &who, amount, Some(origin), f)?;
+			Self::do_burn(id, &who, amount, Some(origin), f)?;
 			Ok(())
 		}
 

substrate/frame/bags-list/src/lib.rs

@@ -303,7 +303,7 @@ pub mod pallet {
 			ensure_signed(origin)?;
 			let dislocated = T::Lookup::lookup(dislocated)?;
 			let current_score = T::ScoreProvider::score(&dislocated);
-			let _ = Pallet::<T, I>::do_rebag(&dislocated, current_score)
+			Pallet::<T, I>::do_rebag(&dislocated, current_score)
 				.map_err::<Error<T, I>, _>(Into::into)?;
 			Ok(())
 		}
@@ -341,7 +341,7 @@ pub mod pallet {
 			heavier: AccountIdLookupOf<T>,
 			lighter: AccountIdLookupOf<T>,
 		) -> DispatchResult {
-			let _ = ensure_signed(origin)?;
+			ensure_signed(origin)?;
 			let lighter = T::Lookup::lookup(lighter)?;
 			let heavier = T::Lookup::lookup(heavier)?;
 			List::<T, I>::put_in_front_of(&lighter, &heavier)

substrate/frame/bags-list/src/list/mod.rs

@@ -569,7 +569,7 @@ impl<T: Config<I>, I: 'static> List<T, I> {
 		// build map of bags and the corresponding nodes to avoid multiple lookups
 		let mut bags_map = BTreeMap::<T::Score, Vec<T::AccountId>>::new();
 
-		let _ = active_bags.clone().try_for_each(|b| {
+		active_bags.clone().try_for_each(|b| {
 			bags_map.insert(
 				b.bag_upper,
 				b.iter().map(|n: Node<T, I>| n.id().clone()).collect::<Vec<_>>(),

substrate/frame/bounties/src/lib.rs

@@ -663,7 +663,7 @@ pub mod pallet {
 			origin: OriginFor<T>,
 			#[pallet::compact] bounty_id: BountyIndex,
 		) -> DispatchResult {
-			let _ = ensure_signed(origin)?; // anyone can trigger claim
+			ensure_signed(origin)?; // anyone can trigger claim
 
 			Bounties::<T, I>::try_mutate_exists(bounty_id, |maybe_bounty| -> DispatchResult {
 				let bounty = maybe_bounty.take().ok_or(Error::<T, I>::InvalidIndex)?;

substrate/frame/broker/src/lib.rs

@@ -811,7 +811,7 @@ pub mod pallet {
 			region_id: RegionId,
 			max_timeslices: Timeslice,
 		) -> DispatchResultWithPostInfo {
-			let _ = ensure_signed(origin)?;
+			ensure_signed(origin)?;
 			Self::do_claim_revenue(region_id, max_timeslices)?;
 			Ok(Pays::No.into())
 		}

substrate/frame/child-bounties/src/lib.rs

@@ -688,7 +688,7 @@ pub mod pallet {
 			#[pallet::compact] parent_bounty_id: BountyIndex,
 			#[pallet::compact] child_bounty_id: BountyIndex,
 		) -> DispatchResult {
-			let _ = ensure_signed(origin)?;
+			ensure_signed(origin)?;
 
 			// Ensure child-bounty is in expected state.
 			ChildBounties::<T>::try_mutate_exists(

substrate/frame/collective/src/lib.rs

@@ -837,7 +837,7 @@ pub mod pallet {
 			proposal_weight_bound: Weight,
 			#[pallet::compact] length_bound: u32,
 		) -> DispatchResultWithPostInfo {
-			let _ = ensure_signed(origin)?;
+			ensure_signed(origin)?;
 
 			Self::do_close(proposal_hash, index, proposal_weight_bound, length_bound)
 		}
@@ -886,13 +886,13 @@ pub mod pallet {
 			origin: OriginFor<T>,
 			proposal_hash: T::Hash,
 		) -> DispatchResult {
-			let _ = ensure_signed_or_root(origin)?;
+			ensure_signed_or_root(origin)?;
 			ensure!(
 				ProposalOf::<T, I>::get(&proposal_hash).is_none(),
 				Error::<T, I>::ProposalActive
 			);
 			if let Some((who, cost)) = <CostOf<T, I>>::take(proposal_hash) {
-				let _ = cost.drop(&who)?;
+				cost.drop(&who)?;
 				Self::deposit_event(Event::ProposalCostReleased { proposal_hash, who });
 			}
 

substrate/frame/contracts/src/exec.rs

@@ -1956,7 +1956,7 @@ mod tests {
 
 		let delegate_ch = MockLoader::insert(Call, move |ctx, _| {
 			assert_eq!(ctx.ext.value_transferred(), value);
-			let _ = ctx.ext.delegate_call(success_ch, Vec::new())?;
+			ctx.ext.delegate_call(success_ch, Vec::new())?;
 			Ok(ExecReturnValue { flags: ReturnFlags::empty(), data: Vec::new() })
 		});
 

substrate/frame/contracts/src/wasm/prepare.rs

@@ -192,7 +192,7 @@ impl LoadedModule {
 				ExternType::Table(_) => return Err("Cannot import tables"),
 				ExternType::Global(_) => return Err("Cannot import globals"),
 				ExternType::Func(_) => {
-					let _ = import.ty().func().ok_or("expected a function")?;
+					import.ty().func().ok_or("expected a function")?;
 
 					if !<T as Config>::ChainExtension::enabled() &&
 						(import.name().as_bytes() == b"seal_call_chain_extension" ||
@@ -368,7 +368,7 @@ pub mod benchmarking {
 			LoadingMode::Checked,
 			CompilationMode::Eager,
 		)?;
-		let _ = contract_module.scan_imports::<T>(schedule)?;
+		contract_module.scan_imports::<T>(schedule)?;
 		let code: CodeVec<T> = code.try_into().map_err(|_| <Error<T>>::CodeTooLarge)?;
 		let code_info = CodeInfo {
 			owner,