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.

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

Allow trait bounds to be overridden in macro #808

Merged
merged 13 commits into from
Jul 4, 2022
Merged

Conversation

lexnv
Copy link
Contributor

@lexnv lexnv commented Jun 24, 2022

This PR adds the server_bounds and client_bounds attributes of the rpc macro.

The attributes allow users to specify custom bounds for their traits, as a way to opt-out
of the default Send + Sync + 'static where appropriate.

Example

Without the custom provided bounds, the RPC macro would add by default
Conf : Send + Sync + 'static + jsonrpsee::core::DeserializeOwned for client implementation and
Conf : Send + Sync + 'static + jsonrpsee::core::Serialize for server implementation.
This rule is quite restrictive, as we need just Conf::Hash to comply to those rules.

pub trait Config {
	type Hash: Send + Sync + 'static;
}

impl Config for ExampleHash {
	type Hash = Self;
}

#[rpc(server, client, namespace = "foo", client_bounds(Conf::Hash: jsonrpsee::core::DeserializeOwned), server_bounds(Conf::Hash: jsonrpsee::core::Serialize))]
pub trait Rpc<Conf: Config> {
	#[method(name = "bar")]
	fn method(&self) -> Result<Conf::Hash, Error>;
}

While at it, add a new example illustrating the custom bounds and a few UI tests to capture
proper error messages.

Closes #696.

lexnv added 8 commits June 23, 2022 16:22
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv lexnv requested a review from a team as a code owner June 24, 2022 12:49
@lexnv lexnv self-assigned this Jun 24, 2022
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
type ExampleHash = [u8; 32];

pub trait Config {
type Hash: Send + Sync + 'static;
Copy link
Member

@niklasad1 niklasad1 Jun 28, 2022

Choose a reason for hiding this comment

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

IIRC: before/"currently" we add the trait bound the entire trait right?

Can you just verify that we don't add trait bounds on associated types that are not used in the RPC trait?

such as:

pub trait Config {
   type Hash: Send + Sync + 'static;
   type NotUsed;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Debugging the proc-macro, it seems that "currently" (without custom bounds attribute):

pub trait Config {
	type Hash: Send + Sync + 'static;
	type NotUsed;
}

#[rpc(server, namespace = "foo")]
pub trait Rpc<Conf: Config> {
	#[method(name = "bar")]
	fn method(&self) -> Result<Conf::Hash, Error>;
}

The rpc macro implies just: Conf : Send + Sync + 'static + jsonrpsee :: core :: Serialize.

Although, this fails to compile due to:

55  | #[rpc(server, client, namespace = "foo", client_bounds(Conf::Hash: jsonrpsee::core::DeserializeOwned))]
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `Serialize` is not implemented for `<Conf as Config>::Hash`

Copy link
Member

@niklasad1 niklasad1 Jun 28, 2022

Choose a reason for hiding this comment

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

Yeah, that's fine I meant with the overrides for the Hash

pub trait Config {
	type Hash: Send + Sync + 'static;
	type NotUsed;
}

#[rpc(server, client, client_bounds(Conf::Hash: jsonrpsee::core::Serialize))]
pub trait Rpc<Conf: Config> {
	#[method(name = "bar")]
	fn method(&self) -> Result<Conf::Hash, Error>;
}

Such that it doesn't expand to something like:

pub trait Rpc<T: Client, C: Config> 
where 
        C::Hash: Serialize,
        // NotUsed is not used so the trait bounds are needless
        // and should not be added.
        C::NotUsed: Send + Sync + Serialize + DeserializeOwned
{
	#[method(name = "bar")]
	fn method(&self) -> Result<Conf::Hash, Error>;
}

But as it works in subxt all fine, IIRC we only inspect the parameters and return types anyway and add bounds on them so all good.

Copy link
Member

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

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

great.

some UI tests that it actually work as well then this should be ready to go :)

@niklasad1
Copy link
Member

I just tested in on my hacky subxt RPC macro PR and it works 🎉

paritytech/subxt#431

lexnv added 2 commits June 28, 2022 20:26
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@@ -63,7 +63,7 @@ impl RpcDescription {
#(#sub_impls)*
}

impl<T #(,#type_idents)*> #trait_name #type_generics for T where T: #super_trait #(,#where_clause)* {}
impl<TypeJsonRpseeInteral #(,#type_idents)*> #trait_name #type_generics for TypeJsonRpseeInteral where TypeJsonRpseeInteral: #super_trait #(,#where_clause)* {}
Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Member

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

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

excellent, nice work

some minor suggestions on testing but really good to have this finally

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@jsdw
Copy link
Collaborator

jsdw commented Jul 4, 2022

Just wondering; is it possible to define something like:

#[rpc(server, client, client_bounds(Conf::Hash: jsonrpsee::core::Serialize))]
pub trait Rpc<Conf: Config> {
	fn generic_input<T>(&self, input: T) -> Result<Conf::Hash, Error>;
        fn generic_output<O>(&self, input: T) -> Result<O, Error>;
}

And if so, can we control the bounds of T and O? This may be a dumb question :)

Copy link
Collaborator

@jsdw jsdw left a comment

Choose a reason for hiding this comment

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

Not sure whether the question above makes sense or not (I can't recall whether it's possible to add generic bounds in that way), but regardless, this looks great; nice job!

@lexnv
Copy link
Contributor Author

lexnv commented Jul 4, 2022

Managed to define something similar:

#[rpc(client, client_bounds(
	Conf::Hash: jsonrpsee::core::DeserializeOwned,
	T: jsonrpsee::core::Serialize + Send + Sync + 'static,
	O: jsonrpsee::core::DeserializeOwned)
)]
pub trait Rpc2<Conf: Config, T, O> {
	#[method(name = "bar")]
	fn generic_input<T>(&self, input: T) -> Result<Conf::Hash, Error>;
	#[method(name = "foo")]
	fn generic_output<T, O>(&self, input: T) -> Result<O, Error>;
}

with T: jsonrpsee::core::Serialize + Send + Sync + 'static (as it is input for client) and O: jsonrpsee::core::DeserializeOwned (output)

@niklasad1
Copy link
Member

niklasad1 commented Jul 4, 2022

Just wondering; is it possible to define something like:

#[rpc(server, client, client_bounds(Conf::Hash: jsonrpsee::core::Serialize))]
pub trait Rpc<Conf: Config> {
	fn generic_input<T>(&self, input: T) -> Result<Conf::Hash, Error>;
        fn generic_output<O>(&self, input: T) -> Result<O, Error>;
}

And if so, can we control the bounds of T and O? This may be a dumb question :)

Bounds on individual methods doesn't work because the server generates fn into_rpc(self) -> jsonrpsee::RpcModule<Self> and then these "type params" are not bound to anything so it won't compile.

We would need to find all individual type params and generate fn into_rpc<T1, ... Tn>(self) -> jsonrpsee::RpcModule<Self>
then it would work I guess.

I think the same applies to client trait too i.e, that we don't really parse type params on individual methods.

I guess we should throw a compile error for bounds on individual methods until that's allowed, you must really have a bound on the entire trait for now.

@niklasad1 niklasad1 merged commit d974914 into master Jul 4, 2022
@niklasad1 niklasad1 deleted the 696_trait_bounds branch July 4, 2022 12:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow trait bounds to be overridden in macro
3 participants