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

Add Sendable to Swift Templates #2318

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

martinmose
Copy link
Contributor

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!

@martinmose martinmose requested a review from a team as a code owner November 17, 2024 23:11
@martinmose martinmose requested review from gruberb and removed request for a team November 17, 2024 23:11
@mhammond
Copy link
Member

lgtm, but not to the tests :)

@Sajjon
Copy link
Contributor

Sajjon commented Nov 25, 2024

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 #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?

Copy link
Contributor

@Sajjon Sajjon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be opt in.

@Sajjon
Copy link
Contributor

Sajjon commented Nov 25, 2024

@heckj s input would be very valuable here

@Sajjon
Copy link
Contributor

Sajjon commented Nov 25, 2024

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
Copy link
Contributor

bendk commented Nov 25, 2024

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
Copy link
Contributor

heckj commented Nov 25, 2024

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
Copy link
Contributor

heckj commented Nov 25, 2024

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
Copy link
Contributor

Sajjon commented Nov 25, 2024

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

My concerns are on the Swift side.

@Sajjon
Copy link
Contributor

Sajjon commented Nov 25, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

martinmose commented Nov 26, 2024

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
Copy link
Member

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
Copy link
Contributor

bendk commented Nov 26, 2024

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
Copy link
Member

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
Copy link
Contributor

Sajjon commented Nov 26, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor

heckj commented Nov 26, 2024

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
Copy link
Member

mhammond commented Nov 26, 2024

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
Copy link
Contributor Author

@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)

  • Should we remove the experimental_sendable_value_types flag and always add Sendable
    or
  • Should I check if experimental_sendable_value_types is true and only apply the @unchecked Sendable changes I have made, as @Sajjon suggested?

@mhammond
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor

Sajjon commented Nov 27, 2024

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
Copy link
Contributor Author

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 🙈

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

Successfully merging this pull request may close these issues.

5 participants