Type-Safe API for User-defined RPCs

[ThePoultryMan]

Edit bromeon: closes #1158

This is a basic implementation of generating a type-safe api for calling RPCs in rust code.

Design

A rpcs() method is exposed on both the owning class/node and the Gd<T> smart pointer to it. Calling this method will build a struct containing a UserRpcObject enum, and a method that returns a RPC-builder for each user-defined RPC. Calling one of these RPC-builder methods will pass the UserRpcObject into a builder. The builder can then be used to call the RPC via call() or call_id(i64).

UserRpcObject contains two variants: 1) Internal containing a mutable reference to a GodotClass that implements WithBaseField and 2) External containing a Gd pointer.

Method parameters of the RPC function are passed in the RPC-builder function and stored in the builder until they are used when the RPC is called.

This API differs from the one proposed in #1158 (.rpc() & .rpc_id(id)) for two reasons:

1. It brings the API more inline with the signal API.
2. It makes it easier to add optional parameters without having to split the api style:

// We could require a `.done()` call at the end, but that brings us back to where we
// are with this implementation.
node.rpc().rpc_with_optionals();
node.rpc().rpc_with_optionals().optional(3).done();

vs.

node.rpcs().rpc_with_optionals().call();
node.rpcs().rpc_with_optionals().optional(3).call();

Regardless, switching to the style proposed in #1158 wouldn't be hard.

Example

// class definition, must inherit `Node`.
use godot::prelude::*;

#[derive(GodotClass)]
#[class(base = Node, init)]
struct MyClass {
  base: Base<Node>
}

#[godot_api]
impl MyClass {
  #[rpc]
  pub fn my_rpc(&mut self) {}
}

// somewhere else.
let mut my_class: MyClass = ...;
my_class.rpcs().my_rpc().call();

let mut my_class_gd: Gd<MyClass> = MyClass::new_alloc();
my_class_gd.rpcs().my_rpc().call_id(1);

Todo (non-exhaustive)

• Document the api & traits.
• Tests
  • The rpc_test.rs acts as an indirect test to see if the generated code compiles, but dedicated test(s) should be added.
• Fix anything that these changes break
• Attempt to loosen trait bounds where possible.
• Attempt to reduce the amount of generated code
• Attempt to reduce the runtime impact
• Propagate errors from the rpc()/rpc_id() godot calls

Future Works

This implementation is not the final stop. Several more pieces need to be addressed, either in this PR, or future PRs.

• Optional Parameters: The generated builders need to have support for optional parameters added. Currently the builder requires all parameters upfront (which I believe includes optional parameters, though I may be wrong).
• Documentation Generation: Generated builders and functions should probably be documented. They could take the documentation from the original function, or generate new documentation.
  In my original, external, implementation of this system, generated documentation included the parameter names & types.

[GodotRust]

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-1535

[Bromeon]

Thanks a lot for your contribution! 👍

---

This API differs from the one proposed in #1158 (.rpc() & .rpc_id(id)) for two reasons:

1. It brings the API more inline with the signal API.
2. It makes it easier to add optional parameters without having to split the api style:

Good design rationale; I think being closer to signals is a plus.

---

Optional Parameters: The generated builders need to have support for optional parameters added. Currently the builder requires all parameters upfront (which I believe includes optional parameters, though I may be wrong).

Default parameters are something that could maybe be discussed once RPCs work flawlessly otherwise, but I wouldn't say it's a required feature for remote procedure calls, and we'd definitely need strong use cases first.
To keep in mind:

• Neither #[signal] nor #[func] supports Rust-side defaults, and #[rpc] is even more niche. The only place where we support default arguments are engine methods, and one reason for it is that it's the only way to not have breaking changes when new parameters are added. Users on the other hand are in full control of their RPC methods.
• Rust itself doesn't have them either. There's the argument that APIs without default parameters are more verbose, but more robust -- you explicitly state what's being sent over the network, it's harder to have version or server/client discrepancies on default values.
• The Ex* builders are a pain to maintain; in every single refactoring they've required special treatment, with lifetimes, extra types, scoping, visibility etc.
• Ex* builders generate considerable amount of code, which is bad for compile times.

