Support Rust returning `-> Result<_, String>` #296

brittlewis12 · 2024-10-14T19:45:17Z

Support Rust returning -> Result<_, String>, e.g.:

#[swift_bridge::bridge]
mod ffi {
    extern "Rust" {
        fn do_fallible_work() -> Result<(), String>;
    }
}

Incorporate the changes from #282 to implement swift's Error protocol for ruststring, & extend them with the tests requested in #295 (comment)

thanks for your work on this lovely crate!

chinedufn

Looks good. Minor feedback, then we can land this. Thank you for working on this.

crates/swift-bridge-ir/src/codegen/codegen_tests/result_codegen_tests.rs

SwiftRustIntegrationTestRunner/SwiftRustIntegrationTestRunnerTests/ResultTests.swift

crates/swift-bridge-build/src/generate_core/rust_string.swift

chinedufn · 2024-10-14T19:50:06Z

Please update your PR body with an example of a simple bridge module that is now possible.

This example will be used in the release notes.

Can just be a bridge module with a simple function that returns -> Result<String, String>

* document relationships between impl & tests

chinedufn · 2024-10-14T20:18:25Z

Looks great. Thank you for your first contribution. I'll merge once tests pass.

colinmarc · 2025-01-26T23:24:41Z

At least on my machine, this causes an error on Swift 6:

