Consistent error APIs

[Bromeon]

There are several places in gdext where things can go wrong. At the moment, there is no unified guideline on how to design such APIs.

Logic vs runtime error

Traditionally, errors can be split into logic errors (bugs that must be fixed) and runtime errors (that need to be expected and handled by the developer). However, this is not a very clear-cut line, since the scene tree or other invariants are technically known at compile time, but can still fail.

For example, try_load::<T>("path/to/Node") can fail for wrong paths or wrong types. This may be a logic error (assuming the developer should know their scene tree) or a runtime error (if tree is created dynamically, e.g. in a level editor). As such, I'm not sure if strict split of logic/runtime is that useful here.

Coarse vs. fine

To me, first priority is that there is a descriptive error message. If something goes wrong, the developer should have a way to know this from the error message, ideally with some context.

Then, there has been the discussion about mapping each internal error to a public enumerator. That would mean most APIs have their own domain-specific error types. But how do we design those, if they have more fine-grained "internal" causes that may not be relevant for the public API?

I would like the design be driven by actual use cases and not theoreticals. Yes, it's possible to expose all information that gdext possesses about an error to the user, but it's also costly and impractical, in terms of maintenance, discoverability, good abstraction boundary, UX. So each symbol that is exposed must have a clear use case.

Integration with Godot

There is currently engine::global::Error which is returned from some engine APIs. We also have domain-specific errors about failed calls, etc. Ideally we can integrate them into our error API design.

Compatibility

Having very fine-grained error enums comes at a cost, even with #[non_exhaustive]. It makes it harder to refactor things, smaller changes often become breaking. For example, changing an error enum from unit (C-style enumerator) to struct style (having fields).

Compatibility also conflicts with fine-grainedness. Once we have crates.io releases and take SemVer seriously, we cannot easily break these things without waiting for the next release, and it's not great if error types are expressing the wrong thing due to technical limitations (like adding a field that would interfere with existing code).

Self-containedness

I don't want to add crates like thiserror or anyhow to gdext. But maybe we can design errors in a way that they are extensible and composable with these crates.

[StatisMike]

Thanks for this, I think that this is great time for this as IMO we could really benefit from good, consolidated structure of gdext errors, as there are still room for improving safety of the API.

mapping each internal error to a public enumerator

In hindsight, using enum wasn't a very good idea on my part, probably inspired by Godot's error and day-to-day work in PHP with error codes :)

To start the topic, as impl std::error::Error is the no-brainer, looking at its current state:

• we have a Display, which will produce an error message, which - I agree - is the main concern and most users will have contact with extensively, but is not usable to identify error cause at runtime to use optimal recovery strategy.
• out of methods on Error trait, cause is deprecated, description is both deprecated and redundant with Display, and provide is nightly, leaving us only with source, our only Error built-in non-display method AFAIK

It would require every discriminant of sort to implement Error itself (Im looking at you, engine::global::error)? This way if only our panic is caused by Godot's error, we could pass it along for the user to decide what to do with it. Everything else needed for the Display to be as informative as possible could be just private.

I just wonder if Option<Box<dyn Error>> could be used to easily discriminate (as I'm not at home currently to test it out), eg with:

if let Some(MySpecificError) = more_general_error.source() {
// ...recover strategy for MySpecificError...
 }

though this way could also introduce great number of Errors, even simple unit struct types. Eg. as try_load can fail for two reasons, it would need:

• either enum error dedicated to it
• two different error unit structs that will be passed to higher-level, domain-scope Error struct in its source, to act as an discriminant

Other way I see is to go back in time to the integer error codes. This way we can define private enum with integer representation and add codes to it, returning only the integer publicly - though IMO it is even worse than pure enum solution.

[Bromeon]

It would require every discriminant of sort to implement Error itself (Im looking at you, engine::global::error)? This way if only our panic is caused by Godot's error, we could pass it along for the user to decide what to do with it.

I'm not sure if we should go this route, as it leads to a proliferation of dozens if not hundreds of dedicated types with overly generic names (ERR_BUG?)

Also, I don't see a real use case for this -- we can easily keep this as a single enum and in its Display impl match on the enumerators (if needed). But more on that in the following sections.

---

I just wonder if Option<Box<dyn Error>> could be used to easily discriminate

The problem I see here is that this reinvents what a (non-exhaustive) enum does. But instead of having an explicit relation supported by the type system and match-able, the user needs to know the possible dynamic types a priori, and check for each in a downcast-chain. It's also much more cumbersome to use... so where's the benefit?

---

In hindsight, using enum wasn't a very good idea on my part, probably inspired by Godot's error and day-to-day work in PHP with error codes :)

