Propagate panics in object constructors to Gd::from_init_fn(), new_gd(), new_alloc()

Author: Bromeon

godot-core/src/obj/gd.rs

@@ -21,7 +21,7 @@ use crate::obj::{
     bounds, cap, Bounds, DynGd, GdDerefTarget, GdMut, GdRef, GodotClass, Inherits, InstanceId,
     OnEditor, RawGd, WithSignals,
 };
-use crate::private::callbacks;
+use crate::private::{callbacks, PanicPayload};
 use crate::registry::property::{object_export_element_type_string, Export, Var};
 use crate::{classes, out};
 
@@ -137,11 +137,16 @@ where
     ///     MyClass { my_base, other_field: 732 }
     /// });
     /// ```
+    ///
+    /// # Panics
+    /// Panics occurring in the `init` function are propagated to the caller.
     pub fn from_init_fn<F>(init: F) -> Self
     where
         F: FnOnce(crate::obj::Base<T::Base>) -> T,
     {
-        let object_ptr = callbacks::create_custom(init);
+        let object_ptr = callbacks::create_custom(init) // or propagate panic.
+            .unwrap_or_else(|payload| PanicPayload::repanic(payload));
+
         unsafe { Gd::from_obj_sys(object_ptr) }
     }
 
@@ -500,6 +505,11 @@ impl<T: GodotClass> Gd<T> {
     /// This is the default for most initializations from FFI. In cases where reference counter
     /// should explicitly **not** be updated, [`Self::from_obj_sys_weak`] is available.
     pub(crate) unsafe fn from_obj_sys(ptr: sys::GDExtensionObjectPtr) -> Self {
+        debug_assert!(
+            !ptr.is_null(),
+            "Gd::from_obj_sys() called with null pointer"
+        );
+
         Self::from_obj_sys_or_none(ptr).unwrap()
     }
 

godot-core/src/obj/traits.rs

@@ -511,6 +511,9 @@ pub trait NewGd: GodotClass {
     /// Return a new, ref-counted `Gd` containing a default-constructed instance.
     ///
     /// `MyClass::new_gd()` is equivalent to `Gd::<MyClass>::default()`.
+    ///
+    /// # Panics
+    /// If `Self` is user-defined and its default constructor `init()` panics, that panic is propagated.
     fn new_gd() -> Gd<Self>;
 }
 
@@ -529,6 +532,9 @@ pub trait NewAlloc: GodotClass {
     ///
     /// The result must be manually managed, e.g. by attaching it to the scene tree or calling `free()` after usage.
     /// Failure to do so will result in memory leaks.
+    ///
+    /// # Panics
+    /// If `Self` is user-defined and its default constructor `init()` panics, that panic is propagated to the caller.
     #[must_use]
     fn new_alloc() -> Gd<Self>;
 }

godot-core/src/private.rs

@@ -386,6 +386,10 @@ impl PanicPayload {
     pub fn into_panic_message(self) -> String {
         extract_panic_message(self.payload.as_ref())
     }
+
+    pub fn repanic(self) -> ! {
+        std::panic::resume_unwind(self.payload)
+    }
 }
 
 /// Executes `code`. If a panic is thrown, it is caught and an error message is printed to Godot.
@@ -495,13 +499,15 @@ pub fn rebuild_gd(object_ref: &classes::Object) -> Gd<classes::Object> {
 
 #[cfg(test)]
 mod tests {
-    use super::{CallError, CallErrors};
+    use super::{CallError, CallErrors, PanicPayload};
     use crate::meta::CallContext;
 
     fn make(index: usize) -> CallError {
         let method_name = format!("method_{index}");
         let ctx = CallContext::func("Class", &method_name);
-        CallError::failed_by_user_panic(&ctx, "some panic reason".to_string())
+        let payload = PanicPayload::new(Box::new("some panic reason".to_string()));
+
+        CallError::failed_by_user_panic(&ctx, payload)
     }
 
     #[test]

godot-core/src/registry/callbacks.rs

@@ -15,61 +15,79 @@ 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::private::{handle_panic, PanicPayload};
 use crate::registry::plugin::ErasedDynGd;
 use crate::storage::{as_storage, InstanceStorage, Storage, StorageRefCounted};
 use godot_ffi as sys;
 use std::any::Any;
 use sys::conv::u32_to_usize;
 use sys::interface_fn;
 
-// Creation callback has `p_notify_postinitialize` parameter since 4.4: https://github.com/godotengine/godot/pull/91018.
+/// Godot FFI default constructor.
+///
+/// If the `init()` constructor panics, null is returned.
+///
+/// Creation callback has `p_notify_postinitialize` parameter since 4.4: <https://github.com/godotengine/godot/pull/91018>.
 #[cfg(since_api = "4.4")]
 pub unsafe extern "C" fn create<T: cap::GodotDefault>(
     _class_userdata: *mut std::ffi::c_void,
     _notify_postinitialize: sys::GDExtensionBool,
 ) -> sys::GDExtensionObjectPtr {
-    create_custom(T::__godot_user_init)
+    create_custom(T::__godot_user_init).unwrap_or(std::ptr::null_mut())
 }
 
 #[cfg(before_api = "4.4")]
 pub unsafe extern "C" fn create<T: cap::GodotDefault>(
     _class_userdata: *mut std::ffi::c_void,
 ) -> sys::GDExtensionObjectPtr {
-    create_custom(T::__godot_user_init)
+    create_custom(T::__godot_user_init).unwrap_or(std::ptr::null_mut())
 }
 
+/// Godot FFI function for recreating a GDExtension instance, e.g. after a hot reload.
+///
+/// If the `init()` constructor panics, null is returned.
 #[cfg(since_api = "4.2")]
 pub unsafe extern "C" fn recreate<T: cap::GodotDefault>(
     _class_userdata: *mut std::ffi::c_void,
     object: sys::GDExtensionObjectPtr,
 ) -> sys::GDExtensionClassInstancePtr {
     create_rust_part_for_existing_godot_part(T::__godot_user_init, object)
+        .unwrap_or(std::ptr::null_mut())
 }
 
-pub(crate) fn create_custom<T, F>(make_user_instance: F) -> sys::GDExtensionObjectPtr
+pub(crate) fn create_custom<T, F>(
+    make_user_instance: F,
+) -> Result<sys::GDExtensionObjectPtr, PanicPayload>
 where
     T: GodotClass,
     F: FnOnce(Base<T::Base>) -> T,
 {
     let base_class_name = T::Base::class_name();
-
     let base_ptr = unsafe { interface_fn!(classdb_construct_object)(base_class_name.string_sys()) };
 
-    create_rust_part_for_existing_godot_part(make_user_instance, base_ptr);
+    match create_rust_part_for_existing_godot_part(make_user_instance, base_ptr) {
+        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) };
+
+            Err(payload)
+        }
+    }
 
     // std::mem::forget(base_class_name);
-    base_ptr
 }
 
