chinedufn / swift-bridge

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

Make nested struct declarations order independent #170

Open bes opened 1 year ago

bes commented 1 year ago

This code

#[swift_bridge::bridge]
mod ffi {
    #[swift_bridge(swift_repr = "struct")]
    pub struct TopLevel {
        pub nested: Option<Nested>,
    }

    #[swift_bridge(swift_repr = "struct")]
    pub struct Nested {
        x: f64,
    }
}

Won't compile in XCode since the generated C header files output the C structs in the wrong order.

swift-bridge outputs the structs in the order they are found inside a #[swift_bridge::bridge] block, but C requires Nested to always be defined before TopLevel.

Code output:

typedef struct __swift_bridge__$TopLevel { struct __swift_bridge__$Option$Nested nested; } __swift_bridge__$TopLevel;
typedef struct __swift_bridge__$Nested { double x; } __swift_bridge__$Nested;
typedef struct __swift_bridge__$Option$Nested { bool x; __swift_bridge__$Nested val; } __swift_bridge__$Option$Nested;

Error message:

Field has incomplete type 'struct __swift_bridge__$Option$Nested'

I guess it either needs to be documented, or swift-bridge would need to sort the structs before generating C headers.

chinedufn commented 1 year ago

Ah, thanks for opening this issue.

We want bridge modules to be type order independent.

Here's a step-by-step guide for anyone that might want to work on this issue:

Integration test

We can add a mod order_indepedent_declarations that can be used to test scenarios like the one in the PR body.

https://github.com/chinedufn/swift-bridge/blob/3c0d00da33fdf19e201eb121c7e6e370897f8070/crates/swift-integration-tests/src/lib.rs#L4-L18

With contents along the lines of:

/// Verify that we can declare a struct that has a field that uses another struct
/// that gets declared later.
#[swift_bridge::bridge]
mod nested_struct {
    #[swift_bridge(swift_repr = "struct")]
    struct GrandparentStruct {
        inner: ParentStruct
    }

    #[swift_bridge(swift_repr = "struct")]
    struct ParentStruct {
        inner: ChildStruct
    }

    #[swift_bridge(swift_repr = "struct")]
    struct ChildStruct;
}

// ... In the future we can add more cases, such as for nested enums ...

Codegen test

We can add a mod order_indepedent_declarations_codegen_tests to our codegen tests

https://github.com/chinedufn/swift-bridge/blob/3c0d00da33fdf19e201eb121c7e6e370897f8070/crates/swift-bridge-ir/src/codegen/codegen_tests.rs#L40-L45

Here's an example of a codegen test:

https://github.com/chinedufn/swift-bridge/blob/c1fd7177b5d3a8918e34b6bc96ae29437cfc1487/crates/swift-bridge-ir/src/codegen/codegen_tests/transparent_struct_codegen_tests.rs#L517-L583

We can make a mod struct_order_independent and use ExpectedRustTokens::SkipTest and ExpectedSwiftTokens::SkipTest.

Then in the ExpectedCTokens we can confirm that the structs are defined in the correct order (ChildStruct gets defined before ParentStruct which gets defined before GrandparentStruct).

Passing the test

We can modify the C header code generation to emit types in the right order.

https://github.com/chinedufn/swift-bridge/blob/07a15d78782dc371a7d24301ac9625124e42f8d8/crates/swift-bridge-ir/src/codegen/generate_c_header.rs#L30-L288

One simple way to do it would be to have something like:

let mut all_types = Vec::with_capacity(self.types.types().len());

for ty in self.types.types() {
    self.push_type(ty, &mut all_types);
}
// Pseudocode
fn push_type(&self, ty: &TypeDeclaration, all_types: &mut Vec<&TypeDeclaration>) {
    if (ty is a struct) {
        for field in ty.struct_fields {
            self.push_type(&field.ty, all_types);
        }
    }

    all_types.push(ty);
}

