[frame] #[pallet::composite_enum] improved variant count handling + removed pallet_balances's MaxHolds config (#2657)

I started this investigation/issue based on @liamaharon question
[here](https://github.com/paritytech/polkadot-sdk/pull/1801#discussion_r1410452499).

## Problem

The `pallet_balances` integrity test should correctly detect that the
runtime has correct distinct `HoldReasons` variant count. I assume the
same situation exists for RuntimeFreezeReason.

It is not a critical problem, if we set `MaxHolds` with a sufficiently
large value, everything should be ok. However, in this case, the
integrity_test check becomes less useful.

**Situation for "any" runtime:**
- `HoldReason` enums from different pallets:
```rust
        /// from pallet_nis
        #[pallet::composite_enum]
	pub enum HoldReason {
		NftReceipt,
	}

        /// from pallet_preimage
        #[pallet::composite_enum]
	pub enum HoldReason {
		Preimage,
	}

        // from pallet_state-trie-migration
        #[pallet::composite_enum]
	pub enum HoldReason {
		SlashForContinueMigrate,
		SlashForMigrateCustomTop,
		SlashForMigrateCustomChild,
	}
```

- generated `RuntimeHoldReason` enum looks like:
```rust
pub enum RuntimeHoldReason {

    #[codec(index = 32u8)]
    Preimage(pallet_preimage::HoldReason),

    #[codec(index = 38u8)]
    Nis(pallet_nis::HoldReason),

    #[codec(index = 42u8)]
    StateTrieMigration(pallet_state_trie_migration::HoldReason),
}
```

- composite enum `RuntimeHoldReason` variant count is detected as `3`
- we set `type MaxHolds = ConstU32<3>`
- `pallet_balances::integrity_test` is ok with `3`(at least 3)

However, the real problem can occur in a live runtime where some
functionality might stop working. This is due to a total of 5 distinct
hold reasons (for pallets with multi-instance support, it is even more),
and not all of them can be used because of an incorrect `MaxHolds`,
which is deemed acceptable according to the `integrity_test`:
  ```
  // pseudo-code - if we try to call all of these:

T::Currency::hold(&pallet_nis::HoldReason::NftReceipt.into(),
&nft_owner, deposit)?;
T::Currency::hold(&pallet_preimage::HoldReason::Preimage.into(),
&nft_owner, deposit)?;

T::Currency::hold(&pallet_state_trie_migration::HoldReason::SlashForContinueMigrate.into(),
&nft_owner, deposit)?;

  // With `type MaxHolds = ConstU32<3>` these two will fail

T::Currency::hold(&pallet_state_trie_migration::HoldReason::SlashForMigrateCustomTop.into(),
&nft_owner, deposit)?;

T::Currency::hold(&pallet_state_trie_migration::HoldReason::SlashForMigrateCustomChild.into(),
&nft_owner, deposit)?;
  ```  


## Solutions

A macro `#[pallet::*]` expansion is extended of `VariantCount`
implementation for the `#[pallet::composite_enum]` enum type. This
expansion generates the `VariantCount` implementation for pallets'
`HoldReason`, `FreezeReason`, `LockId`, and `SlashReason`. Enum variants
must be plain enum values without fields to ensure a deterministic
count.

The composite runtime enum, `RuntimeHoldReason` and
`RuntimeFreezeReason`, now sets `VariantCount::VARIANT_COUNT` as the sum
of pallets' enum `VariantCount::VARIANT_COUNT`:
```rust
#[frame_support::pallet(dev_mode)]
mod module_single_instance {

	#[pallet::composite_enum]
	pub enum HoldReason {
		ModuleSingleInstanceReason1,
		ModuleSingleInstanceReason2,
	}
...
}

#[frame_support::pallet(dev_mode)]
mod module_multi_instance {

	#[pallet::composite_enum]
	pub enum HoldReason<I: 'static = ()> {
		ModuleMultiInstanceReason1,
		ModuleMultiInstanceReason2,
		ModuleMultiInstanceReason3,
	}
...
}


impl self::sp_api_hidden_includes_construct_runtime::hidden_include::traits::VariantCount
    for RuntimeHoldReason
{
    const VARIANT_COUNT: u32 = 0
        + module_single_instance::HoldReason::VARIANT_COUNT
        + module_multi_instance::HoldReason::<module_multi_instance::Instance1>::VARIANT_COUNT
        + module_multi_instance::HoldReason::<module_multi_instance::Instance2>::VARIANT_COUNT
        + module_multi_instance::HoldReason::<module_multi_instance::Instance3>::VARIANT_COUNT;
}
```

In addition, `MaxHolds` is removed (as suggested
[here](https://github.com/paritytech/polkadot-sdk/pull/2657#discussion_r1443324573))
from `pallet_balances`, and its `Holds` are now bounded to
`RuntimeHoldReason::VARIANT_COUNT`. Therefore, there is no need to let
the runtime specify `MaxHolds`.


## For reviewers

Relevant changes can be found here:
- `substrate/frame/support/procedural/src/lib.rs` 
-  `substrate/frame/support/procedural/src/pallet/parse/composite.rs`
-  `substrate/frame/support/procedural/src/pallet/expand/composite.rs`
-
`substrate/frame/support/procedural/src/construct_runtime/expand/composite_helper.rs`
-
`substrate/frame/support/procedural/src/construct_runtime/expand/hold_reason.rs`
-
`substrate/frame/support/procedural/src/construct_runtime/expand/freeze_reason.rs`
- `substrate/frame/support/src/traits/misc.rs`

And the rest of the files is just about removed `MaxHolds` from
`pallet_balances`

## Next steps

Do the same for `MaxFreezes`
https://github.com/paritytech/polkadot-sdk/issues/2997.

---------

Co-authored-by: command-bot <>
Co-authored-by: Bastian Köcher <git@kchr.de>
Co-authored-by: Dónal Murray <donal.murray@parity.io>
Co-authored-by: gupnik <nikhilgupta.iitk@gmail.com>

substrate/frame/support/procedural/src/construct_runtime/expand/composite_helper.rs

@@ -68,3 +68,34 @@ pub(crate) fn expand_variant(
 		}
 	}
 }
+
+pub(crate) fn expand_variant_count(
+	composite_name: &str,
+	path: &PalletPath,
+	instance: Option<&Ident>,
+) -> TokenStream {
+	let composite_name = quote::format_ident!("{}", composite_name);
+
+	if let Some(inst) = instance {
+		quote! {
+			#path::#composite_name::<#path::#inst>::VARIANT_COUNT
+		}
+	} else {
+		// Wrapped `<`..`>` means: use default type parameter for enum.
+		//
+		// This is used for pallets without instance support or pallets with instance support when
+		// we don't specify instance:
+		//
+		// ```nocompile
+		// pub struct Pallet<T, I = ()>{..}
+		//
+		// #[pallet::composite_enum]
+		// pub enum HoldReason<I: 'static = ()> {..}
+		//
+		// Pallet1: pallet_x,  // <- default type parameter
+		// ```
+		quote! {
+			<#path::#composite_name>::VARIANT_COUNT
+		}
+	}
+}

substrate/frame/support/procedural/src/construct_runtime/expand/freeze_reason.rs

@@ -23,6 +23,7 @@ use quote::quote;
 pub fn expand_outer_freeze_reason(pallet_decls: &[Pallet], scrate: &TokenStream) -> TokenStream {
 	let mut conversion_fns = Vec::new();
 	let mut freeze_reason_variants = Vec::new();
+	let mut freeze_reason_variants_count = Vec::new();
 	for decl in pallet_decls {
 		if let Some(_) = decl.find_part("FreezeReason") {
 			let variant_name = &decl.name;
@@ -44,9 +45,14 @@ pub fn expand_outer_freeze_reason(pallet_decls: &[Pallet], scrate: &TokenStream)
 				instance,
 				variant_name,
 			));
+
+			freeze_reason_variants_count.push(composite_helper::expand_variant_count(
+				"FreezeReason",
+				path,
+				instance,
+			));
 		}
 	}
