mozilla / uniffi-rs

a multi-language bindings generator for rust
https://mozilla.github.io/uniffi-rs/
Mozilla Public License 2.0
2.83k stars 231 forks source link

Constructors for records #1935

Open Sajjon opened 10 months ago

Sajjon commented 10 months ago

The #[uniffi::constructor] macro can be attached to a function on a uniffi::Recrod struct's function returning Self today, however, it does not generate and constructor in the generated Swift code. It would be nice to support constructors for records.

Follows up thread on Matrix, @bendk asked me to create a GH issue.

jplatte commented 9 months ago

#[uniffi::constructor] only has an effect inside of #[uniffi::export]-ed impl blocks.

So I would re-frame this feature request as supporting exported impl blocks for non-object types (records, but also enums). I think constructors are only one of the ways in which UniFFI-generated enum and record types could feel more native in some target languages, support for methods would be a big improvement as well.

mhammond commented 9 months ago

The #[uniffi::constructor] macro can be attached to a function on a uniffi::Recrod

As implied by @jplatte, the actual bug here is that the attribute is allowed there.

Regarding the feature request though: IIUC, records do get constructors, with args exactly matching the fields. So the idea of supporting secondary constructors and methods makes sense, but I'm less clear about the primary constructor. Can you describe your use-case some more? Do you really want to avoid consumers being able to specify each field when creating the record?

Sajjon commented 9 months ago

Yes it is manifold: 1) there is a bug, where I'm allowed to attach #[uniffi::constructor] to a Record without compilation error, but it is not exposed. 2) Yes I would like expose other than memberwise init in Swift (and Kotlin), i.e. constructors returning Result (throwing init in Swift).

(And yes ultimately I want methods on Records as well, but I thought that was harder to achieve than constructors)

mhammond commented 9 months ago

2. I would like expose other than memberwise init in Swift

Does that mean "instead of the default constructor" or "as well as the default ctor"?

Sajjon commented 9 months ago

@mhammond alongside of default constructor - which I also would like to be able to make private instead of public as mentioned in another Matrix thread

mhammond commented 9 months ago

The entire "constructor" concept here is a foreign one - so a "private primary constructor" doesn't really make sense - from the bindings POV, this would just mean "no constructor". So it sounds like you do want your constructor to be the primary and only constructor for the bindings?

Sajjon commented 9 months ago

Yes to be able to skip generating the memberwise init, I went into implementation details... that since we already code for generating the memberwise init and we could simply toggle the visibility of it to be private instead of public - or omit it completely.

In Swift it is common practice to "delegate" other inits to the memberwise one, useful for eg logging, so I know since all inits ultimately pass through the memberwise one, I can log there to always see logs when initializing an instance of said type.

mhammond commented 9 months ago

So records could have constructors, like objects do, but just no "primary" constructor (that would be reserved for the local existing init). That sounds good to me!

I'm not sure about how methods would work though - we can't pass references, so a copy of the entire record for each call sounds like a footgun?

Sajjon commented 9 months ago

@mhammond I was gonna ask, what is the status of support for methods on Records? "Status" meaning two things I guess:

  1. How hard/complex would it be to add support for it?
  2. How willing/likely are you to add it?

I'm new to UniFFI and quite new to Rust, but I've been developing Swift since it was in beta, so senior Swift dev. When Apple released Swift they kind of pushed for users to embrace value type over references types, which I (alongside the whole Swift community did), so classes are rarely ever used in Swift - we almost always use structs. For some "*Managers", classes are used with the singleton pattern. But even dependencies like HTTP clients are quite often represented with structs, it is popular to use swift-dependencies package by PointFree.

So to me, I have a hard time seeing and argument against supporting methods on Record.

If UniFFI repo is not going to support it, my team will need to circumvent this by a lot of manual boilerplate code:

#[unffi::Record]
struct User {
    first_name: String,
    last_name: String,
}
impl User {
    // I would LOVE to be able to `#[uniffi::export]`, alas not supported...
    pub fn name(&self) -> String { /* this is actually a REALLY complex method... */ }
}

#[uniffi::export]
pub fn user_name(&user: User) -> String { user.name() /* Uhm, cumbersome boilerplate RUST side */  }

And then both in the land of Swift and Kotlin.