Then we could iterate over all_types instead of self.types.types().

Note that this will probably cause a stack overflow if you have a module like:

#[swift_bridge::bridge]
mod ffi {
    struct CyclicDependencyA {
        cycle: CyclicDependencyB
    }

    struct CyclicDependencyB {
        cycle: CyclicDependencyA
    }
}

So we should add a UI test case for this case which confirms that we get a Rust compile time error, not a stack overflow.

This can be cyclic-dependency.rs and cyclic-dependency.stderr.

Here's an example of a UI test https://github.com/chinedufn/swift-bridge/blob/e5a0c893897f9aa2dddfdf1775876ff56b29d97c/crates/swift-bridge-macro/tests/ui/invalid-module-item.rs#L1-L10

https://github.com/chinedufn/swift-bridge/blob/e5a0c893897f9aa2dddfdf1775876ff56b29d97c/crates/swift-bridge-macro/tests/ui/invalid-module-item.stderr#L1-L11

One way to solve the cyclic dependency issue is to create a a let mut types_to_process: HashMap<String, &TypeDeclaration> in the generate_c_header implementation above. where the String is the type's name.

Then as we process types in the recursive push_type function we can remove them from the types_to_process. We keep processing until there aren't any left.

Goodbyefrog commented 1 year ago

After taking some time to review the rust programming language and brushing up on the swift-bridge handbook a bit . A possible solution, I thought about was rearranging the order of the struct definitions in the ffi module and also using forward declarations to inform the compiler about the existence of the struct before it is fully defined .

`#[swift_bridge::bridge] mod ffi {

[swift_bridge(swift_repr = "struct")]

#[repr(C)]
pub struct Nested {
    x: f64,
}

#[swift_bridge(swift_repr = "struct")]
#[repr(C)]
pub struct TopLevel {
pub nested: Option<*mut Nested>,
}

}`

In my updated code, the nested struct is now defined before it is referenced.

The *mut Nested type represents a mutable raw pointer to Nested. By using a pointer, it can defer the complete definition of Nested until later in the code when necessary

By utilizing forward declarations, you can rearrange the order of the struct definitions. With a little research on interaction with the C language, I discovered that the

[repr(C)] attribute can be used to ensure that the structs are represented in a C-compatible layout.

Again I am very new to the rust programming language, so this is my attempt at solving a bug as a complete beginner. Any advice or tips would be very appreciated and would help me grow.

chinedufn commented 1 year ago

Thanks for looking into this!

A possible solution, I thought about was rearranging the order of the struct definitions in the ffi module

The swift_bridge::bridge annotation on top of the module means that our swift-bridge-macro crate (found in crates/swift-bridge-macro procedural macro runs on all of the tokens in the module and then emits new tokens to replace them.

Importantly, this means that we can interpret the module however we like and replace it with new code in whatever way we like. We could replace the entire module with const HELLO: &str = "world" if we felt like it.

By utilizing forward declarations, you can rearrange the order of the struct definitions.

Because our crates/swift-bridge-macro can emit whatever code it likes, we could easily re-arrange the structs in any order that we want.

Our procedural macro does not need to do this since forward declarations are unnecessary in Rust.

However, the generated C header (generated by crates/swift-bridge-build) that we emit needs to emit the Nested struct before the TopLevel struct.

See https://github.com/chinedufn/swift-bridge/pull/230 as an example of doing something similar

You'll also want to take a look at https://github.com/chinedufn/swift-bridge/issues/170#issuecomment-1431588845


Again I am very new to the rust programming language, so this is my attempt at solving a bug as a complete beginner. Any advice or tips would be very appreciated and would help me grow.

New Rustaceans are welcome here!

Some Rust concepts that you might want to look into in order to better understand swift-bridge:

NiwakaDev commented 1 year ago

@Goodbyefrog

I think you can start by addressing #168 or this issue as a new contributor.