gdext icon indicating copy to clipboard operation
gdext copied to clipboard

Default function parameters

Open you-win opened this issue 2 years ago • 24 comments

Allow for exposing default function parameters to GDScript.

The proposed syntax would look like:

#[godot_api]
impl MyType {
    #[func(defaults = [ 1 ]]
    fn some_func(a: Option<i32>) -> i32 {
        a.unwrap_or(1)
    }
}

This would map roughly to this in C++:

ClassDB::bind_method(D_METHOD("some_func", "a"), &MyType::some_func, DEFVAL(1));

Ideally the macro would also allow for function calls from Godot to automatically insert the default parameters as well so that Option<T> is not required, but I'm not sure if that's feasible.

WIP in #380

you-win avatar Aug 13 '23 06:08 you-win

Thanks! I'll answer here instead of the PR, to first discuss the design before delving into implementation.


1. Function vs. parameter attributes

I wonder if an attribute on the parameter itself is not more intuitive. There are multiple options to do it syntactically.

    #[func]
    fn some_func(
        #[opt(default = 20)] a: i32
    ) -> i32 {
        ...
    }

Here the advantage is not significant, but for long parameter lists and more complex expressions like Vector3i(1, 2, 3), it can make a big difference.


2. Option<T> vs T

Ideally the macro would also allow for function calls from Godot to automatically insert the default parameters as well so that Option<T> is not required, but I'm not sure if that's feasible.

I think that should be possible. Maybe both can be allowed:

    #[func]
    fn some_func(
        #[opt] a: Option<i32>,
        #[opt = "str"] s: GodotString,
    ) -> i32 {
        let a = a.unwrap_or(1);
    }

Magic needed?

First I was thinking to always infer Option as "optional", but that's probably not a good idea because 1) it's not explicit, 2) for some types (objects) it represents nil, not "optional".

But then, do we really need default values to be encoded inside an attribute? Instead of:

    #[func]
    fn some_func(
        #[opt = 20] a: i32,
        #[opt = 15] s: i32,
    ) -> i32 {
        ...
    }

why not this?

    #[func]
    fn some_func(
        #[opt] a: Option<i32>,
        #[opt] b: Option<i32>,
    ) -> i32 {
        let a = unwrap_or(20);
        let b = unwrap_or(15);
    }

While a bit more verbose, it's immediately clear what happens and also allows for more complex relations, for example:

    #[func]
    fn some_func(
        #[opt] a: Option<i32>,
        #[opt] b: Option<i32>,
    ) -> i32 {
        let a = a.or(b).unwrap_or(44);
    }

Bromeon avatar Aug 13 '23 07:08 Bromeon

Regarding your points:

1. Function vs. parameter attributes

I agree, this looks better syntactically. It does require additional verification during parsing, since ~~I believe~~ Godot displays default values like:

