Adding derive() macro support to SharedStructs #198

rkreutz · 2023-03-16T01:05:13Z

This PR adds support for #[derive(...)] macros to SharedStructs.

Example

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

This will successfully add a derived Copy and Clone trait to the struct declared.

chinedufn

Nice looks great. Left a couple minor comments then we can land.

chinedufn · 2023-03-16T01:19:06Z

crates/swift-bridge-ir/src/codegen/generate_rust_tokens/shared_struct.rs

@@ -90,7 +90,15 @@ impl SwiftBridgeModule {
            }
        };

+        let automatic_derives;


We're moving away from automatic derives, so derives is fine.

chinedufn · 2023-03-16T01:20:23Z

crates/swift-bridge-ir/src/bridged_type/shared_struct.rs

@@ -19,6 +19,7 @@ pub(crate) struct SharedStruct {
    pub fields: StructFields,
    pub swift_name: Option<LitStr>,
    pub already_declared: bool,
+    pub derives: Option<Vec<TokenStream>>,


Instead of Option<Vec<TokenStream>> let's use something like:

pub(crate) struct StructDerives { pub copy: bool, pub clone: bool, }

This would kind of limit us to only these kind of derives, if we want to add new ones (like Debug) we would need to open a PR and add them. It also has the potential of growing quite a lot. I don't see why it would be an issue to have the more generic Vec<TokenStream>?

Thanks for explaining your thinking. Makes sense.

The issue is that we only want to support derives that we know are well behaved.
Supporting arbitrary derives might be possible, but requires more research.

As it stands now, supporting arbitrary derives can lead to undefined behavior.
It's incredibly unlikely, but theoretically possible.

Before we support arbitrary derives we want to make UB completely impossible.

Essentially, #[derive(SomeCustomDerive)] could emit something like type u32 = some_crate::NotAU32 and cause swift-bridge to think that it sees a u32 not realizing that a type alias got generated to make u32 a completely different type.

It should be possible to guard against this, but until we guard against it (and any other potential derive related UB scenarios) we can't allow arbitrary derives.

This discussion has more detail #190 (comment)

makes sense, will go with your suggestion then

chinedufn · 2023-03-16T01:23:20Z

crates/swift-bridge-ir/src/parse/parse_struct.rs

    /// Verify that we properly parse multiple comma separated struct attributes.
    #[test]
    fn parses_multiple_struct_attributes() {
        let tokens = quote! {
            #[swift_bridge::bridge]
            mod ffi {
                #[swift_bridge(swift_name = "FfiFoo", swift_repr = "class")]
+                #[derive(Copy, Clone)]


Can we remove this derive?

I'm actually testing here whether #[swift_bridge] and #[derive] work together here. I think it's a good test to have, so I'll just leave this test as is and duplicate it to check if they work ok together

chinedufn · 2023-03-16T01:23:32Z

crates/swift-bridge-ir/src/parse/parse_struct.rs

+        let tokens = quote! {
+            #[swift_bridge::bridge]
+            mod ffi {
+                #[derive(Copy, Clone)]


chinedufn · 2023-03-16T01:30:31Z

Just missing a codegen test and an integration test (don't need the Swift side .. just a Rust integration test is fine for now and in the future we can emit a clone function on the Swift side...)

See the following files in https://github.com/chinedufn/swift-bridge/pull/194/files for an example that you can copy/paste/modify

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

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

Lines 5 to 68 in b23f7d3

    
           /// Verify that we generate debugDescription in Swift and Debug function in Rust when using #\[derive(Debug)] 
        
           mod derive_debug_enum { 
        
               use super::*; 
        
               fn bridge_module_tokens() -> TokenStream { 
        
                   quote! { 
        
                       #[swift_bridge::bridge] 
        
                       mod ffi { 
        
                           #[derive(Debug)] 
        
                           enum SomeEnum { 
        
                               Variant1 
        
                           } 
        
                       } 
        
                   } 
        
               } 
        
               fn expected_rust_tokens() -> ExpectedRustTokens { 
        
                   ExpectedRustTokens::ContainsMany(vec![ 
        
                       quote! { 
        
                           #[derive(Copy, Clone, ::std::fmt::Debug)] 
        
                           pub enum SomeEnum { 
        
                               Variant1 
        
                           } 
        
                       }, 
        
                       quote! { 
        
                           #[export_name = "__swift_bridge__$SomeEnum$_Debug"] 
        
                           pub extern "C" fn __swift_bridge__SomeEnum_Debug(this: __swift_bridge__SomeEnum) -> *mut swift_bridge::string::RustString { 
        
                               swift_bridge::string::RustString(format!("{:?}", this.into_rust_repr())).box_into_raw() 
        
                           } 
        
                       }, 
        
                   ]) 
        
               } 
        
               fn expected_swift_code() -> ExpectedSwiftCode { 
        
                   ExpectedSwiftCode::ContainsAfterTrim( 
        
                       r#"  
        
           extension SomeEnum: CustomDebugStringConvertible { 
        
               public var debugDescription: String { 
        
                   RustString(ptr: __swift_bridge__$SomeEnum$_Debug(self.intoFfiRepr())).toString() 
        
               } 
        
           } 
        
           "#, 
        
                   ) 
        
               } 
        
               fn expected_c_header() -> ExpectedCHeader { 
        
                   ExpectedCHeader::ContainsAfterTrim( 
        
                       r#"  
        
           void* __swift_bridge__$SomeEnum$_Debug(__swift_bridge__$SomeEnum this); 
        
           "#, 
        
                   ) 
        
               } 
        
               #[test] 
        
               fn generates_enum_to_and_from_ffi_conversions_no_data() { 
        
                   CodegenTest { 
        
                       bridge_module: bridge_module_tokens().into(), 
        
                       expected_rust_tokens: expected_rust_tokens(), 
        
                       expected_swift_code: expected_swift_code(), 
        
                       expected_c_header: expected_c_header(), 
        
                   } 
        
                   .test(); 
        
               } 
        
           }

Can just use ExpectedSwiftCode::SkipTest and ExpectedCHeader::SkipTest for now. In the future we can generate a clone method on the Swift side.

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

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

Lines 1 to 7 in b23f7d3

    
           #[swift_bridge::bridge] 
        
           mod ffi { 
        
               #[derive(Debug)] 
        
               enum DeriveDebugEnum { 
        
                   Variant, 
        
               } 
        
           }

rkreutz · 2023-03-16T02:26:18Z

See the following files in https://github.com/chinedufn/swift-bridge/pull/194/files for an example that you can copy/paste/modify

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

@chinedufn should I wait for that PR to be merged to add the tests on this file you mentioned, or should I create a separate file like derive_struct_attribute_codegen_tests.rs?

chinedufn

Looks great! Thanks!!

chinedufn · 2023-03-16T03:00:01Z

crates/swift-bridge-ir/src/parse/parse_struct.rs

+
+        let ty2 = module.types.types()[1].unwrap_shared_struct();
+
+        assert_eq!(ty2.derives.copy, false);


chinedufn · 2023-03-16T03:00:18Z

crates/swift-integration-tests/src/struct_attributes/derive.rs

@@ -0,0 +1,28 @@
+#[swift_bridge::bridge]


Awesome tests. Might've been nice to actually use the Copy/Clone impls outside of the module down below, but this is fine.

chinedufn · 2023-03-16T03:01:59Z

or should I create a separate file like derive_struct_attribute_codegen_tests.rs

Creating a file in this PR sounds good!

rkreutz · 2023-03-16T03:42:24Z

Creating a file in this PR sounds good!

just updated with the tests

chinedufn · 2023-03-16T04:17:08Z

Looks good. I'll merge once tests are passing (looks like you need to run cargo fmt)

…ft-bridge into feature/shared-struct-derive

chinedufn · 2023-03-16T16:30:37Z

Thank you!

extrawurst · 2024-04-28T12:08:13Z

This would be great to have on enum too

Adding derive() macro support to SharedStructs

9fb3327

rkreutz mentioned this pull request Mar 16, 2023

Adding Vec support for transparent structs #193

Closed

chinedufn reviewed Mar 16, 2023

View reviewed changes

Code review

ead0af9

chinedufn approved these changes Mar 16, 2023

View reviewed changes

Adding integration test

c4c916f

rkreutz and others added 2 commits March 16, 2023 04:08

add missing mod

f7d2b7e

Add doc comments to codegen tests

a789e6b

rkreutz added 2 commits March 16, 2023 04:28

cargo fmt

b1e043f

Merge branch 'feature/shared-struct-derive' of github.com:rkreutz/swi…

7b61648

…ft-bridge into feature/shared-struct-derive

chinedufn merged commit 6b151f1 into chinedufn:master Mar 16, 2023

wooden-worm mentioned this pull request Jan 9, 2024

Add support for #[derive(Debug, serde::Serialize, serde::Deserialize)] to SharedStruct #248

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding derive() macro support to SharedStructs #198

Adding derive() macro support to SharedStructs #198

rkreutz commented Mar 16, 2023

chinedufn left a comment

chinedufn Mar 16, 2023

rkreutz Mar 16, 2023

chinedufn Mar 16, 2023

rkreutz Mar 16, 2023

chinedufn Mar 16, 2023

rkreutz Mar 16, 2023

rkreutz Mar 16, 2023

chinedufn Mar 16, 2023

rkreutz Mar 16, 2023

rkreutz Mar 16, 2023

chinedufn Mar 16, 2023

chinedufn commented Mar 16, 2023 •

edited

Loading

rkreutz commented Mar 16, 2023

chinedufn left a comment

chinedufn Mar 16, 2023

chinedufn Mar 16, 2023

chinedufn commented Mar 16, 2023 •

edited

Loading

rkreutz commented Mar 16, 2023

chinedufn commented Mar 16, 2023

chinedufn commented Mar 16, 2023

extrawurst commented Apr 28, 2024


		let ty2 = module.types.types()[1].unwrap_shared_struct();

		assert_eq!(ty2.derives.copy, false);

Adding derive() macro support to SharedStructs #198

Adding derive() macro support to SharedStructs #198

Conversation

rkreutz commented Mar 16, 2023

Example

chinedufn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chinedufn commented Mar 16, 2023 • edited Loading

rkreutz commented Mar 16, 2023

chinedufn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chinedufn commented Mar 16, 2023 • edited Loading

rkreutz commented Mar 16, 2023

chinedufn commented Mar 16, 2023

chinedufn commented Mar 16, 2023

extrawurst commented Apr 28, 2024

chinedufn commented Mar 16, 2023 •

edited

Loading

chinedufn commented Mar 16, 2023 •

edited

Loading