godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.16k stars 97 forks source link

Discard additional arguments in a connected signal's callback function #7258

Open pckv opened 1 year ago

pckv commented 1 year ago

Describe the project you are working on

This applies to all of my projects.

Describe the problem or limitation you are having in your project

There are several cases where I subscribe to a signal where I don't need any arguments in the callback, e.g the area_entered signal of Area2D. Or I just need the first argument of a custom signal I made, such as:

signal count_changed(count: int, prev_count: int)

func _ready():
    count_changed.connect(_on_count_changed)
    count_changed.emit(2, 1)   # fails during runtime

func _on_count_changed(count: int):
    print(count)

Currently this can be solved by unbinding arguments from the signals:

signal count_changed(count: int, prev_count: int)

func _ready():
    count_changed.connect(_on_count_changed.unbind(1))

func _on_count_changed(count: int):
    print(count)

I would prefer to be able to hook to signals without even knowing their signature if all I need is the notification of an event, but this implementation is fine. It works and it's readable.

The real issue, however, is when the signature of a signal is modified. Signals are untyped (the arguments provided in the signal's constructor are just annotations used by the editor and for readability), which means there is no error in the compiler for passing more arguments than are defined as parameters in the callback's signature.

As an aside, the emit function can also receive any number of arguments, which doesn't make any sense to me as the subscribers to a signal should always expect the number of arguments to be the same. With the current implementation of signals requiring all arguments to be defined in the callback, this would result in a runtime error unless the subscribed callback's aren't swapped out with functions that require another number of arguments.

I have personally experienced defining a signal such as signal count_changed(count: int), subscribing to this from multiple paths in a large project, and then refactoring the signal to signal count_changed(count: int, prev_count: int). The game still builds and runs, but will fail at runtime when this signal emits. You have to manually find all the code-paths where it is connected and update the signatures accordingly. Which, admittedly, is not a big task, but the runtime error has confused me for far too long periods before I understood where the issue was.

While the runtime error that occurs in the emit signal does provide a reference to the callback (by reference to the c++ function), this is something that's tedious to stumble on during runtime in case you forgot to update all connected callbacks by adding arguments you most likely won't even be using in them.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

My main proposal is to discard any additional arguments provided to a signal's callback. This means that a signal emitting two arguments could send only the first argument to a connected callback which defined only one parameter. This solution also allows for callbacks with zero arguments being used on signals that emit with arguments.

If possible, I believe this is the best solution. Updating a signal no longer breaks existing code, which makes them backwards compatible. This would also allow for built-in signals to extend their signatures and emit new arguments without breaking any existing projects. I might be overlooking something here, but currently I see no issues with this approach. Signals remain untyped, which would be fine as any subscription to this signal would already use the arguments it needs, and all new arguments are optional.

I have no experience with the internals of Godot so I'm not quite sure how doable this is as functions always require all non-optional arguments to be passed. This change might require modifying how functions work entirely, which could not be feasible. For reference, JavaScript discards additional arguments provided to functions, but I'm not sure of any other languages.


A secondary solution would be to strongly type the signals. This also seems like a lot of work, but if the signal's arguments must be required I believe this is a healthy addition to the compiler. Any changes to a signal would now throw compiler errors in the specific code-paths that connect to it, and to every code-path that emits the signal.

This might potentially break any code where the emit function of a signal is passed with varying number of arguments (as explained in the aside in the previous section). But this is only possible if all current listeners define the same number of arguments as when the emit method is called, i.e unsubscribing a callback with 2 arguments before emitting with 1 argument. I don't see any sensible reason anyone should do this.

Regardless if the main proposal were to be implemented or not, at least checking types in the emit method of a signal should be implemented. Not having typing here leads to runtime errors that really should be caught by the compiler.

I must note that I am not aware of whether typed signals are already planned as I couldn't find any open nor closed issues on this, but it feels like an obvious missing feature to the way the system currently works.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

Main proposal: optional callback arguments

Connecting to a signal should not require any function parameters be defined in the callback, and any additional arguments that are passed to this function by the signal should be discarded.

func _ready():
    Area2D.area_entered.connect(_on_area_entered)

# callback defines no parameters, so the `area: Area2D` argument 
# passed by the signal will be discarded
func _on_area_entered():  
    # ...

As for my main issue, the following change should not break any existing code.

- signal count_changed(count: int)
+ signal count_changed(count: int, prev_count: int)

- count_changed.emit(count)
+ count_changed.emit(count, prev_count)

Full example:

# prev_count can be added later in production with zero breaking
# changes to the code. imagine it being used in many different
# places
signal count_changed(count: int, prev_count: int)

func _ready():
    count_changed.connect(_on_count_changed)
    count_changed.emit(2, 1)  # updated to emit two arguments

# callback defines only one parameter and will accept `count: int`, 
# but `prev_count: int` will be discarded
func _on_count_changed(count: int):
    print(count)

Secondary proposal: typed signals

Signals should be strongly typed unless the main proposal is implemented.

The emit method of a signal should act as a function call and types should be compared if explicitly set.

Image of `Signal.emit` being supplied two arguments when the signal only declares one, and Godot throws an error

The connect method of a signal should verify the signature of the provided callback (this would have to be handled after any additional bind or similar method calls on the referenced callback). Keep in mind this proposal is not applicable if signal arguments are to become optional as per my main proposal.

Image of `Signal.connect` being given a callback with only one argument when the signal declares two, and Godot throws an error

If this enhancement will not be used often, can it be worked around with a few lines of script?

Signals can easily be connected with fewer arguments by calling the .unbind on the callback.

There is no solution from my knowledge that doesn't break the code unexpectedly if a signal emits more arguments than expected by the collective subscribed callback's signatures.

Is there a reason why this should be core and not an add-on in the asset library?

This is a core part of GDScript.

Calinou commented 1 year ago
dalexeev commented 1 year ago

Signal signatures are nominal, both the argument count and argument types. The emit() method does not check the actual arguments. It's more of a metadata for the documentation and editor to generate the callback (adding type hints is still not implemented for GDScript). You should match the signal signature, but you are not required to:

signal s(a, b)

func f(a):
    print(a) # Prints "a".

func _run():
    s.connect(f)
    s.emit("a")

It is possible to connect a method with a different number of required arguments to the signal, use the bind() and unbind() methods:

signal s(a, b)

func f(a):
    print(a) # Prints "a".

func _run():
    s.connect(f.unbind(1)) # Unbinds 1 argument.
    s.emit("a", "b")

If you want to reflect the fact that a signal can be emitted with a different number of arguments, then in theory we could add support for optional signal arguments (like signal my_signal(arg: int, opt_arg: int = 1)). But I'm afraid that this will further increase the confusion. Users might think that the omitted optional arguments will automatically be added when the emit() method is called. But this is not the case, the only difference would be that when a signal is connected in the editor interface, a callback with optional arguments would be generated.

pckv commented 1 year ago

Thanks for the reply!

I am aware of how signals work and I already address everything you said in my issue. I ask that you please read the parts where I'm concerned about this design and why I propose to change it.

Signal signatures are nominal, both the argument count and argument types. The emit() method does not check the actual arguments. It's more of a metadata for the documentation and editor to generate the callback (adding type hints is still not implemented for GDScript). You should match the signal signature, but you are not required to:

To me it doesn't make sense that the emit() method even allows a different number of arguments if the callback must always match the signature anyway. I outline this in my issue where if you pass varying number of arguments to emit(), the listeners will fail with an error since they never expect these number of arguments to suddenly be different. My secondary proposal of using typed signal signatures should also solve this.

It is possible to connect a method with a different number of required arguments to the signal, use the bind() and unbind() methods

Absolutely, and I also explain this in my issue and I don't have many problems with this. I rather believe that a simpler design which also solves further issues with future changes to a signal is to just discard the extra arguments provided to the callback. Another way of thinking about this is to automatically unbind the number of function parameters omitted in the callback. This solution would by design make the unbind() method call useless, as you wouldn't need to unbind parameters anymore.

To reiterate, my proposal would enable the following change to a signal:

# signal signature changes (currently has no effect on runtime 
# or compiler as these are just annotations)
- signal count_changed(count: int)
+ signal count_changed(count: int, prev_count: int)

# by design one would then naturally pass prev_count in the 
# emit() function, so...
- count_changed.emit(count)
+ count_changed.emit(count, prev_count)

This wouldn't break any existing code anywhere if all arguments of connected callbacks were optional.

If you want to reflect the fact that a signal can be emitted with a different number of arguments, then in theory we could add support for optional signal arguments. But I'm afraid that this will further increase the confusion. Users might think that the omitted optional arguments will automatically be added when the emit() method is called.

I might be naive but I honestly don't understand how this would be confusing to the user. I'm proposing that all arguments in a signal are optional. You would only need to enter the number of arguments that you need. I don't see how this could break existing code either, but I do explore at least one potential breaking point in my issue.

dalexeev commented 1 year ago

Thanks for the clarification. Sorry for not reading your proposal in full, I just noticed this fragment at first sight:

As for my main issue, the following change should not break any existing code.

- signal count_changed(count: int)
+ signal count_changed(count: int, prev_count: int)

To be precise, the change will not break anything (except that callbacks with a different number of arguments will now be generated). The code can be broken by the following change:

# by design one would then naturally pass prev_count in the 
# emit() function, so...
- count_changed.emit(count)
+ count_changed.emit(count, prev_count)

In my opinion, it is fine that when changing the number of arguments of the calling code (emitting a signal), you should make sure that the called code (callbacks) is able to receive and process that many arguments.

Basically, you're suggesting that functions ignore that they're called with the wrong argument count, no? Even if we make this exception only for emitting signals, I think this is not good, since it removes validation, which is already not much for signals.

pckv commented 1 year ago

To be precise, the change will not break anything (except that callbacks with a different number of arguments will now be generated). The code can be broken by the following change:

Ah, I did worry I was not being clear with this. Pretty much anytime I mention the signature of a signal, I also implicitly mean that the .emit() call would be updated to reflect this signature. I don't see any reason the annotated parameters of a signal shouldn't be the same as the emitted arguments.

In my opinion, it is fine that when changing the number of arguments of the calling code (emitting a signal), you should make sure that the called code (callbacks) is able to receive and process that many arguments.

I do actually agree that changing the number of arguments should fail the code with the current implementation of signals. If my main proposal falls short I could update this issue (or move to a new one) to only reflect my secondary proposal which is to have stricter typing for signals. These errors should not have to be discovered during runtime, but be caught by the compiler. Again, I don't understand where it would ever be useful to emit the same signal with different number of arguments.

Basically, you're suggesting that functions ignore that they're called with the wrong argument count, no? Even if we make this exception only for emitting signals, I think this is not good, since it removes validation, which is already not much for signals.

Yes, this is my main proposal. The fact that signals already don't have much validation honestly feels like more of a reason to be less strict about the implementation of callbacks to me. If one chooses to ignore arguments in a signal it already means that the callback doesn't need that value anywhere in the code.

Since signals are vaguely validated, I think at least one of my proposed solutions should be implemented. I'll summarize:

1. Make arguments of signals optional in the callback

2. Stronger typing for signals

dalexeev commented 1 year ago

I have no experience with the internals of Godot so I'm not quite sure how doable this is as functions always require all non-optional arguments to be passed. This change might require modifying how functions work entirely, which could not be feasible. For reference, JavaScript discards additional arguments provided to functions, but I'm not sure of any other languages.

Basically, you're suggesting that functions ignore that they're called with the wrong argument count, no? ...

Yes, this is my main proposal.

I don't think removing the argument count validation when calling a function is what we would like. The absence of errors can lead to hard-to-debug bugs. If you want a function to take an unlimited number of arguments, there is a proposal for variadic functions.

As for the emission of signals with a variable number of arguments, the following options look more acceptable to me:

  1. Add a new ConnectFlags constant that would remove extra arguments before calling the callback (but I'm not sure if this info is available for all methods in release builds).
  2. [If we add vararg functions] Allow vararg signal signatures to be declared so that vararg callbacks are generated (but I wrote my concerns about this above).
  3. Add a method similar to unbind(), discard remaining arguments after x arguments. In fact, you can write a wrapper right now:
# utils.gd
static var _NO_ARG = RefCounted.new()

static func truncate_args(callable: Callable, max_count: int) -> Callable:
    assert(max_count >= 0 and max_count <= 10)
    return func (a0 = _NO_ARG, a1 = _NO_ARG, a2 = _NO_ARG, a3 = _NO_ARG, a4 = _NO_ARG,
            a5 = _NO_ARG, a6 = _NO_ARG, a7 = _NO_ARG, a8 = _NO_ARG, a9 = _NO_ARG):
        var args := [a0, a1, a2, a3, a4, a5, a6, a7, a8, a9]
        for i in 10:
            if i == max_count or is_same(args[i], _NO_ARG):
                args.resize(i)
                break
        callable.callv(args)

# other.gd
signal s()

func f(a = 0, b = 0):
    prints(a, b)

func _run():
    s.connect(Utils.truncate_args(f, 2))
    s.emit()
    s.emit(1)
    s.emit(1, 2)
    s.emit(1, 2, 3)
    s.emit(1, 2, 3, 4)
    s.emit(1, 2, 3, 4, 5)
josefalanga commented 1 year ago

Discarding "extra" arguments of a signal sounds a bit hacky, and sets a bad precedent for callables as a whole.

I think it would be better to validate the places where signals are connected with an invalid handler. I'm with you where you describe runtime errors that could have been compilation errors, that's the part I would be addressing, sounds like the actual problem is that, and discarding arguments is a workaround that leads to more inconsistent behavior.

I'm not sure how compatible that would be with the current philosophy behind gdscript, where typing is supported but it's opt-in. Maybe the signature validation of the handlers can come into play if you define your signal with type annotations, and if not, at least we should get a compile-time errors because an incorrect number of parameters is handled.

Runtime errors are the root of all kind of evil bugs.

Taegost commented 10 months ago

I don't think removing the argument count validation when calling a function is what we would like. The absence of errors can lead to hard-to-debug bugs.

The current functionality already leads to hard-to-debug bugs. I personally just spent 20 minutes trying to figure out why my signal connection wasn't working when the signal has an argument, but one of my callbacks doesn't. It just... Doesn't work, and there's no error message at all that I can see anywhere.

  1. Add a new ConnectFlags constant that would remove extra arguments before calling the callback (but I'm not sure if this info is available for all methods in release builds).

I think this is the way to implement this. It makes it totally optional, most people wouldn't even need to know it's there, but it does exist for the scenarios where it would be extremely handy. This way, if the signature of the signal ever changes, it won't break the callback. Here are a couple scenarios I can think of where something like this is preferable to using .unbind(), putting unused variables in the parameter list, or making a bunch of redundant signals (which are hacky workarounds):

  1. A new day dawns. Every day, several things happen: Vendors re-roll their inventory, the day counter on the HUD updates, the character age increments, and scattered detritus such as corpses and dropped loot are removed after several in-game days. When the day starts, we emit the signal new_day(day: int), where "day" is the actual in-game day. The HUD takes the argument passed in and updates itself, but all of the other things that happen on a daily basis don't care what the actual day number is, only that a new day dawned. If they were all listening for the new_day signal, they could just do their thing. Vendor stock is just re-rolled, the character age gets incremented by 1 (The character may be older or younger than the game world itself, such as multiplayer characters like in Valheim), and the detritus doesn't need to know what day it dropped on the ground, only how many days have passed since it was instantiated.

  2. Your character levels up. The level on the HUD is updated, a sound plays, a screen comes up that lets you pick a random ability (like in Vampire Survivors), and a special effect is applied to the edges of the screen. When the character levels up, the signal level_up(new_level:int) is emitted. Like in the previous example, the HUD takes the signal and argument and updates the screen. The sound and screen effects controllers can pick up that same signal and do their thing, but they don't care what the actual level is, only that it's time to celebrate! The level-up screen then comes up and gives you your choice of random upgrade. Again, it doesn't matter what the actual character level is, the upgrades are all random, it just cares that the event occurred and now the player has to choose.

  3. Pausing the game can be messy. If the character is in the level-up screen (which pauses the game), and then hits Escape to bring up the menu because the sound is just way too high, then they close the menu, the game unpauses... But they're still in the level-up screen! Enter the PauseController singleton. It listens for signals like level_up, upgrade_selected, menu_opened, menu_closed, etc. It doesn't care what level the player is, what upgrade they chose, or which menu was opened/closed, only that those events occurred so it can keep track of whether it's safe to unpause or not.

  4. Expanding on examples 2 & 3: Say you've already implemented the level_up signal with a single argument and everything is working great. Later on, you add in the ability to multiclass, so now when the player levels up, we now have to emit both the level and the class, changing the signal contract. If we used .unbind(1) for those cases where it wasn't needed, now they are all going to break because we have to find every case where we connect to that signal update it to .unbind(2) now. If we had a flag that just allowed us to ignore all parameters, we wouldn't have to make any changes, the code would just continue to work instead of failing silently like it does now.

kylekyleton commented 3 months ago

It seems to me that this proposal is soundly in the spirit of signals. Their utility lies in their flexibility, as stated in the official documentation:

Using signals limits coupling and keeps your code flexible.

https://docs.godotengine.org/en/stable/getting_started/step_by_step/signals.html

Signals allow all connected Callables (and by extension their respective objects) to listen and react to events, without directly referencing one another. This keeps the code flexible and easier to manage.

https://docs.godotengine.org/en/stable/classes/class_signal.html

It'd be a natural extension of the flexibility principle to allow for signal connections to discard any passed arguments that overflow the connected callable's argument count. If you go the other way, and strongly type the arguments, aren't you moving in the direction of re-implementing a direct function call?

AThousandShips commented 3 months ago

I don't see how that is flexibility, and how expecting the callback to be a compatible function is "re-implementing a direct function call". The detail there is about how the callable is a convenient way to have a single interface instead of having to juggle objects and methods and allowing you to call it not caring about the exact method or object

kylekyleton commented 3 months ago

I'm looking for the flexibility to have one method respond to more than one kind of signal, which may or may not have arguments. I have a scene that has a list of events to listen for, some of which have parameters to be met. Signals seem like the natural tool to use in this scenario -- but I am only a dilettante, so if I'm wrong, please correct me. In order to avoid having to write potentially many dozens of methods -- one to handle each potential signal -- I like this proposal. It strikes me now, though, that what I'm really after is just a kind of generic signal receiver method; one that merely reports

  1. The signal it received, and
  2. The signal's arguments, if any.

Perhaps I'll open a new proposal.

AThousandShips commented 3 months ago

For that you can just use unbind and bind to ignore arguments, that's how it's done generally and how we do it in the engine code