Implement ToOwned for Event<&'a OsStr>

[i509VCB]

I am using inotify with calloop and one issue I've noticed is the lack of ability to easily convert an event from the methods into an owned event to pass to calloop. Calloop gets kinda messy when an Event in an event source has a lifetime, so getting an owned copy is typically the best way around this issue.

I noticed the presence of into_ownedthat is only available with the streams api, I think we could convert this to delegate to an implementation of ToOwned and made always available.

[i509VCB]

Looks like the proper fix is going to be pending future rust releases, per

error[E0119]: conflicting implementations of trait `std::borrow::ToOwned` for type `events::Event<&std::ffi::OsStr>`
   --> src/events.rs:218:1
    |
218 | impl<'a> ToOwned for Event<&'a OsStr> {
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: conflicting implementation in crate `alloc`:
            - impl<T> ToOwned for T
              where T: Clone;error[E0119]: conflicting implementations of trait `std::borrow::ToOwned` for type `events::Event<&std::ffi::OsStr>`
   --> src/events.rs:218:1
    |
218 | impl<'a> ToOwned for Event<&'a OsStr> {
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: conflicting implementation in crate `alloc`:
            - impl<T> ToOwned for T
              where T: Clone;

For now I will make into_owned available outside of the stream feature then.

[hannobraun]

Hi @i509VCB, thank you for your pull requests! I'm currently on vacation, but I'll be back next week and will take a look then (or maybe the week after, if there's too much of a backlog to catch up on).

[i509VCB]

Ah no worries

[hannobraun]

#179 has been merged, which is a partial resolution to this issue.

[talklittle]

I was curious about this, and from this StackOverflow question it sounds like it is not really needed to implement the ToOwned trait unless there is a specific use case that needs that trait in particular. Having the Event.to_owned() non-trait method may be sufficient, if I understand correctly.

I am not familiar with the calloop crate, but a search over its GitHub source does not find any references to the ToOwned trait.

[i509VCB]

So in the case of calloop, I need to have an owned event since an event source in calloop has an Event assoicated type. If the assoicated type has a lifetime, then it gets quite ugly fast.

[talklittle]

The Event.to_owned() method already satisfies that need to get an owned Event, right? I'm having trouble understanding why the ToOwned trait should be implemented in addition to that?

Reading the SO question again, it sounds like ToOwned isn't really a fit for Event, as its intent is to be applied to reference types like str and OsStr to get a String and OsString. Event on the other hand is not a reference type. We can still define Event.to_owned() as a convention as a lightweight clone, but that doesn't imply we must implement ToOwned.

Maybe the proper solution here is to implement Clone ourselves (just rename to_owned() to clone() essentially), and then we can use the built-in ToOwned impl for Clone types. And then delete our to_owned() method as it would delegate to clone().

Edit: That last paragraph didn't make sense as to_owned() is converting the &OsStr to an owned OsString.

[talklittle]

Okay, I now understand what was meant by this change "pending future Rust releases," presumably referring to impl specialization and RFC 1210. So hypothetically if/when RFC 1210 stabilizes then it will be possible to implement ToOwned here.

[hannobraun]

Thank you for looking into this, @talklittle! I've looked at the documentation of ToOwned, and I believe you're right: the trait doesn't fit Event, and the existing to_owned method should suffice.

Closing this issue. @i509VCB, feel free to reopen, if you disagree.