From 709c1e319757cdad6dc114e704f9b671c2dc687b Mon Sep 17 00:00:00 2001 From: Jan Haller Date: Tue, 28 Oct 2025 08:10:36 +0100 Subject: [PATCH 1/2] Verify that panicking ptrcalls return Godot default value --- itest/godot/input/GenFfiTests.template.gd | 15 +++++++++++++++ itest/rust/build.rs | 19 +++++++++++++------ 2 files changed, 28 insertions(+), 6 deletions(-) diff --git a/itest/godot/input/GenFfiTests.template.gd b/itest/godot/input/GenFfiTests.template.gd index c01966ecd..a732a3e97 100644 --- a/itest/godot/input/GenFfiTests.template.gd +++ b/itest/godot/input/GenFfiTests.template.gd @@ -67,6 +67,21 @@ func test_ptrcall_IDENT(): mark_test_succeeded() #) +# Functions that are invoked via ptrcall do not have an API to propagate the error back to the caller, but Godot pre-initializes their +# return value to the default value of that type. This test verifies that in case of panic, the default value (per Godot) is returned. +#( +func test_ptrcall_panic_IDENT(): + mark_test_pending() + var ffi := GenFfi.new() + + var from_rust: TYPE = ffi.panic_IDENT() + _check_callconv("panic_IDENT", "ptrcall") + + var expected_default: TYPE # initialize to default (null for objects) + assert_eq(from_rust, expected_default, "return value from panicked ptrcall fn == default value") + mark_test_succeeded() +#) + #( func test_ptrcall_static_IDENT(): mark_test_pending() diff --git a/itest/rust/build.rs b/itest/rust/build.rs index 6ca87b303..d664baaaa 100644 --- a/itest/rust/build.rs +++ b/itest/rust/build.rs @@ -320,12 +320,14 @@ fn generate_rust_methods(inputs: &[Input]) -> Vec { .. } = input; - let return_method = format_ident!("return_{}", ident); - let accept_method = format_ident!("accept_{}", ident); - let mirror_method = format_ident!("mirror_{}", ident); - let return_static_method = format_ident!("return_static_{}", ident); - let accept_static_method = format_ident!("accept_static_{}", ident); - let mirror_static_method = format_ident!("mirror_static_{}", ident); + let return_method = format_ident!("return_{ident}"); + let accept_method = format_ident!("accept_{ident}"); + let mirror_method = format_ident!("mirror_{ident}"); + let panic_method = format_ident!("panic_{ident}"); + + let return_static_method = format_ident!("return_static_{ident}"); + let accept_static_method = format_ident!("accept_static_{ident}"); + let mirror_static_method = format_ident!("mirror_static_{ident}"); quote! { #[func] @@ -343,6 +345,11 @@ fn generate_rust_methods(inputs: &[Input]) -> Vec { i } + #[func] + fn #panic_method(&self) -> #rust_ty { + panic!("intentional panic in `{}`", stringify!(#panic_method)); + } + #[func] fn #return_static_method() -> #rust_ty { #rust_val From 98477e94b59e28df35c86a97ddee5ed23b9eaf37 Mon Sep 17 00:00:00 2001 From: Jan Haller Date: Tue, 28 Oct 2025 12:21:23 +0100 Subject: [PATCH 2/2] Change ptrcalls to use Result instead of panics --- godot-core/src/builtin/callable.rs | 4 +- godot-core/src/meta/error/call_error.rs | 3 + godot-core/src/meta/param_tuple.rs | 5 +- godot-core/src/meta/param_tuple/impls.rs | 29 ++++---- godot-core/src/meta/signature.rs | 15 +++-- godot-core/src/private.rs | 78 +++++++++++----------- godot-macros/src/class/data_models/func.rs | 12 ++-- 7 files changed, 72 insertions(+), 74 deletions(-) diff --git a/godot-core/src/builtin/callable.rs b/godot-core/src/builtin/callable.rs index 2071ceb1a..9d174feff 100644 --- a/godot-core/src/builtin/callable.rs +++ b/godot-core/src/builtin/callable.rs @@ -677,7 +677,7 @@ mod custom_callable { let name = ""; let ctx = meta::CallContext::custom_callable(name); - crate::private::handle_varcall_panic(&ctx, &mut *r_error, move || { + crate::private::handle_fallible_varcall(&ctx, &mut *r_error, move || { // Get the RustCallable again inside closure so it doesn't have to be UnwindSafe. let c: &mut C = CallableUserdata::inner_from_raw(callable_userdata); let result = c.invoke(arg_refs); @@ -707,7 +707,7 @@ mod custom_callable { let name = ""; let ctx = meta::CallContext::custom_callable(name); - crate::private::handle_varcall_panic(&ctx, &mut *r_error, move || { + crate::private::handle_fallible_varcall(&ctx, &mut *r_error, move || { // Get the FnWrapper again inside closure so the FnMut doesn't have to be UnwindSafe. let w: &mut FnWrapper = CallableUserdata::inner_from_raw(callable_userdata); diff --git a/godot-core/src/meta/error/call_error.rs b/godot-core/src/meta/error/call_error.rs index 75adc654a..c8d8c36cc 100644 --- a/godot-core/src/meta/error/call_error.rs +++ b/godot-core/src/meta/error/call_error.rs @@ -16,6 +16,9 @@ use crate::meta::{CallContext, ToGodot}; use crate::private::PanicPayload; use crate::sys; +/// Result type for function calls that can fail. +pub(crate) type CallResult = Result; + /// Error capable of representing failed function calls. /// /// This type is returned from _varcall_ functions in the Godot API that begin with `try_` prefixes, diff --git a/godot-core/src/meta/param_tuple.rs b/godot-core/src/meta/param_tuple.rs index de1ab88b7..d2901aec7 100644 --- a/godot-core/src/meta/param_tuple.rs +++ b/godot-core/src/meta/param_tuple.rs @@ -7,8 +7,9 @@ use godot_ffi as sys; -use super::{CallContext, CallResult, PropertyInfo}; use crate::builtin::Variant; +use crate::meta::error::CallResult; +use crate::meta::{CallContext, PropertyInfo}; mod impls; @@ -64,7 +65,7 @@ pub trait InParamTuple: ParamTuple { args_ptr: *const sys::GDExtensionConstTypePtr, call_type: sys::PtrcallType, call_ctx: &CallContext, - ) -> Self; + ) -> CallResult; /// Converts `array` to `Self` by calling [`from_variant`](crate::meta::FromGodot::from_variant) on each argument. fn from_variant_array(array: &[&Variant]) -> Self; diff --git a/godot-core/src/meta/param_tuple/impls.rs b/godot-core/src/meta/param_tuple/impls.rs index 4c55ab35d..7d8e6605d 100644 --- a/godot-core/src/meta/param_tuple/impls.rs +++ b/godot-core/src/meta/param_tuple/impls.rs @@ -14,10 +14,10 @@ use godot_ffi as sys; use sys::GodotFfi; use crate::builtin::Variant; -use crate::meta::error::{CallError, ConvertError}; +use crate::meta::error::{CallError, CallResult}; use crate::meta::{ - signature, ArgPassing, CallContext, FromGodot, GodotConvert, GodotType, InParamTuple, - OutParamTuple, ParamTuple, ToGodot, + ArgPassing, CallContext, FromGodot, GodotConvert, GodotType, InParamTuple, OutParamTuple, + ParamTuple, ToGodot, }; macro_rules! count_idents { @@ -57,7 +57,7 @@ macro_rules! unsafe_impl_param_tuple { unsafe fn from_varcall_args( args_ptr: *const sys::GDExtensionConstVariantPtr, call_ctx: &crate::meta::CallContext, - ) -> signature::CallResult { + ) -> CallResult { let args = ( $( // SAFETY: `args_ptr` is an array with length `Self::LEN` and each element is a valid pointer, since they @@ -80,14 +80,16 @@ macro_rules! unsafe_impl_param_tuple { args_ptr: *const sys::GDExtensionConstTypePtr, call_type: sys::PtrcallType, call_ctx: &crate::meta::CallContext, - ) -> Self { - ( + ) -> CallResult { + let tuple = ( $( // SAFETY: `args_ptr` has length `Self::LEN` and `$n` is less than `Self::LEN`, and `args_ptr` must be an array whose // `$n`-th element is of type `$P`. - unsafe { ptrcall_arg::<$P, $n>(args_ptr, call_ctx, call_type) }, + unsafe { ptrcall_arg::<$P, $n>(args_ptr, call_ctx, call_type)? }, )* - ) + ); + + Ok(tuple) // If none of the `?` above were hit. } fn from_variant_array(array: &[&Variant]) -> Self { @@ -196,7 +198,7 @@ pub(super) unsafe fn ptrcall_arg( args_ptr: *const sys::GDExtensionConstTypePtr, call_ctx: &CallContext, call_type: sys::PtrcallType, -) -> P { +) -> CallResult

