From c7c99efe8b19931a308f82dfdcf9ecdb07071b75 Mon Sep 17 00:00:00 2001 From: Jan Haller Date: Tue, 27 Feb 2024 23:21:16 +0100 Subject: [PATCH 1/5] Add CallError + try_*() overload for fallible varcall methods --- .../src/generator/functions_common.rs | 74 +++- godot-codegen/src/models/domain.rs | 5 + godot-core/src/builtin/meta/call_error.rs | 326 ++++++++++++++++++ .../meta/godot_convert/convert_error.rs | 14 - godot-core/src/builtin/meta/mod.rs | 2 + godot-core/src/builtin/meta/signature.rs | 89 ++--- godot-core/src/builtin/variant/mod.rs | 18 +- godot-core/src/engine/script_instance.rs | 42 +-- godot-core/src/private.rs | 204 ++++++++--- godot-core/src/registry/mod.rs | 2 + godot-ffi/src/extras.rs | 5 +- godot-ffi/src/godot_ffi.rs | 35 -- godot-ffi/src/toolbox.rs | 26 ++ godot-macros/src/class/data_models/func.rs | 23 +- godot-macros/src/class/derive_godot_class.rs | 2 + itest/rust/src/framework/runner.rs | 6 +- .../src/object_tests/dynamic_call_test.rs | 131 +++++++ itest/rust/src/object_tests/mod.rs | 1 + itest/rust/src/object_tests/object_test.rs | 64 +--- 19 files changed, 787 insertions(+), 282 deletions(-) create mode 100644 godot-core/src/builtin/meta/call_error.rs create mode 100644 itest/rust/src/object_tests/dynamic_call_test.rs diff --git a/godot-codegen/src/generator/functions_common.rs b/godot-codegen/src/generator/functions_common.rs index dade4d776..bfb444022 100644 --- a/godot-codegen/src/generator/functions_common.rs +++ b/godot-codegen/src/generator/functions_common.rs @@ -101,7 +101,7 @@ pub fn make_function_definition( // to only use `unsafe` for pointers in parameters (for outbound calls), and in return values (for virtual calls). Or technically more // correct, make the entire trait unsafe as soon as one function can return pointers, but that's very unergonomic and non-local. // Thus, let's keep things simple and more conservative. - let (maybe_unsafe, safety_doc) = if let Some(safety_doc) = safety_doc { + let (maybe_unsafe, maybe_safety_doc) = if let Some(safety_doc) = safety_doc { (quote! { unsafe }, safety_doc) } else if function_uses_pointers(sig) { ( @@ -109,7 +109,8 @@ pub fn make_function_definition( quote! { /// # Safety /// - /// Godot currently does not document safety requirements on this method. Make sure you understand the underlying semantics. + /// This method has automatically been marked `unsafe` because it accepts raw pointers as parameters. + /// If Godot does not document any safety requirements, make sure you understand the underlying semantics. }, ) } else { @@ -143,7 +144,7 @@ pub fn make_function_definition( // Virtual functions quote! { - #safety_doc + #maybe_safety_doc #maybe_unsafe fn #primary_fn_name( #receiver_param #( #params, )* @@ -157,20 +158,61 @@ pub fn make_function_definition( // If the return type is not Variant, then convert to concrete target type let varcall_invocation = &code.varcall_invocation; - // TODO use Result instead of panic on error - quote! { - #safety_doc - #vis #maybe_unsafe fn #primary_fn_name( - #receiver_param - #( #params, )* - varargs: &[Variant] - ) #return_decl { - type CallSig = #call_sig; + // TODO Utility functions: update as well. + if code.receiver.param.is_empty() { + quote! { + #maybe_safety_doc + #vis #maybe_unsafe fn #primary_fn_name( + #receiver_param + #( #params, )* + varargs: &[Variant] + ) #return_decl { + type CallSig = #call_sig; + + let args = (#( #arg_names, )*); + + unsafe { + #varcall_invocation + } + } + } + } else { + let try_return_decl = &sig.return_value().call_result_decl(); + let try_fn_name = format_ident!("try_{}", rust_function_name_str); - let args = (#( #arg_names, )*); + // Note: all varargs functions are non-static, which is why there are some shortcuts in try_*() argument forwarding. + // This can be made more complex if ever necessary. - unsafe { - #varcall_invocation + quote! { + /// # Panics + /// This is a _varcall_ method, meaning parameters and return values are passed as `Variant`. + /// It can detect call failures and will panic in such a case. + #maybe_safety_doc + #vis #maybe_unsafe fn #primary_fn_name( + #receiver_param + #( #params, )* + varargs: &[Variant] + ) #return_decl { + Self::#try_fn_name(self, #( #arg_names, )* varargs) + .unwrap_or_else(|e| panic!("{e}")) + } + + /// # Return type + /// This is a _varcall_ method, meaning parameters and return values are passed as `Variant`. + /// It can detect call failures and will return `Err` in such a case. + #maybe_safety_doc + #vis #maybe_unsafe fn #try_fn_name( + #receiver_param + #( #params, )* + varargs: &[Variant] + ) #try_return_decl { + type CallSig = #call_sig; + + let args = (#( #arg_names, )*); + + unsafe { + #varcall_invocation + } } } } @@ -189,7 +231,7 @@ pub fn make_function_definition( }; quote! { - #safety_doc + #maybe_safety_doc #vis #maybe_unsafe fn #primary_fn_name( #receiver_param #( #params, )* diff --git a/godot-codegen/src/models/domain.rs b/godot-codegen/src/models/domain.rs index 04c15ae13..1f7b2e207 100644 --- a/godot-codegen/src/models/domain.rs +++ b/godot-codegen/src/models/domain.rs @@ -594,6 +594,11 @@ impl FnReturn { } } } + + pub fn call_result_decl(&self) -> TokenStream { + let ret = self.type_tokens(); + quote! { -> Result<#ret, crate::builtin::meta::CallError> } + } } // ---------------------------------------------------------------------------------------------------------------------------------------------- diff --git a/godot-core/src/builtin/meta/call_error.rs b/godot-core/src/builtin/meta/call_error.rs new file mode 100644 index 000000000..e0677bf20 --- /dev/null +++ b/godot-core/src/builtin/meta/call_error.rs @@ -0,0 +1,326 @@ +/* + * Copyright (c) godot-rust; Bromeon and contributors. + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at https://mozilla.org/MPL/2.0/. + */ + +use crate::builtin::meta::{CallContext, ConvertError, ToGodot}; +use crate::builtin::Variant; +use crate::sys; +use godot_ffi::{join_with, VariantType}; +use std::error::Error; +use std::fmt; + +/// Error capable of representing failed function calls. +/// +/// This type is returned from _varcall_ functions in the Godot API that begin with `try_` prefixes, +/// e.g. [`Object::try_call()`](crate::engine::Object::try_call) or [`Node::try_rpc()`](crate::engine::Node::try_rpc). +/// +/// Allows to inspect the involved class and method via `class_name()` and `method_name()`. Implements the `std::error::Error` trait, so +/// it comes with `Display` and `Error::source()` APIs. +/// +/// # Possible error causes +/// Several reasons can cause a function call to fail. The reason is described in the `Display` impl. +/// +/// - **Invalid method**: The method does not exist on the object. +/// - **Failed argument conversion**: The arguments passed to the method cannot be converted to the declared parameter types. +/// - **Failed return value conversion**: The return value of a dynamic method (`Variant`) cannot be converted to the expected return type. +/// - **Too many or too few arguments**: The number of arguments passed to the method does not match the number of parameters. +/// - **User panic**: A Rust method caused a panic. +/// +/// # Chained errors +/// Let's say you have this code, and you want to call the method dynamically with `Object::try_call()`. +/// +/// Then, the immediate `CallError` will refer to the `Object::try_call` method, and its source will refer to `MyClass::my_method` +/// (the actual method that failed). +/// ```no_run +/// use godot::prelude::*; +/// use std::error::Error; +/// # use godot_core::builtin::meta::CallError; +/// #[derive(GodotClass)] +/// # #[class(init)] +/// struct MyClass; +/// +/// #[godot_api] +/// impl MyClass { +/// #[func] +/// fn my_method(&self, arg: i64) {} +/// } +/// +/// fn some_method() { +/// let obj: Gd = MyClass::new_gd(); +/// +/// // Dynamic call. Note: forgot to pass the argument. +/// let result: Result = obj.try_call("my_method", &[]); +/// +/// // Get immediate and original errors. Note that source() can be None or have type ConvertError. +/// let outer: CallError = result.unwrap_err(); +/// let inner: CallError = outer.source().downcast_ref::().unwrap(); +/// } +#[derive(Debug)] +pub struct CallError { + class_name: String, + function_name: String, + call_expr: String, + reason: String, + source: Option, +} + +impl CallError { + // Naming: + // - check_* means possible failure -- Result<(), Self> is returned. + // - failed_* means definitive failure -- Self is returned. + + /// Name of the class/builtin whose method failed. **Not** the dynamic type. + /// + /// Returns `None` if this is a utility function (without a surrounding class/builtin). + /// + /// This is the static and not the dynamic type. For example, if you invoke `call()` on a `Gd`, you are effectively invoking + /// `Object::call()` (through `DerefMut`), and the class name will be `Object`. + pub fn class_name(&self) -> Option<&str> { + if self.class_name.is_empty() { + None + } else { + Some(&self.class_name) + } + } + + /// Name of the function or method that failed. + pub fn method_name(&self) -> &str { + &self.function_name + } + + /// Describes the error. + /// + /// This is the same as the `Display`/`ToString` repr, but without the prefix mentioning that this is a function call error, + /// and without any source error information. + fn message(&self, with_source: bool) -> String { + let Self { + call_expr, reason, .. + } = self; + + let reason_str = if reason.is_empty() { + String::new() + } else { + format!("\n Reason: {reason}") + }; + + // let source_str = if with_source { + // self.source() + // .map(|e| format!("\n Source: {}", e)) + // .unwrap_or_default() + // } else { + // String::new() + // }; + + let source_str = match &self.source { + Some(SourceError::Convert(e)) if with_source => format!("\n Source: {}", e), + Some(SourceError::Call(e)) if with_source => format!("\n Source: {}", e.message(true)), + _ => String::new(), + }; + + format!("{call_expr}{reason_str}{source_str}") + } + + /// Checks whether number of arguments matches the number of parameters. + pub(crate) fn check_arg_count( + call_ctx: &CallContext, + arg_count: i64, + param_count: i64, + ) -> Result<(), Self> { + // This will need to be adjusted once optional parameters are supported in #[func]. + if arg_count == param_count { + return Ok(()); + } + + let param_plural = plural(param_count); + let arg_plural = plural(arg_count); + + Err(Self::new( + call_ctx, + format!( + "function has {param_count} parameter{param_plural}, but received {arg_count} argument{arg_plural}" + ), + None, + )) + } + + /// Checks the Godot side of a varcall (low-level `sys::GDExtensionCallError`). + pub(crate) fn check_out_varcall( + call_ctx: &CallContext, + err: sys::GDExtensionCallError, + explicit_args: &[T], + varargs: &[Variant], + ) -> Result<(), Self> { + if err.error == sys::GDEXTENSION_CALL_OK { + return Ok(()); + } + + let mut arg_types = Vec::with_capacity(explicit_args.len() + varargs.len()); + arg_types.extend(explicit_args.iter().map(|arg| arg.to_variant().get_type())); + arg_types.extend(varargs.iter().map(Variant::get_type)); + + let explicit_args_str = join_args(explicit_args.iter().map(|arg| arg.to_variant())); + let vararg_str = if varargs.is_empty() { + String::new() + } else { + format!(", varargs {}", join_args(varargs.into_iter().cloned())) + }; + + let call_expr = format!("{call_ctx}({explicit_args_str}{vararg_str})"); + + // If the call error encodes an error generated by us, decode it. + let mut source_error = None; + if err.error == sys::GODOT_RUST_CUSTOM_CALL_ERROR { + source_error = crate::private::call_error_remove(&err) //. + .map(|e| SourceError::Call(Box::new(e))); + } + + Err(Self::failed_varcall_inner( + call_ctx, + call_expr, + err, + &arg_types, + source_error, + )) + } + + /// Returns an error for a failed parameter conversion. + pub(crate) fn failed_param_conversion

( + call_ctx: &CallContext, + param_index: isize, + convert_error: ConvertError, + ) -> Self { + let param_ty = std::any::type_name::

(); + + Self::new( + call_ctx, + format!("parameter [{param_index}] of type {param_ty} failed to convert to Variant; {convert_error}"), + Some(convert_error), + ) + } + + /// Returns an error for a failed return type conversion. + pub(crate) fn failed_return_conversion( + call_ctx: &CallContext, + convert_error: ConvertError, + ) -> Self { + let return_ty = std::any::type_name::(); + + Self::new( + call_ctx, + format!("return type {return_ty} failed to convert from Variant; {convert_error}"), + Some(convert_error), + ) + } + + fn failed_varcall_inner( + call_ctx: &CallContext, + call_expr: String, + err: sys::GDExtensionCallError, + arg_types: &[VariantType], + source: Option, + ) -> Self { + // This specializes on reflection-style calls, e.g. call(), rpc() etc. + // In these cases, varargs are the _actual_ arguments, with required args being metadata such as method name. + + debug_assert_ne!(err.error, sys::GDEXTENSION_CALL_OK); // already checked outside + + let sys::GDExtensionCallError { + error, + argument, + expected, + } = err; + + let argc = arg_types.len(); + let reason = match error { + sys::GDEXTENSION_CALL_ERROR_INVALID_METHOD => "method not found".to_string(), + sys::GDEXTENSION_CALL_ERROR_INVALID_ARGUMENT => { + let from = arg_types[argument as usize]; + let to = VariantType::from_sys(expected as sys::GDExtensionVariantType); + let i = argument + 1; + + format!("cannot convert argument #{i} from {from:?} to {to:?}") + } + sys::GDEXTENSION_CALL_ERROR_TOO_MANY_ARGUMENTS => { + format!("too many arguments; expected {argument}, but called with {argc}") + } + sys::GDEXTENSION_CALL_ERROR_TOO_FEW_ARGUMENTS => { + format!("too few arguments; expected {argument}, but called with {argc}") + } + sys::GDEXTENSION_CALL_ERROR_INSTANCE_IS_NULL => "instance is null".to_string(), + sys::GDEXTENSION_CALL_ERROR_METHOD_NOT_CONST => "method is not const".to_string(), // not handled in Godot + sys::GODOT_RUST_CUSTOM_CALL_ERROR => String::new(), + _ => format!("unknown reason (error code {error})"), + }; + + Self { + class_name: call_ctx.class_name.to_string(), + function_name: call_ctx.function_name.to_string(), + call_expr, + reason, + source, + } + } + + #[doc(hidden)] + pub fn failed_by_user_panic(call_ctx: &CallContext, reason: String) -> Self { + Self::new(call_ctx, reason, None) + } + + fn new(call_ctx: &CallContext, reason: String, source: Option) -> Self { + Self { + class_name: call_ctx.class_name.to_string(), + function_name: call_ctx.function_name.to_string(), + call_expr: format!("{call_ctx}()"), + reason, + source: source.map(|e| SourceError::Convert(Box::new(e))), + } + } +} + +impl fmt::Display for CallError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "godot-rust function call failed: {}", self.message(true)) + } +} + +impl Error for CallError { + fn source(&self) -> Option<&(dyn Error + 'static)> { + match self.source.as_ref() { + Some(SourceError::Convert(e)) => deref_to::(e), + Some(SourceError::Call(e)) => deref_to::(e), + None => None, + } + } +} + +// ---------------------------------------------------------------------------------------------------------------------------------------------- +// Implementation + +#[derive(Debug)] +enum SourceError { + Convert(Box), + Call(Box), +} + +/// Explicit dereferencing to a certain type. Avoids accidentally returning `&Box` or so. +fn deref_to(t: &T) -> Option<&(dyn Error + 'static)> +where + T: Error + 'static, +{ + Some(t) +} + +fn join_args(args: impl Iterator) -> String { + join_with(args, ", ", |arg| arg.brief_debug()) +} + +fn plural(count: i64) -> &'static str { + if count == 1 { + "" + } else { + "s" + } +} diff --git a/godot-core/src/builtin/meta/godot_convert/convert_error.rs b/godot-core/src/builtin/meta/godot_convert/convert_error.rs index 82d45eb36..ad0e33a8a 100644 --- a/godot-core/src/builtin/meta/godot_convert/convert_error.rs +++ b/godot-core/src/builtin/meta/godot_convert/convert_error.rs @@ -24,14 +24,6 @@ pub struct ConvertError { impl ConvertError { /// Create a new custom error for a conversion. - pub fn new() -> Self { - Self { - kind: ErrorKind::Custom, - cause: None, - value: None, - } - } - fn custom() -> Self { Self { kind: ErrorKind::Custom, @@ -88,12 +80,6 @@ impl ConvertError { } } -impl Default for ConvertError { - fn default() -> Self { - Self::new() - } -} - impl fmt::Display for ConvertError { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match (self.description(), self.cause.as_ref()) { diff --git a/godot-core/src/builtin/meta/mod.rs b/godot-core/src/builtin/meta/mod.rs index d56dffef1..ff5a5715b 100644 --- a/godot-core/src/builtin/meta/mod.rs +++ b/godot-core/src/builtin/meta/mod.rs @@ -7,11 +7,13 @@ pub mod registration; +mod call_error; mod class_name; mod godot_convert; mod return_marshal; mod signature; +pub use call_error::*; pub use class_name::*; pub use godot_convert::*; #[doc(hidden)] diff --git a/godot-core/src/builtin/meta/signature.rs b/godot-core/src/builtin/meta/signature.rs index b01aa5c71..16e6d8adf 100644 --- a/godot-core/src/builtin/meta/signature.rs +++ b/godot-core/src/builtin/meta/signature.rs @@ -36,7 +36,7 @@ pub trait VarcallSignatureTuple: PtrcallSignatureTuple { ret: sys::GDExtensionVariantPtr, err: *mut sys::GDExtensionCallError, func: fn(sys::GDExtensionClassInstancePtr, Self::Params) -> Self::Ret, - ); + ) -> Result<(), CallError>; unsafe fn out_class_varcall( method_bind: ClassMethodBind, @@ -47,7 +47,7 @@ pub trait VarcallSignatureTuple: PtrcallSignatureTuple { maybe_instance_id: Option, // if not static args: Self::Params, varargs: &[Variant], - ) -> Self::Ret; + ) -> Result; /// Outbound virtual call to a method overridden by a script attached to the object. /// @@ -181,16 +181,17 @@ macro_rules! impl_varcall_signature_for_tuple { ret: sys::GDExtensionVariantPtr, err: *mut sys::GDExtensionCallError, func: fn(sys::GDExtensionClassInstancePtr, Self::Params) -> Self::Ret, - ) { + ) -> Result<(), CallError> { //$crate::out!("in_varcall: {call_ctx}"); - check_arg_count(arg_count, $PARAM_COUNT, call_ctx); + CallError::check_arg_count(call_ctx, arg_count, $PARAM_COUNT)?; let args = ($( - unsafe { varcall_arg::<$Pn, $n>(args_ptr, call_ctx) }, + unsafe { varcall_arg::<$Pn, $n>(args_ptr, call_ctx)? }, )*) ; let rust_result = func(instance_ptr, args); - varcall_return::<$R>(rust_result, ret, err) + varcall_return::<$R>(rust_result, ret, err); + Ok(()) } #[inline] @@ -203,7 +204,7 @@ macro_rules! impl_varcall_signature_for_tuple { maybe_instance_id: Option, // if not static ($($pn,)*): Self::Params, varargs: &[Variant], - ) -> Self::Ret { + ) -> Result { let call_ctx = CallContext::outbound(class_name, method_name); //$crate::out!("out_class_varcall: {call_ctx}"); @@ -224,7 +225,7 @@ macro_rules! impl_varcall_signature_for_tuple { variant_ptrs.extend(explicit_args.iter().map(Variant::var_sys_const)); variant_ptrs.extend(varargs.iter().map(Variant::var_sys_const)); - let variant = Variant::from_var_sys_init(|return_ptr| { + let variant: Result = Variant::from_var_sys_init_result(|return_ptr| { let mut err = sys::default_call_error(); class_fn( method_bind.0, @@ -235,11 +236,15 @@ macro_rules! impl_varcall_signature_for_tuple { std::ptr::addr_of_mut!(err), ); - check_varcall_error(&err, &call_ctx, &explicit_args, varargs); + CallError::check_out_varcall(&call_ctx, err, &explicit_args, varargs) }); - let result = ::try_from_variant(&variant); - result.unwrap_or_else(|err| return_error::(&call_ctx, err)) + variant.and_then(|v| { + v.try_to::() + .or_else(|e| { + Err(CallError::failed_return_conversion::(&call_ctx, e)) + }) + }) } #[cfg(since_api = "4.3")] @@ -281,7 +286,7 @@ macro_rules! impl_varcall_signature_for_tuple { result.unwrap_or_else(|err| return_error::(&call_ctx, err)) } - // Note: this is doing a ptrcall, but uses variant conversions for it + // Note: this is doing a ptrcall, but uses variant conversions for it. #[inline] unsafe fn out_utility_ptrcall_varargs( utility_fn: UtilityFunctionBind, @@ -309,7 +314,6 @@ macro_rules! impl_varcall_signature_for_tuple { result.unwrap_or_else(|err| return_error::(&call_ctx, err)) } - #[inline] fn format_args(args: &Self::Params) -> String { let mut string = String::new(); @@ -464,11 +468,11 @@ macro_rules! impl_ptrcall_signature_for_tuple { unsafe fn varcall_arg( args_ptr: *const sys::GDExtensionConstVariantPtr, call_ctx: &CallContext, -) -> P { +) -> Result { let variant_ref = &*Variant::ptr_from_sys(*args_ptr.offset(N)); - let result = P::try_from_variant(variant_ref); - result.unwrap_or_else(|err| param_error::

(call_ctx, N as i32, err)) + P::try_from_variant(variant_ref) + .or_else(|err| Err(CallError::failed_param_conversion::

(call_ctx, N, err))) } /// Moves `ret_val` into `ret`. @@ -549,39 +553,6 @@ fn return_error(call_ctx: &CallContext, err: ConvertError) -> ! { panic!("in function `{call_ctx}` at return type {return_ty}: {err}"); } -fn check_varcall_error( - err: &sys::GDExtensionCallError, - call_ctx: &CallContext, - explicit_args: &[T], - varargs: &[Variant], -) where - T: Debug + ToGodot, -{ - if err.error == sys::GDEXTENSION_CALL_OK { - return; - } - - // TODO(optimize): split into non-generic, expensive parts after error check - - let mut arg_types = Vec::with_capacity(explicit_args.len() + varargs.len()); - arg_types.extend(explicit_args.iter().map(|arg| arg.to_variant().get_type())); - arg_types.extend(varargs.iter().map(Variant::get_type)); - - let explicit_args_str = join_to_string(explicit_args); - let vararg_str = join_to_string(varargs); - - let func_str = format!("{call_ctx}({explicit_args_str}; varargs {vararg_str})"); - - sys::panic_call_error(err, &func_str, &arg_types); -} - -fn join_to_string(list: &[T]) -> String { - list.iter() - .map(|v| format!("{v:?}")) - .collect::>() - .join(", ") -} - /// Helper trait to support `()` which doesn't implement `FromVariant`. trait FromVariantIndirect { fn convert(variant: Variant) -> Self; @@ -670,23 +641,3 @@ impl<'a> fmt::Display for CallContext<'a> { write!(f, "{}::{}", self.class_name, self.function_name) } } - -fn check_arg_count(arg_count: i64, param_count: i64, call_ctx: &CallContext) { - // This will need to be adjusted once optional parameters are supported in #[func]. - if arg_count != param_count { - let param_plural = plural(param_count); - let arg_plural = plural(arg_count); - panic!( - "{call_ctx} - function has {param_count} parameter{param_plural}, \ - but received {arg_count} argument{arg_plural}" - ); - } -} - -fn plural(count: i64) -> &'static str { - if count == 1 { - "" - } else { - "s" - } -} diff --git a/godot-core/src/builtin/variant/mod.rs b/godot-core/src/builtin/variant/mod.rs index a12a472f4..196dcddc8 100644 --- a/godot-core/src/builtin/variant/mod.rs +++ b/godot-core/src/builtin/variant/mod.rs @@ -228,7 +228,6 @@ impl Variant { } /// # Safety - /// /// See [`GodotFfi::from_sys_init`] and [`GodotFfi::from_sys_init_default`]. #[cfg(before_api = "4.1")] pub unsafe fn from_var_sys_init_or_init_default( @@ -238,7 +237,6 @@ impl Variant { } /// # Safety - /// /// See [`GodotFfi::from_sys_init`] and [`GodotFfi::from_sys_init_default`]. #[cfg(since_api = "4.1")] #[doc(hidden)] @@ -248,6 +246,22 @@ impl Variant { Self::from_var_sys_init(init_fn) } + /// # Safety + /// See [`GodotFfi::from_sys_init`]. + #[doc(hidden)] + pub unsafe fn from_var_sys_init_result( + init_fn: impl FnOnce(sys::GDExtensionUninitializedVariantPtr) -> Result<(), E>, + ) -> Result { + // Relies on current macro expansion of from_var_sys_init() having a certain implementation. + + let mut raw = std::mem::MaybeUninit::::uninit(); + + let var_uninit_ptr = + raw.as_mut_ptr() as ::Ptr; + + init_fn(var_uninit_ptr).map(|_err| Self::from_opaque(raw.assume_init())) + } + #[doc(hidden)] pub fn var_sys_const(&self) -> sys::GDExtensionConstVariantPtr { sys::to_const_ptr(self.var_sys()) diff --git a/godot-core/src/engine/script_instance.rs b/godot-core/src/engine/script_instance.rs index 8bbd38408..009bbae8d 100644 --- a/godot-core/src/engine/script_instance.rs +++ b/godot-core/src/engine/script_instance.rs @@ -486,10 +486,10 @@ mod script_instance_info { }); let result = match return_value { - Some(return_value) => return_value + Ok(return_value) => return_value .map(|variant| transfer_variant_to_godot(variant, r_ret)) .is_some(), - None => false, + Err(_) => false, }; transfer_bool_to_godot(result) @@ -606,13 +606,14 @@ mod script_instance_info { }); match result { - Some(Ok(ret)) => { + Ok(Ok(ret)) => { transfer_variant_to_godot(ret, r_return); (*r_error).error = sys::GDEXTENSION_CALL_OK; } - Some(Err(err)) => (*r_error).error = err, - None => { + Ok(Err(err)) => (*r_error).error = err, + + Err(_) => { (*r_error).error = sys::GDEXTENSION_CALL_ERROR_INVALID_METHOD; } }; @@ -635,8 +636,8 @@ mod script_instance_info { }); match script { - Some(script) => transfer_script_to_godot(&script), - None => std::ptr::null_mut(), + Ok(script) => transfer_script_to_godot(&script), + Err(_) => std::ptr::null_mut(), } } @@ -737,13 +738,13 @@ mod script_instance_info { borrow_instance(instance).get_property_type(name.clone()) }); - let Some(result) = result else { + if let Ok(result) = result { + *r_is_valid = transfer_bool_to_godot(true); + result.sys() + } else { *r_is_valid = transfer_bool_to_godot(false); - return 0; - }; - - *r_is_valid = transfer_bool_to_godot(true); - result.sys() + 0 + } } /// # Safety @@ -775,6 +776,7 @@ mod script_instance_info { Some(inner.to_string()) }) + .ok() .flatten(); let Some(string) = string else { @@ -829,11 +831,11 @@ mod script_instance_info { borrow_instance(instance).get_language() }); - let Some(language) = language else { - return std::ptr::null::() as sys::GDExtensionScriptLanguagePtr; - }; - - transfer_script_lang_to_godot(language) + if let Ok(language) = language { + transfer_script_lang_to_godot(language) + } else { + std::ptr::null::() as sys::GDExtensionScriptLanguagePtr + } } /// # Safety @@ -916,10 +918,10 @@ mod script_instance_info { }); let result = match return_value { - Some(return_value) => return_value + Ok(return_value) => return_value .map(|value| transfer_variant_to_godot(value, r_ret)) .is_some(), - None => false, + Err(_) => false, }; transfer_bool_to_godot(result) diff --git a/godot-core/src/private.rs b/godot-core/src/private.rs index a1b523e52..34ef545a8 100644 --- a/godot-core/src/private.rs +++ b/godot-core/src/private.rs @@ -5,6 +5,8 @@ * file, You can obtain one at https://mozilla.org/MPL/2.0/. */ +use godot_ffi::Global; +use std::collections::HashMap; use std::sync::{Arc, Mutex}; pub use crate::gen::classes::class_macros; @@ -12,24 +14,111 @@ pub use crate::registry::{callbacks, ClassPlugin, ErasedRegisterFn, PluginItem}; pub use crate::storage::{as_storage, Storage}; pub use sys::out; +use crate::builtin::meta::{CallContext, CallError}; use crate::{log, sys}; -// If someone forgets #[godot_api], this causes a compile error, rather than virtual functions not being called at runtime. -#[allow(non_camel_case_types)] -pub trait You_forgot_the_attribute__godot_api {} +// ---------------------------------------------------------------------------------------------------------------------------------------------- +// Global variables + +static CALL_ERRORS: Global = Global::default(); sys::plugin_registry!(pub __GODOT_PLUGIN_REGISTRY: ClassPlugin); +// ---------------------------------------------------------------------------------------------------------------------------------------------- +// Call error handling + +// Note: if this leads to many allocated IDs that are not removed, we could limit to 1 per thread-ID. +// Would need to check if re-entrant calls with multiple errors per thread are possible. +#[derive(Default)] +struct CallErrors { + map: HashMap, + next_id: i32, +} + +impl CallErrors { + fn insert(&mut self, err: CallError) -> i32 { + let id = self.next_id; + self.next_id = self.next_id.wrapping_add(1); + + self.map.insert(id, err); + id + } + + // Returns success or failure. + fn remove(&mut self, id: i32) -> Option { + self.map.remove(&id) + } +} + +fn call_error_insert(err: CallError, out_error: &mut sys::GDExtensionCallError) { + // Wraps around if entire i32 is depleted. If this happens in practice (unlikely, users need to deliberately ignore errors that are printed), + // we just overwrite oldest errors, should still work. + let id = CALL_ERRORS.lock().insert(err); + + // Abuse field to store our ID. + out_error.argument = id; +} + +pub(crate) fn call_error_remove(in_error: &sys::GDExtensionCallError) -> Option { + // Error checks are just quality-of-life diagnostic; do not throw panics if they fail. + + if in_error.error != sys::GODOT_RUST_CUSTOM_CALL_ERROR { + log::godot_error!("Tried to remove non-godot-rust call error {in_error:?}"); + return None; + } + + let call_error = CALL_ERRORS.lock().remove(in_error.argument); + if call_error.is_none() { + // Just a quality-of-life diagnostic; do not throw panics if something like this fails. + log::godot_error!("Failed to remove call error {in_error:?}"); + } + + call_error +} + +// ---------------------------------------------------------------------------------------------------------------------------------------------- +// Plugin handling + pub(crate) fn iterate_plugins(mut visitor: impl FnMut(&ClassPlugin)) { sys::plugin_foreach!(__GODOT_PLUGIN_REGISTRY; visitor); } +// ---------------------------------------------------------------------------------------------------------------------------------------------- +// Traits and types + +// If someone forgets #[godot_api], this causes a compile error, rather than virtual functions not being called at runtime. +#[allow(non_camel_case_types)] +pub trait You_forgot_the_attribute__godot_api {} + pub use crate::obj::rtti::ObjectRtti; pub struct ClassConfig { pub is_tool: bool, } +// ---------------------------------------------------------------------------------------------------------------------------------------------- +// Capability queries and internal access + +pub fn auto_init(l: &mut crate::obj::OnReady) { + l.init_auto(); +} + +#[cfg(since_api = "4.3")] +pub unsafe fn has_virtual_script_method( + object_ptr: sys::GDExtensionObjectPtr, + method_sname: sys::GDExtensionConstStringNamePtr, +) -> bool { + sys::interface_fn!(object_has_script_method)(sys::to_const_ptr(object_ptr), method_sname) != 0 +} + +pub fn flush_stdout() { + use std::io::Write; + std::io::stdout().flush().expect("flush stdout"); +} + +/// Ensure `T` is an editor plugin. +pub const fn is_editor_plugin>() {} + // Starting from 4.3, Godot has "runtime classes"; this emulation is no longer needed. #[cfg(before_api = "4.3")] pub fn is_class_inactive(is_tool: bool) -> bool { @@ -60,43 +149,80 @@ pub fn is_class_runtime(is_tool: bool) -> bool { global_config.tool_only_in_editor } -pub fn print_panic(err: Box) { +// ---------------------------------------------------------------------------------------------------------------------------------------------- +// Panic handling + +struct GodotPanicInfo { + line: u32, + file: String, + //backtrace: Backtrace, // for future use +} + +pub fn extract_panic_message(err: Box) -> String { if let Some(s) = err.downcast_ref::<&'static str>() { - print_panic_message(s); + s.to_string() } else if let Some(s) = err.downcast_ref::() { - print_panic_message(s.as_str()); + s.clone() } else { - log::godot_error!("Rust panic of type ID {:?}", err.type_id()); + format!("(panic of type ID {:?})", err.type_id()) } } -pub fn auto_init(l: &mut crate::obj::OnReady) { - l.init_auto(); -} - -fn print_panic_message(msg: &str) { +fn format_panic_message(msg: String) -> String { // If the message contains newlines, print all of the lines after a line break, and indent them. let lbegin = "\n "; let indented = msg.replace('\n', lbegin); if indented.len() != msg.len() { - log::godot_error!("Panic msg:{lbegin}{indented}"); + format!("Panic msg:{lbegin}{indented}") } else { - log::godot_error!("Panic msg: {msg}"); + format!("Panic msg: {msg}") } } -struct GodotPanicInfo { - line: u32, - file: String, - //backtrace: Backtrace, // for future use -} - /// Executes `code`. If a panic is thrown, it is caught and an error message is printed to Godot. /// -/// Returns `None` if a panic occurred, and `Some(result)` with the result of `code` otherwise. -#[must_use] -pub fn handle_panic(error_context: E, code: F) -> Option +/// Returns `Err(message)` if a panic occurred, and `Ok(result)` with the result of `code` otherwise. +pub fn handle_panic(error_context: E, code: F) -> Result +where + E: FnOnce() -> S, + F: FnOnce() -> R + std::panic::UnwindSafe, + S: std::fmt::Display, +{ + handle_panic_with_print(error_context, code, true) +} + +pub fn handle_varcall_panic( + call_ctx: CallContext, + out_err: &mut sys::GDExtensionCallError, + code: F, +) where + F: FnOnce() -> Result + std::panic::UnwindSafe, +{ + let outcome: Result, String> = + handle_panic_with_print(|| &call_ctx, code, false); + + 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), + }; + + // Print failed calls to Godot's console. + log::godot_error!("{call_error}"); + + out_err.error = sys::GODOT_RUST_CUSTOM_CALL_ERROR; + call_error_insert(call_error, out_err); + + //sys::interface_fn!(variant_new_nil)(sys::AsUninit::as_uninit(ret)); +} + +fn handle_panic_with_print(error_context: E, code: F, print: bool) -> Result where E: FnOnce() -> S, F: FnOnce() -> R + std::panic::UnwindSafe, @@ -126,7 +252,7 @@ where std::panic::set_hook(prev_hook); match panic { - Ok(result) => Some(result), + Ok(result) => Ok(result), Err(err) => { // Flush, to make sure previous Rust output (e.g. test announcement, or debug prints during app) have been printed // TODO write custom panic handler and move this there, before panic backtrace printing @@ -134,31 +260,23 @@ where let guard = info.lock().unwrap(); let info = guard.as_ref().expect("no panic info available"); + // log::godot_print!(""); log::godot_error!( - "Rust function panicked at {}:{}.\nContext: {}", + "Rust function panicked at {}:{}.\n Context: {}", info.file, info.line, error_context() ); //eprintln!("Backtrace:\n{}", info.backtrace); - print_panic(err); - None - } - } -} -#[cfg(since_api = "4.3")] -pub unsafe fn has_virtual_script_method( - object_ptr: sys::GDExtensionObjectPtr, - method_sname: sys::GDExtensionConstStringNamePtr, -) -> bool { - sys::interface_fn!(object_has_script_method)(sys::to_const_ptr(object_ptr), method_sname) != 0 -} + let msg = extract_panic_message(err); + let msg = format_panic_message(msg); -pub fn flush_stdout() { - use std::io::Write; - std::io::stdout().flush().expect("flush stdout"); -} + if print { + log::godot_error!("{msg}"); + } -/// Ensure `T` is an editor plugin. -pub const fn is_editor_plugin>() {} + Err(msg) + } + } +} diff --git a/godot-core/src/registry/mod.rs b/godot-core/src/registry/mod.rs index b967cc648..1b5650525 100644 --- a/godot-core/src/registry/mod.rs +++ b/godot-core/src/registry/mod.rs @@ -425,6 +425,8 @@ fn fill_class_info(item: PluginItem, c: &mut ClassRegistrationInfo) { #[cfg(before_api = "4.2")] assert!(generated_recreate_fn.is_none()); // not used + #[cfg(before_api = "4.3")] + let _ = is_tool; // mark used #[cfg(since_api = "4.3")] { c.godot_params.is_runtime = diff --git a/godot-ffi/src/extras.rs b/godot-ffi/src/extras.rs index 78712fe0c..d82b06247 100644 --- a/godot-ffi/src/extras.rs +++ b/godot-ffi/src/extras.rs @@ -59,7 +59,7 @@ impl_as_uninit!(GDExtensionTypePtr, GDExtensionUninitializedTypePtr); // Helper functions /// Differentiate from `sys::GDEXTENSION_CALL_ERROR_*` codes. -pub const GODOT_RUST_CALL_ERROR: GDExtensionCallErrorType = 40; +pub const GODOT_RUST_CUSTOM_CALL_ERROR: GDExtensionCallErrorType = 40; #[doc(hidden)] #[inline] @@ -71,6 +71,7 @@ pub fn default_call_error() -> GDExtensionCallError { } } +// TODO remove this, in favor of CallError #[doc(hidden)] #[inline] #[track_caller] // panic message points to call site @@ -108,7 +109,7 @@ pub fn panic_call_error( } GDEXTENSION_CALL_ERROR_INSTANCE_IS_NULL => "instance is null".to_string(), GDEXTENSION_CALL_ERROR_METHOD_NOT_CONST => "method is not const".to_string(), // not handled in Godot - GODOT_RUST_CALL_ERROR => "godot-rust function call failed".to_string(), + GODOT_RUST_CUSTOM_CALL_ERROR => "godot-rust function call failed".to_string(), _ => format!("unknown reason (error code {error})"), }; diff --git a/godot-ffi/src/godot_ffi.rs b/godot-ffi/src/godot_ffi.rs index 3f9fba75f..73d124f32 100644 --- a/godot-ffi/src/godot_ffi.rs +++ b/godot-ffi/src/godot_ffi.rs @@ -218,41 +218,6 @@ macro_rules! ffi_methods_one { } }; - // type $Ptr = Opaque - (OpaqueValue $Ptr:ty; $( #[$attr:meta] )? $vis:vis $from_sys:ident = from_sys) => { - $( #[$attr] )? $vis - unsafe fn $from_sys(ptr: $Ptr) -> Self { - let opaque = std::mem::transmute(ptr); - Self::from_opaque(opaque) - } - }; - (OpaqueValue $Ptr:ty; $( #[$attr:meta] )? $vis:vis $from_sys_init:ident = from_sys_init) => { - $( #[$attr] )? $vis - unsafe fn $from_sys_init(init: impl FnOnce(<$Ptr as $crate::AsUninit>::Ptr)) -> Self { - let mut raw = std::mem::MaybeUninit::uninit(); - init(std::mem::transmute(raw.as_mut_ptr())); - Self::from_opaque(raw.assume_init()) - } - }; - (OpaqueValue $Ptr:ty; $( #[$attr:meta] )? $vis:vis $sys:ident = sys) => { - $( #[$attr] )? $vis - fn $sys(&self) -> $Ptr { - unsafe { std::mem::transmute(self.opaque) } - } - }; - (OpaqueValue $Ptr:ty; $( #[$attr:meta] )? $vis:vis $from_arg_ptr:ident = from_arg_ptr) => { - $( #[$attr] )? $vis - unsafe fn $from_arg_ptr(ptr: $Ptr, _call_type: $crate::PtrcallType) -> Self { - Self::from_sys(ptr as *mut _) - } - }; - (OpaqueValue $Ptr:ty; $( #[$attr:meta] )? $vis:vis $move_return_ptr:ident = move_return_ptr) => { - $( #[$attr] )? $vis - unsafe fn $move_return_ptr(mut self, dst: $Ptr, _call_type: $crate::PtrcallType) { - std::ptr::swap(dst, std::mem::transmute::<_, $Ptr>(self.opaque)) - } - }; - // type $Ptr = *mut Self (SelfPtr $Ptr:ty; $( #[$attr:meta] )? $vis:vis $from_sys:ident = from_sys) => { $( #[$attr] )? $vis diff --git a/godot-ffi/src/toolbox.rs b/godot-ffi/src/toolbox.rs index bd5cc4ce4..39fddd80a 100644 --- a/godot-ffi/src/toolbox.rs +++ b/godot-ffi/src/toolbox.rs @@ -137,6 +137,32 @@ pub unsafe fn bitwise_equal(lhs: *const T, rhs: *const T) -> bool { == std::slice::from_raw_parts(rhs as *const u8, std::mem::size_of::()) } +pub fn join(iter: I) -> String +where + T: std::fmt::Display, + I: Iterator, +{ + join_with(iter, ", ", |item| format!("{item}")) +} + +pub fn join_with(mut iter: I, sep: &str, mut format_elem: F) -> String +where + I: Iterator, + F: FnMut(&T) -> String, +{ + let mut result = String::new(); + + if let Some(first) = iter.next() { + result.push_str(&format_elem(&first)); + for item in iter { + result.push_str(sep); + result.push_str(&format_elem(&item)); + } + } + + result +} + /* pub fn unqualified_type_name() -> &'static str { let type_name = std::any::type_name::(); diff --git a/godot-macros/src/class/data_models/func.rs b/godot-macros/src/class/data_models/func.rs index 1aa701703..3f74ae509 100644 --- a/godot-macros/src/class/data_models/func.rs +++ b/godot-macros/src/class/data_models/func.rs @@ -402,6 +402,7 @@ fn make_varcall_func( ) -> TokenStream { let invocation = make_varcall_invocation(call_ctx, sig_tuple, wrapped_method); + // TODO reduce amount of code generated, by delegating work to a library function. Could even be one that produces this function pointer. quote! { { unsafe extern "C" fn function( @@ -412,19 +413,11 @@ fn make_varcall_func( ret: sys::GDExtensionVariantPtr, err: *mut sys::GDExtensionCallError, ) { - let success = ::godot::private::handle_panic( - || #call_ctx, + ::godot::private::handle_varcall_panic( + #call_ctx, + &mut *err, || #invocation ); - - if success.is_none() { - // Signal error and set return type to Nil. - // None of the sys::GDEXTENSION_CALL_ERROR enums fits; so we use our own outside Godot's official range. - (*err).error = sys::GODOT_RUST_CALL_ERROR; - - // TODO(uninit) - sys::interface_fn!(variant_new_nil)(sys::AsUninit::as_uninit(ret)); - } } function @@ -448,14 +441,14 @@ fn make_ptrcall_func( args_ptr: *const sys::GDExtensionConstTypePtr, ret: sys::GDExtensionTypePtr, ) { - let success = ::godot::private::handle_panic( + let _success = ::godot::private::handle_panic( || #call_ctx, || #invocation ); - if success.is_none() { - // TODO set return value to T::default()? - } + // if success.is_err() { + // // TODO set return value to T::default()? + // } } function diff --git a/godot-macros/src/class/derive_godot_class.rs b/godot-macros/src/class/derive_godot_class.rs index ba2533ebd..92b567be8 100644 --- a/godot-macros/src/class/derive_godot_class.rs +++ b/godot-macros/src/class/derive_godot_class.rs @@ -330,6 +330,8 @@ fn parse_struct_attributes(class: &Struct) -> ParseResult { } // #[class(hidden)] + // TODO consider naming this "internal"; godot-cpp uses that terminology: + // https://github.com/godotengine/godot-cpp/blob/master/include/godot_cpp/core/class_db.hpp#L327 if let Some(span) = parser.handle_alone_with_span("hidden")? { require_api_version!("4.2", span, "#[class(hidden)]")?; is_hidden = true; diff --git a/itest/rust/src/framework/runner.rs b/itest/rust/src/framework/runner.rs index 32687ffd9..dd6737cac 100644 --- a/itest/rust/src/framework/runner.rs +++ b/itest/rust/src/framework/runner.rs @@ -184,7 +184,7 @@ impl IntegrationTests { // could not be caught, causing UB at the Godot FFI boundary (in practice, this will be a defined Godot crash with // stack trace though). godot_error!("GDScript test panicked"); - godot::private::print_panic(e); + godot::private::extract_panic_message(e); TestOutcome::Failed } }; @@ -310,9 +310,9 @@ fn run_rust_test(test: &RustTestCase, ctx: &TestContext) -> TestOutcome { // Explicit type to prevent tests from returning a value let err_context = || format!("itest `{}` failed", test.name); - let success: Option<()> = godot::private::handle_panic(err_context, || (test.function)(ctx)); + let success: Result<(), _> = godot::private::handle_panic(err_context, || (test.function)(ctx)); - TestOutcome::from_bool(success.is_some()) + TestOutcome::from_bool(success.is_ok()) } fn print_test_pre(test_case: &str, test_file: String, last_file: &mut Option, flush: bool) { diff --git a/itest/rust/src/object_tests/dynamic_call_test.rs b/itest/rust/src/object_tests/dynamic_call_test.rs new file mode 100644 index 000000000..a5b282c41 --- /dev/null +++ b/itest/rust/src/object_tests/dynamic_call_test.rs @@ -0,0 +1,131 @@ +/* + * Copyright (c) godot-rust; Bromeon and contributors. + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at https://mozilla.org/MPL/2.0/. + */ + +use godot::builtin::meta::{CallError, FromGodot, ToGodot}; +use godot::builtin::{StringName, Variant, Vector3}; +use godot::engine::{Node3D, Object}; +use godot::obj::{InstanceId, NewAlloc}; +use std::error::Error; + +use crate::framework::{expect_panic, itest}; +use crate::object_tests::object_test::ObjPayload; + +#[itest] +fn dynamic_call_no_args() { + let mut node = Node3D::new_alloc().upcast::(); + + let static_id = node.instance_id(); + let reflect_id_variant = node.call(StringName::from("get_instance_id"), &[]); + + let reflect_id = InstanceId::from_variant(&reflect_id_variant); + + assert_eq!(static_id, reflect_id); + node.free(); +} + +#[itest] +fn dynamic_call_with_args() { + let mut node = Node3D::new_alloc(); + + let expected_pos = Vector3::new(2.5, 6.42, -1.11); + + let none = node.call( + StringName::from("set_position"), + &[expected_pos.to_variant()], + ); + let actual_pos = node.call(StringName::from("get_position"), &[]); + + assert_eq!(none, Variant::nil()); + assert_eq!(actual_pos, expected_pos.to_variant()); + node.free(); +} + +#[itest] +fn dynamic_call_with_too_few_args() { + let mut obj = ObjPayload::new_alloc(); + + // Use panicking version. + expect_panic("call with too few arguments", || { + obj.call("take_1_int".into(), &[]); + }); + + // Use Result-based version. + let call_error = obj + .try_call("take_1_int".into(), &[]) + .expect_err("expected failed call"); + + // User-facing method to which error was propagated. + assert_eq!(call_error.class_name(), Some("Object")); + assert_eq!(call_error.method_name(), "call"); + assert_eq!( + call_error.to_string(), + "godot-rust function call failed: Object::call(&\"take_1_int\")\ + \n Source: ObjPayload::take_1_int()\ + \n Reason: function has 1 parameter, but received 0 arguments" + ); + + // Method where error originated (this is not repeated in all tests, the logic for chaining is the same). + let source = call_error.source().expect("must have source CallError"); + assert_eq!( + source.to_string(), + "godot-rust function call failed: ObjPayload::take_1_int()\ + \n Reason: function has 1 parameter, but received 0 arguments" + ); + + let source = source + .downcast_ref::() + .expect("source must be CallError"); + assert_eq!(source.class_name(), Some("ObjPayload")); + assert_eq!(source.method_name(), "take_1_int"); + + obj.free(); +} + +#[itest] +fn dynamic_call_with_too_many_args() { + let mut obj = ObjPayload::new_alloc(); + + // Use panicking version. + expect_panic("call with too many arguments", || { + obj.call("take_1_int".into(), &[42.to_variant(), 43.to_variant()]); + }); + + // Use Result-based version. + let call_error = obj + .try_call("take_1_int".into(), &[42.to_variant(), 43.to_variant()]) + .expect_err("expected failed call"); + + assert_eq!(call_error.class_name(), Some("Object")); + assert_eq!(call_error.method_name(), "call"); + assert_eq!( + call_error.to_string(), + "godot-rust function call failed: Object::call(&\"take_1_int\", varargs 42, 43)\ + \n Source: ObjPayload::take_1_int()\ + \n Reason: function has 1 parameter, but received 2 arguments" + ); + + obj.free(); +} + +#[itest] +fn dynamic_call_with_panic() { + let mut obj = ObjPayload::new_alloc(); + + let result = obj.try_call("do_panic".into(), &[]); + let call_error = result.expect_err("panic should cause a call error"); + + assert_eq!(call_error.class_name(), Some("Object")); + assert_eq!(call_error.method_name(), "call"); + assert_eq!( + call_error.to_string(), + "godot-rust function call failed: Object::call(&\"do_panic\")\ + \n Source: ObjPayload::do_panic()\ + \n Reason: Panic msg: do_panic exploded" + ); + + obj.free(); +} diff --git a/itest/rust/src/object_tests/mod.rs b/itest/rust/src/object_tests/mod.rs index 94d4c8c65..1e9300813 100644 --- a/itest/rust/src/object_tests/mod.rs +++ b/itest/rust/src/object_tests/mod.rs @@ -7,6 +7,7 @@ mod base_test; mod class_rename_test; +mod dynamic_call_test; mod object_swap_test; mod object_test; mod onready_test; diff --git a/itest/rust/src/object_tests/object_test.rs b/itest/rust/src/object_tests/object_test.rs index 21a119601..bf7b7dac1 100644 --- a/itest/rust/src/object_tests/object_test.rs +++ b/itest/rust/src/object_tests/object_test.rs @@ -801,68 +801,6 @@ fn object_user_share_drop() { assert_eq!(*drop_count.borrow(), 1); } -#[itest] -fn object_call_no_args() { - let mut node = Node3D::new_alloc().upcast::(); - - let static_id = node.instance_id(); - let reflect_id_variant = node.call(StringName::from("get_instance_id"), &[]); - - let reflect_id = InstanceId::from_variant(&reflect_id_variant); - - assert_eq!(static_id, reflect_id); - node.free(); -} - -#[itest] -fn object_call_with_args() { - let mut node = Node3D::new_alloc(); - - let expected_pos = Vector3::new(2.5, 6.42, -1.11); - - let none = node.call( - StringName::from("set_position"), - &[expected_pos.to_variant()], - ); - let actual_pos = node.call(StringName::from("get_position"), &[]); - - assert_eq!(none, Variant::nil()); - assert_eq!(actual_pos, expected_pos.to_variant()); - node.free(); -} - -#[itest] -fn object_call_with_too_few_args() { - let mut obj = ObjPayload::new_alloc(); - - expect_panic("call with too few arguments", || { - obj.call("take_1_int".into(), &[]); - }); - - obj.free(); -} - -#[itest] -fn object_call_with_too_many_args() { - let mut obj = ObjPayload::new_alloc(); - - expect_panic("call with too many arguments", || { - obj.call("take_1_int".into(), &[42.to_variant(), 43.to_variant()]); - }); - - obj.free(); -} - -#[itest(skip)] // Not yet implemented. -fn object_call_panic_is_nil() { - let mut obj = ObjPayload::new_alloc(); - - let result = obj.call("do_panic".into(), &[]); - assert_eq!(result, Variant::nil()); - - obj.free(); -} - #[itest] fn object_get_scene_tree(ctx: &TestContext) { let node = Node3D::new_alloc(); @@ -892,7 +830,7 @@ impl ObjPayload { #[func] fn do_panic(&self) { - panic!("panic from Rust"); + panic!("do_panic exploded"); } } From e9029d3d81f34b1d34c701ac8a21ad6bbe9cd017 Mon Sep 17 00:00:00 2001 From: Jan Haller Date: Tue, 27 Feb 2024 23:21:23 +0100 Subject: [PATCH 2/5] Disable printing of most panics/errors in tests --- godot-core/src/obj/gd.rs | 8 +++--- godot-core/src/private.rs | 44 ++++++++++++++++++++++++--------- itest/rust/src/framework/mod.rs | 6 +++-- 3 files changed, 42 insertions(+), 16 deletions(-) diff --git a/godot-core/src/obj/gd.rs b/godot-core/src/obj/gd.rs index 8c142427f..f9f43f63e 100644 --- a/godot-core/src/obj/gd.rs +++ b/godot-core/src/obj/gd.rs @@ -487,9 +487,11 @@ where let is_panic_unwind = std::thread::panicking(); let error_or_panic = |msg: String| { if is_panic_unwind { - crate::godot_error!( - "Encountered 2nd panic in free() during panic unwind; will skip destruction:\n{msg}" - ); + if crate::private::has_error_print_level(1) { + crate::godot_error!( + "Encountered 2nd panic in free() during panic unwind; will skip destruction:\n{msg}" + ); + } } else { panic!("{}", msg); } diff --git a/godot-core/src/private.rs b/godot-core/src/private.rs index 34ef545a8..a958a36e7 100644 --- a/godot-core/src/private.rs +++ b/godot-core/src/private.rs @@ -7,7 +7,7 @@ use godot_ffi::Global; use std::collections::HashMap; -use std::sync::{Arc, Mutex}; +use std::sync::{atomic, Arc, Mutex}; pub use crate::gen::classes::class_macros; pub use crate::registry::{callbacks, ClassPlugin, ErasedRegisterFn, PluginItem}; @@ -22,6 +22,12 @@ use crate::{log, sys}; static CALL_ERRORS: Global = Global::default(); +/// Level: +/// - 0: no error printing (during `expect_panic` in test) +/// - 1: not yet implemented, but intended for `try_` function calls (which are expected to fail, so error is annoying) +/// - 2: normal printing +static ERROR_PRINT_LEVEL: atomic::AtomicU8 = atomic::AtomicU8::new(2); + sys::plugin_registry!(pub __GODOT_PLUGIN_REGISTRY: ClassPlugin); // ---------------------------------------------------------------------------------------------------------------------------------------------- @@ -180,6 +186,16 @@ fn format_panic_message(msg: String) -> String { } } +pub fn set_error_print_level(level: u8) -> u8 { + assert!(level <= 2); + ERROR_PRINT_LEVEL.swap(level, atomic::Ordering::Relaxed) +} + +pub(crate) fn has_error_print_level(level: u8) -> bool { + assert!(level <= 2); + ERROR_PRINT_LEVEL.load(atomic::Ordering::Relaxed) >= level +} + /// 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. @@ -189,7 +205,7 @@ where F: FnOnce() -> R + std::panic::UnwindSafe, S: std::fmt::Display, { - handle_panic_with_print(error_context, code, true) + handle_panic_with_print(error_context, code, has_error_print_level(1)) } pub fn handle_varcall_panic( @@ -214,7 +230,11 @@ pub fn handle_varcall_panic( }; // Print failed calls to Godot's console. - log::godot_error!("{call_error}"); + // 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. + if has_error_print_level(2) { + log::godot_error!("{call_error}"); + } out_err.error = sys::GODOT_RUST_CUSTOM_CALL_ERROR; call_error_insert(call_error, out_err); @@ -260,14 +280,16 @@ where let guard = info.lock().unwrap(); let info = guard.as_ref().expect("no panic info available"); - // log::godot_print!(""); - log::godot_error!( - "Rust function panicked at {}:{}.\n Context: {}", - info.file, - info.line, - error_context() - ); - //eprintln!("Backtrace:\n{}", info.backtrace); + + if print { + log::godot_error!( + "Rust function panicked at {}:{}.\n Context: {}", + info.file, + info.line, + error_context() + ); + //eprintln!("Backtrace:\n{}", info.backtrace); + } let msg = extract_panic_message(err); let msg = format_panic_message(msg); diff --git a/itest/rust/src/framework/mod.rs b/itest/rust/src/framework/mod.rs index daff0561b..9475180cf 100644 --- a/itest/rust/src/framework/mod.rs +++ b/itest/rust/src/framework/mod.rs @@ -106,16 +106,18 @@ pub fn passes_filter(filters: &[String], test_name: &str) -> bool { pub fn expect_panic(context: &str, code: impl FnOnce()) { use std::panic; - // Exchange panic hook, to disable printing during expected panics + // Exchange panic hook, to disable printing during expected panics. Also disable gdext's panic printing. let prev_hook = panic::take_hook(); panic::set_hook(Box::new(|_panic_info| {})); + let prev_print_level = godot::private::set_error_print_level(0); // Generally, types should be unwind safe, and this helps ergonomics in testing (especially around &mut in expect_panic closures). let code = panic::AssertUnwindSafe(code); - // Run code that should panic, restore hook + // Run code that should panic, restore hook + gdext panic printing. let panic = panic::catch_unwind(code); panic::set_hook(prev_hook); + godot::private::set_error_print_level(prev_print_level); assert!( panic.is_err(), From 051ad48c7407fa960dd842fdee1b4fb149f2b405 Mon Sep 17 00:00:00 2001 From: Jan Haller Date: Tue, 27 Feb 2024 23:21:30 +0100 Subject: [PATCH 3/5] Make Variant's Debug impl more concise Previously rendered as Variant(ty=..., val=...). Now dispatches on the value and uses its `Debug` impl instead. This should still be unambiguous, e.g. StringName/NodePath have &"..." and ^"..." syntax to differentiate them from GString. --- godot-codegen/src/generator/central_files.rs | 20 ++++++++++++++------ godot-core/src/builtin/meta/call_error.rs | 4 ++-- godot-core/src/builtin/variant/mod.rs | 8 +++----- godot-ffi/src/toolbox.rs | 8 ++++++++ 4 files changed, 27 insertions(+), 13 deletions(-) diff --git a/godot-codegen/src/generator/central_files.rs b/godot-codegen/src/generator/central_files.rs index 15418456d..3af3a66bd 100644 --- a/godot-codegen/src/generator/central_files.rs +++ b/godot-codegen/src/generator/central_files.rs @@ -126,18 +126,26 @@ pub fn make_core_central_code(api: &ExtensionApi, ctx: &mut Context) -> TokenStr )* } - #[cfg(FALSE)] - impl FromVariant for VariantDispatch { - fn try_from_variant(variant: &Variant) -> Result { - let dispatch = match variant.get_type() { + impl VariantDispatch { + pub fn from_variant(variant: &Variant) -> Self { + match variant.get_type() { VariantType::Nil => Self::Nil, #( VariantType::#variant_ty_enumerators_pascal => Self::#variant_ty_enumerators_pascal(variant.to::<#variant_ty_enumerators_rust>()), )* - }; + } + } + } - Ok(dispatch) + impl std::fmt::Debug for VariantDispatch { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::Nil => write!(f, "null"), + #( + Self::#variant_ty_enumerators_pascal(v) => write!(f, "{v:?}"), + )* + } } } diff --git a/godot-core/src/builtin/meta/call_error.rs b/godot-core/src/builtin/meta/call_error.rs index e0677bf20..8584e2c66 100644 --- a/godot-core/src/builtin/meta/call_error.rs +++ b/godot-core/src/builtin/meta/call_error.rs @@ -8,7 +8,7 @@ use crate::builtin::meta::{CallContext, ConvertError, ToGodot}; use crate::builtin::Variant; use crate::sys; -use godot_ffi::{join_with, VariantType}; +use godot_ffi::{join_debug, VariantType}; use std::error::Error; use std::fmt; @@ -314,7 +314,7 @@ where } fn join_args(args: impl Iterator) -> String { - join_with(args, ", ", |arg| arg.brief_debug()) + join_debug(args) } fn plural(count: i64) -> &'static str { diff --git a/godot-core/src/builtin/variant/mod.rs b/godot-core/src/builtin/variant/mod.rs index 196dcddc8..d85012ed8 100644 --- a/godot-core/src/builtin/variant/mod.rs +++ b/godot-core/src/builtin/variant/mod.rs @@ -5,7 +5,9 @@ * file, You can obtain one at https://mozilla.org/MPL/2.0/. */ +use crate::builtin::meta::{impl_godot_as_self, ConvertError, FromGodot, ToGodot}; use crate::builtin::{GString, StringName}; +use crate::gen::central::VariantDispatch; use godot_ffi as sys; use std::{fmt, ptr}; use sys::types::OpaqueVariant; @@ -15,8 +17,6 @@ mod impls; pub use sys::{VariantOperator, VariantType}; -use super::meta::{impl_godot_as_self, ConvertError, FromGodot, ToGodot}; - /// Godot variant type, able to store a variety of different types. /// /// While Godot variants do not appear very frequently in Rust due to their lack of compile-time type-safety, they are central to all sorts of @@ -375,8 +375,6 @@ impl fmt::Display for Variant { impl fmt::Debug for Variant { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - let ty = self.get_type(); - let val = self.stringify(); - write!(f, "Variant(ty={ty:?}, val={val})") + VariantDispatch::from_variant(self).fmt(f) } } diff --git a/godot-ffi/src/toolbox.rs b/godot-ffi/src/toolbox.rs index 39fddd80a..5bd39f790 100644 --- a/godot-ffi/src/toolbox.rs +++ b/godot-ffi/src/toolbox.rs @@ -145,6 +145,14 @@ where join_with(iter, ", ", |item| format!("{item}")) } +pub fn join_debug(iter: I) -> String +where + T: std::fmt::Debug, + I: Iterator, +{ + join_with(iter, ", ", |item| format!("{item:?}")) +} + pub fn join_with(mut iter: I, sep: &str, mut format_elem: F) -> String where I: Iterator, From 8fada9fe35388be75775fd1ec28f3d88fd9a17ef Mon Sep 17 00:00:00 2001 From: Jan Haller Date: Tue, 27 Feb 2024 23:21:39 +0100 Subject: [PATCH 4/5] Improve error rendering of ConvertError/CallError --- godot-core/src/builtin/meta/call_error.rs | 4 +- .../meta/godot_convert/convert_error.rs | 48 ++++++++----------- godot-macros/src/derive/derive_from_godot.rs | 6 ++- 3 files changed, 27 insertions(+), 31 deletions(-) diff --git a/godot-core/src/builtin/meta/call_error.rs b/godot-core/src/builtin/meta/call_error.rs index 8584e2c66..a01ff9f0b 100644 --- a/godot-core/src/builtin/meta/call_error.rs +++ b/godot-core/src/builtin/meta/call_error.rs @@ -196,7 +196,7 @@ impl CallError { Self::new( call_ctx, - format!("parameter [{param_index}] of type {param_ty} failed to convert to Variant; {convert_error}"), + format!("parameter [{param_index}] of type {param_ty} failed conversion"), Some(convert_error), ) } @@ -210,7 +210,7 @@ impl CallError { Self::new( call_ctx, - format!("return type {return_ty} failed to convert from Variant; {convert_error}"), + format!("return value {return_ty} failed conversion"), Some(convert_error), ) } diff --git a/godot-core/src/builtin/meta/godot_convert/convert_error.rs b/godot-core/src/builtin/meta/godot_convert/convert_error.rs index ad0e33a8a..897032fc1 100644 --- a/godot-core/src/builtin/meta/godot_convert/convert_error.rs +++ b/godot-core/src/builtin/meta/godot_convert/convert_error.rs @@ -19,30 +19,35 @@ type Cause = Box; pub struct ConvertError { kind: ErrorKind, cause: Option, - value: Option, + value_str: Option, } impl ConvertError { + // Constructors are private (or hidden) as only the library or its proc-macros should construct this type. + /// Create a new custom error for a conversion. fn custom() -> Self { Self { kind: ErrorKind::Custom, cause: None, - value: None, + value_str: None, } } /// Create a new custom error for a conversion with the value that failed to convert. - pub fn with_value(value: V) -> Self + pub(crate) fn with_kind_value(kind: ErrorKind, value: V) -> Self where V: fmt::Debug, { - let mut err = Self::custom(); - err.value = Some(format!("{value:?}")); - err + Self { + kind, + cause: None, + value_str: Some(format!("{value:?}")), + } } /// Create a new custom error with a rust-error as an underlying cause for the conversion error. + #[doc(hidden)] pub fn with_cause(cause: C) -> Self where C: Into, @@ -54,6 +59,7 @@ impl ConvertError { /// Create a new custom error with a rust-error as an underlying cause for the conversion error, and the /// value that failed to convert. + #[doc(hidden)] pub fn with_cause_value(cause: C, value: V) -> Self where C: Into, @@ -61,7 +67,7 @@ impl ConvertError { { let mut err = Self::custom(); err.cause = Some(cause.into()); - err.value = Some(format!("{value:?}")); + err.value_str = Some(format!("{value:?}")); err } @@ -72,7 +78,7 @@ impl ConvertError { /// Returns a string representation of the value that failed to convert, if one exists. pub fn value_str(&self) -> Option<&str> { - self.value.as_deref() + self.value_str.as_deref() } fn description(&self) -> Option { @@ -89,8 +95,8 @@ impl fmt::Display for ConvertError { (None, None) => write!(f, "unknown error: {:?}", self.kind)?, } - if let Some(value) = self.value.as_ref() { - write!(f, "\n\tvalue: `{value:?}`")?; + if let Some(value) = self.value_str.as_ref() { + write!(f, ": {value}")?; } Ok(()) @@ -141,11 +147,7 @@ impl FromGodotError { where V: fmt::Debug, { - ConvertError { - kind: ErrorKind::FromGodot(self), - cause: None, - value: Some(format!("{value:?}")), - } + ConvertError::with_kind_value(ErrorKind::FromGodot(self), value) } fn description(&self) -> String { @@ -203,11 +205,7 @@ impl FromFfiError { where V: fmt::Debug, { - ConvertError { - kind: ErrorKind::FromFfi(self), - cause: None, - value: Some(format!("{value:?}")), - } + ConvertError::with_kind_value(ErrorKind::FromFfi(self), value) } fn description(&self) -> String { @@ -244,20 +242,16 @@ impl FromVariantError { where V: fmt::Debug, { - ConvertError { - kind: ErrorKind::FromVariant(self), - cause: None, - value: Some(format!("{value:?}")), - } + ConvertError::with_kind_value(ErrorKind::FromVariant(self), value) } fn description(&self) -> String { match self { Self::BadType { expected, got } => { - format!("expected Variant of type `{expected:?}` but got Variant of type `{got:?}`") + format!("Variant type mismatch -- expected `{expected:?}` but got `{got:?}`") } Self::WrongClass { expected } => { - format!("expected class `{expected}`, got variant with wrong class") + format!("Variant class mismatch -- expected `{expected}`") } } } diff --git a/godot-macros/src/derive/derive_from_godot.rs b/godot-macros/src/derive/derive_from_godot.rs index 598ce0401..ebaeb6de4 100644 --- a/godot-macros/src/derive/derive_from_godot.rs +++ b/godot-macros/src/derive/derive_from_godot.rs @@ -61,7 +61,8 @@ fn make_fromgodot_for_int_enum(name: &Ident, enum_: &CStyleEnum, int: &Ident) -> #( #discriminants => Ok(#name::#names), )* - other => Err(::godot::builtin::meta::ConvertError::with_cause_value(#bad_variant_error, other)) + // Pass `via` and not `other`, to retain debug info of original type. + other => Err(::godot::builtin::meta::ConvertError::with_cause_value(#bad_variant_error, via)) } } } @@ -81,7 +82,8 @@ fn make_fromgodot_for_gstring_enum(name: &Ident, enum_: &CStyleEnum) -> TokenStr #( #names_str => Ok(#name::#names), )* - other => Err(::godot::builtin::meta::ConvertError::with_cause_value(#bad_variant_error, other)) + // Pass `via` and not `other`, to retain debug info of original type. + other => Err(::godot::builtin::meta::ConvertError::with_cause_value(#bad_variant_error, via)) } } } From c4f16c7f667c3c3b9663b5103b3e66f02c434407 Mon Sep 17 00:00:00 2001 From: Jan Haller Date: Tue, 27 Feb 2024 23:21:47 +0100 Subject: [PATCH 5/5] More test cases for dynamic calls; tweak error messages --- godot-core/src/builtin/array.rs | 2 +- godot-core/src/builtin/meta/call_error.rs | 217 ++++++++++++------ .../meta/godot_convert/convert_error.rs | 9 +- godot-core/src/builtin/meta/signature.rs | 8 +- godot-core/src/builtin/variant/impls.rs | 4 +- godot-core/src/log.rs | 17 +- godot-core/src/private.rs | 5 +- .../src/object_tests/dynamic_call_test.rs | 159 ++++++++++++- 8 files changed, 326 insertions(+), 95 deletions(-) diff --git a/godot-core/src/builtin/array.rs b/godot-core/src/builtin/array.rs index 05291f317..acc8cdf3f 100644 --- a/godot-core/src/builtin/array.rs +++ b/godot-core/src/builtin/array.rs @@ -948,7 +948,7 @@ impl GodotFfiVariant for Array { if variant.get_type() != Self::variant_type() { return Err(FromVariantError::BadType { expected: Self::variant_type(), - got: variant.get_type(), + actual: variant.get_type(), } .into_error(variant)); } diff --git a/godot-core/src/builtin/meta/call_error.rs b/godot-core/src/builtin/meta/call_error.rs index a01ff9f0b..e0ef9446e 100644 --- a/godot-core/src/builtin/meta/call_error.rs +++ b/godot-core/src/builtin/meta/call_error.rs @@ -16,6 +16,8 @@ use std::fmt; /// /// This type is returned from _varcall_ functions in the Godot API that begin with `try_` prefixes, /// e.g. [`Object::try_call()`](crate::engine::Object::try_call) or [`Node::try_rpc()`](crate::engine::Node::try_rpc). +/// _Varcall_ refers to the "variant call" calling convention, meaning that arguments and return values are passed as `Variant` (as opposed +/// to _ptrcall_, which passes direct pointers to Rust objects). /// /// Allows to inspect the involved class and method via `class_name()` and `method_name()`. Implements the `std::error::Error` trait, so /// it comes with `Display` and `Error::source()` APIs. @@ -25,7 +27,7 @@ use std::fmt; /// /// - **Invalid method**: The method does not exist on the object. /// - **Failed argument conversion**: The arguments passed to the method cannot be converted to the declared parameter types. -/// - **Failed return value conversion**: The return value of a dynamic method (`Variant`) cannot be converted to the expected return type. +/// - **Failed return value conversion**: The returned `Variant` of a dynamic method cannot be converted to the expected return type. /// - **Too many or too few arguments**: The number of arguments passed to the method does not match the number of parameters. /// - **User panic**: A Rust method caused a panic. /// @@ -49,14 +51,23 @@ use std::fmt; /// } /// /// fn some_method() { -/// let obj: Gd = MyClass::new_gd(); +/// let mut obj: Gd = MyClass::new_gd(); /// /// // Dynamic call. Note: forgot to pass the argument. -/// let result: Result = obj.try_call("my_method", &[]); +/// let result: Result = obj.try_call("my_method".into(), &[]); /// -/// // Get immediate and original errors. Note that source() can be None or have type ConvertError. +/// // Get immediate and original errors. +/// // Note that source() can be None or of type ConvertError. /// let outer: CallError = result.unwrap_err(); -/// let inner: CallError = outer.source().downcast_ref::().unwrap(); +/// let inner: &CallError = outer.source().unwrap().downcast_ref::().unwrap(); +/// +/// // Immediate error: try_call() internally invokes Object::call(). +/// assert_eq!(outer.class_name(), Some("Object")); +/// assert_eq!(outer.method_name(), "call"); +/// +/// // Original error: my_method() failed. +/// assert_eq!(inner.class_name(), Some("MyClass")); +/// assert_eq!(inner.method_name(), "my_method"); /// } #[derive(Debug)] pub struct CallError { @@ -91,59 +102,23 @@ impl CallError { &self.function_name } - /// Describes the error. - /// - /// This is the same as the `Display`/`ToString` repr, but without the prefix mentioning that this is a function call error, - /// and without any source error information. - fn message(&self, with_source: bool) -> String { - let Self { - call_expr, reason, .. - } = self; - - let reason_str = if reason.is_empty() { - String::new() - } else { - format!("\n Reason: {reason}") - }; - - // let source_str = if with_source { - // self.source() - // .map(|e| format!("\n Source: {}", e)) - // .unwrap_or_default() - // } else { - // String::new() - // }; - - let source_str = match &self.source { - Some(SourceError::Convert(e)) if with_source => format!("\n Source: {}", e), - Some(SourceError::Call(e)) if with_source => format!("\n Source: {}", e.message(true)), - _ => String::new(), - }; - - format!("{call_expr}{reason_str}{source_str}") - } + // ------------------------------------------------------------------------------------------------------------------------------------------ + // Constructors returning Result<(), Self>; possible failure /// Checks whether number of arguments matches the number of parameters. pub(crate) fn check_arg_count( call_ctx: &CallContext, - arg_count: i64, - param_count: i64, + arg_count: usize, + param_count: usize, ) -> Result<(), Self> { // This will need to be adjusted once optional parameters are supported in #[func]. if arg_count == param_count { return Ok(()); } - let param_plural = plural(param_count); - let arg_plural = plural(arg_count); + let call_error = Self::failed_param_count(call_ctx, arg_count, param_count); - Err(Self::new( - call_ctx, - format!( - "function has {param_count} parameter{param_plural}, but received {arg_count} argument{arg_plural}" - ), - None, - )) + Err(call_error) } /// Checks the Godot side of a varcall (low-level `sys::GDExtensionCallError`). @@ -165,7 +140,7 @@ impl CallError { let vararg_str = if varargs.is_empty() { String::new() } else { - format!(", varargs {}", join_args(varargs.into_iter().cloned())) + format!(", [va] {}", join_args(varargs.iter().cloned())) }; let call_expr = format!("{call_ctx}({explicit_args_str}{vararg_str})"); @@ -182,10 +157,14 @@ impl CallError { call_expr, err, &arg_types, + explicit_args.len(), source_error, )) } + // ------------------------------------------------------------------------------------------------------------------------------------------ + // Constructors returning Self; guaranteed failure + /// Returns an error for a failed parameter conversion. pub(crate) fn failed_param_conversion