/home/colinmarc/dev/swift-bridge/integration-tests/Sources/SharedLib/Generated/SharedLib.swift:868:14: error: non-final class 'RustString' cannot conform to 'Sendable'; use '@unchecked Sendable'
 866 | protocol SwiftBridgeGenericCopyTypeFfiRepr {}
 867 |
 868 | public class RustString: RustStringRefMut {
     |              `- error: non-final class 'RustString' cannot conform to 'Sendable'; use '@unchecked Sendable'
 869 |     var isOwned: Bool = true
 870 |

/home/colinmarc/dev/swift-bridge/integration-tests/Sources/SharedLib/Generated/SharedLib.swift:869:9: error: stored property 'isOwned' of 'Sendable'-conforming class 'RustString' is mutable
 867 |
 868 | public class RustString: RustStringRefMut {
 869 |     var isOwned: Bool = true
     |         `- error: stored property 'isOwned' of 'Sendable'-conforming class 'RustString' is mutable
 870 |
 871 |     public override init(ptr: UnsafeMutableRawPointer) {

/home/colinmarc/dev/swift-bridge/integration-tests/Sources/SharedLib/Generated/SharedLib.swift:892:14: error: non-final class 'RustStringRefMut' cannot conform to 'Sendable'; use '@unchecked Sendable'
 890 |     }
 891 | }
 892 | public class RustStringRefMut: RustStringRef {
     |              `- error: non-final class 'RustStringRefMut' cannot conform to 'Sendable'; use '@unchecked Sendable'
 893 |     public override init(ptr: UnsafeMutableRawPointer) {
 894 |         super.init(ptr: ptr)

/home/colinmarc/dev/swift-bridge/integration-tests/Sources/SharedLib/Generated/SharedLib.swift:897:14: error: non-final class 'RustStringRef' cannot conform to 'Sendable'; use '@unchecked Sendable'
 895 |     }
 896 | }
 897 | public class RustStringRef {
     |              `- error: non-final class 'RustStringRef' cannot conform to 'Sendable'; use '@unchecked Sendable'
 898 |     var ptr: UnsafeMutableRawPointer
 899 |

/home/colinmarc/dev/swift-bridge/integration-tests/Sources/SharedLib/Generated/SharedLib.swift:898:9: error: stored property 'ptr' of 'Sendable'-conforming class 'RustStringRef' is mutable
 896 | }
 897 | public class RustStringRef {
 898 |     var ptr: UnsafeMutableRawPointer
     |         `- error: stored property 'ptr' of 'Sendable'-conforming class 'RustStringRef' is mutable
 899 |
 900 |     public init(ptr: UnsafeMutableRawPointer) {

/home/colinmarc/dev/swift-bridge/integration-tests/Sources/SharedLib/Generated/SharedLib.swift:868:14: error: non-final class 'RustString' cannot conform to 'Sendable'; use '@unchecked Sendable'
 866 | protocol SwiftBridgeGenericCopyTypeFfiRepr {}
 867 |
 868 | public class RustString: RustStringRefMut {
     |              `- error: non-final class 'RustString' cannot conform to 'Sendable'; use '@unchecked Sendable'
 869 |     var isOwned: Bool = true
 870 |

/home/colinmarc/dev/swift-bridge/integration-tests/Sources/SharedLib/Generated/SharedLib.swift:869:9: error: stored property 'isOwned' of 'Sendable'-conforming class 'RustString' is mutable
 867 |
 868 | public class RustString: RustStringRefMut {
 869 |     var isOwned: Bool = true
     |         `- error: stored property 'isOwned' of 'Sendable'-conforming class 'RustString' is mutable
 870 |
 871 |     public override init(ptr: UnsafeMutableRawPointer) {

/home/colinmarc/dev/swift-bridge/integration-tests/Sources/SharedLib/Generated/SharedLib.swift:892:14: error: non-final class 'RustStringRefMut' cannot conform to 'Sendable'; use '@unchecked Sendable'
 890 |     }
 891 | }
 892 | public class RustStringRefMut: RustStringRef {
     |              `- error: non-final class 'RustStringRefMut' cannot conform to 'Sendable'; use '@unchecked Sendable'
 893 |     public override init(ptr: UnsafeMutableRawPointer) {
 894 |         super.init(ptr: ptr)

/home/colinmarc/dev/swift-bridge/integration-tests/Sources/SharedLib/Generated/SharedLib.swift:897:14: error: non-final class 'RustStringRef' cannot conform to 'Sendable'; use '@unchecked Sendable'
 895 |     }
 896 | }
 897 | public class RustStringRef {
     |              `- error: non-final class 'RustStringRef' cannot conform to 'Sendable'; use '@unchecked Sendable'
 898 |     var ptr: UnsafeMutableRawPointer
 899 |

/home/colinmarc/dev/swift-bridge/integration-tests/Sources/SharedLib/Generated/SharedLib.swift:898:9: error: stored property 'ptr' of 'Sendable'-conforming class 'RustStringRef' is mutable
 896 | }
 897 | public class RustStringRef {
 898 |     var ptr: UnsafeMutableRawPointer
     |         `- error: stored property 'ptr' of 'Sendable'-conforming class 'RustStringRef' is mutable
 899 |
 900 |     public init(ptr: UnsafeMutableRawPointer) {

NiwakaDev · 2025-01-26T23:39:02Z

@colinmarc

At least on my machine, this causes an error on Swift 6:

We need to make the class thread-safe.

See: #150 (comment)

colinmarc · 2025-01-27T08:17:18Z

We need to make the class thread-safe.

I don't think there's a way to (safely) make a mutable heap reference Sendable (aside from a mutex or something explicitly thread-safe). Swift pushes you towards immutability for thread-safety.

Two ways I could see:

Enforce immutability (wrapping the pointer in a smart pointer like Arc that doesn't implement DerefMut)
Copy over the language boundary (and create a normal swift string)

Copying is definitely wrong for most strings, but for errors it might strike the right balance, no? Sorry if I'm retreading a topic that's already been covered, or if I misunderstood something.

Separately, I'm confused that this PR adds throws to the generated method rather than returning a Result from it. The latter seems much more semantically correct - and it's easy to convert a result method into one that throws from the swift side if that's desired.

chinedufn · 2025-01-27T11:35:03Z

As of Jan 2025, Swift's error handling documentation mentions throws 46 times and Result 0 times - https://docs.swift.org/swift-book/documentation/the-swift-programming-language/errorhandling/

The idea is to generate more idiomatic Swift code.
If an argument is made for Result being better (i.e. worth breaking away from idioms), then we can consider it.

If an argument is made for making this configurable via an attribute (i.e. #[swift_bridge(use_this_attribute_to_return_a_result)], then we can consider it.

Ah, yeah, RustStringRef should not implement Sendable, meaning it shouldn't implement Error.
Support for RustStringRef implementing the Error protocol should be removed.

We'll want to either start running CI against Swift 5 and Swift 6, or just drop Swift 5 support entirely.
This way we can catch more potential concurrency issues.
related - #288

I'm confused as to how implementing Error was even possible since RustStringRef doesn't implement Sendable...?
Ah, looks like the Sendable implementation was removed from UnsafeMutableRawPtr in Swift 5.6 https://github.com/swiftlang/swift-evolution/blob/main/proposals/0331-remove-sendable-from-unsafepointer.md

chinedufn · 2025-01-27T11:41:31Z

Opened an issue: #309

This reverts commit c45a38c.

chinedufn · 2025-02-02T12:16:32Z

Ah, yeah, RustStringRef should not implement Sendable

Err, no I was wrong here. RustStringRef should be Sendable.

Explained here -> #309 (comment)

jnbooth added 2 commits October 14, 2024 14:47

Implement Error and LocalizedError for RustStringRef

9695106

Remove LocalizedError implementation

1912e42

brittlewis12 force-pushed the string-error branch from 0b61f34 to 05b0be9 Compare October 14, 2024 19:47

brittlewis12 mentioned this pull request Oct 14, 2024

Swift warning when returning -> Result<*, *> from a Rust method #295

Open

chinedufn reviewed Oct 14, 2024

View reviewed changes

chinedufn mentioned this pull request Oct 14, 2024

Implement Error for RustStringRef #282

Closed

chinedufn changed the title ~~enable interfaces which return Result<_, String>~~ Support Rust returning -> Result<_, String> Oct 14, 2024

Test returned Result<String, String>

5027a48

* document relationships between impl & tests

brittlewis12 force-pushed the string-error branch from 05b0be9 to 5027a48 Compare October 14, 2024 20:10

brittlewis12 requested a review from chinedufn October 14, 2024 20:11

chinedufn merged commit c45a38c into chinedufn:master Oct 14, 2024
5 checks passed

brittlewis12 deleted the string-error branch October 14, 2024 20:21

chinedufn mentioned this pull request Nov 2, 2024

Missing Error impl for throw MyError fn #298

Closed

chinedufn mentioned this pull request Jan 27, 2025

Implement Swift Error and Sendable for RustString #309

Open

colinmarc added a commit to colinmarc/swift-bridge that referenced this pull request Jan 27, 2025

Revert "Support Rust returning -> Result<_, String> (chinedufn#296)"

db73038

This reverts commit c45a38c.

colinmarc mentioned this pull request Jan 27, 2025

Add Sendable to all async return types in tests #312

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Rust returning `-> Result<_, String>` #296

Support Rust returning `-> Result<_, String>` #296

brittlewis12 commented Oct 14, 2024 •

edited by chinedufn

Loading

chinedufn left a comment

chinedufn commented Oct 14, 2024

chinedufn commented Oct 14, 2024

colinmarc commented Jan 26, 2025

NiwakaDev commented Jan 26, 2025

colinmarc commented Jan 27, 2025

chinedufn commented Jan 27, 2025

chinedufn commented Jan 27, 2025

chinedufn commented Feb 2, 2025

Support Rust returning -> Result<_, String> #296

Support Rust returning -> Result<_, String> #296

Conversation

brittlewis12 commented Oct 14, 2024 • edited by chinedufn Loading

chinedufn left a comment

Choose a reason for hiding this comment

chinedufn commented Oct 14, 2024

chinedufn commented Oct 14, 2024

colinmarc commented Jan 26, 2025

NiwakaDev commented Jan 26, 2025

colinmarc commented Jan 27, 2025

chinedufn commented Jan 27, 2025

chinedufn commented Jan 27, 2025

chinedufn commented Feb 2, 2025

Support Rust returning `-> Result<_, String>` #296

Support Rust returning `-> Result<_, String>` #296

brittlewis12 commented Oct 14, 2024 •

edited by chinedufn

Loading