add the ability to declare multiple impl blocks #6795
Conversation
|
/cmd fmt |
pkhry
added
I5-enhancement
|
Command "fmt" has started 🚀 See logs here |
|
Command "fmt" has finished ✅ See logs here |
| @@ -374,7 +404,7 @@ fn generate_runtime_api_base_structures() -> Result<TokenStream> { | |||
| { | |||
| type RuntimeApi = RuntimeApiImpl<Block, C>; | |||
|
|
|||
| fn construct_runtime_api<'a>( | |||
| fn construct_runtime_api<'a>( | |||
There was a problem hiding this comment.
| fn construct_runtime_api<'a>( | |
| fn construct_runtime_api<'a>( |
| pub fn runtime_api_versions() -> #c::ApisVec { | ||
| let api = #c::vec::Vec::from([RUNTIME_API_VERSIONS.into_owned(), #(#modules::RUNTIME_API_VERSIONS.into_owned()),*]).concat(); | ||
| api.into() | ||
| } |
There was a problem hiding this comment.
I don't really like that RUNTIME_API_VERSIONS change its definition from: "all runtime api versions" to "runtime api versions only defined in the main macro". And that people should now use runtime_api_versions.
I would prefer to deprecate or remove, rather than changing the definition like this.
Why not handling this on the macro side: impl_ext can generate a PARTIAL_API_VERSIONS constant and the main macro would then concatenate all the versions in the final constant.
You can concatenate in const like this:
const A: &[(u32, u32)] = &[(0, 1)];
const B: &[(u32, u32)] = &[(0, 1)];
const C: alloc::borrow::Cow<'static, [(u32, u32)]> = Cow::Borrowed({
const LEN: usize = A.len() + B.len();
const ARR: [(u32, u32); LEN] = {
let mut arr = [(0, 0); LEN];
let mut cursor = 0;
let mut i = 0;
while i < A.len() {
arr[i] = A[i];
i += 1;
}
let mut i = 0;
while i < B.len() {
arr[cursor] = B[i];
cursor += 1;
i += 1;
}
arr
};
&ARR
});| let mut path: Vec<Ident> = vec![]; | ||
| fn call(path: &mut Vec<Ident>, item: &UseTree) -> Result<()> { | ||
| match &item { | ||
| syn::UseTree::Path(use_path) => { | ||
| path.push(use_path.ident.clone()); | ||
| call(path, use_path.tree.as_ref()) | ||
| }, | ||
| syn::UseTree::Glob(_) => Ok(()), | ||
| syn::UseTree::Name(_) | syn::UseTree::Rename(_) | syn::UseTree::Group(_) => { | ||
| let error = Error::new( | ||
| item.span(), | ||
| "Unsupported syntax used to import api implementaions from an extension module. \ | ||
| Try using `pub use <path>::*` or `use <path>::*`", | ||
| ); | ||
| return Err(error) | ||
| }, | ||
| } | ||
| } | ||
| call(&mut path, &item.tree)?; | ||
| let tok = quote::quote! {#(#path)::*}; | ||
| Ok(syn::parse::<TypePath>(tok.into()) |
There was a problem hiding this comment.
maybe move this to its own function for readability.
| Kind::Ext => quote! { | ||
| #[doc(hidden)] | ||
| #[inline(always)] | ||
| pub fn runtime_metadata() -> #crate_::vec::Vec<#crate_::metadata_ir::RuntimeApiMetadataIR> { |
There was a problem hiding this comment.
I would name it partial_runtime_metadata to avoid confusion and suggestions from rustc
| fn same_name() -> String { | ||
| "example".to_string() | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Having 2 external implementation would be even better for testing. like ext1 and ext2.
| @@ -102,17 +102,24 @@ mod apis { | |||
|
|
|||
| fn wild_card(_: u32) {} | |||
| } | |||
| pub use ext::*; | |||
There was a problem hiding this comment.
Usually I don't very like to use rust-syntax to derive a different meaning. But here it can be ok as we still reexport ext in the scope, and only impl and use item are allowed.
I also feel a syntax like:
external_impl!{ext, ext2}can be more readable.
There was a problem hiding this comment.
added custom syntax for those blocks following the suggestion as external_impls!{ <listed impls> }
|
/cmd fmt |
|
Command "fmt" has started 🚀 See logs here |
|
Command "fmt" has finished ✅ See logs here |
|
bot update-ui |
|
@pkhry https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7988624 was started for your command Comment |
|
@pkhry Command |
|
/cmd update-ui |
|
Command "update-ui" has started 🚀 See logs here |
|
All GitHub workflows were cancelled due to failure one of the required jobs. |
1 similar comment
|
All GitHub workflows were cancelled due to failure one of the required jobs. |
|
Command "update-ui" has failed ❌! See logs here |
|
/cmd update-ui |
|
Command "update-ui" has started 🚀 See logs here |
|
Command "update-ui" has finished ✅ See logs here |
Description
Adds the ability to split single
impl_runtime_apisinto multiple modules throughsp_api::impl_runtime_apis_ext.Example block would look like:
also see: #3067
Review Notes
The main difference between
impl_runtime_apisandimpl_runtime_apis_extis the fact that the latter doesn't generate base runtime structures and doesn't track the imports to aggregate them inside themetadataandruntime_api_versions.Everything needs to be defined in the same crate due to dependency on
RuntimeApistruct that will need to be imported into theimpl_runtime_apis_extmanually for now.TODOs:
RUNTIME_API_VERSIONwas a
constbefore, i've moved it to a standalone function inside the main impl block.The question is should it be tied to the runtime via trait e.g
RuntimeApiVersionand if yes where should it go?(sp_api or sp_version)I've tried to figure out a way to use a
constbut given that all of theconstdeclarations contain data inside astd::borrow::Cowand and there's no way to get value out in const expression and i've failed to do so, if someone has any idea how to get around this i would really appreciate some input.