mozilla / uniffi-rs

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

Add `Sendable` to Swift Templates #2318

Open martinmose opened 1 week ago

martinmose commented 1 week ago

Hey,

This PR addresses Issue #2274 by marking all generated Uniffi classes in Swift as @unchecked Sendable. This change ensures compatibility with Swift 6 and allows Uniffi-generated classes to be used in concurrent contexts without triggering Sendable-related compiler errors.

The addition of @unchecked Sendable is backward-compatible with iOS 8.0 and above, so it should be good to go?

I’ve tested and verified that my Swift packages and project now work correctly with Swift 6. Here is the generated output I’m getting:

public protocol TicketHandlerProtocol: Sendable, AnyObject {
open class TicketHandler:
    @unchecked Sendable, TicketHandlerProtocol
{

Let me know if there’s anything I missed or if there’s anything else you’d like adjusted. Thanks!

mhammond commented 6 days ago

lgtm, but not to the tests :)

Sajjon commented 4 days ago

I think this should not be merged as is.

We should not unconditionally mark all Swift classes as sendable. I think this makes it possible to breaks Swift Sendability guarantees. So should at the very least be opt-in. I'm happy to be convinced that my claim above is false, but I don't think it is.

This PR ought to be upgraded to use configs, like I did in https://github.com/mozilla/uniffi-rs/pull/2045 so lets call it experimental_sendable_reference_types.

Future improvement (over experimental_sendable_reference_types)

And probably we should add per type config controlled from Rust, something like:

#[derive(uniffi::Object)]
#[uniffi::export_swift(Sendable)] // bikeshedding spelling, `export_swift` does not exist today.
pub struct Foo {
 pub a_bool: bool // OK since `Bool` in Swift is sendable
}

Which with granularity makes Foo be class Foo: @uchecked Sendable {} in Swift.

#[derive(uniffi::Object)] // NOT marked `uniffi::export_swift(Sendable)` so not Sendable in Swift
pub struct Bar {
... 
}

#[derive(uniffi::Object)]
#[uniffi::export_swift(Sendable)] // Bindgen Error! field `bar: Bar` is not Sendable in Swift
pub struct HasBar {
 pub bar: Bar // not Sendable in Swift
}

But we do not have this spelling today - that is - we do not have uniffi::export which is lanugage specific, right?

Sajjon commented 4 days ago

@heckj s input would be very valuable here

Sajjon commented 4 days ago

At the very least this PR ought to verify that the experimental sendable values flag I added is set to true if the UniFFI exported objects exported contains UniFFI exported value types in its fields..

bendk commented 4 days ago

We should not unconditionally mark all Swift classes as sendable. I think this makes it possible to breaks Swift Sendability guarantees. So should at the very least be opt-in. I'm happy to be convinced that my claim above is false, but I don't think it is.

Is your concern about the Rust side of things or the Swift? On the Rust side, we force all interfaces to have a Send + Sync bound, since we know the foreign side might share them between threads. Is this equivalent to Swift's Sendable?

heckj commented 4 days ago

I don't know enough internals of both languages to be able to call out what might be missing in the translation interface, but reporting to swift that a class is unchecked @sendable is telling the compiler that you DEFINITELY have all possible race conditions covered, which I don't think is true, even with Send + Sync. I'm saying this mostly based on my experience with the Automerge core library, written in Rust, that we had to explicitly wrap in threading protections to ensure that it wasn't called from multiple threads.

I'd be fine with this as an opt-in, and it called out that it means the developer consuming the rust library is responsible for enforcing single threaded access, but please - do NOT make this a default without extensive verification and tests that this is a safe consideration.

heckj commented 4 days ago

Since I don't know sufficient details to advise in any reasonably way, I'm asking on the Swift forums - as there's developers there who I believe are familiar with these specifics and can likely better advise on what's appropriate for enabling the data-race safety expectations (from the Swift side at least, maybe also being familiar with the Rust side as well) over an FFI boundary.

Forum thread: https://forums.swift.org/t/question-on-sendability-swift-6-data-race-safety-and-ffi-interfaces/76219

Sajjon commented 3 days ago

Is your concern about the Rust side of things or the Swift?

My concerns are on the Swift side.

Sajjon commented 3 days ago

Furthermore I think we default to generating open class in Swift when we use uniffi::Object.

It is very hard for us in Uniffi to prevent users from footgunning themselves, but defaulting to open class Foo: @unchecked Sendable in Swift feels like a pitfall.

Im actually unsure what happens with subclasses of Foo - Bar which adds mutable non sendable fields.

martinmose commented 3 days ago

We should not unconditionally mark all Swift classes as sendable. I think this makes it possible to breaks Swift Sendability guarantees. So should at the very least be opt-in. I'm happy to be convinced that my claim above is false, but I don't think it is.

Is your concern about the Rust side of things or the Swift? On the Rust side, we force all interfaces to have a Send + Sync bound, since we know the foreign side might share them between threads. Is this equivalent to Swift's Sendable?

Well "yes" but Swift’s Sendable deals with tasks in structured concurrency so it ensures that a type can be safely passed or shared between tasks, such as between an actor and other tasks.

martinmose commented 3 days ago

lgtm, but not to the tests :)