I don't think enum is a bad idea per se, but I'm not sure if we should map every internal error 1:1 to our public API. I'm more in favor of abstracting where reasonable. If really needed, we can provide a low-level method returning Godot's integer error constant (this could even be Error::source(), but it might be easier to have a separate method).

TLDR: I'd like to keep things as simple as possible while still providing the option to differentiate where it makes sense. The less public types, the better.

[Ogeon]

I don't have much to add to the discussion, other than that it's sometimes necessary to make a decision based on the error reason, so it's good to not hide it from the logic if the information exists. Exposing the Godot error integers, as mentioned, should be good enough for that, IMO. This is for when a single godot/gdext function may have multiple error reasons and there's no other way to distinguish between them.

[0awful]

I think there may be value in a runtime/compile time split. I do agree its a very fine line to have and very difficult to discern. The possibility of locking the api into supporting things that are harmful in the future is something I don't see as worthwhile. For developers this makes porting to later versions difficult and for y'all its an additional burden. I can't see benefit for either party.

What I would find most beneficial (as someone who really likes to use the try_ variants of functions) is if there's a conceivable path to recover exposing an error type that helps that recovery. The example of path/type errors makes me want something like a No Node at Path error and a Incorrect type at path error which I can then handle. The No Node at Path error prompting me to attempt to recreate if reasonable to do so or move on without it. The incorrect type at path error prompting me to gracefully exit.

Given the "best" response for at least one branch is inevitably graceful exiting. (the developer used the library incorrectly) Perhaps the distinction should be recoverable vs non-recoverable. Although that is very domain specific and hard to generalize at an engine level. If that direction is adopted I can't imagine a solution that doesn't evolve into error enums for every possible error case being exposed for the developer. Something that is harder for both y'all at the engine level and any new developer to learn.

Given all that, I can't say what would be ideal. Only agree with you that this is a very hard problem.

With that in mind. I do have an idea. Inverting the paradigm. Perhaps the exposed error types should only be those that are impossible to recover from. The user can check for those and then handle the catch all error case based on their domain.

IE: The user looks for a node at the path they have the following cases:

1. They find the node and handle the happy path
2. They find an error case that is impossible to recover from (eg: the type of node present is not the expected type). This should be an error the user should anticipate and resolve by exiting with this information disclosed.
3. They do not have the node and do not have a fatal error. Implicitly this is an error in the domain of the user and something they should define. No specific error needs to be exposed because of that. A catch all is acceptable here imo. That "catch-every-non-fatal-error" leaves space for the developer to define the distinctions and frees you from exposing excessive information.

I can't say how practical this is from an implementation standpoint, but I believe its agreeable from a user standpoint.

[Lamby777]

There is currently engine::global::Error which is returned from some engine APIs. We also have domain-specific errors about failed calls, etc. Ideally we can integrate them into our error API design.

imma be real with you, i only found out this existed today when i tried to unwrap the returned value of change_scene_to_file :P

---

Also, snippet of stuff I wrote in discord:

for the most part i've just been using the panic versions of stuff because usually
i don't have any reason to NOT want a panic if some expected condition isn't
satisfied (like if a node with such name doesn't exist)

like i see the purpose for having the try_ versions, just saying i've never had a
reason to use em yet so i can't give much input on ergonomics

:P

[Bromeon]

@Lamby777 there was a misunderstanding, I meant #407 🙂 to be clear, this thread is about API design of the error types, whereas #407 is about improving error messages.

I moved your post there, feel free to keep or delete here 😉