Merge pull request #1387 from godot-rust/qol/ptrcall-panics

Failed ptrcalls: instead of panic, print error + return default

Author: Bromeon

godot-core/src/builtin/callable.rs

@@ -677,7 +677,7 @@ mod custom_callable {
         let name = "<optimized out>";
         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 = "<optimized out>";
         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<F> = CallableUserdata::inner_from_raw(callable_userdata);
 

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<R> = Result<R, CallError>;
+
 /// Error capable of representing failed function calls.
 ///
 /// This type is returned from _varcall_ functions in the Godot API that begin with `try_` prefixes,

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<Self>;
 
     /// Converts `array` to `Self` by calling [`from_variant`](crate::meta::FromGodot::from_variant) on each argument.
     fn from_variant_array(array: &[&Variant]) -> Self;

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<Self> {
+            ) -> CallResult<Self> {
                 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<Self> {
+                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<P: FromGodot, const N: isize>(
     args_ptr: *const sys::GDExtensionConstTypePtr,
     call_ctx: &CallContext,
     call_type: sys::PtrcallType,
-) -> P {
+) -> CallResult<P> {
     // 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<P: FromGodot, const N: isize>(
 
     <P::Via as GodotType>::try_from_ffi(ffi)
         .and_then(P::try_from_godot)
-        .unwrap_or_else(|err| param_error::<P>(call_ctx, N as i32, err))
+        .map_err(|err| CallError::failed_param_conversion::<P>(call_ctx, N, err))
 }
 
 /// Converts `arg` into a value of type `P`.
@@ -219,7 +221,7 @@ pub(super) unsafe fn varcall_arg<P: FromGodot>(
     arg: sys::GDExtensionConstVariantPtr,
     call_ctx: &CallContext,
     param_index: isize,
-) -> Result<P, CallError> {
+) -> CallResult<P> {
     // 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<P: FromGodot>(
         .map_err(|err| CallError::failed_param_conversion::<P>(call_ctx, param_index, err))
 }
 
-fn param_error<P>(call_ctx: &CallContext, index: i32, err: ConvertError) -> ! {
-    let param_ty = std::any::type_name::<P>();
-    panic!("in function `{call_ctx}` at parameter [{index}] of type {param_ty}: {err}");
-}
-
 fn assert_array_length<P: ParamTuple>(array: &[&Variant]) {
     assert_eq!(
         array.len(),

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<R> = Result<R, CallError>;
-
 /// 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::<Ret>(func(instance_ptr, args), ret, call_ctx, call_type) }
+        unsafe { ptrcall_return::<Ret>(func(instance_ptr, args), ret, call_ctx, call_type) };
+
+        Ok(())
     }
 }
 

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<E, F, R>(error_context: E, code: F) -> Result<R, PanicPayload>
 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<F, R>(
+/// Invokes a function with the _varcall_ calling convention, handling both expected errors and user panics.
+pub fn handle_fallible_varcall<F, R>(
     call_ctx: &CallContext,
     out_err: &mut sys::GDExtensionCallError,
     code: F,
 ) where
-    F: FnOnce() -> Result<R, CallError> + std::panic::UnwindSafe,
+    F: FnOnce() -> CallResult<R> + std::panic::UnwindSafe,
 {
-    let outcome: Result<Result<R, CallError>, 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<F, R>(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<F>(call_ctx: &CallContext, code: F)
 where
-    F: FnOnce() -> R + std::panic::UnwindSafe,
+    F: FnOnce() -> CallResult<()> + std::panic::UnwindSafe,
 {
-    let outcome: Result<R, PanicPayload> = 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<F, R>(call_ctx: &CallContext, code: F, track_globally: bool) -> Option<i32>
+where
+    F: FnOnce() -> CallResult<R> + std::panic::UnwindSafe,
+{
+    let outcome: Result<CallResult<R>, 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.

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()?
-            // }
         }
     }
 }

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()

itest/rust/build.rs

@@ -320,12 +320,14 @@ fn generate_rust_methods(inputs: &[Input]) -> Vec<TokenStream> {
                 ..
             } = 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<TokenStream> {
                     i
                 }
 
+                #[func]
+                fn #panic_method(&self) -> #rust_ty {
+                    panic!("intentional panic in `{}`", stringify!(#panic_method));
+                }
+
                 #[func]
                 fn #return_static_method() -> #rust_ty {
                     #rust_val