Improve Object.connect error handling

[Zoomulator]

Describe the project you are working on:
A rouge-like game, utilizing Godot's signal design pattern to minimize tight coupling between classes.

Describe the problem or limitation you are having in your project:
The Object.connect method both logs an error message and returns an error code.

Doing both causes a few issues; although being minor they cause a bit of confusion.

• Handling the return code will still leave an error in the log.
• Not handling the return code will cause an 'unused return value' warning in the editor.

Using a return code has some other problems:

• Return codes are very uncommon in the Godot code base.
• Only one thing in the entire Godot C++ codebase checks the error code. (+3k other cases ignore it)
• Documentation barely covers which errors may arise.
• It solely returns ERR_INVALID_PARAMETER as an error for many different cases.

The error code may be used to check if something is already connected.

if obj.connect("my_signal", self, "on_my_signal") != OK:
    print("Already connected!")

if obj.my_signal.connect(on_my_signal) != OK:
    print("Already connected!")

The problem here is that there are other things that can go wrong if you as a developer pass bad arguments to the function.
This would cause the if branch to be executed for cases unrelated to the reference count.

As mentioned you'll also get the error littering the log even though you've handled it.

Adding a check beforehand would still give you proper error logging.

if not obj.is_connected("my_signal", self, "on_my_signal"):
   obj.connect("my_signal", self, "on_my_signal")

if not obj.my_signal.is_connected(on_my_signal):
   obj.connect.my_signal(on_my_signal)

Comparisons can be made with File.open which returns an error code. The difference between File.open and connect is that you can only know if File.open succeeds as it is being called. Even if you check that a file exists before opening, it may be gone at the moment you try to open it. In other words File.open is a non-deterministic call while a call to Object.connect/Signal.connect is fully deterministic.

The lack of error codes in the rest of Godot makes this one stand out and makes it appear like there's more going on in connect than there actually is. Any error is easily avoided by some leading checks if the developer needs to handle certain states.

Describe the feature / enhancement and how it helps to overcome the problem or limitation:
Make Object.connect a void function instead of returning an error code. This removes the need to explicitly ignore the error and align the function with the rest of Godot regarding error handling. It would also help new users as it's rather confusing what you should do with the return code, as I experienced myself when learning Godot.

Describe how your proposal will work, with code, pseudocode, mockups, and/or diagrams:
All errors will be presented in the error log and the Object.connect method will return void.

It should be as simple as changing (in Object.connect) the ERR_FAIL_COND_V macros to their non-value counterparts and changing the return signature to void.
The GDScript VM makes an error check in the yield statement which needs to be rewritten to check for bad arguments.

https://github.com/godotengine/godot/blob/master/core/object/object.cpp#L1304
https://github.com/godotengine/godot/blob/master/core/variant/callable.cpp#L385

If this enhancement will not be used often, can it be worked around with a few lines of script?:
You can disable return value discarded warnings, type var _err = connect(...) or use an annotation warning-ignore to just ignore it.

Is there a reason why this should be core and not an add-on in the asset library?:
The proposal is to change an existing method in a core class. You could write your own utility function that ignores the return value for you, but that's a rather patchy fix. This change would mainly benefit new users; avoiding the unnecessary hookup on the obscure error code.

Addendum
It appears to me that the return code for connect is a remnant from another error handling pattern in earlier version of Godot that has changed a bit since. Even though it's a small annoyance, I'd say revising it falls under the ease-of-use policy.
I would personally consider this to be a very simple change, though it is a breaking change. My hope is that it would be included in the 4.0 version.

Some like the idea of still having a return code that is more detailed and useful while removing the error logs. There is no precedence for this in Godot beyond the non-deterministic functions like File.open as explained above. This proposal is for removing the return code in the default implementation of connect but would not oppose an optional method being devised to return an error code instead.

[Zireael07]

Connect() has been already hugely reworked for 4.0, is any of this still relevant?

[Zoomulator]

@Zireael07 Which branch would that be? The method appears to look the same in vulkan? https://github.com/godotengine/godot/blob/vulkan/core/object.cpp#L1429

[Calinou]

@Zoomulator You can read about the signal refactor here.

[Zoomulator]

@Calinou Thanks for the link. Though I'm still confused which branch the refactoring is? It at least sounds like it was merged already.
Anyway, the article doesn't mention the error codes, so to reiterate @Zireael07 question: is this proposal relevant?

[Calinou]

Though I'm still confused which branch the refactoring is? It at least sounds like it was merged already.

