Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support Option<OpaqueSwiftType> #272

Merged
merged 2 commits into from
Apr 28, 2024
Merged

Conversation

Bright-Shard
Copy link
Contributor

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>;
    }
}

@@ -15,34 +15,34 @@ class OptionTests: XCTestCase {
func testSwiftCallRustOptionPrimitive() throws {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Autoformatting did something to the spacing in this whole file... I'll leave a comment where I actually made changes


XCTAssertNil(rust_reflect_option_opaque_swift_type(nil))
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actual changes are here

@@ -65,3 +65,121 @@ func __swift_bridge__some_function (_ arg: UnsafeMutableRawPointer) {
.test();
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved here from extern_swift_function_opaque_swift_type_return_codegen_tests.rs... the original issue mentioned moving those tests

@@ -1,3 +1,5 @@
#![allow(dead_code)]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I faced the same issue as #270 - tests wouldn't even compile without this, since -D warnings is enabled for them. I also had to make this change to result.rs below. Maybe there's a better solution, this was just what I did to get tests running

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please document:

  • why we're allowing dead code
  • what needs to happen to remove this allow dead code
  • link to the issue

Seems like this can be deleted once #270 is closed. So, yeah just mention that.

This will make it easier for us to eventually stop allowing dead code.


Please also apply this feedback to result.rs

Copy link
Owner

@chinedufn chinedufn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work. Just one small bit of feedback.

@@ -1,3 +1,5 @@
#![allow(dead_code)]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please document:

  • why we're allowing dead code
  • what needs to happen to remove this allow dead code
  • link to the issue

Seems like this can be deleted once #270 is closed. So, yeah just mention that.

This will make it easier for us to eventually stop allowing dead code.


Please also apply this feedback to result.rs

@chinedufn
Copy link
Owner

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
Copy link
Contributor Author

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
Copy link
Owner

Hmm, not sure what happened.

Paths should work

fn rust_reflect_option_struct_with_no_data(
arg: Option<ffi::OptionStruct>,
) -> Option<ffi::OptionStruct> {
arg
}

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

@chinedufn chinedufn merged commit 636fa27 into chinedufn:master Apr 28, 2024
5 checks passed
@chinedufn
Copy link
Owner

Thanks!

@chinedufn
Copy link
Owner

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

@chinedufn
Copy link
Owner

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

Explained here b4ba1a7

@chinedufn
Copy link
Owner

chinedufn commented Apr 29, 2024

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

#[repr(C)]
pub struct MyType(*mut std::ffi::c_void);
impl Drop for MyType {
fn drop (&mut self) {
unsafe { __swift_bridge__MyType__free(self.0) }
}
}

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 #273

@Bright-Shard
Copy link
Contributor Author

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
Copy link
Owner

Got it.


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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support bridging Option<SwiftClass> in extern "Rust" function
2 participants