Gd: replace Deref/DerefMut with base() + base_mut(); rework InstanceS…

…torage

No more implicit access to base; avoids runtime-borrowing (Godot objects don't have Rust borrow limits)
and makes access more explicit.

InstanceStorage is now only accessed via shared-ref, which should fix some soundness issues with unbounded &mut.
All fields are immutable or have interior mutability.

godot-core/src/obj/gd.rs

@@ -160,25 +160,68 @@ 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();
+    /// 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
+        // 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>
@@ -506,12 +549,13 @@ where
 
     fn deref(&self) -> &Self::Target {
         // SAFETY:
-        // This relies on Gd<Node3D> having the layout as Node3D (as an example),
+        //
+        // This relies on `Gd<Node3D>` 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,
@@ -527,13 +571,14 @@ where
     fn deref_mut(&mut self) -> &mut T {
         // 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) }
     }
 }
+
 // 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 {

godot-core/src/registry.rs

@@ -307,10 +307,14 @@ 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);
+
+        // 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_ptr = instance.into_raw();
         let instance_ptr = instance_ptr as sys::GDExtensionClassInstancePtr;
 
@@ -334,9 +338,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>(