( call_ctx: &CallContext, @@ -196,12 +175,33 @@ impl CallError { Self::new( call_ctx, - format!("parameter [{param_index}] of type {param_ty} failed conversion"), + format!("parameter #{param_index} ({param_ty}) conversion"), Some(convert_error), ) } + fn failed_param_conversion_engine( + call_ctx: &CallContext, + param_index: i32, + actual: VariantType, + expected: VariantType, + ) -> Self { + // Note: reason is same wording as in FromVariantError::description(). + let reason = format!( + "parameter #{param_index} conversion -- expected type `{expected:?}`, got `{actual:?}`" + ); + + Self::new(call_ctx, reason, None) + } + /// Returns an error for a failed return type conversion. + /// + /// **Note:** There are probably no practical scenarios where this occurs. Different calls: + /// - outbound engine API: return values are statically typed (correct by binding) or Variant (infallible) + /// - #[func] methods: dynamic calls return Variant + /// - GDScript -> Rust calls: value is checked on GDScript side (at parse or runtime), not involving this. + /// + /// It might only occur if there are mistakes in the binding, or if we at some point add typed dynamic calls, à la `call((1, "str"))`. pub(crate) fn failed_return_conversion( call_ctx: &CallContext, convert_error: ConvertError, @@ -210,16 +210,34 @@ impl CallError { Self::new( call_ctx, - format!("return value {return_ty} failed conversion"), + format!("return value {return_ty} conversion"), Some(convert_error), ) } + fn failed_param_count( + call_ctx: &CallContext, + arg_count: usize, + param_count: usize, + ) -> CallError { + let param_plural = plural(param_count); + let arg_plural = plural(arg_count); + + Self::new( + call_ctx, + format!( + "function has {param_count} parameter{param_plural}, but received {arg_count} argument{arg_plural}" + ), + None, + ) + } + fn failed_varcall_inner( call_ctx: &CallContext, call_expr: String, err: sys::GDExtensionCallError, arg_types: &[VariantType], + vararg_offset: usize, source: Option, ) -> Self { // This specializes on reflection-style calls, e.g. call(), rpc() etc. @@ -233,35 +251,52 @@ impl CallError { expected, } = err; - let argc = arg_types.len(); - let reason = match error { - sys::GDEXTENSION_CALL_ERROR_INVALID_METHOD => "method not found".to_string(), + let mut call_error = match error { + sys::GDEXTENSION_CALL_ERROR_INVALID_METHOD => { + Self::new(call_ctx, "method not found", None) + } sys::GDEXTENSION_CALL_ERROR_INVALID_ARGUMENT => { - let from = arg_types[argument as usize]; + // Index calculation relies on patterns like call("...", varargs), might not always work... + let from = arg_types[vararg_offset + argument as usize]; let to = VariantType::from_sys(expected as sys::GDExtensionVariantType); let i = argument + 1; - format!("cannot convert argument #{i} from {from:?} to {to:?}") + Self::failed_param_conversion_engine(call_ctx, i, from, to) + } + sys::GDEXTENSION_CALL_ERROR_TOO_MANY_ARGUMENTS + | sys::GDEXTENSION_CALL_ERROR_TOO_FEW_ARGUMENTS => { + let arg_count = arg_types.len() - vararg_offset; + let param_count = expected as usize; + Self::failed_param_count(call_ctx, arg_count, param_count) + } + sys::GDEXTENSION_CALL_ERROR_INSTANCE_IS_NULL => { + Self::new(call_ctx, "instance is null", None) } - sys::GDEXTENSION_CALL_ERROR_TOO_MANY_ARGUMENTS => { - format!("too many arguments; expected {argument}, but called with {argc}") + sys::GDEXTENSION_CALL_ERROR_METHOD_NOT_CONST => { + Self::new(call_ctx, "method is not const", None) } - sys::GDEXTENSION_CALL_ERROR_TOO_FEW_ARGUMENTS => { - format!("too few arguments; expected {argument}, but called with {argc}") + sys::GODOT_RUST_CUSTOM_CALL_ERROR => { + // Not emitted by Godot. + Self::new(call_ctx, String::new(), None) } - sys::GDEXTENSION_CALL_ERROR_INSTANCE_IS_NULL => "instance is null".to_string(), - sys::GDEXTENSION_CALL_ERROR_METHOD_NOT_CONST => "method is not const".to_string(), // not handled in Godot - sys::GODOT_RUST_CUSTOM_CALL_ERROR => String::new(), - _ => format!("unknown reason (error code {error})"), + _ => Self::new( + call_ctx, + format!("unknown reason (error code {error})"), + None, + ), }; - Self { - class_name: call_ctx.class_name.to_string(), - function_name: call_ctx.function_name.to_string(), - call_expr, - reason, - source, - } + // Self { + // class_name: call_ctx.class_name.to_string(), + // function_name: call_ctx.function_name.to_string(), + // call_expr, + // reason, + // source, + // } + + call_error.source = source; + call_error.call_expr = call_expr; + call_error } #[doc(hidden)] @@ -269,15 +304,51 @@ impl CallError { Self::new(call_ctx, reason, None) } - fn new(call_ctx: &CallContext, reason: String, source: Option) -> Self { + fn new( + call_ctx: &CallContext, + reason: impl Into, + source: Option, + ) -> Self { Self { class_name: call_ctx.class_name.to_string(), function_name: call_ctx.function_name.to_string(), call_expr: format!("{call_ctx}()"), - reason, + reason: reason.into(), source: source.map(|e| SourceError::Convert(Box::new(e))), } } + + /// Describes the error. + /// + /// This is the same as the `Display`/`ToString` repr, but without the prefix mentioning that this is a function call error, + /// and without any source error information. + fn message(&self, with_source: bool) -> String { + let Self { + call_expr, reason, .. + } = self; + + let reason_str = if reason.is_empty() { + String::new() + } else { + format!("\n Reason: {reason}") + }; + + // let source_str = if with_source { + // self.source() + // .map(|e| format!("\n Source: {}", e)) + // .unwrap_or_default() + // } else { + // String::new() + // }; + + let source_str = match &self.source { + Some(SourceError::Convert(e)) if with_source => format!("\n Source: {}", e), + Some(SourceError::Call(e)) if with_source => format!("\n Source: {}", e.message(true)), + _ => String::new(), + }; + + format!("{call_expr}{reason_str}{source_str}") + } } impl fmt::Display for CallError { @@ -317,7 +388,7 @@ fn join_args(args: impl Iterator) -> String { join_debug(args) } -fn plural(count: i64) -> &'static str { +fn plural(count: usize) -> &'static str { if count == 1 { "" } else { diff --git a/godot-core/src/builtin/meta/godot_convert/convert_error.rs b/godot-core/src/builtin/meta/godot_convert/convert_error.rs index 897032fc1..acb4f8abe 100644 --- a/godot-core/src/builtin/meta/godot_convert/convert_error.rs +++ b/godot-core/src/builtin/meta/godot_convert/convert_error.rs @@ -229,7 +229,7 @@ pub(crate) enum FromVariantError { /// Variant type does not match expected type BadType { expected: VariantType, - got: VariantType, + actual: VariantType, }, WrongClass { @@ -247,11 +247,12 @@ impl FromVariantError { fn description(&self) -> String { match self { - Self::BadType { expected, got } => { - format!("Variant type mismatch -- expected `{expected:?}` but got `{got:?}`") + Self::BadType { expected, actual } => { + // Note: wording is the same as in CallError::failed_param_conversion_engine() + format!("expected type `{expected:?}`, got `{actual:?}`") } Self::WrongClass { expected } => { - format!("Variant class mismatch -- expected `{expected}`") + format!("expected class `{expected}`") } } } diff --git a/godot-core/src/builtin/meta/signature.rs b/godot-core/src/builtin/meta/signature.rs index 16e6d8adf..1c00a2a7d 100644 --- a/godot-core/src/builtin/meta/signature.rs +++ b/godot-core/src/builtin/meta/signature.rs @@ -183,7 +183,7 @@ macro_rules! impl_varcall_signature_for_tuple { func: fn(sys::GDExtensionClassInstancePtr, Self::Params) -> Self::Ret, ) -> Result<(), CallError> { //$crate::out!("in_varcall: {call_ctx}"); - CallError::check_arg_count(call_ctx, arg_count, $PARAM_COUNT)?; + CallError::check_arg_count(call_ctx, arg_count as usize, $PARAM_COUNT)?; let args = ($( unsafe { varcall_arg::<$Pn, $n>(args_ptr, call_ctx)? }, @@ -241,9 +241,7 @@ macro_rules! impl_varcall_signature_for_tuple { variant.and_then(|v| { v.try_to::() - .or_else(|e| { - Err(CallError::failed_return_conversion::(&call_ctx, e)) - }) + .map_err(|e| CallError::failed_return_conversion::(&call_ctx, e)) }) } @@ -472,7 +470,7 @@ unsafe fn varcall_arg( let variant_ref = &*Variant::ptr_from_sys(*args_ptr.offset(N)); P::try_from_variant(variant_ref) - .or_else(|err| Err(CallError::failed_param_conversion::

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

(call_ctx, N, err)) } /// Moves `ret_val` into `ret`. diff --git a/godot-core/src/builtin/variant/impls.rs b/godot-core/src/builtin/variant/impls.rs index a022b3a91..ac963a247 100644 --- a/godot-core/src/builtin/variant/impls.rs +++ b/godot-core/src/builtin/variant/impls.rs @@ -40,7 +40,7 @@ macro_rules! impl_ffi_variant { if variant.get_type() != Self::variant_type() { return Err(FromVariantError::BadType { expected: Self::variant_type(), - got: variant.get_type(), + actual: variant.get_type(), } .into_error(variant)); } @@ -158,7 +158,7 @@ impl GodotFfiVariant for () { Err(FromVariantError::BadType { expected: VariantType::Nil, - got: variant.get_type(), + actual: variant.get_type(), } .into_error(variant)) } diff --git a/godot-core/src/log.rs b/godot-core/src/log.rs index f7c450bea..be56c5c35 100644 --- a/godot-core/src/log.rs +++ b/godot-core/src/log.rs @@ -7,6 +7,20 @@ //! Printing and logging functionality. +// https://stackoverflow.com/a/40234666 +#[macro_export] +#[doc(hidden)] +macro_rules! inner_function { + () => {{ + fn f() {} + fn type_name_of(_: T) -> &'static str { + std::any::type_name::() + } + let name = type_name_of(f); + name.strip_suffix("::f").unwrap() + }}; +} + #[macro_export] #[doc(hidden)] macro_rules! inner_godot_msg { @@ -19,9 +33,10 @@ macro_rules! inner_godot_msg { // Check whether engine is loaded, otherwise fall back to stderr. if $crate::sys::is_initialized() { + let function = format!("{}\0", $crate::inner_function!()); $crate::sys::interface_fn!($godot_fn)( $crate::sys::c_str_from_str(&msg), - $crate::sys::c_str(b"\0"), + $crate::sys::c_str_from_str(&function), $crate::sys::c_str_from_str(concat!(file!(), "\0")), line!() as i32, false as $crate::sys::GDExtensionBool, // whether to create a toast notification in editor diff --git a/godot-core/src/private.rs b/godot-core/src/private.rs index a958a36e7..a840042ef 100644 --- a/godot-core/src/private.rs +++ b/godot-core/src/private.rs @@ -158,6 +158,7 @@ pub fn is_class_runtime(is_tool: bool) -> bool { // ---------------------------------------------------------------------------------------------------------------------------------------------- // Panic handling +#[derive(Debug)] struct GodotPanicInfo { line: u32, file: String, @@ -180,9 +181,9 @@ fn format_panic_message(msg: String) -> String { let indented = msg.replace('\n', lbegin); if indented.len() != msg.len() { - format!("Panic msg:{lbegin}{indented}") + format!("[panic]{lbegin}{indented}") } else { - format!("Panic msg: {msg}") + format!("[panic] {msg}") } } diff --git a/itest/rust/src/object_tests/dynamic_call_test.rs b/itest/rust/src/object_tests/dynamic_call_test.rs index a5b282c41..5769ed1d2 100644 --- a/itest/rust/src/object_tests/dynamic_call_test.rs +++ b/itest/rust/src/object_tests/dynamic_call_test.rs @@ -7,11 +7,11 @@ use godot::builtin::meta::{CallError, FromGodot, ToGodot}; use godot::builtin::{StringName, Variant, Vector3}; -use godot::engine::{Node3D, Object}; +use godot::engine::{Node, Node3D, Object}; use godot::obj::{InstanceId, NewAlloc}; use std::error::Error; -use crate::framework::{expect_panic, itest}; +use crate::framework::{expect_panic, itest, runs_release}; use crate::object_tests::object_test::ObjPayload; #[itest] @@ -44,6 +44,9 @@ fn dynamic_call_with_args() { node.free(); } +// ---------------------------------------------------------------------------------------------------------------------------------------------- +// Erroneous dynamic calls to #[func] + #[itest] fn dynamic_call_with_too_few_args() { let mut obj = ObjPayload::new_alloc(); @@ -65,7 +68,7 @@ fn dynamic_call_with_too_few_args() { call_error.to_string(), "godot-rust function call failed: Object::call(&\"take_1_int\")\ \n Source: ObjPayload::take_1_int()\ - \n Reason: function has 1 parameter, but received 0 arguments" + \n Reason: function has 1 parameter, but received 0 arguments" ); // Method where error originated (this is not repeated in all tests, the logic for chaining is the same). @@ -73,7 +76,7 @@ fn dynamic_call_with_too_few_args() { assert_eq!( source.to_string(), "godot-rust function call failed: ObjPayload::take_1_int()\ - \n Reason: function has 1 parameter, but received 0 arguments" + \n Reason: function has 1 parameter, but received 0 arguments" ); let source = source @@ -103,9 +106,36 @@ fn dynamic_call_with_too_many_args() { assert_eq!(call_error.method_name(), "call"); assert_eq!( call_error.to_string(), - "godot-rust function call failed: Object::call(&\"take_1_int\", varargs 42, 43)\ + "godot-rust function call failed: Object::call(&\"take_1_int\", [va] 42, 43)\ + \n Source: ObjPayload::take_1_int()\ + \n Reason: function has 1 parameter, but received 2 arguments" + ); + + obj.free(); +} + +#[itest] +fn dynamic_call_parameter_mismatch() { + let mut obj = ObjPayload::new_alloc(); + + // Use panicking version. + expect_panic("call with wrong argument type", || { + obj.call("take_1_int".into(), &["string".to_variant()]); + }); + + // Use Result-based version. + let call_error = obj + .try_call("take_1_int".into(), &["string".to_variant()]) + .expect_err("expected failed call"); + + assert_eq!(call_error.class_name(), Some("Object")); + assert_eq!(call_error.method_name(), "call"); + assert_eq!( + call_error.to_string(), + "godot-rust function call failed: Object::call(&\"take_1_int\", [va] \"string\")\ \n Source: ObjPayload::take_1_int()\ - \n Reason: function has 1 parameter, but received 2 arguments" + \n Reason: parameter #0 (i64) conversion\ + \n Source: expected type `Int`, got `String`: \"string\"" ); obj.free(); @@ -124,8 +154,123 @@ fn dynamic_call_with_panic() { call_error.to_string(), "godot-rust function call failed: Object::call(&\"do_panic\")\ \n Source: ObjPayload::do_panic()\ - \n Reason: Panic msg: do_panic exploded" + \n Reason: [panic] do_panic exploded" ); obj.free(); } + +// ---------------------------------------------------------------------------------------------------------------------------------------------- +// Erroneous dynamic calls to engine APIs + +#[itest] +fn dynamic_call_with_too_few_args_engine() { + // Disabled in release (parameter count is unchecked by engine). + // Before 4.2, the Godot check had a bug: https://github.com/godotengine/godot/pull/80844. + if runs_release() || cfg!(before_api = "4.2") { + return; + } + + let mut node = Node::new_alloc(); + + // Use panicking version. + expect_panic("call with too few arguments", || { + node.call("rpc_config".into(), &["some_method".to_variant()]); + }); + + // Use Result-based version. + let call_error = node + .try_call("rpc_config".into(), &["some_method".to_variant()]) + .expect_err("expected failed call"); + + assert_eq!(call_error.class_name(), Some("Object")); + assert_eq!(call_error.method_name(), "call"); + assert_eq!( + call_error.to_string(), + "godot-rust function call failed: Object::call(&\"rpc_config\", [va] \"some_method\")\ + \n Reason: function has 2 parameters, but received 1 argument" + ); + + node.free(); +} + +#[itest] +fn dynamic_call_with_too_many_args_engine() { + // Disabled in release (parameter count is unchecked by engine). + // Before 4.2, the Godot check had a bug: https://github.com/godotengine/godot/pull/80844. + if runs_release() || cfg!(before_api = "4.2") { + return; + } + + let mut node = Node::new_alloc(); + + // Use panicking version. + expect_panic("call with too many arguments", || { + node.call( + "rpc_config".into(), + &["some_method".to_variant(), Variant::nil(), 123.to_variant()], + ); + }); + + // Use Result-based version. + let call_error = node + .try_call( + "rpc_config".into(), + &["some_method".to_variant(), Variant::nil(), 123.to_variant()], + ) + .expect_err("expected failed call"); + + assert_eq!(call_error.class_name(), Some("Object")); + assert_eq!(call_error.method_name(), "call"); + assert_eq!( + call_error.to_string(), + "godot-rust function call failed: Object::call(&\"rpc_config\", [va] \"some_method\", null, 123)\ + \n Reason: function has 2 parameters, but received 3 arguments" + ); + + node.free(); +} + +#[itest] +fn dynamic_call_parameter_mismatch_engine() { + // Disabled in release (parameter types are unchecked by engine). + if runs_release() { + return; + } + + let mut node = Node::new_alloc(); + + // Use panicking version. + expect_panic("call with wrong argument type", || { + node.call("set_name".into(), &[123.to_variant()]); + }); + + // Use Result-based version. + let call_error = node + .try_call("set_name".into(), &[123.to_variant()]) + .expect_err("expected failed call"); + + // Note: currently no mention of Node::set_name(). Not sure if easily possible to add. + assert_eq!(call_error.class_name(), Some("Object")); + assert_eq!(call_error.method_name(), "call"); + assert_eq!( + call_error.to_string(), + "godot-rust function call failed: Object::call(&\"set_name\", [va] 123)\ + \n Reason: parameter #1 conversion -- expected type `String`, got `Int`" + ); + + node.free(); +} + +#[itest(skip)] +fn dynamic_call_return_mismatch() { + // Cannot easily test this, as both calls to #[func] and Godot APIs are either strongly typed and correct (ensured by codegen), + // or they return Variant, which then fails on user side only. + + // Even GDScript -> Rust calls cannot really use this. Given this GDScript code: + // var obj = ObjPayload.new() + // var result: String = obj.take_1_int(20) + // + // The parser will fail since it knows the signature of take_1_int(). And if we enforce `: Variant` type hints, it will just + // cause a runtime error, but that's entirely handled in GDScript. +}