Multiple #[godot_api] impl blocks #927

0x53A · 2024-10-23T01:10:34Z

fixes #925

use godot::prelude::*;

use godot::classes::Node;
use godot::classes::INode;

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

#[godot_api]
impl TestNode {
    #[func]
    fn foo() {
        godot_print!("[TestNode] called 'foo'");
    }
}



#[godot_api(secondary)]
impl TestNode {
    #[func]
    fn bar() {
        godot_print!("[TestNode] called 'bar'");
    }
}

It errors with acceptable error messages and spans when unsupported keys are added to the attribute:

#[godot_api(anything_except_secondary)]
impl TestNode {
}



#[godot_api(anything)]
impl INode for TestNode {
}

0x53A · 2024-10-23T01:18:50Z

The idea is that the "primary" impl block defines a static Vec<fn()> (two, actually, one for methods, one for constants) and does the class registration, which executes all therein stored callbacks.

The "secondary" impl blocks then just push into these Vecs.

Here is the generated code, for completeness sake:

use godot::prelude::*;

use godot::classes::Node;
use godot::classes::INode;

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

// Recursive expansion of godot_api macro
// =======================================

impl TestNode {
    fn foo() {
        godot_print!("[TestNode] called 'foo'");
    }
}
#[used]
#[allow(non_upper_case_globals)]
#[doc(hidden)]
static __registration_methods_TestNode: std::sync::Mutex<Vec<fn()>> =
    std::sync::Mutex::new(Vec::new());

#[used]
#[allow(non_upper_case_globals)]
#[doc(hidden)]
static __registration_constants_TestNode: std::sync::Mutex<Vec<fn()>> =
    std::sync::Mutex::new(Vec::new());

impl ::godot::obj::cap::ImplementsGodotApi for TestNode {
    fn __register_methods() {
        let guard = __registration_methods_TestNode.lock().unwrap();
        for f in guard.iter() {
            f();
        }
    }
    fn __register_constants() {
        let guard = __registration_constants_TestNode.lock().unwrap();
        for f in guard.iter() {
            f();
        }
    }
}

