Re-organize Variant conversion methods

[chitoyuu]

Inherent conversion methods are now genericized into new, to, try_to, and coerce_to, to reduce the clutter and make other Variant methods more discoverable in docs.

• new replaces the from_* methods. The original version that returns nil variants is renamed to Variant::nil, to better reflect its behavior.
• to and try_to replaces the try_to_* methods, with naming more consistent with the rest of the Rust ecosystem.
• to_object and try_to_object are kept for convenience around Ref.
• coerce_to replaces the to_* methods. A new trait CoerceFromVariant is introduced and implemented for all GDScript built-in types.
• Documentation is updated around convertion methods/traits to highlight what is used for exported methods.

Close #774

Rare PR with negative LoC

[chitoyuu]

Most of this should be consistent with discussions in #774 and #dev. There might be an argument against the new/nil renaming though: a nil-returning new() is consistent with the Default implementation, even if it isn't immediately obvious what it does from the naming. I think this should be a point open to discussion.

[Bromeon]

to and try_to replaces the try_to_* methods, with naming more consistent with the rest of the Rust ecosystem.

Great idea, I like the approach of separating Option and Result return types. For those who don't have the docs, here are the new methods:

pub fn new<T>(from: T) -> Variant; 
pub fn nil() -> Variant;
pub fn to<T>(&self) -> Option<T>;
pub fn try_to<T>(&self) -> Result<T, FromVariantError>;
pub fn coerce_to<T>(&self) -> T;
pub fn to_object<T>(&self) -> Option<Ref<T, Shared>>;
pub fn try_to_object<T>(&self) -> Result<Ref<T, Shared>, FromVariantError>;

Rare PR with negative LoC

These ones are really satisfying 🙂

[bors]

try

Build succeeded:

• full-ci

[Bromeon]

This looks good! Only point is as mentioned the name new(), which is now "overloaded" for possible types. But I don't really know a better name.

As discussed, from() has the problem that it may look like implementation of the standard trait From, and would thus imply Into as well. But it might be a bit more symmetric with the opposing functions Variant::to() and FromVariant::from_variant().

I'll leave this for a few days for comments. If no more ideas or change requests come up, I'd merge it then. Worst case the constructor can still be renamed later (if necessary with deprecation).

[Bromeon]

Something I noticed while using the new API: the generic functions use type inference due to their nature. This can be nice, but it also makes things less explicit. In combination with the "weak type safety" for numeric types, the new API adds potential for bugs that wasn't present before.

To show the principle:

let unsigned = u64::MAX;               // value: 18446744073709551615
let variant = Variant::new(unsigned);  // value: -1
let back: u8 = variant.to().unwrap();  // value: 255

The same in a more real-world use case:

fn accept_int(int: Option<i16>) { ... }

let variant = Variant::new(-15);
accept_int(variant.to()); // all good

// Later: user refactors i16 -> u16
// Everything compiles, runtime bug

// Note: this would not have happened without type inference:
accept_int(variant.to::<i16>()); // compile error

A completely unrelated scenario:

let dict: Dictionary = ...;
let var = gd::Variant::new(dict);  // looks like move
println!("Dict: {:?}", var);       // works -- not moved!

The examples are not a direct problem of this API, but rather of the underlying traits To/FromVariant traits, which do not check integral conversions and are implemented for references.

In rare cases, implicit narrowing/signed-change can be desired behavior (e.g. u64 -> Variant(i64) -> u64 to get back original value), but I'd say in most cases it's a bug. The problem didn't exist before, as Variant's own interface was more high-level and only offered i64, requiring the user to explicitly convert from/to different integer types via as or try_into(). Similar with the move/by-ref syntax: the previous from_dictionary(&dict) explicitly took a reference.

This is not per se an argument against the new API, the added genericity is nice. But we have to be aware that it encourages patterns which are not very typical for Rust. In the future (not necessarily this PR) we could a) document the behavior and hint to the VariantDispatch alternative, and/or b) consider making To/FromVariant integral-safe by default, with explicit opt-out, or something along those lines.

[chitoyuu]

... the new API adds potential for bugs that wasn't present before.

To show the principle:

let unsigned = u64::MAX;               // value: 18446744073709551615
let variant = Variant::new(unsigned);  // value: -1
let back: u8 = variant.to().unwrap();  // value: 255

This is how To/FromVariant always worked for those types. I agree that we can consider adding bound checks in the relevant impls, but this isn't new behavior introduced by this PR.

Between i64 and u64 in particular, I don't see how we can distinguish one from the other, given how the same variant type is used for both of them.

The same in a more real-world use case:

fn accept_int(int: Option<i16>) { ... }

let variant = Variant::new(-15);
accept_int(variant.to()); // all good

// Later: user refactors i16 -> u16
// Everything compiles, runtime bug

// Note: this would not have happened without type inference:
accept_int(variant.to::<i16>()); // compile error

I'm not sure if we shouldn't accept some runtime errors as par of the course when working with dynamic typing through Variants. In this specific case it's easy to notice that the constant is negative, due to its proximity to the variant.to() call. In a more-real world, that variant is probably coming from somewhere else, and as such the extra compile-time error would not have prevented any runtime bugs, since it isn't calling attention to where the problematic value is produced, merely where it's used.

A completely unrelated scenario:

let dict: Dictionary = ...;
let var = gd::Variant::new(dict);  // looks like move
println!("Dict: {:?}", var);       // works -- not moved!

I'm not sure I understand what you mean by this? dict is moved in this case. You wouldn't be able to print it if you tried. It should come expected that printing the resulting variant would work.

a) document the behavior and hint to the VariantDispatch alternative, and/or b) consider making To/FromVariant integral-safe by default, with explicit opt-out, or something along those lines.

I agree that it can be helpful to have checks for overflow/underflow when converting to smaller integers. More documentation is also always good.

[Bromeon]

This is how To/FromVariant always worked for those types. I agree that we can consider adding bound checks in the relevant impls, but this isn't new behavior introduced by this PR.

Yes, that's exactly what I meant with "The examples are not a direct problem of this API, but rather of the underlying traits To/FromVariant [...]". 🙂 The Variant API itself is a bit higher-level and more likely to be used directly, though.

I'm not sure if we shouldn't accept some runtime errors as par of the course when working with dynamic typing through Variants.

The problem is, there is no runtime error. The conversion silently narrows integers, i.e. produces bugs that can propagate further. In case you meant bugs with errors, maybe you're right and some should be expected.

On the other hand, the presence of Option/Result return types in to()/try_to() is highly indicative of safe conversions. Rustaceans likely expect it to succeed or fail explicitly, similar to u32value.try_into::<u16>() for example.

I understand it's not that easy, because the To/FromVariant traits are used in many places, e.g. the implicit conversions in exported method parameters/return types. But one could argue those might benefit as well. In any case, I think we could discuss this in a separate issue, there's not too much this API could do (beyond docs) to mitigate the issue.

I'm not sure I understand what you mean by this? dict is moved in this case. You wouldn't be able to print it if you tried. It should come expected that printing the resulting variant would work.

Sorry, my fault. Apart from the typo, I had an unimplemented!() macro for Dictionary initialization, and the compiler simply disregarded the remaining code due to the never type, and compiled fine. Makes sense, but I didn't think of it.

(this code, to be clear)

let dict: Dictionary = unimplemented!();
let var = Variant::new(dict);
println!("Dict: {:?}", dict);

[Bromeon]

Going to merge this; the To/FromVariant type checks can be discussed independently.

bors r+

[bors]

Build succeeded:

• full-ci