{ // SAFETY: It is safe to dereference `args_ptr` at `N`. let offset_ptr = unsafe { *args_ptr.offset(N) }; @@ -207,7 +209,7 @@ pub(super) unsafe fn ptrcall_arg( ::try_from_ffi(ffi) .and_then(P::try_from_godot) - .unwrap_or_else(|err| param_error::

(call_ctx, N as i32, err)) + .map_err(|err| CallError::failed_param_conversion::

(call_ctx, N, err)) } /// Converts `arg` into a value of type `P`. @@ -219,7 +221,7 @@ pub(super) unsafe fn varcall_arg( arg: sys::GDExtensionConstVariantPtr, call_ctx: &CallContext, param_index: isize, -) -> Result { +) -> CallResult

{ // SAFETY: It is safe to dereference `args_ptr` at `N` as a `Variant`. let variant_ref = unsafe { Variant::borrow_var_sys(arg) }; @@ -228,11 +230,6 @@ pub(super) unsafe fn varcall_arg( .map_err(|err| CallError::failed_param_conversion::

(call_ctx, param_index, err)) } -fn param_error

(call_ctx: &CallContext, index: i32, err: ConvertError) -> ! { - let param_ty = std::any::type_name::

(); - panic!("in function `{call_ctx}` at parameter [{index}] of type {param_ty}: {err}"); -} - fn assert_array_length(array: &[&Variant]) { assert_eq!( array.len(), diff --git a/godot-core/src/meta/signature.rs b/godot-core/src/meta/signature.rs index 4a9529deb..00844abf9 100644 --- a/godot-core/src/meta/signature.rs +++ b/godot-core/src/meta/signature.rs @@ -9,18 +9,17 @@ use std::borrow::Cow; use std::fmt; use std::marker::PhantomData; -use godot_ffi::{self as sys, GodotFfi}; +use godot_ffi as sys; +use sys::GodotFfi; use crate::builtin::Variant; -use crate::meta::error::{CallError, ConvertError}; +use crate::meta::error::{CallError, CallResult, ConvertError}; use crate::meta::{ FromGodot, GodotConvert, GodotType, InParamTuple, MethodParamOrReturnInfo, OutParamTuple, ParamTuple, ToGodot, }; use crate::obj::{GodotClass, ValidatedObject}; -pub(super) type CallResult = Result; - /// A full signature for a function. /// /// For in-calls (that is, calls from the Godot engine to Rust code) `Params` will implement [`InParamTuple`] and `Ret` @@ -101,19 +100,21 @@ where ret: sys::GDExtensionTypePtr, func: fn(sys::GDExtensionClassInstancePtr, Params) -> Ret, call_type: sys::PtrcallType, - ) { + ) -> CallResult<()> { // $crate::out!("in_ptrcall: {call_ctx}"); #[cfg(feature = "trace")] trace::push(true, true, call_ctx); // SAFETY: TODO. - let args = unsafe { Params::from_ptrcall_args(args_ptr, call_type, call_ctx) }; + let args = unsafe { Params::from_ptrcall_args(args_ptr, call_type, call_ctx)? }; // SAFETY: // `ret` is always a pointer to an initialized value of type $R // TODO: double-check the above - unsafe { ptrcall_return::(func(instance_ptr, args), ret, call_ctx, call_type) } + unsafe { ptrcall_return::(func(instance_ptr, args), ret, call_ctx, call_type) }; + + Ok(()) } } diff --git a/godot-core/src/private.rs b/godot-core/src/private.rs index 6e3148a96..d92a2d7c3 100644 --- a/godot-core/src/private.rs +++ b/godot-core/src/private.rs @@ -13,7 +13,7 @@ use std::sync::atomic; use sys::Global; use crate::global::godot_error; -use crate::meta::error::CallError; +use crate::meta::error::{CallError, CallResult}; use crate::meta::CallContext; use crate::obj::Gd; use crate::{classes, sys}; @@ -417,7 +417,7 @@ impl PanicPayload { /// /// 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, +/// In contrast to [`handle_fallible_varcall`] and [`handle_fallible_ptrcall`], 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 where @@ -440,59 +440,57 @@ where result } -// TODO(bromeon): make call_ctx lazy-evaluated (like error_ctx) everywhere; -// or make it eager everywhere and ensure it's cheaply constructed in the call sites. -pub fn handle_varcall_panic( +/// Invokes a function with the _varcall_ calling convention, handling both expected errors and user panics. +pub fn handle_fallible_varcall( call_ctx: &CallContext, out_err: &mut sys::GDExtensionCallError, code: F, ) where - F: FnOnce() -> Result + std::panic::UnwindSafe, + F: FnOnce() -> CallResult + std::panic::UnwindSafe, { - let outcome: Result, PanicPayload> = - handle_panic(|| call_ctx.to_string(), code); - - let call_error = match outcome { - // All good. - Ok(Ok(_result)) => return, - - // Call error signalled by Godot's or gdext's validation. - Ok(Err(err)) => err, - - // Panic occurred (typically through user): forward message. - Err(panic_msg) => CallError::failed_by_user_panic(call_ctx, panic_msg), - }; - - let error_id = report_call_error(call_error, true); - - // Abuse 'argument' field to store our ID. - *out_err = sys::GDExtensionCallError { - error: sys::GODOT_RUST_CUSTOM_CALL_ERROR, - argument: error_id, - expected: 0, + if let Some(error_id) = handle_fallible_call(call_ctx, code, true) { + // Abuse 'argument' field to store our ID. + *out_err = sys::GDExtensionCallError { + error: sys::GODOT_RUST_CUSTOM_CALL_ERROR, + argument: error_id, + expected: 0, + }; }; //sys::interface_fn!(variant_new_nil)(sys::AsUninit::as_uninit(ret)); } -pub fn handle_ptrcall_panic(call_ctx: &CallContext, code: F) +/// Invokes a function with the _ptrcall_ calling convention, handling both expected errors and user panics. +pub fn handle_fallible_ptrcall(call_ctx: &CallContext, code: F) where - F: FnOnce() -> R + std::panic::UnwindSafe, + F: FnOnce() -> CallResult<()> + std::panic::UnwindSafe, { - let outcome: Result = handle_panic(|| call_ctx.to_string(), code); + handle_fallible_call(call_ctx, code, false); +} + +/// Common error handling for fallible calls, handling detectable errors and user panics. +/// +/// Returns `None` if the call succeeded, or `Some(error_id)` if it failed. +/// +/// `track_globally` indicates whether the error should be stored as an index in the global error database (for varcall calls), to convey +/// out-of-band, godot-rust specific error information to the caller. +fn handle_fallible_call(call_ctx: &CallContext, code: F, track_globally: bool) -> Option +where + F: FnOnce() -> CallResult + std::panic::UnwindSafe, +{ + let outcome: Result, PanicPayload> = handle_panic(|| call_ctx.to_string(), code); let call_error = match outcome { // All good. - Ok(_result) => return, + Ok(Ok(_result)) => return None, - // Panic occurred (typically through user): forward message. - Err(payload) => CallError::failed_by_user_panic(call_ctx, payload), - }; + // Error from Godot or godot-rust validation (e.g. parameter conversion). + Ok(Err(err)) => err, - let _id = report_call_error(call_error, false); -} + // User panic occurred: forward message. + Err(panic_msg) => CallError::failed_by_user_panic(call_ctx, panic_msg), + }; -fn report_call_error(call_error: CallError, track_globally: bool) -> i32 { // Print failed calls to Godot's console. // TODO Level 1 is not yet set, so this will always print if level != 0. Needs better logic to recognize try_* calls and avoid printing. // But a bit tricky with multiple threads and re-entrancy; maybe pass in info in error struct. @@ -501,11 +499,13 @@ fn report_call_error(call_error: CallError, track_globally: bool) -> i32 { } // Once there is a way to auto-remove added errors, this could be always true. - if track_globally { + let error_id = if track_globally { call_error_insert(call_error) } else { 0 - } + }; + + Some(error_id) } // Currently unused; implemented due to temporary need and may come in handy. diff --git a/godot-macros/src/class/data_models/func.rs b/godot-macros/src/class/data_models/func.rs index 744707403..9d8601fee 100644 --- a/godot-macros/src/class/data_models/func.rs +++ b/godot-macros/src/class/data_models/func.rs @@ -86,7 +86,7 @@ pub fn make_virtual_callback( ret: sys::GDExtensionTypePtr, ) { let call_ctx = #call_ctx; - let _success = ::godot::private::handle_ptrcall_panic( + ::godot::private::handle_fallible_ptrcall( &call_ctx, || #invocation ); @@ -566,7 +566,7 @@ fn make_varcall_fn(call_ctx: &TokenStream, wrapped_method: &TokenStream) -> Toke err: *mut sys::GDExtensionCallError, ) { let call_ctx = #call_ctx; - ::godot::private::handle_varcall_panic( + ::godot::private::handle_fallible_varcall( &call_ctx, &mut *err, || #invocation @@ -587,14 +587,10 @@ fn make_ptrcall_fn(call_ctx: &TokenStream, wrapped_method: &TokenStream) -> Toke ret: sys::GDExtensionTypePtr, ) { let call_ctx = #call_ctx; - let _success = ::godot::private::handle_panic( - || format!("{call_ctx}"), + ::godot::private::handle_fallible_ptrcall( + &call_ctx, || #invocation ); - - // if success.is_err() { - // // TODO set return value to T::default()? - // } } } }