Skip to content

Commit a4c8074

Browse files
BromeonBromeon
committed
Make ConvertError: Send + Sync
Instead of a value in a `Box<dyn Debug>`, which cannot really be used except for debug printing, ConvertError now stores a string containing the Debug repr. value() method is now value_str(). Adjusted call-sites to use reference where clones were previously necessary. Also adds static test for `Send`/`Sync`.
1 parent 77e3b95 commit a4c8074

File tree

5 files changed

+58
-38
lines changed

5 files changed

+58
-38
lines changed

‎godot-core/src/builtin/array.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -815,7 +815,7 @@ impl<T: GodotType> GodotFfiVariant for Array<T> {
815815
expected: Self::variant_type(),
816816
got: variant.get_type(),
817817
}
818-
.into_error(variant.clone()));
818+
.into_error(variant));
819819
}
820820

821821
let array = unsafe {

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

Lines changed: 38 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ type Cause = Box<dyn Error + Send + Sync>;
1919
pub struct ConvertError {
2020
kind: ErrorKind,
2121
cause: Option<Cause>,
22-
value: Option<Box<dyn fmt::Debug>>,
22+
value: Option<String>,
2323
}
2424

2525
impl ConvertError {
@@ -41,25 +41,35 @@ impl ConvertError {
4141
}
4242

4343
/// Create a new custom error for a conversion with the value that failed to convert.
44-
pub fn with_value<V: fmt::Debug + 'static>(value: V) -> Self {
44+
pub fn with_value<V>(value: V) -> Self
45+
where
46+
V: fmt::Debug,
47+
{
4548
let mut err = Self::custom();
46-
err.value = Some(Box::new(value));
49+
err.value = Some(format!("{value:?}"));
4750
err
4851
}
4952

5053
/// Create a new custom error with a rust-error as an underlying cause for the conversion error.
51-
pub fn with_cause<C: Into<Cause>>(cause: C) -> Self {
54+
pub fn with_cause<C>(cause: C) -> Self
55+
where
56+
C: Into<Cause>,
57+
{
5258
let mut err = Self::custom();
5359
err.cause = Some(cause.into());
5460
err
5561
}
5662

5763
/// Create a new custom error with a rust-error as an underlying cause for the conversion error, and the
5864
/// value that failed to convert.
59-
pub fn with_cause_value<C: Into<Cause>, V: fmt::Debug + 'static>(cause: C, value: V) -> Self {
65+
pub fn with_cause_value<C, V>(cause: C, value: V) -> Self
66+
where
67+
C: Into<Cause>,
68+
V: fmt::Debug,
69+
{
6070
let mut err = Self::custom();
6171
err.cause = Some(cause.into());
62-
err.value = Some(Box::new(value));
72+
err.value = Some(format!("{value:?}"));
6373
err
6474
}
6575

@@ -68,8 +78,8 @@ impl ConvertError {
6878
self.cause.as_deref()
6979
}
7080

71-
/// Returns the value that failed to convert, if one exists.
72-
pub fn value(&self) -> Option<&(dyn fmt::Debug)> {
81+
/// Returns a string representation of the value that failed to convert, if one exists.
82+
pub fn value_str(&self) -> Option<&str> {
7383
self.value.as_deref()
7484
}
7585

@@ -140,11 +150,14 @@ pub(crate) enum FromGodotError {
140150
}
141151

142152
impl FromGodotError {
143-
pub fn into_error<V: fmt::Debug + 'static>(self, value: V) -> ConvertError {
153+
pub fn into_error<V>(self, value: V) -> ConvertError
154+
where
155+
V: fmt::Debug,
156+
{
144157
ConvertError {
145158
kind: ErrorKind::FromGodot(self),
146159
cause: None,
147-
value: Some(Box::new(value)),
160+
value: Some(format!("{value:?}")),
148161
}
149162
}
150163

@@ -199,11 +212,14 @@ pub(crate) enum FromFfiError {
199212
}
200213

201214
impl FromFfiError {
202-
pub fn into_error<V: fmt::Debug + 'static>(self, value: V) -> ConvertError {
215+
pub fn into_error<V>(self, value: V) -> ConvertError
216+
where
217+
V: fmt::Debug,
218+
{
203219
ConvertError {
204220
kind: ErrorKind::FromFfi(self),
205221
cause: None,
206-
value: Some(Box::new(value)),
222+
value: Some(format!("{value:?}")),
207223
}
208224
}
209225

@@ -237,11 +253,14 @@ pub(crate) enum FromVariantError {
237253
}
238254

239255
impl FromVariantError {
240-
pub fn into_error<V: fmt::Debug + 'static>(self, value: V) -> ConvertError {
256+
pub fn into_error<V>(self, value: V) -> ConvertError
257+
where
258+
V: fmt::Debug,
259+
{
241260
ConvertError {
242261
kind: ErrorKind::FromVariant(self),
243262
cause: None,
244-
value: Some(Box::new(value)),
263+
value: Some(format!("{value:?}")),
245264
}
246265
}
247266

@@ -256,3 +275,8 @@ impl FromVariantError {
256275
}
257276
}
258277
}
278+
279+
fn __ensure_send_sync() {
280+
fn check<T: Send + Sync>() {}
281+
check::<ConvertError>();
282+
}

‎godot-core/src/builtin/variant/impls.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ macro_rules! impl_ffi_variant {
4242
expected: Self::variant_type(),
4343
got: variant.get_type(),
4444
}
45-
.into_error(variant.clone()));
45+
.into_error(variant));
4646
}
4747

4848
// For 4.0:
@@ -160,7 +160,7 @@ impl GodotFfiVariant for () {
160160
expected: VariantType::Nil,
161161
got: variant.get_type(),
162162
}
163-
.into_error(variant.clone()))
163+
.into_error(variant))
164164
}
165165
}
166166