But it's nice to keep the design forward-compatible, should we add it one day 🙂

---

Somehow the rpcs() name feels strange to me; maybe it's the fact that it's only consonants, or that it's "remote procedure calls" rather than "remote procedures", but it's consistent with signals() and I also don't have a better idea... 🤔

[Bromeon]

This can actually be quite hard, especially if you also want to take into account struct visibility. I think there are some comments on visibility regarding signals, and there's a questionable implementation via decl-macros now.

Maybe that could be reused, or we can just support a subset of possible visibilities.

[Bromeon]

Now looking a bit deeper at some technicals.

One thing to note is that with the original design (immediate call instead of terminal .call() / .call_id(id)), we would probably not need to generate intermediate structs, resulting in lower implementation complexity + codegen size. But it does limit some use cases and is slightly different from signals. It's a trade-off we should analyze more carefully...

In Godot, RPCs can be configured to call_local mode. We should probably test this, also regarding re-entrancy (with the &mut self borrow, can we still invoke another method on the same object?).

---

I'm aware that I'm pointing out lots of issues, and fixing all at once can be a bit overwhelming. Don't worry, I don't expect the initial PR to be perfect in this sense 🙂 it's more so that we can collect potential issues early on, and get a plan to address them, both now and later.

[Bromeon]

This may already be an existing problem with RPCs, but it looks like this doesn't honor #[func(rename = ...)].

We should use the Godot method name if anyhow possible.

[ValorZard]

Personally, I’d Ike to offer some encouragement and say this is really dope, keep it up!

[ThePoultryMan]

8aa0050 is the biggest change here. It effectively replaces all generics in the generated code with direct references to the class holding the RPCs. It also removes all unnecessary bounds from generics and traits. The only bound kept is for GodotClass because it is required by Gd.

[ThePoultryMan]

For reference, as of b388168, this is what the generated code for the 14 RPCs in "itest/rust/src/register_tests/rpc_test.rs" looks like:

Details

use __RpcTest_rpcs::*;
mod __RpcTest_rpcs {
    #![allow(non_camel_case_types)]
    use ::godot::obj::{RpcCollection, UserRpcObject};

