Merge #681

681: Unwrangling the mess that is `godot_wrap_method`, adding generic method support as a side effect r=halzy a=toasteater

Due to repeated past additions to `godot_wrap_method_inner`, the implementation has become a total mess of unsafe macro code that is very hard to follow. Meanwhile, the manual interface for method registration has largely been neglected, lagging behind the rest of the crate, exposing `sys` types and unsafe FFI signatures. Recently, I've found myself in need to register a few methods manually and having to deal with this entire situation, so I've taken the chance to make some improvements to this part of the codebase, with all the hindsight that we now have. This has resulted in satisfactory results:

- All the unsafe FFI code previously in `godot_wrap_method_inner` is extracted to the library proper, and can no longer be touched by obscure import/inference problems that come from delayed type checking.
- Argument errors are now represented with a proper error type with a `Display` implementation. The library can report multiple argument errors at once. "Missing/excessive argument" errors are also more informative.
- It's possible to manually register one method for many classes that it may apply to, or many specializations of a generic method to one class by different names.
- Safe, straightforward support for stateful methods.

This is not in itself a breaking change, but it's expected that the older unsafe interface will be removed at the next possibility.

- - -

As a curiosity, the syntax for manually registering generic methods currently looks like this, and I find it already fairly usable even without further macro goodness:

```rust
#[derive(Copy, Clone, Default)]
struct Mixin<T> {
    _marker: PhantomData<T>,
}

#[derive(FromVarargs)]
struct Args<T> {
    a: T,
    b: T,
}

impl<T, C> StaticArgsMethod<C> for Mixin<T>
where
    T: Add<Output = T> + Send + Sync + ToVariant + FromVariant + 'static,
    C: NativeClass,
{
    type Args = Args<T>;
    fn call(&self, _this: RefInstance<'_, C, Shared>, Args { a, b }: Args<T>) -> Variant {
        (a + b).to_variant()
    }
}

fn register_methods(builder: &ClassBuilder<SomeClass>) {
    builder
        .build_method("add_i", StaticArgs::<Mixin::<i32>>::default())
        .done_stateless();
    builder
        .build_method("add_f", StaticArgs::<Mixin::<f32>>::default())
        .done_stateless();
    builder
        .build_method("add_v", StaticArgs::<Mixin::<Vector2>>::default())
        .done_stateless();
}
```

## Unresolved questions

~~As a side effect of moving the error handling to the library side, error sites all point to the library now. This should be fixable with something on the macro side that provides borrow-able site information, like how `profiling::Signature` is handled? This would mean moving the FFI call out of `godot_error` into some interface as well.~~ Resolved.

## Future development

If desired, it should be fairly easy to add a higher-level, derivable `Mixin` interface based on this. It should also be possible to add support for generic methods directly in `#[export]`.

Co-authored-by: toasteater <48371905+toasteater@users.noreply.github.com>

Author: bors[bot]

gdnative-core/src/lib.rs

@@ -41,6 +41,7 @@ mod init;
 #[cfg(feature = "nativescript")]
 pub mod nativescript;
 
+pub mod log;
 mod new_ref;
 pub mod object;
 pub mod ref_kind;

gdnative-core/src/log.rs

@@ -0,0 +1,97 @@
+//! Functions for using the engine's logging system in the editor.
+use std::ffi::CStr;
+use std::fmt::{self, Display};
+
+use crate::core_types::GodotString;
+use crate::private;
+
+/// Value representing a call site for errors and warnings. Can be constructed
+/// using the `godot_site!` macro, or manually.
+#[derive(Copy, Clone, Debug)]
+pub struct Site<'a> {
+    file: &'a CStr,
+    func: &'a CStr,
+    line: u32,
+}
+
+impl<'a> Site<'a> {
+    /// Construct a new `Site` value using values provided manually.
+    #[inline]
+    pub const fn new(file: &'a CStr, func: &'a CStr, line: u32) -> Self {
+        Site { file, func, line }
+    }
+}
+
+impl<'a> Default for Site<'a> {
+    #[inline]
+    fn default() -> Self {
+        let unset = unsafe { CStr::from_bytes_with_nul_unchecked(b"<unset>\0") };
+        Site::new(unset, unset, 0)
+    }
+}
+
+impl<'a> Display for Site<'a> {
+    #[inline]
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        write!(
+            f,
+            "file {}, {}, line {}",
+            self.file.to_string_lossy(),
+            self.func.to_string_lossy(),
+            self.line
+        )
+    }
+}
+
+/// Print a message to the Godot console.
+///
+/// # Panics
+///
+/// If the API isn't initialized.
+#[inline]
+pub fn print<S: Display>(msg: S) {
+    unsafe {
+        let msg = GodotString::from_str(&msg.to_string());
+        (private::get_api().godot_print)(&msg.to_sys() as *const _);
+    }
+}
+
+/// Print a warning to the Godot console.
+///
+/// # Panics
+///
+/// If the API isn't initialized, or if the message contains any NUL-bytes.
+#[inline]
+pub fn warn<S: Display>(site: Site<'_>, msg: S) {
+    let msg = msg.to_string();
+    let msg = ::std::ffi::CString::new(msg).unwrap();
+
+    unsafe {
+        (private::get_api().godot_print_warning)(
+            msg.as_ptr(),
+            site.func.as_ptr(),
+            site.file.as_ptr(),
+            site.line as libc::c_int,
+        );
+    }
+}
+
+/// Print an error to the Godot console.
+///
+/// # Panics
+///
+/// If the API isn't initialized, or if the message contains any NUL-bytes.
+#[inline]
+pub fn error<S: Display>(site: Site<'_>, msg: S) {
+    let msg = msg.to_string();
+    let msg = ::std::ffi::CString::new(msg).unwrap();
+
+    unsafe {
+        (private::get_api().godot_print_error)(
+            msg.as_ptr(),
+            site.func.as_ptr(),
+            site.file.as_ptr(),
+            site.line as libc::c_int,
+        );
+    }
+}