const _: () = {
    #[allow(non_upper_case_globals)]
    #[used]
    #[cfg_attr(target_os = "windows", link_section = ".CRT$XCU")]
    #[cfg_attr(target_os = "ios", link_section = "__DATA,__mod_init_func")]
    #[cfg_attr(target_os = "macos", link_section = "__DATA,__mod_init_func")]
    #[cfg_attr(target_os = "android", link_section = ".init_array")]
    #[cfg_attr(target_os = "dragonfly", link_section = ".init_array")]
    #[cfg_attr(target_os = "freebsd", link_section = ".init_array")]
    #[cfg_attr(target_os = "linux", link_section = ".init_array")]
    #[cfg_attr(target_os = "netbsd", link_section = ".init_array")]
    #[cfg_attr(target_os = "openbsd", link_section = ".init_array")]
    static __init: extern "C" fn() = {
        #[cfg_attr(target_os = "android", link_section = ".text.startup")]
        #[cfg_attr(target_os = "linux", link_section = ".text.startup")]
        extern "C" fn __inner_init() {
            {
                __registration_methods_TestNode.lock().unwrap().push(|| {
                    {
                        use ::godot::builtin::{StringName, Variant};
                        use ::godot::obj::GodotClass;
                        use ::godot::register::private::method::ClassMethodInfo;
                        use ::godot::sys;
                        type Sig = ((),);
                        let method_name = StringName::from("foo");
                        unsafe extern "C" fn varcall_fn(
                            _method_data: *mut std::ffi::c_void,
                            instance_ptr: sys::GDExtensionClassInstancePtr,
                            args_ptr: *const sys::GDExtensionConstVariantPtr,
                            arg_count: sys::GDExtensionInt,
                            ret: sys::GDExtensionVariantPtr,
                            err: *mut sys::GDExtensionCallError,
                        ) {
                            let call_ctx = ::godot::meta::CallContext::func("TestNode", "foo");
                            ::godot::private::handle_varcall_panic(&call_ctx, &mut *err, || {
                                <Sig as ::godot::meta::VarcallSignatureTuple>::in_varcall(
                                    instance_ptr,
                                    &call_ctx,
                                    args_ptr,
                                    arg_count,
                                    ret,
                                    err,
                                    |_, params| {
                                        let () = params;
                                        TestNode::foo()
                                    },
                                )
                            });
                        };
                        unsafe extern "C" fn ptrcall_fn(
                            _method_data: *mut std::ffi::c_void,
                            instance_ptr: sys::GDExtensionClassInstancePtr,
                            args_ptr: *const sys::GDExtensionConstTypePtr,
                            ret: sys::GDExtensionTypePtr,
                        ) {
                            let call_ctx = ::godot::meta::CallContext::func("TestNode", "foo");
                            let _success = ::godot::private::handle_panic(
                                || &call_ctx,
                                || {
                                    <Sig as ::godot::meta::PtrcallSignatureTuple>::in_ptrcall(
                                        instance_ptr,
                                        &call_ctx,
                                        args_ptr,
                                        ret,
                                        |_, params| {
                                            let () = params;
                                            TestNode::foo()
                                        },
                                        sys::PtrcallType::Standard,
                                    )
                                },
                            );
                        };
                        let method_info = unsafe {
                            ClassMethodInfo::from_signature::<TestNode, Sig>(
                                method_name,
                                Some(varcall_fn),
                                Some(ptrcall_fn),
                                ::godot::global::MethodFlags::NORMAL
                                    | ::godot::global::MethodFlags::STATIC,
                                &[],
                            )
                        };
                        {
                            if false {
                                format_args!("   Register fn:   {}::{}", "TestNode", "foo");
                            }
                        };
                        method_info.register_extension_class_method();
                    };
                });
                __registration_constants_TestNode
                    .lock()
                    .unwrap()
                    .push(|| {});
            }
        }
        __inner_init
    };
    #[cfg(target_family = "wasm")]
    {
        core::panicking::panic_fmt(core::const_format_args!(
            "not yet implemented: {}",
            core::format_args!("wasm not yet implemented")
        ));
    }
};

const _: () = {
    #[allow(non_upper_case_globals)]
    #[used]
    #[cfg_attr(target_os = "windows", link_section = ".CRT$XCU")]
    #[cfg_attr(target_os = "ios", link_section = "__DATA,__mod_init_func")]
    #[cfg_attr(target_os = "macos", link_section = "__DATA,__mod_init_func")]
    #[cfg_attr(target_os = "android", link_section = ".init_array")]
    #[cfg_attr(target_os = "dragonfly", link_section = ".init_array")]
    #[cfg_attr(target_os = "freebsd", link_section = ".init_array")]
    #[cfg_attr(target_os = "linux", link_section = ".init_array")]
    #[cfg_attr(target_os = "netbsd", link_section = ".init_array")]
    #[cfg_attr(target_os = "openbsd", link_section = ".init_array")]
    static __init: extern "C" fn() = {
        #[cfg_attr(target_os = "android", link_section = ".text.startup")]
        #[cfg_attr(target_os = "linux", link_section = ".text.startup")]
        extern "C" fn __inner_init() {
            let mut guard = ::godot::private::__godot_rust_plugin___GODOT_PLUGIN_REGISTRY
                .lock()
                .unwrap();
            guard.push(
                (::godot::private::ClassPlugin {
                    class_name: <TestNode as ::godot::obj::GodotClass>::class_name(),
                    item: ::godot::private::PluginItem::InherentImpl(
                        ::godot::private::InherentImpl {
                            register_methods_constants_fn: ::godot::private::ErasedRegisterFn {
                                raw: ::godot::private::callbacks::register_user_methods_constants::<
                                    TestNode,
                                >,
                            },
                            register_rpcs_fn: Some(::godot::private::ErasedRegisterRpcsFn {
                                raw: ::godot::private::callbacks::register_user_rpcs::<TestNode>,
                            }),
                        },
                    ),
                    init_level: <TestNode as ::godot::obj::GodotClass>::INIT_LEVEL,
                }),
            );
        }
        __inner_init
    };
    #[cfg(target_family = "wasm")]
    godot_ffi::gensym! {
        godot_ffi::plugin_add_inner_wasm!()
    }
};




