Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix errors with generics when using the proc macro #433

Merged
merged 1 commit into from
Aug 18, 2021
Merged

Fix errors with generics when using the proc macro #433

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 26 additions & 19 deletions proc-macros/src/new/render_server.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@ impl RpcDescription {
}

fn render_into_rpc(&self) -> Result<TokenStream2, syn::Error> {
let jrps_error = self.jrps_server_item(quote! { types::Error });
let rpc_module = self.jrps_server_item(quote! { RpcModule });

let mut registered = HashSet::new();
Expand All @@ -62,6 +61,18 @@ impl RpcDescription {
}
};

/// Helper that will ignore results of `register_*` method calls, and panic
/// if there have been any errors in debug builds.
///
/// The debug assert is a safeguard should the contract that guarantees the method
/// names to never conflict in the macro be broken in the future.
fn handle_register_result(tokens: TokenStream2) -> TokenStream2 {
quote! {{
let res = #tokens;
debug_assert!(res.is_ok(), "RPC macro method names should never conflict, this is a bug, please report it.");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why make this a debug-only assert? Isn't it worth the cost to catch these violations in all builds?

Also, I'd clarify that the bug is in the user code, in not jsonrpsee:

Suggested change
debug_assert!(res.is_ok(), "RPC macro method names should never conflict, this is a bug, please report it.");
debug_assert!(res.is_ok(), "RPC names must never conflict, please ensure all method names are unique.");

Copy link
Member

@niklasad1 niklasad1 Aug 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just related to jsonrpsee proc macros that actually ensures that each method is unique so the user should not have to do it, it would just be a compile time error then.

But this is just assertion if we introduce any bugs in that logic and I think Maciej doesn't like overhead of expect in release builds :P

Alternatively we could just add a test for this

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively we could just add a test for this

How would that work? Wouldn't it have to be in the users' code?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it has to be in users code but we could just do a UI test:

#[rpc(client, server, namespace = "state")]
pub trait Rpc {
	/// Async method call example.
	#[method(name = "getPairs")]
	async fn storage_pairs(&self, prefix: usize, hash: Option<u128>) -> Vec<usize>;

	/// Async method call example.
	#[method(name = "getPairs")]
	async fn dup(&self, prefix: usize, hash: Option<u128>) -> Vec<usize>;
}

pub struct RpcServerImpl;

#[async_trait]
impl RpcServer for RpcServerImpl {
	async fn storage_pairs(&self, _prefix: usize, _hash: Option<u128>) -> Vec<usize> {
		vec![1, 2, 3, 4]
	}

	async fn dup(&self, _prefix: usize, _hash: Option<u128>) -> Vec<usize> {
		vec![1, 2, 3, 4]
	}
}

expect error: error: "state_getPairs" is already defined

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already got this: https://github.com/paritytech/jsonrpsee/blob/master/proc-macros/tests/ui/incorrect/rpc/rpc_name_conflict.rs so I merged this PR

}}
}

let methods = self
.methods
.iter()
Expand All @@ -78,22 +89,22 @@ impl RpcDescription {
check_name(rpc_method_name.clone(), rust_method_name.span());

if method.signature.sig.asyncness.is_some() {
quote! {
handle_register_result(quote! {
rpc.register_async_method(#rpc_method_name, |params, context| {
let fut = async move {
#parsing
Ok(context.as_ref().#rust_method_name(#params_seq).await)
};
Box::pin(fut)
})?;
}
})
})
} else {
quote! {
handle_register_result(quote! {
rpc.register_method(#rpc_method_name, |params, context| {
#parsing
Ok(context.#rust_method_name(#params_seq))
})?;
}
})
})
}
})
.collect::<Vec<_>>();
Expand All @@ -116,12 +127,12 @@ impl RpcDescription {
check_name(rpc_sub_name.clone(), rust_method_name.span());
check_name(rpc_unsub_name.clone(), rust_method_name.span());

quote! {
handle_register_result(quote! {
rpc.register_subscription(#rpc_sub_name, #rpc_unsub_name, |params, sink, context| {
#parsing
Ok(context.as_ref().#rust_method_name(sink, #params_seq))
})?;
}
})
})
})
.collect::<Vec<_>>();

Expand All @@ -131,17 +142,13 @@ impl RpcDescription {
Ok(quote! {
#[doc = #doc_comment]
fn into_rpc(self) -> #rpc_module<Self> {
let inner = move || -> Result<#rpc_module<Self>, #jrps_error> {
let mut rpc = #rpc_module::new(self);

#(#errors)*
#(#methods)*
#(#subscriptions)*
let mut rpc = #rpc_module::new(self);

Ok(rpc)
};
#(#errors)*
#(#methods)*
#(#subscriptions)*

inner().expect("RPC macro method names should never conflict")
rpc
}
})
}
Expand Down