Add pallet attribute macro to declare pallets

[gui1117]

Fix #2782
Fix #5040
Related #4288
Related #5148
Fix #5678
Related #6494 (will be automatically generated)
Fix #5913

TL;DR;

This PR only introduce attribute macro and its tests, though it doesn't migrate any pallet, not even frame-system

The macro doc is in frame/support/src/lib.rs: https://github.com/paritytech/substrate/pull/6877/files#diff-31d6476b566f793c17f32f9fdefeb487988af58c61ba752d0275b91f6c8aaccaR983

Rendered doc can be found at https://thiolliere.github.io/doc/frame_support/attr.pallet.html

macro code is at frame/support/procedural/src/pallet/* and basically parse the macro to some Def struct. And then expand from thie Def struct.

See controversial point below to make further your opinion.

PR commit description and further PR

• This PR in order of commit
  • system::Trait -> system::Config rename
  • construct_runtime: allow to give Pallet instead of Module
  • pallet attribute macro with tests but not used in substrate yet
  • a helper migration template to be printed by decl_storage
• next PR: use in frame-system and pallet-example: https://github.com/paritytech/substrate/compare/gui-macro-attribute...gui-macro-attribute-upgrade-system-example?expand=1
• next to be splitted PRs: https://github.com/paritytech/substrate/compare/gui-macro-attribute...gui-macro-attribute-everywhere?expand=1

Breaking change for migrated pallet (no pallet are migrated in this PR)

• their Trait is renamed Config
• all storages are now generic over T
• module prefix used by storages is now the PalletInfo::name given by construct_runtime.
• no longer RawEvent.

Changes

• type alias from frame_system::Module to frame_system::Pallet

Addition

• pallet attribute macro the new macro to define a pallet. doc is available at https://thiolliere.github.io/doc/frame_support/attr.pallet.html

Controversial points: (or just points which needs review)

• in the pallet attribute macro: the generic names are enforced. i.e. you can't write
  #[pallet::call] impl<G: Config> ... instead macro enforce to use T: Config.

  justification:

  • It bring consistency in how pallet are declared
  • It makes pallet code easier to check
  • macro error message is very clear about this rule, no user can't get confused.

  I can work on allowing to use any ident instead of just T if ppl feel the need.

  We could also accept either T: Config or C: Config and ask it to be consistent in all declaration, thus where_clause are easily mergeable. Anyway I would rather do that in another PR.

• #[pallet::genesis_config] and #[pallet::genesis_build] will add the attribute
  #[cfg(features = "std")]. This result in code such as:

   #[pallet::genesis_config]
   pub struct GenesisConfig<T: Config> {}
  
   #[cfg(feature = "std")] // This flag need is a bit weird
   impl<T: Config> Default for GenesisConfig<T> {
   	fn default() -> Self {
   		Self {}
   	}
   }
  
   #[pallet::genesis_build]
   impl<T: Config> GenesisBuild<T> for GenesisConfig<T> {
   	fn build(&self) {}
   }

  Another possibilty (thanks Bryan for the idea) could be to wrap them in a module:

   #[cfg(features = "std")]
   mod genesis {
   	#[pallet::genesis_config]
   	pub struct GenesisConfig<T: Trait> {}
   	..
   }

  But I feel more annoying than helpful as it adds even more indentation and namespace

• Because storages now use PalletInfo for prefixes, tests shouldn't use () as PalletInfo as all
  storages will use the same module_prefix "test" which can lead to collision. Anyway in the not
  to far future, everybody should use construct_runtime in their pallet mock/tests.

• Becaues storages now use PalletInfo for prefixes, modifying the name of a pallet in
  construct_runtime require a migration, which might not be expected.
  Also live chain it means instantiable pallet might need to be renamed:
  For instance Council should be renamed migrated from "Instance1Collective" to "Council".

• Becaues storages now use PalletInfo for prefixes, when upgrading a pallet from old decl_* macro
  to new one. every runtime must be careful to rename their pallet or migrate.
  (except for System pallet as its name is enforced to be System anyway)

• upgrade to new macro guidelines: https://thiolliere.github.io/doc/frame_support/attr.pallet.html#upgrade-guidelines

• checking upgrade guidelines: https://thiolliere.github.io/doc/frame_support/attr.pallet.html#checking-upgrade-guidelines

[shawntabrizi]

Does everything need to be wrapped in a single module?

#[frame_support::pallet(Example)]
// NOTE: Example is name of the pallet, it will be used as unique identifier for storage
pub mod pallet {

It would be really nice to be able to write a pallet's logic across multiple files I think.

[gui1117]

Does everything need to be wrapped in a single module?

It would be really nice to be able to write a pallet's logic across multiple files I think.

Everything with a #[pallet::*] attribute needs to be written in this pallet module but like current macros their implementation can refer to code outside the module.

Note that most of our pallet have all those defined in one module.

At some point we could allow to have some stuff defined outside, and maybe referenced to like I don't know some:

#[frame_support::pallet(Example)]
#[pallet::external_event_def(my_path_to_event)]
mod pallet {
}

but that might be tricky in some cases and also it would be more difficult for implementation (as information from one macro to the other needs to be send through regular rust types)

[gui1117]

Just a note:

So right now we derive Eq, PartialEq, Debug and Clone for Event and for Call without doing any generic bound.

(for event this is anyway required because frame_system::Trait::Event requires it)
(EDIT: so pallet could have its event not implementing Eq generically, but at the end the runtime event will have to implement Eq, so I see no real reason not to bound Eq generically directly)

If we derive Eq without where clause we have a not so bad error message (at least it contains the corresponding associated type)

error[E0369]: binary operation `==` cannot be applied to type `&<T as pallet::Trait>::Bar`
 --> $DIR/call_009.rs:1:1
  |
1 | #[frame_support::pallet(Example)]
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: this error originates in a derive macro (in Nightly builds, run with -Z macro-backtrace for more info)

but parity-scale-codec will derive with a where clause and the error message doesn't give information:

error[E0277]: the trait bound `pallet::Call<T>: pallet::_::_parity_scale_codec::Decode` is not satisfied
 --> $DIR/call_008.rs:1:1
  |
1 | #[frame_support::pallet(Example)]
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `pallet::_::_parity_scale_codec::Decode` is not implemented for `pallet::Call<T>`
  |
  = note: this error originates in an attribute macro (in Nightly builds, run with -Z macro-backtrace for more info)

I guess as we need anyway codec on them, we could derive codec without any where clause. This would give a better error message. For this we just need to add it to codec-derive macro.

[gui1117]

so with this mod pallet everything is declared inside the pallet module. So for using it in construct_runtime user needs to do an extra step:

• either importing pallet module as pallet: pub use pallet_treasury::pallet as pallet_treasury_pallet; and use it in construct_runtime: construct_runtime!(... Treasury: pallet_trearsury_pallet..).
• or we modify construct_runtime to be given some path (which is difficult considering construct_runtime still relies on old macros for generating stuff)
• or inside pallet user can do pub use pallet::* or pub use pallet::{Event, Call, Module, Trait}.
  this could be done automatically with a new attribute: #[pallet::export_pallet_inplace] or by default.

EDIT: I think we should go for last option and maybe improve construct_runtime later. That means if pallet_treasury crate want to make its pallet definition available at the root namespace then it needs to write pub use pallet::*.

[gui1117]

I am implementing the helper for storage getter: #[pallet::generate_getter(fn foo)]. But now that the storage is a regular type, QueryKind can actually be generic. So the function must be written: fn foo() -> <MyStorage as StorageValue<#value>>::Query.

But if storage is private then it leaks private type into public interface. Thus solutions are:

• make all storage public (actually I think having all storage public makes sense when you have to do manual migration in the runtime).
• try to get what is QueryKind by reading the generic parameter. This would be a best effort as user can do some type alias that would break this.

I opted for second method.

[bkchr]

What is a type_value?

[gui1117]

the idea came from Gav #5678 (comment) as an easy way to implement a struct which implements Get.

It is same as parameter_types but it allows to have some generic. another possibility is to extend parameter type to allow generic, and it should be directly usable here.
The macro check that generic are consistent (i.e. instance generic is used or not depending on trait) but anyway code will not compile if user mixed himself.

[bkchr]

Ahh it's for setting the default value, I see. Hmm.

[gui1117]

This doesn't touch any existing code but just add for a new pallet macro.
I think syntax is well reviewed, code could have some refactor, but I don't see it as a strict necessity but rather an improvement (But the code should be already decent enough I think).

Thus I think we can merge. if any contradictory opinion we can still revert without any issue.

[gui1117]

bot merge

[ghost]

Waiting for commit status.