// Recursive expansion of godot_api macro
// =======================================

impl TestNode {
    fn bar() {
        godot_print!("[TestNode] called 'bar'");
    }
}
const _: () = {
    #[allow(non_upper_case_globals)]
    #[used]
    #[cfg_attr(target_os = "windows", link_section = ".CRT$XCU")]
    #[cfg_attr(target_os = "ios", link_section = "__DATA,__mod_init_func")]
    #[cfg_attr(target_os = "macos", link_section = "__DATA,__mod_init_func")]
    #[cfg_attr(target_os = "android", link_section = ".init_array")]
    #[cfg_attr(target_os = "dragonfly", link_section = ".init_array")]
    #[cfg_attr(target_os = "freebsd", link_section = ".init_array")]
    #[cfg_attr(target_os = "linux", link_section = ".init_array")]
    #[cfg_attr(target_os = "netbsd", link_section = ".init_array")]
    #[cfg_attr(target_os = "openbsd", link_section = ".init_array")]
    static __init: extern "C" fn() = {
        #[cfg_attr(target_os = "android", link_section = ".text.startup")]
        #[cfg_attr(target_os = "linux", link_section = ".text.startup")]
        extern "C" fn __inner_init() {
            {
                __registration_methods_TestNode.lock().unwrap().push(|| {
                    {
                        use ::godot::builtin::{StringName, Variant};
                        use ::godot::obj::GodotClass;
                        use ::godot::register::private::method::ClassMethodInfo;
                        use ::godot::sys;
                        type Sig = ((),);
                        let method_name = StringName::from("bar");
                        unsafe extern "C" fn varcall_fn(
                            _method_data: *mut std::ffi::c_void,
                            instance_ptr: sys::GDExtensionClassInstancePtr,
                            args_ptr: *const sys::GDExtensionConstVariantPtr,
                            arg_count: sys::GDExtensionInt,
                            ret: sys::GDExtensionVariantPtr,
                            err: *mut sys::GDExtensionCallError,
                        ) {
                            let call_ctx = ::godot::meta::CallContext::func("TestNode", "bar");
                            ::godot::private::handle_varcall_panic(&call_ctx, &mut *err, || {
                                <Sig as ::godot::meta::VarcallSignatureTuple>::in_varcall(
                                    instance_ptr,
                                    &call_ctx,
                                    args_ptr,
                                    arg_count,
                                    ret,
                                    err,
                                    |_, params| {
                                        let () = params;
                                        TestNode::bar()
                                    },
                                )
                            });
                        };
                        unsafe extern "C" fn ptrcall_fn(
                            _method_data: *mut std::ffi::c_void,
                            instance_ptr: sys::GDExtensionClassInstancePtr,
                            args_ptr: *const sys::GDExtensionConstTypePtr,
                            ret: sys::GDExtensionTypePtr,
                        ) {
                            let call_ctx = ::godot::meta::CallContext::func("TestNode", "bar");
                            let _success = ::godot::private::handle_panic(
                                || &call_ctx,
                                || {
                                    <Sig as ::godot::meta::PtrcallSignatureTuple>::in_ptrcall(
                                        instance_ptr,
                                        &call_ctx,
                                        args_ptr,
                                        ret,
                                        |_, params| {
                                            let () = params;
                                            TestNode::bar()
                                        },
                                        sys::PtrcallType::Standard,
                                    )
                                },
                            );
                        };
                        let method_info = unsafe {
                            ClassMethodInfo::from_signature::<TestNode, Sig>(
                                method_name,
                                Some(varcall_fn),
                                Some(ptrcall_fn),
                                ::godot::global::MethodFlags::NORMAL
                                    | ::godot::global::MethodFlags::STATIC,
                                &[],
                            )
                        };
                        {
                            if false {
                                format_args!("   Register fn:   {}::{}", "TestNode", "bar");
                            }
                        };
                        method_info.register_extension_class_method();
                    };
                });
                __registration_constants_TestNode
                    .lock()
                    .unwrap()
                    .push(|| {});
            }
        }
        __inner_init
    };
    #[cfg(target_family = "wasm")]
    {
        core::panicking::panic_fmt(core::const_format_args!(
            "not yet implemented: {}",
            core::format_args!("wasm not yet implemented")
        ));
    }
};