gdnative-core/src/macros.rs

@@ -102,13 +102,7 @@ macro_rules! godot_gdnative_terminate {
 #[macro_export]
 macro_rules! godot_print {
     ($($args:tt)*) => ({
-        let msg = format!($($args)*);
-
-        #[allow(unused_unsafe)]
-        unsafe {
-            let msg = $crate::core_types::GodotString::from_str(msg);
-            ($crate::private::get_api().godot_print)(&msg.to_sys() as *const _);
-        }
+        $crate::log::print(::std::format_args!($($args)*));
     });
 }
 
@@ -139,6 +133,51 @@ macro_rules! godot_dbg {
     };
 }
 
+/// Creates a `gdnative::log::Site` value from the current position in code,
+/// optionally with a function path for identification.
+///
+/// # Examples
+///
+/// ```ignore
+/// // WARN: <unset>: foo At: path/to/file.rs:123
+/// gdnative::log::warn(godot_site!(), "foo");
+/// // WARN: Foo::my_func: bar At: path/to/file.rs:123
+/// gdnative::log::error(godot_site!(Foo::my_func), "bar");
+/// ```
+#[macro_export]
+macro_rules! godot_site {
+    () => {{
+        // SAFETY: I guess we can assume that all sane file systems don't allow
+        // NUL-bytes in paths?
+        #[allow(unused_unsafe)]
+        let site: $crate::log::Site<'static> = unsafe {
+            let file = ::std::ffi::CStr::from_bytes_with_nul_unchecked(
+                ::std::concat!(::std::file!(), "\0").as_bytes(),
+            );
+            let func = ::std::ffi::CStr::from_bytes_with_nul_unchecked(b"<unset>\0");
+            $crate::log::Site::new(file, func, ::std::line!())
+        };
+
+        site
+    }};
+    ($($path:tt)+) => {{
+        // SAFETY: I guess we can assume that all sane file systems don't allow
+        // NUL-bytes in paths?
+        #[allow(unused_unsafe)]
+        let site: $crate::log::Site<'static> = unsafe {
+            let file = ::std::ffi::CStr::from_bytes_with_nul_unchecked(
+                ::std::concat!(::std::file!(), "\0").as_bytes(),
+            );
+            let func = ::std::ffi::CStr::from_bytes_with_nul_unchecked(
+                ::std::concat!(::std::stringify!($($path)+), "\0").as_bytes(),
+            );
+            $crate::log::Site::new(file, func, ::std::line!())
+        };
+
+        site
+    }};
+}
+
 /// Print a warning using the engine's logging system (visible in the editor).
 ///
 /// # Guarantees
@@ -150,22 +189,8 @@ macro_rules! godot_dbg {
 #[macro_export]
 macro_rules! godot_warn {
     ($($args:tt)*) => ({
-        let msg = format!($($args)*);
-        let line = line!();
-        let file = file!();
-        #[allow(unused_unsafe)]
-        unsafe {
-            let msg = ::std::ffi::CString::new(msg).unwrap();
-            let file = ::std::ffi::CString::new(file).unwrap();
-            let func = b"<native>\0";
-            ($crate::private::get_api().godot_print_warning)(
-                msg.as_ptr() as *const _,
-                func.as_ptr() as *const _,
-                file.as_ptr() as *const _,
-                line as _,
-            );
-        }
-    })
+        $crate::log::warn($crate::godot_site!(), ::std::format_args!($($args)*));
+    });
 }
 
 /// Print an error using the engine's logging system (visible in the editor).
