Merge pull request #1029 from arocull/static-callable-test

`Callable::from_local_static()` now requires Godot 4.4+

Author: Bromeon

godot-core/src/builtin/callable.rs

@@ -68,54 +68,29 @@ impl Callable {
     ///
     /// Allows you to call static functions through `Callable`.
     ///
-    /// Note that due to varying support across different engine versions, the resulting `Callable` has unspecified behavior for
-    /// methods such as [`method_name()`][Self::method_name], [`object()`][Self::object], [`object_id()`][Self::object_id] or
-    /// [`get_argument_count()`][Self::arg_len] among others. It is recommended to only use this for calling the function.
-    ///
     /// # Compatibility
-    /// Up until and including Godot 4.3, this method has some limitations:
-    /// - [`is_valid()`][Self::is_valid] will return `false`, even though the call itself succeeds.
-    /// - You cannot use statics to connect signals to such callables. Use the new typed signal API instead.
+    /// Not available before Godot 4.4. Library versions <0.3 used to provide this, however the polyfill used to emulate it was half-broken
+    /// (not supporting signals, bind(), method_name(), is_valid(), etc).
+    #[cfg(since_api = "4.4")]
     pub fn from_local_static(
         class_name: impl meta::AsArg<StringName>,
         function_name: impl meta::AsArg<StringName>,
     ) -> Self {
         meta::arg_into_owned!(class_name);
         meta::arg_into_owned!(function_name);
 
-        // Modern implementation: use ClassDb::class_call_static().
-        #[cfg(since_api = "4.4")]
-        {
-            let callable_name = format!("{class_name}.{function_name}");
-
-            Self::from_local_fn(&callable_name, move |args| {
-                let args = args.iter().cloned().cloned().collect::<Vec<_>>();
-
-                let result: Variant = classes::ClassDb::singleton().class_call_static(
-                    &class_name,
-                    &function_name,
-                    args.as_slice(),
-                );
-                Ok(result)
-            })
-        }
+        let callable_name = format!("{class_name}.{function_name}");
 
-        // Polyfill for <= Godot 4.3: use GDScript expressions.
-        #[cfg(before_api = "4.4")]
-        {
-            use crate::obj::NewGd;
+        Self::from_local_fn(&callable_name, move |args| {
+            let args = args.iter().cloned().cloned().collect::<Vec<_>>();
 
-            let code = format!(
-                "static func __callable():\n\treturn Callable({class_name}, \"{function_name}\")"
+            let result: Variant = classes::ClassDb::singleton().class_call_static(
+                &class_name,
+                &function_name,
+                args.as_slice(),
             );
-
-            let mut script = classes::GDScript::new_gd();
-            script.set_source_code(&code);
-            script.reload();
-
-            let callable = script.call("__callable", &[]);
-            callable.to()
-        }
+            Ok(result)
+        })
     }
 
     #[cfg(since_api = "4.2")]
@@ -125,7 +100,7 @@ impl Callable {
             token: ptr::null_mut(),
             object_id: 0,
             call_func: None,
-            is_valid_func: None, // could be customized, but no real use case yet.
+            is_valid_func: None, // overwritten later.
             free_func: None,
             hash_func: None,
             equal_func: None,
@@ -219,12 +194,14 @@ impl Callable {
         let userdata = CallableUserdata { inner: callable };
 
         let info = CallableCustomInfo {
+            // We could technically associate an object_id with the custom callable. is_valid_func would then check that for validity.
             callable_userdata: Box::into_raw(Box::new(userdata)) as *mut std::ffi::c_void,
             call_func: Some(rust_callable_call_custom::<C>),
             free_func: Some(rust_callable_destroy::<C>),
             hash_func: Some(rust_callable_hash::<C>),
             equal_func: Some(rust_callable_equal::<C>),
             to_string_func: Some(rust_callable_to_string_display::<C>),
+            is_valid_func: Some(rust_callable_is_valid_custom::<C>),
             ..Self::default_callable_custom_info()
         };
 
@@ -243,6 +220,7 @@ impl Callable {
             call_func: Some(rust_callable_call_fn::<F>),
             free_func: Some(rust_callable_destroy::<FnWrapper<F>>),
             to_string_func: Some(rust_callable_to_string_named::<F>),
+            is_valid_func: Some(rust_callable_is_valid),
             ..Self::default_callable_custom_info()
         };
 
@@ -530,6 +508,17 @@ mod custom_callable {
         /// Error handling is mostly needed in case argument number or types mismatch.
         #[allow(clippy::result_unit_err)] // TODO remove once there's a clear error type here.
         fn invoke(&mut self, args: &[&Variant]) -> Result<Variant, ()>;
+
+        // TODO(v0.3): add object_id().
+
+        /// Returns whether the callable is considered valid.
+        ///
+        /// True by default.
+        ///
+        /// If this Callable stores an object, this method should return whether that object is alive.
+        fn is_valid(&self) -> bool {
+            true
+        }
     }
 
     pub unsafe extern "C" fn rust_callable_call_custom<C: RustCallable>(
@@ -641,4 +630,23 @@ mod custom_callable {
         w.name.clone().move_into_string_ptr(r_out);
         *r_is_valid = sys::conv::SYS_TRUE;
     }
+
+    // Implementing this is necessary because the default (nullptr) may consider custom callables as invalid in some cases.
+    pub unsafe extern "C" fn rust_callable_is_valid_custom<C: RustCallable>(
+        callable_userdata: *mut std::ffi::c_void,
+    ) -> sys::GDExtensionBool {
+        let w: &mut C = CallableUserdata::inner_from_raw(callable_userdata);
+        let valid = w.is_valid();
+
+        sys::conv::bool_to_sys(valid)
+    }
+
+    // Implementing this is necessary because the default (nullptr) may consider custom callables as invalid in some cases.
+    pub unsafe extern "C" fn rust_callable_is_valid(
+        _callable_userdata: *mut std::ffi::c_void,
+    ) -> sys::GDExtensionBool {
+        // If we had an object (CallableCustomInfo::object_id field), we could check whether that object is alive.
+        // But since we just take a Rust function/closure, not knowing what happens inside, we assume always valid.
+        sys::conv::SYS_TRUE
+    }
 }

godot-core/src/builtin/variant/mod.rs

@@ -8,7 +8,7 @@
 use crate::builtin::{
     GString, StringName, VariantArray, VariantDispatch, VariantOperator, VariantType,
 };
-use crate::meta::error::{ConvertError, ErrorKind, FromVariantError};
+use crate::meta::error::ConvertError;
 use crate::meta::{arg_into_ref, ArrayElement, AsArg, FromGodot, ToGodot};
 use godot_ffi as sys;
 use std::{fmt, ptr};
@@ -128,6 +128,7 @@ impl Variant {
 
         #[cfg(before_api = "4.4")]
         {
+            use crate::meta::error::{ErrorKind, FromVariantError};
             match self.try_to::<crate::obj::Gd<crate::classes::Object>>() {
                 Ok(obj) => Some(obj.instance_id_unchecked()),
                 Err(c)

godot-core/src/meta/error/convert_error.rs

@@ -92,6 +92,7 @@ impl ConvertError {
         ErasedConvertError::from(self)
     }
 
+    #[cfg(before_api = "4.4")]
     pub(crate) fn kind(&self) -> &ErrorKind {
         &self.kind
     }

itest/rust/src/builtin_tests/containers/callable_test.rs

@@ -98,27 +98,15 @@ fn callable_object_method() {
 }
 
 #[itest]
+#[cfg(since_api = "4.4")]
 fn callable_static() {
     let callable = Callable::from_local_static("CallableTestObj", "concat_array");
 
-    // Test current behavior in <4.4 and >=4.4. Although our API explicitly leaves it unspecified, we then notice change in implementation.
-    if cfg!(since_api = "4.4") {
-        assert_eq!(callable.object(), None);
-        assert_eq!(callable.object_id(), None);
-        assert_eq!(callable.method_name(), None);
-        assert!(callable.is_custom());
-        assert!(callable.is_valid());
-    } else {
-        assert!(callable.object().is_some());
-        assert!(callable.object_id().is_some());
-        assert_eq!(callable.method_name(), Some("concat_array".into()));
-        assert_eq!(callable.to_string(), "GDScriptNativeClass::concat_array");
-        assert!(!callable.is_custom());
-
-        // Surprisingly false, but call still works (see test below).
-        // What DOESN'T work is connecting 4.3 static methods to signals via this approach.
-        assert!(!callable.is_valid());
-    }
+    assert_eq!(callable.object(), None);
+    assert_eq!(callable.object_id(), None);
+    assert_eq!(callable.method_name(), None);
+    assert!(callable.is_custom());
+    assert!(callable.is_valid());
 
     assert!(!callable.is_null());
 
@@ -138,6 +126,32 @@ fn callable_static() {
     assert_eq!(callable.get_argument_count(), 0); // Consistently doesn't work :)
 }
 
+// Regression test, see https://github.com/godot-rust/gdext/pull/1029.
+#[itest]
+#[cfg(since_api = "4.4")]
+fn callable_static_bind() {
+    let callable = Callable::from_local_static("CallableTestObj", "concat_array");
+    assert!(callable.is_valid());
+
+    // Test varying binds to static callables.
+    // Last 3 of 4 arguments. Within Godot, bound arguments are used in-order AFTER call arguments.
+    let bindv = callable.bindv(&varray![
+        "two",
+        array![&NodePath::from("three/four")],
+        &RefCounted::new_gd(),
+    ]);
+    assert!(bindv.is_valid());
+
+    assert!(!bindv.to_variant().is_nil());
+    let args = varray![1];
+    let bindv_result = bindv.callv(&args);
+
+    assert!(!bindv_result.is_nil());
+
+    let bind_result_data: VariantArray = bindv_result.to();
+    assert_eq!(4, bind_result_data.len());
+}
+
 #[itest]
 fn callable_callv() {
     let obj = CallableTestObj::new_gd();