Make Variant conversions hard to misuse

[Bromeon]

There are multiple ways to convert Variant into certain types:

// Do a best effort to create an i64 out of the variant, return default value if type is not convertible
fn Variant::to_i64(&self) -> i64;

// Returns Some(i64) if this variant has exactly type i64, None otherwise.
fn Variant::try_to_i64(&self) -> Option<i64>;

// Returns Ok(i64) if this variant has exactly type i64, Err otherwise
fn i64::from_variant(variant: &Variant) -> Result<Self, FromVariantError>;

// Match the variant's dispatch enum
if let VariantDispatch::I64(value) = variant.dispatch() { ... }

and back:

fn Variant::from_i64(v: i64) -> Variant;
fn i64::to_variant(&self) -> Variant;
fn i64::owned_to_variant(self) -> Variant;

Implementation (after cargo expand)

impl Variant {
    pub fn from_i64(v: i64) -> Variant {
        unsafe {
            let api = get_api();
            let mut dest = sys::godot_variant::default();
            (api.godot_variant_new_int)(&mut dest, v);
            Variant(dest)
        }
    }

    pub fn to_i64(&self) -> I64 {
        unsafe {
            #[allow(clippy::useless_transmute)]
            transmute((get_api().godot_variant_as_int)(&self.0))
        }
    }

    pub fn try_to_i64(&self) -> Option<I64> {
        i64::from_variant(self).ok()
    }

    // Helper method
    fn try_as_sys_of_type(
        &self,
        expected: VariantType,
    ) -> Result<&sys::godot_variant, FromVariantError> {
        let variant_type = self.get_type();
        if variant_type != expected {
            return Err(...);
        }
        Ok(&self.0) // self.0 is the sys handle
    }
}


impl<T: ToVariant> OwnedToVariant for T {
    #[inline]
    fn owned_to_variant(self) -> Variant {
        self.to_variant()
    }
}

impl ToVariantEq for i64 {}

impl ToVariant for i64 {
    #[inline]
    fn to_variant(&self) -> Variant {
        Variant::from_i64(*self)
    }
}

impl FromVariant for i64 {
    #[inline]
    fn from_variant(variant: &Variant) -> Result<Self, FromVariantError> {
        variant
            .try_as_sys_of_type(VariantType::I64)
            .map(|v| unsafe { (get_api().godot_variant_as_int)(v) })
    }
}

impl ToVariantEq for i32 {}

impl ToVariant for i32 {
    #[inline]
    fn to_variant(&self) -> Variant {
        ((*self) as i64).to_variant()
    }
}

impl FromVariant for i32 {
    #[inline]
    fn from_variant(variant: &Variant) -> Result<Self, FromVariantError> {
        <i64>::from_variant(variant).map(|i| i as Self)
    }
}

---

Current behavior

Mostly the conversion to types is interesting.
There are two behaviors:

1. to_i64() -- use GDNative API godot_variant_as_int, emulating GDScript implicit conversions
2. try_to_i64(), i64::from_variant() -- only convert if the variant indeed has exactly this type

In Rust being a strongly typed language with explicit conversions, case 1. can sometimes be surprising, especially if a conversion silently fails and an arbitrary default value is chosen. A while ago I tested how values constructed in GDScript are converted using Rust Variant's conversion methods:

Conversion tests

These are the good cases and more or less expected, but it gets problematic when a type is not representable in the target type, and a default like 0, false, "" etc. is chosen (TODO: include those tests).

---

Possible changes

My suggestion is to rename try_to_*() methods to to_*(), so the most straightforward way to convert returns an Option, making the caller aware of potential errors and handling them explicitly.

For the existing to_*() methods, we have multiple options:

1. remove them, if they are deemed more dangerous than helpful (this might limit some valid use cases, like float -> int conversion)
2. keep them as is, but name them more explicitly, e.g. coerce_to_*() or to_*_or_default()
3. name them something like convert_to_*(), change return type to Option<T> and only return Some(value) on meaningful conversions. That would mean String("3.2") -> f32(3.2) would be OK, but String("3.2k") -> f32(0.0) would not happen. This will likely need a lot of effort and checking cases manually.
4. rethink the entire "direct conversion API" in the presence of the VariantDispatch enum

Independently of what we end up with, we should improve documentation for Variant:

• list each possibility of conversion from/to variants, with their respective use cases
• explain which mechanism the implicit conversion for #[export] method parameters and return types uses
• elabore new VariantDispatch API and recommend its usage with match and if let

[Waridley]

At the moment I'm thinking users should just convert types in Rust if that's what they want, as opposed to letting Godot return its so-called "best effort" conversion... Gives them more control, better documentation, possibly better performance, and access to more idiomatic API's.

However, I also feel like there is value in exposing all of the functions that Godot gives us, in case some users want to use them... Converting from GDScript to Rust is easier if the API and behavior matches, but learning idiomatic Rust might be easier if the API is adapted...

[jacobsky]

As I mostly program in rust and attempt to use the ToVariant and FromVariant traits as little as possible, I wasn't completely aware of this behavior in some of the conversions. I think that there is merit to having both a "best effort conversion" in addition to a more "rusty" option that will panic or return an error when the conversion fails.

One note is that I always took the semantics of ToVariant and FromVariant to be identical to that of the To/From and TryTo/TryFrom where to_* will panic if it cannot convert while try_to_* will return a Result<T, E>. I wasn't aware that it was always going to be a best effort conversion, though I suppose that makes sense if it is using the Godot API.

I see two specific use-cases

1. Mostly Rust with some GDScript. This is what I am mostly doing and I'd prefer a more rusty interface where we use the more idiomatic error handling.
2. Mostly GDScript with performance critical sections in Rust. I've been speaking to some new users that have been asking for help with various conversions between the two languages and for this case, I believe it would be much easier to allow them to leverage the godot interface. Since in most cases they are just trying to convert between GDScript and Rust, keeping the consistency will be very meaningful for them.

At the moment I essentially always convert in the following manner.

fn set_value(&mut self, _: &Node, variant: Variant) {
    if let Some(value) = variant.try_to_int64() {
       self.value. = value;
    } else {
        godot_error!("Value is not int64");
    }
}

[super-continent]

For the past couple days I have been learning to use these bindings and my experience is that there are a couple pitfalls which affected me and without being informed about them I would not have been able to know what was wrong.

One major variant conversion issue was being unable to take an array within an array of arrays containing strings and convert it to a TypedArray<GodotString>, this is supposedly because it doesn't have the type PoolStringArray. In my case, I would have assumed that this VariantArray would properly convert to a TypedArray<GodotString> because it all variants contained within it were of type GodotString

[Bromeon]

@super-continent Is it about conversion from Variant? If yes, could you edit your post to add some code that illustrates the issue?

If Variant is not involved, it could also be a candiate for #758 (and then maybe you could comment there).

[super-continent]

Edited for more clarification, it does seem mainly related to variant conversions being a bit confusing

[Waridley]

Is it about conversion from Variant?

Their original message in Discord mentioned the try_to_array and try_to_string_array methods, so it does seem they were converting from a Variant, not just from an existing VariantArray.

[Waridley]

Would this be a good place to discuss the idea of replacing Variant with the current VariantDispatch, or should that be a separate issue?

[Bromeon]

@Waridley I would open a separate issue for that.