Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow foreign implementations of PartialEq, Eq, Hash etc traits #2043

Open
mgeisler opened this issue Mar 20, 2024 · 5 comments
Open

Allow foreign implementations of PartialEq, Eq, Hash etc traits #2043

mgeisler opened this issue Mar 20, 2024 · 5 comments

Comments

@mgeisler
Copy link
Contributor

From reading the manual and searching a bit, I don't think it's currently possible for my uniffi::Object and uniffi::Record types to expose their PartialEq, Eq, Hash, etc implementations?

Right now, I used a cheap workaround in my PR:

#[uniffi::export]
impl SigningIdentity {
    /// Is `self` equal to `other`?
    fn equals(&self, other: &Self) -> bool {
        self == other
    }
}

Ideally, the PartialEq impl would make it through. In Python, the __eq__ method could be wired up to delegate to PartialEq::eq, etc. The consequence of not having this is that things like assertions become more cumbersome.

Have I missed something in the manual about this?

@mgeisler mgeisler changed the title Is it possible to expose PartialEq, Eq, Hash and other derived traits? Is it possible to expose PartialEq, Eq, Hash and other traits? Mar 20, 2024
@mhammond
Copy link
Member

It should be possible for objects - https://mozilla.github.io/uniffi-rs/udl/interfaces.html#exposing-methods-from-standard-rust-traits. It's not possible for records because records have no methods (and it's not immediately obvious we should fix that, given every method call would involve moving the entire record over the FFI, then constructing an object on the rust side, then making the call, which would obviously be very expensive for very large records)

@mhammond
Copy link
Member

See also https://github.com/mozilla/uniffi-rs/tree/main/fixtures/trait-methods

@mgeisler
Copy link
Contributor Author

It should be possible for objects - https://mozilla.github.io/uniffi-rs/udl/interfaces.html#exposing-methods-from-standard-rust-traits.

Oh, thank you! I need to read the UDL section of the manual much more carefully 😄 I've been hoping to avoid it and just use the proc-macros. I see they support this too, so that's neat.

It's not possible for records because records have no methods (and it's not immediately obvious we should fix that, given every method call would involve moving the entire record over the FFI, then constructing an object on the rust side, then making the call, which would obviously be very expensive for very large records)

Very good point...

For the case of a derived Eq, I guess one could deal with records by generating the same implementation in the target language — compare field by field. Eventually, this should bottom out in an object or in a primitive type. That should give all the nice properties one would expect: short-circuiting if two fields don't match, cheap since nothing is sent back to Rust.

One could do similar things for Debug, Display, and Hash. In general, this would only be correct if people don't customize these implementations on the Rust side. That might suggest that

#[derive(Debug, uniffi::Object)]
#[uniffi::export(Debug)]
struct TodoList {
   ...
}

should be

#[derive(uniffi::Object)]
#[uniffi::derive_and_export(Debug)]
struct TodoList {
   ...
}

which would mean that UniFFI is fully responsible for deriving and exposing a standard Debug implementation.

@mhammond
Copy link
Member

I can see how records might do this for eq. Display needs a Rust impl IIUC. For Debug, the subset of supported types is such that maybe we could think about generating this by default. Or maybe some way of flagging this as something described as, eg, a "simple" type which flags both eq and default get implemented by default? hash is unclear - would it be necessary in some future for everyone to agree on the hash algo?

@mgeisler
Copy link
Contributor Author

Display needs a Rust impl IIUC

Good point! So the logic could be that UniFFI will emit an implementation in the target language for traits which can be derived in Rust. The implementation will do the same as the Rust implementation, but in the target language.

Traits like Display won't be in scope for this since they cannot be derived and so we don't have a default implementation to mimic in the target language.

hash is unclear - would it be necessary in some future for everyone to agree on the hash algo?

I haven't checked what deriving Hash does in Rust, but I imagine it just accumulates hash values for every field. So it should be easy to implement this in the target language.

I would like to try implementing this at some point — if/when we find that we need this.

mgeisler added a commit to mgeisler/mls-rs that referenced this issue Apr 15, 2024
This case up during our API review: instead of exposing a `Vec<u8>`,
we can let the API use a `GroupId` newtype wrapper.

A downside of this is that UniFFI doesn’t support exposing traits like
`Hash` on its record types (see
mozilla/uniffi-rs#2043). A consequence of
this is that the Python `GroupId` type is not hashable. I therefore
ended up accessing the `.id` attribute directly in the integration
test.

Related to awslabs#81.
mgeisler added a commit to mgeisler/mls-rs that referenced this issue Apr 15, 2024
This case up during our API review: instead of exposing a `Vec<u8>`,
we can let the API use a `GroupId` newtype wrapper.

A downside of this is that UniFFI doesn’t support exposing traits like
`Hash` on its record types (see
mozilla/uniffi-rs#2043). A consequence of
this is that the Python `GroupId` type is not hashable. I therefore
ended up accessing the `.id` attribute directly in the integration
test.

Related to awslabs#81.
mgeisler added a commit to mgeisler/mls-rs that referenced this issue Apr 29, 2024
This case up during our API review: instead of exposing a `Vec<u8>`,
we can let the API use a `GroupId` newtype wrapper.

A downside of this is that UniFFI doesn’t support exposing traits like
`Hash` on its record types (see
mozilla/uniffi-rs#2043). A consequence of
this is that the Python `GroupId` type is not hashable. I therefore
ended up accessing the `.id` attribute directly in the integration
test.

Related to awslabs#81.
@mhammond mhammond changed the title Is it possible to expose PartialEq, Eq, Hash and other traits? Allow foreign implementations of PartialEq, Eq, Hash etc traits May 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants