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

Swift: Make it possible to mark value types as Sendable with sendable_value_types config #2045

Merged
merged 1 commit into from
Mar 26, 2024

Conversation

Sajjon
Copy link
Contributor

@Sajjon Sajjon commented Mar 23, 2024

Make it possible to mark Swift value types as Sendable - which is needed for Swift 6.

  • Add new config for Swift: sendable_value_types (false by default)
  • which if true (sendable_value_types: true) will mark:
    • ([uniffi::Record]) Swift structs which does not contain object references as Sendable
    • ([uniffi::Enum]) Swift enumss which does not contain object references as Sendable

This will make it possible for Swift devs to skip having to add @unchecked Sendable all over the place

I was unable to add a meaningful fixture though, since it is not possible to "unit test" this. The only way to "test" it is to ensure that compilation fails... which would be possible if:

Alternatives to consider

AND/OR we can add a different set of (Swift only) config flags: frozen_struct and frozen_enum (or simply frozen_value_types) - because types marked @frozen automatically becomes Sendable, but I think it valuable to not have to mark a value type as @frozen just to get Sendable. I can do another PR for the @frozen annotation if you'd like?

@Sajjon Sajjon requested a review from a team as a code owner March 23, 2024 10:51
@Sajjon Sajjon requested review from bendk and removed request for a team March 23, 2024 10:51
@Sajjon Sajjon force-pushed the swift_sendablility branch from a4623c9 to 630269f Compare March 23, 2024 10:53
@bendk
Copy link
Contributor

bendk commented Mar 23, 2024

Very interesting. I want to understand Sendable in Swift more before I can really comment on this one.

My first question is what's the difference between it and Rust's Send trait? On the Rust side, all types are required to be Send, so why can't we unconditionally mark everything Sendable?

Second question: what's the interaction between @frozen and Sendable? Why does marking value types @frozen automatically make them Sendable?

@Sajjon
Copy link
Contributor Author

Sajjon commented Mar 23, 2024

I believe a good approximation of explaining Swift Sendable in Rust terms is Send + Sync + Copy (but I'm not entirely sure)

Unconditionally marking all value types as Sendable is better than not doing it - I just through letting it be opt it would make it more likely to get this PR in...

I'm not convinced adding it unconditionally for reference types (Swift classes) is a good idea though, maybe that at least should be opt in. But I did not add impl of Sendable for reference types since implementation is trickier - and since personally I dont need it.

Regarding @frozen this text in the original Swift Evolution proposal (SE-0302) of Sendable explains it quite good:

Public non-frozen structs and enums do not get an implicit conformance, because doing so would present a problem for API resilience: the implicit conformance to Sendable would become part of the contract with clients of the API, even if it was not intended to be

@bendk
Copy link
Contributor

bendk commented Mar 25, 2024

I believe a good approximation of explaining Swift Sendable in Rust terms is Send + Sync + Copy (but I'm not entirely sure)

It seems to me that any type used with UniFFI should basically fit this description (although maybe Clone is more precise than Copy). The Rust code assumes it and passes them freely between threads.

Unconditionally marking all value types as Sendable is better than not doing it - I just through letting it be opt it would make it more likely to get this PR in...

However, this makes a lot of sense to me. I certainly feel more comfortable approving this if it's behind a config value.

What do you think about making it opt-in for 0.27.x with the intention of trying to make it the default sometime down the road? If that sounds good to you, please add a CHANGELOG entry saying that and I can approve.

I'm not convinced adding it unconditionally for reference types (Swift classes) is a good idea though, maybe that at least should be opt in. But I did not add impl of Sendable for reference types since implementation is trickier - and since personally I dont need it.

I think there's 2 cases here:

  • Interfaces implemented in Rust. In that case it's fine to pass them across threads. UniFFI has always assumed that foreign languages may do that and has required Send + Sync bounds on any types that are exposed like this.
  • Callback interfaces implemented in Swift. I think that these should be Sendable since the Rust traits they're implementing are Send + Sync and the Rust code could choose to move them to a different thread. I think there should be a Sendable bound on these.

I'm speaking from a strictly conceptual perspective here though, I don't know how hard the implementation would be.

@bendk bendk added the v0.27 Issues/PRs for the 0.27 release label Mar 25, 2024
@mhammond
Copy link
Member

However, this makes a lot of sense to me. I certainly feel more comfortable approving this if it's behind a config value.

We could even go a little further and name the config value with an experimental_ or similar prefix, which would give us license to change either the name, type or semantics of that config pref over the next few releases?

@Sajjon
Copy link
Contributor Author

Sajjon commented Mar 25, 2024

Cool! I will rename flag with experimental_ prefix and update changelog in a couple of hours

…of new config 'sendable_value_types' in uniffi.toml.
@Sajjon Sajjon force-pushed the swift_sendablility branch from 630269f to 13c3fb3 Compare March 25, 2024 17:58
@Sajjon
Copy link
Contributor Author

Sajjon commented Mar 25, 2024

@bendk @mhammond let me know if you want me to change the wording in the markdown files to something else. Otherwise, ready from my point of view.

Copy link
Contributor

@bendk bendk left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for updating the PR.

@bendk bendk merged commit 013f475 into mozilla:main Mar 26, 2024
5 checks passed
@LBeernaertProton
Copy link

Do you folks think this could also be extended to uniffi objects?

@mhammond
Copy link
Member

Do you folks think this could also be extended to uniffi objects?

Sure - but see the conversation above, it's not clear to some of us if it makes sense to be unconditional etc - we'd welcome input into everything about this - ie, whether it should eventually be the default, whether it should be opt-in/opt-out per "broad type" (ie, enum, object), per "single type" (ie, Foo, Bar), etc.

@LBeernaertProton
Copy link

Noted. As far as I'm concerned, I'm okay with this being the default for all uniffi objects as well, since the underlying Rust objects are already Sync+Send. But perhaps it would be better to leave this as opt in at the moment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v0.27 Issues/PRs for the 0.27 release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants