chinedufn / swift-bridge

swift-bridge facilitates Rust and Swift interop.
https://chinedufn.github.io/swift-bridge
Apache License 2.0
810 stars 58 forks source link

Support Option<OpaqueSwiftType> #272

Closed Bright-Shard closed 5 months ago

Bright-Shard commented 5 months ago

This adds support for bridging Option<OpaqueSwiftType> in extern "Rust" functions. This fixes #268, and makes the following now possible:

#[swift_bridge::bridge]
mod ffi {
    extern "Swift" {
        type SomeSwiftType;
    }

    extern "Rust" {
        fn option_arg(arg: Option<SomeSwiftType>);
        fn returns_option() -> Option<SomeSwiftType>;
    }
}
chinedufn commented 5 months ago

Looks like tests are failing:

error[E0412]: cannot find type `OptTestOpaqueSwiftType` in module `super`
   --> crates/swift-integration-tests/src/option.rs:128:14
    |
128 |         type OptTestOpaqueSwiftType;
    |              ^^^^^^^^^^^^^^^^^^^^^^ help: a struct with a similar name exists: `OptTestOpaqueRustType`
...
202 | pub struct OptTestOpaqueRustType {
    | -------------------------------- similarly named struct `OptTestOpaqueRustType` defined here

For more information about this error, try `rustc --explain E0412`.
error: could not compile `swift-integration-tests` (lib) due to 1 previous error
Bright-Shard commented 5 months ago

That's embarrassing... I'd removed a use ffi::SwiftType statement and used absolute paths instead... but apparently you have to import the ffi types or it won't compile.

I commented on #270 with the output but on nightly some of the ui tests fail to run as well. Everything seems to pass now on stable though.

chinedufn commented 5 months ago

Hmm, not sure what happened.

Paths should work https://github.com/chinedufn/swift-bridge/blob/58f4a40f96bb050607c746376422ab3c62e0e771/crates/swift-integration-tests/src/option.rs#L326-L330

Cool. Looks good. I'll merge once tests pass.

chinedufn commented 5 months ago

Thanks!

chinedufn commented 5 months ago

Published as 0.1.54 https://crates.io/crates/swift-bridge/0.1.54

chinedufn commented 5 months ago

Replaced the pointer::casts with more explicit as *mut SomeType.

Explained here https://github.com/chinedufn/swift-bridge/commit/b4ba1a72a682c3917d78d6650ffb249c7109999b

chinedufn commented 5 months ago

When I was working on b4ba1a72a682c3917d78d6650ffb249c7109999b I noticed that when returning Swift types from Rust -> Swift we were running the Drop impl on the Rust representation of the Swift type.

https://github.com/chinedufn/swift-bridge/blob/636fa2774870c052a18f06d98ca5f6966c09bdd6/crates/swift-bridge-ir/src/codegen/codegen_tests/opaque_swift_type_codegen_tests.rs#L26-L33

It looks like this #272 PR was avoiding that Drop by incrementing the Swift type's retain count twice instead of once.

This means that we were leaking memory.

My apologies for not thinking of this during review.

Fixed https://github.com/chinedufn/swift-bridge/pull/273

Bright-Shard commented 5 months ago

That makes a lot more sense... I'd assumed for some reason it wasn't automatically incrementing the retain count, and thought I was just doing it once.

chinedufn commented 5 months ago

Got it.


0.1.55 has been released https://crates.io/crates/swift-bridge/0.1.55