frame/proc-macro: Refactor code for better readability (#4712)

Small refactoring PR to improve the readability of the proc macros. - small improvement in docs - use new `let Some(..) else` expression - removed extra indentations by early returns Discovered during metadata v16 poc, extracted from: #4358 --------- Signed-off-by: Alexandru Vasile <[email protected]> Co-authored-by: Bastian Köcher <[email protected]> Co-authored-by: command-bot <> Co-authored-by: gupnik <[email protected]>
paritytech · Jun 8, 2024 · 07cfcf0 · 07cfcf0
1 parent 48d875d
commit 07cfcf0
Show file tree

Hide file tree

Showing 3 changed files with 70 additions and 77 deletions.
diff --git a/substrate/frame/support/procedural/src/pallet/expand/constants.rs b/substrate/frame/support/procedural/src/pallet/expand/constants.rs
@@ -30,8 +30,7 @@ struct ConstDef {
 	pub metadata_name: Option<syn::Ident>,
 }
 
-///
-/// * Impl fn module_constant_metadata for pallet.
+/// Implement the `pallet_constants_metadata` function for the pallet.
 pub fn expand_constants(def: &mut Def) -> proc_macro2::TokenStream {
 	let frame_support = &def.frame_support;
 	let type_impl_gen = &def.type_impl_generics(proc_macro2::Span::call_site());

diff --git a/substrate/frame/support/procedural/src/pallet/parse/config.rs b/substrate/frame/support/procedural/src/pallet/parse/config.rs
@@ -94,30 +94,26 @@ impl TryFrom<&syn::TraitItemType> for ConstMetadataDef {
 		let bound = trait_ty
 			.bounds
 			.iter()
-			.find_map(|b| {
-				if let syn::TypeParamBound::Trait(tb) = b {
-					tb.path
-						.segments
-						.last()
-						.and_then(|s| if s.ident == "Get" { Some(s) } else { None })
-				} else {
-					None
-				}
+			.find_map(|param_bound| {
+				let syn::TypeParamBound::Trait(trait_bound) = param_bound else { return None };
+
+				trait_bound.path.segments.last().and_then(|s| (s.ident == "Get").then(|| s))
 			})
 			.ok_or_else(|| err(trait_ty.span(), "`Get<T>` trait bound not found"))?;
-		let type_arg = if let syn::PathArguments::AngleBracketed(ref ab) = bound.arguments {
-			if ab.args.len() == 1 {
-				if let syn::GenericArgument::Type(ref ty) = ab.args[0] {
-					Ok(ty)
-				} else {
-					Err(err(ab.args[0].span(), "Expected a type argument"))
-				}
-			} else {
-				Err(err(bound.span(), "Expected a single type argument"))
-			}
-		} else {
-			Err(err(bound.span(), "Expected trait generic args"))
-		}?;
+
+		let syn::PathArguments::AngleBracketed(ref ab) = bound.arguments else {
+			return Err(err(bound.span(), "Expected trait generic args"))
+		};
+
+		// Only one type argument is expected.
+		if ab.args.len() != 1 {
+			return Err(err(bound.span(), "Expected a single type argument"))
+		}
+
+		let syn::GenericArgument::Type(ref type_arg) = ab.args[0] else {
+			return Err(err(ab.args[0].span(), "Expected a type argument"))
+		};
+
 		let type_ = syn::parse2::<syn::Type>(replace_self_by_t(type_arg.to_token_stream()))
 			.expect("Internal error: replacing `Self` by `T` should result in valid type");
 
@@ -223,55 +219,55 @@ fn check_event_type(
 	trait_item: &syn::TraitItem,
 	trait_has_instance: bool,
 ) -> syn::Result<bool> {
-	if let syn::TraitItem::Type(type_) = trait_item {
-		if type_.ident == "RuntimeEvent" {
-			// Check event has no generics
-			if !type_.generics.params.is_empty() || type_.generics.where_clause.is_some() {
-				let msg = "Invalid `type RuntimeEvent`, associated type `RuntimeEvent` is reserved and must have\
-					no generics nor where_clause";
-				return Err(syn::Error::new(trait_item.span(), msg))
-			}
+	let syn::TraitItem::Type(type_) = trait_item else { return Ok(false) };
 
-			// Check bound contains IsType and From
-			let has_is_type_bound = type_.bounds.iter().any(|s| {
-				syn::parse2::<IsTypeBoundEventParse>(s.to_token_stream())
-					.map_or(false, |b| has_expected_system_config(b.0, frame_system))
-			});
-
-			if !has_is_type_bound {
-				let msg = "Invalid `type RuntimeEvent`, associated type `RuntimeEvent` is reserved and must \
-					bound: `IsType<<Self as frame_system::Config>::RuntimeEvent>`".to_string();
-				return Err(syn::Error::new(type_.span(), msg))
-			}
+	if type_.ident != "RuntimeEvent" {
+		return Ok(false)
+	}
 
-			let from_event_bound = type_
-				.bounds
-				.iter()
-				.find_map(|s| syn::parse2::<FromEventParse>(s.to_token_stream()).ok());
+	// Check event has no generics
+	if !type_.generics.params.is_empty() || type_.generics.where_clause.is_some() {
+		let msg =
+			"Invalid `type RuntimeEvent`, associated type `RuntimeEvent` is reserved and must have\
+					no generics nor where_clause";
+		return Err(syn::Error::new(trait_item.span(), msg))
+	}
 
-			let from_event_bound = if let Some(b) = from_event_bound {
-				b
-			} else {
-				let msg = "Invalid `type RuntimeEvent`, associated type `RuntimeEvent` is reserved and must \
-					bound: `From<Event>` or `From<Event<Self>>` or `From<Event<Self, I>>`";
-				return Err(syn::Error::new(type_.span(), msg))
-			};
+	// Check bound contains IsType and From
+	let has_is_type_bound = type_.bounds.iter().any(|s| {
+		syn::parse2::<IsTypeBoundEventParse>(s.to_token_stream())
+			.map_or(false, |b| has_expected_system_config(b.0, frame_system))
+	});
+
+	if !has_is_type_bound {
+		let msg =
+			"Invalid `type RuntimeEvent`, associated type `RuntimeEvent` is reserved and must \
+					bound: `IsType<<Self as frame_system::Config>::RuntimeEvent>`"
+				.to_string();
+		return Err(syn::Error::new(type_.span(), msg))
+	}
 
-			if from_event_bound.is_generic && (from_event_bound.has_instance != trait_has_instance)
-			{
-				let msg = "Invalid `type RuntimeEvent`, associated type `RuntimeEvent` bounds inconsistent \
+	let from_event_bound = type_
+		.bounds
+		.iter()
+		.find_map(|s| syn::parse2::<FromEventParse>(s.to_token_stream()).ok());
+
+	let Some(from_event_bound) = from_event_bound else {
+		let msg =
+			"Invalid `type RuntimeEvent`, associated type `RuntimeEvent` is reserved and must \
+				bound: `From<Event>` or `From<Event<Self>>` or `From<Event<Self, I>>`";
+		return Err(syn::Error::new(type_.span(), msg))
+	};
+
+	if from_event_bound.is_generic && (from_event_bound.has_instance != trait_has_instance) {
+		let msg =
+			"Invalid `type RuntimeEvent`, associated type `RuntimeEvent` bounds inconsistent \
 					`From<Event..>`. Config and generic Event must be both with instance or \
 					without instance";
-				return Err(syn::Error::new(type_.span(), msg))
-			}
-
-			Ok(true)
-		} else {
-			Ok(false)
-		}
-	} else {
-		Ok(false)
+		return Err(syn::Error::new(type_.span(), msg))
 	}
+
+	Ok(true)
 }
 
 /// Check that the path to `frame_system::Config` is valid, this is that the path is just
@@ -334,9 +330,7 @@ impl ConfigDef {
 		item: &mut syn::Item,
 		enable_default: bool,
 	) -> syn::Result<Self> {
-		let item = if let syn::Item::Trait(item) = item {
-			item
-		} else {
+		let syn::Item::Trait(item) = item else {
 			let msg = "Invalid pallet::config, expected trait definition";
 			return Err(syn::Error::new(item.span(), msg))
 		};

diff --git a/substrate/frame/support/procedural/src/pallet/parse/helper.rs b/substrate/frame/support/procedural/src/pallet/parse/helper.rs
@@ -55,16 +55,16 @@ pub(crate) fn take_first_item_pallet_attr<Attr>(
 where
 	Attr: syn::parse::Parse,
 {
-	let attrs = if let Some(attrs) = item.mut_item_attrs() { attrs } else { return Ok(None) };
+	let Some(attrs) = item.mut_item_attrs() else { return Ok(None) };
 
-	if let Some(index) = attrs.iter().position(|attr| {
+	let Some(index) = attrs.iter().position(|attr| {
 		attr.path().segments.first().map_or(false, |segment| segment.ident == "pallet")
-	}) {
-		let pallet_attr = attrs.remove(index);
-		Ok(Some(syn::parse2(pallet_attr.into_token_stream())?))
-	} else {
-		Ok(None)
-	}
+	}) else {
+		return Ok(None)
+	};
+
+	let pallet_attr = attrs.remove(index);
+	Ok(Some(syn::parse2(pallet_attr.into_token_stream())?))
 }
 
 /// Take all the pallet attributes (e.g. attribute like `#[pallet..]`) and decode them to `Attr`