Support `RustString` in extern Swift functions #225

NiwakaDev · 2023-05-06T14:53:42Z

Related to #201. This commit adds support RustString in extern Swift functions.

Here's an example of using this.

//Swift
func reflect_rust_string(arg: RustString) -> RustString {
    arg
}

//Rust
#[swift_bridge::bridge]
mod ffi {
    extern "Swift" {
        fn reflect_rust_string(arg: String) -> String;
    }
}

let foo = "foo";
let string = ffi::reflect_rust_string(foo.to_string());
assert_eq!(string.len(), 3);
assert_eq!(&string, foo);

NiwakaDev · 2023-05-06T14:54:50Z

crates/swift-integration-tests/src/string.rs

@@ -8,13 +8,19 @@ mod ffi {

    extern "Swift" {
        fn create_swift_string() -> String;
+        fn take_and_return_swift_string(arg: String) -> String;
    }
 }

 fn run_string_tests() {
    let string = ffi::create_swift_string();


Maybe, if this PR get merged, we could remove this from crates/swift-integration-tests/src/string.rs.

Makes sense

chinedufn

Left one question.

Tests are failing, but it seems like it started here 7c14ea0

The PR was passing when I merged it so maybe it conflicted with something in the master branch..

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

chinedufn · 2023-05-06T16:54:09Z

crates/swift-integration-tests/src/string.rs

@@ -8,13 +8,19 @@ mod ffi {

    extern "Swift" {
        fn create_swift_string() -> String;
+        fn take_and_return_swift_string(arg: String) -> String;
    }
 }

 fn run_string_tests() {
    let string = ffi::create_swift_string();


Makes sense

chinedufn · 2023-05-06T16:56:15Z

Mind adding an example bridge module to your PR body so that it's more clear that the bridge module uses String and not RustString

NiwakaDev · 2023-05-07T06:23:43Z

Left one question.
Tests are failing, but it seems like it started here 7c14ea0
The PR was passing when I merged it so maybe it conflicted with something in the master branch..

By the way, Tests also failed before #218 got merged. In the local environment, Tests pass.

chinedufn · 2023-05-07T11:26:56Z

Hmmm gotcha. I can't dive in for a couple days. Any idea where the issue might be stemming from?

NiwakaDev · 2023-05-07T23:11:31Z

Any idea where the issue might be stemming from?

I don't understand what causes the failure. I'll investigate it this week.

It might be a good idea to start by addressing the compiler warnings, like so:

/Users/runner/work/swift-bridge/swift-bridge/SwiftRustIntegrationTestRunner/Generated/swift-integration-tests/swift-integration-tests.h:741:232: warning: declaration of 'struct __swift_bridge__$ResultAsyncResultOkEnumAndAsyncResultOpaqueRustType1' will not be visible outside of this function

NiwakaDev · 2023-05-17T11:58:07Z

I found that what cause the failure. I'll make a PR for fixing this problem tomorrow.

chinedufn · 2023-05-17T11:59:55Z

YAYAYAYAYAYAYAYAYAYAYAY!!!!!!!!!! this made me so excited. Great thanks.

NiwakaDev · 2023-05-18T03:45:29Z

@chinedufn

I found that #213 causes the failure:

	field has incomplete type 'struct __swift_bridge__$tuple$I32ResultTestOpaqueRustTypeString'
Test session results, code coverage, and logs:
	failed to emit precompiled header '/Users/runner/Library/Developer/Xcode/DerivedData/SwiftRustIntegrationTestRunner-eunjzlpbulmkoedlbxmntxkdbksm/Build/Intermediates.noindex/PrecompiledHeaders/BridgingHeader-swift_3OA1PTMMNHZKC-clang_1AMT1CLNR4Q9S.pch' for bridging header '/Users/runner/work/swift-bridge/swift-bridge/SwiftRustIntegrationTestRunner/Headers/BridgingHeader.h'
	/Users/runner/Library/Developer/Xcode/DerivedData/SwiftRustIntegrationTestRunner-eunjzlpbulmkoedlbxmntxkdbksm/Logs/Test/Run-SwiftRustIntegrationTestRunner-2023.05.06_14-54-14-+0000.xcresult
	Testing cancelled because the build failed.

Detail

Let's say that we have a tuple type and a result type, like so:

mod ffi {
    extern "Rust" {
        type ResultTestOpaqueRustType;
    }

    extern "Rust" {
        fn rust_func_return_result_tuple_transparent_enum(
            succeed: bool,
        ) -> Result<(i32, ResultTestOpaqueRustType, String), i32>;
    }
}

Will potentiality generate:

struct __swift_bridge__$ResultTupleI32ResultTestOpaqueRustTypeStringAndI32 __swift_bridge__$rust_func_return_result_tuple_transparent_enum(bool succeed);
typedef enum __swift_bridge__$ResultTupleI32ResultTestOpaqueRustTypeStringAndI32$Tag {__swift_bridge__$ResultTupleI32ResultTestOpaqueRustTypeStringAndI32$ResultOk, __swift_bridge__$ResultTupleI32ResultTestOpaqueRustTypeStringAndI32$ResultErr} __swift_bridge__$ResultTupleI32ResultTestOpaqueRustTypeStringAndI32$Tag;
union __swift_bridge__$ResultTupleI32ResultTestOpaqueRustTypeStringAndI32$Fields {struct __swift_bridge__$tuple$I32ResultTestOpaqueRustTypeString ok; int32_t err;};
typedef struct __swift_bridge__$ResultTupleI32ResultTestOpaqueRustTypeStringAndI32{__swift_bridge__$ResultTupleI32ResultTestOpaqueRustTypeStringAndI32$Tag tag; union __swift_bridge__$ResultTupleI32ResultTestOpaqueRustTypeStringAndI32$Fields payload;} __swift_bridge__$ResultTupleI32ResultTestOpaqueRustTypeStringAndI32;
typedef struct __swift_bridge__$tuple$I32ResultTestOpaqueRustTypeString { int32_t _0; void* _1; void* _2; } __swift_bridge__$tuple$I32ResultTestOpaqueRustTypeString;

Or

struct __swift_bridge__$ResultTupleI32ResultTestOpaqueRustTypeStringAndI32 __swift_bridge__$rust_func_return_result_tuple_transparent_enum(bool succeed);
typedef struct __swift_bridge__$tuple$I32ResultTestOpaqueRustTypeString { int32_t _0; void* _1; void* _2; } __swift_bridge__$tuple$I32ResultTestOpaqueRustTypeString;
typedef enum __swift_bridge__$ResultTupleI32ResultTestOpaqueRustTypeStringAndI32$Tag {__swift_bridge__$ResultTupleI32ResultTestOpaqueRustTypeStringAndI32$ResultOk, __swift_bridge__$ResultTupleI32ResultTestOpaqueRustTypeStringAndI32$ResultErr} __swift_bridge__$ResultTupleI32ResultTestOpaqueRustTypeStringAndI32$Tag;
union __swift_bridge__$ResultTupleI32ResultTestOpaqueRustTypeStringAndI32$Fields {struct __swift_bridge__$tuple$I32ResultTestOpaqueRustTypeString ok; int32_t err;};
typedef struct __swift_bridge__$ResultTupleI32ResultTestOpaqueRustTypeStringAndI32{__swift_bridge__$ResultTupleI32ResultTestOpaqueRustTypeStringAndI32$Tag tag; union __swift_bridge__$ResultTupleI32ResultTestOpaqueRustTypeStringAndI32$Fields payload;} __swift_bridge__$ResultTupleI32ResultTestOpaqueRustTypeStringAndI32;

The former causes the failure because __swift_bridge__$ResultTupleI32ResultTestOpaqueRustTypeStringAndI32$Fields couldn't know __swift_bridge__$tuple$I32ResultTestOpaqueRustTypeString, on the other hand the later doesn't cause the failure.

The reason for the non-deterministic ordering is due to HashSet<String>:

swift-bridge/crates/swift-bridge-ir/src/codegen/generate_c_header.rs

Lines 367 to 370 in 7c14ea0

    
           for custom_type_declaration in custom_type_declarations { 
        
               header += &custom_type_declaration; 
        
               header += "\n"; 
        
           }

And Vec<TokenStream> I introduced in #213:

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

Line 384 in 7c14ea0

    
           pub fn generate_custom_c_ffi_types(&self, types: &TypeDeclarations) -> Option<Vec<String>> {

Potential Solution

1

__swift_bridge__$ResultTupleI32ResultTestOpaqueRustTypeStringAndI32$Fields needs to know __swift_bridge__$tuple$I32ResultTestOpaqueRustTypeString, but __swift_bridge__$tuple$I32ResultTestOpaqueRustTypeString could be generated without any dependencies. This means that we need to think the order of generating the two declarations. Maybe by introducing a directed graph or something like that to represent the type dependencies, we could fix this problem.

2

Since Solution 1 might be complicated, so maybe we could stop support #213 to fix the failure.

What do you think about this?

chinedufn · 2023-05-18T10:33:48Z

Thanks for the detailed breakdown and links. This made it very easy for me to understand this complex issue.

I don't think we need a complex solution.

I think we can solve this with the approach described here #170 (comment)

In essence, we use a parse_queue: Vec<&TypeDeclaration> and a encountered: HashSet<String>

We iterate over the types and push them to the parse queue. If the type contains other types we push those other types to the parse queue first, recursively.

Each time we push a type we insert its C header code into the encountered HashSet, this way we don't push the same type twice.

So, this guarantees that a type always appears after all of the types that we depend on.

So, because the solution should be fairly self-contained and not require much code, I like potential solution 1 the best where we just fix the problem.

Does that make sense?

NiwakaDev · 2023-05-18T14:26:00Z

Thank you for your quick reply.

Each time we push a type we insert its C header code into the encountered HashSet, this way we don't push the same type twice.
So, this guarantees that a type always appears after all of the types that we depend on.

I guess that the contents order of HashSet isn't deterministic. Why does this guarantee it?.

chinedufn · 2023-05-18T14:41:17Z

Because we wouldn't be using the HashSet. We'd be using a let parse_queue: Vec<&TypeDeclaration> = vec![] and whenever pushing a type to that vec we would first push any types that it depends on.

The HashSet is just used to check if we've already pushed the type so that we don't push it twice.

So, essentially, we're building a Vec<&TypeDeclaration> that we know is in the right order since whenever we push Result<T, E> we first push T and E .. and whenever we push T or E we first push anything that T and E depend on. So if T is a struct we would first push its fields' types.

Some pseudocode is in #170 (comment)

Does that make sense?

NiwakaDev · 2023-05-18T22:19:09Z

Does that make sense?

Yes. Thanks!!

Related to #225. Before this commit, the integration tests sometimes fail, like so: ``` field has incomplete type 'struct swift_bridge$tuple$I32ResultTestOpaqueRustTypeString' ``` This PR fixes the issue.

NiwakaDev · 2023-06-22T23:23:00Z

SwiftRustIntegrationTestRunner/SwiftRustIntegrationTestRunner/String.swift

+
+func take_and_return_swift_string(arg: RustString) -> String {
+    arg.toString()
+}


@chinedufn

create_swift_string returns String. To maintain consistency between this function and take_and_return_swift_string, I changed take_and_return_swift_string return value from RustString to String. But it is inefficient. what do you think about this?

Here're create_swift_string and take_and_return_swift_string implementation.

// // String.swift // SwiftRustIntegrationTestRunner // // Created by Frankie Nwafili on 2/18/22. // import Foundation func create_swift_string() -> String { "hello" } func take_and_return_swift_string(arg: RustString) -> String { arg.toString() }

Ah it looks like the code supports anything that implements IntoRustString so I think both -> String and -> RustString would work

https://github.com/chinedufn/swift-bridge/pull/123/files#diff-f68df9e9d1a62987e787182cb99274c5b0005fec9d504a521d7c0d8bd0cc2d65R247

I might just make it func take_and_return_swift_string(arg: RustString) -> RustString { so that the reader can learn that they can return both String and RustString

chinedufn

Left minor feedback then looks good to me.

Can you

Address feedback
Update your PR body to show an example bridge module

After that you can merge this PR.

Thanks!

chinedufn · 2023-06-23T11:04:16Z

crates/swift-integration-tests/src/string.rs

@@ -8,13 +8,19 @@ mod ffi {

    extern "Swift" {
        fn create_swift_string() -> String;
+        fn take_and_return_swift_string(arg: String) -> String;


Let's call this take_and_return_rust_string and make the Swift function (arg: RustString) -> RustString

This way we have a test for both -> String and -> RustString

NiwakaDev · 2023-06-23T14:12:18Z

@chinedufn

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

chinedufn · 2023-06-23T16:08:22Z

Please let me know if there are any errors or omissions.

Minor, but the function should be called reflect_rust_string (not reflect_swift_string).

After that LGTM

NiwakaDev · 2023-06-23T21:49:21Z

@chinedufn

Done.

chinedufn · 2023-06-23T21:59:38Z

Thanks!

NiwakaDev commented May 6, 2023

View reviewed changes

chinedufn reviewed May 6, 2023

View reviewed changes

NiwakaDev mentioned this pull request May 25, 2023

Fix already_declared attribute for enums and structs #226

Merged

This was referenced Jun 14, 2023

Swift Vec<primitive> Return Type and Function Argument #229

Merged

Fix non-deterministic generation order #230

Merged

NiwakaDev force-pushed the support_extern_swift_func_takes_and_returns_string branch from ca92199 to 7bd75f3 Compare June 22, 2023 23:16

NiwakaDev commented Jun 22, 2023

View reviewed changes

chinedufn approved these changes Jun 23, 2023

View reviewed changes

Support RustString in extern Swift functions

cf163b5

NiwakaDev force-pushed the support_extern_swift_func_takes_and_returns_string branch from 7bd75f3 to cf163b5 Compare June 23, 2023 14:02

NiwakaDev requested a review from chinedufn June 23, 2023 14:11

rename

12430db

chinedufn merged commit 2724644 into chinedufn:master Jun 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support `RustString` in extern Swift functions #225

Support `RustString` in extern Swift functions #225

NiwakaDev commented May 6, 2023 •

edited

Loading

NiwakaDev May 6, 2023

chinedufn May 6, 2023

chinedufn left a comment

chinedufn May 6, 2023

chinedufn commented May 6, 2023

NiwakaDev commented May 7, 2023 •

edited

Loading

chinedufn commented May 7, 2023

NiwakaDev commented May 7, 2023

NiwakaDev commented May 17, 2023

chinedufn commented May 17, 2023

NiwakaDev commented May 18, 2023 •

edited

Loading

chinedufn commented May 18, 2023

NiwakaDev commented May 18, 2023 •

edited

Loading

chinedufn commented May 18, 2023

NiwakaDev commented May 18, 2023

NiwakaDev Jun 22, 2023 •

edited

Loading

NiwakaDev Jun 22, 2023 •

edited

Loading

chinedufn Jun 23, 2023 •

edited

Loading

chinedufn left a comment

chinedufn Jun 23, 2023

NiwakaDev commented Jun 23, 2023

chinedufn commented Jun 23, 2023

NiwakaDev commented Jun 23, 2023

chinedufn commented Jun 23, 2023

Support RustString in extern Swift functions #225

Support RustString in extern Swift functions #225

Conversation

NiwakaDev commented May 6, 2023 • edited Loading

NiwakaDev May 6, 2023

Choose a reason for hiding this comment

chinedufn May 6, 2023

Choose a reason for hiding this comment

chinedufn left a comment

Choose a reason for hiding this comment

chinedufn May 6, 2023

Choose a reason for hiding this comment

chinedufn commented May 6, 2023

NiwakaDev commented May 7, 2023 • edited Loading

chinedufn commented May 7, 2023

NiwakaDev commented May 7, 2023

NiwakaDev commented May 17, 2023

chinedufn commented May 17, 2023

NiwakaDev commented May 18, 2023 • edited Loading

Detail

Potential Solution

1

2

chinedufn commented May 18, 2023

NiwakaDev commented May 18, 2023 • edited Loading

chinedufn commented May 18, 2023

NiwakaDev commented May 18, 2023

NiwakaDev Jun 22, 2023 • edited Loading

Choose a reason for hiding this comment

NiwakaDev Jun 22, 2023 • edited Loading

Choose a reason for hiding this comment

chinedufn Jun 23, 2023 • edited Loading

Choose a reason for hiding this comment

chinedufn left a comment

Choose a reason for hiding this comment

chinedufn Jun 23, 2023

Choose a reason for hiding this comment

NiwakaDev commented Jun 23, 2023

chinedufn commented Jun 23, 2023

NiwakaDev commented Jun 23, 2023

chinedufn commented Jun 23, 2023

Support `RustString` in extern Swift functions #225

Support `RustString` in extern Swift functions #225

NiwakaDev commented May 6, 2023 •

edited

Loading

NiwakaDev commented May 7, 2023 •

edited

Loading

NiwakaDev commented May 18, 2023 •

edited

Loading

NiwakaDev commented May 18, 2023 •

edited

Loading

NiwakaDev Jun 22, 2023 •

edited

Loading

NiwakaDev Jun 22, 2023 •

edited

Loading

chinedufn Jun 23, 2023 •

edited

Loading