From 6e2607e1970d7d729741f10cf0c12a233a275c3d Mon Sep 17 00:00:00 2001 From: dastansam Date: Fri, 5 Apr 2024 21:25:44 +0200 Subject: [PATCH 01/23] Initial solution --- .../api/proc-macro/src/impl_runtime_apis.rs | 16 ++++++++++++++- .../primitives/api/proc-macro/src/utils.rs | 20 +++++++++++++++++++ 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs b/substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs index 87a381fd7bf9..fb1fd99fc9cf 100644 --- a/substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs +++ b/substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs @@ -18,7 +18,8 @@ use crate::{ common::API_VERSION_ATTRIBUTE, utils::{ - extract_all_signature_types, extract_block_type_from_trait_path, extract_impl_trait, + extract_all_signature_types, extract_block_type_from_trait_path, + extract_idents_from_type_path, extract_impl_trait, extract_parameter_names_types_and_borrows, generate_crate_access, generate_runtime_mod_name_for_trait, parse_runtime_api_version, prefix_function_with_trait, versioned_trait_name, AllowSelfRefInParameters, RequireQualifiedTraitPath, @@ -81,6 +82,19 @@ fn generate_impl_call( let ptypes = params.iter().map(|v| &v.1); let pborrow = params.iter().map(|v| &v.2); + // usage of `Self` is not allowed in the scope of the trait implementation + params.iter().try_for_each(|v| { + if let Type::Path(path) = &v.1 { + if extract_idents_from_type_path(path).iter().any(|i| *i == "Self") { + return Err(Error::new( + path.span(), + "Usage of `Self` is not allowed in the scope of `impl_runtime_apis!`", + )) + } + } + Ok(()) + })?; + let decode_params = if params.is_empty() { quote!( if !#input.is_empty() { diff --git a/substrate/primitives/api/proc-macro/src/utils.rs b/substrate/primitives/api/proc-macro/src/utils.rs index c8c1f12d90a1..65bc3f023705 100644 --- a/substrate/primitives/api/proc-macro/src/utils.rs +++ b/substrate/primitives/api/proc-macro/src/utils.rs @@ -20,6 +20,7 @@ use inflector::Inflector; use proc_macro2::{Span, TokenStream}; use proc_macro_crate::{crate_name, FoundCrate}; use quote::{format_ident, quote}; +use std::collections::HashSet; use syn::{ parse_quote, spanned::Spanned, token::And, Attribute, Error, FnArg, GenericArgument, Ident, ImplItem, ItemImpl, Pat, Path, PathArguments, Result, ReturnType, Signature, Type, TypePath, @@ -238,6 +239,25 @@ pub fn extract_impl_trait(impl_: &ItemImpl, require: RequireQualifiedTraitPath) }) } +/// Extracts all unique `Ident`'s from the given `TypePath` +pub fn extract_idents_from_type_path(type_path: &TypePath) -> HashSet<&Ident> { + let mut idents = HashSet::new(); + + for segment in &type_path.path.segments { + idents.insert(&segment.ident); + + if let PathArguments::AngleBracketed(args) = &segment.arguments { + args.args.iter().for_each(|arg| { + if let GenericArgument::Type(Type::Path(p)) = arg { + idents.extend(extract_idents_from_type_path(p)); + } + }); + } + } + + idents +} + /// Parse the given attribute as `API_VERSION_ATTRIBUTE`. pub fn parse_runtime_api_version(version: &Attribute) -> Result { let version = version.parse_args::().map_err(|_| { From f89b9832f6900032e5d148eefe4cf82ecb8fed72 Mon Sep 17 00:00:00 2001 From: dastansam Date: Fri, 5 Apr 2024 21:37:40 +0200 Subject: [PATCH 02/23] Add prdoc --- prdoc/pr_4012.prdoc | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) create mode 100644 prdoc/pr_4012.prdoc diff --git a/prdoc/pr_4012.prdoc b/prdoc/pr_4012.prdoc new file mode 100644 index 000000000000..16d92acd8229 --- /dev/null +++ b/prdoc/pr_4012.prdoc @@ -0,0 +1,32 @@ +# 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: `impl_runtime_apis!`: prevent the use of `Self` + +doc: + - audience: Runtime Dev + description: | + Currently, if there is a type alias similar to `type HeaderFor` in the scope, it makes sense to expect that + `HeaderFor` and `HeaderFor` are equivalent. However, this is not the case. It currently leads to + a compilation error that `Self is not in scope`, which is confusing. This PR aims to altogether prevent the use of `Self` + inside `impl_runtime_apis!` macro. + + For example, the following code is not allowed: + ```rust + impl apis::Core for Runtime { + fn initialize_block(header: &HeaderFor) -> ExtrinsicInclusionMode { + RuntimeExecutive::initialize_block(header) + } + } + ``` + Instead, it should be written as: + ```rust + impl apis::Core for Runtime { + fn initialize_block(header: &HeaderFor) -> ExtrinsicInclusionMode { + RuntimeExecutive::initialize_block(header) + } + } + ``` +crates: + - name: sp-api + bump: minor From 598f9b07a63fc4bf7bb9ea6c473618d1a7b2a71d Mon Sep 17 00:00:00 2001 From: dastansam Date: Fri, 5 Apr 2024 21:40:01 +0200 Subject: [PATCH 03/23] Fix prdoc --- prdoc/pr_4012.prdoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/prdoc/pr_4012.prdoc b/prdoc/pr_4012.prdoc index 16d92acd8229..57b69771a8f1 100644 --- a/prdoc/pr_4012.prdoc +++ b/prdoc/pr_4012.prdoc @@ -1,7 +1,7 @@ # 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: `impl_runtime_apis!`: prevent the use of `Self` +title: "`impl_runtime_apis!`: prevent the use of `Self`" doc: - audience: Runtime Dev From 30dd063720f56e1032482dec75e417b1994bb3d1 Mon Sep 17 00:00:00 2001 From: dastansam Date: Sat, 6 Apr 2024 14:41:09 +0200 Subject: [PATCH 04/23] Use `Visit` trait --- .../api/proc-macro/src/impl_runtime_apis.rs | 58 ++++++++++++++----- .../primitives/api/proc-macro/src/utils.rs | 12 ++++ 2 files changed, 57 insertions(+), 13 deletions(-) diff --git a/substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs b/substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs index fb1fd99fc9cf..ea84885b74d5 100644 --- a/substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs +++ b/substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs @@ -36,6 +36,7 @@ use syn::{ parse::{Error, Parse, ParseStream, Result}, parse_macro_input, parse_quote, spanned::Spanned, + visit::{self, Visit}, Attribute, Ident, ImplItem, ItemImpl, LitInt, LitStr, Path, Signature, Type, TypePath, }; @@ -82,19 +83,6 @@ fn generate_impl_call( let ptypes = params.iter().map(|v| &v.1); let pborrow = params.iter().map(|v| &v.2); - // usage of `Self` is not allowed in the scope of the trait implementation - params.iter().try_for_each(|v| { - if let Type::Path(path) = &v.1 { - if extract_idents_from_type_path(path).iter().any(|i| *i == "Self") { - return Err(Error::new( - path.span(), - "Usage of `Self` is not allowed in the scope of `impl_runtime_apis!`", - )) - } - } - Ok(()) - })?; - let decode_params = if params.is_empty() { quote!( if !#input.is_empty() { @@ -817,6 +805,49 @@ fn generate_runtime_api_versions(impls: &[ItemImpl]) -> Result { )) } +/// Check trait implementations +struct CheckTraitImpls { + errors: Vec, +} + +impl CheckTraitImpls { + /// Check the given trait implementations. + /// + /// All errors are collected and returned at the end. + fn check(&mut self, trait_: &ItemImpl) { + visit::visit_item_impl(self, trait_) + } +} + +impl<'ast> Visit<'ast> for CheckTraitImpls { + fn visit_type_path(&mut self, i: &'ast syn::TypePath) { + if extract_idents_from_type_path(i).iter().any(|i| *i == "Self") { + self.errors.push(Error::new( + i.span(), + "Usage of `Self` is not allowed in the scope of `impl_runtime_apis!`", + )); + } + + visit::visit_type_path(self, i) + } +} + +/// Check all trait implementations are in the format we expect. +fn check_trait_impls(impls: &[ItemImpl]) -> Result<()> { + let mut checker = CheckTraitImpls { errors: Vec::new() }; + + impls.iter().for_each(|i| checker.check(i)); + + if let Some(err) = checker.errors.pop() { + Err(checker.errors.into_iter().fold(err, |mut acc, e| { + acc.combine(e); + acc + })) + } else { + Ok(()) + } +} + /// The implementation of the `impl_runtime_apis!` macro. pub fn impl_runtime_apis_impl(input: proc_macro::TokenStream) -> proc_macro::TokenStream { // Parse all impl blocks @@ -828,6 +859,7 @@ pub fn impl_runtime_apis_impl(input: proc_macro::TokenStream) -> proc_macro::Tok } fn impl_runtime_apis_impl_inner(api_impls: &[ItemImpl]) -> Result { + check_trait_impls(api_impls)?; let dispatch_impl = generate_dispatch_function(api_impls)?; let api_impls_for_runtime = generate_api_impl_for_runtime(api_impls)?; let base_runtime_api = generate_runtime_api_base_structures()?; diff --git a/substrate/primitives/api/proc-macro/src/utils.rs b/substrate/primitives/api/proc-macro/src/utils.rs index c92d010d467f..314faa7aa1b8 100644 --- a/substrate/primitives/api/proc-macro/src/utils.rs +++ b/substrate/primitives/api/proc-macro/src/utils.rs @@ -348,4 +348,16 @@ mod tests { assert_eq!(cfg_std, filtered[0]); assert_eq!(cfg_benchmarks, filtered[1]); } + + #[test] + fn check_extract_idents_from_type_path() { + let type_path: TypePath = parse_quote!(polkadot_primitives::types::HeaderFor); + let idents = extract_idents_from_type_path(&type_path); + + assert_eq!(idents.len(), 4); + assert!(idents.contains(&&format_ident!("polkadot_primitives"))); + assert!(idents.contains(&&format_ident!("types"))); + assert!(idents.contains(&&format_ident!("HeaderFor"))); + assert!(idents.contains(&&format_ident!("Self"))); + } } From c0a959d06e348baab0b792b18a62800c2f4bb797 Mon Sep 17 00:00:00 2001 From: dastansam Date: Sun, 7 Apr 2024 16:49:30 +0200 Subject: [PATCH 05/23] Prevent type arguments only --- prdoc/pr_4012.prdoc | 11 +++++++---- .../api/proc-macro/src/impl_runtime_apis.rs | 8 ++++---- .../primitives/api/proc-macro/src/utils.rs | 17 +++++++---------- 3 files changed, 18 insertions(+), 18 deletions(-) diff --git a/prdoc/pr_4012.prdoc b/prdoc/pr_4012.prdoc index 57b69771a8f1..674767e7389f 100644 --- a/prdoc/pr_4012.prdoc +++ b/prdoc/pr_4012.prdoc @@ -1,20 +1,22 @@ # 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: "`impl_runtime_apis!`: prevent the use of `Self`" +title: "`impl_runtime_apis!`: prevent the use of `Self` as type argument" doc: - audience: Runtime Dev description: | Currently, if there is a type alias similar to `type HeaderFor` in the scope, it makes sense to expect that `HeaderFor` and `HeaderFor` are equivalent. However, this is not the case. It currently leads to - a compilation error that `Self is not in scope`, which is confusing. This PR aims to altogether prevent the use of `Self` - inside `impl_runtime_apis!` macro. + a compilation error that `Self is not in scope`, which is confusing. This PR Introduces a checker struct, similar to + `CheckTraitDecls` in `decl_runtime_apis!`, `CheckTraitImpls`. It identifies usage of `Self` as a type argument in + `impl_runtime_apis!` and emits an error message. The error message suggests replacing `Self` with the runtime name. - For example, the following code is not allowed: + For example, the following example code is not allowed: ```rust impl apis::Core for Runtime { fn initialize_block(header: &HeaderFor) -> ExtrinsicInclusionMode { + let _: HeaderFor = header.clone(); RuntimeExecutive::initialize_block(header) } } @@ -23,6 +25,7 @@ doc: ```rust impl apis::Core for Runtime { fn initialize_block(header: &HeaderFor) -> ExtrinsicInclusionMode { + let _: HeaderFor = header.clone(); RuntimeExecutive::initialize_block(header) } } diff --git a/substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs b/substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs index ea84885b74d5..0edce14e1638 100644 --- a/substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs +++ b/substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs @@ -18,8 +18,8 @@ use crate::{ common::API_VERSION_ATTRIBUTE, utils::{ - extract_all_signature_types, extract_block_type_from_trait_path, - extract_idents_from_type_path, extract_impl_trait, + extract_all_signature_types, extract_angle_bracketed_idents_from_type_path, + extract_block_type_from_trait_path, extract_impl_trait, extract_parameter_names_types_and_borrows, generate_crate_access, generate_runtime_mod_name_for_trait, parse_runtime_api_version, prefix_function_with_trait, versioned_trait_name, AllowSelfRefInParameters, RequireQualifiedTraitPath, @@ -821,10 +821,10 @@ impl CheckTraitImpls { impl<'ast> Visit<'ast> for CheckTraitImpls { fn visit_type_path(&mut self, i: &'ast syn::TypePath) { - if extract_idents_from_type_path(i).iter().any(|i| *i == "Self") { + if extract_angle_bracketed_idents_from_type_path(i).iter().any(|i| *i == "Self") { self.errors.push(Error::new( i.span(), - "Usage of `Self` is not allowed in the scope of `impl_runtime_apis!`", + "`Self` can not be used as a type argument in the scope of `impl_runtime_apis!`. Use `Runtime` instead.", )); } diff --git a/substrate/primitives/api/proc-macro/src/utils.rs b/substrate/primitives/api/proc-macro/src/utils.rs index 314faa7aa1b8..c6d3956eaf46 100644 --- a/substrate/primitives/api/proc-macro/src/utils.rs +++ b/substrate/primitives/api/proc-macro/src/utils.rs @@ -242,16 +242,16 @@ pub fn extract_impl_trait(impl_: &ItemImpl, require: RequireQualifiedTraitPath) } /// Extracts all unique `Ident`'s from the given `TypePath` -pub fn extract_idents_from_type_path(type_path: &TypePath) -> HashSet<&Ident> { +pub fn extract_angle_bracketed_idents_from_type_path(type_path: &TypePath) -> HashSet<&Ident> { let mut idents = HashSet::new(); for segment in &type_path.path.segments { - idents.insert(&segment.ident); - if let PathArguments::AngleBracketed(args) = &segment.arguments { args.args.iter().for_each(|arg| { if let GenericArgument::Type(Type::Path(p)) = arg { - idents.extend(extract_idents_from_type_path(p)); + if let Some(ident) = p.path.get_ident() { + idents.insert(ident); + } } }); } @@ -350,14 +350,11 @@ mod tests { } #[test] - fn check_extract_idents_from_type_path() { + fn check_extract_angle_bracketed_idents_from_type_path() { let type_path: TypePath = parse_quote!(polkadot_primitives::types::HeaderFor); - let idents = extract_idents_from_type_path(&type_path); + let idents = extract_angle_bracketed_idents_from_type_path(&type_path); - assert_eq!(idents.len(), 4); - assert!(idents.contains(&&format_ident!("polkadot_primitives"))); - assert!(idents.contains(&&format_ident!("types"))); - assert!(idents.contains(&&format_ident!("HeaderFor"))); + assert_eq!(idents.len(), 1); assert!(idents.contains(&&format_ident!("Self"))); } } From ab26d1856a777a44de08c5e7a0197cfc3432db16 Mon Sep 17 00:00:00 2001 From: dastansam Date: Sun, 7 Apr 2024 16:53:52 +0200 Subject: [PATCH 06/23] Rename checker struct --- prdoc/pr_4012.prdoc | 2 +- .../primitives/api/proc-macro/src/impl_runtime_apis.rs | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/prdoc/pr_4012.prdoc b/prdoc/pr_4012.prdoc index 674767e7389f..5f6d89e46fb3 100644 --- a/prdoc/pr_4012.prdoc +++ b/prdoc/pr_4012.prdoc @@ -9,7 +9,7 @@ doc: Currently, if there is a type alias similar to `type HeaderFor` in the scope, it makes sense to expect that `HeaderFor` and `HeaderFor` are equivalent. However, this is not the case. It currently leads to a compilation error that `Self is not in scope`, which is confusing. This PR Introduces a checker struct, similar to - `CheckTraitDecls` in `decl_runtime_apis!`, `CheckTraitImpls`. It identifies usage of `Self` as a type argument in + `CheckTraitDecl` in `decl_runtime_apis!`, `CheckTraitImpl`. It identifies usage of `Self` as a type argument in `impl_runtime_apis!` and emits an error message. The error message suggests replacing `Self` with the runtime name. For example, the following example code is not allowed: diff --git a/substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs b/substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs index 0edce14e1638..a414a6ebf723 100644 --- a/substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs +++ b/substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs @@ -806,11 +806,11 @@ fn generate_runtime_api_versions(impls: &[ItemImpl]) -> Result { } /// Check trait implementations -struct CheckTraitImpls { +struct CheckTraitImpl { errors: Vec, } -impl CheckTraitImpls { +impl CheckTraitImpl { /// Check the given trait implementations. /// /// All errors are collected and returned at the end. @@ -819,7 +819,7 @@ impl CheckTraitImpls { } } -impl<'ast> Visit<'ast> for CheckTraitImpls { +impl<'ast> Visit<'ast> for CheckTraitImpl { fn visit_type_path(&mut self, i: &'ast syn::TypePath) { if extract_angle_bracketed_idents_from_type_path(i).iter().any(|i| *i == "Self") { self.errors.push(Error::new( @@ -834,7 +834,7 @@ impl<'ast> Visit<'ast> for CheckTraitImpls { /// Check all trait implementations are in the format we expect. fn check_trait_impls(impls: &[ItemImpl]) -> Result<()> { - let mut checker = CheckTraitImpls { errors: Vec::new() }; + let mut checker = CheckTraitImpl { errors: Vec::new() }; impls.iter().for_each(|i| checker.check(i)); From 6c5abbedb8beb5af13ee463c56236c5441483e51 Mon Sep 17 00:00:00 2001 From: Dastan <88332432+dastansam@users.noreply.github.com> Date: Sun, 7 Apr 2024 17:19:55 +0200 Subject: [PATCH 07/23] Update pr_4012.prdoc --- prdoc/pr_4012.prdoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/prdoc/pr_4012.prdoc b/prdoc/pr_4012.prdoc index 5f6d89e46fb3..c0288b79bdb8 100644 --- a/prdoc/pr_4012.prdoc +++ b/prdoc/pr_4012.prdoc @@ -10,7 +10,7 @@ doc: `HeaderFor` and `HeaderFor` are equivalent. However, this is not the case. It currently leads to a compilation error that `Self is not in scope`, which is confusing. This PR Introduces a checker struct, similar to `CheckTraitDecl` in `decl_runtime_apis!`, `CheckTraitImpl`. It identifies usage of `Self` as a type argument in - `impl_runtime_apis!` and emits an error message. The error message suggests replacing `Self` with the runtime name. + `impl_runtime_apis!` and emits an error message. The error message suggests replacing `Self` with `Runtime`. For example, the following example code is not allowed: ```rust From 706e2f74d1f898a46b76932ad54b4e1d78b4ad84 Mon Sep 17 00:00:00 2001 From: dastansam Date: Sun, 7 Apr 2024 17:23:11 +0200 Subject: [PATCH 08/23] Final changes --- .../primitives/api/proc-macro/src/impl_runtime_apis.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs b/substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs index a414a6ebf723..8de842b41db0 100644 --- a/substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs +++ b/substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs @@ -805,15 +805,15 @@ fn generate_runtime_api_versions(impls: &[ItemImpl]) -> Result { )) } -/// Check trait implementations +/// Checks that a trait implementation is in the format we expect. struct CheckTraitImpl { errors: Vec, } impl CheckTraitImpl { - /// Check the given trait implementations. + /// Check the given trait implementation. /// - /// All errors are collected and returned at the end. + /// All errors will be collected in `self.errors`. fn check(&mut self, trait_: &ItemImpl) { visit::visit_item_impl(self, trait_) } @@ -860,6 +860,7 @@ pub fn impl_runtime_apis_impl(input: proc_macro::TokenStream) -> proc_macro::Tok fn impl_runtime_apis_impl_inner(api_impls: &[ItemImpl]) -> Result { check_trait_impls(api_impls)?; + let dispatch_impl = generate_dispatch_function(api_impls)?; let api_impls_for_runtime = generate_api_impl_for_runtime(api_impls)?; let base_runtime_api = generate_runtime_api_base_structures()?; From 14cbca2ff7dbc11f08cab2439af762d4b8a54842 Mon Sep 17 00:00:00 2001 From: Pavlo Khrystenko Date: Wed, 23 Oct 2024 00:43:38 +0200 Subject: [PATCH 09/23] Rename "Self" to "Runtime" in sp_api::impl_item_apis --- Cargo.lock | 4 + .../primitives/api/proc-macro/Cargo.toml | 2 +- .../api/proc-macro/src/impl_runtime_apis.rs | 139 ++++++++---------- .../primitives/api/proc-macro/src/utils.rs | 29 ---- substrate/primitives/api/test/Cargo.toml | 4 + 5 files changed, 73 insertions(+), 105 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index c8f828e85e6a..48ce859a9596 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -21380,10 +21380,13 @@ dependencies = [ "futures", "log", "parity-scale-codec", + "proc-macro2 1.0.86", + "quote 1.0.37", "rustversion", "sc-block-builder", "scale-info", "sp-api 26.0.0", + "sp-api-proc-macro 15.0.0", "sp-consensus", "sp-core 28.0.0", "sp-runtime 31.0.1", @@ -21392,6 +21395,7 @@ dependencies = [ "sp-version 29.0.0", "static_assertions", "substrate-test-runtime-client", + "syn 2.0.79", "trybuild", ] diff --git a/substrate/primitives/api/proc-macro/Cargo.toml b/substrate/primitives/api/proc-macro/Cargo.toml index 659307e7b0f8..191578f432ad 100644 --- a/substrate/primitives/api/proc-macro/Cargo.toml +++ b/substrate/primitives/api/proc-macro/Cargo.toml @@ -20,7 +20,7 @@ proc-macro = true [dependencies] quote = { workspace = true } -syn = { features = ["extra-traits", "fold", "full", "visit"], workspace = true } +syn = { features = ["extra-traits", "fold", "full", "visit", "visit-mut"], workspace = true } proc-macro2 = { workspace = true } blake2 = { workspace = true } proc-macro-crate = { workspace = true } diff --git a/substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs b/substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs index d47e8dc740c4..38a8e866dcf5 100644 --- a/substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs +++ b/substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs @@ -7,7 +7,7 @@ // 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 +// 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, @@ -18,8 +18,8 @@ use crate::{ common::API_VERSION_ATTRIBUTE, utils::{ - extract_angle_bracketed_idents_from_type_path, extract_block_type_from_trait_path, - extract_impl_trait, extract_parameter_names_types_and_borrows, generate_crate_access, + extract_block_type_from_trait_path, extract_impl_trait, + extract_parameter_names_types_and_borrows, generate_crate_access, generate_runtime_mod_name_for_trait, parse_runtime_api_version, prefix_function_with_trait, versioned_trait_name, AllowSelfRefInParameters, RequireQualifiedTraitPath, }, @@ -35,8 +35,9 @@ use syn::{ parse::{Error, Parse, ParseStream, Result}, parse_macro_input, parse_quote, spanned::Spanned, - visit::{self, Visit}, - Attribute, Ident, ImplItem, ItemImpl, LitInt, LitStr, Path, Signature, Type, TypePath, + visit_mut::{self, VisitMut}, + Attribute, GenericArgument, Ident, ImplItem, ItemImpl, LitInt, LitStr, + ParenthesizedGenericArguments, Path, Signature, Type, TypePath, }; use std::collections::HashMap; @@ -228,34 +229,34 @@ fn generate_wasm_interface(impls: &[ItemImpl]) -> Result { let c = generate_crate_access(); let impl_calls = - generate_impl_calls(impls, &input)? - .into_iter() - .map(|(trait_, fn_name, impl_, attrs)| { - let fn_name = - Ident::new(&prefix_function_with_trait(&trait_, &fn_name), Span::call_site()); - - quote!( - #c::std_disabled! { - #( #attrs )* - #[no_mangle] - #[cfg_attr(any(target_arch = "riscv32", target_arch = "riscv64"), #c::__private::polkavm_export(abi = #c::__private::polkavm_abi))] - pub unsafe extern fn #fn_name(input_data: *mut u8, input_len: usize) -> u64 { - let mut #input = if input_len == 0 { - &[0u8; 0] - } else { - unsafe { - ::core::slice::from_raw_parts(input_data, input_len) - } - }; - - #c::init_runtime_logger(); - - let output = (move || { #impl_ })(); - #c::to_substrate_wasm_fn_return_value(&output) - } - } - ) - }); + generate_impl_calls(impls, &input)? + .into_iter() + .map(|(trait_, fn_name, impl_, attrs)| { + let fn_name = + Ident::new(&prefix_function_with_trait(&trait_, &fn_name), Span::call_site()); + + quote!( + #c::std_disabled! { + #( #attrs )* + #[no_mangle] + #[cfg_attr(any(target_arch = "riscv32", target_arch = "riscv64"), #c::__private::polkavm_export(abi = #c::__private::polkavm_abi))] + pub unsafe extern fn #fn_name(input_data: *mut u8, input_len: usize) -> u64 { + let mut #input = if input_len == 0 { + &[0u8; 0] + } else { + unsafe { + ::core::slice::from_raw_parts(input_data, input_len) + } + }; + + #c::init_runtime_logger(); + + let output = (move || { #impl_ })(); + #c::to_substrate_wasm_fn_return_value(&output) + } + } + ) + }); Ok(quote!( #( #impl_calls )* )) } @@ -397,10 +398,10 @@ fn generate_runtime_api_base_structures() -> Result { impl> RuntimeApiImpl { fn commit_or_rollback_transaction(&self, commit: bool) { let proof = "\ - We only close a transaction when we opened one ourself. - Other parts of the runtime that make use of transactions (state-machine) - also balance their transactions. The runtime cannot close client initiated - transactions; qed"; + We only close a transaction when we opened one ourself. + Other parts of the runtime that make use of transactions (state-machine) + also balance their transactions. The runtime cannot close client initiated + transactions; qed"; let res = if commit { let res = if let Some(recorder) = &self.recorder { @@ -741,8 +742,8 @@ fn generate_runtime_api_versions(impls: &[ItemImpl]) -> Result { let mut error = Error::new( span, "Two traits with the same name detected! \ - The trait name is used to generate its ID. \ - Please rename one trait at the declaration!", + The trait name is used to generate its ID. \ + Please rename one trait at the declaration!", ); error.combine(Error::new(other_span, "First trait implementation.")); @@ -788,61 +789,49 @@ fn generate_runtime_api_versions(impls: &[ItemImpl]) -> Result { )) } -/// Checks that a trait implementation is in the format we expect. -struct CheckTraitImpl { - errors: Vec, -} +/// replaces `Self`` with explicit `Runtime`. +struct ReplaceSelfImpl {} -impl CheckTraitImpl { - /// Check the given trait implementation. - /// - /// All errors will be collected in `self.errors`. - fn check(&mut self, trait_: &ItemImpl) { - visit::visit_item_impl(self, trait_) +impl ReplaceSelfImpl { + /// Replace `Self` with `Runtime` + fn replace(&mut self, trait_: &mut ItemImpl) { + visit_mut::visit_item_impl_mut(self, trait_) } } -impl<'ast> Visit<'ast> for CheckTraitImpl { - fn visit_type_path(&mut self, i: &'ast syn::TypePath) { - if extract_angle_bracketed_idents_from_type_path(i).iter().any(|i| *i == "Self") { - self.errors.push(Error::new( - i.span(), - "`Self` can not be used as a type argument in the scope of `impl_runtime_apis!`. Use `Runtime` instead.", - )); +impl VisitMut for ReplaceSelfImpl { + fn visit_generic_argument_mut(&mut self, i: &mut syn::GenericArgument) { + if let GenericArgument::Type(Type::Path(p)) = i { + if let Some(ident) = p.path.get_ident() { + if ident == "Self" { + p.path = parse_quote! { Runtime }; + }; + } } - - visit::visit_type_path(self, i) } } -/// Check all trait implementations are in the format we expect. -fn check_trait_impls(impls: &[ItemImpl]) -> Result<()> { - let mut checker = CheckTraitImpl { errors: Vec::new() }; +/// Rename `Self` to `Runtime` in all items. +fn rename_self_in_trait_impls(impls: &mut [ItemImpl]) -> Result<()> { + let mut checker = ReplaceSelfImpl {}; - impls.iter().for_each(|i| checker.check(i)); + impls.iter_mut().for_each(|i| checker.replace(i)); - if let Some(err) = checker.errors.pop() { - Err(checker.errors.into_iter().fold(err, |mut acc, e| { - acc.combine(e); - acc - })) - } else { - Ok(()) - } + Ok(()) } /// The implementation of the `impl_runtime_apis!` macro. pub fn impl_runtime_apis_impl(input: proc_macro::TokenStream) -> proc_macro::TokenStream { // Parse all impl blocks - let RuntimeApiImpls { impls: api_impls } = parse_macro_input!(input as RuntimeApiImpls); + let RuntimeApiImpls { impls: mut api_impls } = parse_macro_input!(input as RuntimeApiImpls); - impl_runtime_apis_impl_inner(&api_impls) + impl_runtime_apis_impl_inner(&mut api_impls) .unwrap_or_else(|e| e.to_compile_error()) .into() } -fn impl_runtime_apis_impl_inner(api_impls: &[ItemImpl]) -> Result { - check_trait_impls(api_impls)?; +fn impl_runtime_apis_impl_inner(api_impls: &mut [ItemImpl]) -> Result { + rename_self_in_trait_impls(api_impls)?; let dispatch_impl = generate_dispatch_function(api_impls)?; let api_impls_for_runtime = generate_api_impl_for_runtime(api_impls)?; @@ -956,7 +945,7 @@ fn extract_api_version(attrs: &Vec, span: Span) -> Result span, format!( "Found multiple #[{}] attributes for an API implementation. \ - Each runtime API can have only one version.", + Each runtime API can have only one version.", API_VERSION_ATTRIBUTE ), )); diff --git a/substrate/primitives/api/proc-macro/src/utils.rs b/substrate/primitives/api/proc-macro/src/utils.rs index b9a1ccb0f56d..7ecf70843dab 100644 --- a/substrate/primitives/api/proc-macro/src/utils.rs +++ b/substrate/primitives/api/proc-macro/src/utils.rs @@ -20,7 +20,6 @@ use inflector::Inflector; use proc_macro2::{Span, TokenStream}; use proc_macro_crate::{crate_name, FoundCrate}; use quote::{format_ident, quote}; -use std::collections::HashSet; use syn::{ parse_quote, punctuated::Punctuated, spanned::Spanned, token::And, Attribute, Error, Expr, ExprLit, FnArg, GenericArgument, Ident, ItemImpl, Lit, Meta, MetaNameValue, Pat, Path, @@ -216,25 +215,6 @@ pub fn extract_impl_trait(impl_: &ItemImpl, require: RequireQualifiedTraitPath) }) } -/// Extracts all unique `Ident`'s from the given `TypePath` -pub fn extract_angle_bracketed_idents_from_type_path(type_path: &TypePath) -> HashSet<&Ident> { - let mut idents = HashSet::new(); - - for segment in &type_path.path.segments { - if let PathArguments::AngleBracketed(args) = &segment.arguments { - args.args.iter().for_each(|arg| { - if let GenericArgument::Type(Type::Path(p)) = arg { - if let Some(ident) = p.path.get_ident() { - idents.insert(ident); - } - } - }); - } - } - - idents -} - /// Parse the given attribute as `API_VERSION_ATTRIBUTE`. pub fn parse_runtime_api_version(version: &Attribute) -> Result { let version = version.parse_args::().map_err(|_| { @@ -401,15 +381,6 @@ mod tests { assert_eq!(cfg_benchmarks, filtered[1]); } - #[test] - fn check_extract_angle_bracketed_idents_from_type_path() { - let type_path: TypePath = parse_quote!(polkadot_primitives::types::HeaderFor); - let idents = extract_angle_bracketed_idents_from_type_path(&type_path); - - assert_eq!(idents.len(), 1); - assert!(idents.contains(&&format_ident!("Self"))); - } - fn check_deprecated_attr() { const FIRST: &'static str = "hello"; const SECOND: &'static str = "WORLD"; diff --git a/substrate/primitives/api/test/Cargo.toml b/substrate/primitives/api/test/Cargo.toml index 1d21f23eb804..b19100261c4d 100644 --- a/substrate/primitives/api/test/Cargo.toml +++ b/substrate/primitives/api/test/Cargo.toml @@ -27,6 +27,10 @@ sp-state-machine = { workspace = true, default-features = true } trybuild = { workspace = true } rustversion = { workspace = true } scale-info = { features = ["derive"], workspace = true } +quote = { workspace = true } +syn = { features = ["extra-traits", "fold", "full", "visit"], workspace = true } +proc-macro2 = { workspace = true } +sp-api-proc-macro = { workspace = true } [dev-dependencies] criterion = { workspace = true, default-features = true } From eb62a316d8cc365a188c31f4b42a01a8f4f573a6 Mon Sep 17 00:00:00 2001 From: Pavlo Khrystenko Date: Wed, 23 Oct 2024 00:47:54 +0200 Subject: [PATCH 10/23] revert cargo lock change --- Cargo.lock | 4 ---- substrate/primitives/api/test/Cargo.toml | 4 ---- 2 files changed, 8 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 48ce859a9596..c8f828e85e6a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -21380,13 +21380,10 @@ dependencies = [ "futures", "log", "parity-scale-codec", - "proc-macro2 1.0.86", - "quote 1.0.37", "rustversion", "sc-block-builder", "scale-info", "sp-api 26.0.0", - "sp-api-proc-macro 15.0.0", "sp-consensus", "sp-core 28.0.0", "sp-runtime 31.0.1", @@ -21395,7 +21392,6 @@ dependencies = [ "sp-version 29.0.0", "static_assertions", "substrate-test-runtime-client", - "syn 2.0.79", "trybuild", ] diff --git a/substrate/primitives/api/test/Cargo.toml b/substrate/primitives/api/test/Cargo.toml index b19100261c4d..1d21f23eb804 100644 --- a/substrate/primitives/api/test/Cargo.toml +++ b/substrate/primitives/api/test/Cargo.toml @@ -27,10 +27,6 @@ sp-state-machine = { workspace = true, default-features = true } trybuild = { workspace = true } rustversion = { workspace = true } scale-info = { features = ["derive"], workspace = true } -quote = { workspace = true } -syn = { features = ["extra-traits", "fold", "full", "visit"], workspace = true } -proc-macro2 = { workspace = true } -sp-api-proc-macro = { workspace = true } [dev-dependencies] criterion = { workspace = true, default-features = true } From 1339e3c813ad7940c3102193158d23f2726e112f Mon Sep 17 00:00:00 2001 From: Pavlo Khrystenko Date: Wed, 23 Oct 2024 01:13:55 +0200 Subject: [PATCH 11/23] "Runtime" -> "ItemImpl.self_ty" --- .../api/proc-macro/src/impl_runtime_apis.rs | 35 +++++++++++-------- 1 file changed, 20 insertions(+), 15 deletions(-) diff --git a/substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs b/substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs index 38a8e866dcf5..bd7e5ab16528 100644 --- a/substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs +++ b/substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs @@ -36,8 +36,8 @@ use syn::{ parse_macro_input, parse_quote, spanned::Spanned, visit_mut::{self, VisitMut}, - Attribute, GenericArgument, Ident, ImplItem, ItemImpl, LitInt, LitStr, - ParenthesizedGenericArguments, Path, Signature, Type, TypePath, + Attribute, GenericArgument, Ident, ImplItem, ItemImpl, LitInt, LitStr, Path, Signature, Type, + TypePath, }; use std::collections::HashMap; @@ -789,11 +789,13 @@ fn generate_runtime_api_versions(impls: &[ItemImpl]) -> Result { )) } -/// replaces `Self`` with explicit `Runtime`. -struct ReplaceSelfImpl {} +/// replaces `Self` with explicit `ItemImpl.self_ty`. +struct ReplaceSelfImpl { + self_ty: Box, +} impl ReplaceSelfImpl { - /// Replace `Self` with `Runtime` + /// Replace `Self` with `ItemImpl.self_ty` fn replace(&mut self, trait_: &mut ItemImpl) { visit_mut::visit_item_impl_mut(self, trait_) } @@ -801,21 +803,24 @@ impl ReplaceSelfImpl { impl VisitMut for ReplaceSelfImpl { fn visit_generic_argument_mut(&mut self, i: &mut syn::GenericArgument) { - if let GenericArgument::Type(Type::Path(p)) = i { - if let Some(ident) = p.path.get_ident() { - if ident == "Self" { - p.path = parse_quote! { Runtime }; - }; - } + let typ = match i { + GenericArgument::Type(Type::Path(p)) + if p.path.get_ident().is_some_and(|ident| ident == "Self") => + Some(GenericArgument::Type(*self.self_ty.clone())), + _ => None, + }; + if let Some(typ) = typ { + *i = typ; } } } -/// Rename `Self` to `Runtime` in all items. +/// Rename `Self` to `ItemImpl.self_ty` in all items. fn rename_self_in_trait_impls(impls: &mut [ItemImpl]) -> Result<()> { - let mut checker = ReplaceSelfImpl {}; - - impls.iter_mut().for_each(|i| checker.replace(i)); + impls.iter_mut().for_each(|i| { + let mut checker = ReplaceSelfImpl { self_ty: i.self_ty.clone() }; + checker.replace(i); + }); Ok(()) } From e7785526f45d228ff7b9221d342d564408971000 Mon Sep 17 00:00:00 2001 From: Pavlo Khrystenko Date: Thu, 24 Oct 2024 11:24:30 +0200 Subject: [PATCH 12/23] add test for renamer --- .../api/proc-macro/src/impl_runtime_apis.rs | 30 +++++++++++++++++++ .../primitives/api/proc-macro/src/utils.rs | 1 + 2 files changed, 31 insertions(+) diff --git a/substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs b/substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs index bd7e5ab16528..d7399e8f4299 100644 --- a/substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs +++ b/substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs @@ -985,4 +985,34 @@ mod tests { assert_eq!(cfg_std, filtered[0]); assert_eq!(cfg_benchmarks, filtered[1]); } + + #[test] + fn impl_trait_rename_self_param() { + let code = quote::quote! { + impl client::Core for Runtime { + fn initialize_block(header: &HeaderFor) -> Output { + let _: HeaderFor = header.clone(); + example_fn::(header) + } + } + }; + let expected = quote::quote! { + impl client::Core for Runtime { + fn initialize_block(header: &HeaderFor) -> Output { + let _: HeaderFor = header.clone(); + example_fn::(header) + } + } + }; + + // Parse the items + let RuntimeApiImpls { impls: mut api_impls } = + syn::parse2::(code).unwrap(); + + // Run the renamer which is being run first in the `impl_runtime_apis!` macro. + rename_self_in_trait_impls(&mut api_impls); + let result: TokenStream = quote::quote! { #(#api_impls)* }; + + assert_eq!(result.to_string(), expected.to_string()); + } } diff --git a/substrate/primitives/api/proc-macro/src/utils.rs b/substrate/primitives/api/proc-macro/src/utils.rs index 7ecf70843dab..dc993c2ac420 100644 --- a/substrate/primitives/api/proc-macro/src/utils.rs +++ b/substrate/primitives/api/proc-macro/src/utils.rs @@ -381,6 +381,7 @@ mod tests { assert_eq!(cfg_benchmarks, filtered[1]); } + #[test] fn check_deprecated_attr() { const FIRST: &'static str = "hello"; const SECOND: &'static str = "WORLD"; From 8ff5005c5d4913c4dd70ca611e4c2a2b99cbf1a1 Mon Sep 17 00:00:00 2001 From: Pavlo Khrystenko Date: Mon, 28 Oct 2024 11:19:55 +0100 Subject: [PATCH 13/23] pr-doc --- prdoc/pr_4012.prdoc | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/prdoc/pr_4012.prdoc b/prdoc/pr_4012.prdoc index c0288b79bdb8..3385a7f2d424 100644 --- a/prdoc/pr_4012.prdoc +++ b/prdoc/pr_4012.prdoc @@ -1,18 +1,18 @@ # 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: "`impl_runtime_apis!`: prevent the use of `Self` as type argument" +title: "`impl_runtime_apis!`: replace the use of `Self` with `Runtime`" doc: - audience: Runtime Dev description: | Currently, if there is a type alias similar to `type HeaderFor` in the scope, it makes sense to expect that `HeaderFor` and `HeaderFor` are equivalent. However, this is not the case. It currently leads to - a compilation error that `Self is not in scope`, which is confusing. This PR Introduces a checker struct, similar to - `CheckTraitDecl` in `decl_runtime_apis!`, `CheckTraitImpl`. It identifies usage of `Self` as a type argument in - `impl_runtime_apis!` and emits an error message. The error message suggests replacing `Self` with `Runtime`. + a compilation error that `Self is not in scope`, which is confusing. This PR Introduces a visitor, similar to + `CheckTraitDecl` in `decl_runtime_apis!`, `ReplaceSelfImpl`. It identifies usage of `Self` as a type argument in + `impl_runtime_apis!` and replaces `Self` with an explicit `Runtime` type. - For example, the following example code is not allowed: + For example, the following example code will be transformed before expansion: ```rust impl apis::Core for Runtime { fn initialize_block(header: &HeaderFor) -> ExtrinsicInclusionMode { @@ -21,7 +21,7 @@ doc: } } ``` - Instead, it should be written as: + Instead, it will be passed to macro as: ```rust impl apis::Core for Runtime { fn initialize_block(header: &HeaderFor) -> ExtrinsicInclusionMode { From 0828b6c5e8a5fa980a4130b58af0ce558aec27e2 Mon Sep 17 00:00:00 2001 From: Pavlo Khrystenko Date: Mon, 28 Oct 2024 12:01:54 +0100 Subject: [PATCH 14/23] remove prdoc --- prdoc/pr_4012.prdoc | 35 ----------------------------------- 1 file changed, 35 deletions(-) delete mode 100644 prdoc/pr_4012.prdoc diff --git a/prdoc/pr_4012.prdoc b/prdoc/pr_4012.prdoc deleted file mode 100644 index 3385a7f2d424..000000000000 --- a/prdoc/pr_4012.prdoc +++ /dev/null @@ -1,35 +0,0 @@ -# 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: "`impl_runtime_apis!`: replace the use of `Self` with `Runtime`" - -doc: - - audience: Runtime Dev - description: | - Currently, if there is a type alias similar to `type HeaderFor` in the scope, it makes sense to expect that - `HeaderFor` and `HeaderFor` are equivalent. However, this is not the case. It currently leads to - a compilation error that `Self is not in scope`, which is confusing. This PR Introduces a visitor, similar to - `CheckTraitDecl` in `decl_runtime_apis!`, `ReplaceSelfImpl`. It identifies usage of `Self` as a type argument in - `impl_runtime_apis!` and replaces `Self` with an explicit `Runtime` type. - - For example, the following example code will be transformed before expansion: - ```rust - impl apis::Core for Runtime { - fn initialize_block(header: &HeaderFor) -> ExtrinsicInclusionMode { - let _: HeaderFor = header.clone(); - RuntimeExecutive::initialize_block(header) - } - } - ``` - Instead, it will be passed to macro as: - ```rust - impl apis::Core for Runtime { - fn initialize_block(header: &HeaderFor) -> ExtrinsicInclusionMode { - let _: HeaderFor = header.clone(); - RuntimeExecutive::initialize_block(header) - } - } - ``` -crates: - - name: sp-api - bump: minor From a736e4fad9f46ae9a1971dd239c62e2bbb28e9ea Mon Sep 17 00:00:00 2001 From: Pavlo Khrystenko Date: Mon, 28 Oct 2024 12:41:26 +0100 Subject: [PATCH 15/23] bring back prdoc --- prdoc/pr_4012.prdoc | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) create mode 100644 prdoc/pr_4012.prdoc diff --git a/prdoc/pr_4012.prdoc b/prdoc/pr_4012.prdoc new file mode 100644 index 000000000000..ef853dce6a11 --- /dev/null +++ b/prdoc/pr_4012.prdoc @@ -0,0 +1,35 @@ +# 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: "`impl_runtime_apis!`: replace the use of `Self` with `Runtime`" + +doc: + - audience: Runtime Dev + description: | + Currently, if there is a type alias similar to `type HeaderFor` in the scope, it makes sense to expect that + `HeaderFor` and `HeaderFor` are equivalent. However, this is not the case. It currently leads to + a compilation error that `Self is not in scope`, which is confusing. This PR Introduces a visitor, similar to + `CheckTraitDecl` in `decl_runtime_apis!`, `ReplaceSelfImpl`. It identifies usage of `Self` as a type argument in + `impl_runtime_apis!` and replaces `Self` with an explicit `Runtime` type. + + For example, the following example code will be transformed before expansion: + ```rust + impl apis::Core for Runtime { + fn initialize_block(header: &HeaderFor) -> ExtrinsicInclusionMode { + let _: HeaderFor = header.clone(); + RuntimeExecutive::initialize_block(header) + } + } + ``` + Instead, it will be passed to macro as: + ```rust + impl apis::Core for Runtime { + fn initialize_block(header: &HeaderFor) -> ExtrinsicInclusionMode { + let _: HeaderFor = header.clone(); + RuntimeExecutive::initialize_block(header) + } + } + ``` +crates: + - name: sp-api + bump: none \ No newline at end of file From 3d4d25f788b568eb5b59adc263563f9f4e1c4d3d Mon Sep 17 00:00:00 2001 From: Pavlo Khrystenko Date: Mon, 28 Oct 2024 12:43:41 +0100 Subject: [PATCH 16/23] clippy --- substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs b/substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs index d7399e8f4299..b9ecbd612dd3 100644 --- a/substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs +++ b/substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs @@ -1010,7 +1010,7 @@ mod tests { syn::parse2::(code).unwrap(); // Run the renamer which is being run first in the `impl_runtime_apis!` macro. - rename_self_in_trait_impls(&mut api_impls); + rename_self_in_trait_impls(&mut api_impls).expect("Will succeed"); let result: TokenStream = quote::quote! { #(#api_impls)* }; assert_eq!(result.to_string(), expected.to_string()); From 0d0f89f07cb973f53bbe3dfa27d4b9dcbf0fc37a Mon Sep 17 00:00:00 2001 From: Pavlo Khrystenko Date: Mon, 28 Oct 2024 12:54:49 +0100 Subject: [PATCH 17/23] add prdoc-change --- prdoc/pr_4012.prdoc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/prdoc/pr_4012.prdoc b/prdoc/pr_4012.prdoc index ef853dce6a11..4383907031e2 100644 --- a/prdoc/pr_4012.prdoc +++ b/prdoc/pr_4012.prdoc @@ -32,4 +32,6 @@ doc: ``` crates: - name: sp-api + bump: none + - name: sp-api-proc-macro bump: none \ No newline at end of file From b5ed2cf73d62f513b3446e2ae7797b90beb5d172 Mon Sep 17 00:00:00 2001 From: Pavlo Khrystenko <45178695+pkhry@users.noreply.github.com> Date: Thu, 31 Oct 2024 16:03:09 +0100 Subject: [PATCH 18/23] Update prdoc/pr_4012.prdoc Co-authored-by: Alexandru Vasile <60601340+lexnv@users.noreply.github.com> --- prdoc/pr_4012.prdoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/prdoc/pr_4012.prdoc b/prdoc/pr_4012.prdoc index 4383907031e2..3a53e31a7fc6 100644 --- a/prdoc/pr_4012.prdoc +++ b/prdoc/pr_4012.prdoc @@ -8,7 +8,7 @@ doc: description: | Currently, if there is a type alias similar to `type HeaderFor` in the scope, it makes sense to expect that `HeaderFor` and `HeaderFor` are equivalent. However, this is not the case. It currently leads to - a compilation error that `Self is not in scope`, which is confusing. This PR Introduces a visitor, similar to + a compilation error that `Self is not in scope`, which is confusing. This PR introduces a visitor, similar to `CheckTraitDecl` in `decl_runtime_apis!`, `ReplaceSelfImpl`. It identifies usage of `Self` as a type argument in `impl_runtime_apis!` and replaces `Self` with an explicit `Runtime` type. From b7eec209f8db5b3d145de5fb4fefaeea83bc0238 Mon Sep 17 00:00:00 2001 From: Pavlo Khrystenko Date: Thu, 31 Oct 2024 16:12:06 +0100 Subject: [PATCH 19/23] review comments --- .../api/proc-macro/src/impl_runtime_apis.rs | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs b/substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs index b9ecbd612dd3..c8a96d367867 100644 --- a/substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs +++ b/substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs @@ -7,7 +7,7 @@ // 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 +// 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, @@ -803,14 +803,10 @@ impl ReplaceSelfImpl { impl VisitMut for ReplaceSelfImpl { fn visit_generic_argument_mut(&mut self, i: &mut syn::GenericArgument) { - let typ = match i { - GenericArgument::Type(Type::Path(p)) - if p.path.get_ident().is_some_and(|ident| ident == "Self") => - Some(GenericArgument::Type(*self.self_ty.clone())), - _ => None, - }; - if let Some(typ) = typ { - *i = typ; + let GenericArgument::Type(Type::Path(p)) = i else { return }; + + if p.path.get_ident().is_some_and(|ident| ident == "Self") { + *i = GenericArgument::Type(*self.self_ty.clone()); } } } From b05cf24b461639282b15e09ad9f14b7ba706d869 Mon Sep 17 00:00:00 2001 From: Pavlo Khrystenko <45178695+pkhry@users.noreply.github.com> Date: Tue, 5 Nov 2024 16:54:35 +0100 Subject: [PATCH 20/23] Update substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Bastian Köcher --- .../api/proc-macro/src/impl_runtime_apis.rs | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs b/substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs index c8a96d367867..0189e7057ca2 100644 --- a/substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs +++ b/substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs @@ -802,12 +802,11 @@ impl ReplaceSelfImpl { } impl VisitMut for ReplaceSelfImpl { - fn visit_generic_argument_mut(&mut self, i: &mut syn::GenericArgument) { - let GenericArgument::Type(Type::Path(p)) = i else { return }; - - if p.path.get_ident().is_some_and(|ident| ident == "Self") { - *i = GenericArgument::Type(*self.self_ty.clone()); - } + fn visit_type_mut(&mut self, ty: &mut syn::Type) { + match ty { + Type::Path(p) if p.path.is_ident("Self") => { *ty = self.self_ty.clone(); }, + ty => syn::visit_mut::visit_type_mut(self, ty), + } } } From d425328152ea30ef2cccc383855987324926e4eb Mon Sep 17 00:00:00 2001 From: Pavlo Khrystenko <45178695+pkhry@users.noreply.github.com> Date: Tue, 5 Nov 2024 16:54:44 +0100 Subject: [PATCH 21/23] Update substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Bastian Köcher --- substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs b/substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs index 0189e7057ca2..b40972b91cf6 100644 --- a/substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs +++ b/substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs @@ -811,13 +811,11 @@ impl VisitMut for ReplaceSelfImpl { } /// Rename `Self` to `ItemImpl.self_ty` in all items. -fn rename_self_in_trait_impls(impls: &mut [ItemImpl]) -> Result<()> { +fn rename_self_in_trait_impls(impls: &mut [ItemImpl]) { impls.iter_mut().for_each(|i| { let mut checker = ReplaceSelfImpl { self_ty: i.self_ty.clone() }; checker.replace(i); }); - - Ok(()) } /// The implementation of the `impl_runtime_apis!` macro. From da66f7b6896095ebc00a1a0bc6da6582e70a9bf8 Mon Sep 17 00:00:00 2001 From: Pavlo Khrystenko Date: Wed, 6 Nov 2024 12:39:17 +0100 Subject: [PATCH 22/23] suggestions fixup --- .../primitives/api/proc-macro/src/impl_runtime_apis.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs b/substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs index b40972b91cf6..9701872c1cdb 100644 --- a/substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs +++ b/substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs @@ -36,7 +36,7 @@ use syn::{ parse_macro_input, parse_quote, spanned::Spanned, visit_mut::{self, VisitMut}, - Attribute, GenericArgument, Ident, ImplItem, ItemImpl, LitInt, LitStr, Path, Signature, Type, + Attribute, Ident, ImplItem, ItemImpl, LitInt, LitStr, Path, Signature, Type, TypePath, }; @@ -804,9 +804,11 @@ impl ReplaceSelfImpl { impl VisitMut for ReplaceSelfImpl { fn visit_type_mut(&mut self, ty: &mut syn::Type) { match ty { - Type::Path(p) if p.path.is_ident("Self") => { *ty = self.self_ty.clone(); }, + Type::Path(p) if p.path.is_ident("Self") => { + *ty = *self.self_ty.clone(); + }, ty => syn::visit_mut::visit_type_mut(self, ty), - } + } } } @@ -829,7 +831,7 @@ pub fn impl_runtime_apis_impl(input: proc_macro::TokenStream) -> proc_macro::Tok } fn impl_runtime_apis_impl_inner(api_impls: &mut [ItemImpl]) -> Result { - rename_self_in_trait_impls(api_impls)?; + rename_self_in_trait_impls(api_impls); let dispatch_impl = generate_dispatch_function(api_impls)?; let api_impls_for_runtime = generate_api_impl_for_runtime(api_impls)?; From ec2579550a7be707a3372fce9df828eb5be3f3c9 Mon Sep 17 00:00:00 2001 From: Pavlo Khrystenko Date: Wed, 6 Nov 2024 13:42:00 +0100 Subject: [PATCH 23/23] clippy --- substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs b/substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs index 852f63c4f4ef..5c9448da2bc7 100644 --- a/substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs +++ b/substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs @@ -918,7 +918,7 @@ mod tests { syn::parse2::(code).unwrap(); // Run the renamer which is being run first in the `impl_runtime_apis!` macro. - rename_self_in_trait_impls(&mut api_impls).expect("Will succeed"); + rename_self_in_trait_impls(&mut api_impls); let result: TokenStream = quote::quote! { #(#api_impls)* }; assert_eq!(result.to_string(), expected.to_string());