GDScript: Add support for variadic functions
- Closes godotengine/godot-proposals#1034.
Production edit: closes godotengine/godot-roadmap#65
Does this support typed arrays as well?
Does this support typed arrays as well?
Currently no. I'm not sure if we should support this, as it would make it harder to check that the number and types of arguments are contravariant.
I understand. I feel we should aim to support it eventually, as it will otherwise leave out users that seek to write type safe gdscript.
How I missed this! I just wanted to suggest this today
Would love to have this. I can't say how many times I abstracted some collection of features away, just to be forced to pass named arguments at specific positions, or abstract the names and use those variably. Can't wait to have this.
@Blade67 Please don't bump issues without contributing significant new information. Use reactions on the first post instead. We are currently focused on the upcoming 4.2 release, which will not include this feature. After that I'm going to continue working on this feature.
If you work just with GDScript, there are some easy workarounds regarding vararg functions. But if you're using GDExtension this is a big deal. I like to communicate with GDScript with the call function:
node->call("functionName");
If I want to pass a variable number of parameters, I can't unless I create an Array. But the most annoying thing in GDExtension is that you just can't initialize arrays or Dictionaries directly, So to achieve this I need to use "append" which is a pain:
Array args;
args.append(arg1);
args.append(arg2);
args.append(arg3);
...
node->call("functionName", args)
With this commit it will be like this:
node->call("functionName", arg1, arg2, arg3, ...)
I decided to split this PR into two parts:
- rest parameter allows you to declare variadic functions (this PR);
- spread syntax allows you to unpack arrays when calling functions and constructing arrays.
With this PR only you won't be able to do:
func vararg_func(x: int, y: int, ...args: Array) -> void: # Rest parameter.
other_vararg_func(x, y, ...args) # Spread syntax.
But you will be able to do:
func vararg_func(x: int, y: int, ...args: Array) -> void: # Rest parameter.
other_vararg_func.callv([x, y] + args)
Now I'm working on the spread syntax. I'm not sure if we could merge just rest parameter for 4.3 and leave the spread syntax for 4.4. But in any case, two PRs are more convenient for review.
As for supporting typed arrays, in addition to the problem mentioned above, there is an obstacle that MethodInfo does not support rest parameter (name and type), only the METHOD_FLAG_VARARG, so the type would not appear in the docs and in editor tooltips (in some cases). This is probably not critical, but I would prefer to leave it for the future.
Finally, there is the question: should we introduce a new token ... or could we reuse .., which is used in match patterns. They have a similar purpose (.. do not allow you to declare a variable for packing rest array elements and dictionary key/value pairs). For variadic functions languages usually use ... (C++, JavaScript, PHP), plus in the documentation we use ellipsis too. Also, match patterns are more like declarations, the question arises what will happen if we want to add spread syntax for patterns (like [var x, ...array, ..var rest]).
I would vote in favor of waiting to merge until the spread syntax is finished. In my opinion, varargs feels like an "incomplete" feature unless there is some sort of spreading syntax.
Regarding waiting for a spread (splat) operator before merging, I don't think a small amount of syntax-asymmetry here is a big problem given that this is a technical feature, which will have technical users. Though getting a spread operator merged at the same time as the ...args syntax for declaring variadic (varargs) functions might be nice, people have been waiting nearly 4 years at this point, and the ability to declare variadic functions is more important than just making Object.callv() calls prettier.
(I found this issue looking for a simple way to generically connect to signals, since they can be emitted with any number of objects passed along to provide extra info.)
Talked about in a GDScript meeting. This kind of feature would nicely fit in a 4.4 release. We need to review it first, though. cc. @HolonProduction @vnen
Also I'm a bit conflicted about how varargs show in the documentation. On one side it's in line with print, but on the other side not showing the param name could get confusing for users writing doc comments:
Also I'm a bit conflicted about how varargs show in the documentation. On one side it's in line with
I think it should show the name. I would argue even the builtin functions should have a name attached to the rest parameter (but it's not a requirement for this PR).
I don't know how to feel about the additional vararg in the class reference. It's repetitive and it feels like args plus ellipsis conveys that information more intuitively.
I feel like you the tooltip should be put on args and/or ellipsis instead.
I think it should show the name. I would argue even the builtin functions should have a name attached to the rest parameter (but it's not a requirement for this PR).
Done:
func f(...test_args):
pass
func g(...test_args: Array):
pass
For native methods ...args: Array is used (but I'm not sure if that's right):
I do reiterate the sentiment that the vararg existing alongside ...args: Array feels needlessly repetitive, now.
I have multiple thoughts on how to address it. I'll use call()'s signature for the examples:
call(method: StringName, ...args: Array)The tooltip is moved to...args.call(method: StringName, ...args)Same as above, withoutArray. This makesargsstand out more. It also prevents readers mistakingargsas an actual array argument to pass into the function, which I foresee. The general convention and tooltip may make the intention clearer.call(method: StringName, ...) varargsSame as it is prior to this PR, but this format should only apply to built-in functions. Would be a problem if virtual, vararg functions are eventually implemented.
call(method: StringName, ...) varargs
In my opinion, ... without a name to the varargs should be avoided, because it may be unclear what the additionally supplied arguments will do. Example:
multiply(...)
This, in my opinion, does not sufficiently convey what is being passed into the function.
multiply(...factors)
This makes it much clearer.
However, I think this is a bit outside the scope of the PR to change.
In my opinion,
...without a name to the varargs should be avoided, because it may be unclear what the additionally supplied arguments will do.
The documentation has already been changed as you suggest, see the comment above. Note that core and extension methods do not support rest parameter name, so hardcoded args was used. Maybe we should leave the lone ... when the rest parameter name is unknown, but then there will be inconsistency between native and GDScript methods.
The Milestone says 4.4, is this still planned for 4.4?
The Milestone says 4.4, is this still planned for 4.4?
Nope, the window for new features was closed with the 4.4 beta. The milestone just hasn't been updated yet.
In my opinion,
...without a name to the varargs should be avoided, because it may be unclear what the additionally supplied arguments will do.The documentation has already been changed as you suggest, see the comment above. Note that core and extension methods do not support rest parameter name, so hardcoded
argswas used. Maybe we should leave the lone...when the rest parameter name is unknown, but then there will be inconsistency between native and GDScript methods.
I would like to add that I'm still in the opinion to omit the array parameter's name for non-virtual, variadic functions.
There's no point showing it because the user cannot override it or directly make use of it. There's already a lot of hints in the class reference. The ellipsis, the varang that can be hovered on, and the description oftentimes mentions the variadic nature of the function of interest.
Is there any chance for this to be added to 4.5?
That would genuinely be the idea, yes. There's not much else blocking this, other than appearances in the class reference. This is desired
Rebased just in case.
The comedic timing of this needing another rebase lmao
Thanks! Excellent work on this PR; thrilled to see this make it in before the feature freeze!
@dalexeev how to call overwritten variadic functions?
class Foo extends RefCounted:
func vararg(...args: Array) -> void:
prints(args)
class Bar extends Foo:
func vararg(...args: Array) -> void:
super(args)
func test_foo() -> void:
Bar.new().vararg(1,2,3)
``
using `callv` is not an option
Indeed, when using super(args) the entire argument array from Bar::vararg is passed as a single argument to Foo::vararg. Maybe this was overlooked while implementing the feature? It would be nice if Callable had a member property super or base referencing the method of the base class, so that we could use vararg.super.callv(args) or vararg.base.callv(args).
@dalexeev how to call overwritten variadic functions?
class Foo extends RefCounted: func vararg(...args: Array) -> void: prints(args) class Bar extends Foo: func vararg(...args: Array) -> void: super(args) func test_foo() -> void: Bar.new().vararg(1,2,3) `` using `callv` is not an option
Maybe this is where we need a spread operator to pass unpacked args array to the super? @dalexeev mentioned they have some draft working in progress https://github.com/godotengine/godot/issues/89916#issuecomment-3322367524
Unfortunately, we've completely missed this case. We can fix it either by introducing spead syntax or by making it possible to get the parent method as a Callable using the super.func_name syntax (without parentheses). Then you'll be able to use callv().
I'll try to finalize my local draft of the spread syntax and look into the feasibility of implementing the second option soon. But we probably won't see a fix until 4.6. In the case of spread syntax, it's a large and complex feature that definitely can't be included in a patch release. The second option, which introduces getting the super method as a Callable, could potentially be considered for a 4.5.x patch, but it's still technically a new feature, even if it's much simpler than the first option.
I'm very sorry for the inconvenience, I completely forgot about this case, assuming that you can use callv() everywhere (native and custom methods of objects, methods of built-in Variant types, global utility functions, lambdas, and other Callable types).