Remove default expansion from construct_runtime! (#4937)

This removes the following syntactic sugar from `construct_runtime!`:

- Expansion of `default` to the default set of module parts
- Expansion of `System: system` to the default set of module parts

The macro now requires the user to provide all the module parts of a pallet.

substrate/bin/node-template/runtime/src/lib.rs

@@ -256,7 +256,7 @@ construct_runtime!(
 		Indices: indices::{Module, Call, Storage, Event<T>, Config<T>},
 		Balances: balances::{Module, Call, Storage, Config<T>, Event<T>},
 		TransactionPayment: transaction_payment::{Module, Storage},
-		Sudo: sudo,
+		Sudo: sudo::{Module, Call, Config<T>, Storage, Event<T>},
 		// Used for the module template in `./template.rs`
 		TemplateModule: template::{Module, Call, Storage, Event<T>},
 		RandomnessCollectiveFlip: randomness_collective_flip::{Module, Call, Storage},

substrate/bin/node/runtime/src/lib.rs

@@ -608,7 +608,7 @@ construct_runtime!(
 		Indices: pallet_indices::{Module, Call, Storage, Config<T>, Event<T>},
 		Balances: pallet_balances::{Module, Call, Storage, Config<T>, Event<T>},
 		TransactionPayment: pallet_transaction_payment::{Module, Storage},
-		Staking: pallet_staking,
+		Staking: pallet_staking::{Module, Call, Config<T>, Storage, Event<T>},
 		Session: pallet_session::{Module, Call, Storage, Event, Config<T>},
 		Democracy: pallet_democracy::{Module, Call, Storage, Config, Event<T>},
 		Council: pallet_collective::<Instance1>::{Module, Call, Storage, Origin<T>, Event<T>, Config<T>},
@@ -618,8 +618,8 @@ construct_runtime!(
 		FinalityTracker: pallet_finality_tracker::{Module, Call, Inherent},
 		Grandpa: pallet_grandpa::{Module, Call, Storage, Config, Event},
 		Treasury: pallet_treasury::{Module, Call, Storage, Config, Event<T>},
-		Contracts: pallet_contracts,
-		Sudo: pallet_sudo,
+		Contracts: pallet_contracts::{Module, Call, Config<T>, Storage, Event<T>},
+		Sudo: pallet_sudo::{Module, Call, Config<T>, Storage, Event<T>},
 		ImOnline: pallet_im_online::{Module, Call, Storage, Event<T>, ValidateUnsigned, Config<T>},
 		AuthorityDiscovery: pallet_authority_discovery::{Module, Call, Config},
 		Offences: pallet_offences::{Module, Call, Storage, Event},

substrate/frame/support/procedural/src/construct_runtime/mod.rs

@@ -217,8 +217,8 @@ fn decl_runtime_metadata<'a>(
 				let filtered_names: Vec<_> = module_declaration
 					.module_parts()
 					.into_iter()
-					.filter(|part| part.name != "Module")
-					.map(|part| part.name.clone())
+					.filter(|part| part.name() != "Module")
+					.map(|part| part.ident())
 					.collect();
 				(module_declaration, filtered_names)
 			})

substrate/frame/support/procedural/src/construct_runtime/parse.rs

@@ -27,6 +27,14 @@ mod keyword {
 	syn::custom_keyword!(Block);
 	syn::custom_keyword!(NodeBlock);
 	syn::custom_keyword!(UncheckedExtrinsic);
+	syn::custom_keyword!(Module);
+	syn::custom_keyword!(Call);
+	syn::custom_keyword!(Storage);
+	syn::custom_keyword!(Event);
+	syn::custom_keyword!(Config);
+	syn::custom_keyword!(Origin);
+	syn::custom_keyword!(Inherent);
+	syn::custom_keyword!(ValidateUnsigned);
 }
 
 #[derive(Debug)]
@@ -145,7 +153,7 @@ pub struct ModuleDeclaration {
 	pub name: Ident,
 	pub module: Ident,
 	pub instance: Option<Ident>,
-	pub details: Option<ext::Braces<ext::Punctuated<ModuleEntry, Token![,]>>>,
+	pub module_parts: Vec<ModulePart>,
 }
 
 impl Parse for ModuleDeclaration {
@@ -162,160 +170,183 @@ impl Parse for ModuleDeclaration {
 		} else {
 			None
 		};
-		let details = if input.peek(Token![::]) {
-			let _: Token![::] = input.parse()?;
-			Some(input.parse()?)
-		} else {
-			None
-		};
+
+		let _: Token![::] = input.parse()?;
+		let module_parts = parse_module_parts(input)?;
+
 		let parsed = Self {
 			name,
 			module,
 			instance,
-			details,
+			module_parts,
 		};
-		if let Some(ref details) = parsed.details {
-			let parts = &details.content.inner;
-			let mut resolved = HashSet::new();
-			let has_default = parts.into_iter().any(|m| m.is_default());
-			for entry in parts {
-				match entry {
-					ModuleEntry::Part(part) => {
-						if has_default && part.is_included_in_default() {
-							let msg = format!(
-									"`{}` is already included in `default`. Either remove `default` or remove `{}`",
-									part.name,
-									part.name
-								);
-							return Err(Error::new(part.name.span(), msg));
-						}
 
-						if !resolved.insert(part.name.clone()) {
-							let msg = format!(
-								"`{}` was already declared before. Please remove the duplicate declaration",
-								part.name
-							);
-							return Err(Error::new(part.name.span(), msg));
-						}
-					}
-					_ => {}
-				}
-			}
-		}
 		Ok(parsed)
 	}
 }
 
 impl ModuleDeclaration {
-	/// Get resolved module parts, i.e. after expanding `default` keyword
-	/// or empty declaration
-	pub fn module_parts(&self) -> Vec<ModulePart> {
-		if let Some(ref details) = self.details {
-			details
-				.content
-				.inner
-				.iter()
-				.flat_map(|entry| match entry {
-					ModuleEntry::Default(ref token) => Self::default_modules(token.span()),
-					ModuleEntry::Part(ref part) => vec![part.clone()],
-				})
-				.collect()
-		} else {
-			Self::default_modules(self.module.span())
-		}
+	/// Get resolved module parts
+	pub fn module_parts(&self) -> &[ModulePart] {
+		&self.module_parts
 	}
 
-	pub fn find_part(&self, name: &str) -> Option<ModulePart> {
-		self.module_parts()
-			.into_iter()
-			.find(|part| part.name == name)
+	pub fn find_part(&self, name: &str) -> Option<&ModulePart> {
+		self.module_parts.iter().find(|part| part.name() == name)
 	}
 
 	pub fn exists_part(&self, name: &str) -> bool {
 		self.find_part(name).is_some()
 	}
+}
 
-	fn default_modules(span: Span) -> Vec<ModulePart> {
-		let mut res: Vec<_> = ["Module", "Call", "Storage"]
-			.iter()
-			.map(|name| ModulePart::with_name(name, span))
-			.collect();
-		res.extend(
-			["Event", "Config"]
-				.iter()
-				.map(|name| ModulePart::with_generics(name, span)),
-		);
-		res
+/// Parse [`ModulePart`]'s from a braces enclosed list that is split by commas, e.g.
+///
+/// `{ Call, Event }`
+fn parse_module_parts(input: ParseStream) -> Result<Vec<ModulePart>> {
+	let module_parts :ext::Braces<ext::Punctuated<ModulePart, Token![,]>> = input.parse()?;
+
+	let mut resolved = HashSet::new();
+	for part in module_parts.content.inner.iter() {
+		if !resolved.insert(part.name()) {
+			let msg = format!(
+				"`{}` was already declared before. Please remove the duplicate declaration",
+				part.name(),
+			);
+			return Err(Error::new(part.keyword.span(), msg));
+		}
 	}
+
+	Ok(module_parts.content.inner.into_iter().collect())
 }
 
-#[derive(Debug)]
-pub enum ModuleEntry {
-	Default(Token![default]),
-	Part(ModulePart),
+#[derive(Debug, Clone)]
+pub enum ModulePartKeyword {
+	Module(keyword::Module),
+	Call(keyword::Call),
+	Storage(keyword::Storage),
+	Event(keyword::Event),
+	Config(keyword::Config),
+	Origin(keyword::Origin),
+	Inherent(keyword::Inherent),
+	ValidateUnsigned(keyword::ValidateUnsigned),
 }
 
-impl Parse for ModuleEntry {
+impl Parse for ModulePartKeyword {
 	fn parse(input: ParseStream) -> Result<Self> {
 		let lookahead = input.lookahead1();
-		if lookahead.peek(Token![default]) {
-			Ok(ModuleEntry::Default(input.parse()?))
-		} else if lookahead.peek(Ident) {
-			Ok(ModuleEntry::Part(input.parse()?))
+
+		if lookahead.peek(keyword::Module) {
+			Ok(Self::Module(input.parse()?))
+		} else if lookahead.peek(keyword::Call) {
+			Ok(Self::Call(input.parse()?))
+		} else if lookahead.peek(keyword::Storage) {
+			Ok(Self::Storage(input.parse()?))
+		} else if lookahead.peek(keyword::Event) {
+			Ok(Self::Event(input.parse()?))
+		} else if lookahead.peek(keyword::Config) {
+			Ok(Self::Config(input.parse()?))
+		} else if lookahead.peek(keyword::Origin) {
+			Ok(Self::Origin(input.parse()?))
+		} else if lookahead.peek(keyword::Inherent) {
+			Ok(Self::Inherent(input.parse()?))
+		} else if lookahead.peek(keyword::ValidateUnsigned) {
+			Ok(Self::ValidateUnsigned(input.parse()?))
 		} else {
 			Err(lookahead.error())
 		}
 	}
 }
 
-impl ModuleEntry {
-	pub fn is_default(&self) -> bool {
+impl ModulePartKeyword {
+	/// Returns the name of `Self`.
+	fn name(&self) -> &'static str {
 		match self {
-			ModuleEntry::Default(_) => true,
-			_ => false,
+			Self::Module(_) => "Module",
+			Self::Call(_) => "Call",
+			Self::Storage(_) => "Storage",
+			Self::Event(_) => "Event",
+			Self::Config(_) => "Config",
+			Self::Origin(_) => "Origin",
+			Self::Inherent(_) => "Inherent",
+			Self::ValidateUnsigned(_) => "ValidateUnsigned",
+		}
+	}
+
+	/// Returns the name as `Ident`.
+	fn ident(&self) -> Ident {
+		Ident::new(self.name(), self.span())
+	}
+
+	/// Returns `true` if this module part allows to have an argument.
+	///
+	/// For example `Inherent(Timestamp)`.
+	fn allows_arg(&self) -> bool {
+		Self::all_allow_arg().iter().any(|n| *n == self.name())
+	}
+
+	/// Returns the names of all module parts that allow to have an argument.
+	fn all_allow_arg() -> &'static [&'static str] {
+		&["Inherent"]
+	}
+
+	/// Returns `true` if this module part is allowed to have generic arguments.
+	fn allows_generic(&self) -> bool {
+		Self::all_generic_arg().iter().any(|n| *n == self.name())
+	}
+
+	/// Returns the names of all module parts that allow to have a generic argument.
+	fn all_generic_arg() -> &'static [&'static str] {
+		&["Event", "Origin", "Config"]
+	}
+}
+
+impl Spanned for ModulePartKeyword {
+	fn span(&self) -> Span {
+		match self {
+			Self::Module(inner) => inner.span(),
+			Self::Call(inner) => inner.span(),
+			Self::Storage(inner) => inner.span(),
+			Self::Event(inner) => inner.span(),
+			Self::Config(inner) => inner.span(),
+			Self::Origin(inner) => inner.span(),
+			Self::Inherent(inner) => inner.span(),
+			Self::ValidateUnsigned(inner) => inner.span(),
 		}
 	}
 }
 
 #[derive(Debug, Clone)]
 pub struct ModulePart {
-	pub name: Ident,
+	pub keyword: ModulePartKeyword,
 	pub generics: syn::Generics,
 	pub args: Option<ext::Parens<ext::Punctuated<Ident, Token![,]>>>,
 }
 
 impl Parse for ModulePart {
 	fn parse(input: ParseStream) -> Result<Self> {
-		let name: Ident = input.parse()?;
-
-		if !ModulePart::all_allowed().iter().any(|n| name == n) {
-			return Err(syn::Error::new(
-				name.span(),
-				format!(
-					"Only the following modules are allowed: {}",
-					ModulePart::format_names(ModulePart::all_allowed()),
-				),
-			))
-		}
+		let keyword: ModulePartKeyword = input.parse()?;
 
 		let generics: syn::Generics = input.parse()?;
-		if !generics.params.is_empty() && !Self::is_allowed_generic(&name) {
-			let valid_generics = ModulePart::format_names(ModulePart::allowed_generics());
+		if !generics.params.is_empty() && !keyword.allows_generic() {
+			let valid_generics = ModulePart::format_names(ModulePartKeyword::all_generic_arg());
 			let msg = format!(
 				"`{}` is not allowed to have generics. \
 				 Only the following modules are allowed to have generics: {}.",
-				name, valid_generics
+				keyword.name(),
+				valid_generics,
 			);
-			return Err(syn::Error::new(name.span(), msg));
+			return Err(syn::Error::new(keyword.span(), msg));
 		}
 		let args = if input.peek(token::Paren) {
-			if !Self::is_allowed_arg(&name) {
+			if !keyword.allows_arg() {
 				let syn::group::Parens { token: parens, .. } = syn::group::parse_parens(input)?;
-				let valid_names = ModulePart::format_names(ModulePart::allowed_args());
+				let valid_names = ModulePart::format_names(ModulePartKeyword::all_allow_arg());
 				let msg = format!(
 					"`{}` is not allowed to have arguments in parens. \
 					 Only the following modules are allowed to have arguments in parens: {}.",
-					name, valid_names
+					keyword.name(),
+					valid_names,
 				);
 				return Err(syn::Error::new(parens.span, msg));
 			}
@@ -325,7 +356,7 @@ impl Parse for ModulePart {
 		};
 
 		Ok(Self {
-			name,
+			keyword,
 			generics,
 			args,
 		})
@@ -333,70 +364,19 @@ impl Parse for ModulePart {
 }
 
 impl ModulePart {
-	pub fn is_allowed_generic(ident: &Ident) -> bool {
-		Self::allowed_generics().into_iter().any(|n| ident == n)
-	}
-
-	pub fn is_allowed_arg(ident: &Ident) -> bool {
-		Self::allowed_args().into_iter().any(|n| ident == n)
-	}
-
-	pub fn allowed_generics() -> &'static [&'static str] {
-		&["Event", "Origin", "Config"]
-	}
-
-	pub fn allowed_args() -> &'static [&'static str] {
-		&["Inherent"]
-	}
-
-	/// Returns all allowed names for module parts.
-	pub fn all_allowed() -> &'static [&'static str] {
-		&["Module", "Call", "Storage", "Event", "Config", "Origin", "Inherent", "ValidateUnsigned"]
-	}
-
 	pub fn format_names(names: &[&'static str]) -> String {
 		let res: Vec<_> = names.into_iter().map(|s| format!("`{}`", s)).collect();
 		res.join(", ")
 	}
 
-	pub fn is_included_in_default(&self) -> bool {
-		["Module", "Call", "Storage", "Event", "Config"]
-			.iter()
-			.any(|name| self.name == name)
+	/// The name of this module part.
+	pub fn name(&self) -> &'static str {
+		self.keyword.name()
 	}
 
-	/// Plain module name like `Event` or `Call`, etc.
-	pub fn with_name(name: &str, span: Span) -> Self {
-		let name = Ident::new(name, span);
-		Self {
-			name,
-			generics: syn::Generics {
-				lt_token: None,
-				gt_token: None,
-				where_clause: None,
-				..Default::default()
-			},
-			args: None,
-		}
-	}
-
-	/// Module name with generic like `Event<T>` or `Call<T>`, etc.
-	pub fn with_generics(name: &str, span: Span) -> Self {
-		let name = Ident::new(name, span);
-		let typ = Ident::new("T", span);
-		let generic_param = syn::GenericParam::Type(typ.into());
-		let generic_params = vec![generic_param].into_iter().collect();
-		let generics = syn::Generics {
-			lt_token: Some(syn::token::Lt { spans: [span] }),
-			params: generic_params,
-			gt_token: Some(syn::token::Gt { spans: [span] }),
-			where_clause: None,
-		};
-		Self {
-			name,
-			generics,
-			args: None,
-		}
+	/// The name of this module part as `Ident`.
+	pub fn ident(&self) -> Ident {
+		self.keyword.ident()
 	}
 }
 

substrate/frame/support/procedural/src/lib.rs

@@ -268,8 +268,8 @@ pub fn decl_storage(input: TokenStream) -> TokenStream {
 ///         NodeBlock = runtime::Block,
 ///         UncheckedExtrinsic = UncheckedExtrinsic
 ///     {
-///         System: system,
-///         Test: test::{default},
+///         System: system::{Module, Call, Event<T>, Config<T>},
+///         Test: test::{Module, Call},
 ///         Test2: test_with_long_module::{Module},
 ///
 ///         // Module with instances
@@ -279,17 +279,12 @@ pub fn decl_storage(input: TokenStream) -> TokenStream {
 /// )
 /// ```
 ///
-/// The module `System: system` will expand to `System: system::{Module, Call, Storage, Event<T>, Config<T>}`.
-/// The identifier `System` is the name of the module and the lower case identifier `system` is the
-/// name of the Rust module/crate for this Substrate module.
+/// The identifier `System` is the name of the pallet and the lower case identifier `system` is the
+/// name of the Rust module/crate for this Substrate module. The identifiers between the braces are
+/// the module parts provided by the pallet. It is important to list these parts here to export
+/// them correctly in the metadata or to make the pallet usable in the runtime.
 ///
-/// The module `Test: test::{default}` will expand to
-/// `Test: test::{Module, Call, Storage, Event<T>, Config<T>}`.
-///
-/// The module `Test2: test_with_long_module::{Module}` will expand to
-/// `Test2: test_with_long_module::{Module}`.
-///
-/// We provide support for the following types in a module:
+/// We provide support for the following module parts in a pallet:
 ///
 /// - `Module`
 /// - `Call`

substrate/frame/support/test/tests/construct_runtime_ui/default_module_invalid_arg.rs

@@ -1,14 +0,0 @@
-use frame_support::construct_runtime;
-
-construct_runtime! {
-	pub enum Runtime where
-		Block = Block,
-		NodeBlock = Block,
-		UncheckedExtrinsic = UncheckedExtrinsic
-	{
-		System: system,
-		Balance: balances::{default, Error},
-	}
-}
-
-fn main() {}

substrate/frame/support/test/tests/construct_runtime_ui/default_module_invalid_arg.stderr

@@ -1,5 +0,0 @@
-error: Only the following modules are allowed: `Module`, `Call`, `Storage`, `Event`, `Config`, `Origin`, `Inherent`, `ValidateUnsigned`
-  --> $DIR/default_module_invalid_arg.rs:10:32
-   |
-10 |         Balance: balances::{default, Error},
-   |                                      ^^^^^

substrate/frame/support/test/tests/construct_runtime_ui/double_module_parts.rs

@@ -6,7 +6,7 @@ construct_runtime! {
 		NodeBlock = Block,
 		UncheckedExtrinsic = UncheckedExtrinsic
 	{
-		System: system,
+		System: system::{Module},
 		Balance: balances::{Config, Call, Config<T>, Origin<T>},
 	}
 }

substrate/frame/support/test/tests/construct_runtime_ui/double_module_parts_default.rs

@@ -1,14 +0,0 @@
-use frame_support::construct_runtime;
-
-construct_runtime! {
-	pub enum Runtime where
-		Block = Block,
-		NodeBlock = Block,
-		UncheckedExtrinsic = UncheckedExtrinsic
-	{
-		System: system,
-		Balance: balances::{default, Config},
-	}
-}
-
-fn main() {}

substrate/frame/support/test/tests/construct_runtime_ui/double_module_parts_default.stderr

@@ -1,5 +0,0 @@
-error: `Config` is already included in `default`. Either remove `default` or remove `Config`
-  --> $DIR/double_module_parts_default.rs:10:32
-   |
-10 |         Balance: balances::{default, Config},
-   |                                      ^^^^^^

substrate/frame/support/test/tests/construct_runtime_ui/generics_in_invalid_module.rs

@@ -6,7 +6,7 @@ construct_runtime! {
 		NodeBlock = Block,
 		UncheckedExtrinsic = UncheckedExtrinsic
 	{
-		System: system,
+		System: system::{Module},
 		Balance: balances::<Instance1>::{Call<T>, Origin<T>},
 	}
 }

substrate/frame/support/test/tests/construct_runtime_ui/invalid_module_details_keyword.stderr

@@ -1,4 +1,4 @@
-error: expected `default` or identifier
+error: expected one of: `Module`, `Call`, `Storage`, `Event`, `Config`, `Origin`, `Inherent`, `ValidateUnsigned`
  --> $DIR/invalid_module_details_keyword.rs:9:20
   |
 9 |         system: System::{enum},

substrate/frame/support/test/tests/construct_runtime_ui/invalid_module_entry.rs

@@ -6,7 +6,7 @@ construct_runtime! {
 		NodeBlock = Block,
 		UncheckedExtrinsic = UncheckedExtrinsic
 	{
-		System: system,
+		System: system::{Module},
 		Balance: balances::{Error},
 	}
 }

substrate/frame/support/test/tests/construct_runtime_ui/invalid_module_entry.stderr

@@ -1,4 +1,4 @@
-error: Only the following modules are allowed: `Module`, `Call`, `Storage`, `Event`, `Config`, `Origin`, `Inherent`, `ValidateUnsigned`
+error: expected one of: `Module`, `Call`, `Storage`, `Event`, `Config`, `Origin`, `Inherent`, `ValidateUnsigned`
   --> $DIR/invalid_module_entry.rs:10:23
    |
 10 |         Balance: balances::{Error},

substrate/frame/support/test/tests/construct_runtime_ui/invalid_token_after_module.stderr

@@ -1,4 +1,4 @@
-error: expected `,`
+error: expected `::`
  --> $DIR/invalid_token_after_module.rs:9:18
   |
 9 |         system: System ?

substrate/frame/support/test/tests/construct_runtime_ui/missing_event_generic_on_module_with_instance.rs

@@ -6,7 +6,7 @@ construct_runtime! {
 		NodeBlock = Block,
 		UncheckedExtrinsic = UncheckedExtrinsic
 	{
-		System: system,
+		System: system::{Module},
 		Balance: balances::<Instance1>::{Event},
 	}
 }

substrate/frame/support/test/tests/construct_runtime_ui/missing_origin_generic_on_module_with_instance.rs

@@ -6,7 +6,7 @@ construct_runtime! {
 		NodeBlock = Block,
 		UncheckedExtrinsic = UncheckedExtrinsic
 	{
-		System: system,
+		System: system::{Module},
 		Balance: balances::<Instance1>::{Origin},
 	}
 }

substrate/frame/support/test/tests/construct_runtime_ui/params_in_invalid_module.rs

@@ -6,7 +6,7 @@ construct_runtime! {
 		NodeBlock = Block,
 		UncheckedExtrinsic = UncheckedExtrinsic
 	{
-		System: system,
+		System: system::{Module},
 		Balance: balances::<Instance1>::{Call(toto), Origin<I>},
 	}
 }