frame pallet macro: fix span for error on wrong returned type. (#5580)

Explicitly give the types in some generated code so that the error shows
up good when user code is wrong.

---------

Co-authored-by: Bastian Köcher <[email protected]>

Author: gui1117

prdoc/pr_5580.prdoc

@@ -0,0 +1,13 @@
+# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0
+# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json
+
+title: Fix error message on pallet macro
+
+doc:
+  - audience: Runtime Dev
+    description: |
+        Improve error message for pallet macro generated code.
+
+crates:
+  - name: frame-support-procedural
+    bump: patch

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

@@ -18,7 +18,7 @@
 use crate::{
 	pallet::{
 		expand::warnings::{weight_constant_warning, weight_witness_warning},
-		parse::call::CallWeightDef,
+		parse::{call::CallWeightDef, helper::CallReturnType},
 		Def,
 	},
 	COUNTER,
@@ -197,18 +197,36 @@ pub fn expand_call(def: &mut Def) -> proc_macro2::TokenStream {
 	let capture_docs = if cfg!(feature = "no-metadata-docs") { "never" } else { "always" };
 
 	// Wrap all calls inside of storage layers
-	if let Some(syn::Item::Impl(item_impl)) = def
-		.call
-		.as_ref()
-		.map(|c| &mut def.item.content.as_mut().expect("Checked by def parser").1[c.index])
-	{
-		item_impl.items.iter_mut().for_each(|i| {
-			if let syn::ImplItem::Fn(method) = i {
+	if let Some(call) = def.call.as_ref() {
+		let item_impl =
+			&mut def.item.content.as_mut().expect("Checked by def parser").1[call.index];
+		let syn::Item::Impl(item_impl) = item_impl else {
+			unreachable!("Checked by def parser");
+		};
+
+		item_impl.items.iter_mut().enumerate().for_each(|(i, item)| {
+			if let syn::ImplItem::Fn(method) = item {
+				let return_type =
+					&call.methods.get(i).expect("def should be consistent with item").return_type;
+
+				let (ok_type, err_type) = match return_type {
+					CallReturnType::DispatchResult => (
+						quote::quote!(()),
+						quote::quote!(#frame_support::pallet_prelude::DispatchError),
+					),
+					CallReturnType::DispatchResultWithPostInfo => (
+						quote::quote!(#frame_support::dispatch::PostDispatchInfo),
+						quote::quote!(#frame_support::dispatch::DispatchErrorWithPostInfo),
+					),
+				};
+
 				let block = &method.block;
 				method.block = syn::parse_quote! {{
 					// We execute all dispatchable in a new storage layer, allowing them
 					// to return an error at any point, and undoing any storage changes.
-					#frame_support::storage::with_storage_layer(|| #block)
+					#frame_support::storage::with_storage_layer::<#ok_type, #err_type, _>(
+						|| #block
+					)
 				}};
 			}
 		});

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

@@ -89,6 +89,8 @@ pub struct CallVariantDef {
 	pub cfg_attrs: Vec<syn::Attribute>,
 	/// The optional `feeless_if` attribute on the `pallet::call`.
 	pub feeless_check: Option<syn::ExprClosure>,
+	/// The return type of the call: `DispatchInfo` or `DispatchResultWithPostInfo`.
+	pub return_type: helper::CallReturnType,
 }
 
 /// Attributes for functions in call impl block.
@@ -260,13 +262,7 @@ impl CallDef {
 					},
 				}
 
-				if let syn::ReturnType::Type(_, type_) = &method.sig.output {
-					helper::check_pallet_call_return_type(type_)?;
-				} else {
-					let msg = "Invalid pallet::call, require return type \
-						DispatchResultWithPostInfo";
-					return Err(syn::Error::new(method.sig.span(), msg))
-				}
+				let return_type = helper::check_pallet_call_return_type(&method.sig)?;
 
 				let cfg_attrs: Vec<syn::Attribute> = helper::get_item_cfg_attrs(&method.attrs);
 				let mut call_idx_attrs = vec![];
@@ -447,6 +443,7 @@ impl CallDef {
 					attrs: method.attrs.clone(),
 					cfg_attrs,
 					feeless_check,
+					return_type,
 				});
 			} else {
 				let msg = "Invalid pallet::call, only method accepted";

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

@@ -597,25 +597,38 @@ pub fn check_type_value_gen(
 	Ok(i)
 }
 
+/// The possible return type of a dispatchable.
+#[derive(Clone)]
+pub enum CallReturnType {
+	DispatchResult,
+	DispatchResultWithPostInfo,
+}
+
 /// Check the keyword `DispatchResultWithPostInfo` or `DispatchResult`.
-pub fn check_pallet_call_return_type(type_: &syn::Type) -> syn::Result<()> {
-	pub struct Checker;
+pub fn check_pallet_call_return_type(sig: &syn::Signature) -> syn::Result<CallReturnType> {
+	let syn::ReturnType::Type(_, type_) = &sig.output else {
+		let msg = "Invalid pallet::call, require return type \
+			DispatchResultWithPostInfo";
+		return Err(syn::Error::new(sig.span(), msg))
+	};
+
+	pub struct Checker(CallReturnType);
 	impl syn::parse::Parse for Checker {
 		fn parse(input: syn::parse::ParseStream) -> syn::Result<Self> {
 			let lookahead = input.lookahead1();
 			if lookahead.peek(keyword::DispatchResultWithPostInfo) {
 				input.parse::<keyword::DispatchResultWithPostInfo>()?;
-				Ok(Self)
+				Ok(Self(CallReturnType::DispatchResultWithPostInfo))
 			} else if lookahead.peek(keyword::DispatchResult) {
 				input.parse::<keyword::DispatchResult>()?;
-				Ok(Self)
+				Ok(Self(CallReturnType::DispatchResult))
 			} else {
 				Err(lookahead.error())
 			}
 		}
 	}
 
-	syn::parse2::<Checker>(type_.to_token_stream()).map(|_| ())
+	syn::parse2::<Checker>(type_.to_token_stream()).map(|c| c.0)
 }
 
 pub(crate) fn two128_str(s: &str) -> TokenStream {

substrate/frame/support/test/tests/pallet_ui/call_span_for_error.rs

@@ -0,0 +1,37 @@
+// 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.
+
+#[frame_support::pallet(dev_mode)]
+mod pallet {
+	use frame_support::pallet_prelude::*;
+	use frame_system::pallet_prelude::*;
+
+	#[pallet::config]
+	pub trait Config: frame_system::Config {}
+
+	#[pallet::pallet]
+	pub struct Pallet<T>(_);
+
+	#[pallet::call]
+	impl <T: Config> Pallet<T> {
+		pub fn foo(origin: OriginFor<T>) -> DispatchResultWithPostInfo {
+			return Err(DispatchError::BadOrigin);
+		}
+	}
+}
+
+fn main() {}

substrate/frame/support/test/tests/pallet_ui/call_span_for_error.stderr

@@ -0,0 +1,26 @@
+error[E0308]: mismatched types
+  --> tests/pallet_ui/call_span_for_error.rs:32:15
+   |
+32 |             return Err(DispatchError::BadOrigin);
+   |                    --- ^^^^^^^^^^^^^^^^^^^^^^^^ expected `DispatchErrorWithPostInfo<PostDispatchInfo>`, found `DispatchError`
+   |                    |
+   |                    arguments to this enum variant are incorrect
+   |
+   = note: expected struct `DispatchErrorWithPostInfo<PostDispatchInfo>`
+                found enum `frame_support::pallet_prelude::DispatchError`
+help: the type constructed contains `frame_support::pallet_prelude::DispatchError` due to the type of the argument passed
+  --> tests/pallet_ui/call_span_for_error.rs:32:11
+   |
+32 |             return Err(DispatchError::BadOrigin);
+   |                    ^^^^------------------------^
+   |                        |
+   |                        this argument influences the type of `Err`
+note: tuple variant defined here
+  --> $RUST/core/src/result.rs
+   |
+   |     Err(#[stable(feature = "rust1", since = "1.0.0")] E),
+   |     ^^^
+help: call `Into::into` on this expression to convert `frame_support::pallet_prelude::DispatchError` into `DispatchErrorWithPostInfo<PostDispatchInfo>`
+   |
+32 |             return Err(DispatchError::BadOrigin.into());
+   |                                                +++++++