Merge #249

249: Fix methods always taking a mutable receiver r=Bromeon a=lilizoey

A lot of methods unnecessarily require a `&mut self` reference, but at least now methods actually request the kind of reference they're declared to take. 

closes #206 

Co-authored-by: Lili Zoey <[email protected]>

Author: bors[bot]

godot-core/src/builtin/meta/signature.rs

@@ -18,6 +18,15 @@ pub trait SignatureTuple {
     fn param_metadata(index: i32) -> sys::GDExtensionClassMethodArgumentMetadata;
 
     unsafe fn varcall<C: GodotClass>(
+        instance_ptr: sys::GDExtensionClassInstancePtr,
+        args_ptr: *const sys::GDExtensionConstVariantPtr,
+        ret: sys::GDExtensionVariantPtr,
+        err: *mut sys::GDExtensionCallError,
+        func: fn(&C, Self::Params) -> Self::Ret,
+        method_name: &str,
+    );
+
+    unsafe fn varcall_mut<C: GodotClass>(
         instance_ptr: sys::GDExtensionClassInstancePtr,
         args_ptr: *const sys::GDExtensionConstVariantPtr,
         ret: sys::GDExtensionVariantPtr,
@@ -29,6 +38,17 @@ pub trait SignatureTuple {
     // Note: this method imposes extra bounds on GodotFfi, which may not be implemented for user types.
     // We could fall back to varcalls in such cases, and not require GodotFfi categorically.
     unsafe fn ptrcall<C: GodotClass>(
+        instance_ptr: sys::GDExtensionClassInstancePtr,
+        args_ptr: *const sys::GDExtensionConstTypePtr,
+        ret: sys::GDExtensionTypePtr,
+        func: fn(&C, Self::Params) -> Self::Ret,
+        method_name: &str,
+        call_type: sys::PtrcallType,
+    );
+
+    // Note: this method imposes extra bounds on GodotFfi, which may not be implemented for user types.
+    // We could fall back to varcalls in such cases, and not require GodotFfi categorically.
+    unsafe fn ptrcall_mut<C: GodotClass>(
         instance_ptr: sys::GDExtensionClassInstancePtr,
         args_ptr: *const sys::GDExtensionConstTypePtr,
         ret: sys::GDExtensionTypePtr,
@@ -111,6 +131,38 @@ macro_rules! impl_signature_for_tuple {
                 args_ptr: *const sys::GDExtensionConstVariantPtr,
                 ret: sys::GDExtensionVariantPtr,
                 err: *mut sys::GDExtensionCallError,
+                func: fn(&C, Self::Params) -> Self::Ret,
+                method_name: &str,
+            ) {
+    	        $crate::out!("varcall: {}", method_name);
+
+                let storage = unsafe { crate::private::as_storage::<C>(instance_ptr) };
+                let instance = storage.get();
+
+                let args = ( $(
+                    {
+                        let variant = unsafe { &*(*args_ptr.offset($n) as *mut Variant) }; // TODO from_var_sys
+                        let arg = <$Pn as FromVariant>::try_from_variant(variant)
+                            .unwrap_or_else(|e| param_error::<$Pn>(method_name, $n, variant));
+
+                        arg
+                    },
+                )* );
+
+				let ret_val = func(&*instance, args);
+                let ret_variant = <$R as ToVariant>::to_variant(&ret_val); // TODO write_sys
+				unsafe {
+                    *(ret as *mut Variant) = ret_variant;
+                    (*err).error = sys::GDEXTENSION_CALL_OK;
+                }
+            }
+
+            #[inline]
+            unsafe fn varcall_mut<C : GodotClass>(
+				instance_ptr: sys::GDExtensionClassInstancePtr,
+                args_ptr: *const sys::GDExtensionConstVariantPtr,
+                ret: sys::GDExtensionVariantPtr,
+                err: *mut sys::GDExtensionCallError,
                 func: fn(&mut C, Self::Params) -> Self::Ret,
                 method_name: &str,
             ) {
@@ -142,6 +194,38 @@ macro_rules! impl_signature_for_tuple {
 				instance_ptr: sys::GDExtensionClassInstancePtr,
                 args_ptr: *const sys::GDExtensionConstTypePtr,
                 ret: sys::GDExtensionTypePtr,
+                func: fn(&C, Self::Params) -> Self::Ret,
+                method_name: &str,
+                call_type: sys::PtrcallType,
+            ) {
+                $crate::out!("ptrcall: {}", method_name);
+
+                let storage = unsafe { crate::private::as_storage::<C>(instance_ptr) };
+                let instance = storage.get();
+
+				let args = ( $(
+                    unsafe {
+                        <$Pn as sys::GodotFuncMarshal>::try_from_arg(
+                            sys::force_mut_ptr(*args_ptr.offset($n)),
+                            call_type
+                        )
+                    }
+                        .unwrap_or_else(|e| param_error::<$Pn>(method_name, $n, &e)),
+                )* );
+
+                let ret_val = func(&*instance, args);
+                // SAFETY:
+                // `ret` is always a pointer to an initialized value of type $R
+                // TODO: double-check the above
+				<$R as sys::GodotFuncMarshal>::try_return(ret_val, ret, call_type)
+                    .unwrap_or_else(|ret_val| return_error::<$R>(method_name, &ret_val));
+            }
+
+            #[inline]
+            unsafe fn ptrcall_mut<C : GodotClass>(
+				instance_ptr: sys::GDExtensionClassInstancePtr,
+                args_ptr: *const sys::GDExtensionConstTypePtr,
+                ret: sys::GDExtensionTypePtr,
                 func: fn(&mut C, Self::Params) -> Self::Ret,
                 method_name: &str,
                 call_type: sys::PtrcallType,

godot-core/src/macros.rs

@@ -41,6 +41,8 @@ macro_rules! gdext_is_not_unit {
 #[macro_export]
 macro_rules! gdext_register_method_inner {
     (
+        $ptrcall:ident,
+        $varcall:ident,
         $Class:ty,
         $map_method:ident,
         fn $method_name:ident(
@@ -69,7 +71,7 @@ macro_rules! gdext_register_method_inner {
                     let success = $crate::private::handle_panic(
                         || stringify!($method_name),
                         || {
-                            <Sig as SignatureTuple>::varcall::< $Class >(
+                            <Sig as SignatureTuple>::$varcall::< $Class >(
                                 instance_ptr,
                                 args,
                                 ret,
@@ -103,7 +105,7 @@ macro_rules! gdext_register_method_inner {
                     let success = $crate::private::handle_panic(
                         || stringify!($method_name),
                         || {
-                            <Sig as SignatureTuple>::ptrcall::< $Class >(
+                            <Sig as SignatureTuple>::$ptrcall::< $Class >(
                                 instance_ptr,
                                 args,
                                 ret,
@@ -203,6 +205,8 @@ macro_rules! gdext_register_method {
         ) -> $RetTy:ty
     ) => {
         $crate::gdext_register_method_inner!(
+            ptrcall_mut,
+            varcall_mut,
             $Class,
             map_mut,
             fn $method_name(
@@ -223,6 +227,8 @@ macro_rules! gdext_register_method {
         ) -> $RetTy:ty
     ) => {
         $crate::gdext_register_method_inner!(
+            ptrcall,
+            varcall,
             $Class,
             map,
             fn $method_name(
@@ -279,6 +285,7 @@ macro_rules! gdext_register_method {
 #[macro_export]
 macro_rules! gdext_virtual_method_callback_inner {
     (
+        $ptrcall:ident,
         $Class:ty,
         $map_method:ident,
         fn $method_name:ident(
@@ -294,6 +301,7 @@ macro_rules! gdext_virtual_method_callback_inner {
                 ret: sys::GDExtensionTypePtr,
             ) {
                 $crate::gdext_ptrcall!(
+                    $ptrcall;
                     instance_ptr, args, ret;
                     $Class;
                     fn $method_name( $( $arg : $Param, )* ) -> $Ret
@@ -320,6 +328,7 @@ macro_rules! gdext_virtual_method_callback {
         ) -> $RetTy:ty
     ) => {
         $crate::gdext_virtual_method_callback_inner!(
+            ptrcall_mut,
             $Class,
             map_mut,
             fn $method_name(
@@ -338,6 +347,7 @@ macro_rules! gdext_virtual_method_callback {
         ) -> $RetTy:ty
     ) => {
         $crate::gdext_virtual_method_callback_inner!(
+            ptrcall,
             $Class,
             map,
             fn $method_name(
@@ -388,14 +398,15 @@ macro_rules! gdext_virtual_method_callback {
 #[macro_export]
 macro_rules! gdext_ptrcall {
     (
+        $ptrcall:ident;
         $instance_ptr:ident, $args_ptr:ident, $ret_ptr:ident;
         $Class:ty;
         fn $method_name:ident(
             $( $arg:ident : $ParamTy:ty, )*
         ) -> $( $RetTy:tt )+
     ) => {
         use $crate::builtin::meta::SignatureTuple;
-        <($($RetTy)+, $($ParamTy,)*) as SignatureTuple>::ptrcall::<$Class>(
+        <($($RetTy)+, $($ParamTy,)*) as SignatureTuple>::$ptrcall::<$Class>(
             $instance_ptr,
             $args_ptr,
             $ret_ptr,

itest/rust/src/object_test.rs

@@ -4,7 +4,7 @@
  * file, You can obtain one at https://mozilla.org/MPL/2.0/.
  */
 
-use std::cell::RefCell;
+use std::cell::{Cell, RefCell};
 use std::mem;
 use std::rc::Rc;
 
@@ -762,3 +762,67 @@ pub mod object_test_gd {
         }
     }
 }
+
+// ----------------------------------------------------------------------------------------------------------------------------------------------
+
+#[derive(GodotClass)]
+#[class(init, base=Object)]
+struct DoubleUse {
+    #[base]
+    base: Base<Object>,
+
+    used: Cell<bool>,
+}
+
+#[godot_api]
+impl DoubleUse {
+    #[func]
+    fn use_1(&self) {
+        self.used.set(true);
+    }
+}
+
+#[derive(GodotClass)]
+#[class(init, base=Object)]
+struct SignalEmitter {
+    #[base]
+    base: Base<Object>,
+}
+
+#[godot_api]
+impl SignalEmitter {
+    #[signal]
+    fn do_use();
+}
+
+#[itest]
+/// Test that godot can call a method that takes `&self`, while there already exists an immutable reference
+/// to that type acquired through `bind`.
+///
+/// This test is not signal-specific, the original bug would happen whenever godot would call a method that
+/// takes `&self`. However this was the easiest way to test the bug i could find.
+fn double_use_reference() {
+    let double_use: Gd<DoubleUse> = Gd::new_default();
+    let emitter: Gd<SignalEmitter> = Gd::new_default();
+
+    emitter
+        .share()
+        .upcast::<Object>()
+        .connect("do_use".into(), double_use.callable("use_1"), 0);
+
+    let guard = double_use.bind();
+
+    assert!(!guard.used.get());
+
+    emitter
+        .share()
+        .upcast::<Object>()
+        .emit_signal("do_use".into(), &[]);
+
+    assert!(guard.used.get(), "use_1 was not called");
+
+    std::mem::drop(guard);
+
+    double_use.free();
+    emitter.free();
+}