(edit: verified it works like this, replaced example with one from godot-cpp's test project)

C++ binding

ClassDB::bind_method(D_METHOD("def_args", "a", "b"), &Example::def_args, DEFVAL(100));

GDScript Doc

int def_args(a: int, b: int = 100)

So if the attribute were implemented per parameter, it would also be necessary to check that optionals are consecutive and that all required parameters come first. It's definitely doable and more ergonomic but also increases parsing complexity.

2. Option<T> vs T

Spliting this into 2 subpoints:

2.1 Inserting the default values when they are not provided by Godot

I like the #[opt = "str"] syntax, however I'm trying to envision how this data would be registered in varcall. I think it's definitely possible like with how parameters are automatically placed into a tuple, but I'm not seeing it quite yet.

2.2. Magically inferring the defaults

I think this would be great, but at some point, doesn't this mean the proc macro would have to contain its own mini Rust parser? This is because the method bind function requires explicit defaults to be passed.

Given the cases below:

fn some_func(&self, #[opt] first: Option<i32>, #[opt] second: Option<i32>) {
    // Probably easy to extract the default
    // Note that this is the second parameter
    let second = second.unwrap_or(44);

    // How to automatically extract the default here?
    // The method bind will be incorrect if the default cannot be determined, so throw an error instead?
    other_related_func_that_takes_option(first);

    // This would also be difficult to determine
    let first = first.unwrap_or_else(|| self.stored_fn_call());
    // Different ways of providing a default
    let first = match first {
        Some(v) => v,
        None => 2,
    }
    let first = if let Some(v) = first {
        v
    } else {
        3
    };
    let first = if second.is_some() {
        second.unwrap();
    } else {
        4
    };
}

Magically determining the default could be restricted to only certain ways of handling options (e.g. the option must be handled using unwrap_or at the top of the fn body), but that doesn't feel great and would be potentially confusing if one didn't realize that's how default params are handled.

I think it would be magical and incredibly ergonomic if defaults could be determined automatically, but I think it would also be more trouble than it's worth.

you-win avatar Aug 13 '23 13:08 you-win

i feel like Option<T> is a bit weird to symbolize a default argument, shouldn't this just mean that you can pass in either nil or a value of type T? so shouldn't an argument of type i32 with a default value still just be an i32. since you'll always get some value of type i32? the main difference here would be that you can't tell whether you were passed a value explicitly or not, but like you can't tell that in gdscript either.

i feel like default values should just use the Default impl with an option to override it. We could maybe also make a type like

struct DefaultI32<const N: i32> {
  value: i32
}

impl<const N: i32> Default for DefaultI32<N> {
  fn default() -> Self {
    Self { value: N }
  }
}

for some cases.

lilizoey avatar Aug 13 '23 17:08 lilizoey

why not this?

    #[func]
    fn some_func(
        #[opt] a: Option<i32>,
        #[opt] b: Option<i32>,
    ) -> i32 {
        let a = unwrap_or(20);
        let b = unwrap_or(15);
    }

While a bit more verbose, it's immediately clear what happens and also allows for more complex relations, for example:

    #[func]
    fn some_func(
        #[opt] a: Option<i32>,
        #[opt] b: Option<i32>,
    ) -> i32 {
        let a = a.or(b).unwrap_or(44);
    }

this goes against rust philosophy. in rust, the signature of a function generally does not depend on the body of a function (yes there is impl Trait but that's basically the one exception). and here you're suggesting we change the signature of a function we expose to godot to depend on the body of the function. i do not like that. neither gdscript nor rust does this, i dont think we should either.

lilizoey avatar Aug 13 '23 18:08 lilizoey

i feel like Option<T> is a bit weird to symbolize a default argument, shouldn't this just mean that you can pass in either nil or a value of type T?

The difficulty with not using Option<T> is that it's impossible to tell if a developer-provided default should be used. i.e. i32::default() is just 0, but 0 is also a valid input.

The wrapper type for Default makes sense, but I wonder if there's a better way.

the main difference here would be that you can't tell whether you were passed a value explicitly or not, but like you can't tell that in gdscript either

This is possible in Rust (and C++ with vararg methods) since Godot passes along an arg_count parameter for varcalls. With how varcalls are implemented in gdext, the expected parameter count is also available when converting params from Godot -> params for Rust.

My WIP PR modifies several signatures to pass along the arg_count, but what to do afterwards is still an open question. Whether it be a wrapper type for Default, inserting default values into the varcall arg converter somehow, requiring default params to be Option<T>, or something else.

you-win avatar Aug 13 '23 19:08 you-win

i feel like Option is a bit weird to symbolize a default argument, shouldn't this just mean that you can pass in either nil or a value of type T?

The difficulty with not using Option<T> is that it's impossible to tell if a developer-provided default should be used. i.e. i32::default() is just 0, but 0 is also a valid input.

I'm not sure why that's needed?

the main difference here would be that you can't tell whether you were passed a value explicitly or not, but like you can't tell that in gdscript either

This is possible in Rust (and C++ with vararg methods) since Godot passes along an arg_count parameter for varcalls. With how varcalls are implemented in gdext, the expected parameter count is also available when converting params from Godot -> params for Rust.

My WIP PR modifies several signatures to pass along the arg_count, but what to do afterwards is still an open question. Whether it be a wrapper type for Default, inserting default values into the varcall arg converter somehow, requiring default params to be Option<T>, or something else.

Yeah it is technically possible in rust, but why would we want people to be able to treat the case of "no argument passed so use the default value" and "default value explicitly passed to the function" as different? to me that honestly seems rather weird and unintuitive. Like i'd expect that those two cases behave identically anyway.

lilizoey avatar Aug 13 '23 19:08 lilizoey

i feel like Option is a bit weird to symbolize a default argument, shouldn't this just mean that you can pass in either nil or a value of type T?

The difficulty with not using Option<T> is that it's impossible to tell if a developer-provided default should be used. i.e. i32::default() is just 0, but 0 is also a valid input.

I'm not sure why that's needed?

It's needed since the dev could define a different default from what Rust considers Default.

the main difference here would be that you can't tell whether you were passed a value explicitly or not, but like you can't tell that in gdscript either

This is possible in Rust (and C++ with vararg methods) since Godot passes along an arg_count parameter for varcalls. With how varcalls are implemented in gdext, the expected parameter count is also available when converting params from Godot -> params for Rust.

My WIP PR modifies several signatures to pass along the arg_count, but what to do afterwards is still an open question. Whether it be a wrapper type for Default, inserting default values into the varcall arg converter somehow, requiring default params to be Option<T>, or something else.

Yeah it is technically possible in rust, but why would we want people to be able to treat the case of "no argument passed so use the default value" and "default value explicitly passed to the function" as different? to me that honestly seems rather weird and unintuitive. Like i'd expect that those two cases behave identically anyway.

Rust doesn't support default params, so there needs to be a way to determine if a default should be used. If the standard Default impl was used, there would be no way to tell in the Rust fn if this is developer input or Default::default().

That's why I brought up Option<T> as a potential solution, which means the default value would be defined in the actual fn body. Other solutions, like a Default wrapper type, would insert the developer's default value during the varcall invocation. Although thinking about it more, the wrapper type wouldn't be able to support defaults per function, which is something that Godot supports.

For example:

Using Default (the current implementation):

unsafe fn varcall_arg<P: FromVariant + Default, const N: isize>(
    args_ptr: *const sys::GDExtensionConstVariantPtr,
    arg_count: isize,
    method_name: &str,
) -> P {
    if arg_count >= N {
        return P::default();
    }
    let variant = &*(*args_ptr.offset(N) as *mut Variant); // TODO from_var_sys
    P::try_from_variant(variant)
        .unwrap_or_else(|_| param_error::<P>(method_name, N as i32, variant))
}

#[func(defaults = [4]]
fn foo(a: i32) {
    ... do stuff ...
}

#[func(defaults = [10]]
fn bar(a: i32) {
    ... how to handle this case? ...
}

vs

Using Option (assuming None is passed if a parameter is missing, using the current macro implementation):

unsafe fn varcall_arg<P: FromVariant, const N: isize>(
    args_ptr: *const sys::GDExtensionConstVariantPtr,
    arg_count: isize,
    method_name: &str,
) -> P {
    if arg_count >= N {
        return None;
    }
    let variant = &*(*args_ptr.offset(N) as *mut Variant); // TODO from_var_sys
    P::try_from_variant(variant)
        .unwrap_or_else(|_| param_error::<P>(method_name, N as i32, variant))
}

#[func(defaults = [4]]
fn foo(a: Option<i32>) {
    let a = a.unwrap_or(4);
}

#[func(defaults = [10]]
fn bar(a: Option<i32>) {
    let a = a.unwrap_or(10);
}

The point is, it's possible to tell where default values should be used. The question is, where should they be handled. I should also mention that Godot does not send any default values, it just sends less parameters.

you-win avatar Aug 13 '23 20:08 you-win

i feel like default values should just use the Default impl with an option to override it. We could maybe also make a type like

struct DefaultI32<const N: i32> {
  value: i32
}

impl<const N: i32> Default for DefaultI32<N> {
  fn default() -> Self {
    Self { value: N }
  }
}

The Rust Default trait keeps getting mixed up with anything that has "default" in the name, but it really doesn't fit here.

Traits have one possible implementation per type. What we want is one implementation per parameter.

I don't think hacking this together via const generics would be the way to go 😉


[you-win] I think this would be great, but at some point, doesn't this mean the proc macro would have to contain its own mini Rust parser? This is because the method bind function requires explicit defaults to be passed.

[lilizoey] this goes against rust philosophy. in rust, the signature of a function generally does not depend on the body of a function (yes there is impl Trait but that's basically the one exception). and here you're suggesting we change the signature of a function we expose to godot to depend on the body of the function. i do not like that. neither gdscript nor rust does this, i dont think we should either.

You both misunderstood that or I didn't express myself clearly; we would not analyze the body to infer any default arguments.

The idea was, since Rust can detect whether a value was provided or not, we could use this to pass Some(val) or None to a user function accepting Option<T>. This could be covered by a simple unwrap_or() and would avoid more proc-macro DSL. No value appearing in the body is advertised.

But there are some caveats, you mentioned some of them already:

  • We cannot display a default value in GDScript docs (or even choose a meaningful one).
  • It's not obvious for the caller that passing nothing is different from passing the default value. In other words, the system works a bit different from other GDScript functions with default parameters, which may be confusing.
  • There is potential confusion with null.

So it's probably better to build something with #[opt]. However I'd like to keep complexity low -- too much magic in proc-macros has a lot of downsides.

Bromeon avatar Aug 13 '23 20:08 Bromeon

The idea was, since Rust can detect whether a value was provided or not, we could use this to pass Some(val) or None to a user function accepting Option<T>.

How would val be defined if the only macro is #[opt]? val needs to be defined somewhere.

We cannot display a default value in GDScript docs (or even choose a meaningful one).

We can, I mentioned that it's possible in an earlier comment. The only issue is that they are displayed in reverse list order, if that makes sense.

you-win avatar Aug 13 '23 20:08 you-win

Let's say we forget about Option, Default, wrapper types or anything else.

Based on my previous suggestion, what about this?

#[func]
fn some_func(
    &self,
    required: i32,
    #[opt = -20] optional: i32,
    #[opt = "hello"] optional2: GodotString,
) {
    // All parameters have defined values here, in all cases.
}

Could be called from GDScript as:

obj.some_func(1)
obj.some_func(1, 2)
obj.some_func(1, 2, "hi")

We'd need to keep in mind that if we do it via proc-macros, there's a limit to how complex the expressions can get, at least if they should stay debuggable. But it's probably an OK trade-off.

Bromeon avatar Aug 13 '23 20:08 Bromeon

How would val be defined if the only macro is #[opt]? val needs to be defined somewhere.

That's a good point, but also there's a point in @lilizoey's argument that it should be possible for a GDScript caller to pass an argument that has the same semantics as the default value. And there may be cases where Option<T> parameters can be assigned null arguments.

So I'm not sure if we'd even want #[opt] stand-alone.

We can, I mentioned that it's possible in an earlier comment. The only issue is that they are displayed in reverse list order, if that makes sense.

I meant that we can only do that if the default value is part of the signature (including attribute). If it's in the body as unwrap_or, we can't.

Bromeon avatar Aug 13 '23 20:08 Bromeon

Rust doesn't support default params, so there needs to be a way to determine if a default should be used. If the standard Default impl was used, there would be no way to tell in the Rust fn if this is developer input or Default::default().

That's why I brought up Option<T> as a potential solution, which means the default value would be defined in the actual fn body. Other solutions, like a Default wrapper type, would insert the developer's default value during the varcall invocation. Although thinking about it more, the wrapper type wouldn't be able to support defaults per function, which is something that Godot supports.

I think we're talking past each other a bit maybe. Yes, we'll likely need to use Option<T> somewhere, for instance to communicate whether a default argument should be used or not. What im arguing is that the creator of the function shouldn't define their function to take an Option<T> to signal this. (of course unless they actually intend None to be a valid value for this argument)

We would simply have some code in-between the user-defined function and the godot call. But the creator of the function would not see any of this. To them a default argument is just that, a default for when the argument is left out. I dont remember exactly where this would be in the code gen, but we already dont just pass the function pointers directly to godot to be called, we can code-gen the default argument value in between there.

lilizoey avatar Aug 13 '23 20:08 lilizoey

Let's say we forget about Option, Default, wrapper types or anything else.

Based on my previous suggestion, what about this?

#[func]
fn some_func(
    &self,
    required: i32,
    #[opt = -20] optional: i32,
    #[opt = "hello"] optional2: GodotString,
) {
    // All parameters have defined values here, in all cases.
}

Could be called from GDScript as:

obj.some_func(1)
obj.some_func(1, 2)
obj.some_func(1, 2, "hi")

We'd need to keep in mind that if we do it via proc-macros, there's a limit to how complex the expressions can get, at least if they should stay debuggable. But it's probably an OK trade-off.

should #[opt] optional: i32 be allowed? if so then i feel it'd make sense for that to be equivalent to #[opt = i32::default()] optional: i32

lilizoey avatar Aug 13 '23 20:08 lilizoey

should #[opt] optional: i32 be allowed? if so then i feel it'd make sense for that to be equivalent to #[opt = i32::default()] optional: i32

I would probably start without it, as it tends to weaken type safety (people abuse empty strings etc. to denote absence of a value).

Syntax is also up to bikeshedding 😀 for fields, we already have #[init(default = ...)], and generally, our KvParser doesn't allow top-level key-value pairs like #[opt = value]. Maybe we could make an exception for parameters, but I'd also like to explore some alternatives 🤔

Bromeon avatar Aug 13 '23 20:08 Bromeon

That's a good point, but also there's a point in lilizoey's argument that it should be possible for a GDScript caller to pass an argument that has the same semantics as the default value. And there may be cases where Option<T> parameters can be assigned null arguments.

Okay that's becoming more clear. There are instances where GDScript null could be passed as a valid parameter.

I dont remember exactly where this would be in the code gen, but we already dont just pass the function pointers directly to godot to be called, we can code-gen the default argument value in between there.

As mentioned previously, the codegen is here for parameters, so inserting user-defined defaults here might make sense.

should #[opt] optional: i32 be allowed? if so then i feel it'd make sense for that to be equivalent to #[opt = i32::default()] optional: i32

If that's meant to be shorthand for "use whatever the Rust default is", I'm for it. I just don't want this case to be forgotten (using new possible syntax):

fn foo(
    #[opt(1)] a: i32, // defaults to 1
    #[opt(2)] b: i32, // defaults to 2
    #[opt] c: i32 // defaults to 0
) {}

you-win avatar Aug 13 '23 20:08 you-win

That's a good point, but also there's a point in lilizoey's argument that it should be possible for a GDScript caller to pass an argument that has the same semantics as the default value. And there may be cases where Option parameters can be assigned null arguments.

Okay that's becoming more clear. There are instances where GDScript null could be passed as a valid parameter.

I dont remember exactly where this would be in the code gen, but we already dont just pass the function pointers directly to godot to be called, we can code-gen the default argument value in between there.

As mentioned previously, the codegen is here for parameters, so inserting user-defined defaults here might make sense.

The codegen i was talking about is in fact here. since this generates something like:

|_, params| {
  let (a,) = params;
  foo(a)
}

and in here we can just have this become something like

|_, params, default_params| {
  let (a,) = params;
  let (b,c,...) = default_params;
  foo(a, b.unwrap_or(b_default), c.unwrap_or(c_default), ...)
}

where the b_default and c_default and so on are the values defined in #[opt = b_default], and the values in the default_params tuple are all Option<T> for each parameter.

lilizoey avatar Aug 13 '23 21:08 lilizoey

That makes sense. So we can split the params tuple by required/optional, and then in varcall, implicitly wrap every default param in Option?

you-win avatar Aug 13 '23 21:08 you-win

That was my idea at least, i definitely do think we should treat required and optional arguments separately. Since i feel like it'd be very messy to try to unify them into a single list of arguments somehow.

lilizoey avatar Aug 13 '23 21:08 lilizoey

What I like about Option<T> to signal that your argument is optional and a default value will be provided, is that it's rather intuitive. I admit that having func(1) and func(1,null) having the same behavior is a bit strange but semantically I think it's alright, passing nothing or null gives the same result, default argument

I imagine if y'all are attached to #[opt] is that it makes it clear that macro magic is happening? I wonder if that's necessary, we're already inside a #[godot_api] annotation, I would expect Option in a method signature to mean optional in gdscript

astrale-sharp avatar Oct 24 '23 11:10 astrale-sharp

What I like about Option<T> to signal that your argument is optional and a default value will be provided, is that it's rather intuitive.

A subtle difference exists though: Option currently means "nullable", not "possibly absent".

There are definitely cases where people want it explicit:

    ship.set_target(null)
    # and not:
    ship.set_target()

Also, Option<T> can appear anywhere in a #[func] parameter list. It would be strange if it can be omitted when at the end, but not somewhere in between.

Something along #[opt] looks nice. The #[opt = defaultValue] or #[opt(default = defaultValue)] was suggested, but the main advantage besides ergonomics is that the default value appears in GDScript docs? Edit: there seems to be another one:

// a)
#[func] fn set_integer(
    #[opt = 23] i: i32
);

// b)
#[func] fn set_integer(
    #[opt] i: Option<i32>
);

a) could only be called with int (or no argument) from GDScript, while b) could also be called with null.

