Skip to content

Commit

Permalink
Improve error messages + test (also in other GodotConvert cases)
Browse files Browse the repository at this point in the history
  • Loading branch information
Bromeon committed Dec 2, 2024
1 parent 54c7721 commit 7b4ee37
Show file tree
Hide file tree
Showing 6 changed files with 89 additions and 13 deletions.
5 changes: 2 additions & 3 deletions godot-core/src/meta/error/call_error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -188,9 +188,8 @@ impl CallError {
expected: VariantType,
) -> Self {
// Note: reason is same wording as in FromVariantError::description().
let reason = format!(
"parameter #{param_index} conversion -- expected type {expected:?}, got {actual:?}"
);
let reason =
format!("parameter #{param_index} -- cannot convert from {actual:?} to {expected:?}");

Self::new(call_ctx, reason, None)
}
Expand Down
19 changes: 14 additions & 5 deletions godot-core/src/meta/error/convert_error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -183,10 +183,13 @@ pub(crate) enum FromGodotError {

/// Cannot map object to `dyn Trait` because none of the known concrete classes implements it.
UnimplementedDynTrait {
trait_id: std::any::TypeId,
trait_name: String,
class_name: String,
},

/// Cannot map object to `dyn Trait` because none of the known concrete classes implements it.
UnregisteredDynTrait { trait_name: String },

/// `InstanceId` cannot be 0.
ZeroInstanceId,
}
Expand Down Expand Up @@ -242,12 +245,18 @@ impl fmt::Display for FromGodotError {
Self::InvalidEnum => write!(f, "invalid engine enum value"),
Self::ZeroInstanceId => write!(f, "`InstanceId` cannot be 0"),
Self::UnimplementedDynTrait {
trait_id,
trait_name,
class_name,
} => {
write!(
f,
"none of the derived classes from {class_name} have been registered with trait {trait_id:?}"
"none of the classes derived from `{class_name}` have been linked to trait `{trait_name}` with #[godot_dyn]"
)
}
FromGodotError::UnregisteredDynTrait { trait_name } => {
write!(
f,
"trait `{trait_name}` has not been registered with #[godot_dyn]"
)
}
}
Expand Down Expand Up @@ -328,11 +337,11 @@ impl fmt::Display for FromVariantError {
match self {
Self::BadType { expected, actual } => {
// Note: wording is the same as in CallError::failed_param_conversion_engine()
write!(f, "expected type {expected:?}, got {actual:?}")
write!(f, "cannot convert from type {actual:?} to {expected:?}")
}
Self::BadValue => write!(f, "value cannot be represented in target type's domain"),
Self::WrongClass { expected } => {
write!(f, "expected class {expected}")
write!(f, "cannot convert to class {expected}")
}
}
}
Expand Down
11 changes: 6 additions & 5 deletions godot-core/src/registry/class.rs
Original file line number Diff line number Diff line change
Expand Up @@ -292,13 +292,14 @@ pub fn auto_register_rpcs<T: GodotClass>(object: &mut T) {
pub(crate) fn try_dynify_object<T: GodotClass, D: ?Sized + 'static>(
object: Gd<T>,
) -> Result<DynGd<T, D>, ConvertError> {
let typeid = std::any::TypeId::of::<D>();
let typeid = any::TypeId::of::<D>();
let trait_name = sys::short_type_name::<D>();

// Iterate all classes that implement the trait.
let dyn_traits_by_typeid = global_dyn_traits_by_typeid();
let relations = dyn_traits_by_typeid.get(&typeid).unwrap_or_else(|| {
panic!("Trait '{typeid:?}' has not been registered with #[godot_dyn].");
});
let Some(relations) = dyn_traits_by_typeid.get(&typeid) else {
return Err(FromGodotError::UnregisteredDynTrait { trait_name }.into_error(object));
};

// TODO maybe use 2nd hashmap instead of linear search.
// (probably not pair of typeid/classname, as that wouldn't allow the above check).
Expand All @@ -322,7 +323,7 @@ pub(crate) fn try_dynify_object<T: GodotClass, D: ?Sized + 'static>(
}

let error = FromGodotError::UnimplementedDynTrait {
trait_id: typeid,
trait_name,
class_name: dynamic_class.to_string(),
};

Expand Down
19 changes: 19 additions & 0 deletions itest/rust/src/builtin_tests/containers/variant_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,25 @@ fn variant_bad_conversions() {
.expect_err("`nil` should not convert to `Dictionary`");
}

#[itest]
fn variant_bad_conversion_error_message() {
let variant = 123.to_variant();

let err = variant
.try_to::<GString>()
.expect_err("i32 -> GString conversion should fail");
assert_eq!(
err.to_string(),
"cannot convert from type INT to STRING: 123"
);

// TODO this error isn't great, but unclear whether it can be improved. If not, document.
let err = variant
.try_to::<Gd<Node>>()
.expect_err("i32 -> Gd<Node> conversion should fail");
assert_eq!(err.to_string(), "`Gd` cannot be null: null");
}

#[itest]
fn variant_array_bad_conversions() {
let i32_array: Array<i32> = array![1, 2, 160, -40];
Expand Down
34 changes: 34 additions & 0 deletions itest/rust/src/object_tests/dyn_gd_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -295,6 +295,40 @@ fn dyn_gd_store_in_godot_array() {
array.at(1).free();
}

#[itest]
fn dyn_gd_error_unregistered_trait() {
trait UnrelatedTrait {}

let obj = Gd::from_object(RefcHealth { hp: 33 }).into_dyn::<dyn Health>();

let variant = obj.to_variant();
let back = variant.try_to::<DynGd<RefcHealth, dyn UnrelatedTrait>>();

let err = back.expect_err("DynGd::try_to() should have failed");
let expected_err = {
// The conversion fails before a DynGd is created, so Display still operates on the Gd.
let obj = obj.into_gd();

format!("trait `dyn UnrelatedTrait` has not been registered with #[godot_dyn]: {obj:?}")
};

assert_eq!(err.to_string(), expected_err);
}

#[itest]
fn dyn_gd_error_unimplemented_trait() {
let obj = RefCounted::new_gd(); //Gd::from_object(RefcHealth { hp: 33 }).into_dyn::<dyn Health>();

let variant = obj.to_variant();
let back = variant.try_to::<DynGd<RefCounted, dyn Health>>();

let err = back.expect_err("DynGd::try_to() should have failed");
assert_eq!(
err.to_string(),
format!("none of the classes derived from `RefCounted` have been linked to trait `dyn Health` with #[godot_dyn]: {obj:?}")
);
}

// ----------------------------------------------------------------------------------------------------------------------------------------------
// Example symbols

Expand Down
14 changes: 14 additions & 0 deletions itest/rust/src/object_tests/object_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -518,6 +518,20 @@ fn object_engine_convert_variant_nil() {
});
}

#[itest]
fn object_engine_convert_variant_error() {
let refc = RefCounted::new_gd();
let variant = refc.to_variant();

let err = Gd::<Area2D>::try_from_variant(&variant)
.expect_err("`Gd<RefCounted>` should not convert to `Gd<Area2D>`");

assert_eq!(
err.to_string(),
format!("cannot convert to class Area2D: {refc:?}")
);
}

#[itest]
fn object_engine_returned_refcount() {
let Some(file) = FileAccess::open("res://itest.gdextension", file_access::ModeFlags::READ)
Expand Down

0 comments on commit 7b4ee37

Please sign in to comment.