‎godot-macros/src/derive/derive_from_variant.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -262,7 +262,7 @@ fn make_enum_tuple(
262262
} else {
263263
quote! {
264264
let #ident = variant.pop_front()
265-
.ok_or(ConvertError::with_cause_value("missing expected value", variant.clone()))?
265+
.ok_or(ConvertError::with_cause_value("missing expected value", &variant))?
266266
.try_to::<#field_type>()?;
267267
}
268268
};
@@ -298,7 +298,7 @@ fn make_enum_named(
298298
let err = format!("missing expected value {field_name_string}");
299299
quote! {
300300
let #field_name = variant.get(#field_name_string)
301-
.ok_or(ConvertError::with_cause_value(#err, variant.clone()))?
301+
.ok_or(ConvertError::with_cause_value(#err, &variant))?
302302
.try_to::<#field_type>()?;
303303
}
304304
};

‎itest/rust/src/builtin_tests/convert_test.rs

Lines changed: 15 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ fn error_has_value_and_no_cause() {
5959

6060
for (err, err_str) in errors.into_iter() {
6161
assert!(
62-
err.value().is_some(),
62+
err.value_str().is_some(),
6363
"{err_str} conversion has no value: {err:?}"
6464
);
6565
assert!(
@@ -74,22 +74,17 @@ fn error_has_value_and_no_cause() {
7474
/// Check that the value stored in an error is the same as the value we tried to convert.
7575
#[itest]
7676
fn error_maintains_value() {
77-
// Need to use Debug to check equality here since we get a `dyn Debug` and thus cannot check equality
78-
// directly.
7977
let value = i32::MAX;
8078
let err = Vector2Axis::try_from_godot(value).unwrap_err();
81-
assert_eq!(format!("{value:?}"), format!("{:?}", err.value().unwrap()));
79+
assert_eq!(format!("{value:?}"), err.value_str().unwrap());
8280

8381
let value = i64::MAX;
8482
let err = value.to_variant().try_to::<i32>().unwrap_err();
85-
assert_eq!(format!("{value:?}"), format!("{:?}", err.value().unwrap()));
83+
assert_eq!(format!("{value:?}"), err.value_str().unwrap());
8684

87-
let value = f64::MAX;
88-
let err = value.to_variant().try_to::<i32>().unwrap_err();
89-
assert_eq!(
90-
format!("{:?}", value.to_variant()),
91-
format!("{:?}", err.value().unwrap())
92-
);
85+
let value = f64::MAX.to_variant();
86+
let err = value.try_to::<i32>().unwrap_err();
87+
assert_eq!(format!("{value:?}"), err.value_str().unwrap());
9388
}
9489

9590
// Manual implementation of `GodotConvert` and related traits to ensure conversion works.
@@ -167,6 +162,7 @@ fn custom_convert_error_from_variant() {
167162
.to_variant()
168163
.try_to::<Foo>()
169164
.expect_err("should be missing key `a`");
165+
170166
assert_eq!(err.cause().unwrap().to_string(), Foo::MISSING_KEY_A);
171167

172168
let missing_b = dict! {
@@ -176,6 +172,7 @@ fn custom_convert_error_from_variant() {
176172
.to_variant()
177173
.try_to::<Foo>()
178174
.expect_err("should be missing key `b`");
175+
179176
assert_eq!(err.cause().unwrap().to_string(), Foo::MISSING_KEY_B);
180177

181178
let too_many_keys = dict! {
@@ -187,6 +184,7 @@ fn custom_convert_error_from_variant() {
187184
.to_variant()
188185
.try_to::<Foo>()
189186
.expect_err("should have too many keys");
187+
190188
assert_eq!(err.cause().unwrap().to_string(), Foo::TOO_MANY_KEYS);
191189

192190
let wrong_type_a = dict! {
@@ -197,11 +195,10 @@ fn custom_convert_error_from_variant() {
197195
.to_variant()
198196
.try_to::<Foo>()
199197
.expect_err("should have wrongly typed key `a`");
198+
200199
assert!(err.cause().is_none());
201-
// Need to use Debug to check equality here since we get a `dyn Debug` and thus cannot check equality
202-
// directly.
203200
assert_eq!(
204-
format!("{:?}", err.value().unwrap()),
201+
err.value_str().unwrap(),
205202
format!("{:?}", "hello".to_variant())
206203
);
207204

@@ -213,9 +210,10 @@ fn custom_convert_error_from_variant() {
213210
.to_variant()
214211
.try_to::<Foo>()
215212
.expect_err("should have wrongly typed key `b`");
213+
216214
assert!(err.cause().is_none());
217215
assert_eq!(
218-
format!("{:?}", err.value().unwrap()),
216+
err.value_str().unwrap(),
219217
format!("{:?}", Vector2::new(1.0, 23.4).to_variant())
220218
);
221219

@@ -227,9 +225,7 @@ fn custom_convert_error_from_variant() {
227225
.to_variant()
228226
.try_to::<Foo>()
229227
.expect_err("should have too big value for field `a`");
228+
230229
assert!(err.cause().is_none());
231-
assert_eq!(
232-
format!("{:?}", err.value().unwrap()),
233-
format!("{:?}", i64::MAX)
234-
);
230+
assert_eq!(err.value_str().unwrap(), format!("{:?}", i64::MAX));
235231
}

0 commit comments

Comments
 (0)