diff --git a/prdoc/pr_5580.prdoc b/prdoc/pr_5580.prdoc new file mode 100644 index 000000000000..e03b946070aa --- /dev/null +++ b/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 diff --git a/substrate/frame/support/procedural/src/pallet/expand/call.rs b/substrate/frame/support/procedural/src/pallet/expand/call.rs index f395872c8a80..5dc8dc3146cf 100644 --- a/substrate/frame/support/procedural/src/pallet/expand/call.rs +++ b/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 + ) }}; } }); diff --git a/substrate/frame/support/procedural/src/pallet/parse/call.rs b/substrate/frame/support/procedural/src/pallet/parse/call.rs index 4e09b86fddec..68c2cb8bd1b3 100644 --- a/substrate/frame/support/procedural/src/pallet/parse/call.rs +++ b/substrate/frame/support/procedural/src/pallet/parse/call.rs @@ -89,6 +89,8 @@ pub struct CallVariantDef { pub cfg_attrs: Vec, /// The optional `feeless_if` attribute on the `pallet::call`. pub feeless_check: Option, + /// 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 = 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"; diff --git a/substrate/frame/support/procedural/src/pallet/parse/helper.rs b/substrate/frame/support/procedural/src/pallet/parse/helper.rs index d4f58a4c56df..d5ae607d90f9 100644 --- a/substrate/frame/support/procedural/src/pallet/parse/helper.rs +++ b/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 { + 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 { let lookahead = input.lookahead1(); if lookahead.peek(keyword::DispatchResultWithPostInfo) { input.parse::()?; - Ok(Self) + Ok(Self(CallReturnType::DispatchResultWithPostInfo)) } else if lookahead.peek(keyword::DispatchResult) { input.parse::()?; - Ok(Self) + Ok(Self(CallReturnType::DispatchResult)) } else { Err(lookahead.error()) } } } - syn::parse2::(type_.to_token_stream()).map(|_| ()) + syn::parse2::(type_.to_token_stream()).map(|c| c.0) } pub(crate) fn two128_str(s: &str) -> TokenStream { diff --git a/substrate/frame/support/test/tests/pallet_ui/call_span_for_error.rs b/substrate/frame/support/test/tests/pallet_ui/call_span_for_error.rs new file mode 100644 index 000000000000..08b42c29a68b --- /dev/null +++ b/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(_); + + #[pallet::call] + impl Pallet { + pub fn foo(origin: OriginFor) -> DispatchResultWithPostInfo { + return Err(DispatchError::BadOrigin); + } + } +} + +fn main() {} diff --git a/substrate/frame/support/test/tests/pallet_ui/call_span_for_error.stderr b/substrate/frame/support/test/tests/pallet_ui/call_span_for_error.stderr new file mode 100644 index 000000000000..8f3003c02227 --- /dev/null +++ b/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`, found `DispatchError` + | | + | arguments to this enum variant are incorrect + | + = note: expected struct `DispatchErrorWithPostInfo` + 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` + | +32 | return Err(DispatchError::BadOrigin.into()); + | +++++++