It's in the current master branch 🙂

Anyway, the article doesn't mention the error codes, so to reiterate @Zireael07 question: is this proposal relevant?

I think the error code system was left in, which means this proposal is still relevant.

[cgbeutler]

One option would be to do like a lot of libraries do with a try_connect and connect. One would return the error, the other would print the errors.
If any changes are made to connect, they will also need to be mirrored to disconnect as well.
Because gdscript has no exception catching and prefers the error route, some version of connect with error returning will probably always have to exist. I occasionally use this style connect:

if list_item.connect("changed", self, "__update_item", [list_button]):
    __update_item(list_item)

[Zoomulator]

@cgbeutler I see where you're coming from and it's a good idea if there actually were exceptions being thrown.
Take your example, have it ever failed and skipped __update_item? It still leaves you with the error in the log if it does; isn't that annoying?

Object.connect is deterministic, but it seems some think it's not due to the return code. There's no I/O or asynchrony that may throw a wrench in the wheels right as you call connect. It's possible to check all requirements up front, like if the signal exists etc and then call the method without worry.

The error communicate a programming error, while the return code communicate that something unforeseen happened during runtime that you can't know will happen or not when you're programming. From my experience in Godot, that's how the error handling is dealt with.

A try_connect would be a convenience, but there's no precedence in Godot of that as far as I know?

[cgbeutler]

Yeah, I agree that it probably shouldn't print errors and return errors.
I think Godot lacks a super clear precedence here. Calls like file.open(..) simply return an error and others like ResourceLoader.load(.) do like you describe, requiring you check stuff yourself beforehand for any/all error cases. There are quite a few proposals for other error checking methods, but I doubt any will get traction in time for 4.0. As such, I'm not sure what the best answer would be...

A Workaround

