Support returning `Result<OpaqueRust, OpaqueRust>` from async function #158

NiwakaDev · 2023-02-08T15:22:44Z

This PR addresses #157 and adds support for returning a Result<OpaqueRust, OpaqueRust> from async functions.

Here's an example of using this.

//Rust
#[swift_bridge::bridge]
mod ffi {
    extern "Rust" {
        type OpaqueType1;
        type OpaqueType2;
        async fn some_function() -> Result<OpaqueType1, OpaqueType2>;
    }
}

//Swift
do {
    let opaqueType1 = try await some_function()
} catch {
    //...
}

chinedufn

@NiwakaDev this is truly outstanding work. AWESOME.

I left some minor feedback, but overall this is excellent. Really impressive stuff. Thanks a lot.

chinedufn · 2023-02-08T15:36:41Z

book/src/bridge-module/async-functions/README.md

@@ -18,3 +18,19 @@ async fn some_async_function(list: Vec<u8>) -> ffi::MyStruct {
    ffi::MyStruct
 }
 ```
+
+## Return `Result<OpaqueType, OpaqueType>` from async functions


I don't think that we need this documentation since it doesn't really communicate anything other than "results can be used as return types", but we already have a chapter that describes all supported types.

But, it would be nice to highlight a Result in the example since async functions often return results.

Maybe just change the bridge module above to:

#[swift_bridge::bridge] mod ffi { extern "Rust" { type User; type ApiError; async fn user_count() -> u32; async fn load_user(url: &str) -> Result<User, ApiError>; } }

As well as a Swift block below it:

// Swift let totalUsers = await user_count() do { let user = try await load_user("https://example.com/users/5") } catch let error as ApiError { // ... error handling ... }

chinedufn · 2023-02-08T15:48:39Z

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

+        }
+    }
+
+    return try await withCheckedThrowingContinuation({ (continuation: CheckedContinuation<SomeType1, Error>) in


Should this be SomeType2 instead of Error?

I tried to do this, but I got the following error.

Testing failed: Cannot convert value of type '(CheckedContinuation<AsyncResultOpaqueRustType1, AsyncResultOpaqueRustType2>) -> Void' to expected argument type '(CheckedContinuation<AsyncResultOpaqueRustType1, any Error>) -> Void' Testing cancelled because the build failed.

I made the minimal code to reproduce this error.

struct ZeroError: Error{} func hoge(value: Int) async throws -> Int { try await withCheckedThrowingContinuation { (continuation: CheckedContinuation<Int, ZeroError>) in if value == 0{ continuation.resume(throwing: ZeroError()) }else{ continuation.resume(returning: value*2) } } }

error message:

Cannot convert value of type '(CheckedContinuation<Int, ZeroError>) -> Void' to expected argument type '(CheckedContinuation<Int, any Error>) -> Void'

Thanks for the minimal example.

This compiles:

struct ZeroError: Error{} func hoge(value: Int) async throws -> Int { try await withCheckedThrowingContinuation { (continuation: CheckedContinuation<Int, _>) in if value == 0{ continuation.resume(throwing: ZeroError()) }else{ continuation.resume(returning: value*2) } } }

Since ZeroError doesn't compile leaving it as Error is fine.

Would just leave a comment somewhere in the test.

Maybe:

TODO: Replace `Error` with the concrete error type `SomeType2`. As of Feb 2023 using the concrete error type leads to a compile time error. This seems like a bug in the Swift compiler.

Doesn't really matter since it doesn't impact the user experience... I just don't want to look at this in a year and forget why we're using Error instead of the concrete error type.

chinedufn · 2023-02-08T16:07:04Z

crates/swift-bridge-ir/src/bridged_type.rs

@@ -54,6 +54,13 @@ pub(crate) trait BridgeableType: Debug {
    /// Whether or not this is a `Result<T,E>`.
    fn is_result(&self) -> bool;

+    /// Extract the Swift representation of each variant from Rust's Result<T,E>.


This exact Swift results variant feels a bit too specific to belong in the BridgeableType trait.

Here's what I'm thinking instead:

We add an as_result method to the BridgeableType trait

trait BridgeableType { // ... fn is_result(&self) -> bool { self.as_result().is_some() } fn as_result(&self) -> Option<&BuiltInResult>; }

Then we can use .as_result() to get back the BuiltInResult, and use a method(s) on the built in result to generate whatever information we need when generating the async support code on the Swift side.

chinedufn · 2023-02-08T16:07:31Z

SwiftRustIntegrationTestRunner/SwiftRustIntegrationTestRunnerTests/AsyncTests.swift

+        do {
+            let result = try await rust_async_func_reflect_result_opaque_rust(.Ok(AsyncResultOpaqueRustType1(10)))
+        } catch {
+            XCTFail()


Nice idea with the XCTFail()

What about replacing fatalError() with XCTFail() in other tests?

Great idea, please go for it!

chinedufn · 2023-02-08T16:08:17Z

SwiftRustIntegrationTestRunner/SwiftRustIntegrationTestRunnerTests/AsyncTests.swift

+            XCTFail()
+        } catch let error as AsyncResultOpaqueRustType2 {
+            XCTAssertEqual(error.val(), 100)
+        }


Should we add another catch block that has XCTFail(), which proves that we're definitely hitting the catch let error as AsyncResultOpaqueRustType2 block?

chinedufn · 2023-02-08T16:09:25Z

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

+            #[swift_bridge::bridge]
+            mod ffi {
+                extern "Rust" {
+                    type SomeType1;


Nice idea having two different types.

What about naming them OkType and ErrorType?

Sounds good!

chinedufn · 2023-02-08T16:10:02Z

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

+class CbWrapper$some_function {
+    var cb: (Result<SomeType1, Error>) -> ()
+
+    public init(cb: @escaping (Result<SomeType1, Error>) -> ()) {


Same idea here. Should all of these Errors be SomeType2?

chinedufn · 2023-02-08T16:20:04Z

crates/swift-bridge-ir/src/codegen/generate_swift/generate_function_swift_calls_rust.rs

-                    types,
-                ),
-            )
+            if func_ret_ty.is_result() {


We shouldn't be hard coding types.

Since we have to support a lot of types in a lot of different places, it gets really hard to maintain and refactor our code generation if types are hard coded everywhere.

Instead, the convert_ffi_value_to_swift_value should be tuned to give us the proper value here.

I think we can accomplish this by changing this:

swift-bridge/crates/swift-bridge-ir/src/bridged_type/bridgeable_result.rs

Lines 108 to 119 in 2ab37cb

match type_pos {

TypePosition::FnReturn(_) => self.ok_ty.to_swift_type(type_pos, types),

TypePosition::FnArg(_, _)

| TypePosition::SharedStructField

| TypePosition::SwiftCallsRustAsyncOnCompleteReturnTy => {

format!(

"RustResult<{}, {}>",

self.ok_ty.to_swift_type(type_pos, types),

self.err_ty.to_swift_type(type_pos, types),

)

}

}

to have the TypePosition::SwiftCallsRustAsyncOnCompleteReturnTy match block return __private__ResultPtrAndPtr.

This is better because it means that anything that calls to_swift_type will get the proper value given this TypePosition::SwiftCallsRustAsyncOnCompleteReturnTy.

chinedufn · 2023-02-08T20:57:31Z

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

@@ -533,3 +533,107 @@ void __swift_bridge__$SomeType$some_method(void* callback_wrapper, void __swift_
        .test();
    }
 }
+
+mod extern_rust_async_function_returns_result_opaque {


Mind adding a Verify that ... comment like the other bridge modules?

chinedufn · 2023-02-08T23:07:22Z

Just wanted to say again that this is a really great PR. Thanks for implementing this!!

NiwakaDev · 2023-02-09T10:20:54Z

Addressed your review! Please let me know if there are any omissions.

chinedufn

One minor bit of cleanup than looks good to me!

chinedufn · 2023-02-09T11:11:37Z

SwiftRustIntegrationTestRunner/SwiftRustIntegrationTestRunnerTests/AsyncTests.swift

+        do {
+            let result = try await rust_async_func_reflect_result_opaque_rust(.Ok(AsyncResultOpaqueRustType1(10)))
+        } catch {
+            XCTFail()


Great idea, please go for it!

chinedufn · 2023-02-09T11:15:27Z

crates/swift-bridge-ir/src/codegen/generate_swift/generate_function_swift_calls_rust.rs

        let (maybe_on_complete_sig_ret_val, on_complete_ret_val) = if func_ret_ty.is_null() {
            ("".to_string(), "()".to_string())
        } else {
-            (
-                format!(


We can undo this change and go back to the original code.

The if and else blocks that were added are identical.

NiwakaDev added 5 commits February 8, 2023 21:21

Support returning Result<OpaqueRust, OpaqueRust> from async function

e5f158a

refactor

fb3ffa5

reformat

15521fb

update document

fd9dccf

update document

83285e9

chinedufn reviewed Feb 8, 2023

View reviewed changes

NiwakaDev added 2 commits February 9, 2023 07:07

replace with

50b1927

avoid hardcoding

00fa22b

NiwakaDev added 2 commits February 9, 2023 12:37

address review

c3606a8

re-format

425bcd4

chinedufn reviewed Feb 9, 2023

View reviewed changes

chinedufn mentioned this pull request Feb 9, 2023

Add support for Result<T, E> #111

Merged

NiwakaDev added 2 commits February 9, 2023 20:56

address review

b9a324e

re-format

cfa1f28

chinedufn merged commit 37e282c into chinedufn:master Feb 9, 2023

This was referenced Feb 9, 2023

Return Result<OpaqueRust, OpaqueRust> from an async function #157

Closed

Add support for async functions returning opaque Rust types #120

Closed

chinedufn mentioned this pull request Feb 25, 2023

Support returning Result<(), OpaqueRust> from Rust functions #180

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support returning `Result<OpaqueRust, OpaqueRust>` from async function #158

Support returning `Result<OpaqueRust, OpaqueRust>` from async function #158

NiwakaDev commented Feb 8, 2023

chinedufn left a comment

chinedufn Feb 8, 2023 •

edited

Loading

chinedufn Feb 8, 2023

chinedufn Feb 8, 2023

NiwakaDev Feb 8, 2023 •

edited

Loading

NiwakaDev Feb 8, 2023 •

edited

Loading

chinedufn Feb 8, 2023 •

edited

Loading

chinedufn Feb 8, 2023

chinedufn Feb 8, 2023

NiwakaDev Feb 9, 2023 •

edited

Loading

chinedufn Feb 9, 2023

chinedufn Feb 8, 2023

chinedufn Feb 8, 2023 •

edited

Loading

NiwakaDev Feb 8, 2023

chinedufn Feb 8, 2023

chinedufn Feb 8, 2023

chinedufn Feb 8, 2023

chinedufn commented Feb 8, 2023

NiwakaDev commented Feb 9, 2023

chinedufn left a comment

chinedufn Feb 9, 2023

chinedufn Feb 9, 2023

NiwakaDev Feb 9, 2023

	match type_pos {
	TypePosition::FnReturn(_) => self.ok_ty.to_swift_type(type_pos, types),
	TypePosition::FnArg(_, _)
	\| TypePosition::SharedStructField
	\| TypePosition::SwiftCallsRustAsyncOnCompleteReturnTy => {
	format!(
	"RustResult<{}, {}>",
	self.ok_ty.to_swift_type(type_pos, types),
	self.err_ty.to_swift_type(type_pos, types),
	)
	}
	}

Support returning Result<OpaqueRust, OpaqueRust> from async function #158

Support returning Result<OpaqueRust, OpaqueRust> from async function #158

Conversation

NiwakaDev commented Feb 8, 2023

chinedufn left a comment

Choose a reason for hiding this comment

chinedufn Feb 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NiwakaDev Feb 8, 2023 • edited Loading

Choose a reason for hiding this comment

NiwakaDev Feb 8, 2023 • edited Loading

Choose a reason for hiding this comment

chinedufn Feb 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NiwakaDev Feb 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chinedufn Feb 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chinedufn commented Feb 8, 2023

NiwakaDev commented Feb 9, 2023

chinedufn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Support returning `Result<OpaqueRust, OpaqueRust>` from async function #158

Support returning `Result<OpaqueRust, OpaqueRust>` from async function #158

chinedufn Feb 8, 2023 •

edited

Loading

NiwakaDev Feb 8, 2023 •

edited

Loading

NiwakaDev Feb 8, 2023 •

edited

Loading

chinedufn Feb 8, 2023 •

edited

Loading

NiwakaDev Feb 9, 2023 •

edited

Loading

chinedufn Feb 8, 2023 •

edited

Loading