@@ -179,22 +204,8 @@ macro_rules! godot_warn {
 #[macro_export]
 macro_rules! godot_error {
     ($($args:tt)*) => ({
-        let msg = format!($($args)*);
-        let line = line!();
-        let file = file!();
-        #[allow(unused_unsafe)]
-        unsafe {
-            let msg = ::std::ffi::CString::new(msg).unwrap();
-            let file = ::std::ffi::CString::new(file).unwrap();
-            let func = b"<native>\0";
-            ($crate::private::get_api().godot_print_error)(
-                msg.as_ptr() as *const _,
-                func.as_ptr() as *const _,
-                file.as_ptr() as *const _,
-                line as _,
-            );
-        }
-    })
+        $crate::log::error($crate::godot_site!(), ::std::format_args!($($args)*));
+    });
 }
 
 macro_rules! impl_basic_trait_as_sys {

gdnative-core/src/nativescript/init.rs

@@ -31,6 +31,9 @@
 //! For full examples, see [`examples`](https://github.com/godot-rust/godot-rust/tree/master/examples)
 //! in the godot-rust repository.
 
+// Temporary for unsafe method registration
+#![allow(deprecated)]
+
 use crate::*;
 
 use std::ffi::CString;
@@ -45,8 +48,12 @@ use crate::private::get_api;
 
 use super::emplace;
 
+pub mod method;
 pub mod property;
 
+pub use self::method::{
+    Method, MethodBuilder, RpcMode, ScriptMethod, ScriptMethodAttributes, ScriptMethodFn, Varargs,
+};
 pub use self::property::{Export, ExportInfo, PropertyBuilder, Usage as PropertyUsage};
 
 /// A handle that can register new classes to the engine during initialization.
@@ -212,37 +219,6 @@ impl InitHandle {
     }
 }
 
-pub type ScriptMethodFn = unsafe extern "C" fn(
-    *mut sys::godot_object,
-    *mut libc::c_void,
-    *mut libc::c_void,
-    libc::c_int,
-    *mut *mut sys::godot_variant,
-) -> sys::godot_variant;
-
-pub enum RpcMode {
-    Disabled,
-    Remote,
-    RemoteSync,
-    Master,
-    Puppet,
-    MasterSync,
-    PuppetSync,
-}
-
-pub struct ScriptMethodAttributes {
-    pub rpc_mode: RpcMode,
-}
-
-pub struct ScriptMethod<'l> {
-    pub name: &'l str,
-    pub method_ptr: Option<ScriptMethodFn>,
-    pub attributes: ScriptMethodAttributes,
-
-    pub method_data: *mut libc::c_void,
-    pub free_func: Option<unsafe extern "C" fn(*mut libc::c_void) -> ()>,
-}
-
 #[derive(Debug)]
 pub struct ClassBuilder<C> {
     init_handle: *mut libc::c_void,
@@ -252,6 +228,7 @@ pub struct ClassBuilder<C> {
 
 impl<C: NativeClass> ClassBuilder<C> {
     #[inline]
+    #[deprecated(note = "Unsafe registration is deprecated. Use `build_method` instead.")]
     pub fn add_method_advanced(&self, method: ScriptMethod) {
         let method_name = CString::new(method.name).unwrap();
 
@@ -285,6 +262,7 @@ impl<C: NativeClass> ClassBuilder<C> {
     }
 
     #[inline]
+    #[deprecated(note = "Unsafe registration is deprecated. Use `build_method` instead.")]
     pub fn add_method_with_rpc_mode(&self, name: &str, method: ScriptMethodFn, rpc_mode: RpcMode) {
         self.add_method_advanced(ScriptMethod {
             name,
@@ -296,10 +274,35 @@ impl<C: NativeClass> ClassBuilder<C> {
     }
 
     #[inline]
+    #[deprecated(note = "Unsafe registration is deprecated. Use `build_method` instead.")]
     pub fn add_method(&self, name: &str, method: ScriptMethodFn) {
         self.add_method_with_rpc_mode(name, method, RpcMode::Disabled);
     }
 
+    /// Returns a `MethodBuilder` which can be used to add a method to the class being
+    /// registered.
+    ///
+    /// # Examples
+    ///
+    /// Basic usage:
+    ///
+    /// ```ignore
+    /// // `Bar` is a stateful method implementing `Method<C>`
+    ///
+    /// builder
+    ///     .build_method("foo", Bar { baz: 42 })
+    ///     .with_rpc_mode(RpcMode::RemoteSync)
+    ///     .done();
+    /// ```
+    #[inline]
+    pub fn build_method<'a, F: Method<C>>(
+        &'a self,
+        name: &'a str,
+        method: F,
+    ) -> MethodBuilder<'a, C, F> {
+        MethodBuilder::new(self, name, method)
+    }
+
     /// Returns a `PropertyBuilder` which can be used to add a property to the class being
     /// registered.
     ///