Allow Deref/DerefMut for all Gd<T> instead of base/base_mut

More uniform and should be safe.

Author: Bromeon

examples/dodge-the-creeps/rust/src/main_scene.rs

@@ -100,13 +100,12 @@ impl Main {
             rng.gen_range(mob.min_speed..mob.max_speed)
         };
 
-        let mut mob = mob.base_mut();
         mob.set_linear_velocity(Vector2::new(range, 0.0));
         let lin_vel = mob.get_linear_velocity().rotated(real::from_f32(direction));
         mob.set_linear_velocity(lin_vel);
 
         let mut hud = self.base.get_node_as::<Hud>("Hud");
-        hud.base_mut().connect(
+        hud.connect(
             "start_game".into(),
             Callable::from_object_method(mob, "on_start_game"),
         );

godot-core/src/obj/gd.rs

@@ -160,36 +160,6 @@ where
         GdMut::from_cell(self.storage().get_mut())
     }
 
-    /// Access base without locking the user object.
-    ///
-    /// If you need mutations, use [`base_mut()`][Self::base_mut]. If you need a value copy, use [`base().share()`][Share::share].
-    pub fn base(&self) -> &Gd<T::Base> {
-        self.storage().base().deref()
-    }
-
-    /// Access base mutably without locking the user object.
-    ///
-    /// The idea is to support the `gd.base_mut().mutating_method()` pattern, which is not allowed with `gd.base()`.
-    /// If you need a copy of a `Gd` object, use `base().share()`.
-    ///
-    /// The return type is currently `Gd` and not `&mut Gd` because of `&mut` aliasing restrictions. This may change in the future.
-    pub fn base_mut(&mut self) -> Gd<T::Base> {
-        // Note: we cannot give safe mutable access to a base without RefCell, because multiple Gd pointers could call base_mut(),
-        // leading to aliased &mut. And we don't want RefCell, as C++ objects (nodes etc.) don't underly Rust's exclusive-ref limitations.
-        // The whole point of the Gd::base*() methods are to not require runtime-exclusive access to the Rust object.
-        //
-        // This is not a problem when accessing the `#[base]` field of a user struct directly, because `self` is guarded by the
-        // RefCell/RwLock in the InstanceStorage.
-        //
-        // Here, we instead return a copy (for now), which for the user looks mostly the same. The idea is that:
-        // - gd.base().mutating_method() fails
-        // - gd.base_mut().mutating_method() works.
-        //
-        // Downside: small ref-counting overhead for `RefCounted` types. If this is an issue in a real game (highly unlikely),
-        // the user could always cache Gd base pointers.
-        self.storage().base().share()
-    }
-
     /// Storage object associated with the extension instance.
     pub(crate) fn storage(&self) -> &InstanceStorage<T> {
         // SAFETY: instance pointer belongs to this instance. We only get a shared reference, no exclusive access, so even
@@ -541,16 +511,16 @@ where
     }
 }
 
