diff --git a/godot-codegen/src/models/json.rs b/godot-codegen/src/models/json.rs index 16f37ca5a..335f29146 100644 --- a/godot-codegen/src/models/json.rs +++ b/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, + #[allow(dead_code)] pub is_keyed: bool, // pub members: Option>, // pub constants: Option>, @@ -156,11 +156,14 @@ pub struct JsonBuiltinConstant { #[derive(DeJson)] pub struct JsonOperator { pub name: String, + #[allow(dead_code)] pub right_type: Option, // 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, - /// `"general"` or `"math"` + /// Category: `"general"` or `"math"` + #[allow(dead_code)] pub category: String, pub is_vararg: bool, pub hash: i64, diff --git a/godot-codegen/src/special_cases/special_cases.rs b/godot-codegen/src/special_cases/special_cases.rs index 9ae87dc08..a328f41c9 100644 --- a/godot-codegen/src/special_cases/special_cases.rs +++ b/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() { diff --git a/godot-core/src/meta/error/call_error.rs b/godot-core/src/meta/error/call_error.rs index 3dee7a3c8..0d5c8c2e8 100644 --- a/godot-core/src/meta/error/call_error.rs +++ b/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) } diff --git a/godot-core/src/obj/gd.rs b/godot-core/src/obj/gd.rs index 7facb5c2d..775daa6d5 100644 --- a/godot-core/src/obj/gd.rs +++ b/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(init: F) -> Self where F: FnOnce(crate::obj::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 Gd { /// 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() } diff --git a/godot-core/src/obj/traits.rs b/godot-core/src/obj/traits.rs index 39dfeea3c..b635df277 100644 --- a/godot-core/src/obj/traits.rs +++ b/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::::default()`. + /// + /// # Panics + /// If `Self` is user-defined and its default constructor `init()` panics, that panic is propagated. fn new_gd() -> Gd; } @@ -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; } diff --git a/godot-core/src/private.rs b/godot-core/src/private.rs index 3aba75592..7ccf5da5c 100644 --- a/godot-core/src/private.rs +++ b/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 { None } +// ---------------------------------------------------------------------------------------------------------------------------------------------- +// Panic unwinding and catching + +pub struct PanicPayload { + payload: Box, +} + +impl PanicPayload { + pub fn new(payload: Box) -> 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(error_context: E, code: F) -> Result +pub fn handle_panic(error_context: E, code: F) -> Result 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( ) where F: FnOnce() -> Result + std::panic::UnwindSafe, { - let outcome: Result, String> = - handle_panic(|| format!("{call_ctx}"), code); + let outcome: Result, PanicPayload> = + handle_panic(|| call_ctx.to_string(), code); let call_error = match outcome { // All good. @@ -437,14 +458,14 @@ pub fn handle_ptrcall_panic(call_ctx: &CallContext, code: F) where F: FnOnce() -> R + std::panic::UnwindSafe, { - let outcome: Result = handle_panic(|| format!("{call_ctx}"), code); + let outcome: Result = 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 { #[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] diff --git a/godot-core/src/registry/callbacks.rs b/godot-core/src/registry/callbacks.rs index 23234dba3..3846a5651 100644 --- a/godot-core/src/registry/callbacks.rs +++ b/godot-core/src/registry/callbacks.rs @@ -15,6 +15,7 @@ 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; @@ -22,54 +23,71 @@ 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: . #[cfg(since_api = "4.4")] pub unsafe extern "C" fn create( _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( _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( _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(make_user_instance: F) -> sys::GDExtensionObjectPtr +pub(crate) fn create_custom( + make_user_instance: F, +) -> Result where T: GodotClass, F: FnOnce(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( make_user_instance: F, base_ptr: sys::GDExtensionObjectPtr, -) -> sys::GDExtensionClassInstancePtr +) -> Result where T: GodotClass, F: FnOnce(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::::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( @@ -387,20 +411,10 @@ pub unsafe extern "C" fn validate_property( 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(base: Box) -> Box { - let concrete = base - .downcast::::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(_class_builder: &mut dyn Any) { // TODO use actual argument, once class builder carries state // let class_builder = class_builder diff --git a/itest/godot/ManualFfiTests.gd b/itest/godot/ManualFfiTests.gd index d991da221..0b34c5f94 100644 --- a/itest/godot/ManualFfiTests.gd +++ b/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) diff --git a/itest/rust/src/register_tests/func_test.rs b/itest/rust/src/register_tests/func_test.rs index 31458b6a7..c46f22710 100644 --- a/itest/rust/src/register_tests/func_test.rs +++ b/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 { + 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::::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.