To panic or not to panic when passing incorrect argument types?

[andreymal]

An example function:

#[derive(GodotClass)]
#[class(init)]
struct CallMe;

#[godot_api]
impl CallMe {
    #[func]
    fn call_me(a: Gd<Resource>, b: i32) -> Gd<Resource> {
        Resource::new_gd()
    }
}

These calls don't panic and abort the script instead:

# godot-rust function call failed / Bug, call error: #40

var not_a_resource: Variant = Node.new()
CallMe.call_me(not_a_resource, 0)

CallMe.call_me(null, 0)

var overflow_me_untyped: Variant = 9223372036854775807
CallMe.call_me(Resource.new(), overflow_me_untyped)

But these ones panic and do not abort the script (null is returned instead):

# godot-core/src/private.rs:369 - Rust function panicked at godot-core/src/meta/signature.rs:556.
#    Context: CallMe::call_me
#  godot-core/src/private.rs:382 - [panic]  in function `CallMe::call_me` at parameter [0] of type godot_core::obj::gd::Gd<godot_core::gen::classes::resource::re_export::Resource>: `Gd` cannot be null: null
var maybe_resource: Resource = null
CallMe.call_me(maybe_resource, 0)

# godot-core/src/private.rs:369 - Rust function panicked at godot-core/src/meta/signature.rs:556.
#    Context: CallMe::call_me
#  godot-core/src/private.rs:382 - [panic]  in function `CallMe::call_me` at parameter [1] of type i32: `i32` cannot store the given value: 9223372036854775807
var overflow_me_typed := 9223372036854775807
CallMe.call_me(Resource.new(), overflow_me_typed)

I don't know which behavior is correct (but according to API design principles _{(which contains a broken link btw)} and https://github.com/godot-rust/gdext/issues/327#issuecomment-2097020581, I assume you are a fan of panics)

But regardless, I expect consistent behavior: panic everywhere or don't panic at all.

^{(In general, I'd prefer not to panic because I'm trying to compile with panic = "abort" in hopes of reducing binary size, godot-rust binaries are so damn huge...) _{(or maybe explicitly disallow panic = "abort" somewhere in the "Getting Started" chapter of the book if you think this sucks)}}

[Bromeon]

But regardless, I expect consistent behavior: panic everywhere or don't panic at all.

A bit of background here: Godot has two calling conventions -- variant call (varcall) and pointer call (ptrcall). The former is fully dynamic and is used whenever you use reflection, e.g. Object::call(). The latter is used when the method can be dispatched at compile/parse time, and is more efficient.

If you look at the way how Godot defines those in the GDExtension API, you'll come across two signatures:

typedef void (*GDExtensionInterfaceObjectMethodBindCall)(
    GDExtensionMethodBindPtr p_method_bind, 
    GDExtensionObjectPtr p_instance, 
    const GDExtensionConstVariantPtr *p_args, 
    GDExtensionInt p_arg_count, 
    GDExtensionUninitializedVariantPtr r_ret, 
    GDExtensionCallError *r_error
);

typedef void (*GDExtensionInterfaceObjectMethodBindPtrcall)(
    GDExtensionMethodBindPtr p_method_bind, 
    GDExtensionObjectPtr p_instance, 
    const GDExtensionConstTypePtr *p_args, 
    GDExtensionTypePtr r_ret
);

The varcall function pointer offers an output parameter GDExtensionCallError *r_error, while the ptrcall does not. In other words:

Godot cannot propagate errors in ptrcalls.

This is a limitation of the engine, and the reason why godot-rust can currently not let GDScript abort in such cases. Maybe there is a way -- in case you discover one, I'd be interested to hear!

[Bromeon]

godot-rust binaries are so damn huge.

Try the lazy-function-tables feature, that should help. See lib.rs docs for explanation.

panic = "abort"

You can try this, but I'm not sure how many things break in the library, as it's not designed for it. So use at your own risk.

[Bromeon]

After some discussion with the GDExtension team, it seems like the varcall error handling capability is not intended for domain-specific errors (such as Rust panics), but rather type and arity checking of the arguments.

If we handle Rust panics purely in Rust code and don't tap into the varcall error mechanism, we may be able to make the experience more consistent with ptrcalls.

Closely related: https://github.com/godot-rust/gdext/issues/105#issuecomment-2091840490

[andreymal]

godot-rust binaries are so damn huge.

A bit off-topic here but I want to note somewhere that handle_panic_with_print is heavy, replacing its body with return Ok(code()); made my test binary ~200KB smaller (~4KB per #[func])

[lilizoey]

godot-rust binaries are so damn huge.

A bit off-topic here but I want to note somewhere that handle_panic_with_print is heavy, replacing its body with return Ok(code()); made my test binary ~200KB smaller (~4KB per #[func])

I think this would be useful to mention in a new separate issue. It's not really related to this issue at all. We havent really prioritized binary size very much and we dont even have an issue tracking it specifically, so getting some more conversation started about it would help gauge interest and see how it should be prioritized.

[Bromeon]

Any ideas on how to address this discrepancy? We have a very similar issue here: https://github.com/godot-rust/gdext/issues/105#issuecomment-2091840490