    use super::*;
    #[doc(hidden)]
    pub struct __RpcTestRpcCollection<'c> {
        object: UserRpcObject<'c, RpcTest>,
    }
    impl<'c> __RpcTestRpcCollection<'c> {
        #[must_use]
        pub fn default_args(self) -> __RpcBuilder_default_args<'c> {
            __RpcBuilder_default_args {
                object: self.object,
            }
        }
        #[must_use]
        pub fn arg_any_peer(self) -> __RpcBuilder_arg_any_peer<'c> {
            __RpcBuilder_arg_any_peer {
                object: self.object,
            }
        }
        #[must_use]
        pub fn arg_authority(self) -> __RpcBuilder_arg_authority<'c> {
            __RpcBuilder_arg_authority {
                object: self.object,
            }
        }
        #[must_use]
        pub fn arg_reliable(self) -> __RpcBuilder_arg_reliable<'c> {
            __RpcBuilder_arg_reliable {
                object: self.object,
            }
        }
        #[must_use]
        pub fn arg_unreliable(self) -> __RpcBuilder_arg_unreliable<'c> {
            __RpcBuilder_arg_unreliable {
                object: self.object,
            }
        }
        #[must_use]
        pub fn arg_unreliable_ordered(self) -> __RpcBuilder_arg_unreliable_ordered<'c> {
            __RpcBuilder_arg_unreliable_ordered {
                object: self.object,
            }
        }
        #[must_use]
        pub fn arg_call_local(self) -> __RpcBuilder_arg_call_local<'c> {
            __RpcBuilder_arg_call_local {
                object: self.object,
            }
        }
        #[must_use]
        pub fn arg_call_remote(self) -> __RpcBuilder_arg_call_remote<'c> {
            __RpcBuilder_arg_call_remote {
                object: self.object,
            }
        }
        #[must_use]
        pub fn arg_channel(self) -> __RpcBuilder_arg_channel<'c> {
            __RpcBuilder_arg_channel {
                object: self.object,
            }
        }
        #[must_use]
        pub fn all_args(self) -> __RpcBuilder_all_args<'c> {
            __RpcBuilder_all_args {
                object: self.object,
            }
        }
        #[must_use]
        pub fn args_func(self) -> __RpcBuilder_args_func<'c> {
            __RpcBuilder_args_func {
                object: self.object,
            }
        }
        #[must_use]
        pub fn args_func_gd_self(self) -> __RpcBuilder_args_func_gd_self<'c> {
            __RpcBuilder_args_func_gd_self {
                object: self.object,
            }
        }
        #[must_use]
        pub fn arg_config_const(self) -> __RpcBuilder_arg_config_const<'c> {
            __RpcBuilder_arg_config_const {
                object: self.object,
            }
        }
        #[must_use]
        pub fn arg_config_fn(self) -> __RpcBuilder_arg_config_fn<'c> {
            __RpcBuilder_arg_config_fn {
                object: self.object,
            }
        }
    }
    impl<'c> RpcCollection<'c, RpcTest> for __RpcTestRpcCollection<'c> {
        fn from_user_rpc_object(object: UserRpcObject<'c, RpcTest>) -> Self {
            __RpcTestRpcCollection { object }
        }
    }
    impl<'c> ::godot::obj::WithUserRpcs<'c, RpcTest> for RpcTest
    where
        RpcTest: ::godot::obj::WithBaseField,
    {
        type Collection = __RpcTestRpcCollection<'c>;
        fn rpcs(&'c mut self) -> Self::Collection {
            Self::Collection::from_user_rpc_object(UserRpcObject::Internal(self))
        }
    }
    #[doc(hidden)]
    pub struct __RpcBuilder_default_args<'c> {
        object: UserRpcObject<'c, RpcTest>,
    }
    impl<'c> __RpcBuilder_default_args<'c> {
        pub fn call(mut self) {
            self.object.call_rpc("default_args", &[]);
        }
        pub fn call_id(mut self, id: i64) {
            self.object.call_rpc_id("default_args", id, &[]);
        }
    }
    #[doc(hidden)]
    pub struct __RpcBuilder_arg_any_peer<'c> {
        object: UserRpcObject<'c, RpcTest>,
    }
    impl<'c> __RpcBuilder_arg_any_peer<'c> {
        pub fn call(mut self) {
            self.object.call_rpc("arg_any_peer", &[]);
        }
        pub fn call_id(mut self, id: i64) {
            self.object.call_rpc_id("arg_any_peer", id, &[]);
        }
    }
    #[doc(hidden)]
    pub struct __RpcBuilder_arg_authority<'c> {
        object: UserRpcObject<'c, RpcTest>,
    }
    impl<'c> __RpcBuilder_arg_authority<'c> {
        pub fn call(mut self) {
            self.object.call_rpc("arg_authority", &[]);
        }
        pub fn call_id(mut self, id: i64) {
            self.object.call_rpc_id("arg_authority", id, &[]);
        }
    }
    #[doc(hidden)]
    pub struct __RpcBuilder_arg_reliable<'c> {
        object: UserRpcObject<'c, RpcTest>,
    }
    impl<'c> __RpcBuilder_arg_reliable<'c> {
        pub fn call(mut self) {
            self.object.call_rpc("arg_reliable", &[]);
        }
        pub fn call_id(mut self, id: i64) {
            self.object.call_rpc_id("arg_reliable", id, &[]);
        }
    }
    #[doc(hidden)]
    pub struct __RpcBuilder_arg_unreliable<'c> {
        object: UserRpcObject<'c, RpcTest>,
    }
    impl<'c> __RpcBuilder_arg_unreliable<'c> {
        pub fn call(mut self) {
            self.object.call_rpc("arg_unreliable", &[]);
        }
        pub fn call_id(mut self, id: i64) {
            self.object.call_rpc_id("arg_unreliable", id, &[]);
        }
    }
    #[doc(hidden)]
    pub struct __RpcBuilder_arg_unreliable_ordered<'c> {
        object: UserRpcObject<'c, RpcTest>,
    }
    impl<'c> __RpcBuilder_arg_unreliable_ordered<'c> {
        pub fn call(mut self) {
            self.object.call_rpc("arg_unreliable_ordered", &[]);
        }
        pub fn call_id(mut self, id: i64) {
            self.object.call_rpc_id("arg_unreliable_ordered", id, &[]);
        }
    }
    #[doc(hidden)]
    pub struct __RpcBuilder_arg_call_local<'c> {
        object: UserRpcObject<'c, RpcTest>,
    }
    impl<'c> __RpcBuilder_arg_call_local<'c> {
        pub fn call(mut self) {
            self.object.call_rpc("arg_call_local", &[]);
        }
        pub fn call_id(mut self, id: i64) {
            self.object.call_rpc_id("arg_call_local", id, &[]);
        }
    }
    #[doc(hidden)]
    pub struct __RpcBuilder_arg_call_remote<'c> {
        object: UserRpcObject<'c, RpcTest>,
    }
    impl<'c> __RpcBuilder_arg_call_remote<'c> {
        pub fn call(mut self) {
            self.object.call_rpc("arg_call_remote", &[]);
        }
        pub fn call_id(mut self, id: i64) {
            self.object.call_rpc_id("arg_call_remote", id, &[]);
        }
    }
    #[doc(hidden)]
    pub struct __RpcBuilder_arg_channel<'c> {
        object: UserRpcObject<'c, RpcTest>,
    }
    impl<'c> __RpcBuilder_arg_channel<'c> {
        pub fn call(mut self) {
            self.object.call_rpc("arg_channel", &[]);
        }
        pub fn call_id(mut self, id: i64) {
            self.object.call_rpc_id("arg_channel", id, &[]);
        }
    }
    #[doc(hidden)]
    pub struct __RpcBuilder_all_args<'c> {
        object: UserRpcObject<'c, RpcTest>,
    }
    impl<'c> __RpcBuilder_all_args<'c> {
        pub fn call(mut self) {
            self.object.call_rpc("all_args", &[]);
        }
        pub fn call_id(mut self, id: i64) {
            self.object.call_rpc_id("all_args", id, &[]);
        }
    }
    #[doc(hidden)]
    pub struct __RpcBuilder_args_func<'c> {
        object: UserRpcObject<'c, RpcTest>,
    }
    impl<'c> __RpcBuilder_args_func<'c> {
        pub fn call(mut self) {
            self.object.call_rpc("args_func", &[]);
        }
        pub fn call_id(mut self, id: i64) {
            self.object.call_rpc_id("args_func", id, &[]);
        }
    }
    #[doc(hidden)]
    pub struct __RpcBuilder_args_func_gd_self<'c> {
        object: UserRpcObject<'c, RpcTest>,
    }
    impl<'c> __RpcBuilder_args_func_gd_self<'c> {
        pub fn call(mut self) {
            self.object.call_rpc("args_func_gd_self", &[]);
        }
        pub fn call_id(mut self, id: i64) {
            self.object.call_rpc_id("args_func_gd_self", id, &[]);
        }
    }
    #[doc(hidden)]
    pub struct __RpcBuilder_arg_config_const<'c> {
        object: UserRpcObject<'c, RpcTest>,
    }
    impl<'c> __RpcBuilder_arg_config_const<'c> {
        pub fn call(mut self) {
            self.object.call_rpc("arg_config_const", &[]);
        }
        pub fn call_id(mut self, id: i64) {
            self.object.call_rpc_id("arg_config_const", id, &[]);
        }
    }
    #[doc(hidden)]
    pub struct __RpcBuilder_arg_config_fn<'c> {
        object: UserRpcObject<'c, RpcTest>,
    }
    impl<'c> __RpcBuilder_arg_config_fn<'c> {
        pub fn call(mut self) {
            self.object.call_rpc("arg_config_fn", &[]);
        }
        pub fn call_id(mut self, id: i64) {
            self.object.call_rpc_id("arg_config_fn", id, &[]);
        }
    }
}

