Merge pull request #1140 from godot-rust/bugfix/init-unhandled-panic

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

Author: Bromeon

godot-codegen/src/models/json.rs

@@ -5,16 +5,12 @@
  * file, You can obtain one at https://mozilla.org/MPL/2.0/.
  */
 
-// TODO remove this warning once impl is complete
-// Several types have #[allow(dead_code)], can be subsequently removed.
-
-// In #[derive(DeJson)].
+// In #[derive(DeJson)]: "this block may be rewritten with the `?` operator"
 #![allow(clippy::question_mark)]
-//
+
 // This file acts as deserialization check of the JSON file. Even if some fields are unused, having them declared makes sure they're
 // deserializable and conform to our expectations. It also doesn't add much value to annotate individual fields; it doesn't really
 // matter if some are unused because it's external input data.
-#![allow(dead_code)]
 
 use nanoserde::DeJson;
 
@@ -38,7 +34,9 @@ pub struct JsonHeader {
     pub version_major: u8,
     pub version_minor: u8,
     pub version_patch: u8,
+    #[allow(dead_code)]
     pub version_status: String,
+    #[allow(dead_code)]
     pub version_build: String,
     pub version_full_name: String,
 }
@@ -58,7 +56,9 @@ pub struct JsonBuiltinSizeForConfig {
 #[derive(DeJson)]
 pub struct JsonBuiltinClass {
     pub name: String,
+    #[allow(dead_code)]
     pub indexing_return_type: Option<String>,
+    #[allow(dead_code)]
     pub is_keyed: bool,
     // pub members: Option<Vec<Member>>,
     // pub constants: Option<Vec<BuiltinConstant>>,
@@ -156,11 +156,14 @@ pub struct JsonBuiltinConstant {
 #[derive(DeJson)]
 pub struct JsonOperator {
     pub name: String,
+    #[allow(dead_code)]
     pub right_type: Option<String>, // null if unary
+    #[allow(dead_code)]
     pub return_type: String,
 }
 
 #[derive(DeJson)]
+#[allow(dead_code)]
 pub struct JsonMember {
     pub name: String,
     #[nserde(rename = "type")]
@@ -194,7 +197,8 @@ pub struct JsonConstructor {
 pub struct JsonUtilityFunction {
     pub name: String,
     pub return_type: Option<String>,
-    /// `"general"` or `"math"`
+    /// Category: `"general"` or `"math"`
+    #[allow(dead_code)]
     pub category: String,
     pub is_vararg: bool,
     pub hash: i64,

godot-codegen/src/special_cases/special_cases.rs

@@ -595,6 +595,7 @@ pub fn maybe_rename_virtual_method<'m>(class_name: &TyName, rust_method_name: &'
 
 // TODO method-level extra docs, for:
 // - Node::rpc_config() -> link to RpcConfig.
+// - Node::process/physics_process -> mention `f32`/`f64` duality.
 
 pub fn get_class_extra_docs(class_name: &TyName) -> Option<&'static str> {
     match class_name.godot_ty.as_str() {

godot-core/src/meta/error/call_error.rs

@@ -8,6 +8,7 @@
 use crate::builtin::{Variant, VariantType};
 use crate::meta::error::{ConvertError, ErasedConvertError};
 use crate::meta::{CallContext, ToGodot};
+use crate::private::PanicPayload;
 use crate::sys;
 use godot_ffi::join_debug;
 use std::error::Error;
@@ -300,11 +301,13 @@ impl CallError {
     }
 
     #[doc(hidden)]
-    pub fn failed_by_user_panic(call_ctx: &CallContext, reason: String) -> Self {
+    pub fn failed_by_user_panic(call_ctx: &CallContext, panic_payload: PanicPayload) -> Self {
         // This can cause the panic message to be printed twice in some scenarios (e.g. bind_mut() borrow failure).
         // But in other cases (e.g. itest `dynamic_call_with_panic`), it is only printed once.
         // Would need some work to have a consistent experience.
 
+        let reason = panic_payload.into_panic_message();
+
         Self::new(call_ctx, format!("function panicked: {reason}"), None)
     }
 

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

@@ -218,7 +218,7 @@ pub fn is_class_runtime(is_tool: bool) -> bool {
 }
 
 // ----------------------------------------------------------------------------------------------------------------------------------------------
-// Panic handling
+// Panic *hook* management
 
 pub fn extract_panic_message(err: &(dyn Send + std::any::Any)) -> String {
     if let Some(s) = err.downcast_ref::<&'static str>() {
@@ -370,13 +370,35 @@ pub fn get_gdext_panic_context() -> Option<String> {
     None
 }
 
+// ----------------------------------------------------------------------------------------------------------------------------------------------
+// Panic unwinding and catching
+
+pub struct PanicPayload {
+    payload: Box<dyn std::any::Any + Send + 'static>,
+}
+
+impl PanicPayload {
+    pub fn new(payload: Box<dyn std::any::Any + Send + 'static>) -> Self {
+        Self { payload }
+    }
+
+    // While this could be `&self`, it's usually good practice to pass panic payloads around linearly and have only 1 representation at a time.
+    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.
 ///
 /// Returns `Err(message)` if a panic occurred, and `Ok(result)` with the result of `code` otherwise.
 ///
 /// In contrast to [`handle_varcall_panic`] and [`handle_ptrcall_panic`], this function is not intended for use in `try_` functions,
 /// where the error is propagated as a `CallError` in a global variable.
-pub fn handle_panic<E, F, R>(error_context: E, code: F) -> Result<R, String>
+pub fn handle_panic<E, F, R>(error_context: E, code: F) -> Result<R, PanicPayload>
 where
     E: Fn() -> String,
     F: FnOnce() -> R + std::panic::UnwindSafe,
@@ -390,8 +412,7 @@ where
         cell.borrow_mut().push_function(&error_context)
     });
 
-    let result =
-        std::panic::catch_unwind(code).map_err(|payload| extract_panic_message(payload.as_ref()));
+    let result = std::panic::catch_unwind(code).map_err(PanicPayload::new);
 
     #[cfg(debug_assertions)]
     ERROR_CONTEXT_STACK.with(|cell| cell.borrow_mut().pop_function());
@@ -407,8 +428,8 @@ pub fn handle_varcall_panic<F, R>(
 ) where
     F: FnOnce() -> Result<R, CallError> + std::panic::UnwindSafe,
 {
-    let outcome: Result<Result<R, CallError>, String> =
-        handle_panic(|| format!("{call_ctx}"), code);
+    let outcome: Result<Result<R, CallError>, PanicPayload> =
+        handle_panic(|| call_ctx.to_string(), code);
 
     let call_error = match outcome {
         // All good.
@@ -437,14 +458,14 @@ pub fn handle_ptrcall_panic<F, R>(call_ctx: &CallContext, code: F)
 where
     F: FnOnce() -> R + std::panic::UnwindSafe,
 {
-    let outcome: Result<R, String> = handle_panic(|| format!("{call_ctx}"), code);
+    let outcome: Result<R, PanicPayload> = handle_panic(|| call_ctx.to_string(), code);
 
     let call_error = match outcome {
         // All good.
         Ok(_result) => return,
 
         // Panic occurred (typically through user): forward message.
-        Err(panic_msg) => CallError::failed_by_user_panic(call_ctx, panic_msg),
+        Err(payload) => CallError::failed_by_user_panic(call_ctx, payload),
     };
 
     let _id = report_call_error(call_error, false);
@@ -478,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