From 891e1da80848069d893b3b5bee0430259c4f495d Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Wed, 2 Aug 2023 10:59:18 +0300 Subject: [PATCH 01/21] Handle `cfg_attr` in `decl_runtime_api` --- .../api/proc-macro/src/impl_runtime_apis.rs | 248 ++++++++++++++++-- 1 file changed, 230 insertions(+), 18 deletions(-) diff --git a/primitives/api/proc-macro/src/impl_runtime_apis.rs b/primitives/api/proc-macro/src/impl_runtime_apis.rs index cb683b664495c..f984f2e90e3ba 100644 --- a/primitives/api/proc-macro/src/impl_runtime_apis.rs +++ b/primitives/api/proc-macro/src/impl_runtime_apis.rs @@ -31,10 +31,12 @@ use quote::quote; use syn::{ fold::{self, Fold}, + parenthesized, parse::{Error, Parse, ParseStream, Result}, parse_macro_input, parse_quote, spanned::Spanned, - Attribute, Ident, ImplItem, ItemImpl, Path, Signature, Type, TypePath, + Attribute, Ident, ImplItem, ImplItemFn, ItemImpl, LitInt, LitStr, Path, Signature, Type, + TypePath, }; use std::collections::HashSet; @@ -67,6 +69,7 @@ fn generate_impl_call( runtime: &Type, input: &Ident, impl_trait: &Path, + api_version: &ApiVersion, ) -> Result { let params = extract_parameter_names_types_and_borrows(signature, AllowSelfRefInParameters::No)?; @@ -111,11 +114,40 @@ fn generate_impl_call( ) }; + let fn_calls = if let Some(feature_gated) = &api_version.feature_gated { + let pnames = pnames2; + let pnames2 = pnames.clone(); + let pborrow2 = pborrow.clone(); + + let feature_name = &feature_gated.0; + let impl_trait_fg = extend_with_api_version(impl_trait.clone(), Some(feature_gated.1)); + let impl_trait = extend_with_api_version(impl_trait.clone(), api_version.custom); + + quote!( + #[cfg(feature = #feature_name)] + #[allow(deprecated)] + let r = <#runtime as #impl_trait_fg>::#fn_name(#( #pborrow #pnames ),*); + + #[cfg(not(feature = #feature_name))] + #[allow(deprecated)] + let r = <#runtime as #impl_trait>::#fn_name(#( #pborrow2 #pnames2 ),*); + + r + ) + } else { + let pnames = pnames2; + let impl_trait = extend_with_api_version(impl_trait.clone(), api_version.custom); + + quote!( + #[allow(deprecated)] + <#runtime as #impl_trait>::#fn_name(#( #pborrow #pnames ),*) + ) + }; + Ok(quote!( #decode_params - #[allow(deprecated)] - <#runtime as #impl_trait>::#fn_name(#( #pborrow #pnames2 ),*) + #fn_calls )) } @@ -130,7 +162,6 @@ fn generate_impl_calls( let trait_api_ver = extract_api_version(&impl_.attrs, impl_.span())?; let impl_trait_path = extract_impl_trait(impl_, RequireQualifiedTraitPath::Yes)?; let impl_trait = extend_with_runtime_decl_path(impl_trait_path.clone()); - let impl_trait = extend_with_api_version(impl_trait, trait_api_ver); let impl_trait_ident = &impl_trait_path .segments .last() @@ -139,14 +170,28 @@ fn generate_impl_calls( for item in &impl_.items { if let ImplItem::Fn(method) = item { - let impl_call = - generate_impl_call(&method.sig, &impl_.self_ty, input, &impl_trait)?; + let impl_call = generate_impl_call( + &method.sig, + &impl_.self_ty, + input, + &impl_trait, + &trait_api_ver, + )?; + let mut attrs = filter_cfg_attrs(&impl_.attrs); + + let method_api_version = extract_api_version(&method.attrs, method.span())?; + if method_api_version.custom.is_some() { + return Err(Error::new(method.span(), "`api_version` attribute is not allowed `decl_runtime_api` method implementations")) + } + if let Some(feature_gated) = method_api_version.feature_gated { + add_feature_guard(&mut attrs, &feature_gated.0, true) + } impl_calls.push(( impl_trait_ident.clone(), method.sig.ident.clone(), impl_call, - filter_cfg_attrs(&impl_.attrs), + attrs, )); } } @@ -448,6 +493,74 @@ fn extend_with_api_version(mut trait_: Path, version: Option) -> Path { trait_ } +// Adds a `#[cfg( feature = ...)]` attribute to attributes. Parameters: +// feature_name - name of the feature +// t - positive or negative feature guard. +// true => `#[cfg(feature = ...)]` +// false => `#[cfg(not(feature = ...))]` +fn add_feature_guard(attrs: &mut Vec, feature_name: &str, t: bool) { + let attr = match t { + true => parse_quote!(#[cfg(feature = #feature_name)]), + false => parse_quote!(#[cfg(not(feature = #feature_name))]), + }; + attrs.push(attr); +} + +// Fold implementation which processes function implementations in impl block for a trait. It is +// used to process `cfg_attr`s related to staging methods (e.g. `#[cfg_attr(feature = +// "enable-staging-api", api_version(99))]`). Replaces the `cfg_attr` attribute with a feature guard +// so that staging methods are added only for staging api implementations. +struct ApiImplItem { + errors: Vec, +} + +impl ApiImplItem { + pub fn new() -> ApiImplItem { + ApiImplItem { errors: Vec::new() } + } + + pub fn process(&mut self, item: ItemImpl) -> Result { + let res = self.fold_item_impl(item); + + if let Some(e) = self.errors.pop() { + // TODO: what to do with the rest of the errors? + return Err(e) + } + + Ok(res) + } +} + +impl Fold for ApiImplItem { + fn fold_impl_item_fn(&mut self, mut i: ImplItemFn) -> ImplItemFn { + let v = extract_api_version(&i.attrs, i.span()); + + if let Err(e) = v { + self.errors.push(e); + return fold::fold_impl_item_fn(self, i) + } + + let v = v + .expect("Explicitly checked for and handled the error case on the previous line. qed."); + + if v.custom.is_some() { + self.errors.push(Error::new( + i.span(), + "`api_version` attribute is not allowed `decl_runtime_api` method implementations", + )); + return fold::fold_impl_item_fn(self, i) + } + + if let Some(v) = v.feature_gated { + add_feature_guard(&mut i.attrs, &v.0, true); + } + + i.attrs = filter_cfg_attrs(&i.attrs); + + fold::fold_impl_item_fn(self, i) + } +} + /// Generates the implementations of the apis for the runtime. fn generate_api_impl_for_runtime(impls: &[ItemImpl]) -> Result { let mut impls_prepared = Vec::new(); @@ -458,12 +571,31 @@ fn generate_api_impl_for_runtime(impls: &[ItemImpl]) -> Result { let trait_api_ver = extract_api_version(&impl_.attrs, impl_.span())?; let mut impl_ = impl_.clone(); + impl_.attrs = filter_cfg_attrs(&impl_.attrs); + // Process all method implementations add add feature gates where necessary + let mut impl_ = ApiImplItem::new().process(impl_)?; + let trait_ = extract_impl_trait(&impl_, RequireQualifiedTraitPath::Yes)?.clone(); let trait_ = extend_with_runtime_decl_path(trait_); - let trait_ = extend_with_api_version(trait_, trait_api_ver); + // If the trait api contains feature gated version - there are staging methods in it. Handle + // them explicitly here by adding staging implementation with `#cfg(feature = ...)` and + // stable implementation with `#[cfg(not(feature = ...))]`. + if let Some(feature_gated) = trait_api_ver.feature_gated { + let mut feature_gated_impl = impl_.clone(); + add_feature_guard(&mut feature_gated_impl.attrs, &feature_gated.0, true); + feature_gated_impl.trait_.as_mut().unwrap().1 = + extend_with_api_version(trait_.clone(), Some(feature_gated.1)); + + impls_prepared.push(feature_gated_impl); + + // Finally add `#[cfg(not(feature = ...))]` for the stable implementation (which is + // generated outside this if). + add_feature_guard(&mut impl_.attrs, &feature_gated.0, false); + } + // Generate stable trait implementation. + let trait_ = extend_with_api_version(trait_, trait_api_ver.custom); impl_.trait_.as_mut().unwrap().1 = trait_; - impl_.attrs = filter_cfg_attrs(&impl_.attrs); impls_prepared.push(impl_); } @@ -671,7 +803,8 @@ fn generate_runtime_api_versions(impls: &[ItemImpl]) -> Result { let c = generate_crate_access(); for impl_ in impls { - let api_ver = extract_api_version(&impl_.attrs, impl_.span())?.map(|a| a as u32); + let versions = extract_api_version(&impl_.attrs, impl_.span())?; + let api_ver = versions.custom.map(|a| a as u32); let mut path = extend_with_runtime_decl_path( extract_impl_trait(impl_, RequireQualifiedTraitPath::Yes)?.clone(), @@ -695,11 +828,34 @@ fn generate_runtime_api_versions(impls: &[ItemImpl]) -> Result { } let id: Path = parse_quote!( #path ID ); - let version = quote!( #path VERSION ); - let attrs = filter_cfg_attrs(&impl_.attrs); + let mut attrs = filter_cfg_attrs(&impl_.attrs); + + // Handle API versioning + // If feature gated version is set - handle it first + if let Some(feature_gated) = versions.feature_gated { + let feature_gated_version = feature_gated.1 as u32; + // the attributes for the feature gated staging api + let mut feature_gated_attrs = attrs.clone(); + add_feature_guard(&mut feature_gated_attrs, &feature_gated.0, true); + populate_runtime_api_versions( + &mut result, + &mut sections, + feature_gated_attrs, + id.clone(), + quote!( #feature_gated_version ), + &c, + ); + + // Add `#[cfg(not(feature ...))]` to the initial attributes. If the staging feature flag + // is not set we want to set the stable api version + add_feature_guard(&mut attrs, &feature_gated.0, false); + } - let api_ver = api_ver.map(|a| quote!( #a )).unwrap_or_else(|| version); - populate_runtime_api_versions(&mut result, &mut sections, attrs, id, api_ver, &c) + // Now add the stable api version to the versions list. If the api has got staging functions + // there might be a `#[cfg(not(feature ...))]` attribute attached to the stable version. + let base_api_version = quote!( #path VERSION ); + let api_ver = api_ver.map(|a| quote!( #a )).unwrap_or_else(|| base_api_version); + populate_runtime_api_versions(&mut result, &mut sections, attrs, id, api_ver, &c); } Ok(quote!( @@ -766,12 +922,65 @@ fn filter_cfg_attrs(attrs: &[Attribute]) -> Vec { attrs.iter().filter(|a| a.path().is_ident("cfg")).cloned().collect() } +// Parse feature flagged api_version. +// E.g. `#[cfg_attr(feature = "enable-staging-api", api_version(99))]` +fn extract_cfg_api_version(attrs: &Vec, span: Span) -> Result> { + let cfg_attrs = attrs.iter().filter(|a| a.path().is_ident("cfg_attr")).collect::>(); + + let mut cfg_api_version_attr = Vec::new(); + for cfg_attr in cfg_attrs { + let mut feature_name = None; + let mut api_version = None; + cfg_attr.parse_nested_meta(|m| { + if m.path.is_ident("feature") { + let a = m.value()?; + let b: LitStr = a.parse()?; + feature_name = Some(b.value()); + } else if m.path.is_ident(API_VERSION_ATTRIBUTE) { + let content; + parenthesized!(content in m.input); + let ver: LitInt = content.parse()?; + api_version = Some(ver.base10_parse::()?); + } else { + // we don't want to enforce any restrictions on the `cfg` attributes so just do + // nothing here. Otherwise return an error + // return Err(Error::new(span, format!("Unsupported cfg attribute"))) + } + Ok(()) + })?; + + // If there is a cfg attribute containing api_version - save if for processing + if let (Some(feature_name), Some(api_version)) = (feature_name, api_version) { + cfg_api_version_attr.push((feature_name, api_version)); + } + } + + if cfg_api_version_attr.len() > 1 { + return Err(Error::new(span, format!("Found multiple feature gated api versions (cfg attribute with nested {} attribute). This is not supported.", API_VERSION_ATTRIBUTE))) + } + + match cfg_api_version_attr.len() { + 0 => Ok(None), + _ => Ok(Some(cfg_api_version_attr.pop()) + .expect("match expression guarantees that the vec is not empty. qed.")), + } +} + +// Represents an API version. +// `custom` corresponds to `#[api_version(X)]` attribute. +// `feature_gated` corresponds to `#[cfg_attr(feature = "enable-staging-api", api_version(99))]` +// attribute. `String` is the feature name, `u64` the staging api version. +// Both fields may or may not exist for an item so they are both `Option`. +struct ApiVersion { + pub custom: Option, + pub feature_gated: Option<(String, u64)>, +} + // Extracts the value of `API_VERSION_ATTRIBUTE` and handles errors. // Returns: // - Err if the version is malformed -// - Some(u64) if the version is set -// - None if the version is not set (this is valid). -fn extract_api_version(attrs: &Vec, span: Span) -> Result> { +// - `ApiVersion` on success. IF a version is set or not is determined by the fields of `ApiVersion` +fn extract_api_version(attrs: &Vec, span: Span) -> Result { // First fetch all `API_VERSION_ATTRIBUTE` values (should be only one) let api_ver = attrs .iter() @@ -790,7 +999,10 @@ fn extract_api_version(attrs: &Vec, span: Span) -> Result } // Parse the runtime version if there exists one. - api_ver.first().map(|v| parse_runtime_api_version(v)).transpose() + Ok(ApiVersion { + custom: api_ver.first().map(|v| parse_runtime_api_version(v)).transpose()?, + feature_gated: extract_cfg_api_version(attrs, span)?, + }) } #[cfg(test)] From 9e0663fc19528e97929b2b0b9805d2f6f93442db Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Thu, 3 Aug 2023 13:24:55 +0300 Subject: [PATCH 02/21] Integration tests --- primitives/api/test/Cargo.toml | 3 + primitives/api/test/tests/decl_and_impl.rs | 83 ++++++++++++++++++++++ 2 files changed, 86 insertions(+) diff --git a/primitives/api/test/Cargo.toml b/primitives/api/test/Cargo.toml index 87c5eb6199efe..48eb067e4e474 100644 --- a/primitives/api/test/Cargo.toml +++ b/primitives/api/test/Cargo.toml @@ -35,3 +35,6 @@ static_assertions = "1.1.0" [[bench]] name = "bench" harness = false + +[features] +"enable-staging-api" = [] diff --git a/primitives/api/test/tests/decl_and_impl.rs b/primitives/api/test/tests/decl_and_impl.rs index 274f80bd1b465..156fda2c4cf57 100644 --- a/primitives/api/test/tests/decl_and_impl.rs +++ b/primitives/api/test/tests/decl_and_impl.rs @@ -50,6 +50,29 @@ decl_runtime_apis! { #[api_version(4)] fn glory_one(); } + + pub trait ApiWithStagingMethod { + fn stable_one(data: u64); + #[api_version(99)] + fn staging_one(); + } + + pub trait ApiWithStagingAndVersionedMethods { + fn stable_one(data: u64); + #[api_version(2)] + fn new_one(); + #[api_version(99)] + fn staging_one(); + } + + #[api_version(2)] + pub trait ApiWithStagingAndChangedBase { + fn stable_one(data: u64); + fn new_one(); + #[api_version(99)] + fn staging_one(); + } + } impl_runtime_apis! { @@ -82,6 +105,33 @@ impl_runtime_apis! { fn new_one() {} } + #[cfg_attr(feature = "enable-staging-api", api_version(99))] + impl self::ApiWithStagingMethod for Runtime { + fn stable_one(_: u64) {} + + #[cfg_attr(feature = "enable-staging-api", api_version(99))] + fn staging_one() {} + } + + #[cfg_attr(feature = "enable-staging-api", api_version(99))] + #[api_version(2)] + impl self::ApiWithStagingAndVersionedMethods for Runtime { + fn stable_one(_: u64) {} + fn new_one() {} + + #[cfg_attr(feature = "enable-staging-api", api_version(99))] + fn staging_one() {} + } + + #[cfg_attr(feature = "enable-staging-api", api_version(99))] + impl self::ApiWithStagingAndChangedBase for Runtime { + fn stable_one(_: u64) {} + fn new_one() {} + + #[cfg_attr(feature = "enable-staging-api", api_version(99))] + fn staging_one() {} + } + impl sp_api::Core for Runtime { fn version() -> sp_version::RuntimeVersion { unimplemented!() @@ -179,12 +229,38 @@ fn check_runtime_api_info() { // The stable version of the API assert_eq!(>::VERSION, 2); + + assert_eq!(>::VERSION, 1); + assert_eq!(>::VERSION, 1); + assert_eq!(>::VERSION, 2); } fn check_runtime_api_versions_contains() { assert!(RUNTIME_API_VERSIONS.iter().any(|v| v == &(T::ID, T::VERSION))); } +fn check_staging_runtime_api_versions(_staging_ver: u32) { + // Staging APIs should contain staging version if the feature is set... + #[cfg(feature = "enable-staging-api")] + assert!(RUNTIME_API_VERSIONS.iter().any(|v| v == &(T::ID, _staging_ver))); + //... otherwise the base version should be set + #[cfg(not(feature = "enable-staging-api"))] + check_runtime_api_versions_contains::>(); +} + +#[allow(unused_assignments)] +fn check_staging_multiver_runtime_api_versions( + staging_ver: u32, + _stable_ver: u32, +) { + // Staging APIs should contain staging version if the feature is set... + #[cfg(feature = "enable-staging-api")] + assert!(RUNTIME_API_VERSIONS.iter().any(|v| v == &(T::ID, staging_ver))); + //... otherwise the base version should be set + #[cfg(not(feature = "enable-staging-api"))] + assert!(RUNTIME_API_VERSIONS.iter().any(|v| v == &(T::ID, _stable_ver))); +} + #[test] fn check_runtime_api_versions() { check_runtime_api_versions_contains::>(); @@ -192,6 +268,13 @@ fn check_runtime_api_versions() { assert!(RUNTIME_API_VERSIONS .iter() .any(|v| v == &(>::ID, 3))); + + check_staging_runtime_api_versions::>(99); + check_staging_multiver_runtime_api_versions::>( + 99, 2, + ); + check_staging_runtime_api_versions::>(99); + check_runtime_api_versions_contains::>(); } From 05fab75c919937a63fa63655fb422754d2710012 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Thu, 3 Aug 2023 19:49:52 +0300 Subject: [PATCH 03/21] UI tests --- .../api/proc-macro/src/impl_runtime_apis.rs | 35 ++++++++++++--- primitives/api/test/tests/decl_and_impl.rs | 4 +- .../test/tests/ui/staging_fn_ver_missmatch.rs | 27 ++++++++++++ .../tests/ui/staging_fn_ver_missmatch.stderr | 11 +++++ .../tests/ui/staging_fn_wo_staging_api.rs | 44 +++++++++++++++++++ .../tests/ui/staging_fn_wo_staging_api.stderr | 11 +++++ .../api/test/tests/ui/version_in_impl_fn.rs | 20 +++++++++ .../test/tests/ui/version_in_impl_fn.stderr | 5 +++ 8 files changed, 149 insertions(+), 8 deletions(-) create mode 100644 primitives/api/test/tests/ui/staging_fn_ver_missmatch.rs create mode 100644 primitives/api/test/tests/ui/staging_fn_ver_missmatch.stderr create mode 100644 primitives/api/test/tests/ui/staging_fn_wo_staging_api.rs create mode 100644 primitives/api/test/tests/ui/staging_fn_wo_staging_api.stderr create mode 100644 primitives/api/test/tests/ui/version_in_impl_fn.rs create mode 100644 primitives/api/test/tests/ui/version_in_impl_fn.stderr diff --git a/primitives/api/proc-macro/src/impl_runtime_apis.rs b/primitives/api/proc-macro/src/impl_runtime_apis.rs index f984f2e90e3ba..21537712fe5e7 100644 --- a/primitives/api/proc-macro/src/impl_runtime_apis.rs +++ b/primitives/api/proc-macro/src/impl_runtime_apis.rs @@ -510,13 +510,15 @@ fn add_feature_guard(attrs: &mut Vec, feature_name: &str, t: bool) { // used to process `cfg_attr`s related to staging methods (e.g. `#[cfg_attr(feature = // "enable-staging-api", api_version(99))]`). Replaces the `cfg_attr` attribute with a feature guard // so that staging methods are added only for staging api implementations. -struct ApiImplItem { +struct ApiImplItem<'a> { + trait_api_ver: &'a Option<(String, u64)>, + trait_span: Span, errors: Vec, } -impl ApiImplItem { - pub fn new() -> ApiImplItem { - ApiImplItem { errors: Vec::new() } +impl<'a> ApiImplItem<'a> { + pub fn new(trait_api_ver: &Option<(String, u64)>, trait_span: Span) -> ApiImplItem { + ApiImplItem { trait_api_ver, trait_span, errors: Vec::new() } } pub fn process(&mut self, item: ItemImpl) -> Result { @@ -531,7 +533,7 @@ impl ApiImplItem { } } -impl Fold for ApiImplItem { +impl<'a> Fold for ApiImplItem<'a> { fn fold_impl_item_fn(&mut self, mut i: ImplItemFn) -> ImplItemFn { let v = extract_api_version(&i.attrs, i.span()); @@ -552,6 +554,26 @@ impl Fold for ApiImplItem { } if let Some(v) = v.feature_gated { + // Check for a mismatch between the trait and the function implementation + match self.trait_api_ver { + None => { + let mut e = Error::new(i.span(), "Found `cfg_attr` for implementation function but the trait is not versioned"); + e.combine(Error::new(self.trait_span, "Put a `cfg_attr` here")); + self.errors.push(e); + return fold::fold_impl_item_fn(self, i) + }, + Some(trait_ver) if v != *trait_ver => { + let mut e = Error::new( + i.span(), + "Different `cfg_attr` for the trait and the implementation function", + ); + e.combine(Error::new(self.trait_span, "Trait `cfg_attr` is here")); + self.errors.push(e); + return fold::fold_impl_item_fn(self, i) + }, + _ => {}, + } + add_feature_guard(&mut i.attrs, &v.0, true); } @@ -573,7 +595,8 @@ fn generate_api_impl_for_runtime(impls: &[ItemImpl]) -> Result { let mut impl_ = impl_.clone(); impl_.attrs = filter_cfg_attrs(&impl_.attrs); // Process all method implementations add add feature gates where necessary - let mut impl_ = ApiImplItem::new().process(impl_)?; + let mut impl_ = + ApiImplItem::new(&trait_api_ver.feature_gated, impl_.span()).process(impl_)?; let trait_ = extract_impl_trait(&impl_, RequireQualifiedTraitPath::Yes)?.clone(); let trait_ = extend_with_runtime_decl_path(trait_); diff --git a/primitives/api/test/tests/decl_and_impl.rs b/primitives/api/test/tests/decl_and_impl.rs index 156fda2c4cf57..826fb292a9f06 100644 --- a/primitives/api/test/tests/decl_and_impl.rs +++ b/primitives/api/test/tests/decl_and_impl.rs @@ -250,12 +250,12 @@ fn check_staging_runtime_api_versions(_staging_ver: #[allow(unused_assignments)] fn check_staging_multiver_runtime_api_versions( - staging_ver: u32, + _staging_ver: u32, _stable_ver: u32, ) { // Staging APIs should contain staging version if the feature is set... #[cfg(feature = "enable-staging-api")] - assert!(RUNTIME_API_VERSIONS.iter().any(|v| v == &(T::ID, staging_ver))); + assert!(RUNTIME_API_VERSIONS.iter().any(|v| v == &(T::ID, _staging_ver))); //... otherwise the base version should be set #[cfg(not(feature = "enable-staging-api"))] assert!(RUNTIME_API_VERSIONS.iter().any(|v| v == &(T::ID, _stable_ver))); diff --git a/primitives/api/test/tests/ui/staging_fn_ver_missmatch.rs b/primitives/api/test/tests/ui/staging_fn_ver_missmatch.rs new file mode 100644 index 0000000000000..564cb44437576 --- /dev/null +++ b/primitives/api/test/tests/ui/staging_fn_ver_missmatch.rs @@ -0,0 +1,27 @@ +/// The declaration of the `Runtime` type is done by the `construct_runtime!` macro in a real +/// runtime. +struct Runtime {} + +sp_api::decl_runtime_apis! { + pub trait Api1 { + fn test(data: u64); + #[api_version(99)] + fn staging(); + } +} + +sp_api::impl_runtime_apis! { + #[cfg_attr(feature = "enable-staging-api", api_version(42))] + impl self::Api1 for Runtime { + fn test(data: u64) { + unimplemented!() + } + + #[cfg_attr(feature = "enable-staging-api", api_version(99))] + fn staging(data: u64) { + unimplemented!() + } + } +} + +fn main() {} diff --git a/primitives/api/test/tests/ui/staging_fn_ver_missmatch.stderr b/primitives/api/test/tests/ui/staging_fn_ver_missmatch.stderr new file mode 100644 index 0000000000000..4c0b3154b3521 --- /dev/null +++ b/primitives/api/test/tests/ui/staging_fn_ver_missmatch.stderr @@ -0,0 +1,11 @@ +error: Different `cfg_attr` for the trait and the implementation function + --> tests/ui/staging_fn_ver_missmatch.rs:20:3 + | +20 | #[cfg_attr(feature = "enable-staging-api", api_version(99))] + | ^ + +error: Trait `cfg_attr` is here + --> tests/ui/staging_fn_ver_missmatch.rs:15:2 + | +15 | impl self::Api1 for Runtime { + | ^^^^ diff --git a/primitives/api/test/tests/ui/staging_fn_wo_staging_api.rs b/primitives/api/test/tests/ui/staging_fn_wo_staging_api.rs new file mode 100644 index 0000000000000..2b650fcc1c3be --- /dev/null +++ b/primitives/api/test/tests/ui/staging_fn_wo_staging_api.rs @@ -0,0 +1,44 @@ +/// The declaration of the `Runtime` type is done by the `construct_runtime!` macro in a real +/// runtime. +struct Runtime {} + +sp_api::decl_runtime_apis! { + pub trait Api1 { + fn test(data: u64); + #[api_version(99)] + fn staging(); + } + + pub trait Ap2 { + fn test(data: u64); + #[api_version(99)] + fn staging(); + } +} + +sp_api::impl_runtime_apis! { + impl self::Api1 for Runtime { + fn test(data: u64) { + unimplemented!() + } + + #[cfg_attr(feature = "enable-staging-api", api_version(99))] + fn staging(data: u64) { + unimplemented!() + } + } + + #[cfg_attr(feature = "enable-staging-api", api_version(42))] + impl self::Api2 for Runtime { + fn test(data: u64) { + unimplemented!() + } + + #[cfg_attr(feature = "enable-staging-api", api_version(99))] + fn staging(data: u64) { + unimplemented!() + } + } +} + +fn main() {} diff --git a/primitives/api/test/tests/ui/staging_fn_wo_staging_api.stderr b/primitives/api/test/tests/ui/staging_fn_wo_staging_api.stderr new file mode 100644 index 0000000000000..ef3f7b60b807b --- /dev/null +++ b/primitives/api/test/tests/ui/staging_fn_wo_staging_api.stderr @@ -0,0 +1,11 @@ +error: Found `cfg_attr` for implementation function but the trait is not versioned + --> tests/ui/staging_fn_wo_staging_api.rs:25:3 + | +25 | #[cfg_attr(feature = "enable-staging-api", api_version(99))] + | ^ + +error: Put a `cfg_attr` here + --> tests/ui/staging_fn_wo_staging_api.rs:20:2 + | +20 | impl self::Api1 for Runtime { + | ^^^^ diff --git a/primitives/api/test/tests/ui/version_in_impl_fn.rs b/primitives/api/test/tests/ui/version_in_impl_fn.rs new file mode 100644 index 0000000000000..67d7e4d792b58 --- /dev/null +++ b/primitives/api/test/tests/ui/version_in_impl_fn.rs @@ -0,0 +1,20 @@ +/// The declaration of the `Runtime` type is done by the `construct_runtime!` macro in a real +/// runtime. +struct Runtime {} + +sp_api::decl_runtime_apis! { + pub trait Api { + fn test(data: u64); + } +} + +sp_api::impl_runtime_apis! { + impl self::Api for Runtime { + #[api_version(2)] + fn test(data: u64) { + unimplemented!() + } + } +} + +fn main() {} diff --git a/primitives/api/test/tests/ui/version_in_impl_fn.stderr b/primitives/api/test/tests/ui/version_in_impl_fn.stderr new file mode 100644 index 0000000000000..3b802c87ac56b --- /dev/null +++ b/primitives/api/test/tests/ui/version_in_impl_fn.stderr @@ -0,0 +1,5 @@ +error: `api_version` attribute is not allowed `decl_runtime_api` method implementations + --> tests/ui/version_in_impl_fn.rs:13:3 + | +13 | #[api_version(2)] + | ^ From 4a23b556d296ed316b001db0b6cb3463f5702b31 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Thu, 3 Aug 2023 19:56:17 +0300 Subject: [PATCH 04/21] Enable staging api tests on CI --- primitives/api/test/Cargo.toml | 1 + scripts/ci/gitlab/pipeline/test.yml | 2 ++ 2 files changed, 3 insertions(+) diff --git a/primitives/api/test/Cargo.toml b/primitives/api/test/Cargo.toml index 48eb067e4e474..9505abc18122c 100644 --- a/primitives/api/test/Cargo.toml +++ b/primitives/api/test/Cargo.toml @@ -37,4 +37,5 @@ name = "bench" harness = false [features] +# used only in tests "enable-staging-api" = [] diff --git a/scripts/ci/gitlab/pipeline/test.yml b/scripts/ci/gitlab/pipeline/test.yml index 69d53012f79e7..d4405a65f967e 100644 --- a/scripts/ci/gitlab/pipeline/test.yml +++ b/scripts/ci/gitlab/pipeline/test.yml @@ -224,6 +224,8 @@ test-linux-stable: --features runtime-benchmarks,try-runtime,experimental --manifest-path ./bin/node/cli/Cargo.toml --partition count:${CI_NODE_INDEX}/${CI_NODE_TOTAL} + # run runtime-api tests with `enable-staging-api` feature + - time cargo nextest run -p sp-api-test --features enable-staging-api # we need to update cache only from one job - if [ ${CI_NODE_INDEX} == 1 ]; then rusty-cachier cache upload; fi # Upload tests results to Elasticsearch From cbeb0c72c770ce1aa099cdb33f485f3b21192041 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Fri, 4 Aug 2023 10:19:40 +0300 Subject: [PATCH 05/21] docs --- primitives/api/src/lib.rs | 45 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/primitives/api/src/lib.rs b/primitives/api/src/lib.rs index 2cc20dcb356a8..51d6c146b4bc4 100644 --- a/primitives/api/src/lib.rs +++ b/primitives/api/src/lib.rs @@ -344,6 +344,51 @@ pub use sp_api_proc_macro::decl_runtime_apis; /// ``` /// In this case `Balance` api version 3 is being implemented for `Runtime`. The `impl` block /// must contain all methods declared in version 3 and below. +/// +/// # Conditional version implementation +/// +/// `decl_runtime_apis!` supports `cfg_attr` attribute for conditional compilation. For example +/// let's say you want to implement a staging version of the runtime api and put it behind a +/// feature flag. You can do it this way: +/// ```ignore +/// pub struct Runtime {} +/// decl_runtime_apis! { +/// pub trait ApiWithStagingMethod { +/// fn stable_one(data: u64); +/// #[api_version(99)] +/// fn staging_one(); +/// } +/// +/// impl_runtime_apis! { +/// #[cfg_attr(feature = "enable-staging-api", api_version(99))] +/// impl self::ApiWithStagingMethod for Runtime { +/// fn stable_one(_: u64) {} +/// +/// #[cfg_attr(feature = "enable-staging-api", api_version(99))] +/// fn staging_one() {} +/// } +/// ``` +/// +/// `decl_runtime_apis!` declares two version of the api - 1 (the default one, which is +/// considered stable in our example) and 99 (which is considered staging). In +/// `impl_runtime_apis!` a `cfg_attr` attribute is attached to the `ApiWithStagingMethod` +/// implementation. If the code is compiled with `enable-staging-api` feature a version 99 of +/// the runtime api will be built which will include `staging_one`. Note that `staging_one` +/// also has got the same `cfg_attr` attached. +/// +/// If the code is compiled without `enable-staging-api` version 1 (the default one) will be +/// built which doesn't include `staging_one`. +/// +/// `cfg_attr` can also be used together with `api_version`. For the next snippet will build +/// version 99 if `enable-staging-api` is enabled and version 2 otherwise because both +/// `cfg_attr` and `api_version` are attached to the impl block: +/// ```ignore +/// #[cfg_attr(feature = "enable-staging-api", api_version(99))] +/// #[api_version(2)] +/// impl self::ApiWithStagingAndVersionedMethods for Runtime { +/// ... +/// } +/// ``` pub use sp_api_proc_macro::impl_runtime_apis; /// Mocks given trait implementations as runtime apis. From 697a90272e95e09445cb47f342931cf15c50e156 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Fri, 4 Aug 2023 10:29:33 +0300 Subject: [PATCH 06/21] Comments and minor style fixes --- .../api/proc-macro/src/impl_runtime_apis.rs | 20 +++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/primitives/api/proc-macro/src/impl_runtime_apis.rs b/primitives/api/proc-macro/src/impl_runtime_apis.rs index 21537712fe5e7..bf686a47836ff 100644 --- a/primitives/api/proc-macro/src/impl_runtime_apis.rs +++ b/primitives/api/proc-macro/src/impl_runtime_apis.rs @@ -511,8 +511,11 @@ fn add_feature_guard(attrs: &mut Vec, feature_name: &str, t: bool) { // "enable-staging-api", api_version(99))]`). Replaces the `cfg_attr` attribute with a feature guard // so that staging methods are added only for staging api implementations. struct ApiImplItem<'a> { + // Version of the trait where the `ImplItemFn` is located. Used for error reporting. trait_api_ver: &'a Option<(String, u64)>, + // Span of the trait where the `ImplItemFn` is located. Used for error reporting. trait_span: Span, + // All errors which occurred during folding errors: Vec, } @@ -537,13 +540,14 @@ impl<'a> Fold for ApiImplItem<'a> { fn fold_impl_item_fn(&mut self, mut i: ImplItemFn) -> ImplItemFn { let v = extract_api_version(&i.attrs, i.span()); - if let Err(e) = v { - self.errors.push(e); - return fold::fold_impl_item_fn(self, i) - } - - let v = v - .expect("Explicitly checked for and handled the error case on the previous line. qed."); + let v = match v { + Ok(ver) => ver, + Err(e) => { + // `api_version` attribute seems to be malformed + self.errors.push(e); + return fold::fold_impl_item_fn(self, i) + }, + }; if v.custom.is_some() { self.errors.push(Error::new( @@ -1002,7 +1006,7 @@ struct ApiVersion { // Extracts the value of `API_VERSION_ATTRIBUTE` and handles errors. // Returns: // - Err if the version is malformed -// - `ApiVersion` on success. IF a version is set or not is determined by the fields of `ApiVersion` +// - `ApiVersion` on success. If a version is set or not is determined by the fields of `ApiVersion` fn extract_api_version(attrs: &Vec, span: Span) -> Result { // First fetch all `API_VERSION_ATTRIBUTE` values (should be only one) let api_ver = attrs From 4af83d8d006506915a524f5d7c3603ab493add97 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Fri, 4 Aug 2023 14:11:30 +0300 Subject: [PATCH 07/21] Fix doc comments --- primitives/api/src/lib.rs | 27 +++++++++++++++------------ 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/primitives/api/src/lib.rs b/primitives/api/src/lib.rs index 51d6c146b4bc4..bdfd643b25e5b 100644 --- a/primitives/api/src/lib.rs +++ b/primitives/api/src/lib.rs @@ -352,20 +352,23 @@ pub use sp_api_proc_macro::decl_runtime_apis; /// feature flag. You can do it this way: /// ```ignore /// pub struct Runtime {} -/// decl_runtime_apis! { -/// pub trait ApiWithStagingMethod { -/// fn stable_one(data: u64); -/// #[api_version(99)] -/// fn staging_one(); -/// } +/// sp_api::decl_runtime_apis! { +/// pub trait ApiWithStagingMethod { +/// fn stable_one(data: u64); /// -/// impl_runtime_apis! { -/// #[cfg_attr(feature = "enable-staging-api", api_version(99))] -/// impl self::ApiWithStagingMethod for Runtime { -/// fn stable_one(_: u64) {} +/// #[api_version(99)] +/// fn staging_one(); +/// } +/// } /// +/// sp_api::impl_runtime_apis! { /// #[cfg_attr(feature = "enable-staging-api", api_version(99))] -/// fn staging_one() {} +/// impl self::ApiWithStagingMethod for Runtime { +/// fn stable_one(_: u64) {} +/// +/// #[cfg_attr(feature = "enable-staging-api", api_version(99))] +/// fn staging_one() {} +/// } /// } /// ``` /// @@ -386,7 +389,7 @@ pub use sp_api_proc_macro::decl_runtime_apis; /// #[cfg_attr(feature = "enable-staging-api", api_version(99))] /// #[api_version(2)] /// impl self::ApiWithStagingAndVersionedMethods for Runtime { -/// ... +/// // impl skipped /// } /// ``` pub use sp_api_proc_macro::impl_runtime_apis; From d69a857fcf640dc7812968802d5bdb6032d2803a Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Thu, 17 Aug 2023 14:51:41 +0300 Subject: [PATCH 08/21] Apply suggestions from code review 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 | 23 +++++++------------ primitives/api/test/Cargo.toml | 1 - 2 files changed, 8 insertions(+), 16 deletions(-) diff --git a/primitives/api/proc-macro/src/impl_runtime_apis.rs b/primitives/api/proc-macro/src/impl_runtime_apis.rs index bf686a47836ff..9c8f44cdab50d 100644 --- a/primitives/api/proc-macro/src/impl_runtime_apis.rs +++ b/primitives/api/proc-macro/src/impl_runtime_apis.rs @@ -493,11 +493,9 @@ fn extend_with_api_version(mut trait_: Path, version: Option) -> Path { trait_ } -// Adds a `#[cfg( feature = ...)]` attribute to attributes. Parameters: -// feature_name - name of the feature -// t - positive or negative feature guard. -// true => `#[cfg(feature = ...)]` -// false => `#[cfg(not(feature = ...))]` +/// Adds a feature guard to `attributes`. +/// +/// Depending on `enable`, the feature guard either enables ('feature = "something"`) or disables (`not(feature = "something")`). fn add_feature_guard(attrs: &mut Vec, feature_name: &str, t: bool) { let attr = match t { true => parse_quote!(#[cfg(feature = #feature_name)]), @@ -552,7 +550,7 @@ impl<'a> Fold for ApiImplItem<'a> { if v.custom.is_some() { self.errors.push(Error::new( i.span(), - "`api_version` attribute is not allowed `decl_runtime_api` method implementations", + "`api_version` attribute is not allowed on method implementations", )); return fold::fold_impl_item_fn(self, i) } @@ -968,10 +966,6 @@ fn extract_cfg_api_version(attrs: &Vec, span: Span) -> Result()?); - } else { - // we don't want to enforce any restrictions on the `cfg` attributes so just do - // nothing here. Otherwise return an error - // return Err(Error::new(span, format!("Unsupported cfg attribute"))) } Ok(()) })?; @@ -993,13 +987,12 @@ fn extract_cfg_api_version(attrs: &Vec, span: Span) -> Result, + /// Corresponds to `#[cfg_attr(feature = "enable-staging-api", api_version(99))]` + /// attribute. `String` is the feature name, `u64` the staging api version. pub feature_gated: Option<(String, u64)>, } diff --git a/primitives/api/test/Cargo.toml b/primitives/api/test/Cargo.toml index 9505abc18122c..48eb067e4e474 100644 --- a/primitives/api/test/Cargo.toml +++ b/primitives/api/test/Cargo.toml @@ -37,5 +37,4 @@ name = "bench" harness = false [features] -# used only in tests "enable-staging-api" = [] From fd33d1c743216aaece324b512efa7be053f9f073 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Thu, 17 Aug 2023 15:14:29 +0300 Subject: [PATCH 09/21] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Bastian Köcher --- primitives/api/proc-macro/src/impl_runtime_apis.rs | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/primitives/api/proc-macro/src/impl_runtime_apis.rs b/primitives/api/proc-macro/src/impl_runtime_apis.rs index 9c8f44cdab50d..af9f9a4f05232 100644 --- a/primitives/api/proc-macro/src/impl_runtime_apis.rs +++ b/primitives/api/proc-macro/src/impl_runtime_apis.rs @@ -496,7 +496,7 @@ fn extend_with_api_version(mut trait_: Path, version: Option) -> Path { /// Adds a feature guard to `attributes`. /// /// Depending on `enable`, the feature guard either enables ('feature = "something"`) or disables (`not(feature = "something")`). -fn add_feature_guard(attrs: &mut Vec, feature_name: &str, t: bool) { +fn add_feature_guard(attrs: &mut Vec, feature_name: &str, enable: bool) { let attr = match t { true => parse_quote!(#[cfg(feature = #feature_name)]), false => parse_quote!(#[cfg(not(feature = #feature_name))]), @@ -980,11 +980,7 @@ fn extract_cfg_api_version(attrs: &Vec, span: Span) -> Result Ok(None), - _ => Ok(Some(cfg_api_version_attr.pop()) - .expect("match expression guarantees that the vec is not empty. qed.")), - } + Ok(cfg_api_version_attr.drain().next()) } /// Represents an API version. From aa340a0404798e815ca77fb39b03ade282ddcaf2 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Thu, 17 Aug 2023 15:26:19 +0300 Subject: [PATCH 10/21] Fix formatting and a compilation error --- primitives/api/proc-macro/src/impl_runtime_apis.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/primitives/api/proc-macro/src/impl_runtime_apis.rs b/primitives/api/proc-macro/src/impl_runtime_apis.rs index af9f9a4f05232..b59e5497d287c 100644 --- a/primitives/api/proc-macro/src/impl_runtime_apis.rs +++ b/primitives/api/proc-macro/src/impl_runtime_apis.rs @@ -495,9 +495,10 @@ fn extend_with_api_version(mut trait_: Path, version: Option) -> Path { /// Adds a feature guard to `attributes`. /// -/// Depending on `enable`, the feature guard either enables ('feature = "something"`) or disables (`not(feature = "something")`). +/// Depending on `enable`, the feature guard either enables ('feature = "something"`) or disables +/// (`not(feature = "something")`). fn add_feature_guard(attrs: &mut Vec, feature_name: &str, enable: bool) { - let attr = match t { + let attr = match enable { true => parse_quote!(#[cfg(feature = #feature_name)]), false => parse_quote!(#[cfg(not(feature = #feature_name))]), }; @@ -980,7 +981,8 @@ fn extract_cfg_api_version(attrs: &Vec, span: Span) -> Result Date: Thu, 17 Aug 2023 15:40:46 +0300 Subject: [PATCH 11/21] Fix a doc comment --- primitives/api/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/primitives/api/src/lib.rs b/primitives/api/src/lib.rs index bdfd643b25e5b..9b0198c3e2530 100644 --- a/primitives/api/src/lib.rs +++ b/primitives/api/src/lib.rs @@ -347,7 +347,7 @@ pub use sp_api_proc_macro::decl_runtime_apis; /// /// # Conditional version implementation /// -/// `decl_runtime_apis!` supports `cfg_attr` attribute for conditional compilation. For example +/// `impl_runtime_apis!` supports `cfg_attr` attribute for conditional compilation. For example /// let's say you want to implement a staging version of the runtime api and put it behind a /// feature flag. You can do it this way: /// ```ignore From ea50aaf54f276eadee4b8fa02bda9dceda23f208 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Sat, 19 Aug 2023 14:24:58 +0300 Subject: [PATCH 12/21] Combine the errors from `ApiImplItem` --- .../api/proc-macro/src/impl_runtime_apis.rs | 10 +++--- .../api/test/tests/ui/multiple_errors.rs | 34 +++++++++++++++++++ .../api/test/tests/ui/multiple_errors.stderr | 17 ++++++++++ 3 files changed, 57 insertions(+), 4 deletions(-) create mode 100644 primitives/api/test/tests/ui/multiple_errors.rs create mode 100644 primitives/api/test/tests/ui/multiple_errors.stderr diff --git a/primitives/api/proc-macro/src/impl_runtime_apis.rs b/primitives/api/proc-macro/src/impl_runtime_apis.rs index 957e1d301fd19..c3054f20f46e8 100644 --- a/primitives/api/proc-macro/src/impl_runtime_apis.rs +++ b/primitives/api/proc-macro/src/impl_runtime_apis.rs @@ -518,10 +518,12 @@ impl<'a> ApiImplItem<'a> { pub fn process(&mut self, item: ItemImpl) -> Result { let res = self.fold_item_impl(item); - - if let Some(e) = self.errors.pop() { - // TODO: what to do with the rest of the errors? - return Err(e) + if let Some(mut err) = self.errors.pop() { + // combine all errors and return them + for e in self.errors.drain(..) { + err.combine(e); + } + return Err(err) } Ok(res) diff --git a/primitives/api/test/tests/ui/multiple_errors.rs b/primitives/api/test/tests/ui/multiple_errors.rs new file mode 100644 index 0000000000000..f45eafa4afb34 --- /dev/null +++ b/primitives/api/test/tests/ui/multiple_errors.rs @@ -0,0 +1,34 @@ +/// The declaration of the `Runtime` type is done by the `construct_runtime!` macro in a real +/// runtime. +struct Runtime {} + +sp_api::decl_runtime_apis! { + pub trait Api1 { + fn test(data: u64); + #[api_version(99)] + fn staging1(); + #[api_version(99)] + fn staging2(); + } +} + +sp_api::impl_runtime_apis! { + #[cfg_attr(feature = "enable-staging-api", api_version(99))] + impl self::Api1 for Runtime { + fn test(data: u64) { + unimplemented!() + } + + #[cfg_attr(feature = "enable-staging-api", api_version(98))] + fn staging1(data: u64) { + unimplemented!() + } + + #[cfg_attr(feature = "enable-staging-api", api_version(98))] + fn staging2(data: u64) { + unimplemented!() + } + } +} + +fn main() {} diff --git a/primitives/api/test/tests/ui/multiple_errors.stderr b/primitives/api/test/tests/ui/multiple_errors.stderr new file mode 100644 index 0000000000000..5055a8b52291d --- /dev/null +++ b/primitives/api/test/tests/ui/multiple_errors.stderr @@ -0,0 +1,17 @@ +error: Different `cfg_attr` for the trait and the implementation function + --> tests/ui/multiple_errors.rs:27:3 + | +27 | #[cfg_attr(feature = "enable-staging-api", api_version(98))] + | ^ + +error: Trait `cfg_attr` is here + --> tests/ui/multiple_errors.rs:17:2 + | +17 | impl self::Api1 for Runtime { + | ^^^^ + +error: Different `cfg_attr` for the trait and the implementation function + --> tests/ui/multiple_errors.rs:22:3 + | +22 | #[cfg_attr(feature = "enable-staging-api", api_version(98))] + | ^ From 96ebc53e3c92bdd7d8156d8b3aef12477bf9757d Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Sat, 19 Aug 2023 14:25:11 +0300 Subject: [PATCH 13/21] Fix a typo in UI test --- primitives/api/test/tests/ui/staging_fn_wo_staging_api.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/primitives/api/test/tests/ui/staging_fn_wo_staging_api.rs b/primitives/api/test/tests/ui/staging_fn_wo_staging_api.rs index 2b650fcc1c3be..297d82babe968 100644 --- a/primitives/api/test/tests/ui/staging_fn_wo_staging_api.rs +++ b/primitives/api/test/tests/ui/staging_fn_wo_staging_api.rs @@ -9,7 +9,7 @@ sp_api::decl_runtime_apis! { fn staging(); } - pub trait Ap2 { + pub trait Api2 { fn test(data: u64); #[api_version(99)] fn staging(); From 84bdf5be0deb5899ab64c69de08e7b58ca124d81 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Sat, 19 Aug 2023 15:32:23 +0300 Subject: [PATCH 14/21] Use attribute span when reporting multiple conditional `api_version` error --- .../api/proc-macro/src/impl_runtime_apis.rs | 11 ++++++-- .../test/tests/ui/method_with_two_api_ver.rs | 28 +++++++++++++++++++ .../tests/ui/method_with_two_api_ver.stderr | 17 +++++++++++ 3 files changed, 53 insertions(+), 3 deletions(-) create mode 100644 primitives/api/test/tests/ui/method_with_two_api_ver.rs create mode 100644 primitives/api/test/tests/ui/method_with_two_api_ver.stderr diff --git a/primitives/api/proc-macro/src/impl_runtime_apis.rs b/primitives/api/proc-macro/src/impl_runtime_apis.rs index c3054f20f46e8..a86a653fd04a5 100644 --- a/primitives/api/proc-macro/src/impl_runtime_apis.rs +++ b/primitives/api/proc-macro/src/impl_runtime_apis.rs @@ -967,15 +967,20 @@ fn extract_cfg_api_version(attrs: &Vec, span: Span) -> Result 1 { - return Err(Error::new(span, format!("Found multiple feature gated api versions (cfg attribute with nested {} attribute). This is not supported.", API_VERSION_ATTRIBUTE))) + let mut err = Error::new(span, format!("Found multiple feature gated api versions (cfg attribute with nested `{}` attribute). This is not supported.", API_VERSION_ATTRIBUTE)); + for (_, _, attr_span) in cfg_api_version_attr { + err.combine(Error::new(attr_span, format!("`{}` found here", API_VERSION_ATTRIBUTE))); + } + + return Err(err) } - let result = cfg_api_version_attr.drain(..).next(); + let result = cfg_api_version_attr.drain(..).next().map(|(feature, name, _)| (feature, name)); Ok(result) } diff --git a/primitives/api/test/tests/ui/method_with_two_api_ver.rs b/primitives/api/test/tests/ui/method_with_two_api_ver.rs new file mode 100644 index 0000000000000..e146bb867d815 --- /dev/null +++ b/primitives/api/test/tests/ui/method_with_two_api_ver.rs @@ -0,0 +1,28 @@ +/// The declaration of the `Runtime` type is done by the `construct_runtime!` macro in a real +/// runtime. +struct Runtime {} + +sp_api::decl_runtime_apis! { + pub trait Api1 { + fn test(data: u64); + #[api_version(99)] + fn staging(); + } +} + +sp_api::impl_runtime_apis! { + #[cfg_attr(feature = "enable-staging-api", api_version(99))] + impl self::Api1 for Runtime { + fn test(data: u64) { + unimplemented!() + } + + #[cfg_attr(feature = "enable-staging-api", api_version(99))] + #[cfg_attr(feature = "enable-staging-api", api_version(98))] + fn staging(data: u64) { + unimplemented!() + } + } +} + +fn main() {} diff --git a/primitives/api/test/tests/ui/method_with_two_api_ver.stderr b/primitives/api/test/tests/ui/method_with_two_api_ver.stderr new file mode 100644 index 0000000000000..515aebc9a9bc9 --- /dev/null +++ b/primitives/api/test/tests/ui/method_with_two_api_ver.stderr @@ -0,0 +1,17 @@ +error: Found multiple feature gated api versions (cfg attribute with nested `api_version` attribute). This is not supported. + --> tests/ui/method_with_two_api_ver.rs:20:3 + | +20 | #[cfg_attr(feature = "enable-staging-api", api_version(99))] + | ^ + +error: `api_version` found here + --> tests/ui/method_with_two_api_ver.rs:20:3 + | +20 | #[cfg_attr(feature = "enable-staging-api", api_version(99))] + | ^ + +error: `api_version` found here + --> tests/ui/method_with_two_api_ver.rs:21:3 + | +21 | #[cfg_attr(feature = "enable-staging-api", api_version(98))] + | ^ From 83e3aed7f69ccbbee131848832195115f84da2c5 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Tue, 22 Aug 2023 19:18:47 +0300 Subject: [PATCH 15/21] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Bastian Köcher --- primitives/api/proc-macro/src/impl_runtime_apis.rs | 4 ++-- primitives/api/src/lib.rs | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/primitives/api/proc-macro/src/impl_runtime_apis.rs b/primitives/api/proc-macro/src/impl_runtime_apis.rs index a86a653fd04a5..c57af7f7c69ad 100644 --- a/primitives/api/proc-macro/src/impl_runtime_apis.rs +++ b/primitives/api/proc-macro/src/impl_runtime_apis.rs @@ -942,8 +942,8 @@ fn filter_cfg_attrs(attrs: &[Attribute]) -> Vec { attrs.iter().filter(|a| a.path().is_ident("cfg")).cloned().collect() } -// Parse feature flagged api_version. -// E.g. `#[cfg_attr(feature = "enable-staging-api", api_version(99))]` +/// Parse feature flagged api_version. +/// E.g. `#[cfg_attr(feature = "enable-staging-api", api_version(99))]` fn extract_cfg_api_version(attrs: &Vec, span: Span) -> Result> { let cfg_attrs = attrs.iter().filter(|a| a.path().is_ident("cfg_attr")).collect::>(); diff --git a/primitives/api/src/lib.rs b/primitives/api/src/lib.rs index c0702317d6425..ef7f5ae1ea4d4 100644 --- a/primitives/api/src/lib.rs +++ b/primitives/api/src/lib.rs @@ -372,7 +372,7 @@ pub use sp_api_proc_macro::decl_runtime_apis; /// } /// ``` /// -/// `decl_runtime_apis!` declares two version of the api - 1 (the default one, which is +/// [`decl_runtime_apis!`] declares two version of the api - 1 (the default one, which is /// considered stable in our example) and 99 (which is considered staging). In /// `impl_runtime_apis!` a `cfg_attr` attribute is attached to the `ApiWithStagingMethod` /// implementation. If the code is compiled with `enable-staging-api` feature a version 99 of From beb1ddb2c06d0481280998616f210705b83cb5cc Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Wed, 23 Aug 2023 13:41:57 +0300 Subject: [PATCH 16/21] Don't use `cfg_attr` for methods. Use simple feature gate instead --- .../api/proc-macro/src/impl_runtime_apis.rs | 134 ++++++------------ primitives/api/src/lib.rs | 4 +- primitives/api/test/tests/decl_and_impl.rs | 8 +- .../test/tests/ui/method_with_two_api_ver.rs | 28 ---- .../tests/ui/method_with_two_api_ver.stderr | 17 --- .../api/test/tests/ui/multiple_errors.rs | 34 ----- .../api/test/tests/ui/multiple_errors.stderr | 17 --- .../test/tests/ui/staging_fn_ver_missmatch.rs | 27 ---- .../tests/ui/staging_fn_ver_missmatch.stderr | 11 -- .../tests/ui/staging_fn_wo_staging_api.rs | 44 ------ .../tests/ui/staging_fn_wo_staging_api.stderr | 11 -- .../api/test/tests/ui/version_in_impl_fn.rs | 20 --- .../test/tests/ui/version_in_impl_fn.stderr | 5 - 13 files changed, 47 insertions(+), 313 deletions(-) delete mode 100644 primitives/api/test/tests/ui/method_with_two_api_ver.rs delete mode 100644 primitives/api/test/tests/ui/method_with_two_api_ver.stderr delete mode 100644 primitives/api/test/tests/ui/multiple_errors.rs delete mode 100644 primitives/api/test/tests/ui/multiple_errors.stderr delete mode 100644 primitives/api/test/tests/ui/staging_fn_ver_missmatch.rs delete mode 100644 primitives/api/test/tests/ui/staging_fn_ver_missmatch.stderr delete mode 100644 primitives/api/test/tests/ui/staging_fn_wo_staging_api.rs delete mode 100644 primitives/api/test/tests/ui/staging_fn_wo_staging_api.stderr delete mode 100644 primitives/api/test/tests/ui/version_in_impl_fn.rs delete mode 100644 primitives/api/test/tests/ui/version_in_impl_fn.stderr diff --git a/primitives/api/proc-macro/src/impl_runtime_apis.rs b/primitives/api/proc-macro/src/impl_runtime_apis.rs index c57af7f7c69ad..3d5d993f32057 100644 --- a/primitives/api/proc-macro/src/impl_runtime_apis.rs +++ b/primitives/api/proc-macro/src/impl_runtime_apis.rs @@ -35,8 +35,7 @@ use syn::{ parse::{Error, Parse, ParseStream, Result}, parse_macro_input, parse_quote, spanned::Spanned, - Attribute, Ident, ImplItem, ImplItemFn, ItemImpl, LitInt, LitStr, Path, Signature, Type, - TypePath, + Attribute, Ident, ImplItem, ItemImpl, LitInt, LitStr, Path, Signature, Type, TypePath, }; use std::collections::HashSet; @@ -179,12 +178,15 @@ fn generate_impl_calls( )?; let mut attrs = filter_cfg_attrs(&impl_.attrs); - let method_api_version = extract_api_version(&method.attrs, method.span())?; - if method_api_version.custom.is_some() { - return Err(Error::new(method.span(), "`api_version` attribute is not allowed `decl_runtime_api` method implementations")) - } - if let Some(feature_gated) = method_api_version.feature_gated { - add_feature_guard(&mut attrs, &feature_gated.0, true) + // If the method has got a `#[cfg(feature = X)]` attribute match it against the impl + // block `cfg_attr` add a feature flag for this impl call. Note that the feature + // guard is added to `Vec` which is returned as a result and not to the + // method itself. + if let Some((feature_name, _)) = &trait_api_ver.feature_gated { + if method_feature_flag_matches_trait_feature_gate(&method.attrs, &feature_name)? + { + add_feature_guard(&mut attrs, &feature_name, true) + } } impl_calls.push(( @@ -498,89 +500,6 @@ fn add_feature_guard(attrs: &mut Vec, feature_name: &str, enable: boo attrs.push(attr); } -// Fold implementation which processes function implementations in impl block for a trait. It is -// used to process `cfg_attr`s related to staging methods (e.g. `#[cfg_attr(feature = -// "enable-staging-api", api_version(99))]`). Replaces the `cfg_attr` attribute with a feature guard -// so that staging methods are added only for staging api implementations. -struct ApiImplItem<'a> { - // Version of the trait where the `ImplItemFn` is located. Used for error reporting. - trait_api_ver: &'a Option<(String, u64)>, - // Span of the trait where the `ImplItemFn` is located. Used for error reporting. - trait_span: Span, - // All errors which occurred during folding - errors: Vec, -} - -impl<'a> ApiImplItem<'a> { - pub fn new(trait_api_ver: &Option<(String, u64)>, trait_span: Span) -> ApiImplItem { - ApiImplItem { trait_api_ver, trait_span, errors: Vec::new() } - } - - pub fn process(&mut self, item: ItemImpl) -> Result { - let res = self.fold_item_impl(item); - if let Some(mut err) = self.errors.pop() { - // combine all errors and return them - for e in self.errors.drain(..) { - err.combine(e); - } - return Err(err) - } - - Ok(res) - } -} - -impl<'a> Fold for ApiImplItem<'a> { - fn fold_impl_item_fn(&mut self, mut i: ImplItemFn) -> ImplItemFn { - let v = extract_api_version(&i.attrs, i.span()); - - let v = match v { - Ok(ver) => ver, - Err(e) => { - // `api_version` attribute seems to be malformed - self.errors.push(e); - return fold::fold_impl_item_fn(self, i) - }, - }; - - if v.custom.is_some() { - self.errors.push(Error::new( - i.span(), - "`api_version` attribute is not allowed on method implementations", - )); - return fold::fold_impl_item_fn(self, i) - } - - if let Some(v) = v.feature_gated { - // Check for a mismatch between the trait and the function implementation - match self.trait_api_ver { - None => { - let mut e = Error::new(i.span(), "Found `cfg_attr` for implementation function but the trait is not versioned"); - e.combine(Error::new(self.trait_span, "Put a `cfg_attr` here")); - self.errors.push(e); - return fold::fold_impl_item_fn(self, i) - }, - Some(trait_ver) if v != *trait_ver => { - let mut e = Error::new( - i.span(), - "Different `cfg_attr` for the trait and the implementation function", - ); - e.combine(Error::new(self.trait_span, "Trait `cfg_attr` is here")); - self.errors.push(e); - return fold::fold_impl_item_fn(self, i) - }, - _ => {}, - } - - add_feature_guard(&mut i.attrs, &v.0, true); - } - - i.attrs = filter_cfg_attrs(&i.attrs); - - fold::fold_impl_item_fn(self, i) - } -} - /// Generates the implementations of the apis for the runtime. fn generate_api_impl_for_runtime(impls: &[ItemImpl]) -> Result { let mut impls_prepared = Vec::new(); @@ -593,8 +512,6 @@ fn generate_api_impl_for_runtime(impls: &[ItemImpl]) -> Result { let mut impl_ = impl_.clone(); impl_.attrs = filter_cfg_attrs(&impl_.attrs); // Process all method implementations add add feature gates where necessary - let mut impl_ = - ApiImplItem::new(&trait_api_ver.feature_gated, impl_.span()).process(impl_)?; let trait_ = extract_impl_trait(&impl_, RequireQualifiedTraitPath::Yes)?.clone(); let trait_ = extend_with_runtime_decl_path(trait_); @@ -1022,6 +939,37 @@ fn extract_api_version(attrs: &Vec, span: Span) -> Result }) } +/// This function looks for a matching trait `cfg_attr` and method feature flag. +/// It looks for a `#[feature = X]` attribute in `attrs`. Returns `true` if X matches +/// `trait_feature_gate`. +fn method_feature_flag_matches_trait_feature_gate( + attrs: &Vec, + trait_feature_gate: &String, +) -> Result { + let cfg_attrs = attrs.iter().filter(|a| a.path().is_ident("cfg")).collect::>(); + + for cfg_attr in cfg_attrs { + let mut feature_name = None; + cfg_attr.parse_nested_meta(|m| { + if m.path.is_ident("feature") { + let a = m.value()?; + let b: LitStr = a.parse()?; + feature_name = Some(b.value()); + } + Ok(()) + })?; + + // If there is a cfg attribute containing api_version - save if for processing + if let Some(feature_name) = feature_name { + if feature_name == *trait_feature_gate { + return Ok(true) + } + } + } + + Ok(false) +} + #[cfg(test)] mod tests { use super::*; diff --git a/primitives/api/src/lib.rs b/primitives/api/src/lib.rs index ef7f5ae1ea4d4..c3f80acf09ae5 100644 --- a/primitives/api/src/lib.rs +++ b/primitives/api/src/lib.rs @@ -366,7 +366,7 @@ pub use sp_api_proc_macro::decl_runtime_apis; /// impl self::ApiWithStagingMethod for Runtime { /// fn stable_one(_: u64) {} /// -/// #[cfg_attr(feature = "enable-staging-api", api_version(99))] +/// #[cfg(feature = "enable-staging-api")] /// fn staging_one() {} /// } /// } @@ -377,7 +377,7 @@ pub use sp_api_proc_macro::decl_runtime_apis; /// `impl_runtime_apis!` a `cfg_attr` attribute is attached to the `ApiWithStagingMethod` /// implementation. If the code is compiled with `enable-staging-api` feature a version 99 of /// the runtime api will be built which will include `staging_one`. Note that `staging_one` -/// also has got the same `cfg_attr` attached. +/// implementation is feature gated by `#[cfg(feature = ... )]` attribute. /// /// If the code is compiled without `enable-staging-api` version 1 (the default one) will be /// built which doesn't include `staging_one`. diff --git a/primitives/api/test/tests/decl_and_impl.rs b/primitives/api/test/tests/decl_and_impl.rs index 826fb292a9f06..6e895680e41c4 100644 --- a/primitives/api/test/tests/decl_and_impl.rs +++ b/primitives/api/test/tests/decl_and_impl.rs @@ -109,8 +109,8 @@ impl_runtime_apis! { impl self::ApiWithStagingMethod for Runtime { fn stable_one(_: u64) {} - #[cfg_attr(feature = "enable-staging-api", api_version(99))] - fn staging_one() {} + #[cfg(feature = "enable-staging-api")] + fn staging_one() { } } #[cfg_attr(feature = "enable-staging-api", api_version(99))] @@ -119,7 +119,7 @@ impl_runtime_apis! { fn stable_one(_: u64) {} fn new_one() {} - #[cfg_attr(feature = "enable-staging-api", api_version(99))] + #[cfg(feature = "enable-staging-api")] fn staging_one() {} } @@ -128,7 +128,7 @@ impl_runtime_apis! { fn stable_one(_: u64) {} fn new_one() {} - #[cfg_attr(feature = "enable-staging-api", api_version(99))] + #[cfg(feature = "enable-staging-api")] fn staging_one() {} } diff --git a/primitives/api/test/tests/ui/method_with_two_api_ver.rs b/primitives/api/test/tests/ui/method_with_two_api_ver.rs deleted file mode 100644 index e146bb867d815..0000000000000 --- a/primitives/api/test/tests/ui/method_with_two_api_ver.rs +++ /dev/null @@ -1,28 +0,0 @@ -/// The declaration of the `Runtime` type is done by the `construct_runtime!` macro in a real -/// runtime. -struct Runtime {} - -sp_api::decl_runtime_apis! { - pub trait Api1 { - fn test(data: u64); - #[api_version(99)] - fn staging(); - } -} - -sp_api::impl_runtime_apis! { - #[cfg_attr(feature = "enable-staging-api", api_version(99))] - impl self::Api1 for Runtime { - fn test(data: u64) { - unimplemented!() - } - - #[cfg_attr(feature = "enable-staging-api", api_version(99))] - #[cfg_attr(feature = "enable-staging-api", api_version(98))] - fn staging(data: u64) { - unimplemented!() - } - } -} - -fn main() {} diff --git a/primitives/api/test/tests/ui/method_with_two_api_ver.stderr b/primitives/api/test/tests/ui/method_with_two_api_ver.stderr deleted file mode 100644 index 515aebc9a9bc9..0000000000000 --- a/primitives/api/test/tests/ui/method_with_two_api_ver.stderr +++ /dev/null @@ -1,17 +0,0 @@ -error: Found multiple feature gated api versions (cfg attribute with nested `api_version` attribute). This is not supported. - --> tests/ui/method_with_two_api_ver.rs:20:3 - | -20 | #[cfg_attr(feature = "enable-staging-api", api_version(99))] - | ^ - -error: `api_version` found here - --> tests/ui/method_with_two_api_ver.rs:20:3 - | -20 | #[cfg_attr(feature = "enable-staging-api", api_version(99))] - | ^ - -error: `api_version` found here - --> tests/ui/method_with_two_api_ver.rs:21:3 - | -21 | #[cfg_attr(feature = "enable-staging-api", api_version(98))] - | ^ diff --git a/primitives/api/test/tests/ui/multiple_errors.rs b/primitives/api/test/tests/ui/multiple_errors.rs deleted file mode 100644 index f45eafa4afb34..0000000000000 --- a/primitives/api/test/tests/ui/multiple_errors.rs +++ /dev/null @@ -1,34 +0,0 @@ -/// The declaration of the `Runtime` type is done by the `construct_runtime!` macro in a real -/// runtime. -struct Runtime {} - -sp_api::decl_runtime_apis! { - pub trait Api1 { - fn test(data: u64); - #[api_version(99)] - fn staging1(); - #[api_version(99)] - fn staging2(); - } -} - -sp_api::impl_runtime_apis! { - #[cfg_attr(feature = "enable-staging-api", api_version(99))] - impl self::Api1 for Runtime { - fn test(data: u64) { - unimplemented!() - } - - #[cfg_attr(feature = "enable-staging-api", api_version(98))] - fn staging1(data: u64) { - unimplemented!() - } - - #[cfg_attr(feature = "enable-staging-api", api_version(98))] - fn staging2(data: u64) { - unimplemented!() - } - } -} - -fn main() {} diff --git a/primitives/api/test/tests/ui/multiple_errors.stderr b/primitives/api/test/tests/ui/multiple_errors.stderr deleted file mode 100644 index 5055a8b52291d..0000000000000 --- a/primitives/api/test/tests/ui/multiple_errors.stderr +++ /dev/null @@ -1,17 +0,0 @@ -error: Different `cfg_attr` for the trait and the implementation function - --> tests/ui/multiple_errors.rs:27:3 - | -27 | #[cfg_attr(feature = "enable-staging-api", api_version(98))] - | ^ - -error: Trait `cfg_attr` is here - --> tests/ui/multiple_errors.rs:17:2 - | -17 | impl self::Api1 for Runtime { - | ^^^^ - -error: Different `cfg_attr` for the trait and the implementation function - --> tests/ui/multiple_errors.rs:22:3 - | -22 | #[cfg_attr(feature = "enable-staging-api", api_version(98))] - | ^ diff --git a/primitives/api/test/tests/ui/staging_fn_ver_missmatch.rs b/primitives/api/test/tests/ui/staging_fn_ver_missmatch.rs deleted file mode 100644 index 564cb44437576..0000000000000 --- a/primitives/api/test/tests/ui/staging_fn_ver_missmatch.rs +++ /dev/null @@ -1,27 +0,0 @@ -/// The declaration of the `Runtime` type is done by the `construct_runtime!` macro in a real -/// runtime. -struct Runtime {} - -sp_api::decl_runtime_apis! { - pub trait Api1 { - fn test(data: u64); - #[api_version(99)] - fn staging(); - } -} - -sp_api::impl_runtime_apis! { - #[cfg_attr(feature = "enable-staging-api", api_version(42))] - impl self::Api1 for Runtime { - fn test(data: u64) { - unimplemented!() - } - - #[cfg_attr(feature = "enable-staging-api", api_version(99))] - fn staging(data: u64) { - unimplemented!() - } - } -} - -fn main() {} diff --git a/primitives/api/test/tests/ui/staging_fn_ver_missmatch.stderr b/primitives/api/test/tests/ui/staging_fn_ver_missmatch.stderr deleted file mode 100644 index 4c0b3154b3521..0000000000000 --- a/primitives/api/test/tests/ui/staging_fn_ver_missmatch.stderr +++ /dev/null @@ -1,11 +0,0 @@ -error: Different `cfg_attr` for the trait and the implementation function - --> tests/ui/staging_fn_ver_missmatch.rs:20:3 - | -20 | #[cfg_attr(feature = "enable-staging-api", api_version(99))] - | ^ - -error: Trait `cfg_attr` is here - --> tests/ui/staging_fn_ver_missmatch.rs:15:2 - | -15 | impl self::Api1 for Runtime { - | ^^^^ diff --git a/primitives/api/test/tests/ui/staging_fn_wo_staging_api.rs b/primitives/api/test/tests/ui/staging_fn_wo_staging_api.rs deleted file mode 100644 index 297d82babe968..0000000000000 --- a/primitives/api/test/tests/ui/staging_fn_wo_staging_api.rs +++ /dev/null @@ -1,44 +0,0 @@ -/// The declaration of the `Runtime` type is done by the `construct_runtime!` macro in a real -/// runtime. -struct Runtime {} - -sp_api::decl_runtime_apis! { - pub trait Api1 { - fn test(data: u64); - #[api_version(99)] - fn staging(); - } - - pub trait Api2 { - fn test(data: u64); - #[api_version(99)] - fn staging(); - } -} - -sp_api::impl_runtime_apis! { - impl self::Api1 for Runtime { - fn test(data: u64) { - unimplemented!() - } - - #[cfg_attr(feature = "enable-staging-api", api_version(99))] - fn staging(data: u64) { - unimplemented!() - } - } - - #[cfg_attr(feature = "enable-staging-api", api_version(42))] - impl self::Api2 for Runtime { - fn test(data: u64) { - unimplemented!() - } - - #[cfg_attr(feature = "enable-staging-api", api_version(99))] - fn staging(data: u64) { - unimplemented!() - } - } -} - -fn main() {} diff --git a/primitives/api/test/tests/ui/staging_fn_wo_staging_api.stderr b/primitives/api/test/tests/ui/staging_fn_wo_staging_api.stderr deleted file mode 100644 index ef3f7b60b807b..0000000000000 --- a/primitives/api/test/tests/ui/staging_fn_wo_staging_api.stderr +++ /dev/null @@ -1,11 +0,0 @@ -error: Found `cfg_attr` for implementation function but the trait is not versioned - --> tests/ui/staging_fn_wo_staging_api.rs:25:3 - | -25 | #[cfg_attr(feature = "enable-staging-api", api_version(99))] - | ^ - -error: Put a `cfg_attr` here - --> tests/ui/staging_fn_wo_staging_api.rs:20:2 - | -20 | impl self::Api1 for Runtime { - | ^^^^ diff --git a/primitives/api/test/tests/ui/version_in_impl_fn.rs b/primitives/api/test/tests/ui/version_in_impl_fn.rs deleted file mode 100644 index 67d7e4d792b58..0000000000000 --- a/primitives/api/test/tests/ui/version_in_impl_fn.rs +++ /dev/null @@ -1,20 +0,0 @@ -/// The declaration of the `Runtime` type is done by the `construct_runtime!` macro in a real -/// runtime. -struct Runtime {} - -sp_api::decl_runtime_apis! { - pub trait Api { - fn test(data: u64); - } -} - -sp_api::impl_runtime_apis! { - impl self::Api for Runtime { - #[api_version(2)] - fn test(data: u64) { - unimplemented!() - } - } -} - -fn main() {} diff --git a/primitives/api/test/tests/ui/version_in_impl_fn.stderr b/primitives/api/test/tests/ui/version_in_impl_fn.stderr deleted file mode 100644 index 3b802c87ac56b..0000000000000 --- a/primitives/api/test/tests/ui/version_in_impl_fn.stderr +++ /dev/null @@ -1,5 +0,0 @@ -error: `api_version` attribute is not allowed `decl_runtime_api` method implementations - --> tests/ui/version_in_impl_fn.rs:13:3 - | -13 | #[api_version(2)] - | ^ From 9ce15f01d5196e204119206d2494165bfd3cab8a Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Wed, 23 Aug 2023 15:25:13 +0300 Subject: [PATCH 17/21] Remove a stale comment --- primitives/api/proc-macro/src/impl_runtime_apis.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/primitives/api/proc-macro/src/impl_runtime_apis.rs b/primitives/api/proc-macro/src/impl_runtime_apis.rs index 3d5d993f32057..59a5b24d5b9a4 100644 --- a/primitives/api/proc-macro/src/impl_runtime_apis.rs +++ b/primitives/api/proc-macro/src/impl_runtime_apis.rs @@ -511,7 +511,6 @@ fn generate_api_impl_for_runtime(impls: &[ItemImpl]) -> Result { let mut impl_ = impl_.clone(); impl_.attrs = filter_cfg_attrs(&impl_.attrs); - // Process all method implementations add add feature gates where necessary let trait_ = extract_impl_trait(&impl_, RequireQualifiedTraitPath::Yes)?.clone(); let trait_ = extend_with_runtime_decl_path(trait_); From 4da20a79bd762580ebf502e9f807c2d7e5876106 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Wed, 23 Aug 2023 16:38:37 +0300 Subject: [PATCH 18/21] Update 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 --- primitives/api/proc-macro/src/impl_runtime_apis.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/primitives/api/proc-macro/src/impl_runtime_apis.rs b/primitives/api/proc-macro/src/impl_runtime_apis.rs index 59a5b24d5b9a4..40d4c51d353bb 100644 --- a/primitives/api/proc-macro/src/impl_runtime_apis.rs +++ b/primitives/api/proc-macro/src/impl_runtime_apis.rs @@ -896,8 +896,7 @@ fn extract_cfg_api_version(attrs: &Vec, span: Span) -> Result Date: Wed, 23 Aug 2023 16:41:30 +0300 Subject: [PATCH 19/21] Revert "Update primitives/api/proc-macro/src/impl_runtime_apis.rs" This reverts commit 4da20a79bd762580ebf502e9f807c2d7e5876106. --- primitives/api/proc-macro/src/impl_runtime_apis.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/primitives/api/proc-macro/src/impl_runtime_apis.rs b/primitives/api/proc-macro/src/impl_runtime_apis.rs index 40d4c51d353bb..59a5b24d5b9a4 100644 --- a/primitives/api/proc-macro/src/impl_runtime_apis.rs +++ b/primitives/api/proc-macro/src/impl_runtime_apis.rs @@ -896,7 +896,8 @@ fn extract_cfg_api_version(attrs: &Vec, span: Span) -> Result Date: Wed, 23 Aug 2023 16:51:34 +0300 Subject: [PATCH 20/21] Code review feedback --- .../api/proc-macro/src/impl_runtime_apis.rs | 43 +------------------ 1 file changed, 2 insertions(+), 41 deletions(-) diff --git a/primitives/api/proc-macro/src/impl_runtime_apis.rs b/primitives/api/proc-macro/src/impl_runtime_apis.rs index 59a5b24d5b9a4..695e5c2ca87fb 100644 --- a/primitives/api/proc-macro/src/impl_runtime_apis.rs +++ b/primitives/api/proc-macro/src/impl_runtime_apis.rs @@ -178,16 +178,8 @@ fn generate_impl_calls( )?; let mut attrs = filter_cfg_attrs(&impl_.attrs); - // If the method has got a `#[cfg(feature = X)]` attribute match it against the impl - // block `cfg_attr` add a feature flag for this impl call. Note that the feature - // guard is added to `Vec` which is returned as a result and not to the - // method itself. - if let Some((feature_name, _)) = &trait_api_ver.feature_gated { - if method_feature_flag_matches_trait_feature_gate(&method.attrs, &feature_name)? - { - add_feature_guard(&mut attrs, &feature_name, true) - } - } + // Add any `#[cfg(feature = X)]` attributes of the method to result + attrs.extend(filter_cfg_attrs(&method.attrs)); impl_calls.push(( impl_trait_ident.clone(), @@ -938,37 +930,6 @@ fn extract_api_version(attrs: &Vec, span: Span) -> Result }) } -/// This function looks for a matching trait `cfg_attr` and method feature flag. -/// It looks for a `#[feature = X]` attribute in `attrs`. Returns `true` if X matches -/// `trait_feature_gate`. -fn method_feature_flag_matches_trait_feature_gate( - attrs: &Vec, - trait_feature_gate: &String, -) -> Result { - let cfg_attrs = attrs.iter().filter(|a| a.path().is_ident("cfg")).collect::>(); - - for cfg_attr in cfg_attrs { - let mut feature_name = None; - cfg_attr.parse_nested_meta(|m| { - if m.path.is_ident("feature") { - let a = m.value()?; - let b: LitStr = a.parse()?; - feature_name = Some(b.value()); - } - Ok(()) - })?; - - // If there is a cfg attribute containing api_version - save if for processing - if let Some(feature_name) = feature_name { - if feature_name == *trait_feature_gate { - return Ok(true) - } - } - } - - Ok(false) -} - #[cfg(test)] mod tests { use super::*; From 436f7c89742b146470ff245d03e98e36161a83af Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Thu, 24 Aug 2023 09:15:10 +0300 Subject: [PATCH 21/21] Style improvements --- primitives/api/proc-macro/src/impl_runtime_apis.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/primitives/api/proc-macro/src/impl_runtime_apis.rs b/primitives/api/proc-macro/src/impl_runtime_apis.rs index 695e5c2ca87fb..74cfa0980623b 100644 --- a/primitives/api/proc-macro/src/impl_runtime_apis.rs +++ b/primitives/api/proc-macro/src/impl_runtime_apis.rs @@ -888,8 +888,10 @@ fn extract_cfg_api_version(attrs: &Vec, span: Span) -> Result