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

Fix `already_declared` attribute for enums and structs #226

Closed conectado closed 1 year ago

conectado commented 1 year ago

Hi! I encounted an issue while working with the crate while using already_declared:

When generating the rust function wrappers for an extern swift function with an already_declared struct/enum the generated code used a locally defined struct/enum with this patch we add super to that function signature.

An example of code that was previously not working:

#[swift_bridge::bridge]
mod ffi_i {
  enum Foo {
    Bar.
  }
}

use ffi_i::Foo;

#[swift_bridge::bridge]
mod ffi {
  #[swift_bridge(already_declared)]
  enum Foo {}

  extern "Rust" {}
  extern "Swift" {
    fn baz(a: Foo);
  }
}

Previously the genrated function would be something like:

mod ffi {
  fn baz(a: Foo) {
    // ...
  }
}

now it's

mod ffi {
  fn baz(a: super::Foo) {
    // ...
  }
}

I'm not sure if it fixes all edge cases but it's working for me locally.

Let me know if I there's something to change to get this merged or if I was just not using already_declared correctly 😅

conectado commented 1 year ago

Thanks for your first contribution!

Just need to add tests and then we can land this.

Can add an extern "Swift" function in here

https://github.com/chinedufn/swift-bridge/blob/72e1759e0504e3a5a32587605c98e410f12ea9e7/crates/swift-integration-tests/src/enum_attributes/already_declared.rs#L13-L21

And in here

https://github.com/chinedufn/swift-bridge/blob/72e1759e0504e3a5a32587605c98e410f12ea9e7/crates/swift-integration-tests/src/struct_attributes/already_declared.rs#L14-L24

And then you can add an extern "Swift" function to this codegen test

https://github.com/chinedufn/swift-bridge/blob/2bffcb08c1f0ca8bbcba9c78c5ae28a9cddfb632/crates/swift-bridge-ir/src/codegen/codegen_tests/already_declared_attribute_codegen_tests.rs#L149-L177

and this one

https://github.com/chinedufn/swift-bridge/blob/2bffcb08c1f0ca8bbcba9c78c5ae28a9cddfb632/crates/swift-bridge-ir/src/codegen/codegen_tests/already_declared_attribute_codegen_tests.rs#L102-L123

In both of the tests you can use the ExpectedRustTokens::ContainsManyAndDoesNotContainMany to confirm that we generate code for the extern Swift function that uses super::TypeName

Thanks for the review and detailed guidance!

Added the tests :)

chinedufn commented 1 year ago

Yeah CI is currently failing due to https://github.com/chinedufn/swift-bridge/pull/225#issuecomment-1552361044

Test looks good, I'll land this. Thanks!