Custom errors

[MOZGIII]

Closes #973.

---

To do:

• permit other paths to Result in the error type rewrite macro code

[dmitrylavrenov]

Useful PR and good test example!

[niklasad1]

Generally looks good and nice improvement.

The only thing I worry about is the readability of client trait i.e if one provides a custom type then
we would modify the error type to jsonrpsee::core::Error under the hood which is hard to understand.

We are already doing this anyway for subscriptions to some extent and generates a slightly different trait.

Give us a couple days to think about it, I reach out again soon.

[jsdw]

I am wondering whether we'd gain a bit more flexibility ultimately by copying Axum a bit and having a trait which is something like

trait IntoResponse {
    fn into_response(self) -> MethodResponse;
}

This could then be impl'd for Result<V, E> where V: Serialize, E: Into<Error> for instance:

impl <V, E> IntoResponse for Result<V, E> 
where
    V: Serialize,
    E: Into<Error>
{
    fn into_response(self) -> MethodResponse {
        // look at impl MethodResponse::response/error and use this here
    }
} 

fns like this register_method would expect a callback which was like Fn(Params, &Context) -> R (where R: IntoResponse) and would call into_response() on the result (perhaps passing in any specific bits they need to as args to that).

Then we could impl things like From<Infallible> for Error to allow Result<V, Infallible> responses for instance, and perhaps have more room then to consider supporting other response types than Result in the future.

Just thinking out loud really; maybe for now it's fine just to support custom errors but still expect a Result back..

[MOZGIII]

Oh yes, I considered it, but ultimately decided against doing huge changes in a single PR. This is something that, I think, can be added later iteratively - and, also, maybe someone else would like to tackle that instead of me. :D I'm fine with looking into it, but I'm also happy to share the fun.

[MOZGIII]

I propose that we split the error layers into jsonrpsee's and application level, and, further, split the jsonrpsee's error from one single struct into separate ClientError and ServerError as there are very distinctly different in handling. I think it would significantly improve the ergonomics, and unlock more explicit and sensible error customization for the client side, as this PR only addresses the server side really.

[MOZGIII]

If you like the direction I proposed I'll create an issue for splitting the errors and mark this discussion as resolved.

[jsdw]

@niklasad1 whatcha reckon? I think you were less keen on splitting the errors?

But in any case, I think for now I'm fine with just allowing the custom error and not being more general; perhaps in some future iteration we can add the above sort of thing if it makes sense :)

[niklasad1]

I think it will be weird to keep the proc macro API i.e, to share the same definition with different error types for the client and server. It's still possible to do it in the proc macro code as you have changed it this PR.

Example both client and server

struct CustomError;

#[rpc(server, client)]
pub trait Rpc
{
	// The client will return `ClientError` here.....
	async fn f(&self) -> Result<String, CustomError>;
}

Example only client

#[rpc(client)]
pub trait Rpc
{
	// I can enter whatever type I want here but it will return ClientError anyway.....
	async fn f(&self) -> Result<String, ClientError>;
}

I mostly care about having the client and server to share the API definition which I like to avoid a bunch of boiler plate and so on.

[MOZGIII]

I'm thinking about it more like this:

struct CustomError;

#[rpc(server, client)]
pub trait Rpc {
	// The client will return `ClientError` here.....
	async fn f(&self) -> Result<String, CustomError>;
}


struct Server;

#[async_trait]
impl RpcServer for Server {
    async fn f(&self) -> Result<String, ServerError<CustomError>>; // <------ see the `ServerError`
}

And the client would have a ClientError or ClientError<CustomError> type.

[niklasad1]

Sure, that idea is interesting but it complicates things in other crates such as core but I would prefer to keep it out of this PR.

That alone would cause plenty of changes

The benefit of splitting it up is that is easier to reason about the error scenarios but forces users to import a few different error types (fine though)

[jsdw]

I had a play and wonder whether this would be a clearer approach to take:

