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.

Sign up for GitHub

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 all async return types in tests #312

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

Add Sendable to all async return types in tests #312

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 19 additions & 11 deletions SwiftRustIntegrationTestRunner/SwiftRustIntegrationTestRunner/Result.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,23 +6,29 @@
//

func swift_func_takes_callback_with_result_arg(
arg: (RustResult<CallbackTestOpaqueRustType, String>) -> ()
arg: (RustResult<CallbackTestOpaqueRustType, String>) -> Void
) {
arg(.Ok(CallbackTestOpaqueRustType(555)))
arg(.Ok(CallbackTestOpaqueRustType(555)))
}

public class ResultTestOpaqueSwiftType {
var num: UInt32
init(val: UInt32) {
self.num = val
}
func val() -> UInt32 {
self.num
}
var num: UInt32

init(val: UInt32) {
self.num = val
}

func val() -> UInt32 {
self.num
}
}

// TODO: we can delete these type assertions once we correctly generate Sendable
// types. See the following issue:
// https://github.com/chinedufn/swift-bridge/issues/150

extension AsyncRustFnReturnStruct: @unchecked Sendable {}
Copy link
Owner

Choose a reason for hiding this comment

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

Currently safe since this struct only contains a primitive

swift-bridge/crates/swift-integration-tests/src/async_function.rs

Lines 8 to 11 in 636fa27

#[swift_bridge(swift_repr = "struct")]
struct AsyncRustFnReturnStruct {
field: u8,
}

But we'll still eventually want to replace all of these @unchecked Sendable with proper swift-bridge support for emitting Sendable impls as described in #269


extension ResultTestOpaqueRustType: @unchecked Sendable {}
extension ResultTestOpaqueRustType: Error {}

Expand All @@ -41,5 +47,7 @@ extension ResultTransparentStruct: Error {}
extension SameEnum: @unchecked Sendable {}
extension SameEnum: Error {}

extension AsyncResultOkEnum: @unchecked Sendable {}
Copy link
Owner

Choose a reason for hiding this comment

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

Hmm we'll want to move away from these @unchecked Sendable {}s in favor of enabling swift-bridge to emit Sendable extension.

In the meantime, you can make this @unchecked Sendable safe by replacing the inner String with a primitive such as u32.

swift-bridge/crates/swift-integration-tests/src/async_function.rs

Lines 50 to 54 in 636fa27

enum AsyncResultOkEnum {
NoFields,
UnnamedFields(i32, String),
NamedFields { value: u8 },
}

Let's also leave a TODO in this file to ditch @unchecked Sendable in favor of proper Sendable support. Can link to #269

Copy link
Author

Choose a reason for hiding this comment

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

For future reference, I think you linked the wrong issue by accident - the relevant one is #150.


extension AsyncResultErrEnum: @unchecked Sendable {}
extension AsyncResultErrEnum: Error {}
9 changes: 7 additions & 2 deletions crates/swift-integration-tests/src/async_function.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,19 @@ mod ffi {
extern "Rust" {
async fn rust_async_return_null();
async fn rust_async_reflect_u8(arg: u8) -> u8;
async fn rust_async_reflect_string(string: String) -> String;

async fn rust_async_return_struct() -> AsyncRustFnReturnStruct;
async fn rust_async_func_reflect_result_opaque_rust(
arg: Result<AsyncResultOpaqueRustType1, AsyncResultOpaqueRustType2>,
) -> Result<AsyncResultOpaqueRustType1, AsyncResultOpaqueRustType2>;
async fn rust_async_func_return_result_null_opaque_rust(
succeed: bool,
) -> Result<(), AsyncResultOpaqueRustType2>;

// TODO: this is broken because RustString is not Sendable.
// Work around making String and other opaque types Sendable is tracked
// here: https://github.com/chinedufn/swift-bridge/issues/150
// async fn rust_async_reflect_string(string: String) -> String;
Comment on lines +25 to +28
Copy link
Owner

@chinedufn chinedufn Feb 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

RustString should be safe to implement Sendable for today, so long as the Swift code does not make multiple copies of the RustString class.

It will be fully safe once we start using ~Copyable to guarantee that the Swift code cannot copy the RustString (copy as in, copy the class RustString, which is a pointer to the underlying Rust std::string::String.).

So, this code can be added back.

Safety claims are here #309 (comment)

Copy link
Author

Choose a reason for hiding this comment

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

Uncommenting this line makes it fail to compile on Swift 6, and I think changing core code in this PR would be out of scope, no?

}

extern "Rust" {
Expand Down Expand Up @@ -49,7 +54,7 @@ mod ffi {

enum AsyncResultOkEnum {
NoFields,
UnnamedFields(i32, String),
UnnamedFields(i32, u32),
NamedFields { value: u8 },
}

Expand Down