Unwrangling the mess that is godot_wrap_method, adding generic method support as a side effect

[ghost]

Due to repeated past additions to godot_wrap_method_inner, the implementation has become a total mess of unsafe macro code that is very hard to follow. Meanwhile, the manual interface for method registration has largely been neglected, lagging behind the rest of the crate, exposing sys types and unsafe FFI signatures. Recently, I've found myself in need to register a few methods manually and having to deal with this entire situation, so I've taken the chance to make some improvements to this part of the codebase, with all the hindsight that we now have. This has resulted in satisfactory results:

• All the unsafe FFI code previously in godot_wrap_method_inner is extracted to the library proper, and can no longer be touched by obscure import/inference problems that come from delayed type checking.
• Argument errors are now represented with a proper error type with a Display implementation. The library can report multiple argument errors at once. "Missing/excessive argument" errors are also more informative.
• It's possible to manually register one method for many classes that it may apply to, or many specializations of a generic method to one class by different names.
• Safe, straightforward support for stateful methods.

This is not in itself a breaking change, but it's expected that the older unsafe interface will be removed at the next possibility.

---

As a curiosity, the syntax for manually registering generic methods currently looks like this, and I find it already fairly usable even without further macro goodness:

#[derive(Copy, Clone, Default)]
struct Mixin<T> {
    _marker: PhantomData<T>,
}

#[derive(FromVarargs)]
struct Args<T> {
    a: T,
    b: T,
}

impl<T, C> StaticArgsMethod<C> for Mixin<T>
where
    T: Add<Output = T> + Send + Sync + ToVariant + FromVariant + 'static,
    C: NativeClass,
{
    type Args = Args<T>;
    fn call(&self, _this: RefInstance<'_, C, Shared>, Args { a, b }: Args<T>) -> Variant {
        (a + b).to_variant()
    }
}

fn register_methods(builder: &ClassBuilder<SomeClass>) {
    builder
        .build_method("add_i", StaticArgs::<Mixin::<i32>>::default())
        .done_stateless();
    builder
        .build_method("add_f", StaticArgs::<Mixin::<f32>>::default())
        .done_stateless();
    builder
        .build_method("add_v", StaticArgs::<Mixin::<Vector2>>::default())
        .done_stateless();
}

Unresolved questions

As a side effect of moving the error handling to the library side, error sites all point to the library now. This should be fixable with something on the macro side that provides borrow-able site information, like how profiling::Signature is handled? This would mean moving the FFI call out of godot_error into some interface as well. Resolved.

Future development

If desired, it should be fairly easy to add a higher-level, derivable Mixin interface based on this. It should also be possible to add support for generic methods directly in #[export].

[ghost]

bors try

[bors]

try

Build succeeded:

• ci

[halzy]

If class_builder and name don't have the same lifetime, will this still work?

[ghost]

Since the lifetime isn't carried on ClassBuilder itself, but is inferred by the compiler, the lifetime of name simply places a shorter bound on 'a, which is then inferred by the compiler to be the same as class_builder. As a result, this does compile: builder.build_method(&format!("foo{}", 123), method).done().

[halzy]

Nice changes! It is much more understandable than before.

[halzy]

Could you comment on why 1 for the void*.

[ghost]

Stateless<F> is a ZST for any type F.

[halzy]

Will name and ty have the same issue? If they have different lifetimes?

[ghost]

This one does turn out to be problematic since all error objects need to have the same lifetime parameter, which will have to be 'a since 'r is temporary. I think it might be best to store a Cow<'static, str> here instead, since we don't want to allocate all the parameter names when the vast majority of them are just going to be 'static.

[ghost]

Added explanation of the ZST pointer in comments. Made strings in ArgumentError Cow. Added a Site API to deal with the error reporting problem introduced by the original PR. Errors from wrapped methods now have correct file paths and type/method names set:

ERROR: OptionalArgs :: opt_sum: missing non-optional parameter `b` (#1)
   At: test/src/lib.rs:198.
ERROR: OptionalArgs :: opt_sum: an excessive argument is given: I64(6)
   At: test/src/lib.rs:198.

Compared to original PR (path pointing to library):

ERROR: <native>: missing non-optional parameter `b` (#1)
   At: gdnative-core/src/nativescript/init/method.rs:188.
ERROR: <native>: an excessive argument is given: I64(6)
   At: gdnative-core/src/nativescript/init/method.rs:181.

Compared to current master (type and method names not shown, worse message):

ERROR: <native>: Incorrect number of parameters: required 2 but got 1
   At: test/src/lib.rs:198.
ERROR: <native>: Incorrect number of parameters: expected at most 5 but got 6
   At: test/src/lib.rs:198.

[ghost]

bors try

[bors]

try

Build succeeded:

• ci

[ghost]

Marking as breaking change since I just realized that this breaks code currently using godot_wrap_method manually to create pointers.

[ghost]

Found an issue with the current code: since <$retty as $crate::core_types::OwnedToVariant>::owned_to_variant(ret) is now wrapped in an impl StaticArgsMethod<T> for ThisMethod block, any Selfs in the return type signature now refers to ThisMethod instead of what's intended. Should be easy to fix by just dropping the fully qualified syntax and using OwnedToVariant::owned_to_variant, will update later.

[ghost]

Should be ready for merging!

[halzy]

bors r+

[bors]

Build succeeded:

• ci