Adding Vec support for transparent structs

[rkreutz]

Original PR was #193, but I had decided to open a PR from a feature branch rather than from master from my fork.

---

This PR addresses #91

Added all the shims so transparent structs can support Vec. A few caveats though:

• Structs with swift_repr = "class" are not supported, this PR only add support to swift_repr = "struct"
• For Vec support to actually be generated, the struct must be annotated with #[derive(Copy, Clone)]
  • Vec support is also generated if the struct is annotated with only #[derive(Clone)], this is useful for structs with fields that cannot implement Copy (like String), however there is a performance overhead of using Clone rather than Copy, so it is advised to include #[derive(Copy)] as well.
  • For Vec support to be generated, you MUST use the #[derive] macro, implementing the trait with impl Copy for Struct {} won't generate the Vec support code.

---

Here is an example of the Rust code:

#[swift_bridge::bridge]
mod ffi {
    #[swift_bridge(swift_repr = "struct")]
    #[derive(Copy, Clone)]
    struct SomeStruct {
        field: u8,
    }

    #[swift_bridge(swift_repr = "struct")]
    #[derive(Clone)] // This is supported but not recommended because of performance overhead of executing .clone() when getting elements from the Vec/Array
    struct AnotherStruct {
        field: String,
    }

    extern "Rust" {
        fn do_something(vec1: Vec<SomeStruct>, vec2: Vec<AnotherStruct>);
    }
}

pub fn do_something(vec1: Vec<SomeStruct>, vec2: Vec<AnotherStruct>) {
    /* do your thing */
}

And then on Swift you can:

let vec = RustVec<SomeStruct>()
vec.push(SomeStruct(field: 10))

let vec2 = RustVec<AnotherStruct>()
vec2.push(AnotherStruct(field: "cool".intoRustString()))

do_something(vec, vec2)

[rkreutz]

Notice that if the struct has the Copy derive we'll opt for using a copy rather than a clone. This whole block is guarded by an if statement checking if can_generate_vec_of_transparent_struct_functions, which at the moment will only return true if it is a swift_repr = "struct" with a #[derive(Copy, Clone)] or #[derive(Clone)].

[rkreutz]

@chinedufn this should be ready to be reviewed once you have the time. Cheers

[chinedufn]

Awesome! I'll review this weekend.
Thanks a lot for the detailed PR body. I found it to be very helpful.

Vec support is also generated if the struct is annotated with only #[derive(Clone)]

I don't think that we can land this part. swift-bridge is designed for high performance, zero or near-zero overhead bridging library.
Users need to trust that swift-bridge will never be the root cause of a surprising performance issue in their systems.

This means that implicit cloning is unacceptable.

I think that we should land the Copy vec support, ditch the Clone support and implement Vec<NonCopyStruct> after swift_repr = "class" support lands.

[rkreutz]

Do you think we could put this kind of behavior behind a flag on the crate, so people can explicitly opt for the degraded performance in favor of supporting String fields?

[chinedufn]

What's your motivation?

I'd rather just implement swift_repr = "class" and have a performant solution to this problem, but I'm curious about your thoughts / motives here?

[rkreutz]

Hey,

yeah, so my reasoning is that on most cases structs would contain Strings or other fields that can't implement Copy, so this change wouldn't be of much use. I understand that we could wait until swift_repr = "class" support is implemented, but I don't see that happening anytime soon tbh (at least I have no clue how to transform the pointers between Rust repr and Swift repr, so I don't think there is a solution any time soon), so Vec support will effectively take much more time to be added here.

---

About using pointers to pass the values around and not needing to use Copy/Clone, I guess whatever implementation we come up with will be compatible with both struct and class Swift repr.

I was thinking about how to do it, the main issue I've stumbled is that we pass a pointer to the Rust repr to Swift (on current Vec support that is) so Swift doesn't have a way to transform that Pointer into it's own repr, so we would have to start using the FFI repr pointer instead, which both Swift and Rust can understand, but that would mean that we would need to change somehow fn arguments and return values to be of type *const Vec<ffi_repr> rather than *const Vec<rust_repr>, which I have no clue how to do...

I might be completely wrong on how to approach this as well. But yeah that was pretty much why I thought would be a good idea to enable the use of Clone...

[chinedufn]

Gotcha, thanks for explaining. Yeah, agreed that Copy Vec support isn't super useful.

---

I'll take some time on Tuesday to think through swift_repr = "class" and write down a potential design.
I might be able to whip up an implementation of the design.

I had some ideas pop into my head earlier today. It might actually be fairly straightforwards.. but I have to sketch it out to be sure..

If it ends up being easy we can

1. Add swift_repr = "class"
2. Try to land Vec support for classes.

---

I would say that no matter what we probably won't land clone based Vec support, even behind a feature flag.
I get that it's a quick fix, but it strays too far from our high performance goals.

Happy to land this PR with Copy swift_repr = "struct" support.

Otherwise I should have time to design how swift_repr = "class" can work on Tuesday.

[rkreutz]

Hey, I think it will be better to just wait until we land the class repr Vec support then we can do the same for struct. Cheers

[chinedufn]

Sounds good. Feel free to subscribe to the swift_repr = "class" issue.

---

Just wrote down a design that I think should work idea but I think it needs more iteration... #196 (comment)

[rkreutz]

hey @chinedufn I've added a new feature flag to the package here (compatibility) which would prioritise compatibility over performance. If the feature is enabled then struct deriving Clone and not Copy would use .clone() on the vec support code; if the feature is disabled (which is the default) structs that don't derive Copy wouldn't have the vec support code.

If you still don't think this is worth it feel free to close this PR, I'll be using my fork for the time being since I needed this functionality for a project I'm working on.

Cheers

[chinedufn]

If you still don't think this is worth it feel free to close this PR, I'll be using my fork for the time being since I needed this functionality for a project I'm working on.

I don't think that a feature flag should land but I'll still keep it open since once class based structs are solved your code will have us most of the way there.

My apologies if this is at all frustrating for you.
Thanks for your work and I'm very glad to hear that you have a solution in the meantime!

[rkreutz]

No problem at all, will leave it open then. Cheers

[wooden-worm]

I really need this feature, how far are we from landing class based structs and unblock this @chinedufn?