0x53A · 2024-10-23T01:20:44Z

It should be possible to get rid of the explicit secondary attribute by keeping static local state in between macro invocations, I'll look at that later

0x53A · 2024-10-23T01:24:35Z

One limitation currently is that you can have multiple impl blocks, but only in the same file.

The Vec<fn()> could be moved to a Map<String, Vec<fn()>> (with key = classname) inside gdext, similar to the PLUGIN_REGISTRY, then you could theoretically have multiple impl blocks, spread over multiple files.

GodotRust · 2024-10-23T08:51:53Z

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

Bromeon · 2024-10-23T08:52:29Z

Wow, impressive 😮 thanks a lot!

It should be possible to get rid of the explicit secondary attribute by keeping static local state in between macro invocations, I'll look at that later

Let's avoid that, stateful macros are a hack that may cease to work in the future, or with sandboxed macros (c.f. experimental projects like https://github.com/dtolnay/watt).

It's a very minor QoL impediment to write "secondary", I don't see it as an issue in practice.

Are multiple #[godot_api(secondary)] blocks supported?

The Vec<fn()> could be moved to a Map<String, Vec<fn()>> (with key = classname) inside gdext, similar to the PLUGIN_REGISTRY, then you could theoretically have multiple impl blocks, spread over multiple files.

Imo it's fine to have a first version with this limitation, and look into this feature in a separate PR. That way, the amount of changes to review and check isn't too big 🙂

0x53A · 2024-10-26T20:33:12Z

~~It's still missing the docs, but~~ I think it's ready for bikeshedding - a review would be appreciated!

Are multiple #[godot_api(secondary)] blocks supported?

yes, unlimited, unless there's a limit w.r.t. the linker etc

have the key 'secondary'

Bromeon · 2024-11-07T13:50:51Z

Thanks! I'm a bit busy with preparing v0.2 release, so review may have to wait a bit, but I haven't forgotten! 🙂

fpdotmonkey · 2024-11-22T11:30:42Z

I can confirm that as of 2a5660f, the automated tests pass on Linux.

fpdotmonkey · 2024-11-22T11:48:43Z

itest/rust/src/object_tests/object_test.rs

+// ----------------------------------------------------------------------------------------------------------------------------------------------
+
+#[derive(GodotClass)]
+#[class(init, base=Object)]


Could you add some kind of IObject impl

to what end?

fpdotmonkey · 2024-11-22T11:58:10Z

godot-macros/src/class/godot_api.rs

+        if meta.to_string() != "" {
+            return bail!(
+                meta,
+                "#[godot_api] on a trait implementation currently does not support any parameters"
+            );
+        }


I'm confused what this is supposed to catch

A user could do #[godot_api(a, b, x=y)], which in the old implementation would just be ignored, and now would throw an error.

The main thought behind this was to catch people accidentally adding secondary to a trait impl block instead of an inherent impl block.

Very good corner case -- could you add this explanation to the code itself as a comment?

Bromeon

Thanks a lot, still impressed how quickly you figured this out! 👍