-	let freeze_reason_variants_count = freeze_reason_variants.len() as u32;
 
 	quote! {
 		/// A reason for placing a freeze on funds.
@@ -61,7 +67,7 @@ pub fn expand_outer_freeze_reason(pallet_decls: &[Pallet], scrate: &TokenStream)
 		}
 
 		impl #scrate::traits::VariantCount for RuntimeFreezeReason {
-			const VARIANT_COUNT: u32 = #freeze_reason_variants_count;
+			const VARIANT_COUNT: u32 = 0 #( + #freeze_reason_variants_count )*;
 		}
 
 		#( #conversion_fns )*

substrate/frame/support/procedural/src/construct_runtime/expand/hold_reason.rs

@@ -23,6 +23,7 @@ use quote::quote;
 pub fn expand_outer_hold_reason(pallet_decls: &[Pallet], scrate: &TokenStream) -> TokenStream {
 	let mut conversion_fns = Vec::new();
 	let mut hold_reason_variants = Vec::new();
+	let mut hold_reason_variants_count = Vec::new();
 	for decl in pallet_decls {
 		if let Some(_) = decl.find_part("HoldReason") {
 			let variant_name = &decl.name;
@@ -44,9 +45,14 @@ pub fn expand_outer_hold_reason(pallet_decls: &[Pallet], scrate: &TokenStream) -
 				instance,
 				variant_name,
 			));
+
+			hold_reason_variants_count.push(composite_helper::expand_variant_count(
+				"HoldReason",
+				path,
+				instance,
+			));
 		}
 	}
