gdnative: GDNative default parameters cause breaking changes

Using “varcalls” instead of “ptrcalls” should make those additions of optional parameters non-breaking changes, at the expense of having slower method calls into engine classes.

Using “varcall” is essentially the same mechansim that GDScript or VisualScript uses, so also shares the performance characteristics.

Doing a “ptrcall” is basically the same as taking an untyped function pointer that points directly (more or less) to the in-engine implementation of a method. When doing such a call the number and types of arguments and return values must match exactly, because it’s like calling a function pointer.

Because the performance is better with ptrcalls, the bindings generator tries to select those wherever possible (there are some conditions where a varcall is necessary).

For more “used across different versions” projects, such as plugins that build on GDNative and ship with binaries, it might make sense to switch every method binding to use varcalls. That would mean performance might be a little worse, but it would be guaranteed to be compatible across minor versions, even with the same binary.

We could add a feature flag like generate_only_varcalls (or with a different name) that overrides this selection process to never generate ptrcalls, in case a plugin author wants to guarantee compatibility across minor versions.

https://github.com/godot-rust/godot-rust/blob/42a6e768a02ed32068ae1dfe5a5e655181ea2e6f/bindings_generator/src/methods.rs#L124-L135

Would that be interesting for users of this crate or does this seem like a niche case and not requiring an extra feature flag and another knob to turn or not turn?

Would it make sense to make varcalls the default and recommend users to use ptrcalls only in release builds?

Let me know what you think!

Sorry, forgot to update this issue.

Over the next days, I’ll work on making master compatible with stable Godot 3.4. There are likely more people who are willing to upgrade and benefit from latest Godot features/bugfixes, than downgrade. Those who want to stay with 3.2 or 3.3 can still manually generate the api.json.

In a next step, we should think about simplifying the custom api.json generation step. https://github.com/godot-rust/godot-rust/issues/640 offers a very interesting idea, for example.

I don’t think we want to jump to conclusions here, before looking at empirical data of how often did any of these “exotic” breaking patterns actually happen for the whole duration of Godot 3, if ever. Every approach here is indeed ugly, but not in equal ways so:

Or, we just add a new base method:

obj.method(many, no, longer, required, arguments); // now deprecated
obj.method_v2(many); // new one

The nightmare case is, of course, as you have said, becoming forced to keep multiple legacy versions for years. Docs get clogged with v1, v2, v3 and v4s of multiple functions and everything is terrible. The best case here, however, is just perfectly natural Rust.

This would be ideal in a world where optionalizations are few and far-between, and when they do happen, multiple arguments become optional at once (so fewer versions are necessary).

// positional -- ex method *always* takes exactly one tuple.
// the trait would have (min=1, max=5) arity, and enlarging this range is backwards-compatible
obj.method_ex((many, no, longer, required, arguments));  // initial
obj.method_ex((many,));                                  // later

This avoids the nightmare case, at the cost of making every single method call slightly, but uniformly ugly, regardless of which world we’re living in.

This would be ideal in a world where arguments become optionalized, one by one, all the time, as such events do not generate extra permanent ugliness under this design.

• It’s not a feature that C++ and GDScript provide and is thus expected by users.

It can also be argued that it’s a feature that Rust libraries commonly provide, and is thus expected by users.

• Changing a parameter name now becomes a breaking change in Rust, while it wasn’t in GDScript/C++.