Yikes... The tests ran fine with cargo test, so I thought I was good to go. But I can see them fail when running with the Docker image 🤷‍♂️. Anyways, I go fix. Thanks for the feedback!

martinmose commented 3 days ago

We should not unconditionally mark all Swift classes as sendable. I think this makes it possible to breaks Swift Sendability guarantees. So should at the very least be opt-in. I'm happy to be convinced that my claim above is false, but I don't think it is.

Thank you for your valuable feedback.

I’m trying to understand your concern, so please bear with me if I’m missing something.

If all mutable state in Rust must already be thread-safe (via Send + Sync), doesn’t it logically follow that UniFFI-generated types would inherently comply with Swift’s Sendable guarantees? For example, wouldn’t a UniFFI object that internally uses a thread-safe structure like Arc<Mutex<T>> ensure that any access to mutable state is already thread-safe?

I feel that making this opt-in could add unnecessary "complexity". While I acknowledge the existence of the experimental_sendable_value_types flag, requiring developers to figure out when and how to opt-in might make getting started with a Swift 6 project more challenging than necessary.

That said, if someone more knowledgeable than me agrees that it should indeed be a flag, I’ll gladly add it.

Sidenote: Instead of relying on @unchecked Sendable for all cases, would it be crazy to consider a different approach—such as generating actor types when using uniffi::Object? This might align more closely with Swift’s concurrency model and eliminate the need for unchecked annotations altogether. Of course, this could introduce some other “side effects.” 🤔

mhammond commented 3 days ago

I see nothing in https://developer.apple.com/documentation/swift/sendable which suggests we can't use @Sendable unconditionally in our implementation - it sounds exactly like Send+Sync?

bendk commented 3 days ago

Somewhat related: maybe we should make the classes final. I can't think of a great use-case for subclassing them. We generate a protocol for each UniFFI interface, so if you wanted 2 implementations you probably want to implement the protocol rather than subclassing the class.

mhammond commented 3 days ago

I thought we used open classes for test-ability - git blame should show us more. If that's true then I'd be a little torn about whether to break that, or just document that these classes should not be extended?

Sajjon commented 3 days ago

Instead of relying on @unchecked Sendable for all cases, would it be crazy to consider a different approach—such as generating actor types when using uniffi::Object? This might align more closely with Swift’s concurrency model and eliminate the need for unchecked annotations altogether. Of course, this could introduce some other “side effects.” 🤔

Yes very much so! We can either make reference types translate into @Mainactor bound actors which makes using methods on uniffi::Object work directly from UI.

But it is ofc a bigger future change.

martinmose commented 3 days ago

Somewhat related: maybe we should make the classes final. I can't think of a great use-case for subclassing them. We generate a protocol for each UniFFI interface, so if you wanted 2 implementations you probably want to implement the protocol rather than subclassing the class.

Kinda off-topic—but also related. It’s very much in the spirit of Swift to emphasize protocols and composition over inheritance (a protocol-oriented approach). So, I think this is a really good suggestion!

martinmose commented 2 days ago

Instead of relying on @unchecked Sendable for all cases, would it be crazy to consider a different approach—such as generating actor types when using uniffi::Object? This might align more closely with Swift’s concurrency model and eliminate the need for unchecked annotations altogether. Of course, this could introduce some other “side effects.” 🤔

Yes very much so! We can either make reference types translate into @mainactor bound actors which makes using methods on uniffi::Object work directly from UI.

But it is ofc a bigger future change.

Indeed, I just thought it was worth mentioning.

mhammond commented 2 days ago

