-
Notifications
You must be signed in to change notification settings - Fork 64
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 all async return types in tests #312
base: master
Are you sure you want to change the base?
Conversation
9e341f9
to
48f774b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor feedback around documentation and changing a String
to a u32
, then we can land this
@@ -41,5 +43,7 @@ extension ResultTransparentStruct: Error {} | |||
extension SameEnum: @unchecked Sendable {} | |||
extension SameEnum: Error {} | |||
|
|||
extension AsyncResultOkEnum: @unchecked Sendable {} |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
func val() -> UInt32 { | ||
self.num | ||
} | ||
} | ||
|
||
extension AsyncRustFnReturnStruct: @unchecked Sendable {} |
There was a problem hiding this comment.
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
// TODO: this is broken because RustString is not Sendable. | ||
// async fn rust_async_reflect_string(string: String) -> String; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The TODO
describes the problem but not what we should do about it.
Do we need to do research? Do we already know the solution? Is the problem unsolvable? etc.
Let's make it more clear here what the next step is on this TODO.
48f774b
to
859fd09
Compare
I think I addressed your comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good just one request.
Can you look over #309 (comment) and confirm that that reasoning is sound?
If so, please add the String
code back that you commented out in this PR.
// 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; |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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?
Hm, I don't think |
We should make it Sendable and, for now, put the onus on the user to maintain the safety rules outlined in the book. If users aren't following the book's safety rules then they're going to hit UB regardless of whether we implement If in the future we're able to guarantee safety at compile time, even better. For now, Swift is fairly limited in this regard. Swift does not yet make it possible for a fully safe FFI boundary. We need to accept that and work with it, rather than wishing that the Swift side had Rust's level of memory safety. In time we hope that it will get easier and easier to make the Swift side safer and safer. |
Regardless of how you want to proceed on the |
Without this fix I get errors like the following under Swift 6:
I avoided adding
Sendable
forRustString
since that's being discussed in #296 and elsewhere. Instead, I commented it out and left a TODO.Fixes #311.