Added some remarks, most of them smaller bikeshedding things. As a general style rule, comments should typically start with uppercase and end with a period (unless they're really just keywords).

Would also need a rebase.

Bromeon · 2024-11-25T17:03:07Z

itest/rust/src/object_tests/object_test.rs

+/// Test that multiple inherent '#[godot_api]' impl blocks can be registered.
+/// https://github.com/godot-rust/gdext/pull/927
+#[itest]
+fn godot_api_multiple_impl_blocks_pull_927() {


The link is enough, no need to include _pull_927 in the test name 🙂

Bromeon · 2024-11-25T17:04:35Z

itest/rust/src/object_tests/object_test.rs

+        method_name: StringName,
+        expected_result: String,


These could both be &str parameters and convert internally.

Bromeon · 2024-11-25T17:05:21Z

itest/rust/src/object_tests/object_test.rs

+        assert!(result_as_string.is_ok());
+        assert_eq!(expected_result, result_as_string.unwrap());


Better as one statement -- also, expected should be on the right:

Suggested change

assert!(result_as_string.is_ok());

assert_eq!(expected_result, result_as_string.unwrap());

assert_eq!(result_as_string, Ok(expected_result));

Bromeon · 2024-11-25T17:08:39Z

itest/rust/src/object_tests/object_test.rs

@@ -1076,3 +1076,59 @@ fn double_use_reference() {
    double_use.free();
    emitter.free();
 }
+


This isn't really testing objects (Gd) but rather the registration API (godot::register::godot_api macro).

As such, it should be in register_tests directory. Maybe you could even crate a new file multiple_impls_test.rs?

Bromeon · 2024-11-25T17:13:34Z

godot-macros/src/lib.rs

@@ -679,9 +679,36 @@ pub fn derive_godot_class(input: TokenStream) -> TokenStream {
 /// # Constants and signals
 ///
 /// Please refer to [the book](https://godot-rust.github.io/book/register/constants.html).
+///
+/// # Multiple inherent impl blocks


Suggested change

/// # Multiple inherent impl blocks

/// # Multiple inherent `impl` blocks

Also, it's not part of the table of contents in the beginning. Alternatively, you can replace the existing ToC with a mention of Sections in the sidebar, maybe in bold, something along:

For a table of contents, consult the Sections label in the sidebar on the left.

Bromeon · 2024-11-25T17:20:52Z

itest/rust/src/object_tests/object_test.rs

+    // just call all three methods, if that works, then they have all been correctly registered
+    call_and_check_result(&mut obj, "foo".into(), "result of foo".into());
+    call_and_check_result(&mut obj, "bar".into(), "result of bar".into());
+    call_and_check_result(&mut obj, "baz".into(), "result of baz".into());


Suggested change

// just call all three methods, if that works, then they have all been correctly registered

call_and_check_result(&mut obj, "foo".into(), "result of foo".into());

call_and_check_result(&mut obj, "bar".into(), "result of bar".into());

call_and_check_result(&mut obj, "baz".into(), "result of baz".into());

// Just call all three methods; if that works, then they have all been correctly registered.

call_and_check_result(&mut obj, "foo", "result of foo");

call_and_check_result(&mut obj, "bar", "result of bar");

call_and_check_result(&mut obj, "baz", "result of baz");

Bromeon · 2024-11-25T17:21:22Z

godot-macros/src/class/data_models/inherent_impl.rs

+        let result = quote! {
+
+            #impl_block
+
+            #storage
+
+            #trait_impl
+
+            #fill_storage
+
+            #class_registration
+
+        };


Empty lines are helpful when there are multi-line code groups that need to be separated, but here there's individual lines only. Please remove the empty lines in between, including after opening { and before closing }.

Also below 🙂

Bromeon · 2024-11-25T17:22:14Z

godot-macros/src/class/data_models/inherent_impl.rs

        });
    };