ISTM that actor types would just implement another layer of locking, which should be unnecessary as the Rust object already guarantees what actors guarantee. Use of @mainactor seems like it might make sense for a small subset of objects, but not many (ie, I don't think it would make sense for any used by Mozilla). I guess I'd be fine with allowing uniffi.toml to specify actor annotations for named objects, but I doubt the ergonomics of that would actually make sense.

Stepping back though, I'd still like to understand the problem with that unchecked assertion - as I said, I believe the Rust implemented objects are such that this annotation is completely safe and appropriate (notwithstanding concerns around sub-classing, so maybe preventing that is something we should do). I'm not sure if the arguments against that are saying that the assertion is not safe to add, or whether it introduces other ergonomic (rather than correctness) issues?

I also see nothing in that forum thread which gives me pause. As the last comment in that forum thread mentions, "if the Rust struct is not Sync, then the Swift class shouldn’t be Sendable.", which is my understanding of the situation. The Rust structs are Sync.

mhammond commented 2 days ago

I thought we used open classes for test-ability - git blame should show us more. If that's true then I'd be a little torn about whether to break that, or just document that these classes should not be extended?

That does appear to be true - it was added in #1975

heckj commented 2 days ago

My concern is that it wasn't safe to add - that asserting that everything exported as a reference type was sendable (safe to be used across various threads and executors) was valid. It looks like Records are from what I'm reading in https://forums.swift.org/t/question-on-sendability-swift-6-data-race-safety-and-ffi-interfaces/76219/7, but is not for classes with mutable stored properties.

It may be that asserting the unchecked sendable is the correct path - but it's a huge assertion on the swift side that I wanted to validate, hence the Swift forums thread inquiring.

mhammond commented 2 days ago

but is not for classes with mutable stored properties.

Right, that's my understanding too, but my assertion is that the Rust implementation of these classes meets all the requirements

martinmose commented 2 days ago

@mhammond Thanks so much for your insight and feedback! I agree with you and understand your point. How should we proceed? (Of course, the tests should pass)

mhammond commented 2 days ago

I personally think it should be on by default, and I'm really not even sure about the opt-out - can anyone identify when it would be harmful or a problem with that attribute having been applied? What's a use-case for opting out from this?

ie, I think your patch is fine and maybe could add an opt-out later if it becomes an actual thing?

mhammond commented 2 days ago

I think this makes it possible to breaks Swift Sendability guarantees.

@Sajjon I don't see how that would be possible, can you please elaborate?

Sajjon commented 2 days ago

I think this makes it possible to breaks Swift Sendability guarantees

Right so I think the only concern is subclassing actually.

// MARK: - Generated by UniFFI bindgen
open class Animal: @unchecked   Sendable {
    public let name: String
    public init(name: String) {
        self.name = name
    }
}

// MARK: - Pure Swift (not UniFFI)
public class DogOwner {} // NOT Sendable
public class Dog: Animal {
    public let dogOwner: DogOwner
    public init(dogOwner: DogOwner, name: String) {
        self.dogOwner = dogOwner
        super.init(name: name)
    }
}

I thought this was going to be an issue, alas, Swift has got us covered (at least Xcode 16.1), because we get this warning:

Screenshot 2024-11-27 at 10 07 55

So even footgunning is hard, thanks to Swift saying that we must restate @unchecked Sendable for subclasses of a superclass which is marked with it 👍

So I think this PR is indeed safe to merge.

Sorry that I was "alarmist" but I think it resulted in a good discussion above :).

[!NOTE] If we do this for classes, we should probably "remove" the experimental_sendable_value_types and mark all uniffi::Record as Sendable by default.

martinmose commented 1 day ago

I thought this was going to be an issue, alas, Swift has got us covered (at least Xcode 16.1), because we get this warning:

But when we’re talking about Swift 6 and all the strict concurrency, you are indeed forced to use a newer version of Xcode. So I think we’re good! And if you’re not using Swift 6, you’re essentially “on your own,” so it doesn’t really matter if @unchecked Sendable is added or not.

Note

If we do this for classes, we should probably "remove" the experimental_sendable_value_types and mark all uniffi::Record as Sendable by default.

Agree - that’s exactly what I was trying to suggest with the second option here 😅:

* Should I check if `experimental_sendable_value_types` is true and only apply the `@unchecked Sendable` changes I have made, as @Sajjon suggested?

And finally, I have to say… That font you’re using—wow 🙈