It's a lot. Any ideas on how to reduce it are welcome.

---

About the API design: We could drop the .call() and .call_id(), if we don't support optional parameters. ID selection would have to be somewhere else in the API, probably as .rpcs_id(i64) or .rpcs().my_rpc_id(i64). I think that it would be a weird and clunky API, but it could reduce the amount of generated code.

An additional reason to avoid the .rpc() API proposed #1158 is that .rpc() is already a method on any Gd that holds an object that inherits from Node, so the same API couldn't be exposed on Gd, if it is desired that Gd has access to type-safe RPCs.

[ThePoultryMan]

Also, I cannot figure out why this test is failing.

[ValorZard]

I’d like to see https://github.com/godot-rust/demo-projects/tree/master/net-pong
Get updated with the type safe RPCs once this PR gets merged

(we could also resurrect the old multiplayer-bomber example at some point as well)

[Bromeon]

It's a lot. Any ideas on how to reduce it are welcome.

Several options:

1. Biggest impact would really be the other API design .rpcs().method(args) rather than .rpcs().method().call(args), but we should fully list what we're sacrificing in return, before going that way.

2. For signals, the actual logic is implemented in TypedSignal, which has a generic emit_tuple() method taking tuples. Apart from having business logic in one place, this also allows generic programming -- otherwise, the individual RPC structs have nothing in common. But I don't know if that flexibility is truly needed here.

