Make `FromGodot` conversions fallible #467

lilizoey · 2023-10-27T22:05:18Z

Adds ConvertError which is used when a conversion fails.
Makes FromGodot use Result<Self, ConvertError> instead of Option<Self>.

The main use for this is for debugging and error messages, as a failed conversion will now communicate what failed and why.

This can also be used for more granular error handling, but at the moment this is fairly limited. We can consider adding more ability to inspect the errors in the future.

I also made each method in the Signature traits take the method/function name as the second argument (for consistency) so that error messages always can print what method/function had an error.

closes #451

GodotRust · 2023-10-27T22:11:39Z

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

Bromeon

Thanks a lot!

ConvertError currently appears in both builtin::meta and builtin modules, but should only have two symbols in total (once in builtin::meta, once in prelude.

Not yet reviewed everything, but already submitting first half 🙂

Unrelated to this PR, but I'm not sure if we should nest modules inside godot::prelude. It probably helps to keep some types shorter but still somewhat qualified? The others like prelude::{math, utilities, real_consts} etc. make some sense, but prelude::meta in particular is questionable. Its traits ToGodot/etc should be part of prelude itself, and other things like ClassName or PropertyInfo are so niche that they can just be fully qualified in their original module.

No need to change anything here (apart from the ConvertError duplication), but I wanted to ask how you see this?

Bromeon · 2023-11-15T10:50:13Z

godot-core/src/builtin/meta/godot_convert/convert_error.rs

+    }
+}
+
+#[derive(Debug, PartialEq, Eq)]


Suggested change

#[derive(Debug, PartialEq, Eq)]

#[derive(Eq, PartialEq, Debug)]

😛 (see here)

Still open, but I can address it sometime else, not worth blocking this 😉

Bromeon · 2023-11-15T10:50:38Z

godot-core/src/builtin/meta/godot_convert/convert_error.rs

+}
+
+#[derive(Debug, PartialEq, Eq)]
+#[non_exhaustive]


Do we need #[non_exhaustive] if the type is not public?
Inside the crate itself, we're not forced to write else branches in match.

we shouldn't, it's leftover from when the enums were public

Bromeon · 2023-11-15T10:54:28Z

godot-core/src/builtin/meta/godot_convert/convert_error.rs

+            Self::BadType { expected, got } => {
+                format!("expected Variant of type `{expected:?}` but got Variant of type `{got:?}`")
+            }
+            Self::WrongClass { expected } => {
+                format!("got variant of wrong class, expected class `{expected}`")
+            }


I would keep the "expected A, but got B" order consistent throughout error messages.

Also, first message uses Debug and second Display repr.

i can't use Display for the first one without implementing Display for VariantType. and the implementation would likely be identical to the Debug impl.

and i dont want a debug version of ClassName printed, i just want the class name.

I see, thanks for elaborating!

Bromeon · 2023-11-15T10:57:05Z

godot-core/src/builtin/meta/godot_convert/convert_error.rs

+    /// Variant value cannot be represented in target type
+    BadValue,


This should probably also contain the variant type for more context:

Suggested change

/// Variant value cannot be represented in target type

BadValue,

/// Variant value cannot be represented in target type

BadValue {

expected: VariantType,

}

i think i can actually just remove it, since it's only used in one place where BadType is the more appropriate error

Bromeon · 2023-11-15T11:03:02Z

godot-core/src/builtin/meta/godot_convert/convert_error.rs

+impl ConvertError {
+    /// Create a new error for a conversion.
+    fn new(kind: ErrorKind) -> Self {
+        Self {
+            kind,
+            cause: None,
+            value: None,
+        }
+    }
+
+    /// Create a new custom error for a conversion.
+    pub fn custom() -> Self {
+        Self::new(ErrorKind::Custom)
+    }
+
+    /// Add a rust-error as an underlying cause for the conversion error.
+    pub fn with_cause<C: Into<Cause>>(mut self, cause: C) -> Self {
+        self.cause = Some(cause.into());
+        self
+    }
+
+    /// Returns the rust-error that caused this error, if one exists.
+    pub fn cause(&self) -> Option<&(dyn Error + Send + Sync)> {
+        self.cause.as_deref()
+    }
+
+    /// Add the value that failed to be converted.
+    pub fn with_value<V: fmt::Debug + 'static>(mut self, value: V) -> Self {
+        self.value = Some(Box::new(value));
+        self
+    }


