Fix `already_declared` attribute for enums and structs #226

conectado · 2023-05-11T22:25:40Z

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 😅

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.

chinedufn

Thanks for your first contribution!

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

Can add an extern "Swift" function in here

swift-bridge/crates/swift-integration-tests/src/enum_attributes/already_declared.rs

Lines 13 to 21 in 72e1759

    
           #[swift_bridge::bridge] 
        
           mod ffi2 { 
        
               #[swift_bridge(already_declared)] 
        
               enum AlreadyDeclaredEnumTest {} 
        
               extern "Rust" { 
        
                   fn reflect_already_declared_enum(arg: AlreadyDeclaredEnumTest) -> AlreadyDeclaredEnumTest; 
        
               } 
        
           }

And in here

swift-bridge/crates/swift-integration-tests/src/struct_attributes/already_declared.rs

Lines 14 to 24 in 72e1759

    
           #[swift_bridge::bridge] 
        
           mod ffi2 { 
        
               #[swift_bridge(already_declared, swift_repr = "struct")] 
        
               struct AlreadyDeclaredStructTest; 
        
               extern "Rust" { 
        
                   fn reflect_already_declared_struct( 
        
                       arg: AlreadyDeclaredStructTest, 
        
                   ) -> AlreadyDeclaredStructTest; 
        
               } 
        
           }

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

swift-bridge/crates/swift-bridge-ir/src/codegen/codegen_tests/already_declared_attribute_codegen_tests.rs

