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

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

Closed PrismaPhonic closed 6 months ago

PrismaPhonic commented 6 months ago

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>;
  }
}
PrismaPhonic commented 6 months ago

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 commented 6 months ago

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:

https://github.com/chinedufn/swift-bridge/blob/4fbd30f81085dc366bda3c0303eac66345de3ed9/crates/swift-bridge-ir/src/bridged_type/bridged_opaque_type.rs#L438

Something like:

if self.reference {
    Some(unsafe {& * #expression})
} else {
    Some(unsafe { * Box::from_raw(#expression) } )
}
PrismaPhonic commented 6 months ago

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:

https://github.com/chinedufn/swift-bridge/blob/4fbd30f81085dc366bda3c0303eac66345de3ed9/crates/swift-bridge-ir/src/bridged_type/bridged_opaque_type.rs#L438

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 commented 6 months ago

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 commented 6 months ago

@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 commented 6 months ago

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

chinedufn commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

Addressed all comments - tests are passing for me locally now

chinedufn commented 6 months ago

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.