Add Erased Convert Error Type

[Dheatly23]

Erased error type is useful to separate non-sendable values in ConvertError with sendable error type. But the trade-off is some data might be gone in conversion.

For now it does not report the value, though this might change. Also in the future ConvertError can be made richer without sacrificing eg. sendability.

[GodotRust]

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-644

[Bromeon]

Generally these error APIs are not very sophisticated by design, as they add complexity in a part of the library that hardly benefits from it. And I'd definitely like to keep the API clutter down to a minimum (e.g. additional type ErasedConvertError).

Erased error type is useful to separate non-sendable values in ConvertError with sendable error type. But the trade-off is some data might be gone in conversion.

Can you demonstrate an example that requires it? Rust's Error trait is generally designed around errors being Send, so if possible I'd like to stick with that.

Also keep in mind current changes in #645 which is a bugfix.

[Dheatly23]

I thought it's a bit better implementation of #525 , by separating error type into sendable and non-sendable part (since Godot types are currently non-sendable). I noticed the former is slow (writing string), particularly if conversion is retried again (eg. multiple dispatch). Kinda work around it, but it's not satisfactory IMO. And now that i have some free time, i can work it to my satisfaction 😅

With naming, it can be worked out later. It's definitely not SendableConvertError because some data is lost (like value).

[Bromeon]

since Godot types are currently non-sendable

But can you show concretely in which situation this is a problem, with code?

Values are currently erased via fmt::Debug, and the cause is supposed to be another error (i.e. recursively has the same requirements as this one).

---

With naming, it can be worked out later

I'm not concerned about naming, but about APIs becoming more complex than necessary. See also here for detailed reasoning.

---

I noticed the former is slow (writing string), particularly if conversion is retried again (eg. multiple dispatch)

Now I'm curious, how did you notice it, did you measure it? Not that I don't believe it, but I'm wondering if this is something worth optimizing 🙂

[Dheatly23]

But can you show concretely in which situation this is a problem, with code?

Values are currently erased via fmt::Debug, and the cause is supposed to be another error (i.e. recursively has the same requirements as this one).

Casting a sufficiently complex value should suffice. Probably not very heavy, but Debuging that probably isn't very efficient.

Now I'm curious, how did you notice it, did you measure it? Not that I don't believe it, but I'm wondering if this is something worth optimizing 🙂

Mostly because i had configuration struct that can be multiple types (int/float, string/bytes, file, (non)null, etc). Multiply that by however many values, and it's hitting pretty hard. Especially if the actual value is long to print (like an entire file in PoolByteArray or GString).

If there's a VariantDispatch it can be way faster. Though typed array can throw a wrench in that 🙃

[Bromeon]

Fair enough, however this alone doesn't justify making the API more complex. Even if we use an erased value (as Variant or VariantDispatch), I would not add a separate ErasedConvertError. This can be kept internal.

And removing Send + Sync also has its price, but it might (?) be OK given the benefits.

[Bromeon]

Would be nice if you could rebase it on top of #645.

If you add type erasure, why not use Option<Variant> directly as the value? dyn Debug is less useful...

Please also keep API changes to a minimum. Would be cool if we didn't need a separate ErasedConvertError 🙂

[Bromeon]

Please use variables here.

[Bromeon]

I think it would be nicer to have single ConvertError, no erased one. Then this enumerator could be kept simple, as well.

[Dheatly23]

It won't compile because ConvertError is no longer Send, so i have to work around that.

[Dheatly23]

Would be cool if we didn't need a separate ErasedConvertError 🙂

Should the type be hidden from user? It can be accessed by fn into_erased(self) -> impl Error + Send + Sync

[Bromeon]

At the risk of repeating myself 😀

Can you demonstrate an example that requires it?

But can you show concretely in which situation this is a problem, with code?

It would be good to justify any addition to the API (here ErasedConvertError) with a motivating use case. And showing a short code snippet is often more expressive than long descriptions. You may have encountered scenarios in your projects that make this valid, but don't assume everyone in this thread has done the same.

I understand the technical limitation (general errors not being Send+Sync, erased ones sacrificing fields in order to be that) but I'm not sure if this separation occurs often enough in practice to justify two types. Maybe it's fine to just not have Send + Sync on this type, and reconsider the decision once users run into problems?

[Dheatly23]

The (slow) code looks something like this:

if let Ok(v) = GString::try_from_variant(&var) {
    // Argument is a text
} else if let Ok(v) = PackedByteArray::try_from_variant(&var) {
    // Argument is a binary data
} else if let Ok(v) = <Gd<FileAccess>>::try_from_variant(&var) {
    // Argument is a file
} else if let v = <Gd<SomeType>>::try_from_variant(&var) {
    // Let's clone from our type instead
} else {
    // Return an error
}

And that's not the worst one. Let's say there are 8 Packed*Array types to handle. I kinda had to manually implement variant dispatcher.

You're right though, it's not strictly necessary. It's just a ok addition, with some edge case making printing and discarding the same error 8 times.

[Bromeon]

As mentioned, I agree about the performance. But my last question was specifically about ErasedConvertError -- how would that one make the above code more readable?

Minor thing, you can use variant.try_to::<T>() instead of <T>::try_from_variant(variant).

[Dheatly23]

As mentioned, I agree about the performance. But my last question was specifically about ErasedConvertError -- how would that one make the above code more readable?

For readability, it's almost unchanging. Except for error conversion.

let s = String::try_from_variant(&v).map_err(|v| v.into_erased())?;

There can be a convenience trait impl for Result<T, ConvertError> for easier use.

Oh and btw, the erased type is no longer public.

[lilizoey]

Did you consider making ConvertError generic to fix this instead? maybe something like

pub struct GenericConvertError<V> {
    kind: ErrorKind,
    value: Option<V>,
}

pub type ConvertError = GenericConvertError<Variant>;
pub type ErasedConvertError = GenericConvertError<String>;

I'm not sure if this is better, mostly just a question. It does avoid a separate type for ErasedConvertError though.

[Bromeon]

As I understood, the chain that led to the proposed solution is:

1. Stringifying variants on each error construction is expensive (compounded by if/else chains).
2. So, instead of the string Debug representation, let's store the original Variant.
3. This can no longer be Send/Sync because some variant types aren't thread-safe.
4. Thus we need two types: an original one without Send/Sync, and an erased one with.

---

We will likely face the same problem in any error that includes values (could e.g. affect CallError if we decide to incorporate argument values). But I also don't want to overengineer a super-general solution here, maybe just think a bit ahead so we don't have to build a specific hack again next time.

We have different concrete errors like ConvertError, CallError, IoError etc -- what if they can all be converted into a ErasedError that's basically implementing std::error::Error + Send + Sync + Debug + Display + 'static? I'm not sure if that's worth it for just Send/Sync alone, but maybe someone sees other benefits?

[Bromeon]

Rebased.

@Dheatly23 any update here? I'd like to consolidate some of the errors soon -- so even as-is, the PR would be good as a first design on which we can iterate.

Could you also fix the tests?

[Bromeon]

(I quickly need to test something, will close and reopen)

[GodotRust]

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-644

[Dheatly23]

Regarding tests, i don't understand why it fails. For example, i don't touch anything close to test object_from_invalid_instance_id, yet some assertion failed. Still trying to trace the call.

Edit: (Kinda) found it. It apparently tries to clone invalid object. Maybe it can work if it skips calling if pointer is null.

[Bromeon]

Thank you!

I'll merge this for now, but keep in mind that the API may still evolve once we try to unify the different error types. Maybe there's a way to avoid the type-state pattern for this specific error 🙂