Provide elaborate error message on failed varcalls

Author: Bromeon

godot-codegen/src/class_generator.rs

@@ -7,7 +7,7 @@
 //! Generates a file for each Godot engine + builtin class
 
 use proc_macro2::{Ident, TokenStream};
-use quote::{format_ident, quote};
+use quote::{format_ident, quote, ToTokens};
 use std::path::{Path, PathBuf};
 
 use crate::api_parser::*;
@@ -712,12 +712,43 @@ fn make_function_definition(
 
     let is_varcall = variant_ffi.is_some();
     let fn_name = safe_ident(function_name);
-    let (params, arg_exprs) = make_params(method_args, is_varcall, ctx);
+    let [params, variant_types, arg_exprs, arg_names] = make_params(method_args, is_varcall, ctx);
+
+    let (prepare_arg_types, error_fn_context);
+    if variant_ffi.is_some() {
+        // varcall (using varargs)
+        prepare_arg_types = quote! {
+            let mut __arg_types = Vec::with_capacity(__explicit_args.len() + varargs.len());
+            // __arg_types.extend(__explicit_args.iter().map(Variant::get_type));
+            __arg_types.extend(varargs.iter().map(Variant::get_type));
+            let __vararg_str = varargs.iter().map(|v| format!("{v}")).collect::<Vec<_>>().join(", ");
+        };
+
+        let joined = arg_names
+            .iter()
+            .map(|n| format!("{{{n}:?}}"))
+            .collect::<Vec<_>>()
+            .join(", ");
+
+        let fmt = format!("{function_name}({joined}; {{__vararg_str}})");
+        error_fn_context = quote! { &format!(#fmt) };
+    } else {
+        // ptrcall
+        prepare_arg_types = quote! {
+            let __arg_types = [
+                #( #variant_types ),*
+            ];
+        };
+        error_fn_context = function_name.to_token_stream();
+    };
+
     let (return_decl, call_code) = make_return(
         return_value,
         variant_ffi.as_ref(),
         varcall_invocation,
         ptrcall_invocation,
+        prepare_arg_types,
+        error_fn_context,
         ctx,
     );
 
@@ -733,7 +764,7 @@ fn make_function_definition(
                         #( #arg_exprs ),*
                     ];
 
-                    let mut __args = Vec::new();
+                    let mut __args = Vec::with_capacity(__explicit_args.len() + varargs.len());
                     __args.extend(__explicit_args.iter().map(Variant::#sys_method));
                     __args.extend(varargs.iter().map(Variant::#sys_method));
 
@@ -789,39 +820,41 @@ fn make_params(
     method_args: &Option<Vec<MethodArg>>,
     is_varcall: bool,
     ctx: &mut Context,
-) -> (Vec<TokenStream>, Vec<TokenStream>) {
+) -> [Vec<TokenStream>; 4] {
     let empty = vec![];
     let method_args = method_args.as_ref().unwrap_or(&empty);
 
     let mut params = vec![];
+    let mut variant_types = vec![];
     let mut arg_exprs = vec![];
+    let mut arg_names = vec![];
     for arg in method_args.iter() {
         let param_name = safe_ident(&arg.name);
         let param_ty = to_rust_type(&arg.type_, ctx);
 
-        params.push(quote! { #param_name: #param_ty });
-        if is_varcall {
-            arg_exprs.push(quote! {
-                <#param_ty as ToVariant>::to_variant(&#param_name)
-            });
-        } else if let RustTy::EngineClass { tokens: path, .. } = param_ty {
-            arg_exprs.push(quote! {
-                <#path as AsArg>::as_arg_ptr(&#param_name)
-            });
+        let arg_expr = if is_varcall {
+            quote! { <#param_ty as ToVariant>::to_variant(&#param_name) }
+        } else if let RustTy::EngineClass { tokens: path, .. } = &param_ty {
+            quote! { <#path as AsArg>::as_arg_ptr(&#param_name) }
         } else {
-            arg_exprs.push(quote! {
-                <#param_ty as sys::GodotFfi>::sys_const(&#param_name)
-            });
-        }
+            quote! { <#param_ty as sys::GodotFfi>::sys_const(&#param_name) }
+        };
+
+        params.push(quote! { #param_name: #param_ty });
+        variant_types.push(quote! { <#param_ty as VariantMetadata>::variant_type() });
+        arg_exprs.push(arg_expr);
+        arg_names.push(quote! { #param_name });
     }
-    (params, arg_exprs)
+    [params, variant_types, arg_exprs, arg_names]
 }
 
 fn make_return(
     return_value: Option<&MethodReturn>,
     variant_ffi: Option<&VariantFfi>,
     varcall_invocation: &TokenStream,
     ptrcall_invocation: &TokenStream,
+    prepare_arg_types: TokenStream,
+    error_fn_context: TokenStream, // only for panic message
     ctx: &mut Context,
 ) -> (TokenStream, TokenStream) {
     let return_decl: TokenStream;
@@ -851,7 +884,10 @@ fn make_return(
                 let variant = Variant::#from_sys_init_method(|return_ptr| {
                     let mut __err = sys::default_call_error();
                     #varcall_invocation
-                    sys::panic_on_call_error(&__err);
+                    if __err.error != sys::GDEXTENSION_CALL_OK {
+                        #prepare_arg_types
+                        sys::panic_call_error(&__err, #error_fn_context, &__arg_types);
+                    }
                 });
                 #return_expr
             }
@@ -863,7 +899,10 @@ fn make_return(
                 let mut __err = sys::default_call_error();
                 let return_ptr = std::ptr::null_mut();
                 #varcall_invocation
-                sys::panic_on_call_error(&__err);
+                if __err.error != sys::GDEXTENSION_CALL_OK {
+                    #prepare_arg_types
+                    sys::panic_call_error(&__err, #error_fn_context, &__arg_types);
+                }
             }
         }
         (None, Some(RustTy::EngineClass { tokens, .. })) => {

godot-core/src/builtin/variant/mod.rs

@@ -122,7 +122,10 @@ impl Variant {
             )
         };
 
-        sys::panic_on_call_error(&error);
+        if error.error != sys::GDEXTENSION_CALL_OK {
+            let arg_types: Vec<_> = args.iter().map(Variant::get_type).collect();
+            sys::panic_call_error(&error, "call", &arg_types);
+        }
         result
     }
 

godot-ffi/src/lib.rs

@@ -139,42 +139,7 @@ unsafe fn unwrap_ref_unchecked_mut<T>(opt: &mut Option<T>) -> &mut T {
     }
 }
 
-#[doc(hidden)]
-#[inline]
-pub fn default_call_error() -> GDExtensionCallError {
-    GDExtensionCallError {
-        error: GDEXTENSION_CALL_OK,
-        argument: -1,
-        expected: -1,
-    }
-}
-
-#[doc(hidden)]
-#[inline]
-#[track_caller] // panic message points to call site
-pub fn panic_on_call_error(err: &GDExtensionCallError) {
-    let actual = err.error;
-
-    assert_eq!(
-        actual,
-        GDEXTENSION_CALL_OK,
-        "encountered Godot error code {}",
-        call_error_to_string(actual)
-    );
-}
-
-fn call_error_to_string(err: GDExtensionCallErrorType) -> &'static str {
-    match err {
-        GDEXTENSION_CALL_OK => "OK",
-        GDEXTENSION_CALL_ERROR_INVALID_METHOD => "ERROR_INVALID_METHOD",
-        GDEXTENSION_CALL_ERROR_INVALID_ARGUMENT => "ERROR_INVALID_ARGUMENT",
-        GDEXTENSION_CALL_ERROR_TOO_MANY_ARGUMENTS => "ERROR_TOO_MANY_ARGUMENTS",
-        GDEXTENSION_CALL_ERROR_TOO_FEW_ARGUMENTS => "ERROR_TOO_FEW_ARGUMENTS",
-        GDEXTENSION_CALL_ERROR_INSTANCE_IS_NULL => "ERROR_INSTANCE_IS_NULL",
-        GDEXTENSION_CALL_ERROR_METHOD_NOT_CONST => "ERROR_METHOD_NOT_CONST",
-        _ => "(unknown)",
-    }
-}
+// ----------------------------------------------------------------------------------------------------------------------------------------------
 
 #[macro_export]
 #[doc(hidden)]
@@ -192,8 +157,6 @@ macro_rules! builtin_call {
         };
     }
 
-// ----------------------------------------------------------------------------------------------------------------------------------------------
-
 #[macro_export]
 #[doc(hidden)]
 macro_rules! interface_fn {
@@ -256,3 +219,57 @@ where
         Some(mapper(ptr))
     }
 }
+
+// ----------------------------------------------------------------------------------------------------------------------------------------------
+
+#[doc(hidden)]
+#[inline]
+pub fn default_call_error() -> GDExtensionCallError {
+    GDExtensionCallError {
+        error: GDEXTENSION_CALL_OK,
+        argument: -1,
+        expected: -1,
+    }
+}
+
+#[doc(hidden)]
+#[inline]
+#[track_caller] // panic message points to call site
+pub fn panic_call_error(
+    err: &GDExtensionCallError,
+    function_name: &str,
+    arg_types: &[VariantType],
+) -> ! {
+    debug_assert_ne!(err.error, GDEXTENSION_CALL_OK); // already checked outside
+
+    let GDExtensionCallError {
+        error,
+        argument,
+        expected,
+    } = *err;
+
+    let argc = arg_types.len();
+    let reason = match error {
+        GDEXTENSION_CALL_ERROR_INVALID_METHOD => "method not found".to_string(),
+        GDEXTENSION_CALL_ERROR_INVALID_ARGUMENT => {
+            let from = arg_types[argument as usize];
+            let to = VariantType::from_sys(expected as GDExtensionVariantType);
+            let i = argument + 1;
+
+            format!("cannot convert argument #{i} from {from:?} to {to:?}")
+        }
+        GDEXTENSION_CALL_ERROR_TOO_MANY_ARGUMENTS => {
+            format!("too many arguments; expected {argument}, but called with {argc}")
+        }
+        GDEXTENSION_CALL_ERROR_TOO_FEW_ARGUMENTS => {
+            format!("too few arguments; expected {argument}, but called with {argc}")
+        }
+        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
+        _ => format!("unknown reason (error code {error})"),
+    };
+
+    // Note: Godot also outputs thread ID
+    // In Godot source: variant.cpp:3043 or core_bind.cpp:2742
+    panic!("Function call failed:  {function_name} -- {reason}.");
+}

itest/godot/ManualFfiTests.gd

@@ -37,9 +37,8 @@ func test_export():
 	assert_eq(obj.object_val, node)
 
 	var texture_val_meta = obj.get_property_list().filter(
-		func(el)->bool:
-			return el["name"] == "texture_val"
-			).front()
+		func(el): return el["name"] == "texture_val"
+	).front()
 
 	assert_that(texture_val_meta != null, "'texture_val' is defined")
 	assert_eq(texture_val_meta["hint"], PropertyHint.PROPERTY_HINT_RESOURCE_TYPE)

itest/rust/src/node_test.rs

@@ -4,8 +4,8 @@
  * file, You can obtain one at https://mozilla.org/MPL/2.0/.
  */
 
-use crate::itest;
-use godot::builtin::NodePath;
+use crate::{itest, TestContext};
+use godot::builtin::{NodePath, Variant};
 use godot::engine::{global, node, Node, Node3D, NodeExt, PackedScene, SceneTree};
 use godot::obj::Share;
 
@@ -81,3 +81,13 @@ fn node_scene_tree() {
     parent.free();
     child.free();
 }
+
+// FIXME: call_group() crashes
+#[itest(skip)]
+fn node_call_group(ctx: &TestContext) {
+    let mut node = ctx.scene_tree.share();
+    let mut tree = node.get_tree().unwrap();
+
+    node.add_to_group("group".into(), true);
+    tree.call_group("group".into(), "set_name".into(), &[Variant::from("name")]);
+}

itest/rust/src/runner.rs

@@ -172,7 +172,7 @@ fn run_rust_test(test: &RustTestCase, ctx: &TestContext) -> TestOutcome {
     }
 
     // Explicit type to prevent tests from returning a value
-    let err_context = || format!("   !! Test {} failed", test.name);
+    let err_context = || format!("itest `{}` failed", test.name);
     let success: Option<()> = godot::private::handle_panic(err_context, || (test.function)(ctx));
 
     TestOutcome::from_bool(success.is_some())