Allow Deref/DerefMut for all Gd<T>; remove Gd::base/base_mut() again

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/registry.rs

@@ -311,10 +311,7 @@ pub mod callbacks {
         let base = unsafe { Base::from_sys(base_ptr) };
         let user_instance = make_user_instance(base);
 
-        // Create 2nd base to cache inside storage. This one will remain weak forever (no action on destruction).
-        let cached_base = unsafe { Base::from_sys(base_ptr) };
-
-        let instance = InstanceStorage::<T>::construct(user_instance, cached_base);
+        let instance = InstanceStorage::<T>::construct(user_instance);
         let instance_ptr = instance.into_raw();
         let instance_ptr = instance_ptr as sys::GDExtensionClassInstancePtr;
 

godot-core/src/storage.rs

@@ -12,31 +12,31 @@ 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_threaded::*;
 
 #[cfg(feature = "threads")]
-pub use multi_thread::*;
+pub(crate) use multi_threaded::*;
 
 #[cfg(not(feature = "threads"))]
-mod single_thread {
+mod single_threaded {
     use std::any::type_name;
     use std::cell;
 
-    use crate::obj::{Base, GodotClass};
+    use crate::obj::GodotClass;
     use crate::out;
 
     use super::Lifecycle;
 
     /// Manages storage and lifecycle of user's extension class instances.
     pub struct InstanceStorage<T: GodotClass> {
         user_instance: cell::RefCell<T>,
-        cached_base: Base<T::Base>,
 
         // Declared after `user_instance`, is dropped last
         pub lifecycle: cell::Cell<Lifecycle>,
@@ -45,12 +45,11 @@ mod single_thread {
 
     /// For all Godot extension classes
     impl<T: GodotClass> InstanceStorage<T> {
-        pub fn construct(user_instance: T, cached_base: Base<T::Base>) -> Self {
+        pub fn construct(user_instance: T) -> Self {
             out!("    Storage::construct             <{}>", type_name::<T>());
 
             Self {
                 user_instance: cell::RefCell::new(user_instance),
-                cached_base,
                 lifecycle: cell::Cell::new(Lifecycle::Alive),
                 godot_ref_count: cell::Cell::new(1),
             }
@@ -102,51 +101,70 @@ mod single_thread {
             })
         }
 
-        pub fn base(&self) -> &Base<T::Base> {
-            &self.cached_base
-        }
-
         pub(super) fn godot_ref_count(&self) -> u32 {
             self.godot_ref_count.get()
         }
     }
 }
 
 #[cfg(feature = "threads")]
-mod multi_thread {
+mod multi_threaded {
     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::obj::GodotClass;
+    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,
     }
 
     /// For all Godot extension classes
     impl<T: GodotClass> InstanceStorage<T> {
-        pub fn construct(user_instance: T, cached_base: Base<T::Base>) -> Self {
+        pub fn construct(user_instance: T) -> Self {
             out!("    Storage::construct             <{}>", type_name::<T>());
 
             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 +174,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={})     <{}>", // -- {:?}",
@@ -188,14 +206,25 @@ mod multi_thread {
             })
         }
 
-        pub fn base(&self) -> &Base<T::Base> {
-            &self.cached_base
-        }
-
         pub(super) fn godot_ref_count(&self) -> u32 {
             self.godot_ref_count.load(Ordering::Relaxed)
         }
+
+        // fn __static_type_check() {
+        //     enforce_sync::<InstanceStorage<T>>();
+        // }
     }
+
+    // TODO make InstanceStorage<T> Sync
+    // This type can be accessed concurrently from multiple threads, so it should be Sync. That implies however that T must be Sync too
+    // (and possibly Send, because with `&mut` access, a `T` can be extracted as a value using mem::take() etc.).
+    // Which again means that we need to infest half the codebase with T: Sync + Send bounds, *and* make it all conditional on
+    // `#[cfg(feature = "threads")]`. Until the multi-threading design is clarified, we'll thus leave it as is.
+    //
+    // The following code + __static_type_check() above would make sure that InstanceStorage is Sync.
+
+    // 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 +296,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 {