3. We can additionally provide an opt-out, like with signals:

   #[godot_api(no_typed_rpcs)]

   We might even consider a combined attribute key for both...

---

Also, I cannot figure out why this test is failing.

Can you try running it locally?

---

@ValorZard

I’d like to see https://github.com/godot-rust/demo-projects/tree/master/net-pong
Get updated with the type safe RPCs once this PR gets merged

Sounds good! Feel free to open a PR once that happens 🙂

[ThePoultryMan]

bfb9948 is, in my opinion, the best compromise for reducing generated code. Since every builder, for non-optional-containing RPCs, can be represented by a single format, I think it's best we do that. Later down the line, we can extend the generic builder within the codegen for optionals, so we would end up with something like:

// where `bar` and `baz` are optional parameters
__RpcBuilder_foo<'c, C: GodotClass> {
  base_builder: GenericRpcBuilder<'c, C>,
  bar: Option<Variant>,
  baz: Option<Variant>,
}

This method would allows us to skip unnecessary code generation where it is unneeded, but the caveat is that we are putting more processing into the runtime environment.

---

9b2099d should fix re-entry issues by using .base_mut() instead of .to_gd().

[Bromeon]

Thanks for the updates!

bfb9948 is, in my opinion, the best compromise for reducing generated code. Since every builder, for non-optional-containing RPCs, can be represented by a single format, I think it's best we do that.

Great approach 👍 I like that each RPC mostly boils down to a single method, and we can reuse the call/call_id APIs.

---

In general, could you keep the naming as close as possible to the existing signal codegen? This makes it easier to see the parallels.

pub trait WithSignals: Inherits<crate::classes::Object> {
    type SignalCollection<'c, C>
    where
        C: WithSignals;

    #[doc(hidden)]
    type __SignalObj<'c>: SignalObject<'c>;

    fn __signals_from_external(external: &Gd<Self>) -> Self::SignalCollection<'_, Self>;
}

[Bromeon]

One more thing, could you locate the new RPC-specific types in a obj::rpc module? Then it would be like obj::signal after #1537.

Maybe WithUserRpcs can stay in obj itself (currently the case for WithSignals/WithUserSignals) 🙂

[ThePoultryMan]

ad7f1d7 removes the C::Base: Inherits<Node> bound from the UserRpcObject. It also replaces the upcasting within the Internal branch with:

self_mut
    .base_mut()
    .clone()
    .owned_cast::<Node>()
    .expect("This is a bug, please report it.")
    .rpc(name, parameters);

If my understanding is correct, this (should) never error because the C: Inherits<Node> bound is present.

---

I created a dedicated submodule for the RPC system, like that done in #1537. I split the RpcBuilder and UserRpcObject into their own modules. However, I did not split the RpcCollection trait, as I did not feel it was necessary. Let me know if you would like those split as well.

[ThePoultryMan]

For any errors that don't have a dedicated variant in RpcError they are assigned to RpcError::OtherUnknown(i32) instead. We could default to another variant, or panic, but this method keeps everything panic-free and still represents the true state.

Other than that, we could consider storing some more information within each variant if we want to improve the error messages.

[Bromeon]

@ThePoultryMan now that v0.5 has been released, we should have a bit more time to delve deeper into RPCs.

Some administrative PR tips:

• If you'd like more people to review this PR, feel free to change it from draft to "ready for review" status.
• The CI should be green for this however, just to make sure people don't chase down compile errors or failing tests
• Also, it's OK to occasionally squash commits -- even force-pushes are shown in GitHub with differences.
• A rebase would also be nice 🙂

If you need help with any of these, don't hesitate to reach out!

[ThePoultryMan]

The doctest was failing due to rust-lang/rust#130274. I worked around that by adding fn main() {} to the test. For this test in particular it should be fine because the test doesn't actually run any code.

[Bromeon]

So is this ready for review? If yes, please change the PR state 🙂

[Bromeon]

Fantastic, this starts to look very nice 👍

[Bromeon]

There are some open comments left -- when addressing these, could you squash the commits? We're getting close to merge 🙂

[Bromeon]

@ThePoultryMan Do you think you'll find time for an update in the near future? 🙂

[ThePoultryMan]

Apologies for the lack of updates, I've been busy with school. I'll be able to pick this up again near the end of next week.

[ThePoultryMan]

This version contain a breaking change: the rpc_name() functions now take references to the parameters instead of an owned type, since we only need a reference for variant conversion. See here for what that looks like.

The only problem here is that, for values that implement Copy, we still need to pass by reference so we end up with something like: my_object.rpc_name(&3).call(). We could implement something (maybe a trait?) that doesn't require references for types that implement Copy, but that might be overengineering a little bit.

[Bromeon]

We could implement something (maybe a trait?) that doesn't require references for types that implement Copy, but that might be overengineering a little bit.

Luckily I did this overengineering already a while ago 😀

AsArg does exactly this, and it's in fact what engine APIs and generated signal emit() functions use. You can look at signal's codegen for implementation, or at tests to see how it looks in action:

gdext/itest/rust/src/builtin_tests/containers/signal_test.rs

Line 145 in b546205

sig.emit(&arg, "hello");

[Bromeon]

Also here, comments are redundant when seeing the declaration. I think it would be more interesting to state when each variant is used, e.g. link to the method that creates it.

My question would again be: does the user ever need to interact with these variants? Would it be better to hide them (either #[doc(hidden)] on variants, which is a bit unusual, or wrap the enum into an opaque struct)?

Nitpick, RustDoc should generally end with a . period.

[ThePoultryMan]

For the test, we do still need to lock it behind codegen-full because, if we don't, the RPCs aren't registered and running them will fail. But, I do have two ideas: 1) When not(feature = "codegen-full"), check to see if they fail as expected OR 2) don't run the checks at all.

Note that, in my testing, registering the RPCs manually will still result in them erroring when codegen-full is disabled.