Type-Safe API for User-defined RPCs

[ThePoultryMan]

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]

These should ideally delegate to (hidden) functions in the library, to keep the number of codegen tokens as low as possible.

Does the evaluation of vslice![#rpc_self_args] necessarily need to happen only here? Or could be the builder just store Vec<Variant> or &[Variant] directly?

[Bromeon]

Further, they discard the return type.

According to docs:

May return @GlobalScope.OK if the call is successful, @GlobalScope.ERR_INVALID_PARAMETER if the arguments passed in the method do not match, @GlobalScope.ERR_UNCONFIGURED if the node's multiplayer cannot be fetched (such as when the node is not inside the tree), @GlobalScope.ERR_CONNECTION_ERROR if multiplayer's connection is not available.

It would be good if we could have our custom error type as meta::error::RpcError, and then return Result<(), RpcError>. The type doesn't need to be an enum, just something with reasonable Display impls.

All this translation should happen in the library, not in generated code.

[ThePoultryMan]

Does the evaluation of vslice![#rpc_self_args] necessarily need to happen only here? Or could be the builder just store Vec<Variant> or &[Variant] directly?

As of b388168, the builders have named fields with the Variant type, and convert from the raw types into Variant at construction. I personally don't use optionals, so I'm going to confirm my assumptions about them before collapsing the stored parameters into a Vec or slice.

Further, they discard the return type.

Adding to the todo-list

The type doesn't need to be an enum, just something with reasonable Display impls.

What would be the reason to not use an enum? And if we aren't using an enum, what would be used instead? A new-type around an int (RpcError(i32))?

[Bromeon]

You're right, maybe in this case it can make sense to have an enum to differentiate the 3 Godot errors, but overall the API should largely follow existing types in meta::error module.

[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]

This shouldn't be necessary.

[Bromeon]

The comment doesn't say much -- it should elaborate why. Is it for mocking purposes? Functional limitations? Something else?

[Bromeon]

Suggested change

	assert_eq!(Ok(()), node.rpcs().say_hello_to("godot".to_owned()).call());
	let arg = "godot".to_string();
	assert_eq!(Ok(()), node.rpcs().say_hello_to(arg).call());

And then reuse the variable further down.

[Bromeon]

I guess MultiplayerApi is needed because of this?

Note: You can only safely use RPCs on clients after you received the MultiplayerAPI.connected_to_server signal from the MultiplayerAPI. You also need to keep track of the connection state, either by the MultiplayerAPI signals like MultiplayerAPI.server_disconnected or by checking (get_multiplayer().peer.get_connection_status() == CONNECTION_CONNECTED).

Because it limits this file to __codegen-full feature, which reduces the number of scenarios where RPCs can be tested. But I guess it's OK?

[Bromeon]

This seems to be only used in a # expansion within quote!.

As such, the .collect() call is only needed if you run into borrow issues -- otherwise #variable can expand iterators as well.

[Bromeon]

Please remove the TODO -- as mentioned, optional parameters aren't something I plan to support right now.

[Bromeon]

I wonder, should we make Inherits<Node> a supertrait of WithUserRpcs?

There's no scenario where user-defined RPCs can be used if the class isn't Node based, so this would require fewer bounds, also in generic user code.

[Bromeon]

You can inject this via test argument, see e.g. here:

gdext/itest/rust/src/object_tests/object_test.rs

Lines 871 to 876 in ed460cd

	#[itest]
	fn object_get_scene_tree(ctx: &TestContext) {
	let node = Node3D::new_alloc();
	let mut tree = ctx.scene_tree.clone();
	tree.add_child(&node);

[Bromeon]

Strictly speaking, these are arguments (values passed at the call site), not parameters (declaration-site types/values). Could you rename it?

Also, as written in https://github.com/godot-rust/gdext/pull/1535/changes#r3075179469, you could probably just store a slice here? (Assuming lifetime 'c would work...)

[Bromeon]

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