From 7699049e95ed282f6a8cacca389e7e9bbc099c01 Mon Sep 17 00:00:00 2001 From: Luo Zhihao Date: Wed, 24 Sep 2025 08:55:11 +0800 Subject: [PATCH 1/2] Make `base_mut` not clone `Gd` and work in init and predelete --- godot-core/src/classes/class_runtime.rs | 24 ------- godot-core/src/obj/base.rs | 8 +-- godot-core/src/obj/guards.rs | 5 +- godot-core/src/obj/raw_gd.rs | 39 ++++++++-- godot-core/src/obj/script.rs | 2 +- godot-core/src/obj/traits.rs | 22 +++--- itest/rust/src/object_tests/base_init_test.rs | 72 ++++++++++++++++++- 7 files changed, 117 insertions(+), 55 deletions(-) diff --git a/godot-core/src/classes/class_runtime.rs b/godot-core/src/classes/class_runtime.rs index f73ecada4..2d3fcd4ff 100644 --- a/godot-core/src/classes/class_runtime.rs +++ b/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(binding: sys::GDExtensionClassInstancePtr) -where - T: GodotClass + Bounds, -{ - 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::() - ) - } else { - panic!( - "Class {} -- null instance; does the class have a Godot creator function?", - std::any::type_name::() - ); - } -} - // ---------------------------------------------------------------------------------------------------------------------------------------------- // Implementation of this file diff --git a/godot-core/src/obj/base.rs b/godot-core/src/obj/base.rs index 49d1ca130..dc2331708 100644 --- a/godot-core/src/obj/base.rs +++ b/godot-core/src/obj/base.rs @@ -320,7 +320,7 @@ impl Base { /// Returns `true` if this `Base` 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 Base { /// # 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 { - #[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()) } } diff --git a/godot-core/src/obj/guards.rs b/godot-core/src/obj/guards.rs index d35db2689..9a2b6b0b1 100644 --- a/godot-core/src/obj/guards.rs +++ b/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, - _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>, } impl<'a, T: $bound> $ident<'a, T> { pub(crate) fn new( passive_gd: PassiveGd, - inaccessible_guard: InaccessibleGuard<'a, T>, + inaccessible_guard: Option>, ) -> Self { Self { passive_gd, diff --git a/godot-core/src/obj/raw_gd.rs b/godot-core/src/obj/raw_gd.rs index ef7d6a913..709940d18 100644 --- a/godot-core/src/obj/raw_gd.rs +++ b/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> { + #[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,8 +480,29 @@ 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::() + ) + } else { + panic!( + "Class {} -- null instance; does the class have a Godot creator function?", + std::any::type_name::() + ); + } + } + /// 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: @@ -483,9 +510,6 @@ where /// - 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::(ptr); + if ptr.is_null() { + return ptr::null_mut(); + } self.cached_storage_ptr.set(ptr); ptr diff --git a/godot-core/src/obj/script.rs b/godot-core/src/obj/script.rs index d6054a87b..c478331a1 100644 --- a/godot-core/src/obj/script.rs +++ b/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)) } } diff --git a/godot-core/src/obj/traits.rs b/godot-core/src/obj/traits.rs index 692f7b38d..f1ec487cb 100644 --- a/godot-core/src/obj/traits.rs +++ b/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,12 +516,17 @@ pub trait WithBaseField: GodotClass + Bounds { // 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` is guaranteed to be valid `Gd`. + 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` 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` 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 @@ -529,13 +534,8 @@ pub trait WithBaseField: GodotClass + Bounds { // 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; 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) diff --git a/itest/rust/src/object_tests/base_init_test.rs b/itest/rust/src/object_tests/base_init_test.rs index d78e58ade..f8cd35c13 100644 --- a/itest/rust/src/object_tests/base_init_test.rs +++ b/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 { + let mut arrays = Array::new(); + arrays.resize(mesh::ArrayType::ENUMERATOR_COUNT, &Variant::nil()); + let indices = PackedArray::::from([0, 1, 2]); + let vertex = PackedArray::::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, +} + +#[rustfmt::skip] +#[godot_api] +impl IArrayMesh for InitArrayMeshTest { + fn init(base: Base) -> 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 { unreachable!(); } + fn surface_get_blend_shape_arrays(&self, _index: i32) -> Array> { 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>) { unreachable!(); } + #[cfg(feature = "codegen-full")] + fn surface_get_material(&self, _index: i32) -> Option> { 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()); +} From a644d65e2ac334cf1a14ed76fd6af7d42ffbfd6f Mon Sep 17 00:00:00 2001 From: Luo Zhihao Date: Wed, 24 Sep 2025 08:58:00 +0800 Subject: [PATCH 2/2] Improve error messages of object freed in init and predelete --- godot-core/src/obj/bounds.rs | 8 ++- godot-core/src/registry/callbacks.rs | 29 +++++++--- itest/rust/src/object_tests/object_test.rs | 66 +++++++++++++++++++++- 3 files changed, 92 insertions(+), 11 deletions(-) diff --git a/godot-core/src/obj/bounds.rs b/godot-core/src/obj/bounds.rs index 47086f908..f698cb3a5 100644 --- a/godot-core/src/obj/bounds.rs +++ b/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 diff --git a/godot-core/src/registry/callbacks.rs b/godot-core/src/registry/callbacks.rs index accf583d6..99e2627cd 100644 --- a/godot-core/src/registry/callbacks.rs +++ b/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::::from_obj_sys_weak(base_ptr) }; + let mut obj = unsafe { PassiveGd::::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"); diff --git a/itest/rust/src/object_tests/object_test.rs b/itest/rust/src/object_tests/object_test.rs index 6fd31ae30..581f9c881 100644 --- a/itest/rust/src/object_tests/object_test.rs +++ b/itest/rust/src/object_tests/object_test.rs @@ -12,13 +12,16 @@ use std::cell::{Cell, RefCell}; use std::rc::Rc; use godot::builtin::{GString, StringName, Variant, Vector3}; +use godot::classes::notify::ObjectNotification; use godot::classes::{ - file_access, Engine, FileAccess, IRefCounted, Node, Node2D, Node3D, Object, RefCounted, + file_access, Engine, FileAccess, IObject, IRefCounted, Node, Node2D, Node3D, Object, RefCounted, }; use godot::global::godot_str; #[allow(deprecated)] use godot::meta::{FromGodot, GodotType, ToGodot}; -use godot::obj::{Base, Gd, Inherits, InstanceId, NewAlloc, NewGd, RawGd, Singleton}; +use godot::obj::{ + Base, Gd, Inherits, InstanceId, NewAlloc, NewGd, RawGd, Singleton, WithBaseField, +}; use godot::register::{godot_api, GodotClass}; use godot::sys::{self, interface_fn, GodotFfi}; @@ -1114,6 +1117,65 @@ fn double_use_reference() { emitter.free(); } +#[derive(GodotClass)] +#[class(base=Object)] +pub struct ObjInitFree { + base: Base, +} + +#[godot_api] +impl IObject for ObjInitFree { + fn init(base: Base) -> Self { + let sf = Self { base }; + Gd::::from_instance_id(sf.base().instance_id()).free(); + sf + } +} + +#[derive(GodotClass)] +#[class] +pub struct RefcInitUnref { + base: Base, +} + +#[godot_api] +impl IRefCounted for RefcInitUnref { + fn init(base: Base) -> Self { + let sf = Self { base }; + Gd::::from_instance_id(sf.base().instance_id()); + sf + } +} + +#[derive(GodotClass)] +#[class(init)] +struct RefcPredeleteUnref { + base: Base, +} + +#[godot_api] +impl IRefCounted for RefcPredeleteUnref { + fn on_notification(&mut self, what: ObjectNotification) { + if what == ObjectNotification::PREDELETE { + Gd::::from_instance_id(self.base().instance_id()); + } + } +} + +#[cfg(debug_assertions)] +#[itest] +fn object_unexpected_freed() { + expect_panic("Object freed in init()", || { + ObjInitFree::new_alloc().free(); + }); + expect_panic("RefCounted ref and unref in init()", || { + RefcInitUnref::new_gd(); + }); + + // RefCounted ref and unref in predelete. + RefcPredeleteUnref::new_gd(); +} + // ---------------------------------------------------------------------------------------------------------------------------------------------- // Test that one class can be declared multiple times (using #[cfg]) without conflicts