extension User {
    public func name() -> String {
         /* Cumbersome boilerplate Swift (and Kotlin) side */
        userName(self)
    }
}

And then multiply this by 500x methods.... That is a lot of boilerplate.... Not sure what I hope your answer is... I guess I would love to hear:

  1. "Easy"
  2. "We will add method support for Records soon"

but if 1. "HARD" - I'm curious as to why (might be above my knowledge level....) and if 2. "Wont support" - I'm really curious to hear why.

I don't see any need for mutable methods for now if that changes the answers to my two questions, it would be interesting to hear.

mhammond commented 9 months ago

So to me, I have a hard time seeing and argument against supporting methods on Record

I wrote:

I'm not sure about how methods would work though - we can't pass references, so a copy of the entire record for each call sounds like a footgun?

That's not my argument "against" it, but a reason it should be carefully considered.

Sajjon commented 9 months ago

@mhammond When considered, I think it makes sense to split the "add support for #[uniffi::export] for methods on Records"-feature request into two:

  1. support for non-mutating methods (fn get_foo_with_logic(&self) -> Foo)
  2. support for mutating methods (fn set_foo_with_logic(&mut self, foo: Foo))

Where hopefully 1. would be somewhat straightforward but where 2. is tricker ?

mhammond commented 9 months ago

support for non-mutating methods (fn get_foo_with_logic(&self) -> Foo)

This is a good example of the footgun I mentioned and why I'm not a huge fan of this. The Rust function says it takes a &self, which hides the fact that an inefficient copy of the structure is made, moved across the FFI, then that local copy passed by reference. IMO, the &self implies a far more optimal call is being made.

Sajjon commented 9 months ago

@mhammond As a UniFFI author and consumer, frankly, I don't care about it being inefficient 😄 that "cost" is far far a smaller cost then as an author having to manually build virtual support for this for all my 500-5000 methods as described in https://github.com/mozilla/uniffi-rs/issues/1935#issuecomment-1914222155

THAT cost is huge 😅 and the benefits of being able to UniFFI export methods in Records - however inefficient at runtime - greatly exceeds any risk of them being inefficient to call IMO.

bendk commented 9 months ago

This is a good example of the footgun I mentioned and why I'm not a huge fan of this. The Rust function says it takes a &self, which hides the fact that an inefficient copy of the structure is made, moved across the FFI, then that local copy passed by reference. IMO, the &self implies a far more optimal call is being made.

I don't know if there's any solution to this though. We could choose to only support self, but even then I don't feel like it communicates the fact that a copy is made before the call. For example, I think the hello_name example is making like 4 extra copies and possibly 2 unicode conversions behind the scenes, but that's not apparent from the signature. ISTM that this is not something that can be solved with argument parameters.

Personally, I'd be okay with adding support and informing users of what's happening behind the scenes in the manual. Maybe a future extension could be to also support Arc<DataStruct> and/or Arc<Mutex<DataStruct>> as ways of avoiding the copy (but accepting a different set of trade offs).

mhammond commented 9 months ago

Without advocating for either position, an alternative would be to not support it for dictionaries but add good getter/setter support to interfaces. For many use-cases this would look much like a dictionary, but as you mention, with a different set of tradeoffs.

Eg, Kotlin already considers a dictionary a "data class", so IIUC this would mean they are only sometimes able to be that. And as we are discussing elsewhere, our JS bindings for Gecko are moving more towards dictionaries being "pure data" objects without a way to sanely attach methods.

It would be fantastic to hear perspectives from the maintainers of other bindings about this.

Sajjon commented 9 months ago

I absolutely understand Kotlin and Python (and bindings for other languages) might bring different perspectives on how Records ought to be used.

But I would like to highlight the importance of #[derive(uniffi::Record)] over uniffi::Object for Swift and Apple platform app development specifically:

Using classes in Swift is quite uncommon.. We tend use Swift struct everywhere. I can almost count the number of times I've really needed a class on my two hands, in the past 9 years.

If a few unnecessary copies in the Rust side of things is footgun, "forcing" UniFFI consumers to use classes in Swift is a bazooka to the head 😅 . By forcing I mean, if UniFFI would go the route of instead further developing Record (Swift structs) you would instead further develop Object with getter/setter.

