allow deriving of `ToVariant` and `FromVariant` for uninhabitable enums

[karroffel]

At the moment this code will not compile:

#[derive(ToVariant, FromVariant)]
pub enum Test {}

This should be possible however as both traits can fail during the conversion. Test::from_variant(&v) will ALWAYS be an error, on the other hand the ToVariant implementation can never be called in the first place because no value of type Test could exist in the first place.

This is probably not very useful for most users but there's also no reason to disallow this.

[ghost]

I think ToVariant already works for uninhabitable enums before #443, so it's only FromVariant that need to be changed.

[karroffel]

You are right!

The ToVariant version panics at runtime (could just return an error instead? Does it make a difference?) https://github.com/godot-rust/godot-rust/blob/c37a3649b685bdbcdc5319c9b94e64f42b063c12/gdnative-derive/src/variant/to.rs#L30-L33

The FromVariant version panics at compile time instead.

https://github.com/godot-rust/godot-rust/blob/c37a3649b685bdbcdc5319c9b94e64f42b063c12/gdnative-derive/src/variant/from.rs#L32-L34

This should not be too hard to change in future though.

[ghost]

to_variant always returns Variant currently so a panic is necessary without changing the trait definition. I don't currently see a lot of point in allowing it to fail, and panic should be perfectly fine here since it should be unreachable.

[Bogay]

Which error should the from_variant implementation return? Is it always a FromVariantError::UnknownEnumVariant? (because there isn't any valid variant for an uninhabitable enum) Or is it stilled needed to check the input variant is a valid enum representation? (i.e. a dict with only 1 entry)

[Bromeon]

from_variant() for inhabitable enums will unconditionally fail, so I don't think we should choose something with an expected field, as that suggests the user has made a mistake. Maybe this might warrant a new variant (however, breaking change because not #[non_exhaustive]).

I wonder generally if this is useful -- it basically translates a compile-time error (trait not implemented) to a runtime error (Result::Err or panic if unwrapped). If the uninhabited enum is part of a larger struct/enum for which FromVariant is derived, then it will break the entire conversion.

So... what's the use case?

[chitoyuu]

I guess InvalidEnumRepr could also be an option? An uninhabitable type can be thought to have a representation nothing else is compatible with. Also technically breaking due to lack of #[non_exhaustive] on VariantEnumRepr. That was quite the oversight.

If the uninhabited enum is part of a larger struct/enum for which FromVariant is derived, then it will break the entire conversion.

I don't think it would, since the enums are externally tagged. The implementation won't be invoked unless the input has the corresponding tag.

So... what's the use case?

I think it can be used for API prototyping, where you know that you'll need something enum-shaped as an option, just nothing for now? e.g.

#[derive(FromVariant)]
struct DoStuffOptions {
    v1: DoStuffOptionsV1,
    extensions: Option<DoStuffOptionsExt>,
}

#[derive(FromVariant)]
enum DoStuffOptionsExt {
    // Maybe later: V2(DoStuffOptionsV2), V3(DoStuffOptionsV3), etc...
}

[Bromeon]

I think it can be used for API prototyping, where you know that you'll need something enum-shaped as an option, just nothing for now?

That makes a lot of sense, thanks for elaboration!

I guess InvalidEnumRepr could also be an option? An uninhabitable type can be thought to have a representation nothing else is compatible with. Also technically breaking due to lack of #[non_exhaustive] on VariantEnumRepr. That was quite the oversight.

True. Generally I have nothing against releasing gdnative 0.12 in 1-2 months, but ideally we wouldn't have to let the PR go stale until then. Maybe we can fix some other things until then.

It would be possible to choose one of the existing error types, and then break that in the next minor version. This might be even worse if the user relies on the representation, but most people usually just check if it's an error, not which one. Tough call...

[Bogay]

@Bromeon Sorry, I didn't have a use case in mind... just thought others may need this feature.