Fix errors with generics when using the proc macro

[maciejhirsz]

The inner closure was causing issues when parent types were using generics, since those types aren't usable inside closures (unless said closure would be made to be generic).

Since all we used the closure for is collect all errors to .expect on them at the end, this PR strips the closure altogether, and instead handles errors in place: ignoring them and panicking in debug builds if the macro somehow were to produce conflicting names.

[niklasad1]

hmm suppose I want to have this:

// Hash needs some bounds and lifetime but to simplify the point.
#[rpc(server, namespace = "state")]
pub trait Rpc<Hash> {
	/// Async method call example.
	#[method(name = "getPairs")]
	async fn storage_pairs(&self, hash: Hash) -> Vec<usize>;
}

pub struct RpcServerImpl;

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

That won't work right?
First we don't have any generics in the impl then there are the serde impls that I think needs the concrete type?!

Otherwise, please show an example how to use this 🙏

[niklasad1]

code looks good but it's not quite want I had in mind but I guess a step in the right direction?

see my previous comment

[dvdplm]

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.");

[niklasad1]

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

[dvdplm]

Alternatively we could just add a test for this

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

[niklasad1]

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

[niklasad1]

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

[jsdw]

Looks sane to me, and fairly simple/easy to adjust if needed going forwards!