Otherwise we can also start simple, let the user use unwrap_or() and only extend the API on demand.

Bromeon avatar Oct 24 '23 11:10 Bromeon

A subtle difference exists though: Option currently means "nullable", not "possibly absent".

I didn't think about that, I'm convinced.

#[func] fn set_integer(
    #[opt(default = defaultValue)] i: i32
);

#[func] fn set_integer(
    #[opt] i: Option<i32>
);

I like these two options best, let's definitely start simple though, I know how it is with proc macros now and I feel like small PRs are more easily made with quality~~

astrale-sharp avatar Oct 24 '23 21:10 astrale-sharp

implementation wise, should I try to have a modified varcall_arg function named varcall_arg_default -> Option<R> so that if there is nothing at *args_ptr.offset(N) (which is to say that args_len < N) it then returns None?

Then PtrcallSignatureTuple::in_ptrcall, if #[opt] was present uses that function?

astrale-sharp avatar Oct 26 '23 16:10 astrale-sharp

Any news on this feature?

Zitronenjoghurt avatar Aug 12 '24 12:08 Zitronenjoghurt

No 🙂 it's not a priority for the coming versions, so unless someone in the community comes up with a PR, it's probably not happening anytime soon. In that case, we should probably finalize the design first.

Bromeon avatar Aug 12 '24 15:08 Bromeon