Option<T> does not implement OwnedToVariant when T implements only OwnedToVariant.

[B-head]

For example, the following code will not work.

#[derive(NativeClass)]
#[no_constructor]
struct Foo {}

#[methods]
impl Foo {
    #[export]
    fn get(&self, _owner: &Reference) -> Option<Instance<Foo, Unique>> {
            Some((Foo{}).emplace())
    }
}

This issue can be avoided by using into_shared() to perform type conversion.

#[derive(NativeClass)]
#[no_constructor]
struct Foo {}

#[methods]
impl Foo {
    #[export]
    fn get(&self, _owner: &Reference) -> Option<Instance<Foo>> {
            Some((Foo{}).emplace().into_shared())
    }
}

I tried to solve this issue, but implementing impl<T: OwnedToVariant > OwnedToVariant for Option<T> causes a conflict with impl<T: ToVariant> OwnedToVariant for T.

[Bromeon]

I'm not sure how to address this now, due to the conflicts you mentioned. Rust's lack of specialization is quite limiting sometimes.

Any ideas?

[B-head]

I'm not familiar with Rust either😅 In conclusion, I do not have an easy idea. So what I write below is for reference only.

First, since there is no specialization in Rust, I would remove impl<T: ToVariant> OwnedToVariant for T. After removing them, there are two possible designs.

Implement separately

Require users to implement ToVariant and OwnedToVariant separately.

#[derive(ToVariant, OwnedToVariant)]
struct Foo {}

Pros

• Library code is easy to implement.
• The meaning of each trait is clear.

Cons

• User codes are redundant.
• Care should be taken to maintain implementation consistency for ToVariant and OwnedToVariant.

Standard library approach

The same approach is used to implement the standard library in From (as Into). The define of ToVariant is changed as follows, and OwnedToVariant is removed.

trait ToVariant {
    // Note that there is no & in self.
    fn to_variant(self) -> Variant;
}

In the implementation to the trait, separate implementations are made for T corresponding to OwnedToVariant and &T corresponding to the old ToVariant. (String in the standard library is helpful)

impl ToVariant for Foo {
     fn to_variant(self) -> Variant {
        /* Implementation in move (or copy) semantics. */
     }
}

impl ToVariant for &'_ Foo {
     fn to_variant(self) -> Variant {
        /* Implementation in clone semantics. */
     }
}

Pros

• Users can implement only what they need.
• Ensure extensibility of library code.
• Standard library implementations can be helpful.

Cons

• The meaning of trait is implementation-dependent.

What I feel

I think it is silly to make breaking changes to solve small issues that have workarounds. So better to resolve this issue in conjunction with #808 and related issues.

[Bromeon]

I'm not even sure how many users manually implement ToVariant, OwnedToVariant etc. As an uneducated guess, I'd say most people are happy with the conversions that Ref, Instance and the core types provide.

I agree that we shouldn't break the code for 0.10, so I'll leave this open for later. One option might also be that we allow

#[derive(ToVariant)]`
struct MyStruct { ... }

which would use OwnedToVariant.

[Bromeon]

Assigned to 0.11, but honestly not sure if this can be solvable at all. If we don't get any ideas in reasonable time, I'd tend to close this.

Maybe there can be some takeaways for GDExtension w.r.t. how to design those traits.

[chitoyuu]

I think https://github.com/godot-rust/godot-rust/issues/869#issuecomment-1068641915 is the approach one would have taken if the traits were to be designed from scratch. This is the same way serde and co does it.

The OwnedToVariant trait was only created to avoid changing the signature of the earlier ToVariant, IIRC.

I don't think there are a lot of people who write their own ToVariant implementations, outside of a few specific cases that the macro doesn't handle yet like #546 which I'm working on. Once that's dealt with I don't think the impact of the change would be as negative if at all.

[chitoyuu]

Made an attempt at this, but unfortunately ran into a rustc bug with impls on references: https://github.com/rust-lang/rust/issues/39959

Seems like this will have to be delayed until the upstream bug is fixed, as there isn't an obvious workaround that can be reliably generated.