-// with GDExt, custom object consists from two parts: Godot object and Rust object, that are
-// bound to each other. this method takes the first by pointer, creates the second with
-// supplied state and binds them together. that's used for both brand-new objects creation and
-// hot reload - during hot-reload, Rust objects are disposed and then created again with an
-// updated code, so that's necessary to link them to Godot objects again.
+/// Add Rust-side state for a GDExtension base object.
+///
+/// With godot-rust, custom objects consist of two parts: the Godot object and the Rust object. This method takes the Godot part by pointer,
+/// creates the Rust part with the supplied state, and links them together. This is used for both brand-new object creation and hot reload.
+/// During hot reload, Rust objects are disposed of and then created again with updated code, so it's necessary to re-link them to Godot objects.
 fn create_rust_part_for_existing_godot_part<T, F>(
     make_user_instance: F,
     base_ptr: sys::GDExtensionObjectPtr,
-) -> sys::GDExtensionClassInstancePtr
+) -> Result<sys::GDExtensionClassInstancePtr, PanicPayload>
 where
     T: GodotClass,
     F: FnOnce(Base<T::Base>) -> T,
@@ -79,7 +97,13 @@ where
     //out!("create callback: {}", class_name.backing);
 
     let base = unsafe { Base::from_sys(base_ptr) };