What you guys have done with UniFFI is truly remarkable 😍 . I'm convinced it is the future for mobile development, with shared app core in Rust and with "stupid" iOS and Android apps responsible for UI layer impl only. But to get there on the Apple side of things, I feel we must have proper support for structs, with methods. To iterate my earlier point, these methods need not be mutating - since mutation is logic and that should happen in Rust land. And until UniFFI supports it, I will have to manually impl using the earlier written boilerplate in Swift land (and in Kotlin land too, a colleague of mine would have to do it...)

The setup I currently have is (feedback welcome 😄, hopefully I'm not doing anything terribly wrong, my Swift bindgen tests make this approach look promising):

/// A VERY complex structure, JSON of it is typically 20-100 KB.
#[derive(serde::Serialize, serde::Deserialize, uniffi::Record)]
struct Profile {
  ...
}

/// A holder of a Profile, which can mutate the Profile!
#[derive(uniffi::Object)]
struct Wallet {
    profile: RwLock<Profile>
}
impl Wallet {
    pub(crate) fn update_profile_with<F, R>(&self, mutate: F) -> R where F: Fn(RwLockWriteGuard<'_, Profile>) -> R {
        self.profile
            .try_write()
            .map(mutate)
            .expect("Should not write Profile while reading it from other threads.")
    }

    pub(crate) fn access_profile_with<T: Clone, F>(&self, access: F) -> T where F: Fn(RwLockReadGuard<'_, Profile>) -> T {
        self.profile
            .try_read()
            .map(access)
            .expect("Should not write Profile while reading it from other threads.")
    }

    /// Clone the profile and return it.
        #[uniffi::export]
    pub fn profile(&self) -> Profile {
        self.access_profile_with(|p| p.clone())
    }
}

The nice thing with this is that iOS app can have a Wallet singleton object (class) and call mutating functions on it, and read out the latest value using wallet.profile(). I could even wrap Wallet in a Swift actor holding the wallet instance, further protecting against any silly multi-thread usage, thus making my RwLock assumptions in Rust land valid.

So I don't need any mutating func on my Swift structs. But a lot of those Swift structs have non-trivial formatting logic, performed on its fields, which I ofc wanna have Rust side and export with UniFFI.

And apart from methods on Records, like the title of this issue, it would also be nice to be able to export constructors other than (or replace) memberwise init.

mhammond commented 9 months ago

It's worth nothing that Mozilla ships an iOS browser by leveraging uniffi so swift is very important to us, but some of these perspectives aren't necessarily shared by all iOS developers - classes are used extensively there. Further, these days we try and be mindful of golang, c# and ruby too, and I expect that in the future more languages will appear.

Sajjon commented 9 months ago

Right, but SwiftUI does not work with classes out of the box, SwiftUI is unable to update and react to changes in a class - but "just works" with structs. That is a huge argument. The new Observation framework changes that a bit, but I have a hard time seeing how Observation would play with UniFFI.

How does the UI layer work on Mozillas iOS app? Do you have a viewmodel class marked with ("old") @ObservableObject and then you have to mark all your stored properties with @Published? Or maybe your View Layer does not heavily rely on the models on your UniFFI lib...? In my scenario - we do.

Another big argument is that the post popular architecture on iOS is TCA - which is completely built on the idea that State is a struct consisting of only structs. TCA does not work with classes.

Sajjon commented 9 months ago

Some documentation: https://developer.apple.com/documentation/swift/choosing-between-structures-and-classes

First item on the list: Use structures by default.

mhammond commented 9 months ago

My point is just that an existing high-profile and (to the core uniffi team) very important consumer of the swift bindings does exist which uses different patterns. A decision about whether we should add functions to records needs to take into account both swift consumers that use different patterns and also languages other than swift.

To be extra clear, I'm not saying that how Firefox for iOS uses Swift is a reason not to do it, but I'm also not convinced that anything above is a compelling reason to do it without taking these other things into account. In the meantime, functions are a work-around that offer the same capabilities albeit with worse ergonomics.

mhammond commented 9 months ago

So I would re-frame this feature request as supporting exported impl blocks for non-object types (records, but also enums).

Yeah, there are 2 parts to this:

Collecting "sane" metadata and exposing it to the binding templates makes perfect sense and would be the first step here anyway.

mhammond commented 9 months ago

similar discussions for enums in #1470