draft: attempt to fix code duplication and AllPallets footgun

frame/executive/src/lib.rs

@@ -148,9 +148,10 @@ pub type OriginOf<E, C> = <CallOf<E, C> as Dispatchable>::Origin;
 /// - `Block`: The block type of the runtime
 /// - `Context`: The context that is used when checking an extrinsic.
 /// - `UnsignedValidator`: The unsigned transaction validator of the runtime.
-/// - `AllPallets`: Tuple that contains all modules. Will be used to call e.g. `on_initialize`.
-/// - `OnRuntimeUpgrade`: Custom logic that should be called after a runtime upgrade. Modules are
-///   already called by `AllPallets`. It will be called before all modules will be called.
+/// - `AllPallets`: Tuple that contains all pallets. Will be used to call e.g. `on_initialize`,
+///    it must include system pallet.
+/// - `OnRuntimeUpgrade`: Custom logic that should be called after a runtime upgrade. Pallets are
+///   already called by `AllPallets`. It will be called before all pallets will be called.
 pub struct Executive<System, Block, Context, UnsignedValidator, AllPallets, OnRuntimeUpgrade = ()>(
 	PhantomData<(System, Block, Context, UnsignedValidator, AllPallets, OnRuntimeUpgrade)>,
 );
@@ -212,9 +213,6 @@ where
 	pub fn execute_on_runtime_upgrade() -> frame_support::weights::Weight {
 		let mut weight = 0;
 		weight = weight.saturating_add(COnRuntimeUpgrade::on_runtime_upgrade());
-		weight = weight.saturating_add(
-			<frame_system::Pallet<System> as OnRuntimeUpgrade>::on_runtime_upgrade(),
-		);
 		weight = weight.saturating_add(<AllPallets as OnRuntimeUpgrade>::on_runtime_upgrade());
 
 		weight
@@ -257,15 +255,15 @@ where
 	#[cfg(feature = "try-runtime")]
 	pub fn try_runtime_upgrade() -> Result<frame_support::weights::Weight, &'static str> {
 		<
-			(frame_system::Pallet::<System>, COnRuntimeUpgrade, AllPallets)
+			(COnRuntimeUpgrade, AllPallets)
 			as
 			OnRuntimeUpgrade
 		>::pre_upgrade().unwrap();
 
 		let weight = Self::execute_on_runtime_upgrade();
 
 		<
-			(frame_system::Pallet::<System>, COnRuntimeUpgrade, AllPallets)
+			(COnRuntimeUpgrade, AllPallets)
 			as
 			OnRuntimeUpgrade
 		>::post_upgrade().unwrap();
@@ -306,9 +304,6 @@ where
 			digest,
 			frame_system::InitKind::Full,
 		);
-		weight = weight.saturating_add(<frame_system::Pallet<System> as OnInitialize<
-			System::BlockNumber,
-		>>::on_initialize(*block_number));
 		weight = weight.saturating_add(
 			<AllPallets as OnInitialize<System::BlockNumber>>::on_initialize(*block_number),
 		);
@@ -419,26 +414,16 @@ where
 		let mut remaining_weight = max_weight.saturating_sub(weight.total());
 
 		if remaining_weight > 0 {
-			let mut used_weight =
-				<frame_system::Pallet<System> as OnIdle<System::BlockNumber>>::on_idle(
-					block_number,
-					remaining_weight,
-				);
-			remaining_weight = remaining_weight.saturating_sub(used_weight);
-			used_weight = <AllPallets as OnIdle<System::BlockNumber>>::on_idle(
+			let used_weight = <AllPallets as OnIdle<System::BlockNumber>>::on_idle(
 				block_number,
 				remaining_weight,
-			)
-			.saturating_add(used_weight);
+			);
 			<frame_system::Pallet<System>>::register_extra_weight_unchecked(
 				used_weight,
 				DispatchClass::Mandatory,
 			);
 		}
 
-		<frame_system::Pallet<System> as OnFinalize<System::BlockNumber>>::on_finalize(
-			block_number,
-		);
 		<AllPallets as OnFinalize<System::BlockNumber>>::on_finalize(block_number);
 	}
 
@@ -1364,8 +1349,6 @@ mod tests {
 			let frame_system_upgrade_weight = frame_system::Pallet::<Runtime>::on_runtime_upgrade();
 			let custom_runtime_upgrade_weight = CustomOnRuntimeUpgrade::on_runtime_upgrade();
 			let runtime_upgrade_weight = <AllPallets as OnRuntimeUpgrade>::on_runtime_upgrade();
-			let frame_system_on_initialize_weight =
-				frame_system::Pallet::<Runtime>::on_initialize(block_number);
 			let on_initialize_weight =
 				<AllPallets as OnInitialize<u64>>::on_initialize(block_number);
 			let base_block_weight =
@@ -1377,7 +1360,6 @@ mod tests {
 				frame_system_upgrade_weight +
 					custom_runtime_upgrade_weight +
 					runtime_upgrade_weight +
-					frame_system_on_initialize_weight +
 					on_initialize_weight + base_block_weight,
 			);
 		});

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

@@ -206,32 +206,38 @@ fn decl_all_pallets<'a>(
 	}
 	// Make nested tuple structure like (((Babe, Consensus), Grandpa), ...)
 	// But ignore the system pallet.
-	let all_pallets = names
+	let all_pallets_without_system = names
 		.iter()
 		.filter(|n| **n != SYSTEM_PALLET_NAME)
 		.fold(TokenStream2::default(), |combined, name| quote!((#name, #combined)));
 
-	let all_pallets_with_system = names
+	let all_pallets = names
 		.iter()
 		.fold(TokenStream2::default(), |combined, name| quote!((#name, #combined)));
 
 	quote!(
 		#types
 		/// All pallets included in the runtime as a nested tuple of types.
-		/// Excludes the System pallet.
 		pub type AllPallets = ( #all_pallets );
+
 		/// All pallets included in the runtime as a nested tuple of types.
-		pub type AllPalletsWithSystem = ( #all_pallets_with_system );
+		#[deprecated(note = "use `AllPallets` instead")]
+		pub type AllPalletsWithSystem = AllPallets;
+
+		/// All pallets included in the runtime as a nested tuple of types.
+		/// Excludes the System pallet.
+		pub type AllPalletsWithoutSystem = ( #all_pallets_without_system );
 
 		/// All modules included in the runtime as a nested tuple of types.
 		/// Excludes the System pallet.
 		#[deprecated(note = "use `AllPallets` instead")]
 		#[allow(dead_code)]
-		pub type AllModules = ( #all_pallets );
+		#[allow(deprecated)]
+		pub type AllModules = AllPalletsWithoutSystem;
 		/// All modules included in the runtime as a nested tuple of types.
 		#[deprecated(note = "use `AllPalletsWithSystem` instead")]
 		#[allow(dead_code)]
-		pub type AllModulesWithSystem = ( #all_pallets_with_system );
+		pub type AllModulesWithSystem = AllPallets;
 	)
 }
 

frame/support/test/tests/pallet.rs

@@ -942,7 +942,7 @@ fn pallet_hooks_expand() {
 		assert_eq!(AllPallets::on_initialize(1), 10);
 		AllPallets::on_finalize(1);
 
-		assert_eq!(AllPallets::on_runtime_upgrade(), 30);
+		AllPallets::on_runtime_upgrade();
 
 		assert_eq!(
 			frame_system::Pallet::<Runtime>::events()[0].event,
@@ -956,6 +956,8 @@ fn pallet_hooks_expand() {
 			frame_system::Pallet::<Runtime>::events()[2].event,
 			Event::Example(pallet::Event::Something(30)),
 		);
+
+		assert_eq!(Example::on_runtime_upgrade(), 30);
 	})
 }
 

frame/support/test/tests/pallet_instance.rs

@@ -513,7 +513,7 @@ fn pallet_hooks_expand() {
 		assert_eq!(AllPallets::on_initialize(1), 21);
 		AllPallets::on_finalize(1);
 
-		assert_eq!(AllPallets::on_runtime_upgrade(), 61);
+		AllPallets::on_runtime_upgrade();
 
 		// The order is indeed reversed due to https://github.com/paritytech/substrate/issues/6280
 		assert_eq!(
@@ -540,6 +540,8 @@ fn pallet_hooks_expand() {
 			frame_system::Pallet::<Runtime>::events()[5].event,
 			Event::Example(pallet::Event::Something(30)),
 		);
+
+		assert_eq!(<(Example, Instance1Example)>::on_runtime_upgrade(), 61);
 	})
 }