Move Deref/DerefMut out of T to Gd<T>; InstanceStorage safety

.github/workflows/minimal-ci.yml

@@ -90,7 +90,6 @@ jobs:
   godot-itest:
     name: godot-itest (${{ matrix.name }})
     runs-on: ${{ matrix.os }}
-    if: github.event.pull_request.draft != true
     continue-on-error: false
     timeout-minutes: 15
     strategy:

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

@@ -94,18 +94,18 @@ impl Main {
         self.base.add_child(mob_scene.share().upcast());
 
         let mut mob = mob_scene.cast::<mob::Mob>();
-        {
-            // Local scope to bind `mob`
-            let mut mob = mob.bind_mut();
-            let range = rng.gen_range(mob.min_speed..mob.max_speed);
-
-            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 range = {
+            // Local scope to bind `mob` user object
+            let mob = mob.bind();
+            rng.gen_range(mob.min_speed..mob.max_speed)
+        };
+
+        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.bind_mut().connect(
+        hud.connect(
             "start_game".into(),
             Callable::from_object_method(mob, "on_start_game"),
         );

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

@@ -58,8 +58,6 @@ impl Area2DVirtual for Player {
     }
 
     fn process(&mut self, delta: f64) {
-        println!("process");
-
         let mut animated_sprite = self
             .base
             .get_node_as::<AnimatedSprite2D>("AnimatedSprite2D");

godot-core/src/obj/gd.rs

@@ -160,25 +160,38 @@ where
         GdMut::from_cell(self.storage().get_mut())
     }
 
-    /// Storage object associated with the extension instance
-    // FIXME proper + safe interior mutability, also that Clippy is happy
-    #[allow(clippy::mut_from_ref)]
-    pub(crate) fn storage(&self) -> &mut InstanceStorage<T> {
-        let callbacks = crate::storage::nop_instance_callbacks();
-
+    /// 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
+        // calling this from multiple Gd pointers is safe.
+        // Potential issue is a concurrent free() in multi-threaded access; but that would need to be guarded against inside free().
         unsafe {
-            let token = sys::get_library() as *mut std::ffi::c_void;
-            let binding =
-                interface_fn!(object_get_instance_binding)(self.obj_sys(), token, &callbacks);
-
-            debug_assert!(
-                !binding.is_null(),
-                "Class {} -- null instance; does the class have a Godot creator function?",
-                std::any::type_name::<T>()
-            );
-            crate::private::as_storage::<T>(binding as sys::GDExtensionClassInstancePtr)
+            let binding = self.resolve_instance_ptr();
+            crate::private::as_storage::<T>(binding)
         }
     }
+
+    /// Storage object associated with the extension instance.
+    // pub(crate) fn storage_mut(&mut self) -> &mut InstanceStorage<T> {
+    //     // SAFETY:
+    //     unsafe {
+    //         let binding = self.resolve_instance_ptr();
+    //         crate::private::as_storage_mut::<T>(binding)
+    //     }
+    // }
+
+    unsafe fn resolve_instance_ptr(&self) -> sys::GDExtensionClassInstancePtr {
+        let callbacks = crate::storage::nop_instance_callbacks();
+        let token = sys::get_library() as *mut std::ffi::c_void;
+        let binding = interface_fn!(object_get_instance_binding)(self.obj_sys(), token, &callbacks);
+
+        debug_assert!(
+            !binding.is_null(),
+            "Class {} -- null instance; does the class have a Godot creator function?",
+            std::any::type_name::<T>()
+        );
+        binding as sys::GDExtensionClassInstancePtr
+    }
 }
 
 /// _The methods in this impl block are available for any `T`._ <br><br>
@@ -498,42 +511,43 @@ 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> {
-        //     opaque: OpaqueObject,         <- size of GDExtensionObjectPtr
-        //     _marker: PhantomData,         <- ZST
+        //     opaque: OpaqueObject,        // <- size of GDExtensionObjectPtr
+        //     _marker: PhantomData,        // <- ZST
         // }
         // 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.
+        // 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) }
     }
 }
+
 // SAFETY:
 // - `move_return_ptr`
 //   When the `call_type` is `PtrcallType::Virtual`, and the current type is known to inherit from `RefCounted`

godot-core/src/obj/traits.rs

@@ -122,17 +122,18 @@ pub trait Inherits<Base>: GodotClass {}
 
 impl<T: GodotClass> Inherits<T> for T {}
 
-/// Trait implemented for all objects that inherit from `Resource` or `Node`. As those are the only objects
-/// you can export to the editor.
+/// Trait implemented for all objects that inherit from `Resource` or `Node`.
+///
+/// Those are the only objects you can export to the editor.
 pub trait ExportableObject: GodotClass {}
 
-/// Auto-implemented for all engine-provided classes
+/// Auto-implemented for all engine-provided classes.
 pub trait EngineClass: GodotClass {
     fn as_object_ptr(&self) -> sys::GDExtensionObjectPtr;
     fn as_type_ptr(&self) -> sys::GDExtensionTypePtr;
 }
 
-/// Auto-implemented for all engine-provided enums
+/// Auto-implemented for all engine-provided enums.
 pub trait EngineEnum: Copy {
     fn try_from_ord(ord: i32) -> Option<Self>;
 
@@ -176,6 +177,7 @@ pub trait IndexEnum: EngineEnum {
 /// Capability traits, providing dedicated functionalities for Godot classes
 pub mod cap {
     use super::*;
+    use crate::obj::Gd;
 
     /// Trait for all classes that are constructible from the Godot engine.
     ///
@@ -192,6 +194,17 @@ pub mod cap {
         fn __godot_init(base: Base<Self::Base>) -> Self;
     }
 
+    /// Trait that's implemented for user-defined classes that provide a `#[base]` field.
+    ///
+    /// Gives direct access to the base pointer without going through upcast FFI.
+    pub trait WithBaseField: GodotClass {
+        #[doc(hidden)]
+        fn __godot_base(&self) -> &Gd<Self::Base>;
+
+        #[doc(hidden)]
+        fn __godot_base_mut(&mut self) -> &mut Gd<Self::Base>;
+    }
+
     // TODO Evaluate whether we want this public or not
     #[doc(hidden)]
     pub trait GodotToString: GodotClass {
@@ -247,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
@@ -258,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>,
@@ -271,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

@@ -307,9 +307,10 @@ pub mod callbacks {
 
         let base_ptr =
             unsafe { interface_fn!(classdb_construct_object)(base_class_name.string_sys()) };
-        let base = unsafe { Base::from_sys(base_ptr) };
 
+        let base = unsafe { Base::from_sys(base_ptr) };
         let user_instance = make_user_instance(base);
+
         let instance = InstanceStorage::<T>::construct(user_instance);
         let instance_ptr = instance.into_raw();
         let instance_ptr = instance_ptr as sys::GDExtensionClassInstancePtr;
@@ -334,9 +335,12 @@ pub mod callbacks {
         _class_user_data: *mut std::ffi::c_void,
         instance: sys::GDExtensionClassInstancePtr,
     ) {
-        let storage = as_storage::<T>(instance);
-        storage.mark_destroyed_by_godot();
-        let _drop = Box::from_raw(storage as *mut InstanceStorage<_>);
+        {
+            let storage = as_storage::<T>(instance);
+            storage.mark_destroyed_by_godot();
+        } // Ref no longer valid once next statement is executed.
+
+        crate::storage::destroy_storage::<T>(instance);
     }
 
     pub unsafe extern "C" fn get_virtual<T: cap::ImplementsGodotVirtual>(