From 5647e719473673de93f1f317e64c80f90b731b93 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Sat, 10 Sep 2022 20:50:59 +0100 Subject: [PATCH] construct_runtime: Fix generation of types behind features (#12229) * construct_runtime: Fix generation of types behind features With the recent addition of supporting features in `construct_runtime!` there was a bug overseen. The `AllPalletsWithSystem` etc type declarations would be declared twice when a certain was enabled. The problem was that in the macro we didn't feature gate the types that should be declared when there is no feature enabled. This pull request now takes care of feature gating this type behind `all(#( not(feature) ))`. So, these types will only be enabled if no of the configured features is enabled. * Fix tests * FMT --- .../procedural/src/construct_runtime/mod.rs | 138 +++++++++--------- substrate/frame/support/test/Cargo.toml | 7 +- substrate/frame/support/test/tests/pallet.rs | 78 +++++++--- substrate/scripts/ci/gitlab/pipeline/test.yml | 2 +- 4 files changed, 133 insertions(+), 92 deletions(-) diff --git a/substrate/frame/support/procedural/src/construct_runtime/mod.rs b/substrate/frame/support/procedural/src/construct_runtime/mod.rs index cfd582d0e4..e20cb61b7a 100644 --- a/substrate/frame/support/procedural/src/construct_runtime/mod.rs +++ b/substrate/frame/support/procedural/src/construct_runtime/mod.rs @@ -156,10 +156,7 @@ use parse::{ use proc_macro::TokenStream; use proc_macro2::TokenStream as TokenStream2; use quote::quote; -use std::{ - collections::{HashMap, HashSet}, - str::FromStr, -}; +use std::{collections::HashSet, str::FromStr}; use syn::{Ident, Result}; /// The fixed name of the system pallet. @@ -324,11 +321,15 @@ fn decl_all_pallets<'a>( features: &HashSet<&str>, ) -> TokenStream2 { let mut types = TokenStream2::new(); - let mut names_by_feature = features + + // Every feature set to the pallet names that should be included by this feature set. + let mut features_to_names = features .iter() + .map(|f| *f) .powerset() - .map(|feat| (feat, Vec::new())) - .collect::>(); + .map(|feat| (HashSet::from_iter(feat), Vec::new())) + .collect::, Vec<_>)>>(); + for pallet_declaration in pallet_declarations { let type_name = &pallet_declaration.name; let pallet = &pallet_declaration.path; @@ -346,21 +347,22 @@ fn decl_all_pallets<'a>( types.extend(type_decl); if pallet_declaration.cfg_pattern.is_empty() { - for names in names_by_feature.values_mut() { + for (_, names) in features_to_names.iter_mut() { names.push(&pallet_declaration.name); } } else { - for (feature_set, names) in &mut names_by_feature { + for (feature_set, names) in &mut features_to_names { // Rust tidbit: if we have multiple `#[cfg]` feature on the same item, then the // predicates listed in all `#[cfg]` attributes are effectively joined by `and()`, // meaning that all of them must match in order to activate the item let is_feature_active = pallet_declaration.cfg_pattern.iter().all(|expr| { expr.eval(|pred| match pred { - Predicate::Feature(f) => feature_set.contains(&f), - Predicate::Test => feature_set.contains(&&"test"), + Predicate::Feature(f) => feature_set.contains(f), + Predicate::Test => feature_set.contains(&"test"), _ => false, }) }); + if is_feature_active { names.push(&pallet_declaration.name); } @@ -368,11 +370,34 @@ fn decl_all_pallets<'a>( } } - let all_pallets_without_system = names_by_feature.iter().map(|(feature_set, names)| { - let mut feature_set = feature_set.iter().collect::>(); - let test_cfg = feature_set.remove(&&&"test").then_some(quote!(test)).into_iter(); - let feature_set = feature_set.into_iter(); - let attr = quote!(#[cfg(all( #(#test_cfg),* #(feature = #feature_set),* ))]); + // All possible features. This will be used below for the empty feature set. + let mut all_features = features_to_names + .iter() + .flat_map(|f| f.0.iter().cloned()) + .collect::>(); + let attribute_to_names = features_to_names + .into_iter() + .map(|(mut features, names)| { + // If this is the empty feature set, it needs to be changed to negate all available + // features. So, we ensure that there is some type declared when all features are not + // enabled. + if features.is_empty() { + let test_cfg = all_features.remove("test").then_some(quote!(test)).into_iter(); + let features = all_features.iter(); + let attr = quote!(#[cfg(all( #(not(#test_cfg)),* #(not(feature = #features)),* ))]); + + (attr, names) + } else { + let test_cfg = features.remove("test").then_some(quote!(test)).into_iter(); + let features = features.iter(); + let attr = quote!(#[cfg(all( #(#test_cfg),* #(feature = #features),* ))]); + + (attr, names) + } + }) + .collect::>(); + + let all_pallets_without_system = attribute_to_names.iter().map(|(attr, names)| { let names = names.iter().filter(|n| **n != SYSTEM_PALLET_NAME); quote! { #attr @@ -382,11 +407,7 @@ fn decl_all_pallets<'a>( } }); - let all_pallets_with_system = names_by_feature.iter().map(|(feature_set, names)| { - let mut feature_set = feature_set.iter().collect::>(); - let test_cfg = feature_set.remove(&&&"test").then_some(quote!(test)).into_iter(); - let feature_set = feature_set.into_iter(); - let attr = quote!(#[cfg(all( #(#test_cfg),* #(feature = #feature_set),* ))]); + let all_pallets_with_system = attribute_to_names.iter().map(|(attr, names)| { quote! { #attr /// All pallets included in the runtime as a nested tuple of types. @@ -394,28 +415,19 @@ fn decl_all_pallets<'a>( } }); - let all_pallets_without_system_reversed = - names_by_feature.iter().map(|(feature_set, names)| { - let mut feature_set = feature_set.iter().collect::>(); - let test_cfg = feature_set.remove(&&&"test").then_some(quote!(test)).into_iter(); - let feature_set = feature_set.into_iter(); - let attr = quote!(#[cfg(all( #(#test_cfg),* #(feature = #feature_set),* ))]); - let names = names.iter().filter(|n| **n != SYSTEM_PALLET_NAME).rev(); - quote! { - #attr - /// All pallets included in the runtime as a nested tuple of types in reversed order. - /// Excludes the System pallet. - #[deprecated(note = "Using reverse pallet orders is deprecated. use only \ - `AllPalletWithSystem or AllPalletsWithoutSystem`")] - pub type AllPalletsWithoutSystemReversed = ( #(#names,)* ); - } - }); + let all_pallets_without_system_reversed = attribute_to_names.iter().map(|(attr, names)| { + let names = names.iter().filter(|n| **n != SYSTEM_PALLET_NAME).rev(); + quote! { + #attr + /// All pallets included in the runtime as a nested tuple of types in reversed order. + /// Excludes the System pallet. + #[deprecated(note = "Using reverse pallet orders is deprecated. use only \ + `AllPalletWithSystem or AllPalletsWithoutSystem`")] + pub type AllPalletsWithoutSystemReversed = ( #(#names,)* ); + } + }); - let all_pallets_with_system_reversed = names_by_feature.iter().map(|(feature_set, names)| { - let mut feature_set = feature_set.iter().collect::>(); - let test_cfg = feature_set.remove(&&&"test").then_some(quote!(test)).into_iter(); - let feature_set = feature_set.into_iter(); - let attr = quote!(#[cfg(all( #(#test_cfg),* #(feature = #feature_set),* ))]); + let all_pallets_with_system_reversed = attribute_to_names.iter().map(|(attr, names)| { let names = names.iter().rev(); quote! { #attr @@ -426,35 +438,19 @@ fn decl_all_pallets<'a>( } }); - let system_pallet = - match names_by_feature[&Vec::new()].iter().find(|n| **n == SYSTEM_PALLET_NAME) { - Some(name) => name, - None => - return syn::Error::new( - proc_macro2::Span::call_site(), - "`System` pallet declaration is missing. \ - Please add this line: `System: frame_system,`", - ) - .into_compile_error(), - }; - - let all_pallets_reversed_with_system_first = - names_by_feature.iter().map(|(feature_set, names)| { - let mut feature_set = feature_set.iter().collect::>(); - let test_cfg = feature_set.remove(&&&"test").then_some(quote!(test)).into_iter(); - let feature_set = feature_set.into_iter(); - let attr = quote!(#[cfg(all( #(#test_cfg),* #(feature = #feature_set),* ))]); - let names = std::iter::once(system_pallet) - .chain(names.iter().rev().filter(|n| **n != SYSTEM_PALLET_NAME)); - quote! { - #attr - /// All pallets included in the runtime as a nested tuple of types in reversed order. - /// With the system pallet first. - #[deprecated(note = "Using reverse pallet orders is deprecated. use only \ - `AllPalletWithSystem or AllPalletsWithoutSystem`")] - pub type AllPalletsReversedWithSystemFirst = ( #(#names,)* ); - } - }); + let all_pallets_reversed_with_system_first = attribute_to_names.iter().map(|(attr, names)| { + let system = quote::format_ident!("{}", SYSTEM_PALLET_NAME); + let names = std::iter::once(&system) + .chain(names.iter().rev().filter(|n| **n != SYSTEM_PALLET_NAME).cloned()); + quote! { + #attr + /// All pallets included in the runtime as a nested tuple of types in reversed order. + /// With the system pallet first. + #[deprecated(note = "Using reverse pallet orders is deprecated. use only \ + `AllPalletWithSystem or AllPalletsWithoutSystem`")] + pub type AllPalletsReversedWithSystemFirst = ( #(#names,)* ); + } + }); quote!( #types diff --git a/substrate/frame/support/test/Cargo.toml b/substrate/frame/support/test/Cargo.toml index dd23d7e6b0..d7d3bfc98f 100644 --- a/substrate/frame/support/test/Cargo.toml +++ b/substrate/frame/support/test/Cargo.toml @@ -47,9 +47,10 @@ std = [ "sp-version/std", ] try-runtime = ["frame-support/try-runtime"] -# WARNING: CI only execute pallet test with this feature, -# if the feature intended to be used outside, CI and this message need to be updated. -conditional-storage = [] +# WARNING: +# Only CI runs with this feature enabled. This feature is for testing stuff related to the FRAME macros +# in conjunction with rust features. +frame-feature-testing = [] # Disable ui tests disable-ui-tests = [] no-metadata-docs = ["frame-support/no-metadata-docs"] diff --git a/substrate/frame/support/test/tests/pallet.rs b/substrate/frame/support/test/tests/pallet.rs index 907a52f118..d683399482 100644 --- a/substrate/frame/support/test/tests/pallet.rs +++ b/substrate/frame/support/test/tests/pallet.rs @@ -334,22 +334,22 @@ pub mod pallet { #[pallet::storage] #[pallet::getter(fn conditional_value)] - #[cfg(feature = "conditional-storage")] + #[cfg(feature = "frame-feature-testing")] pub type ConditionalValue = StorageValue<_, u32>; - #[cfg(feature = "conditional-storage")] + #[cfg(feature = "frame-feature-testing")] #[pallet::storage] #[pallet::getter(fn conditional_map)] pub type ConditionalMap = StorageMap<_, Twox64Concat, u16, u32, OptionQuery, GetDefault, ConstU32<12>>; - #[cfg(feature = "conditional-storage")] + #[cfg(feature = "frame-feature-testing")] #[pallet::storage] #[pallet::getter(fn conditional_double_map)] pub type ConditionalDoubleMap = StorageDoubleMap<_, Blake2_128Concat, u8, Twox64Concat, u16, u32>; - #[cfg(feature = "conditional-storage")] + #[cfg(feature = "frame-feature-testing")] #[pallet::storage] #[pallet::getter(fn conditional_nmap)] pub type ConditionalNMap = @@ -550,7 +550,9 @@ pub mod pallet2 { #[frame_support::pallet] pub mod pallet3 { #[pallet::config] - pub trait Config: frame_system::Config {} + pub trait Config: frame_system::Config::Origin> { + type Origin; + } #[pallet::pallet] pub struct Pallet(_); @@ -612,6 +614,11 @@ impl pallet2::Config for Runtime { impl pallet4::Config for Runtime {} +#[cfg(feature = "frame-feature-testing")] +impl pallet3::Config for Runtime { + type Origin = Origin; +} + pub type Header = sp_runtime::generic::Header; pub type Block = sp_runtime::generic::Block; pub type UncheckedExtrinsic = sp_runtime::generic::UncheckedExtrinsic; @@ -626,7 +633,7 @@ frame_support::construct_runtime!( System: frame_system exclude_parts { Pallet, Storage }, Example: pallet, Example2: pallet2 exclude_parts { Call }, - #[cfg(feature = "example3")] + #[cfg(feature = "frame-feature-testing")] Example3: pallet3, Example4: pallet4 use_parts { Call }, } @@ -1024,7 +1031,7 @@ fn storage_expand() { Err(pallet::Error::::NonExistentStorageValue), ); - #[cfg(feature = "conditional-storage")] + #[cfg(feature = "frame-feature-testing")] { pallet::ConditionalValue::::put(1); pallet::ConditionalMap::::insert(1, 2); @@ -1164,8 +1171,10 @@ fn migrate_from_pallet_version_to_storage_version() { AllPalletsWithSystem, >(&db_weight); - // 4 pallets, 2 writes and every write costs 5 weight. - assert_eq!(Weight::from_ref_time(4 * 2 * 5), weight); + let pallet_num = if cfg!(feature = "frame-feature-testing") { 5 } else { 4 }; + + // `pallet_num` pallets, 2 writes and every write costs 5 weight. + assert_eq!(Weight::from_ref_time(pallet_num * 2 * 5), weight); // All pallet versions should be removed assert!(sp_io::storage::get(&pallet_version_key(Example::name())).is_none()); @@ -1332,7 +1341,7 @@ fn metadata() { default: vec![1, 1], docs: vec![], }, - #[cfg(feature = "conditional-storage")] + #[cfg(feature = "frame-feature-testing")] StorageEntryMetadata { name: "ConditionalValue", modifier: StorageEntryModifier::Optional, @@ -1340,7 +1349,7 @@ fn metadata() { default: vec![0], docs: vec![], }, - #[cfg(feature = "conditional-storage")] + #[cfg(feature = "frame-feature-testing")] StorageEntryMetadata { name: "ConditionalMap", modifier: StorageEntryModifier::Optional, @@ -1352,7 +1361,7 @@ fn metadata() { default: vec![0], docs: vec![], }, - #[cfg(feature = "conditional-storage")] + #[cfg(feature = "frame-feature-testing")] StorageEntryMetadata { name: "ConditionalDoubleMap", modifier: StorageEntryModifier::Optional, @@ -1367,7 +1376,7 @@ fn metadata() { default: vec![0], docs: vec![], }, - #[cfg(feature = "conditional-storage")] + #[cfg(feature = "frame-feature-testing")] StorageEntryMetadata { name: "ConditionalNMap", modifier: StorageEntryModifier::Optional, @@ -1489,6 +1498,16 @@ fn metadata() { constants: vec![], error: None, }, + #[cfg(feature = "frame-feature-testing")] + PalletMetadata { + index: 3, + name: "Example3", + storage: None, + calls: None, + event: None, + constants: vec![], + error: None, + }, ]; let empty_doc = pallets[0].event.as_ref().unwrap().ty.type_info().docs().is_empty() && @@ -1633,7 +1652,7 @@ fn test_storage_info() { max_values: None, max_size: Some(16 + 1 + 8 + 2 + 16), }, - #[cfg(feature = "conditional-storage")] + #[cfg(feature = "frame-feature-testing")] { StorageInfo { pallet_name: b"Example".to_vec(), @@ -1643,7 +1662,7 @@ fn test_storage_info() { max_size: Some(4), } }, - #[cfg(feature = "conditional-storage")] + #[cfg(feature = "frame-feature-testing")] { StorageInfo { pallet_name: b"Example".to_vec(), @@ -1653,7 +1672,7 @@ fn test_storage_info() { max_size: Some(8 + 2 + 4), } }, - #[cfg(feature = "conditional-storage")] + #[cfg(feature = "frame-feature-testing")] { StorageInfo { pallet_name: b"Example".to_vec(), @@ -1663,7 +1682,7 @@ fn test_storage_info() { max_size: Some(16 + 1 + 8 + 2 + 4), } }, - #[cfg(feature = "conditional-storage")] + #[cfg(feature = "frame-feature-testing")] { StorageInfo { pallet_name: b"Example".to_vec(), @@ -1730,27 +1749,42 @@ fn assert_type_all_pallets_reversed_with_system_first_is_correct() { // Just ensure the 2 types are same. #[allow(deprecated)] fn _a(_t: AllPalletsReversedWithSystemFirst) {} + #[cfg(not(feature = "frame-feature-testing"))] fn _b(t: (System, Example4, Example2, Example)) { _a(t) } + #[cfg(feature = "frame-feature-testing")] + fn _b(t: (System, Example4, Example3, Example2, Example)) { + _a(t) + } } #[test] fn assert_type_all_pallets_with_system_is_correct() { // Just ensure the 2 types are same. fn _a(_t: AllPalletsWithSystem) {} + #[cfg(not(feature = "frame-feature-testing"))] fn _b(t: (System, Example, Example2, Example4)) { _a(t) } + #[cfg(feature = "frame-feature-testing")] + fn _b(t: (System, Example, Example2, Example3, Example4)) { + _a(t) + } } #[test] fn assert_type_all_pallets_without_system_is_correct() { // Just ensure the 2 types are same. fn _a(_t: AllPalletsWithoutSystem) {} + #[cfg(not(feature = "frame-feature-testing"))] fn _b(t: (Example, Example2, Example4)) { _a(t) } + #[cfg(feature = "frame-feature-testing")] + fn _b(t: (Example, Example2, Example3, Example4)) { + _a(t) + } } #[test] @@ -1758,9 +1792,14 @@ fn assert_type_all_pallets_with_system_reversed_is_correct() { // Just ensure the 2 types are same. #[allow(deprecated)] fn _a(_t: AllPalletsWithSystemReversed) {} + #[cfg(not(feature = "frame-feature-testing"))] fn _b(t: (Example4, Example2, Example, System)) { _a(t) } + #[cfg(feature = "frame-feature-testing")] + fn _b(t: (Example4, Example3, Example2, Example, System)) { + _a(t) + } } #[test] @@ -1768,9 +1807,14 @@ fn assert_type_all_pallets_without_system_reversed_is_correct() { // Just ensure the 2 types are same. #[allow(deprecated)] fn _a(_t: AllPalletsWithoutSystemReversed) {} + #[cfg(not(feature = "frame-feature-testing"))] fn _b(t: (Example4, Example2, Example)) { _a(t) } + #[cfg(feature = "frame-feature-testing")] + fn _b(t: (Example4, Example3, Example2, Example)) { + _a(t) + } } #[test] diff --git a/substrate/scripts/ci/gitlab/pipeline/test.yml b/substrate/scripts/ci/gitlab/pipeline/test.yml index 8036e2cec2..50916a37fc 100644 --- a/substrate/scripts/ci/gitlab/pipeline/test.yml +++ b/substrate/scripts/ci/gitlab/pipeline/test.yml @@ -251,7 +251,7 @@ test-frame-support: RUN_UI_TESTS: 1 script: - rusty-cachier snapshot create - - time cargo test -p frame-support-test --features=conditional-storage,no-metadata-docs --manifest-path ./frame/support/test/Cargo.toml --test pallet # does not reuse cache 1 min 44 sec + - time cargo test -p frame-support-test --features=frame-feature-testing,no-metadata-docs --manifest-path ./frame/support/test/Cargo.toml --test pallet # does not reuse cache 1 min 44 sec - SUBSTRATE_TEST_TIMEOUT=1 time cargo test -p substrate-test-utils --release --verbose --locked -- --ignored timeout - rusty-cachier cache upload