swift-bridge icon indicating copy to clipboard operation
swift-bridge copied to clipboard

Replacing our custom `enum RustResult` with Swift's `Result` enum when possible

Open chinedufn opened this issue 2 years ago • 4 comments

The Problem

Right now we define a Swift enum RustResult.

https://github.com/chinedufn/swift-bridge/blob/420ed791c0cef08f7cfff7f4024ee39cd13d31c2/crates/swift-bridge-build/src/generate_core/result_support.rs#L2-L5

We use RustResult instead of Swift's standard Result type since Swift's Result<T, E> requires the E to conform to Swift's Error trait.

In cases where the E conforms to Error we should use a Swift Result instead of a RustResult.

For example, given the following Swift code:

final class MyOpaqueError: Error {
    // ...
}

func downloadImage(callback: (Result<Vec<u8>, MyOpaqueError>) -> ()) {
    // ...
    callback(Result.success(bytes))
}

We want to be able to import the Swift downloadImage function into Rust with a bridge module along the lines of:

#[swift_bridge::bridge]
mod ffi {
    extern "Swift" {
        #[swift_bridge(Error)]
        type MyOpaqueError;

        #[swift_name = "downloadImage"]
        fn download_image(callback: Box<dyn FnOnce(Result<(), MyOpaqueError>)>;
    }
}

Today we have to use a RustResult on the Swift side, instead of the above Result.

// Note the `RustResult` here. We want it to be possible to use `Result` instead.
func downloadImage(callback: (RustResult<Vec<u8>, MyOpaqueError>) -> ()) {
    // ...
}

Potential Solutions

Potential Solution 1 - Annotation

We can let the user specify that a type conforms to the Error protocol.

Then, when that type is used as the E in Result<T, E> our code generation can use a Swift Result instead of the RustResult enum.

  • [ ] We need to implement support for the #[swift_bridge(Error)] attribute shown above. We'd track whether or not this this Error attribute was present on the opaque type https://github.com/chinedufn/swift-bridge/blob/07a15d78782dc371a7d24301ac9625124e42f8d8/crates/swift-bridge-ir/src/parse/parse_extern_mod/opaque_type_attributes.rs#L17-L35

  • [ ] When Error is present on an imported opaque Swift type it would emit Swift code along the lines of:

    extension MyOpaqueError: __SwiftBridgeCheckIfError {}
    
    • protocol __SwiftBridgeCheckIfError: Error {} would get defined inside of an error_support module in swift-bridge-build https://github.com/chinedufn/swift-bridge/blob/e8d3859d2a68b5d5e0012185a87e88b3c811f853/crates/swift-bridge-build/src/generate_core.rs#L13-L14
  • [ ] During Result related code generation if there is an Error attribute then we can use Swift's Result instead of RustResult.

    • We can add a codegen test for this similar to https://github.com/chinedufn/swift-bridge/blob/2ab37cbb949be4a466848129e7f17ecbb3fecf5e/crates/swift-bridge-ir/src/codegen/codegen_tests/result_codegen_tests.rs#L67-L82
    • And an integration test similar to https://github.com/chinedufn/swift-bridge/blob/2ab37cbb949be4a466848129e7f17ecbb3fecf5e/crates/swift-integration-tests/src/result.rs#L10
    • and similar to https://github.com/chinedufn/swift-bridge/blob/2ab37cbb949be4a466848129e7f17ecbb3fecf5e/SwiftRustIntegrationTestRunner/SwiftRustIntegrationTestRunnerTests/ResultTests.swift#L11-L15

Potential Solution 2 - ...?

...?

Additional Considerations

It's inconsistent to have the generated Swift code use Result sometimes and RustResult other times.

One way to solve this would be to only support bridging a result when the E type conforms to Swift's Error protocol. This would let us get rid of RustResult entirely.

That might be too restrictive though.

For example, you wouldn't be able to bridge results that had errors that didn't implement Send + Sync, since Swift's Error protocol requires the type to conform to Sendable.


A good starting point might be to add documentation to the enum RustResult that explains why it exists. This way if someone sees it in their generated code they're more likely to figure out what it is and why it's there.

As well as adding documentation to the Result chapter in the swift-bridge book.

chinedufn avatar Jan 29 '23 13:01 chinedufn

I want to try to address this issue this week.

NiwakaDev avatar Jan 29 '23 17:01 NiwakaDev

Sounds great!

chinedufn avatar Jan 29 '23 19:01 chinedufn

@NiwakaDev were you able to look into this?

gemcoder21 avatar Feb 12 '23 03:02 gemcoder21

were you able to look into this?

@DegenTim , I haven't done it yet. If you are interested, I'll leave this to you.

NiwakaDev avatar Feb 12 '23 03:02 NiwakaDev