From c2514d88e015ee8d0e703effc8d871f702673aad Mon Sep 17 00:00:00 2001 From: Dheatly23 <71598333+Dheatly23@users.noreply.github.com> Date: Tue, 12 Mar 2024 15:20:32 +0700 Subject: [PATCH] Add Erased Convert Error Type --- godot-core/src/builtin/array.rs | 2 +- godot-core/src/builtin/meta/call_error.rs | 26 ++- .../meta/godot_convert/convert_error.rs | 169 +++++++++++------- godot-core/src/builtin/variant/impls.rs | 6 +- godot-core/src/obj/raw.rs | 4 +- godot-macros/src/derive/derive_from_godot.rs | 4 +- itest/rust/src/builtin_tests/convert_test.rs | 23 +-- 7 files changed, 144 insertions(+), 90 deletions(-) diff --git a/godot-core/src/builtin/array.rs b/godot-core/src/builtin/array.rs index 83e104c23..0cf0bd6a5 100644 --- a/godot-core/src/builtin/array.rs +++ b/godot-core/src/builtin/array.rs @@ -929,7 +929,7 @@ impl GodotFfiVariant for Array { expected: Self::variant_type(), actual: variant.get_type(), } - .into_error(variant)); + .into_error(variant.clone())); } let array = unsafe { diff --git a/godot-core/src/builtin/meta/call_error.rs b/godot-core/src/builtin/meta/call_error.rs index e0ef9446e..4c60e6019 100644 --- a/godot-core/src/builtin/meta/call_error.rs +++ b/godot-core/src/builtin/meta/call_error.rs @@ -5,7 +5,7 @@ * file, You can obtain one at https://mozilla.org/MPL/2.0/. */ -use crate::builtin::meta::{CallContext, ConvertError, ToGodot}; +use crate::builtin::meta::{CallContext, ConvertError, ErasedConvertError, ToGodot}; use crate::builtin::Variant; use crate::sys; use godot_ffi::{join_debug, VariantType}; @@ -314,7 +314,10 @@ impl CallError { function_name: call_ctx.function_name.to_string(), call_expr: format!("{call_ctx}()"), reason: reason.into(), - source: source.map(|e| SourceError::Convert(Box::new(e))), + source: source.map(|e| SourceError::Convert { + value: e.value().map_or_else(String::new, |v| format!("{:?}", v)), + erased_error: e.into(), + }), } } @@ -342,7 +345,15 @@ impl CallError { // }; let source_str = match &self.source { - Some(SourceError::Convert(e)) if with_source => format!("\n Source: {}", e), + Some(SourceError::Convert { + erased_error, + value, + }) if with_source => { + format!( + "\n Source: {erased_error}{}{value}", + if value.is_empty() { "" } else { ": " }, + ) + } Some(SourceError::Call(e)) if with_source => format!("\n Source: {}", e.message(true)), _ => String::new(), }; @@ -360,7 +371,9 @@ impl fmt::Display for CallError { impl Error for CallError { fn source(&self) -> Option<&(dyn Error + 'static)> { match self.source.as_ref() { - Some(SourceError::Convert(e)) => deref_to::(e), + Some(SourceError::Convert { + erased_error: e, .. + }) => deref_to::(e), Some(SourceError::Call(e)) => deref_to::(e), None => None, } @@ -372,7 +385,10 @@ impl Error for CallError { #[derive(Debug)] enum SourceError { - Convert(Box), + Convert { + erased_error: ErasedConvertError, + value: String, + }, Call(Box), } diff --git a/godot-core/src/builtin/meta/godot_convert/convert_error.rs b/godot-core/src/builtin/meta/godot_convert/convert_error.rs index 5d06291db..12b0dc0e7 100644 --- a/godot-core/src/builtin/meta/godot_convert/convert_error.rs +++ b/godot-core/src/builtin/meta/godot_convert/convert_error.rs @@ -10,7 +10,11 @@ use std::fmt; use godot_ffi::VariantType; -use crate::builtin::{array_inner, meta::ClassName}; +use crate::builtin::{ + array_inner, + meta::{ClassName, ToGodot}, + Variant, +}; type Cause = Box; @@ -20,8 +24,7 @@ type Cause = Box; #[derive(Debug)] pub struct ConvertError { kind: ErrorKind, - cause: Option, - value_str: Option, + value: Option, } impl ConvertError { @@ -30,77 +33,71 @@ impl ConvertError { /// If you don't need a custom message, consider using [`ConvertError::default()`] instead. pub fn new(user_message: impl Into) -> Self { Self { - kind: ErrorKind::Custom(Some(user_message.into())), - cause: None, - value_str: None, + kind: ErrorKind::Custom(Some(user_message.into().into())), + ..Default::default() } } /// Create a new custom error for a conversion with the value that failed to convert. pub(crate) fn with_kind_value(kind: ErrorKind, value: V) -> Self where - V: fmt::Debug, + V: ToGodot, { Self { kind, - cause: None, - value_str: Some(format!("{value:?}")), + value: Some(value.to_variant()), } } - /// Create a new custom error with a rust-error as an underlying cause for the conversion error. - #[doc(hidden)] - pub fn with_cause(cause: C) -> Self + /// Create a new custom error wrapping an [`Error`]. + pub fn with_error(error: E) -> Self where - C: Into, + E: Into>, { Self { - cause: Some(cause.into()), + kind: ErrorKind::Custom(Some(error.into())), ..Default::default() } } - /// Create a new custom error with a rust-error as an underlying cause for the conversion error, and the - /// value that failed to convert. - #[doc(hidden)] - pub fn with_cause_value(cause: C, value: V) -> Self + /// Create a new custom error wrapping an [`Error`] and the value that failed to convert. + pub fn with_error_value(error: E, value: V) -> Self where - C: Into, - V: fmt::Debug, + E: Into>, + V: ToGodot, { Self { - cause: Some(cause.into()), - value_str: Some(format!("{value:?}")), - ..Default::default() + kind: ErrorKind::Custom(Some(error.into())), + value: Some(value.to_variant()), } } /// Returns the rust-error that caused this error, if one exists. - pub fn cause(&self) -> Option<&(dyn Error + Send + Sync)> { - self.cause.as_deref() + pub fn cause(&self) -> Option<&(dyn Error + Send + Sync + 'static)> { + match &self.kind { + ErrorKind::Custom(Some(cause)) => Some(&**cause), + _ => None, + } } - /// Returns a string representation of the value that failed to convert, if one exists. - pub fn value_str(&self) -> Option<&str> { - self.value_str.as_deref() + /// Returns a reference of the value that failed to convert, if one exists. + pub fn value(&self) -> Option<&Variant> { + self.value.as_ref() } - fn description(&self) -> Option { - self.kind.description() + /// Converts error into generic error type. It is useful to send error across thread. + /// Do note that some data might get lost during conversion. + pub fn into_erased(self) -> impl Error + Send + Sync { + ErasedConvertError::from(self) } } impl fmt::Display for ConvertError { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - match (self.description(), self.cause.as_ref()) { - (Some(desc), Some(cause)) => write!(f, "{desc}: {cause}")?, - (Some(desc), None) => write!(f, "{desc}")?, - (None, Some(cause)) => write!(f, "{cause}")?, - (None, None) => write!(f, "unknown error: {:?}", self.kind)?, - } + write!(f, "{}", self.kind)?; - if let Some(value) = self.value_str.as_ref() { - write!(f, ": {value}")?; + if let Some(value) = &self.value { + write!(f, ": {value:?}")?; } Ok(()) @@ -109,9 +106,7 @@ impl fmt::Display for ConvertError { impl Error for ConvertError { fn source(&self) -> Option<&(dyn Error + 'static)> { - self.cause - .as_ref() - .map(|cause| &**cause as &(dyn Error + 'static)) + self.cause().map(|v| v as &(dyn Error + 'static)) } } @@ -122,27 +117,54 @@ impl Default for ConvertError { fn default() -> Self { Self { kind: ErrorKind::Custom(None), - cause: None, - value_str: None, + value: None, } } } -#[derive(Eq, PartialEq, Debug)] +/// Erased type of [`ConvertError`]. +#[derive(Debug)] +pub(crate) struct ErasedConvertError { + kind: ErrorKind, +} + +impl From for ErasedConvertError { + fn from(v: ConvertError) -> Self { + let ConvertError { kind, .. } = v; + Self { kind } + } +} + +impl fmt::Display for ErasedConvertError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "{}", self.kind) + } +} + +impl Error for ErasedConvertError { + fn source(&self) -> Option<&(dyn Error + 'static)> { + match &self.kind { + ErrorKind::Custom(Some(cause)) => Some(&**cause), + _ => None, + } + } +} + +#[derive(Debug)] pub(crate) enum ErrorKind { FromGodot(FromGodotError), FromFfi(FromFfiError), FromVariant(FromVariantError), - Custom(Option), + Custom(Option), } -impl ErrorKind { - fn description(&self) -> Option { +impl fmt::Display for ErrorKind { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { - Self::FromGodot(from_godot) => Some(from_godot.description()), - Self::FromVariant(from_variant) => Some(from_variant.description()), - Self::FromFfi(from_ffi) => Some(from_ffi.description()), - Self::Custom(description) => description.clone(), + Self::FromGodot(from_godot) => write!(f, "{from_godot}"), + Self::FromVariant(from_variant) => write!(f, "{from_variant}"), + Self::FromFfi(from_ffi) => write!(f, "{from_ffi}"), + Self::Custom(cause) => write!(f, "{cause:?}"), } } } @@ -162,23 +184,27 @@ pub(crate) enum FromGodotError { impl FromGodotError { pub fn into_error(self, value: V) -> ConvertError where - V: fmt::Debug, + V: ToGodot, { ConvertError::with_kind_value(ErrorKind::FromGodot(self), value) } +} - fn description(&self) -> String { +impl fmt::Display for FromGodotError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { Self::BadArrayType { expected, actual } => { if expected.variant_type() != actual.variant_type() { return if expected.is_typed() { - format!( + write!( + f, "expected array of type {:?}, got array of type {:?}", expected.variant_type(), actual.variant_type() ) } else { - format!( + write!( + f, "expected untyped array, got array of type {:?}", actual.variant_type() ) @@ -191,14 +217,15 @@ impl FromGodotError { "BadArrayType with expected == got, this is a gdext bug" ); - format!( + write!( + f, "expected array of class {}, got array of class {}", expected.class_name(), actual.class_name() ) } - Self::InvalidEnum => "invalid engine enum value".into(), - Self::ZeroInstanceId => "`InstanceId` cannot be 0".into(), + Self::InvalidEnum => write!(f, "invalid engine enum value"), + Self::ZeroInstanceId => write!(f, "`InstanceId` cannot be 0"), } } } @@ -220,15 +247,19 @@ pub(crate) enum FromFfiError { impl FromFfiError { pub fn into_error(self, value: V) -> ConvertError where - V: fmt::Debug, + V: ToGodot, { ConvertError::with_kind_value(ErrorKind::FromFfi(self), value) } +} - fn description(&self) -> String { +impl fmt::Display for FromFfiError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { let target = match self { - Self::NullRawGd => return "`Gd` cannot be null".into(), - Self::WrongObjectType => return "given object cannot be cast to target type".into(), + Self::NullRawGd => return write!(f, "`Gd` cannot be null"), + Self::WrongObjectType => { + return write!(f, "given object cannot be cast to target type") + } Self::I32 => "i32", Self::I16 => "i16", Self::I8 => "i8", @@ -237,7 +268,7 @@ impl FromFfiError { Self::U8 => "u8", }; - format!("`{target}` cannot store the given value") + write!(f, "`{target}` cannot store the given value") } } @@ -257,19 +288,21 @@ pub(crate) enum FromVariantError { impl FromVariantError { pub fn into_error(self, value: V) -> ConvertError where - V: fmt::Debug, + V: ToGodot, { ConvertError::with_kind_value(ErrorKind::FromVariant(self), value) } +} - fn description(&self) -> String { +impl fmt::Display for FromVariantError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { Self::BadType { expected, actual } => { // Note: wording is the same as in CallError::failed_param_conversion_engine() - format!("expected type `{expected:?}`, got `{actual:?}`") + write!(f, "expected type `{expected:?}`, got `{actual:?}`") } Self::WrongClass { expected } => { - format!("expected class `{expected}`") + write!(f, "expected class `{expected}`") } } } @@ -277,5 +310,5 @@ impl FromVariantError { fn __ensure_send_sync() { fn check() {} - check::(); + check::(); } diff --git a/godot-core/src/builtin/variant/impls.rs b/godot-core/src/builtin/variant/impls.rs index c6fb2c67b..f99f961f8 100644 --- a/godot-core/src/builtin/variant/impls.rs +++ b/godot-core/src/builtin/variant/impls.rs @@ -41,7 +41,7 @@ macro_rules! impl_ffi_variant { expected: Self::variant_type(), actual: variant.get_type(), } - .into_error(variant)); + .into_error(variant.clone())); } // For 4.0: @@ -138,7 +138,7 @@ mod impls { impl_ffi_variant!(Dictionary, dictionary_to_variant, dictionary_from_variant); impl_ffi_variant!(i64, int_to_variant, int_from_variant; int); impl_ffi_variant!(f64, float_to_variant, float_from_variant; float); - + } // ---------------------------------------------------------------------------------------------------------------------------------------------- @@ -159,7 +159,7 @@ impl GodotFfiVariant for () { expected: VariantType::Nil, actual: variant.get_type(), } - .into_error(variant)) + .into_error(variant.clone())) } } diff --git a/godot-core/src/obj/raw.rs b/godot-core/src/obj/raw.rs index c044e8224..1ffdbc7bd 100644 --- a/godot-core/src/obj/raw.rs +++ b/godot-core/src/obj/raw.rs @@ -628,7 +628,9 @@ impl Drop for RawGd { impl Clone for RawGd { fn clone(&self) -> Self { out!("RawGd::clone"); - self.check_rtti("clone"); + if !self.is_null() { + self.check_rtti("clone"); + } if !self.is_null() { unsafe { Self::from_obj_sys(self.obj as sys::GDExtensionObjectPtr) } diff --git a/godot-macros/src/derive/derive_from_godot.rs b/godot-macros/src/derive/derive_from_godot.rs index ebaeb6de4..e482b43a1 100644 --- a/godot-macros/src/derive/derive_from_godot.rs +++ b/godot-macros/src/derive/derive_from_godot.rs @@ -62,7 +62,7 @@ fn make_fromgodot_for_int_enum(name: &Ident, enum_: &CStyleEnum, int: &Ident) -> #discriminants => Ok(#name::#names), )* // Pass `via` and not `other`, to retain debug info of original type. - other => Err(::godot::builtin::meta::ConvertError::with_cause_value(#bad_variant_error, via)) + other => Err(::godot::builtin::meta::ConvertError::with_error_value(#bad_variant_error, via)) } } } @@ -83,7 +83,7 @@ fn make_fromgodot_for_gstring_enum(name: &Ident, enum_: &CStyleEnum) -> TokenStr #names_str => Ok(#name::#names), )* // Pass `via` and not `other`, to retain debug info of original type. - other => Err(::godot::builtin::meta::ConvertError::with_cause_value(#bad_variant_error, via)) + other => Err(::godot::builtin::meta::ConvertError::with_error_value(#bad_variant_error, via)) } } } diff --git a/itest/rust/src/builtin_tests/convert_test.rs b/itest/rust/src/builtin_tests/convert_test.rs index 7fb5bc365..0949d0603 100644 --- a/itest/rust/src/builtin_tests/convert_test.rs +++ b/itest/rust/src/builtin_tests/convert_test.rs @@ -59,7 +59,7 @@ fn error_has_value_and_no_cause() { for (err, err_str) in errors.into_iter() { assert!( - err.value_str().is_some(), + err.value().is_some(), "{err_str} conversion has no value: {err:?}" ); assert!( @@ -76,15 +76,15 @@ fn error_has_value_and_no_cause() { fn error_maintains_value() { let value = i32::MAX; let err = Vector2Axis::try_from_godot(value).unwrap_err(); - assert_eq!(format!("{value:?}"), err.value_str().unwrap()); + assert_eq!(format!("{value:?}"), format!("{:?}", err.value().unwrap())); let value = i64::MAX; let err = value.to_variant().try_to::().unwrap_err(); - assert_eq!(format!("{value:?}"), err.value_str().unwrap()); + assert_eq!(format!("{value:?}"), format!("{:?}", err.value().unwrap())); let value = f64::MAX.to_variant(); let err = value.try_to::().unwrap_err(); - assert_eq!(format!("{value:?}"), err.value_str().unwrap()); + assert_eq!(format!("{value:?}"), format!("{:?}", err.value().unwrap())); } // Manual implementation of `GodotConvert` and related traits to ensure conversion works. @@ -117,16 +117,16 @@ impl FromGodot for Foo { fn try_from_godot(via: Self::Via) -> Result { let a = match via.get("a") { Some(a) => a, - None => return Err(ConvertError::with_cause_value(Self::MISSING_KEY_A, via)), + None => return Err(ConvertError::with_error_value(Self::MISSING_KEY_A, via)), }; let b = match via.get("b") { Some(b) => b, - None => return Err(ConvertError::with_cause_value(Self::MISSING_KEY_B, via)), + None => return Err(ConvertError::with_error_value(Self::MISSING_KEY_B, via)), }; if via.len() > 2 { - return Err(ConvertError::with_cause_value(Self::TOO_MANY_KEYS, via)); + return Err(ConvertError::with_error_value(Self::TOO_MANY_KEYS, via)); } Ok(Self { @@ -198,7 +198,7 @@ fn custom_convert_error_from_variant() { assert!(err.cause().is_none()); assert_eq!( - err.value_str().unwrap(), + format!("{:?}", err.value().unwrap()), format!("{:?}", "hello".to_variant()) ); @@ -213,7 +213,7 @@ fn custom_convert_error_from_variant() { assert!(err.cause().is_none()); assert_eq!( - err.value_str().unwrap(), + format!("{:?}", err.value().unwrap()), format!("{:?}", Vector2::new(1.0, 23.4).to_variant()) ); @@ -227,5 +227,8 @@ fn custom_convert_error_from_variant() { .expect_err("should have too big value for field `a`"); assert!(err.cause().is_none()); - assert_eq!(err.value_str().unwrap(), format!("{:?}", i64::MAX)); + assert_eq!( + format!("{:?}", err.value().unwrap()), + format!("{:?}", i64::MAX) + ); }