chinedufn / swift-bridge

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

Can't seem to return refs of custom types #256

Closed PrismaPhonic closed 8 months ago

PrismaPhonic commented 8 months ago

I'm having an issue where I can't seem to return refs of custom types. Here's the code:

#[swift_bridge::bridge]
mod ffi {
    extern "Rust" {
        type User;
        type Program;

        fn current_program_ref(self: &User) -> Option<&Program>;
    }
}

The above models the function signature from User that I'm copying. (I have other methods that are working that omitted for brevity).

This is the generated code for this method:

    #[export_name = "__swift_bridge__$User$current_program_ref"]
    pub extern "C" fn __swift_bridge__User_current_program_ref(
        this: *mut super::User,
    ) -> *mut super::Program {
        if let Some(val) = (unsafe { &*this }).current_program_ref() {
            Box::into_raw(Box::new(val))
        } else {
            std::ptr::null_mut()
        }
    }

The rest of the code generated relevant to the Program type:

    #[export_name = "__swift_bridge__$Program$_free"]
    pub extern "C" fn __swift_bridge__Program__free(this: *mut super::Program) {
        let this = unsafe { Box::from_raw(this) };
        drop(this);
    }
    const _: () = {
        #[doc(hidden)]
        #[export_name = "__swift_bridge__$Vec_Program$new"]
        pub extern "C" fn _new() -> *mut Vec<super::Program> {
            Box::into_raw(Box::new(Vec::new()))
        }
        #[doc(hidden)]
        #[export_name = "__swift_bridge__$Vec_Program$drop"]
        pub extern "C" fn _drop(vec: *mut Vec<super::Program>) {
            let vec = unsafe { Box::from_raw(vec) };
            drop(vec)
        }
        #[doc(hidden)]
        #[export_name = "__swift_bridge__$Vec_Program$len"]
        pub extern "C" fn _len(vec: *const Vec<super::Program>) -> usize {
            unsafe { &*vec }.len()
        }
        #[doc(hidden)]
        #[export_name = "__swift_bridge__$Vec_Program$get"]
        pub extern "C" fn _get(
            vec: *const Vec<super::Program>,
            index: usize,
        ) -> *const super::Program {
            let vec = unsafe { &*vec };
            if let Some(val) = vec.get(index) {
                val as *const super::Program
            } else {
                std::ptr::null()
            }
        }
        #[doc(hidden)]
        #[export_name = "__swift_bridge__$Vec_Program$get_mut"]
        pub extern "C" fn _get_mut(
            vec: *mut Vec<super::Program>,
            index: usize,
        ) -> *mut super::Program {
            let vec = unsafe { &mut *vec };
            if let Some(val) = vec.get_mut(index) {
                val as *mut super::Program
            } else {
                std::ptr::null::<super::Program>() as *mut super::Program
            }
        }
        #[doc(hidden)]
        #[export_name = "__swift_bridge__$Vec_Program$push"]
        pub extern "C" fn _push(
            vec: *mut Vec<super::Program>,
            val: *mut super::Program,
        ) {
            unsafe { &mut *vec }.push(unsafe { *Box::from_raw(val) })
        }
        #[doc(hidden)]
        #[export_name = "__swift_bridge__$Vec_Program$pop"]
        pub extern "C" fn _pop(vec: *mut Vec<super::Program>) -> *mut super::Program {
            let vec = unsafe { &mut *vec };
            if let Some(val) = vec.pop() {
                Box::into_raw(Box::new(val))
            } else {
                std::ptr::null::<super::Program>() as *mut super::Program
            }
        }
        #[doc(hidden)]
        #[export_name = "__swift_bridge__$Vec_Program$as_ptr"]
        pub extern "C" fn _as_ptr(
            vec: *const Vec<super::Program>,
        ) -> *const super::Program {
            unsafe { &*vec }.as_ptr()
        }

and the error:

error[E0308]: mismatched types
   --> libs/workout-planner-bindings/workout-planner-swift/src/lib.rs:10:1
    |
10  | #[swift_bridge::bridge]
    | ^^^^^^^^^^^^^^^^^^^^^^^
    | |
    | expected `Program`, found `&Program`
    | arguments to this function are incorrect
chinedufn commented 8 months ago

Nevermind. I see that you addressed this in your comment.




Does the implementation of User::current_program_ref return Option<Program> (notice the missing &)?

If so, you'll want something like:

#[swift_bridge::bridge]
mod ffi {
    extern "Rust" {
        type User;
        type Program;

        #[swift_bridge(return_with = Option::as_ref)]
        fn current_program_ref(self: &User) -> Option<&Program>;
    }
}

// Assuming: your code looks like:
impl User {
    fn current_program_ref(&self) -> Option<Program> { unimplemented!() }
}
chinedufn commented 8 months ago

Ah, it looks like we currently only test returning Option<T>

https://github.com/chinedufn/swift-bridge/blob/dd5bef56af28db4f1a1244d86115def6abede931/SwiftRustIntegrationTestRunner/SwiftRustIntegrationTestRunnerTests/OptionTests.swift#L93-L99

https://github.com/chinedufn/swift-bridge/blob/dd5bef56af28db4f1a1244d86115def6abede931/crates/swift-integration-tests/src/option.rs#L84-L86

https://github.com/chinedufn/swift-bridge/blob/dd5bef56af28db4f1a1244d86115def6abede931/crates/swift-bridge-ir/src/codegen/codegen_tests/option_codegen_tests.rs#L405-L461


I'd be happy to provide instructions on how to add support for returning Option<&T>. Would boil down to adding a new test case next to each of the ones linked above.

chinedufn commented 8 months ago

Look like this:

#[export_name = "__swift_bridge__$User$current_program_ref"]
pub extern "C" fn __swift_bridge__User_current_program_ref(
    this: *mut super::User,
) -> *mut super::Program {
    if let Some(val) = (unsafe { &*this }).current_program_ref() {
        Box::into_raw(Box::new(val))
    } else {
        std::ptr::null_mut()
    }
}

Should be:

#[export_name = "__swift_bridge__$User$current_program_ref"]
pub extern "C" fn __swift_bridge__User_current_program_ref(
    this: *mut super::User,
) -> *const super::Program {
    if let Some(val) = (unsafe { &*this }).current_program_ref() {
        val as *const super::Program
    } else {
        std::ptr::null()
    }
}
PrismaPhonic commented 8 months ago

I was about to say - I just found from experimenting some more that it appears the issue lies in the ref being nested inside the Option. Wow, you're fast!

PrismaPhonic commented 8 months ago

I can add the new test cases but I would imagine you would be faster at the actual implementation :-P but if you can give instructions for where the translation is happening I can give it a shot

chinedufn commented 8 months ago

Implementation would go here:

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

Can replace the linked code with:

let ty = &self.ty;

if self.reference {
    quote! {
        if let Some(val) = #expression {
            val as *const super::#ty
        } else {
            std::ptr::null()
        }
    }
} else {
    quote! {
        if let Some(val) = #expression {
            Box::into_raw(Box::new(val))
        } else {
            std::ptr::null_mut()
        }
    }
}