I'm not sure about this "almost-builder" constructor design. You have a lot of patterns like this:

ConvertError::from(err).with_value(val)

which could be written more type-safely as:

ConvertError::with_value(err, val)

This would get rid of the From conversion and would protect invariants (value is not forgotten).
From feels wrong here, the resulting object is not equivalent but needs additional information to be useful.

How many possible combinations are we facing here? Now there are already 3-4 constructors/builders, I don't think there would be significantly more constructors if all cases are covered explicitly.

They're mostly just there so people can quickly create custom errors if they want tbh, but i think i can make the internal methods more type safe

godot-core/src/builtin/meta/return_marshal.rs

Bromeon · 2023-11-15T11:11:26Z

godot-core/src/builtin/meta/signature.rs

                PtrcallReturnT::<$R>::call(|return_ptr| {
                    utility_fn(return_ptr, type_ptrs.as_ptr(), type_ptrs.len() as i32);
-                })
+                }).unwrap_or_else(|err| return_error::<Self::Ret>(method_name, err))


Consider using a variable here (this pattern appears 4x in total).

Bromeon · 2023-11-15T11:12:12Z

godot-core/src/builtin/meta/signature.rs

@@ -205,13 +210,15 @@ macro_rules! impl_varcall_signature_for_tuple {

                    check_varcall_error(&err, method_name, &explicit_args, varargs);
                });
-                <Self::Ret as FromVariantIndirect>::convert(variant)
+
+                <Self::Ret as FromGodot>::try_from_variant(&variant).unwrap_or_else(|err| return_error::<Self::Ret>(method_name, err))


Also here, a variable to store <Self::Ret as FromGodot>::try_from_variant(&variant) might be more readable.

It would also help annotate intermediate expressions with types in IDEs.

Bromeon

Reviewed the rest. Thanks a lot for all the tests! 👍

Bromeon · 2023-11-15T13:42:29Z

godot-macros/src/derive/derive_from_variant.rs

