Add test cases for Option<&T> and fix rust codegen

[PrismaPhonic]

Add test cases for Option<&T> and fix rust codegen

Currently swift-bridge only has tests for Option - this adds test
cases for Option<&T> and fixes a bug in rust codegen that does not
correctly translate Option<&T>.

This is now possible:

mod ffi {
  extern "Rust" {
    type MyStruct;

    fn my_func(arg: Option<&MyStruct>) -> Option<&MyStruct>;
  }
}

[chinedufn]

Looks great. Thanks a lot for taking the time to put this together.

Looks like you found the if self.reference { quote! { *const super::#type_name #generics } } bit. Awesome.

---

Just two changes please:

---

Avoid dropping the underlying Rust type when the Swift side drops the reference to the Rust type.
This is explained in the PR comments.

---

Update your PR body to show an example of a bridge module that did not work before this commit but works as of this commit.

Example: #203 (comment)

This commit adds support for Option<PrimitiveType> in `extern "Swift"`
functions.

For example, the following is now possible:

```rust
mod ffi {
    extern "Swift" {
        fn my_func(arg: Option<u8>) -> Option<u8>;
    }
}
```

Reasons:

• These examples get used in release notes.
• We'll sometimes link a new contributor to an old similar PR. It's useful to be able to quickly see exactly what the PR is enabling.

---

After that this looks great and can land. Thank you for putting this together.

[chinedufn]

Left some notes on how to pass CI.

Also need to run cargo fmt.

[chinedufn]

Looks like you need one more codegen test module for extern_rust_fn_arg_option_ref_opaque_rust_type where you have

fn bridge_module_tokens() -> TokenStream {
    quote! {
        mod ffi {
            extern "Rust" {
                type SomeType;
                fn some_function (arg: Option<&SomeType>);
            }
        }
    }
}

CI is failing because the codegen for the arg: Option<&SomeType> is currently incorrect.

Once that's fixed CI should pass. I'm happy to provide .......... pointers.

[PrismaPhonic]

Hahahaha, thank you. This is my first foray into FFI land so I appreciate it.

I think I am missing what is needed here. Another bridge_module_tokens() method?

[chinedufn]

Gotcha:

1. Copy the module extern_rust_fn_return_option_ref_opaque_rust_type to a new module below it extern_rust_fn_arg_option_ref_opaque_rust_type

2. Replace the bridge module in extern_rust_fn_arg_option_ref_opaque_rust_type with the one in the comment above.

3. extern_rust_fn_arg_option_ref_opaque_rust_type will pass

4. Tweak the expected_rust_tokens to make it pass

[PrismaPhonic]

Alright - did that to the best of my abilities and pushed up.

[chinedufn]

This won't compile since we're trying to return a reference to a temporary value.

Can do something like:

pub struct OptTestOpaqueRustTypeRef {
    field: u8,
}

impl OptTestOpaqueRustTypeRef {
    fn new_some(field: u8) -> Option<Self> {
        Some(Self { field })
    }

    fn field(&self) -> u8 {
        self.field
    }
}

fn rust_convert_option_ty_to_option_ref_ty(val: Option<OptTestOpaqueRustTypeRef>) -> Option<&OptTestOpaqueRustTypeRef> {
    val.as_ref()
}

Then on the Swift side can call OptTestOpaqueRustTypeRef.new_some and then rust_convert_option_ty_to_option_ref_ty to get the Option<&OptTestOpaqueRustTypeRef> that you need.

---

Here's how to call an associated method such as new_some

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

Lines 146 to 189 in dd5bef5

	#### #[swift_bridge(associated_to = SomeType)]
	Indicates that we are exposing an associated function for a type.
	```rust
	// Rust
	#[swift_bridge::bridge]
	mod ffi {
	extern "Rust" {
	type Message;
	// Exposes Message::parse to Swift as Message.parse
	#[swift_bridge(associated_to = Message)]
	fn parse(text: &str) -> Option<Message>;
	}
	}
	struct LongMessage(String);
	impl LongMessage {
	fn parse(text: impl ToString) -> Option<Self> {
	let text = text.to_string();
	if text.len() > 10_000 {
	Some(LongMessage(text))
	} else {
	None
	}
	}
	}
	```
	```swift
	// Swift
	func maybeSendLongMessage(text: String) {
	let maybeMessage = Message.parse(text)
	if let message = maybeMessage {
	// ... send the message
	}
	}
	```

[PrismaPhonic]

I believe I've done that correctly - but I'm brand new to swift (why I'm trying out swift-rust-bridge :-P )

[PrismaPhonic]

Ahhh so the compiler still isn't happy here because ownership of the inner value isn't clear which makes sense. We are passing back the ref, and not the original - the original would be dropped by the end of the function call leaving a ref with no owner

We could hand back a tuple pair of the original and the ref version but I don't know if tuples with Option<&Ref> inside are supported yet?

[chinedufn]

Ah whoops.

This is one solution:

impl OptTestOpaqueRustTypeRef {
    fn new(field: u8) -> Self {
        Self { field }
    }

    fn field(&self) -> u8 {
        self.field
    }
}

fn rust_convert_ref_ty_to_option_ref_ty(val: &OptTestOpaqueRustTypeRef) -> Option<&OptTestOpaqueRustTypeRef> {
    Some(val)
}
``

[PrismaPhonic]

Maybe something like this?

pub struct OptTestOpaqueRustTypeRef {
    field: Option<OptTestOpaqueRustType>,
}

impl OptTestOpaqueRustTypeRef {
    fn new(field: u8) -> Self {
        Some(Self { field: Some(OptTestOpaqueRustType::new(field)) })
    }

    fn field(&self) -> Option<OptTestOpaqueRustType> {
        self.field
    }

    fn field_ref(&self) -> &Option<OptTestOpaqueRustType> {
        &self.field
    }
}

fn rust_convert_ref_option_ty_to_option_ref_ty(
    val: &Option<OptTestOpaqueRustType>,
) -> Option<&OptTestOpaqueRustType> {
    val.as_ref()
}

Then the lifetime of the returned inner T is tied to the lifetime of the passed in &Option?

[chinedufn]

Only challenge is that I'm not sure if &Option<T> is supported yet. SO the val: &Option<OptTestOpaqueRustType> might not work.

I like the general direction though. What about:

pub struct OptTestOpaqueRustTypeRef {
    field: Option<GoodNameForOpaqueRustTypeHere>,
}

impl OptTestOpaqueRustTypeRef {
    fn new(field: u8) -> Self {
        Some(Self { field: Some(GoodNameForOpaqueRustTypeHere::new(field)) })
    }

    fn field_ref(&self) -> Option<&GoodNameForOpaqueRustTypeHere> {
        self.field.as_ref()
    }
}

Then the Swift test code can call .field_ref() and then reflect the Option<&GoodNameForOpaqueRustTypeHere>.

[PrismaPhonic]

Oh right - that should work! Because then the parent object still owns the inner type. I'll try that :) thanks!

[chinedufn]

Team work made the dream work!

[PrismaPhonic]

Alright, got that pushed up - hitting another compiler error (that I hit before) listed below in a general comment

[PrismaPhonic]

I've addressed all concerns listed here but seem to still hit this compiler error:

error[E0308]: mismatched types
   --> crates/swift-integration-tests/src/option.rs:95:13
    |
3   | #[swift_bridge::bridge]
    | ----------------------- arguments to this function are incorrect
...
95  |             arg: Option<&OptTestOpaqueRustType>,
    |             ^^^ types differ in mutability
    |
    = note: expected raw pointer `*mut _`
               found raw pointer `*const OptTestOpaqueRustType`
note: associated function defined here
   --> /home/pmfarr/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/boxed.rs:950:19
    |
950 |     pub unsafe fn from_raw(raw: *mut T) -> Self {

EDIT: Do you know if there's way to have the compiler show the error at the expanded code? I know I can cargo expand to see the full expansion but I'd love it if I could link cargo check to show the expanded version. These compiler errors aren't the most useful since they always point at the macro.

I think that we are generating a function that takes in a *const (which it should) but then tries to call Box::from_raw() on it which wants a *mut - but I'm not sure where to fix that

[chinedufn]

Do you know if there's way to have the compiler show the error at the expanded code?

I think there's a rustc flag for it but I've never used it https://stackoverflow.com/a/68843349

I think that we are generating a function that takes in a *const (which it should) but then tries to call Box::from_raw() on it which wants a *mut - but I'm not sure where to fix that

Yup. Can do another if self.reference in here:

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

Line 438 in 4fbd30f

Some(unsafe { * Box::from_raw(#expression) } )

Something like:

if self.reference {
    Some(unsafe {& * #expression})
} else {
    Some(unsafe { * Box::from_raw(#expression) } )
}

[chinedufn]

I think this comment got swallowed since I was commenting on orphaned commits, so let me try here in the PR's "files changed" page.

Can remove the -> Option<&SomeType> here.

I made a mistake when I said to add it. The return case is already tested in the other test that you added so we don't need it here.

[PrismaPhonic]

So remove the entire module then? or...

[chinedufn]

Keep the module but remove the -> Option<&SomeType> from this line 530 right here

So fn some_function (arg: Option<&SomeType>) -> Option<&SomeType>;

becomes fn some_function (arg: Option<&SomeType>);

[chinedufn]

I think this should do the trick

[chinedufn]

Just FYI here's a good test to take inspiration from:

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

Lines 463 to 523 in fdca9fc

	/// Test code generation for Rust function that takes an Option<OpaqueRustType> argument.
	mod extern_rust_fn_with_option_opaque_rust_type_arg {
	use super::*;
	fn bridge_module_tokens() -> TokenStream {
	quote! {
	mod ffi {
	extern "Rust" {
	type SomeType;
	fn some_function (arg: Option<SomeType>);
	}
	}
	}
	}
	fn expected_rust_tokens() -> ExpectedRustTokens {
	ExpectedRustTokens::Contains(quote! {
	#[export_name = "__swift_bridge__$some_function"]
	pub extern "C" fn __swift_bridge__some_function(
	arg: *mut super::SomeType
	) {
	super::some_function(
	if arg.is_null() {
	None
	} else {
	Some( unsafe { * Box::from_raw(arg) } )
	}
	)
	}
	})
	}
	fn expected_swift_code() -> ExpectedSwiftCode {
	ExpectedSwiftCode::ContainsAfterTrim(
	r#"
	func some_function(_ arg: Optional<SomeType>) {
	__swift_bridge__$some_function({ if let val = arg { val.isOwned = false; return val.ptr } else { return nil } }())
	}
	"#,
	)
	}
	fn expected_c_header() -> ExpectedCHeader {
	ExpectedCHeader::ContainsAfterTrim(
	r#"
	void __swift_bridge__$some_function(void* arg);
	"#,
	)
	}
	#[test]
	fn extern_rust_fn_with_option_opaque_rust_type_arg() {
	CodegenTest {
	bridge_module: bridge_module_tokens().into(),
	expected_rust_tokens: expected_rust_tokens(),
	expected_swift_code: expected_swift_code(),
	expected_c_header: expected_c_header(),
	}
	.test();
	}
	}

I'll comment below with the changes though, so no need to study that one too carefully.

[chinedufn]

This can be:

#[export_name = "__swift_bridge__$some_function"]
pub extern "C" fn __swift_bridge__some_function(
    arg: *const super::SomeType
) {
    super::some_function(
        if arg.is_null() {
            None
        } else {
            Some( unsafe { & * arg } )
        }
    )
}

[chinedufn]

This one is similar to:

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

Lines 91 to 99 in 607e305

	fn expected_swift_code() -> ExpectedSwiftCode {
	ExpectedSwiftCode::ContainsAfterTrim(
	r#"
	func some_function(_ arg: SomeTypeRef) {
	__swift_bridge__$some_function(arg.ptr)
	}
	"#,
	)
	}

So it can be:

func some_function(_ arg: Optional<SomeTypeRef>) {
    __swift_bridge__$some_function({ if let val = arg { return val.ptr } else { return nil } }())
}

[PrismaPhonic]

Seems that generated is this:

func some_function(_ arg: Optional<SomeTypeRef>) {
    __swift_bridge__$some_function({ if let val = arg { val.isOwned = false; return val.ptr } else { return nil } }())
}

Just checking that the assignment to setting isOwned to false is correct?

[PrismaPhonic]

It does by the way look like codegen on the previous test (that you told me to change to return SomeTypeRef is actually generating return SomeType - I'm guessing this is a mistake and something I need to correct in translation to swift somewhere. Do you know where that would be?

[chinedufn]

isOwned is available on owned SomeType

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

Lines 36 to 48 in dd5bef5

	public class SomeType: SomeTypeRefMut {
	var isOwned: Bool = true
	public override init(ptr: UnsafeMutableRawPointer) {
	super.init(ptr: ptr)
	}
	deinit {
	if isOwned {
	__swift_bridge__$SomeType$_free(ptr)
	}
	}
	}

but not on SomeTypeRef

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

Lines 54 to 60 in dd5bef5

	public class SomeTypeRef {
	var ptr: UnsafeMutableRawPointer
	public init(ptr: UnsafeMutableRawPointer) {
	self.ptr = ptr
	}
	}

---

So, we just need what should hopefully be one last if self.reference here

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

Lines 312 to 325 in 4fbd30f

	TypePosition::FnArg(func_host_lang, _)
	| TypePosition::FnReturn(func_host_lang) => {
	if func_host_lang.is_rust() {
	format!(
	"{{{}.isOwned = false; return {}.ptr;}}()",
	expression, expression
	)
	} else {
	format!(
	"{{{}.isOwned = false; return {}.ptr;}}()",
	expression, expression
	)
	}
	}

[PrismaPhonic]

Oh wait - think I found it. How do you feel about adding a helper method to OpaqueForeignType of:

    pub fn swift_ref_name(&self) -> String {
        format!("{}Ref", self.ty)
    }

It looks like we only call swift_name which then doesn't include the Ref suffix

[chinedufn]

Hahaha look what I was about to comment with before I saw these last two comments:

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

Lines 639 to 642 in 4fbd30f

	impl OpaqueForeignType {
	pub fn swift_name(&self) -> String {
	format!("{}", self.ty)
	}

pub fn swift_name(&self) -> String {
	if self.reference {
		format!("{}", self.ty)
	} else {
		format!("{}Ref", self.ty)
	}
}

Your FFI-fu has grown strong my friend.

[PrismaPhonic]

:-P

Soooo this section you mentioned, the two blocks for if and else are already the same:

     if func_host_lang.is_rust() { 
         format!( 
             "{{{}.isOwned = false; return {}.ptr;}}()", 
             expression, expression 
         ) 
     } else { 
         format!( 
             "{{{}.isOwned = false; return {}.ptr;}}()", 
             expression, expression 
         ) 
     } 

I imagine that's a mistake - what should they be? I imagine they were supposed to be different depending on the host lang

[chinedufn]

I think that as you start handling more cases the two languages start to diverge but based on what we currently have tested they're always the same.

As in, you end up starting to add certain if statements based on the language.

As in, in some cases we generate the same code no matter which language the type came from, in other cases we don't.

So what happened here was probably something like:

1. if rust do X, if swift todo!()
2. one day we needed to add support for Swift so we wrote a test case
3. then we changed the logic to -> if rust do X, if swift do X
4. FUTURE -> if rust do X or Y depending on Z, if swift do X or Q depending on W

So, this is just in the middle of that transition to step 4.

I suppose you could collapse then if you wanted, but I think it's better as is so that when someone someday needs to modify one language they can clearly see where to do it (I haven't had to do this in a while so I forget what can cause the divergence).

If you disagree and think we should collapse back down until that day comes, feel free to collapse it.

I imagine they were supposed to be different depending on the host lang

Oh I missed this part.

100% of types/scenarios that we support have corresponding tests so there shouldn't be any logical errors or regressions.

The only things that shouldn't work are things that we have not yet explicitly added support for.

So, there shouldn't be anything that is broken here.

[PrismaPhonic]

Gotcha! I have clippy running by rust_analyzer so it likes to alert me to lots of things like this. I'll update both branches to be the same then, thanks!

[PrismaPhonic]

One more spot I needed to check reference, but all working now!

In convert_option_swift_expression_to_ffi_type

[chinedufn]

This one is similar to

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

Lines 101 to 107 in 607e305

	fn expected_c_header() -> ExpectedCHeader {
	ExpectedCHeader::ContainsAfterTrim(
	r#"
	void __swift_bridge__$some_function(void* arg);
	"#,
	)
	}

So it can be:

void __swift_bridge__$some_function(void* arg);

[PrismaPhonic]

Do you know if there's way to have the compiler show the error at the expanded code?

I think there's a rustc flag for it but I've never used it https://stackoverflow.com/a/68843349

I think that we are generating a function that takes in a *const (which it should) but then tries to call Box::from_raw() on it which wants a *mut - but I'm not sure where to fix that

Yup. Can do another if self.reference in here:

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

Line 438 in 4fbd30f

Some(unsafe { * Box::from_raw(#expression) } )

Something like:

if self.reference {
    Some(unsafe {& * #expression})
} else {
    Some(unsafe { * Box::from_raw(#expression) } )
}

Done, annnnnd now I'm hitting way more compiler errors :(

error[E0424]: expected value, found module `self`
  --> crates/swift-integration-tests/src/option.rs:3:1
   |
3  | #[swift_bridge::bridge]
   | ^^^^^^^^^^^^^^^^^^^^^^^ `self` value is a keyword only available in methods with a `self` parameter
...
90 |         fn rust_reflect_option_opaque_rust_type(
   |            ------------------------------------ this function can't have a `self` parameter
   |
   = note: this error originates in the attribute macro `swift_bridge::bridge` (in Nightly builds, run with -Z macro-backtrace for more info)

Somehow now we are generating functions that don't have self to have self. I'm soooooo confused

[chinedufn]

Done, annnnnd now I'm hitting way more compiler errors :(

Ah gotcha. Just need the self to be outside of the quote! :

if self.reference {
    quote! {
        if #expression.is_null() {
            None
        } else {
            Some(unsafe {& * #expression})
        }
    }
} else {
    quote! {
        if #expression.is_null() {
            None
        } else {
            Some(unsafe { * Box::from_raw(#expression) } )
        }
    }
}

[PrismaPhonic]

@chinedufn addressed all issues - all tests are passing for me locally (using the command in the README)

EDIT: I am seeing now that command from the README seems to only run the rust tests. How do I have it run the swift tests?

EDIT-EDIT: Ran them from my Macbook and all working! I do all of my Rust dev on my work laptop (Arch Linux) and so it wasn't running the swift tests. Should be good to go to run in CI now

[PrismaPhonic]

Huh, I wonder why integration tests failed in the pipeline but that one test in particular succeeds locally for me

[chinedufn]

Just need to delete a file and rename a type and then looks good to me.

Thanks for putting this together!

[chinedufn]

Need to rename this. the type OptTestOpaqueRustType above generates class OptTestOpaqueRustTypeRef on the Swift side, and then this also generates a class with the same name.

Can call it OptTestOpaqueRefRustType instead

[PrismaPhonic]

ohhhhh right - that makes sense. Doh! magical strings hahaha

[chinedufn]

Here's more information as to why this error is happening.

No need to read this link to solve the problem.
Renaming the type (I gave an example under the screenshot in the previous comment) will do.

swift-bridge/book/src/bridge-module/opaque-types/README.md

Lines 52 to 133 in dd5bef5

	## Owned, Ref and RefMut
	When you define a type in an `extern "Rust"` block, three corresponding Swift classes get generated.
	```swift
	// Equivalent to `SomeType` in Rust
	class SomeType: SomeTypeRefMut {
	// ...
	}
	// Equivalent to `&mut SomeType` in Rust
	class SomeTypeRefMut: SomeTypeRef {
	// ...
	}
	// Equivalent to `&SomeType` in Rust
	class SomeTypeRef {
	// ...
	}
	```
	Here's an example of how `&Type` and `&mut Type` are enforced:
	```rust
	// Rust
	extern "Rust" {
	type SomeType;
	#[swift_bridge(init)]
	fn new() -> SomeType;
	// Callable by SomeType, SomeTypeRef and SomeTypeRefMut.
	fn (&self) everyone();
	// Callable by SomeType, and SomeTypeRefMut.
	fn (&mut self) only_owned_and_ref_mut();
	// Only callable by SomeType.
	fn (self) only_owned();
	}
	extern "Rust" {
	fn make_ref() -> &'static SomeType;
	fn make_ref_mut() -> &'static mut SomeType;
	}
	```
	```swift
	// Swift
	func methods() {
	let someType: SomeType = SomeType()
	let someTypeRef: SomeTypeRef = make_ref()
	let someTypeRefMut: SomeTypeRefMut = make_ref_mut()
	someType.everyone()
	someType.only_owned_and_ref_mut()
	someType.only_owned()
	someTypeRefMut.everyone()
	someTypeRefMut.only_owned_and_ref_mut()
	someTypeRef.everyone()
	}
	func functions() {
	let someType: SomeType = SomeType()
	let someTypeRef: SomeTypeRef = make_ref()
	let someTypeRefMut: SomeTypeRefMut = make_ref_mut()
	takeReference(someType)
	takeReference(someTypeRef)
	takeReference(someTypeRefMut)
	}
	// Can be called with SomeType, SomeTypeRef and SomeTypeRefMut
	func useSomeType(someType: SomeTypeRef) {
	// ...
	}
	```

[PrismaPhonic]

Done and pushed up

[PrismaPhonic]

Hmm, actually, now I'm getting this failure locally:

Testing failed:
	Missing argument label 'ptr:' in call
	Cannot convert value of type 'Int' to expected argument type 'UnsafeMutableRawPointer'
	'nil' requires a contextual type
	Command SwiftCompile failed with a nonzero exit code
	Testing cancelled because the build failed.

[PrismaPhonic]

I'm taking a wild guess here as I don't know swift that well but I'm guessing it's related to my logic here:

    func testSwiftCallRustWithOptionRefOpaqueRustType() throws {
        let val = OptTestOpaqueRefRustType.new(123)
        let opt_ref = val.field_ref()

        let reflect = rust_reflect_option_ref_opaque_rust_type(opt_ref)
        XCTAssertEqual(reflect!.field(), 123)
        XCTAssertNil(rust_reflect_option_ref_opaque_rust_type(nil))
        let reflect = nil
        
        let second_reference = rust_reflect_option_ref_opaque_rust_type(opt_ref)
        XCTAssertEqual(second_reference!.field(), 123)
    }

I wonder if this is the problem:

        XCTAssertEqual(reflect!.field(), 123)

Is it saying that the result of field() is a unsafe mutable raw pointer and it's trying to coerce the int of 123 into that but failing for the equality comparison?

In this case I call val.field_ref() which should get us the inner Option<&Type> and then I thought reflect!.field() goes inside that option and calls the field() method on the inner type which should then get us an int out?

[chinedufn]

Mind pushing new commits instead of force pushing? This way I can see what has changed. I'll squash them all at the end.

[PrismaPhonic]

Mind pushing new commits instead of force pushing? This way I can see what has changed. I'll squash them all at the end.

Can do - Github in particular has this problem unfortunately. In other platforms you can see a diff between rebases

[PrismaPhonic]

Addressed all comments - tests are passing for me locally now

[chinedufn]

Thanks for reporting this issue and thanks for putting together a fix. Appreciate you pushing through the unexpected compile time error after compile time error after compile time error.

Cheers.