Skip to content

Commit

Permalink
Always use associated type bound
Browse files Browse the repository at this point in the history
  • Loading branch information
@dastansam
dastansam committed Feb 16, 2025
1 parent f4e70bd commit 85171bc
Show file tree
Hide file tree
Showing 7 changed files with 51 additions and 222 deletions.
8 changes: 4 additions & 4 deletions prdoc/pr_7229.prdoc
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0
# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json

title: "FRAME: Remove `RuntimeEvent` associated type from `Config` trait"
title: "FRAME: Deprecate `RuntimeEvent` associated type from `Config` trait"
doc:
- audience: Runtime Dev
description: |
Expand All @@ -25,12 +25,12 @@ doc:
type RuntimeEvent: From<Event<Self>> + IsType<<Self as frame_system::Config>::RuntimeEvent>;
}
```

Or if developers want to explicitly define the `RuntimeEvent` type bound, they can still do so.
The latter compiles but is redundant since the associated type bound is automatically appended
if pallet defines `Event` type, i.e it looks like this after macro expansion:

```rs
#[pallet::config]
pub trait Config: frame_system::Config<RuntimeEvent: From<Event<Self>>> {
pub trait Config: frame_system::Config + frame_system::Config<RuntimeEvent: From<Event<Self>>> {
}
```

Expand Down
64 changes: 22 additions & 42 deletions substrate/frame/support/procedural/src/pallet/expand/config.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,10 @@
// See the License for the specific language governing permissions and
// limitations under the License.

use crate::pallet::{parse::config::has_expected_system_config, Def};
use crate::pallet::{parse::GenericKind, Def};
use proc_macro2::TokenStream;
use quote::{quote, ToTokens};
use syn::{parse_quote, Item, PathArguments, TypeParamBound};
use quote::quote;
use syn::{parse_quote, Item};

///
/// * Generate default rust doc
Expand Down Expand Up @@ -50,34 +50,25 @@ Consequently, a runtime that wants to include this pallet must implement this tr

// insert `frame_system::Config` supertrait with `RuntimeEvent: From<Event<Self>>` if neither
// associated type nor type bound is defined.
if def.event.is_some() && !config.has_event_type && !config.has_event_bound {
// find the `frame_system::Config` supertrait
let frame_system = crate::generate_access_from_frame_or_crate("frame-system")
.expect("should have access to frame-system");

if let Some(TypeParamBound::Trait(trait_bound)) =
config_item.supertraits.iter_mut().find(|s| {
syn::parse2::<syn::Path>(s.to_token_stream())
.map_or(false, |b| has_expected_system_config(b, &frame_system))
}) {
let event_bound: syn::GenericArgument = parse_quote!(RuntimeEvent: From<Event<Self>>);

if let Some(segment) =
trait_bound.path.segments.iter_mut().find(|s| s.ident == "Config")
{
match &mut segment.arguments {
// when `Config<SomeType: Bound`
syn::PathArguments::AngleBracketed(args) => {
args.args.push(event_bound);
},
// when `Config`
syn::PathArguments::None => {
segment.arguments =
PathArguments::AngleBracketed(parse_quote!(<#event_bound>));
},
_ => unreachable!("Checked by has_expected_system_config"),
}
}
if let Some(event) = &def.event {
if !def.is_frame_system {
let frame_system = crate::generate_access_from_frame_or_crate("frame-system")
.expect("should have access to frame-system");

// can't use `type_use_gen()` since it returns `T`, not `Self`
let event_use_gen = match event.gen_kind {
GenericKind::None => quote!(),
GenericKind::Config => quote::quote_spanned! {event.attr_span => Self},
GenericKind::ConfigAndInstance =>
quote::quote_spanned! {event.attr_span => Self, I},
};

let supertrait_with_event_bound = syn::parse2::<syn::TypeParamBound>(
quote! { #frame_system::Config<RuntimeEvent: From<Event<#event_use_gen>>> },
)
.expect("should be possible to parse system supertrait");

config_item.supertraits.push(supertrait_with_event_bound.into());
}
}

Expand Down Expand Up @@ -177,14 +168,3 @@ pub fn expand_config_metadata(def: &Def) -> proc_macro2::TokenStream {
}
)
}

#[test]
fn test_parse_quote() {
// pub trait Config: pallet_balances::Config + frame_system::Config<RuntimeEvent:
// From<Event<Self>>>{

// the `RuntimeEvent: From<Event<Self>>` part
let event: syn::AngleBracketedGenericArguments = parse_quote!(RuntimeEvent: From<Event<Self>>);
let event_bound = syn::parse2::<syn::PathSegment>(event.to_token_stream()).unwrap();
println!("{:?}", event_bound);
}
15 changes: 3 additions & 12 deletions substrate/frame/support/procedural/src/pallet/expand/event.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -136,32 +136,23 @@ pub fn expand_event(def: &mut Def) -> proc_macro2::TokenStream {

let deposit_event = if let Some(deposit_event) = &event.deposit_event {
let event_use_gen = &event.gen_kind.type_use_gen(event.attr_span);
let trait_use_gen = &def.trait_use_generics(event.attr_span);
let type_impl_gen = &def.type_impl_generics(event.attr_span);
let type_use_gen = &def.type_use_generics(event.attr_span);
let pallet_ident = &def.pallet_struct.pallet;

let PalletEventDepositAttr { fn_vis, fn_span, .. } = deposit_event;

// `RuntimeEvent` can be defined either as associated type in `Config` or as a type bound in
// system supertrait.
let runtime_event_path = if def.config.has_event_bound || !def.config.has_event_type {
quote::quote! { <T as #frame_system::Config>::RuntimeEvent }
} else {
quote::quote! { <T as Config #trait_use_gen>::RuntimeEvent }
};

quote::quote_spanned!(*fn_span =>
impl<#type_impl_gen> #pallet_ident<#type_use_gen> #completed_where_clause {
#fn_vis fn deposit_event(event: Event<#event_use_gen>) {
let event = <
#runtime_event_path as
<T as #frame_system::Config>::RuntimeEvent as
From<Event<#event_use_gen>>
>::from(event);

let event = <
#runtime_event_path as
Into<#runtime_event_path>
<T as #frame_system::Config>::RuntimeEvent as
Into<<T as #frame_system::Config>::RuntimeEvent>
>::into(event);

<#frame_system::Pallet<T>>::deposit_event(event)
Expand Down
102 changes: 1 addition & 101 deletions substrate/frame/support/procedural/src/pallet/parse/config.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,13 +59,6 @@ pub struct ConfigDef {
pub consts_metadata: Vec<ConstMetadataDef>,
/// Associated types metadata.
pub associated_types_metadata: Vec<AssociatedTypeMetadataDef>,
/// Whether the trait has the associated type `Event`, note that those bounds are
/// checked:
/// * `IsType<Self as frame_system::Config>::RuntimeEvent`
/// * `From<Event>` or `From<Event<T>>` or `From<Event<T, I>>`
pub has_event_type: bool,
/// Whether the supertrait `frame_system::Config` defines associated type `RuntimeEvent`.
pub has_event_bound: bool,
/// The where clause on trait definition but modified so `Self` is `T`.
pub where_clause: Option<syn::WhereClause>,
/// Whether a default sub-trait should be generated.
Expand Down Expand Up @@ -371,38 +364,6 @@ fn contains_type_info_bound(ty: &TraitItemType) -> bool {
})
}

/// Check that supertrait `Config` contains `RuntimeEvent` associated type bound with
/// `From<Event<Self>>`.
///
/// NOTE: Does not check if the supertrait path is valid system config path.
///
/// ```rs
/// pub trait Config: frame_system::Config {
/// ```
fn contains_runtime_event_associated_type_bound(supertrait: &syn::Path) -> bool {
if let Some(args) = supertrait.segments.iter().find(|s| s.ident == "Config") {
if let syn::PathArguments::AngleBracketed(args) = &args.arguments {
for arg in &args.args {
if let syn::GenericArgument::Constraint(c) = arg {
if c.ident != "RuntimeEvent" {
continue;
}

// Check `From<Event<Self>>` bound
let from_event_bound = c
.bounds
.iter()
.find_map(|s| syn::parse2::<FromEventParse>(s.to_token_stream()).ok());

return from_event_bound.is_some();
}
}
}
}

false
}

impl ConfigDef {
pub fn try_from(
frame_system: &syn::Path,
Expand Down Expand Up @@ -444,16 +405,6 @@ impl ConfigDef {
false
};

let has_event_bound = if is_frame_system {
false
} else {
item.supertraits.iter().any(|supertrait| {
syn::parse2::<syn::Path>(supertrait.to_token_stream())
.map_or(false, |b| contains_runtime_event_associated_type_bound(&b))
})
};

let mut has_event_type = false;
let mut consts_metadata = vec![];
let mut associated_types_metadata = vec![];
let mut warnings = vec![];
Expand All @@ -464,7 +415,6 @@ impl ConfigDef {
};
for trait_item in &mut item.items {
let is_event = check_event_type(frame_system, trait_item, has_instance)?;
has_event_type = has_event_type || is_event;

let mut already_no_default = false;
let mut already_constant = false;
Expand All @@ -480,7 +430,7 @@ impl ConfigDef {
if !type_event.attrs.iter().any(|attr| attr == &allow_dep) {
let warning = Warning::new_deprecated("RuntimeEvent")
.old("have `RuntimeEvent` associated type in the pallet config")
.new("remove it or explicitly define it as an associated type bound in the system supertrait: \n
.new("remove it as it is redundant since associated bound gets appended automatically: \n
pub trait Config: frame_system::Config<RuntimeEvent: From<Event<Self>>> { }")
.help_link("https://github.com/paritytech/polkadot-sdk/pull/7229")
.span(type_event.ident.span())
Expand Down Expand Up @@ -648,8 +598,6 @@ impl ConfigDef {
has_instance,
consts_metadata,
associated_types_metadata,
has_event_type,
has_event_bound,
where_clause,
default_sub_trait,
warnings,
Expand Down Expand Up @@ -774,52 +722,4 @@ mod tests {
let path = syn::parse2::<syn::Path>(quote::quote!(something::Config)).unwrap();
assert!(!has_expected_system_config(path, &frame_system));
}

#[test]
fn contains_runtime_event_associated_type_bound_no_bound() {
let supertrait = syn::parse2::<syn::Path>(quote::quote!(frame_system::Config)).unwrap();
assert!(!contains_runtime_event_associated_type_bound(&supertrait));
}

#[test]
fn contains_runtime_event_associated_type_bound_works() {
let supertrait = syn::parse2::<syn::Path>(quote::quote!(Config));
assert!(contains_runtime_event_associated_type_bound(&supertrait.unwrap()));
}
#[test]
fn contains_runtime_event_associated_type_bound_works_interface() {
let supertrait = syn::parse2::<syn::Path>(quote::quote!(
Config<RuntimeEvent: From<Event<Self, I>>>
));
assert!(contains_runtime_event_associated_type_bound(&supertrait.unwrap()));
}

#[test]
fn contains_runtime_event_associated_type_bound_works_full_path() {
let supertrait = syn::parse2::<syn::Path>(quote::quote!(frame_system::Config));
assert!(contains_runtime_event_associated_type_bound(&supertrait.unwrap()));
}

#[test]
fn contains_runtime_event_associated_type_bound_invalid_supertrait() {
let supertrait =
syn::parse2::<syn::Path>(quote::quote!(SystemConfig<RuntimeEvent: From<Event<Self>>>))
.unwrap();
assert!(!contains_runtime_event_associated_type_bound(&supertrait));
}

#[test]
fn contains_runtime_event_associated_type_bound_invalid_assoc_type_name() {
let supertrait =
syn::parse2::<syn::Path>(quote::quote!(Config<NonRuntimeEvent: From<Event<Self>>>))
.unwrap();
assert!(!contains_runtime_event_associated_type_bound(&supertrait));
}
#[test]
fn contains_runtime_event_associated_type_bound_invalid_trait_bound() {
let supertrait =
syn::parse2::<syn::Path>(quote::quote!(Config<RuntimeEvent: TryFrom<Event<Self>>>))
.unwrap();
assert!(!contains_runtime_event_associated_type_bound(&supertrait));
}
}
36 changes: 7 additions & 29 deletions substrate/frame/support/procedural/src/pallet/parse/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ pub struct Def {
pub frame_support: syn::Path,
pub dev_mode: bool,
pub view_functions: Option<view_functions::ViewFunctionsImplDef>,
pub is_frame_system: bool,
}

impl Def {
Expand Down Expand Up @@ -106,20 +107,23 @@ impl Def {
let mut type_values = vec![];
let mut composites: Vec<CompositeDef> = vec![];
let mut view_functions = None;
let mut is_frame_system = false;

for (index, item) in items.iter_mut().enumerate() {
let pallet_attr: Option<PalletAttr> = helper::take_first_item_pallet_attr(item)?;

match pallet_attr {
Some(PalletAttr::Config{ with_default, is_frame_system, without_automatic_metadata, ..}) if config.is_none() =>
Some(PalletAttr::Config{ with_default, is_frame_system: is_frame_system_val, without_automatic_metadata, ..}) if config.is_none() => {
is_frame_system = is_frame_system_val;
config = Some(config::ConfigDef::try_from(
&frame_system,
index,
item,
with_default,
without_automatic_metadata,
is_frame_system,
)?),
)?);
},
Some(PalletAttr::Pallet(span)) if pallet_struct.is_none() => {
let p = pallet_struct::PalletStructDef::try_from(span, index, item)?;
pallet_struct = Some(p);
Expand Down Expand Up @@ -258,10 +262,10 @@ impl Def {
frame_support,
dev_mode,
view_functions,
is_frame_system,
};

def.check_instance_usage()?;
def.check_event_usage()?;

Ok(def)
}
Expand Down Expand Up @@ -359,32 +363,6 @@ impl Def {
Ok(())
}

/// Check that usage of trait `Event` is consistent with the definition, i.e. it is declared
/// and trait defines type or type bound `RuntimeEvent`, or not declared and no trait associated
/// type.
fn check_event_usage(&self) -> syn::Result<()> {
match (self.config.has_event_type, self.config.has_event_bound, self.event.is_some()) {
(true, false, false) => {
let msg = "Invalid usage of RuntimeEvent, `Config` contains associated type `RuntimeEvent`, \
but enum `Event` is not declared (i.e. no use of `#[pallet::event]`). \
Note that type `RuntimeEvent` in trait is reserved to work alongside pallet event.";
Err(syn::Error::new(proc_macro2::Span::call_site(), msg))
},
(false, true, false) => {
let msg = "Invalid usage of `RuntimeEvent`, `frame_system::Config` contains associated type bound `RuntimeEvent`, \
but enum `Event` is not declared (i.e. no use of `#[pallet::event]`). \
Note that type associated type bound `RuntimeEvent` in trait is reserved to work alongside pallet event.";
Err(syn::Error::new(proc_macro2::Span::call_site(), msg))
},
(true, true, _) => {
let msg = "Invalid usage of RuntimeEvent, `Config` contains associated type `RuntimeEvent` and associated type bound `RuntimeEvent`. \
Only one of them should be used.";
Err(syn::Error::new(proc_macro2::Span::call_site(), msg))
},
_ => Ok(()),
}
}

/// Check that usage of trait `Config` is consistent with the definition, i.e. it is used with
/// instance iff it is defined with instance.
fn check_instance_usage(&self) -> syn::Result<()> {
Expand Down
Loading

0 comments on commit 85171bc

Please sign in to comment.