Lines 149 to 177 in 2bffcb0

    
           /// Verify that we do not re-declare an already defined enum. 
        
           mod already_declared_enum { 
        
               use super::*; 
        
               fn bridge_module_tokens() -> TokenStream { 
        
                   quote! { 
        
                       mod ffi { 
        
                           #[swift_bridge(already_declared)] 
        
                           enum FfiSomeEnum {} 
        
                           extern "Rust" { 
        
                               fn some_function(arg: FfiSomeEnum) -> FfiSomeEnum; 
        
                           } 
        
                       } 
        
                   } 
        
               } 
        
               fn expected_rust_tokens() -> ExpectedRustTokens { 
        
                   ExpectedRustTokens::ContainsManyAndDoesNotContainMany { 
        
                       contains: vec![quote! { 
        
                           pub extern "C" fn __swift_bridge__some_function(arg: <super::FfiSomeEnum as swift_bridge::SharedEnum>::FfiRepr) -> <super::FfiSomeEnum as swift_bridge::SharedEnum>::FfiRepr { 
        
                               super::some_function(arg.into_rust_repr()).into_ffi_repr() 
        
                           } 
        
                       }], 
        
                       does_not_contain: vec![quote! { 
        
                           enum FfiSomeEnum 
        
                       }], 
        
                   } 
        
               }

and this one

swift-bridge/crates/swift-bridge-ir/src/codegen/codegen_tests/already_declared_attribute_codegen_tests.rs

Lines 102 to 123 in 2bffcb0

    
           /// Verify that we do not re-declare an already defined struct. 
        
           mod already_declared_struct { 
        
               use super::*; 
        
               fn bridge_module_tokens() -> TokenStream { 
        
                   quote! { 
        
                       mod ffi { 
        
                           #[swift_bridge(already_declared, swift_repr = "struct")] 
        
                           struct FfiSomeType; 
        
                           extern "Rust" { 
        
                               fn some_function(arg: FfiSomeType) -> FfiSomeType; 
        
                           } 
        
                       } 
        
                   } 
        
               } 
        
               fn expected_rust_tokens() -> ExpectedRustTokens { 
        
                   ExpectedRustTokens::DoesNotContain(quote! { 
        
                       struct FfiSomeType 
        
                   }) 
        
               }

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

conectado · 2023-05-16T20:33:12Z

Thanks for your first contribution!

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

Can add an extern "Swift" function in here

swift-bridge/crates/swift-integration-tests/src/enum_attributes/already_declared.rs

Lines 13 to 21 in 72e1759

#[swift_bridge::bridge]

mod ffi2 {

#[swift_bridge(already_declared)]

enum AlreadyDeclaredEnumTest {}

extern "Rust" {

fn reflect_already_declared_enum(arg: AlreadyDeclaredEnumTest) -> AlreadyDeclaredEnumTest;

}

}

And in here

swift-bridge/crates/swift-integration-tests/src/struct_attributes/already_declared.rs

Lines 14 to 24 in 72e1759

#[swift_bridge::bridge]

mod ffi2 {

#[swift_bridge(already_declared, swift_repr = "struct")]

struct AlreadyDeclaredStructTest;

extern "Rust" {

fn reflect_already_declared_struct(

arg: AlreadyDeclaredStructTest,

) -> AlreadyDeclaredStructTest;

}

}

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

swift-bridge/crates/swift-bridge-ir/src/codegen/codegen_tests/already_declared_attribute_codegen_tests.rs

Lines 149 to 177 in 2bffcb0

/// Verify that we do not re-declare an already defined enum.

mod already_declared_enum {

use super::*;

fn bridge_module_tokens() -> TokenStream {

quote! {

mod ffi {

#[swift_bridge(already_declared)]

enum FfiSomeEnum {}

extern "Rust" {

fn some_function(arg: FfiSomeEnum) -> FfiSomeEnum;

}

}

}

}

fn expected_rust_tokens() -> ExpectedRustTokens {

ExpectedRustTokens::ContainsManyAndDoesNotContainMany {

contains: vec![quote! {

pub extern "C" fn __swift_bridge__some_function(arg: <super::FfiSomeEnum as swift_bridge::SharedEnum>::FfiRepr) -> <super::FfiSomeEnum as swift_bridge::SharedEnum>::FfiRepr {

super::some_function(arg.into_rust_repr()).into_ffi_repr()

}

}],

does_not_contain: vec![quote! {

enum FfiSomeEnum

}],

}

}

and this one

swift-bridge/crates/swift-bridge-ir/src/codegen/codegen_tests/already_declared_attribute_codegen_tests.rs

Lines 102 to 123 in 2bffcb0

/// Verify that we do not re-declare an already defined struct.

mod already_declared_struct {

use super::*;

fn bridge_module_tokens() -> TokenStream {

quote! {

mod ffi {

#[swift_bridge(already_declared, swift_repr = "struct")]

struct FfiSomeType;

extern "Rust" {

fn some_function(arg: FfiSomeType) -> FfiSomeType;

}

}

}

}

fn expected_rust_tokens() -> ExpectedRustTokens {

ExpectedRustTokens::DoesNotContain(quote! {

struct FfiSomeType

})

}

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

Nice!

NiwakaDev · 2023-05-17T10:22:04Z

crates/swift-integration-tests/src/enum_attributes/already_declared.rs

+    extern "Swift" {
+        fn swift_reflect_already_declared_enum(
+            arg: AlreadyDeclaredEnumTest,
+        ) -> AlreadyDeclaredEnumTest;


@chinedufn

I guess that this function isn't called from anywhere. What do you think about this?

Ah yeah we should call it. Great point. Want to tell @conectado how to best call it?

I added a test here that I think does this.

I couldn't run the tests because I think the CI is failing so lmk if this work or I should do something else?

Hi, @conectado
The CI failure is due to #213. If you want to know more detail, please see #225.

I guess it should be fine as long as you run the test multiple times and then the test pass at least once.

Locally it's consistently failing with:

Testing failed:
field has incomplete type 'struct swift_bridge$tuple$I32ResultTestOpaqueRustTypeString'
failed to emit precompiled header '/Users/conectado/Library/Developer/Xcode/DerivedData/SwiftRustIntegrationTestRunner-cocipvlohdpefifldegpkicaiuld/Build/Intermediates.noindex/PrecompiledHeaders/BridgingHeader-swift_4X3E80DLUM90-clang_KV4PBZK8M8GA.pch' for bridging header '/Users/conectado/repos/firezone/swift-bridge/SwiftRustIntegrationTestRunner/Headers/BridgingHeader.h'
Testing cancelled because the build failed

This error is due to #213.

Locally it's consistently failing with:

It is not consistently. Right now, when we run ./test-swift-rust-integration.sh, The tests will sometimes fails. If you check #225, You could understand why the tests sometimes fails.

On the other hand, when we run cargo test --all, the tests will consistently pass.

chinedufn · 2023-05-27T00:24:35Z

Yeah CI is currently failing due to #225 (comment)

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

fix a already_declared attribute for enums and struct

2d77477

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.

chinedufn changed the title ~~fix a already_declared attribute for enums and structs~~ Fix already_declared attribute for enums and structs May 16, 2023

chinedufn reviewed May 16, 2023

View reviewed changes

update tests to reflect fixes to already_declared attribute

cf6607d

conectado requested a review from chinedufn May 16, 2023 20:33

chinedufn approved these changes May 16, 2023

View reviewed changes

NiwakaDev reviewed May 17, 2023

View reviewed changes

add swift from rust integration test for already_declared enums

7415b91

chinedufn merged commit ecd3b97 into chinedufn:master May 27, 2023

NiwakaDev mentioned this pull request Jun 17, 2023

Fix non-deterministic generation order #230

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `already_declared` attribute for enums and structs #226

Fix `already_declared` attribute for enums and structs #226

conectado commented May 11, 2023

chinedufn left a comment

conectado commented May 16, 2023

chinedufn left a comment

NiwakaDev May 17, 2023 •

edited

Loading

chinedufn May 17, 2023

conectado May 25, 2023

NiwakaDev May 25, 2023

conectado May 25, 2023

NiwakaDev May 25, 2023

chinedufn commented May 27, 2023

	#[swift_bridge::bridge]
	mod ffi2 {
	#[swift_bridge(already_declared)]
	enum AlreadyDeclaredEnumTest {}

	extern "Rust" {
	fn reflect_already_declared_enum(arg: AlreadyDeclaredEnumTest) -> AlreadyDeclaredEnumTest;
	}
	}

	#[swift_bridge::bridge]
	mod ffi2 {
	#[swift_bridge(already_declared, swift_repr = "struct")]
	struct AlreadyDeclaredStructTest;

	extern "Rust" {
	fn reflect_already_declared_struct(
	arg: AlreadyDeclaredStructTest,
	) -> AlreadyDeclaredStructTest;
	}
	}

	/// Verify that we do not re-declare an already defined enum.
	mod already_declared_enum {
	use super::*;

	fn bridge_module_tokens() -> TokenStream {
	quote! {
	mod ffi {
	#[swift_bridge(already_declared)]
	enum FfiSomeEnum {}

	extern "Rust" {
	fn some_function(arg: FfiSomeEnum) -> FfiSomeEnum;
	}
	}
	}
	}

	fn expected_rust_tokens() -> ExpectedRustTokens {
	ExpectedRustTokens::ContainsManyAndDoesNotContainMany {
	contains: vec![quote! {
	pub extern "C" fn __swift_bridge__some_function(arg: <super::FfiSomeEnum as swift_bridge::SharedEnum>::FfiRepr) -> <super::FfiSomeEnum as swift_bridge::SharedEnum>::FfiRepr {
	super::some_function(arg.into_rust_repr()).into_ffi_repr()
	}
	}],
	does_not_contain: vec![quote! {
	enum FfiSomeEnum
	}],
	}
	}

	/// Verify that we do not re-declare an already defined struct.
	mod already_declared_struct {
	use super::*;

	fn bridge_module_tokens() -> TokenStream {
	quote! {
	mod ffi {
	#[swift_bridge(already_declared, swift_repr = "struct")]
	struct FfiSomeType;

	extern "Rust" {
	fn some_function(arg: FfiSomeType) -> FfiSomeType;
	}
	}
	}
	}

	fn expected_rust_tokens() -> ExpectedRustTokens {
	ExpectedRustTokens::DoesNotContain(quote! {
	struct FfiSomeType
	})
	}

Fix already_declared attribute for enums and structs #226

Fix already_declared attribute for enums and structs #226

Conversation

conectado commented May 11, 2023

chinedufn left a comment

Choose a reason for hiding this comment

conectado commented May 16, 2023

chinedufn left a comment

Choose a reason for hiding this comment

NiwakaDev May 17, 2023 • edited Loading

Choose a reason for hiding this comment

chinedufn May 17, 2023

Choose a reason for hiding this comment

conectado May 25, 2023

Choose a reason for hiding this comment

NiwakaDev May 25, 2023

Choose a reason for hiding this comment

conectado May 25, 2023

Choose a reason for hiding this comment

NiwakaDev May 25, 2023

Choose a reason for hiding this comment

chinedufn commented May 27, 2023

Fix `already_declared` attribute for enums and structs #226

Fix `already_declared` attribute for enums and structs #226

NiwakaDev May 17, 2023 •

edited

Loading