-    let user_instance = make_user_instance(unsafe { Base::from_base(&base) });
+
+    // 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))?;
+    // 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");
 
     let instance = InstanceStorage::<T>::construct(user_instance, base);
     let instance_ptr = instance.into_raw();
@@ -97,7 +121,7 @@ where
     }
 
     // std::mem::forget(class_name);
-    instance_ptr
+    Ok(instance_ptr)
 }
 
 pub unsafe extern "C" fn free<T: GodotClass>(
@@ -387,20 +411,10 @@ pub unsafe extern "C" fn validate_property<T: cap::GodotValidateProperty>(
 
     sys::conv::SYS_TRUE
 }
+
 // ----------------------------------------------------------------------------------------------------------------------------------------------
 // Safe, higher-level methods
 
-/// Abstracts the `GodotDefault` away, for contexts where this trait bound is not statically available
-pub fn erased_init<T: cap::GodotDefault>(base: Box<dyn Any>) -> Box<dyn Any> {
-    let concrete = base
-        .downcast::<Base<<T as GodotClass>::Base>>()
-        .expect("erased_init: bad type erasure");
-    let extracted: Base<_> = sys::unbox(concrete);
-
-    let instance = T::__godot_user_init(extracted);
-    Box::new(instance)
-}
-
 pub fn register_class_by_builder<T: cap::GodotRegisterClass>(_class_builder: &mut dyn Any) {
     // TODO use actual argument, once class builder carries state
     // let class_builder = class_builder

itest/godot/ManualFfiTests.gd

@@ -338,6 +338,13 @@ func test_func_rename():
 	assert_eq(func_rename.has_method("spell_static"), true)
 	assert_eq(func_rename.spell_static(), "static")
 
+func test_init_panic():
+	var obj := InitPanic.new() # panics in Rust
+	assert_eq(obj, null, "Rust panic in init() returns null in GDScript")
+
+	# Alternative behavior (probably not desired):
+	# assert_eq(obj.get_class(), "RefCounted", "panic in init() returns base instance without GDExtension part")
+
 var gd_self_obj: GdSelfObj
 func update_self_reference(value):
 	gd_self_obj.update_internal(value)

itest/rust/src/register_tests/func_test.rs

@@ -8,7 +8,7 @@
 // Needed for Clippy to accept #[cfg(all())]
 #![allow(clippy::non_minimal_cfg)]
 
-use crate::framework::itest;
+use crate::framework::{expect_panic, itest};
 use godot::classes::ClassDb;
 use godot::prelude::*;
 
@@ -263,9 +263,39 @@ impl IRefCounted for GdSelfObj {
     }
 }
 
+// ----------------------------------------------------------------------------------------------------------------------------------------------
+
+// Also tests lack of #[class].
+#[derive(GodotClass)]
+struct InitPanic;
+
+#[godot_api]
+impl IRefCounted for InitPanic {
+    // Panicking constructor.
+    fn init(_base: Base<Self::Base>) -> Self {
+        panic!("InitPanic::init() exploded");
+    }
+}
+
 // ----------------------------------------------------------------------------------------------------------------------------------------------
 // Tests
 
+#[itest]
+fn init_panic_is_caught() {
+    expect_panic("default construction propagates panic", || {
+        let _obj = InitPanic::new_gd();
+    });
+}
+
+#[itest]
+fn init_fn_panic_is_caught() {
+    expect_panic("Gd::from_init_fn() propagates panic", || {
+        let _obj = Gd::<InitPanic>::from_init_fn(|_base| panic!("custom init closure exploded"));
+    });
+}
+
+// No test for Gd::from_object(), as that simply moves the existing object without running user code.
+
 #[itest]
 fn cfg_doesnt_interfere_with_valid_method_impls() {
     // If we re-implement this method but the re-implementation is removed, that should keep the non-removed implementation.