Skip to content

Commit 99e8916

Browse files
BromeonBromeon
authored
Merge pull request #700 from godot-rust/bugfix/u64-conversions
Fix wrong conversions for `u64`
2 parents ed88b6f + 071ab75 commit 99e8916

File tree

10 files changed

+217
-105
lines changed

10 files changed

+217
-105
lines changed

‎godot-core/src/builtin/meta/call_error.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -188,7 +188,7 @@ impl CallError {
188188
) -> Self {
189189
// Note: reason is same wording as in FromVariantError::description().
190190
let reason = format!(
191-
"parameter #{param_index} conversion -- expected type `{expected:?}`, got `{actual:?}`"
191+
"parameter #{param_index} conversion -- expected type {expected:?}, got {actual:?}"
192192
);
193193

194194
Self::new(call_ctx, reason, None)

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

Lines changed: 17 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -236,12 +236,13 @@ impl fmt::Display for FromGodotError {
236236
pub(crate) enum FromFfiError {
237237
NullRawGd,
238238
WrongObjectType,
239-
I32,
240-
I16,
241239
I8,
242-
U32,
243-
U16,
244240
U8,
241+
I16,
242+
U16,
243+
I32,
244+
U32,
245+
U64,
245246
}
246247

247248
impl FromFfiError {
@@ -260,12 +261,13 @@ impl fmt::Display for FromFfiError {
260261
Self::WrongObjectType => {
261262
return write!(f, "given object cannot be cast to target type")
262263
}
263-
Self::I32 => "i32",
264-
Self::I16 => "i16",
265264
Self::I8 => "i8",
266-
Self::U32 => "u32",
267-
Self::U16 => "u16",
268265
Self::U8 => "u8",
266+
Self::I16 => "i16",
267+
Self::U16 => "u16",
268+
Self::I32 => "i32",
269+
Self::U32 => "u32",
270+
Self::U64 => "u64",
269271
};
270272

271273
write!(f, "`{target}` cannot store the given value")
@@ -274,12 +276,15 @@ impl fmt::Display for FromFfiError {
274276

275277
#[derive(Eq, PartialEq, Debug)]
276278
pub(crate) enum FromVariantError {
277-
/// Variant type does not match expected type
279+
/// Variant type does not match expected type.
278280
BadType {
279281
expected: VariantType,
280282
actual: VariantType,
281283
},
282284

285+
/// Value cannot be represented in target type's domain.
286+
BadValue,
287+
283288
WrongClass {
284289
expected: ClassName,
285290
},
@@ -299,10 +304,11 @@ impl fmt::Display for FromVariantError {
299304
match self {
300305
Self::BadType { expected, actual } => {
301306
// Note: wording is the same as in CallError::failed_param_conversion_engine()
302-
write!(f, "expected type `{expected:?}`, got `{actual:?}`")
307+
write!(f, "expected type {expected:?}, got {actual:?}")
303308
}
309+
Self::BadValue => write!(f, "value cannot be represented in target type's domain"),
304310
Self::WrongClass { expected } => {
305-
write!(f, "expected class `{expected}`")
311+
write!(f, "expected class {expected}")
306312
}
307313
}
308314
}

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

Lines changed: 96 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -131,7 +131,7 @@ impl FromGodot for sys::VariantOperator {
131131
// Scalars
132132

133133
macro_rules! impl_godot_scalar {
134-
($T:ty as $Via:ty, $err:path $(, $param_metadata:expr)?) => {
134+
($T:ty as $Via:ty, $err:path, $param_metadata:expr) => {
135135
impl GodotType for $T {
136136
type Ffi = $Via;
137137

@@ -144,40 +144,20 @@ macro_rules! impl_godot_scalar {
144144
}
145145

146146
fn try_from_ffi(ffi: Self::Ffi) -> Result<Self, ConvertError> {
147-
Self::try_from(ffi).map_err(|_| $err.into_error(ffi))
147+
Self::try_from(ffi).map_err(|_rust_err| {
148+
// rust_err is something like "out of range integral type conversion attempted", not adding extra information.
149+
// TODO consider passing value into error message, but how thread-safely? don't eagerly convert to string.
150+
$err.into_error(ffi)
151+
})
148152
}
149153

150-
$(
151-
fn param_metadata() -> sys::GDExtensionClassMethodArgumentMetadata {
152-
$param_metadata
153-
}
154-
)?
155-
156-
fn godot_type_name() -> String {
157-
<$Via as GodotType>::godot_type_name()
158-
}
159-
}
160-
161-
impl ArrayElement for $T {}
162-
163-
impl GodotConvert for $T {
164-
type Via = $T;
165-
}
166-
167-
impl ToGodot for $T {
168-
fn to_godot(&self) -> Self::Via {
169-
*self
170-
}
154+
impl_godot_scalar!(@shared_fns; $Via, $param_metadata);
171155
}
172156

173-
impl FromGodot for $T {
174-
fn try_from_godot(via: Self::Via) -> Result<Self, ConvertError> {
175-
Ok(via)
176-
}
177-
}
157+
impl_godot_scalar!(@shared_traits; $T);
178158
};
179159

180-
($T:ty as $Via:ty $(, $param_metadata:expr)?; lossy) => {
160+
($T:ty as $Via:ty, $param_metadata:expr; lossy) => {
181161
impl GodotType for $T {
182162
type Ffi = $Via;
183163

@@ -189,21 +169,27 @@ macro_rules! impl_godot_scalar {
189169
self as $Via
190170
}
191171

192-
fn try_from_ffi(ffi: Self::Ffi) -> Result<Self,ConvertError> {
172+
fn try_from_ffi(ffi: Self::Ffi) -> Result<Self, ConvertError> {
193173
Ok(ffi as $T)
194174
}
195175

196-
$(
197-
fn param_metadata() -> sys::GDExtensionClassMethodArgumentMetadata {
198-
$param_metadata
199-
}
200-
)?
176+
impl_godot_scalar!(@shared_fns; $Via, $param_metadata);
177+
}
201178

202-
fn godot_type_name() -> String {
203-
<$Via as GodotType>::godot_type_name()
204-
}
179+
impl_godot_scalar!(@shared_traits; $T);
180+
};
181+
182+
(@shared_fns; $Via:ty, $param_metadata:expr) => {
183+
fn param_metadata() -> sys::GDExtensionClassMethodArgumentMetadata {
184+
$param_metadata
185+
}
186+
187+
fn godot_type_name() -> String {
188+
<$Via as GodotType>::godot_type_name()
205189
}
190+
};
206191

192+
(@shared_traits; $T:ty) => {
207193
impl ArrayElement for $T {}
208194

209195
impl GodotConvert for $T {
@@ -231,47 +217,100 @@ impl_godot_as_self!(f64);
231217
impl_godot_as_self!(());
232218

233219
// Also implements ArrayElement.
234-
impl_godot_scalar!(
235-
i32 as i64,
236-
crate::builtin::meta::FromFfiError::I32,
237-
sys::GDEXTENSION_METHOD_ARGUMENT_METADATA_INT_IS_INT32
238-
);
239-
impl_godot_scalar!(
240-
i16 as i64,
241-
crate::builtin::meta::FromFfiError::I16,
242-
sys::GDEXTENSION_METHOD_ARGUMENT_METADATA_INT_IS_INT16
243-
);
244220
impl_godot_scalar!(
245221
i8 as i64,
246222
crate::builtin::meta::FromFfiError::I8,
247223
sys::GDEXTENSION_METHOD_ARGUMENT_METADATA_INT_IS_INT8
248224
);
249225
impl_godot_scalar!(
250-
u32 as i64,
251-
crate::builtin::meta::FromFfiError::U32,
252-
sys::GDEXTENSION_METHOD_ARGUMENT_METADATA_INT_IS_UINT32
226+
u8 as i64,
227+
crate::builtin::meta::FromFfiError::U8,
228+
sys::GDEXTENSION_METHOD_ARGUMENT_METADATA_INT_IS_UINT8
229+
);
230+
impl_godot_scalar!(
231+
i16 as i64,
232+
crate::builtin::meta::FromFfiError::I16,
233+
sys::GDEXTENSION_METHOD_ARGUMENT_METADATA_INT_IS_INT16
253234
);
254235
impl_godot_scalar!(
255236
u16 as i64,
256237
crate::builtin::meta::FromFfiError::U16,
257238
sys::GDEXTENSION_METHOD_ARGUMENT_METADATA_INT_IS_UINT16
258239
);
259240
impl_godot_scalar!(
260-
u8 as i64,
261-
crate::builtin::meta::FromFfiError::U8,
262-
sys::GDEXTENSION_METHOD_ARGUMENT_METADATA_INT_IS_UINT8
241+
i32 as i64,
242+
crate::builtin::meta::FromFfiError::I32,
243+
sys::GDEXTENSION_METHOD_ARGUMENT_METADATA_INT_IS_INT32
263244
);
264245
impl_godot_scalar!(
265-
u64 as i64,
266-
sys::GDEXTENSION_METHOD_ARGUMENT_METADATA_INT_IS_UINT64;
267-
lossy
246+
u32 as i64,
247+
crate::builtin::meta::FromFfiError::U32,
248+
sys::GDEXTENSION_METHOD_ARGUMENT_METADATA_INT_IS_UINT32
268249
);
269250
impl_godot_scalar!(
270251
f32 as f64,
271252
sys::GDEXTENSION_METHOD_ARGUMENT_METADATA_REAL_IS_FLOAT;
272253
lossy
273254
);
274255

256+
// ----------------------------------------------------------------------------------------------------------------------------------------------
257+
// u64: manually implemented, to ensure that type is not altered during conversion.
258+
259+
impl GodotType for u64 {
260+
type Ffi = i64;
261+
262+
fn to_ffi(&self) -> Self::Ffi {
263+
*self as i64
264+
}
265+
266+
fn into_ffi(self) -> Self::Ffi {
267+
self as i64
268+
}
269+
270+
fn try_from_ffi(ffi: Self::Ffi) -> Result<Self, ConvertError> {
271+
// Ok(ffi as u64)
272+
Self::try_from(ffi)
273+
.map_err(|_rust_err| crate::builtin::meta::FromFfiError::U64.into_error(ffi))
274+
}
275+
276+
impl_godot_scalar!(@shared_fns; i64, sys::GDEXTENSION_METHOD_ARGUMENT_METADATA_INT_IS_UINT64);
277+
}
278+
279+
impl GodotConvert for u64 {
280+
type Via = u64;
281+
}
282+
283+
impl ToGodot for u64 {
284+
fn to_godot(&self) -> Self::Via {
285+
*self
286+
}
287+
288+
fn to_variant(&self) -> Variant {
289+
// TODO panic doesn't fit the trait's infallibility too well; maybe in the future try_to_godot/try_to_variant() methods are possible.
290+
i64::try_from(*self)
291+
.map(|v| v.to_variant())
292+
.unwrap_or_else(|_| {
293+
panic!("to_variant(): u64 value {} is not representable inside Variant, which can only store i64 integers", self)
294+
})
295+
}
296+
}
297+
298+
impl FromGodot for u64 {
299+
fn try_from_godot(via: Self::Via) -> Result<Self, ConvertError> {
300+
Ok(via)
301+
}
302+
303+
fn try_from_variant(variant: &Variant) -> Result<Self, ConvertError> {
304+
// Fail for values that are not representable as u64.
305+
let value = variant.try_to::<i64>()?;
306+
307+
u64::try_from(value).map_err(|_rust_err| {
308+
// TODO maybe use better error enumerator
309+
crate::builtin::meta::FromVariantError::BadValue.into_error(value)
310+
})
311+
}
312+
}
313+
275314
// ----------------------------------------------------------------------------------------------------------------------------------------------
276315
// Raw pointers
277316

‎godot-core/src/obj/instance_id.rs

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -76,17 +76,19 @@ impl Debug for InstanceId {
7676
}
7777

7878
impl GodotConvert for InstanceId {
79-
type Via = u64;
79+
// Use i64 and not u64 because the former can be represented in Variant, and is also the number format GDScript uses.
80+
// The engine's C++ code can still use u64.
81+
type Via = i64;
8082
}
8183

8284
impl ToGodot for InstanceId {
8385
fn to_godot(&self) -> Self::Via {
84-
self.value.get()
86+
self.to_i64()
8587
}
8688
}
8789

8890
impl FromGodot for InstanceId {
8991
fn try_from_godot(via: Self::Via) -> Result<Self, ConvertError> {
90-
Self::try_from_u64(via).ok_or_else(|| FromGodotError::ZeroInstanceId.into_error(via))
92+
Self::try_from_i64(via).ok_or_else(|| FromGodotError::ZeroInstanceId.into_error(via))
9193
}
9294
}

‎itest/godot/TestSuite.gd

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ func assert_that(what: bool, message: String = "") -> bool:
2727
print_error("GDScript assertion failed: %s" % message)
2828
else:
2929
print_error("GDScript assertion failed.")
30+
3031
return false
3132

3233
func assert_eq(actual, expected, message: String = "") -> bool:
@@ -37,9 +38,11 @@ func assert_eq(actual, expected, message: String = "") -> bool:
3738

3839
print_newline() # previous line not yet broken
3940
if message:
40-
print_error("GDScript assertion failed: %s\n actual: %s\n expected: %s" % [message, actual, expected])
41+
print_error("GDScript assertion failed: %s\n actual: %s\n expected: %s" % [message, actual, expected])
4142
else:
42-
print_error("GDScript assertion failed: `(actual == expected)`\n actual: %s\n expected: %s" % [actual, expected])
43+
print_error("GDScript assertion failed: `(actual == expected)`\n actual: %s\n expected: %s" % [actual, expected])
44+
45+
# Note: stacktrace cannot be printed, because not connected to a debugging server (editor).
4346
return false
4447

4548
# Disable error message printing from godot.

0 commit comments

Comments
 (0)