This is something I haven’t considered about. It should be however be possible to “remember” all the past names of an argument with automation, and keep any old named methods around (with #[deprecated]). This only needs to apply to crates builds, of course.

We might need to dig into past api.jsons to find out the actual probability of this happening, but I do not anticipate it to be very high.

• Not compatible if a required parameter becomes optional. This is also true for the named approach.

This neither. For the named approach it’s possible to force all parameters to be specified through the builder interface (i.e. no arguments in the initial is_action_pressed_ex call), but this still leaves open the question of non-builderized versions.

One option is again to memorize the original required parameter count, and generate a new version each time a parameter is optional-ized and deprecate the original. I’m not sure how ugly this will get though, since this scenario may have a higher chance of actual occurrence compared to the previous one, and the impact is higher (new method on the base type vs. new method on a builder). Might need to dig into past api.jsons to find out.

I’m not sure I can come up with something better for the positional API.

Overall I feel like that the proposed pros of the positional API do not outweigh the ability to have a idiomatic Rust interface. Realistically call() is very unlikely to be forgotten due to #[must_use] – the lack of strict linearity being only a theoretical issue. The last two a named approach does as well. So ultimately, it comes down to a stylistic choice, and I sense that this is where the two libraries must diverge.

Thanks a lot for your input!

Very nice! I like the use of () to signal “no argument present”.

One thing I was thinking about – in our previous discussions, we always assumed that default parameters should be provided as named parameters. However, in both GDScript and C++ they are simply positional ones.

While it’s useful to skip some default parameters (e.g. only provide a value for the last one), things to consider:

• It’s not a feature that C++ and GDScript provide and is thus expected by users.
• Changing a parameter name now becomes a breaking change in Rust, while it wasn’t in GDScript/C++.
• On the other hand, an order change of optional parameters is no longer breaking, but this is very unlikely to happen due to still being breaking in GDScript/C++.

That said, it could still prove a very useful addition, especially compared to the current approach.

For GDExtension, I’d like to experiment a bit with positional arguments. One idea I had:

// Machinery
trait OptionalArgs2<P0, P1> { ... }

impl<P0, P1> OptionalArgs2<P0, P1> for () // not strictly needed
    { ... }
impl<P0, P1, A0> OptionalArgs2<P0, P1> for (A0,) 
    where A0: Into<P0> { ... }
impl<P0, P1, A0, A1> OptionalArgs2<P0, P1> for (A0, A1)
    where A0: Into<P0>, A1: Into<P1> { ... }

// API

// *Every* method has this signature for all required parameters:
fn some_method(arg: i64);

// Those with default ones additionally have this:
fn some_method_ext(arg: i64, optionals: impl OptionalArgs2<String, bool>);

// Usage
some_method(32);
some_method_ext(32, ()); 
some_method_ext(32, ("hello")); 
some_method_ext(32, ("hello", true)); 

Pros:

• Syntax is as close as we can get to GDScript calls (at least without using macros).
• No forgetting of call() at the end.
• Compatible if a first default parameter is added to method (method + method_ext overloads).
• Compatible if another default parameter is added (OptionalArgs3 would still have impl for a 2-tuple).

Cons:

• Syntax may not feel very natural/idiomatic for some Rust devs.
• You can only omit arguments from the end of the list, not skip some.
• Not compatible if a required parameter becomes optional. This is also true for the named approach.
• Can be less readable for long argument lists, due to not having named arguments (IDE might help).

The ABI part is addressed in #973. I’ll try to implement the call-builder solution for the API part for v0.12. The plans to deal with previously unresolved questions (https://github.com/godot-rust/gdnative/issues/814#issuecomment-963308724) are:

• Builderized methods are named {original_name}_ex. “Raw” versions with full optional args are no longer generated. Currently, there are no methods with a _ex suffix in the Godot API. To guard against name collisions in the future, any Godot methods with names that end in _ex(_ex|_godot)+ will have an extra _godot appended to their names (and _godot_ex appended to their builderized versions).

foo
foo_ex

// Implausible names
foo_ex_godot
foo_ex_godot_ex
foo_ex_godot_ex_ex
foo_ex_godot_ex_ex_godot

foo (minimal version of `foo`)
foo_ex (builderized version of `foo`)
foo_ex_godot (minimal version of `foo_ex`)
foo_ex_godot_ex (builderized version of `foo_ex`)

// Implausible names
foo_ex_godot_godot (minimal version of `foo_ex_godot`)
foo_ex_godot_godot_ex (builderized version of `foo_ex_godot`)
foo_ex_godot_ex_godot (minimal version of `foo_ex_godot_ex`)
foo_ex_godot_ex_godot_ex (builderized version of `foo_ex_godot_ex`)
foo_ex_godot_ex_ex_godot (minimal version of `foo_ex_godot_ex_ex`)
foo_ex_godot_ex_ex_godot_ex (builderized version of `foo_ex_godot_ex_ex`)
foo_ex_godot_ex_ex_godot_godot (minimal version of `foo_ex_godot_ex_ex_godot`)
foo_ex_godot_ex_ex_godot_godot_ex (builderized version of `foo_ex_godot_ex_ex_godot`)

• Call-builders do nothing when dropped. However, they will be annotated with #[must_use], which should cause Rust to emit warnings unless they are explicitly ignored (probably not what the user wants).
• For the initial implementation, optional parameters must be provided in the correct order. This will hopefully be enforced statically (see below).
• For the initial implementation, ptrcall will only be used if all optional parameters are provided, and if the ptrcall feature is enabled. Non-builderized versions of methods will always use varcall under the hood, if any optional arguments exist, even if ptrcall is enabled as a feature.
• The type will have to be nameable for the sake of doc generation, but stability promises will be explicitly limited.

I expect the initial API to be something like:

#[must_use]
struct IsActionPressedEx<T1, T2 = ()> {
    action: T1,
    exact: T2,
}

impl SomeType {
    fn is_action_pressed_ex<T1>(&self, action: T1) -> IsActionPressedEx<T1, ()>
    where
        T1: Into<GodotString>,
    {
        IsActionPressedEx {
            action,
            exact: (),
        }
    }
}

impl<T1> IsActionPressedEx<T1, ()>
where
    T1: Into<GodotString>,
{
    fn exact(self, exact: bool) -> IsActionPressedEx<T1, bool> {
        IsActionPressedEx {
            action: self.action,
            exact,
        }
    }

    fn call(self) -> bool {
        todo!("use varcall")
    }
}

impl IsActionPressedEx<T1, bool>
where
    T1: Into<GodotString>,
{
    // shouldn't cause a conflict with the `()` impl above
    fn call(self) -> bool {
        todo!("use varcall or ptrcall depending on feature flags")
    }
}