-        let root = variant.try_to::<::godot::builtin::Dictionary>().ok()?;
-        let root = root.get(#name_string)?;
+        let root = variant.try_to::<::godot::builtin::Dictionary>()?;
+        let root = root.get(#name_string).ok_or(ConvertError::custom().with_cause(concat!("missing expected value ", #name_string)).with_value(root.clone()))?;


Instead of concat! in the generated code, the string could be concatenated outside of quote!.

Also, splitting root.get(#name_string) into a separate variable would allow to avoid the root.clone(), no? Since get returns a value and doesn't borrow root any longer.

sure though i was planning on rewriting this whole derive macro soon anyway

Ah ok, in that case no need to bother 🙂

Bromeon · 2023-11-15T13:45:41Z

godot-macros/src/derive/derive_from_variant.rs

@@ -128,20 +131,20 @@ fn make_named_struct(
        } else {
            (
                quote! {
-                    let #ident = root.get(#string_ident)?;
+                    let #ident = root.get(#string_ident).ok_or(ConvertError::custom().with_cause(concat!("missing expected value ", #string_ident)).with_value(root.clone()))?;


Bromeon · 2023-11-15T13:46:35Z

godot-macros/src/derive/derive_from_variant.rs

+                    let #ident = root.pop_front()
+                                     .ok_or(ConvertError::custom().with_cause("missing expected value").with_value(root.clone()))?
+                                     .try_to::<#field_type>()?;


I would do indentation like regular rustfmt: from start of the scope, not aligned with the first .

Bromeon · 2023-11-15T13:47:19Z

godot-macros/src/derive/derive_from_variant.rs

+                let #ident = variant.pop_front()
+                                    .ok_or(ConvertError::custom().with_cause("missing expected value").with_value(variant.clone()))?
+                                    .try_to::<#field_type>()?;


Bromeon · 2023-11-15T13:49:27Z

itest/rust/src/builtin_tests/convert_test.rs

+    node.free();
+}
+
+/// Check that the value stored in an error is the same as what was given.


"what was given", you mean passed to its constructor?

Bromeon · 2023-11-15T13:50:07Z

itest/rust/src/builtin_tests/convert_test.rs

+    const MISSING_KEY_A: &'static str = "missing `a` key";
+    const MISSING_KEY_B: &'static str = "missing `a` key";
+    const TOO_MANY_KEYS: &'static str = "missing `a` key";


Probably forgot to update 2nd and 3rd literal 🙂

Bromeon · 2023-11-15T13:54:41Z

itest/rust/src/builtin_tests/convert_test.rs

+        format!("{:?}", err.value().unwrap()),
+        format!("{:?}", "hello".to_variant())


It's not obvious why you compare the Debug reprs here -- is it because one can't fetch/downcast the original type anymore? Maybe add a comment 🤔

(also in other such cases)

oh yeah i did add a comment earlier in another case but i can add it here too

Bromeon · 2023-11-19T12:28:20Z

Rebased onto master, should fix the clippy issue.

Bromeon

Mostly good, a few unresolved conversations (also from last time). Feel free to arrange/squash commits to your liking 🙂

Bromeon · 2023-11-22T08:56:35Z

godot-core/src/builtin/array.rs

+    /// Creates a new reference to the data in this array.
+    ///
+    /// This does not ensure the Array's type matches what we statically expect.
+    fn clone_unchecked(&self) -> Self {
+        // SAFETY: `self` is a valid array, since we have a reference that keeps it alive.
+        unsafe {
+            Self::from_sys_init(|self_ptr| {
+                let ctor = ::godot_ffi::builtin_fn!(array_construct_copy);
+                let args = [self.sys_const()];
+                ctor(self_ptr, args.as_ptr());
+            })
+        }
+    }


Would this method need to be unsafe itself, as the caller needs to make sure the invariants are upheld?

Nvm turns out that changing this to no longer need this method was an easy fix

I dont think this actually needs to be a safety, it just needs to be a correctness invariant. As i don't believe anything in Array actually relies on the values stored in the array to be T, and anything that expects it to be will panic/error when it's not. If this actually should be unsafe then i'd need to rewrite some stuff. Maybe @ttencate knows for sure?

~~We can refer to the assume_type method here, whose safety doc says:~~

/// In and of itself, calling this does not result in undefined behavior. However: /// - If `T` is not `Variant`, the returned array should not be written to, because the runtime /// type check may fail. /// - If `U` is not `Variant`, the returned array should not be read from, because conversion /// from variants may fail. /// /// In the current implementation, both cases will produce a panic rather than undefined /// behavior, but this should not be relied upon.

I guess not marking this as unsafe would be a commitment to the current behavior, meaning assume_type should be changed to be a safe function. Which wouldn't be the worst, we could in the future provide *_unchecked methods as alternatives in cases where we currently panic?

Bromeon · 2023-11-22T08:57:23Z

godot-core/src/builtin/array.rs

@@ -627,7 +651,8 @@ impl<T: GodotType> GodotConvert for Array<T> {

 impl<T: GodotType> ToGodot for Array<T> {
    fn to_godot(&self) -> Self::Via {
-        self.clone()
+        // `to_godot` is sometimes intentionally called with an array in an invalid state.


Could you clarify "sometimes" here, when is that needed?

Apparently it was just needed in the to_variant method so i can just change that method to not use to_godot(), not sure why i didnt see that initially

Bromeon · 2023-11-22T09:07:04Z

godot-core/src/builtin/meta/godot_convert/convert_error.rs

+    /// Create a new custom error for a conversion with the value that failed to convert.
+    pub fn new_with_value<V: fmt::Debug + 'static>(value: V) -> Self {
+        let mut err = Self::custom();
+        err.value = Some(Box::new(value));
+        err
+    }
+
+    /// Create a new custom error with a rust-error as an underlying cause for the conversion error.
+    pub fn new_with_cause<C: Into<Cause>>(cause: C) -> Self {
+        let mut err = Self::custom();
+        err.cause = Some(cause.into());
+        err
+    }
+
+    /// Create a new custom error with a rust-error as an underlying cause for the conversion error, and the
+    /// value that failed to convert.
+    pub fn new_with_cause_value<C: Into<Cause>, V: fmt::Debug + 'static>(
+        cause: C,
+        value: V,
+    ) -> Self {
+        let mut err = Self::custom();
+        err.cause = Some(cause.into());
+        err.value = Some(Box::new(value));
+        err
+    }


Nitpick, maybe with_* instead of new_with_* would be enough?

Interestingly, Rust itself has both 🙈 Vec::with_capacity and SipHasher::new_with_keys. Although the former is much more common and not deprecated...

Make `FromGodot` use `Result<Via, ConvertError>` Fix up everything to use the new fallible methods Make itests use `expect` and `expect_err` for fallible conversion testing Add some tests to ensure conversion works as expected

Bromeon

Thanks a lot!

Bromeon · 2023-11-25T08:26:37Z

godot-core/src/builtin/meta/godot_convert/convert_error.rs

+    }
+}
+
+#[derive(Debug, PartialEq, Eq)]


Still open, but I can address it sometime else, not worth blocking this 😉

Bromeon · 2023-11-25T08:28:54Z

godot-macros/src/derive/derive_from_variant.rs

            quote! {
-                let #ident = variant.pop_front()?
-                                    .try_to::<#field_type>()
-                                    .ok()?;
+                let #ident = variant.pop_front()
+                                    .ok_or(ConvertError::with_cause_value("missing expected value", variant.clone()))?
+                                    .try_to::<#field_type>()?;
            }


Reminder for me to go through quote! formatting at some point...

lilizoey mentioned this pull request Oct 27, 2023

Implement ToGodot/FromGodot for Vec<T> #263

Closed

lilizoey force-pushed the feature/godot-convert-result2 branch 3 times, most recently from 585b828 to 5bd9984 Compare November 14, 2023 13:24

lilizoey marked this pull request as ready for review November 14, 2023 14:18

lilizoey requested a review from Bromeon November 14, 2023 14:19

Bromeon added feature Adds functionality to the library c: core Core components labels Nov 15, 2023

Bromeon reviewed Nov 15, 2023

View reviewed changes

Bromeon force-pushed the feature/godot-convert-result2 branch from 2984e19 to 6ed8bd5 Compare November 19, 2023 12:28

lilizoey force-pushed the feature/godot-convert-result2 branch from 6ed8bd5 to 51ab4e0 Compare November 19, 2023 18:07

Bromeon reviewed Nov 22, 2023

View reviewed changes

Add ConvertError

20b0f50

Make `FromGodot` use `Result<Via, ConvertError>` Fix up everything to use the new fallible methods Make itests use `expect` and `expect_err` for fallible conversion testing Add some tests to ensure conversion works as expected

lilizoey force-pushed the feature/godot-convert-result2 branch from 51ab4e0 to 20b0f50 Compare November 24, 2023 09:58

Bromeon approved these changes Nov 25, 2023

View reviewed changes

Bromeon added this pull request to the merge queue Nov 25, 2023

Merged via the queue into godot-rust:master with commit 9016ae2 Nov 25, 2023
15 checks passed

lilizoey mentioned this pull request Nov 29, 2023

Improve certain error messages #407

Open

PgBiel mentioned this pull request Dec 2, 2023

Type-safe API for external classes (GDScript, GDExtension, C#) #372

Open

Bromeon mentioned this pull request Dec 12, 2023

save and try_save functions and IO-utilites refactor in godot_core::engine refactor #535

Merged

	#[derive(Debug, PartialEq, Eq)]
	#[derive(Eq, PartialEq, Debug)]

		/// Variant value cannot be represented in target type
		BadValue,

		format!("{:?}", err.value().unwrap()),
		format!("{:?}", "hello".to_variant())

Make FromGodot conversions fallible #467

Make FromGodot conversions fallible #467

Conversation

lilizoey commented Oct 27, 2023 • edited Loading

GodotRust commented Oct 27, 2023

Bromeon left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bromeon Nov 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bromeon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bromeon commented Nov 19, 2023

Bromeon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilizoey Nov 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bromeon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Make `FromGodot` conversions fallible #467

Make `FromGodot` conversions fallible #467

lilizoey commented Oct 27, 2023 •

edited

Loading

Bromeon left a comment •

edited

Loading

Bromeon Nov 15, 2023 •

edited

Loading

lilizoey Nov 24, 2023 •

edited

Loading