From 8e6a0e2014dc7ba73a9a0216c1745ce25899af45 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile <60601340+lexnv@users.noreply.github.com> Date: Sat, 8 Jun 2024 10:48:42 +0300 Subject: [PATCH] frame/proc-macro: Refactor code for better readability (#4712) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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: https://github.com/paritytech/polkadot-sdk/pull/4358 --------- Signed-off-by: Alexandru Vasile Co-authored-by: Bastian Köcher Co-authored-by: command-bot <> Co-authored-by: gupnik --- .../procedural/src/pallet/expand/constants.rs | 3 +- .../procedural/src/pallet/parse/config.rs | 128 +++++++++--------- .../procedural/src/pallet/parse/helper.rs | 16 +-- 3 files changed, 70 insertions(+), 77 deletions(-) diff --git a/substrate/frame/support/procedural/src/pallet/expand/constants.rs b/substrate/frame/support/procedural/src/pallet/expand/constants.rs index 57fa8b7f3cd9a..d7fbb5a718973 100644 --- 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, } -/// -/// * 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 index 406072df4b9d6..eaeaab2475880 100644 --- 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` 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::(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 { - 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::(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<::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::(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` or `From>` or `From>`"; - 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::(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<::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::(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` or `From>` or `From>`"; + 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`. 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 { - 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 index 3187c9139c8f4..d4f58a4c56df8 100644 --- 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( 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`