Opt-in support for functions that return Result<T,GodotError> to indicate failable operations

[ogapo]

In order to support Result, we should first define a GdError enum that implements std::error::Error. It could replace engine::global::Error but I think it would probably make more sense to have our own enum so it's extensible. Ideally this would contain engine error as one option:

#[non_exhaustive]
pub enum GdError {
    Engine(engine::global::Error), // explicitly returned errors from Godot
    ArgMarshaling(MarshalingErrorDetails), // variant argument marshaling error
    ReturnMarshaling(MarshalingErrorDetails), // variant return value marshaling error
    ScriptError(ScriptErrorDetails), // an error occurred within GdScript
}
// impl std::error Error for GdError
// impl Display, etc
// impl From<> for the various internal types

Some of these *Details structs may just be unit types if there's no details available right now (Godot apparently doesn't have a GetLastScriptError API for instance, but would leave the door open for improvement in the future). It may also make sense to make multiple error enums for functions where only some of these are possible (perhaps GdCallError should differ for instance, YMMV) but the idea is the same.

Once this is defined, it would be ideal to have a policy of having a function which returns Result whenever an operation is failable. In particular, anything that triggers GDScript (inline script only, deferred script calls need not offer this guarantee) may potentially fail due to a script error and surfacing that failure to calling code is important to be able to enforce invariants. Only the calling code knows whether that error should be propagated or if it's safe to eat it, or perhaps handle it in some custom manner (possibly custom to that particular call site).

To that end, the following calls are proposed to extend the existing interface:

• Object::try_call, Object::try_get, Object::try_set
• Callable::try_callv
• PackedScene::try_instantiate(&self)
• If Object::emit_signal is inline, it should have a try, but I think it's deferred (not sure).
• There's probably more candidates but these seem like the majority of the ways Rust invokes gdscript and would represent a great start IMHO.

These should all return the same value as the regular non-try function in the Ok case and GdError in the Err case. To make sure the code paths get exercise, the non-try variant could be implemented in terms of the try_* variant with .unwrap().

The above changes would allow Rust code to determine when scripts fail. To compliment this, it would also be good to accept Rust functions exposed to Godot that return values of the form Result<T : ToGodot, E : std::error::Error>. When called via Godot, this would simply return the Ok type or behave in the same was as though there was a trapped panic (in today's behavior) in the event of an Err.

#[godot_api]
impl MyStruct {
    #[func]
    pub fn foo(&self) -> Result<(),GdError> {
        self.base.try_call(StringName::from("bar"), &[])?; // propagate any script errors to the calling script
        Ok(())
    }
}

This seems like trivial behavior, but it has a few advantages:

1. Not all callsites for marshaled functions necessarily come from Godot. Rust callsites can still freely handle the Result so enabling that function signature would have ergonomic value.
2. It means that in the fairly common case where Godot calls into Rust which then calls back into Godot (via call/set/get/etc), the Rust function can propagate the try_* error results with the try operator (?) if it doesn't want to do specific handling.
3. The generated glue no longer needs to trap panics in Rust functions that return Result since there's already a mechanism for propagating failures. panic trapping can remain the default for other function signatures but this is consistent with Rust's pay-for-what-you-use paradigm.

[Bromeon]

Thanks a lot for all the thoughts and this detailed proposal! Looks sensible to me 👍

We should probably wait until #421 is merged, as that may change/simplify a few things. There may also be a clearer idea how marshaling errors map to enums.

The two changes -- try_* additions to godot::engine API and Result return types in #[func] can be considered separate features and shouldn't be implemented in a single PR.

To make sure the code paths get exercise, the non-try variant could be implemented in terms of the try_* variant with .unwrap().

Rather unwrap_or_else() combined with a panic! that formats the enum into a useful error message.

When called via Godot, this would simply return the Ok type or behave in the same was as though there was a trapped panic (in today's behavior) in the event of an Err.

(Emphasis mine) That can be meaningful for some calls, but it also makes it impossible to handle errors on GDScript side. Basically the same rationale behind this issue -- namely being able to precisely handle errors instead of just panics -- may apply to GDScript, albeit to a lesser extent.

[ogapo]

Sounds good. I'll keep an eye on #421 then. Also, noted on separating PRs. I think it would probably make sense to support Result on #[func] first and get that nailed down.

the same rationale behind this issue -- namely being able to precisely handle errors instead of just panics -- may apply to GDScript

Completely agree. I figured this would require some additional API on the Godot side to be able to report an error with messaging (and maybe even a backtrace if the ext supports it). Does such an API already exist? Or is there an appropriate return value already? In the absence of that I'd just expect to keep the present behavior and treat it like panic! (via the actual panic returned by unwrap_or_else or more structured if possible).

[Bromeon]

Completely agree. I figured this would require some additional API on the Godot side to be able to report an error with messaging (and maybe even a backtrace if the ext supports it). Does such an API already exist?

Not that I'm aware of, unfortunately. There has been some discussion in https://github.com/godotengine/godot-proposals/discussions/4736, but no results yet.

[ogapo]

Making a note that part of this proposal has been implemented in #634 (as part of #411)

[Bromeon]

Good point, thanks. I think there are still some changes left to do:

1. Those from #634
   • Utility functions (those should not get try_ overloads, but at least use the same code path).
   • Variant::call()
   • Callable::call().
   • Don't print error on try_* invocations.
   • More robust passing of CallError across Godot function boundaries (maybe 1 per thread instead of ever-growing).
2. Allow returning Result from #[func]
   • see also #263
3. Integrate godot::global::Error with Result and possibly use it in some APIs

[ogapo]

Nice, I think I might investigate working on these once Godot 4.3 launches and we start targeting it in main

[Bromeon]

There was a recent discussion on Discord with some more insights.

There are different views how Result should behave:

1. map to a custom RustResult class
   • GDScript has no generics, so needs Variant for return type
   • unless we do crazy monomorphization per type (likely unreasonable)
   • most flexible, as GDScript can handle errors
2. map to Godot's global::Error enum
   • mostly OK and FAILED, rest will hardly be used -> glorified bool
   • extremely limited, cannot be used whenever a result needs to be returned
3. just print an error message
   • not recoverable
   • for ptrcalls, we still need to return something

This is compounded by the fact that GDScript has no proper error handling, and there are various ways in Godot APIs to work around this (Error enum, bool return type, using null to indicate errors, requiring an explicit feasibility check before calling a method, printing messages, ...).

---

One of my attempts to sum it up in concrete terms:

1. By default, Result maps to a custom class RustResult which looks something like:

   
   # Custom properties, access to wrong one causes error
   var ok: Variant
   var err: Variant

func is_ok(): bool func to_godot_error(): Error # OK or FAILED

maybe more APIs

2. We add an attribute key to `#[func]` which customizes how `Result<T, E>` is mapped, for those who don't find `RustResult` useful.
Possibilities are (syntax TBD):
```rs
#[func(on_err=print)]
fn do_sth() -> Result<Gd<Node>, String>
// effectively returns Gd<Node>, but calls godot_print!(err) on Err

#[func(on_err=map_to_error)]
fn do_sth() -> Result<Gd<Node>, String>
// effectively returns global::Error, discards any values in T/E, only considers which one is set

Personally, my concern was that passing errors to GDScript that just behave like a panic (i.e. print error and use dummy result) are unrecoverable and should maybe not be thrown in the same bucket as actual Result types that carry error handling information.