fn return_result_type(&self, ty: &syn::Type) -> TokenStream2 {
	// We expect a valid type path.
	let syn::Type::Path(type_path) = ty else  {
		return quote_spanned!(ty.span() => compile_error!("Expecting something like 'Result<Foo, Err>' here. (1)"));
	};

	// The path (eg std::result::Result) should have a final segment like 'Result'.
	let Some(type_name) = type_path.path.segments.last() else {
		return quote_spanned!(ty.span() => compile_error!("Expecting this path to end in something like 'Result<Foo, Err>'"));
	};

	// Get the generic args eg the <T, E> in Result<T, E>.
	let PathArguments::AngleBracketed(AngleBracketedGenericArguments { args, .. }) = &type_name.arguments else {
		return quote_spanned!(ty.span() => compile_error!("Expecting something like 'Result<Foo, Err>' here, but got no generic args (eg no '<Foo,Err>')."));
	};

	if type_name.ident == "Result"{
		// Result<T, E> should have 2 generic args.
		if args.len() != 2 {
			return quote_spanned!(args.span() => compile_error!("Expecting two generic args here."));
		}
	} else if type_name.ident == "RpcResult" {
		// RpcResult<T> (an alias we export) should have 1 generic arg.
		if args.len() != 1 {
			return quote_spanned!(args.span() => compile_error!("Expecting two generic args here."));
		}
	} else {
		// Any other type name isn't allowed.
		return quote_spanned!(type_name.span() => compile_error!("The response type should be Result or RpcResult"));
	}

	// The first generic arg is our custom return type.
	// Return a new Result with that custom type and jsonrpsee::core::Error:
	let custom_return_type = args.first().unwrap();
	let error_type = self.jrps_client_item(quote! { core::Error });
	quote!(std::result::Result<#custom_return_type, #error_type>)
}

Aside from being documented, it checks for both Result and RpcResult (and verified the number of generics), and returns nicer compile errors which highlight the offending return type when it's wrong.

Instead of modifying the type I just looked for the generic param and then return a new Result type which uses it and the standard error.

Needs testing properly though but it compiles ok!

[MOZGIII]

Yeah, it is a different approach; when choosing between the two I figured I'd rather impose as few restrictions as possible, and thus tried to surgically target the Result's error. My rationale for going this way is, for instance, I'm not certain at all that types other than RpcResult and Result<_, _> are not allowed... But after a quick look through the tests, I can tell it is the case.
So, yeah, there is definitely no such restriction explicitly anywhere just yet, and I wasn't sure it would be the right way of introducing it.

There's also a concern with supporting multiple paths to [RpcResult] - but I've implemented a nice helper for handling paths for [Result] that would work with [RpcResult] too.

[MOZGIII]

Tried it! One unexpected artifact is this UI warning:

┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈
warning: unused import: `core::RpcResult`
 --> .../jsonrpsee/proc-macros/tests/ui/correct/only_client.rs:3:17
  |
3 | use jsonrpsee::{core::RpcResult, proc_macros::rpc};
  |                 ^^^^^^^^^^^^^^^
  |
  = note: `#[warn(unused_imports)]` on by default

This comes from this code:

//! Example of using proc macro to generate working client and server.

use jsonrpsee::{core::RpcResult, proc_macros::rpc};

#[rpc(client)]
pub trait Rpc {
	#[method(name = "foo")]
	async fn async_method(&self, param_a: u8, param_b: String) -> RpcResult<u16>;

	#[method(name = "bar")]
	fn sync_method(&self) -> RpcResult<u16>;

	#[subscription(name = "subscribe", item = String)]
	fn sub(&self);
}

fn main() {}

If we rewrite the code to use Result explicitly it might actually make RpcResult unused! Good thing there are UI tests in this project, here...

---

We can easily handle it by passing through the RpcResult.

[MOZGIII]

What if user has a custom Result type for whatever reason? Shall we replace it with std::result::Result? It is a pretty edge case, yet I'd suggest that we pass through that type too, and just alter the last argument. That would be a bit less surprising for someone injecting custom Results.

[MOZGIII]

I think I have a version that covers all concerns now, please take a look. It does error rewrite again to be more permissive but provides those nice compile errors (for which I need to add tests actually!).

[jsdw]

What if user has a custom Result type for whatever reason? Shall we replace it with std::result::Result? It is a pretty edge case, yet I'd suggest that we pass through that type too, and just alter the last argument. That would be a bit less surprising for someone injecting custom Results.

I think that is the user has some custom type that isn't something we expect (ie a Result or an RpcResult) then offhand the safest option is to complain, since in the general case we can't possibly know what the structure of said custom type is. @niklasad1 do you have any opinions on this?

[jsdw]

FYI I had a look at your latest version; it doesn't look any more permissive to me since it still requires the name to be Result or RpcResult (and the Result type still requires 2 args, so it's highly unlikely somebody will be importing a "Result" type alias of that kind anyway). So I still prefer my approach (and avoiding mutating things for clarity) but I'm not super bothered.

[niklasad1]

I think that is the user has some custom type that isn't something we expect (ie a Result or an RpcResult) then offhand the safest option is to complain, since in the general case we can't possibly know what the structure of said custom type is. @niklasad1 do you have any opinions on this?

Yeah, I agree I think it's better to throw a compile-time error if the error type isn't Result/RpcResult.

[MOZGIII]

The case where this solution is more permissive is when the user provides a type called Result that has the same shape as the std Result, but is a different type. For instance, a struct instead of enum (a case that is unlikely to happen), or a wrapper (much more common, although maybe not for Result), for instance for implementing some traits on it that conflict otherwise.

[MOZGIII]

I think we might want to add a corrsponding code to check for Result/RpcResult to the server side as well. No rewrites just yet (to be added in the subsequest PRs), only the checking that the type must be explicitly Result or RpcResult.

What do you think?

[niklasad1]

I think we might want to add a corrsponding code to check for Result/RpcResult to the server side as well. No rewrites just yet (to be added in the subsequest PRs), only the checking that the type must be explicitly Result or RpcResult.

What do you think?

AFAIU, the only reason we need that for the client is because we are trying to rewrite to return type as Result<T, jsonrpsee::core::Error> and for the server that is not needed.

Thus, the "ordinary" compile error should good enough then.