Please don't hesitate to let me know if you have any questions.

PrismaPhonic commented 8 months ago

Alright pushed that up: https://github.com/chinedufn/swift-bridge/pull/257

How do I run the full suite of tests?

EDIT: NVM, I see in the README - running now

PrismaPhonic commented 8 months ago

Hmm, I seem to be hitting this:

error[E0308]: mismatched types
 --> crates/swift-integration-tests/src/option.rs:3:1
  |
3 | #[swift_bridge::bridge]
  | ^^^^^^^^^^^^^^^^^^^^^^^ expected `&OptTestOpaqueRustType`, found `OptTestOpaqueRustType`

Not sure if it's obvious to you from looking at the PR I linked. I would try cargo expand but in this case it expands so much I have trouble finding the relevant code.

Edit: I would guess it probably has something to do with my addition of this:

fn rust_reflect_option_ref_opaque_rust_type(
    arg: Option<&OptTestOpaqueRustType>,
) -> Option<&OptTestOpaqueRustType> {
    arg
}

And then my import for generation:

        fn rust_reflect_option_ref_opaque_rust_type(
            arg: Option<&OptTestOpaqueRustType>,
        ) -> Option<&OptTestOpaqueRustType>;

But I'm not sure why this alone would produce this error as it seems like an extremely simple function and a very simple declaration in the extern "Rust" block

PrismaPhonic commented 8 months ago

May have found it. I think I needed to also change fn to_ffi_compatible_option_rust_type() in bridged_opaque_type.rs to check the reference as well, and conditionally return either *const or *mut but now I'm getting this:

error[E0308]: mismatched types
   --> crates/swift-integration-tests/src/option.rs:89:13
    |
3   | #[swift_bridge::bridge]
    | ----------------------- arguments to this function are incorrect
...
89  |             arg: Option<&OptTestOpaqueRustType>,
    |             ^^^ types differ in mutability
    |
    = note: expected raw pointer `*mut _`
               found raw pointer `*const OptTestOpaqueRustType`

It seems that the pointer it found is the type we want - I'm not sure why it expects *mut

PrismaPhonic commented 8 months ago

from cargo expand it looks like the generated code for the test now:

        #[export_name = "__swift_bridge__$rust_reflect_option_ref_opaque_rust_type"]
        pub extern "C" fn __swift_bridge__rust_reflect_option_ref_opaque_rust_type(
            arg: *const super::OptTestOpaqueRustTypeRef,
        ) -> *const super::OptTestOpaqueRustTypeRef {
            if let Some(val) = super::rust_reflect_option_ref_opaque_rust_type(
                if arg.is_null() { None } else { Some(unsafe { *Box::from_raw(arg) }) },
            ) {
                val as *const super::OptTestOpaqueRustTypeRef
            } else {
                std::ptr::null()
            }
        }

It seems to have an issue with from_raw() getting called. I can see that Box::from_raw expects a *mut but I'm not sure what the correct replacement would be here

chinedufn commented 8 months ago

Thanks for reporting this issue and including clear details. Much appreciated.

I'll reply to any other questions here -> https://github.com/chinedufn/swift-bridge/pull/257