Allow ignoring enum variants in `To/FromVariant`

[wyattjsmith1]

Note: In this issue, I will discuss both rust's enum variants, and Godot's Variants. To avoid confusion I will use an upper case V for Godot's as it is a class, and lowercase v for rust's enums.

Problem

Currently, we can derive ToVariant: https://docs.rs/gdnative/0.9.3/gdnative/core_types/trait.ToVariant.html. This works well, and for the cases where it isn't perfect, we can add #[variant(...)] attributes to specify how the macro should process certain pieces of data. One group of these attributes is #[variant(skip_to_variant|skip_from_variant|skip)]. As the name implies, this will skip a field when converting to/from a Variant. Currently, this must be placed on a struct's field:

#[derive(ToVariant, FromVariant)]
struct MyStruct {
  converted_field: u8,
  #[variant(skip)]
  skipped_field: u8,
}

One area this could be expanded is to skipping enum variants. For example, the following currently results in a compiler error:

#[derive(ToVariant, FromVariant)]
enum MyEnum {
   VariantA,
  #[variant(skip)]
  VariantB(NonToVariantType),
}

In this example, the variant(skip) is ignored, and because VariantB contains a non-ToVariant field, this fails to compile.

Proposal

Allow #[variant(skip)] (and its siblings) to be placed on enum variants. This does pose the following issues:

1. What if we try to call from_variant with a skipped variant?: We should consider this a failure and return an Err as if we called from_variant with a Dictionary that isn't a variant.
2. What if we try to call to_variant with a skipped variant?: There are a couple possibilities:

• Return Nil or possible empty dict. Benefits are no crashing, disadvantages is that there may be silent bugs.
• panic. Benefits are that we are bringing the issues up to the developer immediately, disadvantage is that panic is not really rusty when it could be better represented as an Result or something.

3. Why bother ignoring certain variants?: This admittedly comes from a specific use case I have, but I believe it would be good to expand the use of variant(skip). There may be cases where we need to convert a subset of enum variants to Variants while still retaining the non-To/FromVariant variants. My solution in the short-term will be to add #[variant(skip)] to the fields in my variant that are non-To/FromVariant.

[Bromeon]

Related:

• https://github.com/godot-rust/godot-rust/issues/546
• https://github.com/godot-rust/godot-rust/issues/444

[chitoyuu]

Question 2 is an interesting one: originally the ToVariant trait was only meant to be implemented for simple types that are easily converted to Variant representations, which is I assume why it was designed to be infallible. This made features like this hard to support though, and I wonder if it's best to redesign the trait to return Result instead, possibly with a provided panicking method for convenience.

Since this doesn't seem to be a very commonly requested feature, it might be best to leave it until the trait redesigns are done: https://github.com/godot-rust/gdnative/issues/869#issuecomment-1278649972.

[Bromeon]

Question 2 is an interesting one: originally the ToVariant trait was only meant to be implemented for simple types that are easily converted to Variant representations, which is I assume why it was designed to be infallible. This made features like this hard to support though, and I wonder if it's best to redesign the trait to return Result instead, possibly with a provided panicking method for convenience.

This is an interesting topic that I also came across during GDExtension development. In my case, the type in question was u64. I didn't want to use GDNative's behavior where values are silently cast to signed -- but it would be possible to support conversion from the "valid range" of u64, namely the first half of values, which remain the same value when converted to i64.

Since every other conversion was infallible, I decided for now that it was more type-safe to disallow u64 (and leave casts to explicit user code), and to make ToVariant always succeed if it can be implemented.

This example shows though that there may be other use cases. The trade-off I don't like here is that 99% of ToVariant cases are indeed infallible. Introducing Result would weaken API expressivity and introduce tons of unwrap()s all over the place -- a problem that godot-rust already suffers from.

An alternative might be two separate traits, similar to From and TryFrom, but this also adds a lot of complexity and the orphan rule might not be happy with it, either...