-impl<T> Deref for Gd<T>
-where
-    T: GodotClass<Declarer = dom::EngineDomain>,
-{
-    type Target = T;
+impl<T: GodotClass> Deref for Gd<T> {
+    // Target is always an engine class:
+    // * if T is an engine class => T
+    // * if T is a user class => T::Base
+    type Target = <<T as GodotClass>::Declarer as dom::Domain>::DerefTarget<T>;
 
     fn deref(&self) -> &Self::Target {
         // SAFETY:
         //
-        // This relies on `Gd<Node3D>` having the layout as `Node3D` (as an example),
+        // This relies on `Gd<Node3D>.opaque` having the layout as `Node3D` (as an example),
         // which also needs #[repr(transparent)]:
         //
         // struct Gd<T: GodotClass> {
@@ -560,22 +530,21 @@ where
         // struct Node3D {
         //     object_ptr: sys::GDExtensionObjectPtr,
         // }
-        unsafe { std::mem::transmute::<&OpaqueObject, &T>(&self.opaque) }
+        unsafe { std::mem::transmute::<&OpaqueObject, &Self::Target>(&self.opaque) }
     }
 }
 
-impl<T> DerefMut for Gd<T>
-where
-    T: GodotClass<Declarer = dom::EngineDomain>,
-{
-    fn deref_mut(&mut self) -> &mut T {
+impl<T: GodotClass> DerefMut for Gd<T> {
+    fn deref_mut(&mut self) -> &mut Self::Target {
         // SAFETY: see also Deref
         //
         // The resulting `&mut T` is transmuted from `&mut OpaqueObject`, i.e. a *pointer* to the `opaque` field.
         // `opaque` itself has a different *address* for each Gd instance, meaning that two simultaneous
         // DerefMut borrows on two Gd instances will not alias, *even if* the underlying Godot object is the
         // same (i.e. `opaque` has the same value, but not address).
-        unsafe { std::mem::transmute::<&mut OpaqueObject, &mut T>(&mut self.opaque) }
+        //
+        // The `&mut self` guarantees that no other base access can take place for *the same Gd instance* (access to other Gds is OK).
+        unsafe { std::mem::transmute::<&mut OpaqueObject, &mut Self::Target>(&mut self.opaque) }
     }
 }
 

godot-core/src/obj/traits.rs

@@ -260,6 +260,8 @@ pub mod dom {
 
     /// Trait that specifies who declares a given `GodotClass`.
     pub trait Domain: Sealed {
+        type DerefTarget<T: GodotClass>;
+
         #[doc(hidden)]
         fn scoped_mut<T, F, R>(obj: &mut Gd<T>, closure: F) -> R
         where
@@ -271,6 +273,8 @@ pub mod dom {
     pub enum EngineDomain {}
     impl Sealed for EngineDomain {}
     impl Domain for EngineDomain {
+        type DerefTarget<T: GodotClass> = T;
+
         fn scoped_mut<T, F, R>(obj: &mut Gd<T>, closure: F) -> R
         where
             T: GodotClass<Declarer = EngineDomain>,
@@ -284,6 +288,8 @@ pub mod dom {
     pub enum UserDomain {}
     impl Sealed for UserDomain {}
     impl Domain for UserDomain {
+        type DerefTarget<T: GodotClass> = T::Base;
+
         fn scoped_mut<T, F, R>(obj: &mut Gd<T>, closure: F) -> R
         where
             T: GodotClass<Declarer = Self>,

godot-core/src/storage.rs

@@ -12,16 +12,17 @@ use std::any::type_name;
 
 #[derive(Copy, Clone, Debug)]
 pub enum Lifecycle {
+    // Warning: when reordering/changing enumerators, update match in AtomicLifecycle below
     Alive,
     Destroying,
     Dead, // reading this would typically already be too late, only best-effort in case of UB
 }
 
 #[cfg(not(feature = "threads"))]
-pub use single_thread::*;
+pub(crate) use single_thread::*;
 
 #[cfg(feature = "threads")]
-pub use multi_thread::*;
+pub(crate) use multi_thread::*;
 
 #[cfg(not(feature = "threads"))]
 mod single_thread {
@@ -115,21 +116,46 @@ mod single_thread {
 #[cfg(feature = "threads")]
 mod multi_thread {
     use std::any::type_name;
+    use std::sync;
     use std::sync::atomic::{AtomicU32, Ordering};
-    use std::{cell, sync};
 
     use crate::obj::{Base, GodotClass};
-    use crate::{out, sys};
+    use crate::out;
 
     use super::Lifecycle;
 
+    pub struct AtomicLifecycle {
+        atomic: AtomicU32,
+    }
+
+    impl AtomicLifecycle {
+        pub fn new(value: Lifecycle) -> Self {
+            Self {
+                atomic: AtomicU32::new(value as u32),
+            }
+        }
+
+        pub fn get(&self) -> Lifecycle {
+            match self.atomic.load(Ordering::Relaxed) {
+                0 => Lifecycle::Alive,
+                1 => Lifecycle::Dead,
+                2 => Lifecycle::Destroying,
+                other => panic!("Invalid lifecycle {other}"),
+            }
+        }
+
+        pub fn set(&self, value: Lifecycle) {
+            self.atomic.store(value as u32, Ordering::Relaxed);
+        }
+    }
+
     /// Manages storage and lifecycle of user's extension class instances.
     pub struct InstanceStorage<T: GodotClass> {
         user_instance: sync::RwLock<T>,
         cached_base: Base<T::Base>,
 
         // Declared after `user_instance`, is dropped last
-        pub lifecycle: cell::Cell<Lifecycle>,
+        pub lifecycle: AtomicLifecycle,
         godot_ref_count: AtomicU32,
     }
 
@@ -141,12 +167,12 @@ mod multi_thread {
             Self {
                 user_instance: sync::RwLock::new(user_instance),
                 cached_base,
-                lifecycle: cell::Cell::new(Lifecycle::Alive),
+                lifecycle: AtomicLifecycle::new(Lifecycle::Alive),
                 godot_ref_count: AtomicU32::new(1),
             }
         }
 
-        pub(crate) fn on_inc_ref(&mut self) {
+        pub(crate) fn on_inc_ref(&self) {
             self.godot_ref_count.fetch_add(1, Ordering::Relaxed);
             out!(
                 "    Storage::on_inc_ref (rc={})     <{}>", // -- {:?}",
@@ -156,7 +182,7 @@ mod multi_thread {
             );
         }
 
-        pub(crate) fn on_dec_ref(&mut self) {
+        pub(crate) fn on_dec_ref(&self) {
             self.godot_ref_count.fetch_sub(1, Ordering::Relaxed);
             out!(
                 "  | Storage::on_dec_ref (rc={})     <{}>", // -- {:?}",
@@ -195,7 +221,15 @@ mod multi_thread {
         pub(super) fn godot_ref_count(&self) -> u32 {
             self.godot_ref_count.load(Ordering::Relaxed)
         }
+
+        // TODO enable once there is better thread support
+        //fn __static_type_check() {
+        //    enforce_sync::<InstanceStorage<T>>();
+        //}
     }
+
+    // Make sure storage is Sync in multi-threaded case, as it can be concurrently accessed through aliased Gd<T> pointers.
+    //fn enforce_sync<T: Sync>() {}
 }
 
 impl<T: GodotClass> InstanceStorage<T> {
@@ -267,6 +301,9 @@ pub unsafe fn destroy_storage<T: GodotClass>(instance_ptr: sys::GDExtensionClass
     let _drop = Box::from_raw(instance_ptr as *mut InstanceStorage<T>);
 }
 
+// ----------------------------------------------------------------------------------------------------------------------------------------------
+// Callbacks
+
 pub fn nop_instance_callbacks() -> sys::GDExtensionInstanceBindingCallbacks {
     // These could also be null pointers, if they are definitely not invoked (e.g. create_callback only passed to object_get_instance_binding(),
     // when there is already a binding). Current "empty but not null" impl corresponds to godot-cpp (wrapped.hpp).

itest/rust/src/base_test.rs

@@ -27,12 +27,9 @@ fn base_instance_id() {
 fn base_access_unbound() {
     let mut obj = Gd::<Based>::new_default();
 
-    {
-        let pos = Vector2::new(-5.5, 7.0);
-        obj.base_mut().set_position(pos);
-
-        assert_eq!(obj.base().get_position(), pos);
-    }
+    let pos = Vector2::new(-5.5, 7.0);
+    obj.set_position(pos);
+    assert_eq!(obj.get_position(), pos);
 
     obj.free();
 }
@@ -42,12 +39,9 @@ fn base_access_unbound() {
 fn base_access_unbound_no_field() {
     let mut obj = Gd::<Baseless>::new_default();
 
-    {
-        let pos = Vector2::new(-5.5, 7.0);
-        obj.base_mut().set_position(pos);
-
-        assert_eq!(obj.base().get_position(), pos);
-    }
+    let pos = Vector2::new(-5.5, 7.0);
+    obj.set_position(pos);
+    assert_eq!(obj.get_position(), pos);
 
     obj.free();
 }

itest/rust/src/object_test.rs

@@ -763,9 +763,6 @@ pub mod object_test_gd {
     #[derive(GodotClass)]
     #[class(base=Object)]
     pub struct CustomConstructor {
-        #[base]
-        base: Base<Object>,
-
         #[var]
         pub val: i64,
     }
@@ -774,7 +771,7 @@ pub mod object_test_gd {
     impl CustomConstructor {
         #[func]
         pub fn construct_object(val: i64) -> Gd<CustomConstructor> {
-            Gd::with_base(|base| Self { base, val })
+            Gd::with_base(|_base| Self { val })
         }
     }
 }
@@ -804,10 +801,7 @@ impl DoubleUse {
 
 #[derive(GodotClass)]
 #[class(init, base=Object)]
-struct SignalEmitter {
-    #[base]
-    base: Base<Object>,
-}
+struct SignalEmitter {}
 
 #[godot_api]
 impl SignalEmitter {

itest/rust/src/property_test.rs

@@ -15,28 +15,33 @@ use godot::{
 #[derive(GodotClass)]
 #[class(base=Node)]
 struct HasProperty {
-    #[base]
-    base: Base<Node>,
-
     #[var]
     int_val: i32,
+
     #[var(get = get_int_val_read)]
     int_val_read: i32,
+
     #[var(set = set_int_val_write)]
     int_val_write: i32,
+
     #[var(get = get_int_val_rw, set = set_int_val_rw)]
     int_val_rw: i32,
+
     #[var(get = get_int_val_getter, set)]
     int_val_getter: i32,
+
     #[var(get, set = set_int_val_setter)]
     int_val_setter: i32,
 
     #[var(get = get_string_val, set = set_string_val)]
     string_val: GodotString,
+
     #[var(get = get_object_val, set = set_object_val)]
     object_val: Option<Gd<Object>>,
+
     #[var]
     texture_val: Gd<Texture>,
+
     #[var(get = get_texture_val, set = set_texture_val, hint = PROPERTY_HINT_RESOURCE_TYPE, hint_string = "Texture")]
     texture_val_rw: Option<Gd<Texture>>,
 }
@@ -120,7 +125,7 @@ impl HasProperty {
 
 #[godot_api]
 impl NodeVirtual for HasProperty {
-    fn init(base: Base<Node>) -> Self {
+    fn init(_base: Base<Node>) -> Self {
         HasProperty {
             int_val: 0,
             int_val_read: 2,
@@ -132,7 +137,6 @@ impl NodeVirtual for HasProperty {
             string_val: GodotString::new(),
             texture_val: Texture::new(),
             texture_val_rw: None,
-            base,
         }
     }
 }

itest/rust/src/signal_test.rs

@@ -17,10 +17,7 @@ use crate::itest;
 
 #[derive(GodotClass)]
 #[class(init, base=Object)]
-struct Emitter {
-    #[base]
-    base: Base<Object>,
-}
+struct Emitter {}
 
 #[godot_api]
 impl Emitter {
@@ -81,11 +78,8 @@ fn signals() {
         let signal_name = format!("signal_{i}_arg");
         let receiver_name = format!("receive_{i}_arg");
 
-        emitter
-            .base_mut()
-            .connect(signal_name.clone().into(), receiver.callable(receiver_name));
-
-        emitter.base_mut().emit_signal(signal_name.into(), arg);
+        emitter.connect(signal_name.clone().into(), receiver.callable(receiver_name));
+        emitter.emit_signal(signal_name.into(), arg);
 
         assert!(receiver.bind().used[i].get());
     }