For now, you could mostly achieve the desired result by making a result obj that only pushes an error if the error was never checked.
Note that this still prints warnings for unused returns and adds new errors for "unused signals." (I usually turn those warnings off anyway, though... cuz I'm a bad person)

Example Usage:

var result = help_obj.try_connect(self, "nonexistent_signal", self, "hello")
if result.error:  print( result )  #prints error here, no error pushed to editor
help_obj.try_connect(self, "nonexistent_signal", self, "hello")
# result of the above is GCd without the error being checked, resulting in an error getting pushed to editor

Helper code itself:
https://gist.github.com/cgbeutler/7d59adf99d37f4bfac12e0e39b2727f5

Again, it's not perfect and would probably require turning off warnings, so if that doesn't fit your workflow, you'll have to find another way.

[Error7Studios]

@Calinou Will the refactored signal code fix the following issues? (Also, I think invalid (dis)connect() args should be an error)

1. Allowed to (re)connect to a nonexistent method. Passes first error check. Connecting again gives error: already connected

2. Allowed to disconnect from a nonexistent method.

3. Allowed to connect a nonexistent signal.

[Jummit]

Here are some error constants that could be used for return values:

ERR_ALREADY_IN_USE Already connected.
ERR_METHOD_NOT_FOUND Invalid method.
ERR_DOES_NOT_EXIST Invalid signal.

But I agree that making the method return void is the better solution.

[cgbeutler]

I think returning void would be a mistake. Without some kind of feedback, there is no way to programmatically know if it connected. You would have to be aware of and check for every way connect could fail to fully know if it failed, which seems problematic for new/naive people. Heck, I don't even know if I'm checking all the cases correctly! For example, the already-in-use case is based on the flags, so the check would be:

if 0 == (flags & Object.CONNECT_REFERENCE_COUNTED) and source.is_connected(signal, target, method):
  return ERR_ALREADY_IN_USE

Something tells me some folks may not understand these complexities of the connect command's checks.

For stability of reacting to connect failures, error returns seem waaay more reliable/easy to work from.

I, personally, also feel like it's a D.R.Y. violation to have to duplicate all the error checking logic that's already being done inside a function like this. If I add all the checks before calling connect, those are all technically being done twice! And if my set is off, then my error handling has a bug, which is the "Jurassic Park"(book) way of ensuring cascading failures just seem to slip on by.

[YuriSizov]

which seems problematic for new/naive people

To be fair people mostly don't check for the return value either. I see very narrow application for the return value, something where you'd want to wrap connect into some higher order function to replace it.

I'd also say that in engine we never check the return value of connect, but we also have the benefit of a compiler checking for errors, so it's moot 🙃

[cgbeutler]

I get the appeal for making it void, I do. This call is one that will almost always rear errors at dev time. Most of the time connect's errors are there just for a programmer to see and fix, almost more like a syntax check. Returning an error feels like hiding the syntax check, which is equally bad.

Thus we have a standoff! Rare cases where connect is used in a more complex way rely on knowing if it succeeded. It is no longer a syntax check, but a programmatic one that is still needed at run-time.

I guess that's why I suggested 2 separate functions. Godot often has 2 ways to do things. One for the simple use cases and one for when you need more access to what the engine is doing. (For example ResourceLoader.load(...) just gives null or not, while ResourceLoader.load_interactive() gives you access to all the actual load errors.)

If adding try_ functions aren't it then perhaps 'connect' could become the complex one while the Godot 4 style becomes the simple version? In other words:

# returns error
var err := button.connect("pressed", self, "_on_button_pressed")
# void that prints error
button.pressed.connect(self._on_button_pressed)

[YuriSizov]

try_connect sounds sensible to me. I would imagine it doesn't print any errors itself and instead returns a code. Kind of like we have get_node_or_null, in a sense.

[Zoomulator]

Looking at the source here:
https://github.com/godotengine/godot/blob/master/core/object/object.cpp#L1250

All errors are for checking the arguments, except reference-counting. Non existing signals, methods or objects sounds like the responsibility of the caller to keep right and are efficiently communicated via push_error log messages. No other method gives a return code saying you passed a non-object, null or using a non existing method; they just break and log it. It's basic housekeeping for the user to make sure you pass sensible things to a method.

That leaves the reference counting. It only returns ERR_INVALID_PARAMETER if you do it wrong.

I'm yet to see an actual complex example where the return code has been used to deal with a failing connection and adjust to another call at runtime. It would surprise me greatly if there is, as I stated in the initial post, the only error code that is currently returned is ERR_INVALID_PARAMETER. All you can do is sweep it under the rug (and still get an error in the log).

The current implementation of the return code is giving you no information about what went wrong. Leading me to conclude that the complex case that may be out there only exists in theory and is not actually a requested feature.

Removing the error code would have no impact on the versatility of connect in its current state. You can absolutely argue for expanding the error codes given and actually make them usable, but I'd say it's at the risk of being somewhat pointless if you can't give a use case.

If in the future someone requires it for a good reason, wouldn't it be better to create a try_connect at that point when there's actually a concrete and existing use case for it?

[cgbeutler]

@Zoomulator I get that you wouldn't use the complex version. I would hope you can respect the fact that I need it.
I would oblige and detail all the spots where I use my helper method, but I've done open source long enough to recognize when an off-topic code-style argument is starting.
We both have needs and want changes here. In an attempt to stay on topic and ensure this proposal doesn't die in a blaze of an opinion argument shut down by mods, I would ask if you would be ok with 2 different methods?

[Zoomulator]

@cgbeutler I apologize if you took offense; I can come across as pretty blunt. I just wanted to emphasize that the current state of the error code is unusable and not worth saving for the sake of there being an error code historically. But if you do need it I'd welcome you to propose how the error code would be improved to communicate things properly for your use case.

Two or one method makes no difference to me as long as we get spared the mix of error types. I'd only argue for the void variant as the default since checking return codes is easily overlooked by new users and would hide errors from them if the code is ignored.

[cgbeutler]

@Zoomulator no worries. No offense taken, just trying to figure out what folks consider "deal breakers" on this. Sounds like a try_connect function is ok with most folks.

I agree that the void-return version of connect makes more sense as the default. As it stands for 4.0, connect(...) only returns ERR_INVALID_PARAMETER and OK, so it probably shouldn't return anything at all. Changing the return type would break backward compatibility, so it would be nice to get it in the 4.0 switcharoo.
The less-appealing alternative of having the main connect function return real errors is not without benefits. It's a tiny bit more backwards compatible (kinda.) Unfortunately, folks that have ignored the compiler warning for "ignored return value" will not ever know there was an error with connect. While that's "how it should be," it could certainly catch folks with their pants down.

SO! Seems like the connect should probly be made void. If the try_ versions don't happen as a part of this proposal, I will survive on helper methods and can add a different proposal for a try_connect. (If there are no objections to a try_connect that returns real errors as a part of this proposal, I would very much appreciate it 😉, though.)

[Zoomulator]

Proposal edit: Applied an anti-snark filter to my writing. Sorry if I came across rude in the original proposal.

[YuriSizov]

The idea to remove the returned error has been approved!