[FEATURE REQUEST] TryFrom<u32> for glib::Enum

[swsnr]

Currently, #[derive(glib::Enum)] generates the following trait impls to convert the integer value of the enum back to the enum value itself:

impl glib::translate::TryFromGlib<i32> for MyEnum {
    type Error = i32;
    #[inline]
    unsafe fn try_from_glib(value: i32) -> ::core::result::Result<Self, i32> {
        let from_glib = || {       
            [..] // Lengthy if dispatch over all variants here
            ::core::option::Option::None
        };
        from_glib().ok_or(value)
    }
}
impl glib::translate::FromGlib<i32> for MyEnum {
    #[inline]
    unsafe fn from_glib(value: i32) -> Self {
        use glib::translate::TryFromGlib;
        Self::try_from_glib(value).unwrap()
    }
}

This is literally just a perfectly safe TryFrom<u32> except that it's unsafe because TryFromGlib itself is unsafe, presumably because conversion from Glib is usually unsafe for non-trivial types because pointers and such.

But since the conversion from i32 to an enum value is perfectly safe, it'd be helpful if glib::Enum could first generate a safe TryFrom<i32> and then create TryFromGlib and FromGlib on top of that, presumably for Gir compatibility. In other words, it'd be helpful if glib::Enum generated this instead:

impl ::core::convert::TryFrom<i32> for MyEnum {
    type Error = i32;
    #[inline]
    unsafe fn try_from(value: i32) -> ::core::result::Result<Self, i32> {
        let from_glib = || {       
            [..] // Lengthy if dispatch over all variants here
            ::core::option::Option::None
        };
        from_glib().ok_or(value)
    }
}
impl glib::translate::TryFromGlib<i32> for MyEnum {
    type Error = i32;
    #[inline]
    unsafe fn try_from_glib(value: i32) -> ::core::result::Result<Self, i32> {
        use ::core::convert::TryFrom;
        MyEnum::try_from(value)
    }
}
impl glib::translate::FromGlib<i32> for MyEnum {
    #[inline]
    unsafe fn from_glib(value: i32) -> Self {
        use glib::translate::TryFromGlib;
        Self::try_from_glib(value).unwrap()
    }
}

I'd be willing to make a pull request for this, if it's acceptable.

See original discussion in Matrix.

[swsnr]

@sdroege As discussed in Matrix.

[sdroege]

So, FromGlib should probably stay because panicking From impls are not very nice. But TryFromGlib can probably go completely away and be replaced by TryFrom, and not just for enums but the trait itself should be possible to remove from glib.

@fengalin Do you remember why TryFromGlib exists and why it's unsafe? Is there anything I'm missing?

[swsnr]

Mh, while I can make a pull request to implement the change suggested in the description, I'm not sure I feel comfortable refactoring all of glib and probably Gir as well 😇

I can certainly try, but it'll take time and likely require quite a bit of hand holding 😬😅

[fengalin]

This is literally just a perfectly safe TryFrom except that it's unsafe because TryFromGlib itself is unsafe, presumably because conversion from Glib is usually unsafe for non-trivial types because pointers and such.

Calling these conversions is unsafe even for trivial types as they allow you to pass a stronger-typed value, which must comply with all their invariants, to the rest of your Rust code, which will trust all those invariants hold.

We introduced TryFromGlib for GStreamer's formatted values initially. In C, those values are all represented by a u64, sometimes coerced to an i64, but they can actually convey time, byte, percent, ... representations. As a matter of fact, there's also a sentinel value to indicate that the value is not defined (None in Rust).

From a Rust standpoint, dealing with an Option<ClockTime> is not the same as dealing with a ClockTime or a Percent, while they are all u64s in C. The only way to make sure the Rust code can trust these representations is by enforcing proper checks at the ffi, which is the purpose of unsafe. If a C function can return a ClockTime or a Percent for instance, you would write a safe wrapper around an unsafe block which would differentiate the return type based on some other information. That's the only place where this needs to be checked, the rest of the Rust code can then trust the type.

Would you take the risk of your program trusting a MyEnum built from a random i32 that was found lying in memory? 😄

[swsnr]

@fengalin I can literally just copy the TryFromGlib body out of the macro expansion into a safe TryFrom implementation (or use derive_more to generate more or less exactly the same code for me). Pardon me, but I don't see what unsafe should protect against here, from a Rust standpoint?

I see your point about C enums crossing FFI boundaries, but this issue is about Rust enums with glib::Enum derived for the purpose of putting them into properties or e.g. into adw::EnumListModel.

[sdroege]

@fengalin What you write seems to apply to FromGlib (if we didn't panic there, which we do so maybe that one is actually fine too) but not to TryFromGlib. Are you aware of any impl of either types that can lead to problems? Option<ClockTime> from u64 is a 1:1 mapping AFAICS.

[bilelmoussaoui]

Imho, the introspected enums already have an unknown variant that holds any value that is > N. So what does unsafe serves here really?

[fengalin]

I see your point about C enums crossing FFI boundaries, but this issue is about Rust enums with glib::Enum derived for the purpose of putting them into properties or e.g. into adw::EnumListModel.

My point is about crossing FFI boundaries, this is why TryFromGlib was introduced for. I can't speak for #[derive(glib::Enum)].

@slomo @bilelmoussaoui Sorry my English is poor. I was talking about ensuring the type conversion is appropriate regarding the API. In other words, the unsafe marker makes sure the developer who writes the FFI bindings pays extra attention to a potentially wrong conversion. When you get a u64 for a Percent out of a C function, and you return a ClockTime, the resulting type is wrong from a Rust standpoint and that violates assumptions in subsequent code. Even returning a ClockTime instead of an Option<ClockTime> is wrong. So imo it's not unsafe in the sense that this may break memory alignment or some such, it's unsafe in the sense of an API contract enforcement.

Quoting the doc:

Not all uses of unsafe are equivalent: some are here to mark the existence of a contract the programmer must check, others are to say “I have checked the contract, go ahead and do this”.

[sdroege]

Ah I see what you mean. But OTOH that's not very different from any other TryFrom or From impl between integers and enums out there. E.g. derive-more is basically deriving exactly that TryFrom impl for enums: https://docs.rs/derive_more/latest/derive_more/derive.TryFrom.html

That our integers come via FFI does not really change anything here. It wouldn't be different if that integer comes via JSON from some web service.

[fengalin]

I don't remember all the details now (and I'm on my phone rn :)), but this trait interacts with other traits in glib::translate to help when dealing with from_glib(). Like the Error associated type encodes whether the conversion results in an Option or an Error and gir automatically generates .expect() when the function is defined to return the inner type iirc. See also https://docs.rs/glib/latest/glib/translate/index.html

[swsnr]

How do we proceed? Replacing the FromGlib traits seems to require a bit of a discussion, so can I proceed to make a pull request to add a separate TryFrom as in the original description above, to at least address my immediate pain point? 😇

[bilelmoussaoui]

How do we proceed? Replacing the FromGlib traits seems to require a bit of a discussion, so can I proceed to make a pull request to add a separate TryFrom as in the original description above, to at least address my immediate pain point? 😇

I would go with that for now, yes

[sdroege]

At least until someone has time to think the implications of the wider change through.

[sdroege]

OTOH you can as well just use derive-more for deriving that TryFrom impl so not sure we need to hurry anything here

[swsnr]

No hurry, of course, but I think the change is pretty obvious, and it's cheap too, just a few lines in the derive macro. 🤷

But whatever you like, it's not like this is blocking anything. As you say, derive more exists 😊