-	let hold_reason_variants_count = hold_reason_variants.len() as u32;
 
 	quote! {
 		/// A reason for placing a hold on funds.
@@ -61,7 +67,7 @@ pub fn expand_outer_hold_reason(pallet_decls: &[Pallet], scrate: &TokenStream) -
 		}
 
 		impl #scrate::traits::VariantCount for RuntimeHoldReason {
-			const VARIANT_COUNT: u32 = #hold_reason_variants_count;
+			const VARIANT_COUNT: u32 = 0 #( + #hold_reason_variants_count )*;
 		}
 
 		#( #conversion_fns )*

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

@@ -432,10 +432,7 @@ pub fn derive_runtime_debug_no_bound(input: TokenStream) -> TokenStream {
 	if cfg!(any(feature = "std", feature = "try-runtime")) {
 		no_bound::debug::derive_debug_no_bound(input)
 	} else {
-		let input: syn::DeriveInput = match syn::parse(input) {
-			Ok(input) => input,
-			Err(e) => return e.to_compile_error().into(),
-		};
+		let input = syn::parse_macro_input!(input as syn::DeriveInput);
 
 		let name = &input.ident;
 		let (impl_generics, ty_generics, where_clause) = input.generics.split_for_impl();
@@ -463,10 +460,7 @@ pub fn derive_partial_eq_no_bound(input: TokenStream) -> TokenStream {
 /// Derive [`Eq`] but do no bound any generic. Docs are at `frame_support::EqNoBound`.
 #[proc_macro_derive(EqNoBound)]
 pub fn derive_eq_no_bound(input: TokenStream) -> TokenStream {
-	let input: syn::DeriveInput = match syn::parse(input) {
-		Ok(input) => input,
-		Err(e) => return e.to_compile_error().into(),
-	};
+	let input = syn::parse_macro_input!(input as syn::DeriveInput);
 
 	let name = &input.ident;
 	let (impl_generics, ty_generics, where_clause) = input.generics.split_for_impl();
@@ -1519,13 +1513,14 @@ pub fn origin(_: TokenStream, _: TokenStream) -> TokenStream {
 ///
 /// ```ignore
 /// Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Encode, Decode, MaxEncodedLen, TypeInfo,
-/// RuntimeDebug
+/// RuntimeDebug,
 /// ```
 ///
 /// For ease of usage, when no `#[derive]` attributes are found for the enum under
 /// `#[pallet::composite_enum]`, the aforementioned traits are automatically derived for it. The
 /// inverse is also true: if there are any `#[derive]` attributes found for the enum, then no traits
-/// will automatically be derived for it.
+/// will automatically be derived for it (this implies that you need to provide the
+/// `frame_support::traits::VariantCount` implementation).
 #[proc_macro_attribute]
 pub fn composite_enum(_: TokenStream, _: TokenStream) -> TokenStream {
 	pallet_macro_stub()

substrate/frame/support/procedural/src/no_bound/clone.rs

@@ -19,10 +19,7 @@ use syn::spanned::Spanned;
 
 /// Derive Clone but do not bound any generic.
 pub fn derive_clone_no_bound(input: proc_macro::TokenStream) -> proc_macro::TokenStream {
-	let input: syn::DeriveInput = match syn::parse(input) {
-		Ok(input) => input,
-		Err(e) => return e.to_compile_error().into(),
-	};
+	let input = syn::parse_macro_input!(input as syn::DeriveInput);
 
 	let name = &input.ident;
 	let (impl_generics, ty_generics, where_clause) = input.generics.split_for_impl();

substrate/frame/support/procedural/src/no_bound/debug.rs

@@ -19,10 +19,7 @@ use syn::spanned::Spanned;
 
 /// Derive Debug but do not bound any generics.
 pub fn derive_debug_no_bound(input: proc_macro::TokenStream) -> proc_macro::TokenStream {
-	let input: syn::DeriveInput = match syn::parse(input) {
-		Ok(input) => input,
-		Err(e) => return e.to_compile_error().into(),
-	};
+	let input = syn::parse_macro_input!(input as syn::DeriveInput);
 
 	let input_ident = &input.ident;
 	let (impl_generics, ty_generics, where_clause) = input.generics.split_for_impl();

substrate/frame/support/procedural/src/no_bound/partial_eq.rs

@@ -19,10 +19,7 @@ use syn::spanned::Spanned;
 
 /// Derive PartialEq but do not bound any generic.
 pub fn derive_partial_eq_no_bound(input: proc_macro::TokenStream) -> proc_macro::TokenStream {
-	let input: syn::DeriveInput = match syn::parse(input) {
-		Ok(input) => input,
-		Err(e) => return e.to_compile_error().into(),
-	};
+	let input = syn::parse_macro_input!(input as syn::DeriveInput);
 
 	let name = &input.ident;
 	let (impl_generics, ty_generics, where_clause) = input.generics.split_for_impl();

substrate/frame/support/procedural/src/pallet/expand/composite.rs

@@ -0,0 +1,40 @@
+// This file is part of Substrate.
+
+// Copyright (C) Parity Technologies (UK) Ltd.
+// SPDX-License-Identifier: Apache-2.0
+
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// 	http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+use crate::pallet::Def;
+use proc_macro2::TokenStream;
+
+/// Expands `composite_enum` and adds the `VariantCount` implementation for it."
+pub fn expand_composites(def: &mut Def) -> TokenStream {
+	let mut expand = quote::quote!();
+	let frame_support = &def.frame_support;
+
+	for composite in &def.composites {
+		let name = &composite.ident;
+		let (impl_generics, ty_generics, where_clause) = composite.generics.split_for_impl();
+		let variants_count = composite.variant_count;
+
+		// add `VariantCount` implementation for `composite_enum`
+		expand.extend(quote::quote_spanned!(composite.attr_span =>
+			impl #impl_generics #frame_support::traits::VariantCount for #name #ty_generics #where_clause {
+				const VARIANT_COUNT: u32 = #variants_count;
+			}
+		));
+	}
+
+	expand
+}

substrate/frame/support/procedural/src/pallet/expand/mod.rs

@@ -16,6 +16,7 @@
 // limitations under the License.
 
 mod call;
+mod composite;
 mod config;
 mod constants;
 mod doc_only;
@@ -76,6 +77,7 @@ pub fn expand(mut def: Def) -> proc_macro2::TokenStream {
 	let validate_unsigned = validate_unsigned::expand_validate_unsigned(&mut def);
 	let tt_default_parts = tt_default_parts::expand_tt_default_parts(&mut def);
 	let doc_only = doc_only::expand_doc_only(&mut def);
+	let composites = composite::expand_composites(&mut def);
 
 	def.item.attrs.insert(
 		0,
@@ -117,6 +119,7 @@ storage item. Otherwise, all storage items are listed among [*Type Definitions*]
 		#validate_unsigned
 		#tt_default_parts
 		#doc_only
+		#composites
 	);
 
 	def.item

substrate/frame/support/procedural/src/pallet/parse/composite.rs

@@ -91,8 +91,16 @@ pub struct CompositeDef {
 	pub index: usize,
 	/// The composite keyword used (contains span).
 	pub composite_keyword: keyword::CompositeKeyword,
+
+	/// Name of the associated type.
+	pub ident: syn::Ident,
+	/// Type parameters and where clause attached to a declaration of the pallet::composite_enum.
+	pub generics: syn::Generics,
 	/// The span of the pallet::composite_enum attribute.
 	pub attr_span: proc_macro2::Span,
+
+	/// Variant count of the pallet::composite_enum.
+	pub variant_count: u32,
 }
 
 impl CompositeDef {
@@ -103,6 +111,19 @@ impl CompositeDef {
 		item: &mut syn::Item,
 	) -> syn::Result<Self> {
 		let item = if let syn::Item::Enum(item) = item {
+			// check variants: composite enums support only field-less enum variants. This is
+			// because fields can introduce too many possibilities, making it challenging to compute
+			// a fixed variant count.
+			for variant in &item.variants {
+				match variant.fields {
+					syn::Fields::Named(_) | syn::Fields::Unnamed(_) =>
+						return Err(syn::Error::new(
+							variant.ident.span(),
+							"The composite enum does not support variants with fields!",
+						)),
+					syn::Fields::Unit => (),
+				}
+			}
 			item
 		} else {
 			return Err(syn::Error::new(
@@ -160,6 +181,13 @@ impl CompositeDef {
 		let composite_keyword =
 			syn::parse2::<keyword::CompositeKeyword>(item.ident.to_token_stream())?;
 
-		Ok(CompositeDef { index, composite_keyword, attr_span })
+		Ok(CompositeDef {
+			index,
+			composite_keyword,
+			attr_span,
+			generics: item.generics.clone(),
+			variant_count: item.variants.len() as u32,
+			ident: item.ident.clone(),
+		})
 	}
 }