Make base_mut not clone Gd and work in init and predelete

godot-core/src/classes/class_runtime.rs

@@ -226,30 +226,6 @@ pub(crate) fn ensure_object_inherits(derived: ClassId, base: ClassId, instance_i
     )
 }
 
-#[cfg(debug_assertions)]
-pub(crate) fn ensure_binding_not_null<T>(binding: sys::GDExtensionClassInstancePtr)
-where
-    T: GodotClass + Bounds<Declarer = bounds::DeclUser>,
-{
-    if !binding.is_null() {
-        return;
-    }
-
-    // Non-tool classes can't be instantiated in the editor.
-    if crate::classes::Engine::singleton().is_editor_hint() {
-        panic!(
-            "Class {} -- null instance; does the class have a Godot creator function? \
-            Ensure that the given class is a tool class with #[class(tool)], if it is being accessed in the editor.",
-            std::any::type_name::<T>()
-        )
-    } else {
-        panic!(
-            "Class {} -- null instance; does the class have a Godot creator function?",
-            std::any::type_name::<T>()
-        );
-    }
-}
-
 // ----------------------------------------------------------------------------------------------------------------------------------------------
 // Implementation of this file
 

godot-core/src/obj/base.rs

@@ -320,7 +320,7 @@ impl<T: GodotClass> Base<T> {
 
     /// Returns `true` if this `Base<T>` is currently in the initializing state.
     #[cfg(debug_assertions)]
-    fn is_initializing(&self) -> bool {
+    pub(crate) fn is_initializing(&self) -> bool {
         self.init_state.get() == InitState::ObjectConstructing
     }
 
@@ -344,12 +344,6 @@ impl<T: GodotClass> Base<T> {
     /// # Safety
     /// Caller must ensure that the underlying object remains valid for the entire lifetime of the returned `PassiveGd`.
     pub(crate) unsafe fn constructed_passive(&self) -> PassiveGd<T> {
-        #[cfg(debug_assertions)] // debug_assert! still checks existence of symbols.
-        assert!(
-            !self.is_initializing(),
-            "WithBaseField::base(), base_mut() can only be called on fully-constructed objects, after I*::init() or Gd::from_init_fn()"
-        );
-
         // SAFETY: object pointer is valid and remains valid as long as self is alive (per safety precondition of this fn).
         unsafe { PassiveGd::from_obj_sys(self.obj.obj_sys()) }
     }

godot-core/src/obj/bounds.rs

@@ -221,7 +221,9 @@ impl DynMemory for MemRefCounted {
         }
         obj.with_ref_counted(|refc| {
             let success = refc.init_ref();
-            assert!(success, "init_ref() failed");
+            if !success {
+                crate::godot_error!("init_ref() failed, make sure you don't create a `Gd` pointer to base/self in predelete or drop()");
+            };
         });
 
         /*
@@ -249,6 +251,10 @@ impl DynMemory for MemRefCounted {
             return false;
         }
         obj.with_ref_counted(|refc| {
+            if refc.get_reference_count() == 0 {
+	            crate::godot_error!("Trying to unreference a RefCounted whose reference count is already zero, make sure you don't create a `Gd` pointer to base/self in predelete or drop()");
+				return false;
+            }
             let is_last = refc.unreference();
             out!("  +-- was last={is_last}");
             is_last

godot-core/src/obj/guards.rs

@@ -232,13 +232,14 @@ macro_rules! make_base_mut {
         #[doc = concat!("See [`", stringify!($doc_type), "::base_mut()`](", stringify!($doc_path), "::base_mut()) for usage.\n")]
         pub struct $ident<'a, T: $bound> {
             passive_gd: PassiveGd<T::Base>,
-            _inaccessible_guard: InaccessibleGuard<'a, T>,
+       		// Option because we can't make a guard yet since the instance storage is null during initializing.
+    	    _inaccessible_guard: Option<InaccessibleGuard<'a, T>>,
         }
 
         impl<'a, T: $bound> $ident<'a, T> {
             pub(crate) fn new(
                 passive_gd: PassiveGd<T::Base>,
-                inaccessible_guard: InaccessibleGuard<'a, T>,
+                inaccessible_guard: Option<InaccessibleGuard<'a, T>>,
             ) -> Self {
                 Self {
                     passive_gd,

godot-core/src/obj/raw_gd.rs

@@ -18,6 +18,7 @@ use crate::meta::{
 use crate::obj::bounds::{Declarer, DynMemory as _};
 use crate::obj::casts::CastSuccess;
 use crate::obj::rtti::ObjectRtti;
+use crate::obj::traits::Singleton;
 use crate::obj::{bounds, Bounds, Gd, GdDerefTarget, GdMut, GdRef, GodotClass, InstanceId};
 use crate::storage::{InstanceCache, InstanceStorage, Storage};
 use crate::{classes, meta, out};
@@ -440,7 +441,12 @@ where
     /// Storage object associated with the extension instance.
     ///
     /// Returns `None` if self is null.
+    ///
+    /// # Panics
+    /// In Debug mode, if binding is null.
     pub(crate) fn storage(&self) -> Option<&InstanceStorage<T>> {
+        #[cfg(debug_assertions)]
+        self.ensure_storage_not_null();
         // SAFETY:
         // - We have a `&self`, so the storage must already have been created.
         // - The storage cannot be destroyed while we have a `&self` reference, so it will not be
@@ -450,7 +456,7 @@ where
 
     /// Storage object associated with the extension instance.
     ///
-    /// Returns `None` if self is null.
+    /// Returns `None` if self is null, or if binding is null.
     ///
     /// # Safety
     ///
@@ -474,18 +480,36 @@ where
         }
     }
 
+    #[cfg(debug_assertions)]
+    pub(crate) fn ensure_storage_not_null(&self) {
+        let binding = self.resolve_instance_ptr();
+        if !binding.is_null() {
+            return;
+        }
+        // Non-tool classes can't be instantiated in the editor.
+        if crate::classes::Engine::singleton().is_editor_hint() {
+            panic!(
+                "Class {} -- null instance; does the class have a Godot creator function? \
+                Ensure that the given class is a tool class with #[class(tool)], if it is being accessed in the editor.",
+                std::any::type_name::<Self>()
+            )
+        } else {
+            panic!(
+                "Class {} -- null instance; does the class have a Godot creator function?",
+                std::any::type_name::<Self>()
+            );
+        }
+    }
+
     /// Retrieves and caches pointer to this class instance if `self.obj` is non-null.
-    /// Returns a null pointer otherwise.
+    /// Returns a null pointer otherwise, or if binding is null.
     ///
     /// Note: The returned pointer to the GDExtensionClass instance (even when `self.obj` is non-null)
     /// might still be null when:
     /// - The class isn't instantiable in the current context.
     /// - The instance is a placeholder (e.g., non-`tool` classes in the editor).
     ///
     /// However, null pointers might also occur in other, undocumented contexts.
-    ///
-    /// # Panics
-    /// In Debug mode, if binding is null.
     fn resolve_instance_ptr(&self) -> sys::GDExtensionClassInstancePtr {
         if self.is_null() {
             return ptr::null_mut();
@@ -509,8 +533,9 @@ where
 
         let ptr: sys::GDExtensionClassInstancePtr = binding.cast();
 
-        #[cfg(debug_assertions)]
-        crate::classes::ensure_binding_not_null::<T>(ptr);
+        if ptr.is_null() {
+            return ptr::null_mut();
+        }
 
         self.cached_storage_ptr.set(ptr);
         ptr

godot-core/src/obj/script.rs

@@ -503,7 +503,7 @@ impl<'a, T: ScriptInstance> SiMut<'a, T> {
         let guard = self.cell.make_inaccessible(self.mut_ref).unwrap();
         let passive_gd = self.base_ref.to_script_passive();
 
-        ScriptBaseMut::new(passive_gd, guard)
+        ScriptBaseMut::new(passive_gd, Some(guard))
     }
 }
 

godot-core/src/obj/traits.rs

@@ -12,7 +12,7 @@ use crate::builtin::GString;
 use crate::init::InitLevel;
 use crate::meta::inspect::EnumConstant;
 use crate::meta::ClassId;
-use crate::obj::{bounds, Base, BaseMut, BaseRef, Bounds, Gd};
+use crate::obj::{bounds, Base, BaseMut, BaseRef, Bounds, Gd, PassiveGd};
 use crate::registry::signal::SignalObject;
 use crate::storage::Storage;
 
@@ -516,26 +516,26 @@ pub trait WithBaseField: GodotClass + Bounds<Declarer = bounds::DeclUser> {
         // We need to construct this first, as the mut-borrow below will block all other access.
         // SAFETY: lifetime is re-established at the bottom BaseMut construction, since return type of this fn has lifetime bound to instance.
         let passive_gd = unsafe { self.base_field().constructed_passive() };
+        // SAFETY: The object ptr of `Gd<Self::Base>` is guaranteed to be valid `Gd<Self>`.
+        let gd = unsafe { PassiveGd::from_obj_sys(passive_gd.obj_sys()) };
 
-        let gd = self.to_gd();
+        // If the object is fully constructed, the associated instance storage must have been created.
+        #[cfg(debug_assertions)]
+        if !self.base_field().is_initializing() {
+            gd.raw.ensure_storage_not_null();
+        }
 
         // SAFETY:
-        // - We have a `Gd<Self>` so, provided that `storage_unbounded` succeeds, the associated instance
-        //   storage has been created.
+        // - `storage_unbounded` will succeed if the object is fully constructed, otherwise it returns None.
         //
         // - Since we can get a `&'a Base<Self::Base>` from `&'a self`, that must mean we have a Rust object
         //   somewhere that has this base object. The only way to have such a base object is by being the
         //   Rust object referenced by that base object. I.e. this storage's user-instance is that Rust
         //   object. That means this storage cannot be destroyed for the lifetime of that Rust object. And
         //   since we have a reference to the base object derived from that Rust object, then that Rust
         //   object must outlive `'a`. And so the storage cannot be destroyed during the lifetime `'a`.
-        let storage = unsafe {
-            gd.raw
-                .storage_unbounded()
-                .expect("we have Gd<Self>; its RawGd should not be null")
-        };
-
-        let guard = storage.get_inaccessible(self);
+        let storage = unsafe { gd.raw.storage_unbounded() };
+        let guard = storage.map(|s| s.get_inaccessible(self));
 
         // Narrows lifetime again from 'static to 'self.
         BaseMut::new(passive_gd, guard)

godot-core/src/registry/callbacks.rs

@@ -20,7 +20,9 @@ use crate::builder::ClassBuilder;
 use crate::builtin::{StringName, Variant};
 use crate::classes::Object;
 use crate::meta::PropertyInfo;
-use crate::obj::{bounds, cap, AsDyn, Base, Bounds, Gd, GodotClass, Inherits, UserClass};
+use crate::obj::{
+    bounds, cap, AsDyn, Base, Bounds, Gd, GodotClass, Inherits, PassiveGd, UserClass,
+};
 use crate::private::{handle_panic, IntoVirtualMethodReceiver, PanicPayload};
 use crate::registry::plugin::ErasedDynGd;
 use crate::storage::{as_storage, InstanceStorage, Storage, StorageRefCounted};
@@ -100,24 +102,25 @@ where
 {
     let base_class_name = T::Base::class_id();
     let base_ptr = unsafe { sys::classdb_construct_object(base_class_name.string_sys()) };
+    let raw_id = unsafe { interface_fn!(object_get_instance_id)(base_ptr) };
 
     let postinit = |base_ptr| {
         #[cfg(since_api = "4.4")]
         if notify_postinitialize {
             // Should notify it with a weak pointer, during `NOTIFICATION_POSTINITIALIZE`, ref-counted object is not yet fully-initialized.
-            let mut obj = unsafe { Gd::<Object>::from_obj_sys_weak(base_ptr) };
+            let mut obj = unsafe { PassiveGd::<Object>::from_obj_sys(base_ptr) };
             obj.notify(crate::classes::notify::ObjectNotification::POSTINITIALIZE);
-            obj.drop_weak();
         }
     };
 
     match create_rust_part_for_existing_godot_part(make_user_instance, base_ptr, postinit) {
         Ok(_extension_ptr) => Ok(base_ptr),
         Err(payload) => {
-            // Creation of extension object failed; we must now also destroy the base object to avoid leak.
-            // SAFETY: `base_ptr` was just created above.
-            unsafe { interface_fn!(object_destroy)(base_ptr) };
-
+            if crate::global::is_instance_id_valid(raw_id as i64) {
+                // Creation of extension object failed; we must now also destroy the base object to avoid leak.
+                // SAFETY: the validity of `base_ptr` is checked above.
+                unsafe { interface_fn!(object_destroy)(base_ptr) };
+            }
             Err(payload)
         }
     }
@@ -144,11 +147,21 @@ where
     //out!("create callback: {}", class_name.backing);
 
     let base = unsafe { Base::from_sys(base_ptr) };
+    let raw_id = unsafe { interface_fn!(object_get_instance_id)(base_ptr) };
 
     // User constructor init() can panic, which crashes the engine if unhandled.
     let context = || format!("panic during {class_name}::init() constructor");
     let code = || make_user_instance(unsafe { Base::from_base(&base) });
-    let user_instance = handle_panic(context, std::panic::AssertUnwindSafe(code))?;
+    let user_instance = handle_panic(
+        context,
+        std::panic::AssertUnwindSafe(|| {
+            let instance = code();
+            debug_assert!(crate::global::is_instance_id_valid(raw_id as i64),
+			   	"Object is released during {class_name}::init(). For RefCounted, make sure you don't create a `Gd` pointer to base/self in init(). For Object, make sure you don't call `Gd.free()` in init()"
+        	);
+            instance
+        }),
+    )?;
 
     // Print shouldn't be necessary as panic itself is printed. If this changes, re-enable in error case:
     // godot_error!("failed to create instance of {class_name}; Rust init() panicked");

itest/rust/src/object_tests/base_init_test.rs

@@ -6,11 +6,11 @@
  */
 
 use godot::builtin::real_consts::FRAC_PI_3;
-use godot::builtin::Vector2;
+use godot::builtin::{Array, PackedArray, Variant, Vector2};
 use godot::classes::notify::ObjectNotification;
-use godot::classes::{ClassDb, IRefCounted, RefCounted};
+use godot::classes::{mesh, ArrayMesh, ClassDb, IArrayMesh, IRefCounted, RefCounted};
 use godot::meta::ToGodot;
-use godot::obj::{Base, Gd, InstanceId, NewAlloc, NewGd, Singleton, WithBaseField};
+use godot::obj::{Base, Gd, IndexEnum, InstanceId, NewAlloc, NewGd, Singleton, WithBaseField};
 use godot::register::{godot_api, GodotClass};
 use godot::task::TaskHandle;
 
@@ -197,3 +197,69 @@ fn base_postinit_refcounted() -> TaskHandle {
     assert_eq!(obj.get_reference_count(), 2);
     next_frame(move || assert_eq!(obj.get_reference_count(), 1, "eventual dec-ref happens"))
 }
+
+fn make_mesh_arrays() -> Array<Variant> {
+    let mut arrays = Array::new();
+    arrays.resize(mesh::ArrayType::ENUMERATOR_COUNT, &Variant::nil());
+    let indices = PackedArray::<i32>::from([0, 1, 2]);
+    let vertex = PackedArray::<Vector2>::from([
+        Vector2::new(0.0, 0.0),
+        Vector2::new(1.0, 0.0),
+        Vector2::new(0.0, 1.0),
+    ]);
+    arrays.set(mesh::ArrayType::INDEX.to_index(), &indices.to_variant());
+    arrays.set(mesh::ArrayType::VERTEX.to_index(), &vertex.to_variant());
+    arrays
+}
+
+#[derive(GodotClass)]
+#[class(base=ArrayMesh)]
+struct InitArrayMeshTest {
+    base: Base<ArrayMesh>,
+}
+
+#[rustfmt::skip]
+#[godot_api]
+impl IArrayMesh for InitArrayMeshTest {
+    fn init(base: Base<ArrayMesh>) -> Self {
+        let mut sf = Self { base };
+        sf.base_mut()
+            .add_surface_from_arrays(mesh::PrimitiveType::TRIANGLES, &make_mesh_arrays());
+        sf
+    }
+
+    fn on_notification(&mut self, what: ObjectNotification) {
+        if what == ObjectNotification::PREDELETE {
+            let arr = make_mesh_arrays();
+            self.base_mut().add_surface_from_arrays(mesh::PrimitiveType::TRIANGLES, &arr);
+
+            assert_eq!(self.base().get_surface_count(), 2);
+            assert_eq!(self.base().surface_get_arrays(0), arr);
+            assert_eq!(self.base().surface_get_arrays(1), arr);
+        }
+    }
+
+    fn get_surface_count(&self) -> i32 { unreachable!(); }
+    fn surface_get_array_len(&self, _index: i32) -> i32 { unreachable!(); }
+    fn surface_get_array_index_len(&self, _index: i32) -> i32 { unreachable!(); }
+    fn surface_get_arrays(&self, _index: i32) -> Array<Variant> { unreachable!(); }
+    fn surface_get_blend_shape_arrays(&self, _index: i32) -> Array<Array<Variant>> { unreachable!(); }
+    fn surface_get_lods(&self, _index: i32) -> godot::builtin::Dictionary { unreachable!(); }
+    fn surface_get_format(&self, _index: i32) -> u32 { unreachable!(); }
+    fn surface_get_primitive_type(&self, _index: i32) -> u32 { unreachable!(); }
+    #[cfg(feature = "codegen-full")]
+    fn surface_set_material(&mut self, _index: i32, _material: Option<Gd<godot::classes::Material>>) { unreachable!(); }
+    #[cfg(feature = "codegen-full")]
+    fn surface_get_material(&self, _index: i32) -> Option<Gd<godot::classes::Material>> { unreachable!(); }
+    fn get_blend_shape_count(&self) -> i32 { unreachable!(); }
+    fn get_blend_shape_name(&self, _index: i32) -> godot::builtin::StringName { unreachable!(); }
+    fn set_blend_shape_name(&mut self, _index: i32, _name: godot::builtin::StringName){ unreachable!(); }
+    fn get_aabb(&self) -> godot::builtin::Aabb { unreachable!(); }
+}
+
+#[itest]
+fn base_mut_init_array_mesh() {
+    let mesh = InitArrayMeshTest::new_gd();
+    assert_eq!(mesh.get_surface_count(), 1);
+    assert_eq!(mesh.surface_get_arrays(0), make_mesh_arrays());
+}