-    Ok(result)
+    if !meta.secondary {
+        // we are the primary


Suggested change

// we are the primary

// We are the primary impl block.

Bromeon · 2024-11-25T17:22:31Z

godot-macros/src/class/data_models/inherent_impl.rs

+        let storage = quote! {
+
+            #[used]


Suggested change

let storage = quote! {

#[used]

let storage = quote! {

#[used]

Bromeon · 2024-11-25T17:25:32Z

godot-ffi/src/plugins.rs

@@ -49,14 +49,15 @@ macro_rules! plugin_add_inner_wasm {
    };
 }

+/// executes a block of code before main by utilising platform specific linker instructions


Suggested change

/// executes a block of code before main by utilising platform specific linker instructions

/// Executes a block of code before main, by utilising platform specific linker instructions.

0x53A marked this pull request as draft October 23, 2024 01:11

0x53A marked this pull request as ready for review October 26, 2024 20:27

0x53A force-pushed the dev-multiple-impl-blocks branch from d5c73e6 to 6adbdc3 Compare October 26, 2024 21:19

0x53A changed the title ~~[WIP] Multiple #[godot_api] impl blocks~~ Multiple #[godot_api] impl blocks Oct 26, 2024

workflows/full-ci: allow manually triggering the workflow

43e214a

0x53A force-pushed the dev-multiple-impl-blocks branch from 2006f8a to a0df6ed Compare November 6, 2024 23:05

[macros] allow multiple inherent impl blocks. subsequent blocks must

2a5660f

have the key 'secondary'

0x53A force-pushed the dev-multiple-impl-blocks branch from a0df6ed to 2a5660f Compare November 6, 2024 23:17

Bromeon force-pushed the master branch from b75df8a to 4c7db48 Compare November 15, 2024 09:23

fpdotmonkey reviewed Nov 22, 2024

View reviewed changes

Bromeon added feature Adds functionality to the library c: register Register classes, functions and other symbols to GDScript labels Nov 23, 2024

Bromeon reviewed Nov 25, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multiple #[godot_api] impl blocks #927

Multiple #[godot_api] impl blocks #927

0x53A commented Oct 23, 2024 •

edited

Loading

0x53A commented Oct 23, 2024 •

edited

Loading

0x53A commented Oct 23, 2024

0x53A commented Oct 23, 2024

GodotRust commented Oct 23, 2024

Bromeon commented Oct 23, 2024

0x53A commented Oct 26, 2024 •

edited

Loading

Bromeon commented Nov 7, 2024

fpdotmonkey commented Nov 22, 2024

fpdotmonkey Nov 22, 2024 •

edited

Loading

0x53A Nov 23, 2024

fpdotmonkey Nov 22, 2024

0x53A Nov 23, 2024

Bromeon Nov 23, 2024

Bromeon left a comment •

edited

Loading

Bromeon Nov 25, 2024

Bromeon Nov 25, 2024

Bromeon Nov 25, 2024

Bromeon Nov 25, 2024

Bromeon Nov 25, 2024

Bromeon Nov 25, 2024

Bromeon Nov 25, 2024

Bromeon Nov 25, 2024

Bromeon Nov 25, 2024

Bromeon Nov 25, 2024

		assert!(result_as_string.is_ok());
		assert_eq!(expected_result, result_as_string.unwrap());

	assert!(result_as_string.is_ok());
	assert_eq!(expected_result, result_as_string.unwrap());
	assert_eq!(result_as_string, Ok(expected_result));

	/// # Multiple inherent impl blocks
	/// # Multiple inherent `impl` blocks

	/// executes a block of code before main by utilising platform specific linker instructions
	/// Executes a block of code before main, by utilising platform specific linker instructions.

Multiple #[godot_api] impl blocks #927

Are you sure you want to change the base?

Multiple #[godot_api] impl blocks #927

Conversation

0x53A commented Oct 23, 2024 • edited Loading

0x53A commented Oct 23, 2024 • edited Loading

0x53A commented Oct 23, 2024

0x53A commented Oct 23, 2024

GodotRust commented Oct 23, 2024

Bromeon commented Oct 23, 2024

0x53A commented Oct 26, 2024 • edited Loading

Bromeon commented Nov 7, 2024

fpdotmonkey commented Nov 22, 2024

fpdotmonkey Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bromeon left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0x53A commented Oct 23, 2024 •

edited

Loading

0x53A commented Oct 23, 2024 •

edited

Loading

0x53A commented Oct 26, 2024 •

edited

Loading

fpdotmonkey Nov 22, 2024 •

edited

Loading

Bromeon left a comment •

edited

Loading