Skip to content

Commit

Permalink
Fix non-deterministic generation order (#230)
Browse files Browse the repository at this point in the history
Related to #225.

Before this commit, the integration tests sometimes fail, like so:

```
field has incomplete type 'struct swift_bridge$tuple$I32ResultTestOpaqueRustTypeString'
```

This PR fixes the issue.
  • Loading branch information
NiwakaDev authored Jun 20, 2023
1 parent ffe20d8 commit 4fbd30f
Show file tree
Hide file tree
Showing 15 changed files with 169 additions and 49 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -19,3 +19,7 @@ func extern_swift_struct_rename_3(arg: StructRename3) -> StructRename3 {
arg
}

func swift_reflect_already_declared_struct(arg: AlreadyDeclaredStructTest) -> AlreadyDeclaredStructTest {
arg
}

Original file line number Diff line number Diff line change
Expand Up @@ -24,16 +24,16 @@ class SharedEnumAttributeTests: XCTestCase {
/// Verify that we can call a rust function from swift that uses a type that was already declared in a different bridge module.
func testSharedEnumAlreadyDeclared() throws {
XCTAssertEqual(
reflect_already_declared_enum(
rust_reflect_already_declared_enum(
AlreadyDeclaredEnumTest.Variant
),
AlreadyDeclaredEnumTest.Variant
)
}

/// Verify that we can call a swift function from rust that uses a type that was already declared in a different bridge module.
func testSharedEnumAlreadyDeclared() throws {
test_rust_calls_swift_already_declared()
func testRustCallsSwiftFunctionSharedEnumAlreadyDeclared() throws {
test_rust_calls_swift_already_declared_enum()
}

/// Verify that we can use the generated Debug impl.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,14 @@ class SharedStructAttributeTests: XCTestCase {
let val = AlreadyDeclaredStructTest(field: 123)

XCTAssertEqual(
reflect_already_declared_struct(val).field,
rust_reflect_already_declared_struct(val).field,
123
)
}

/// Verify that we can call a swift function from rust that uses a type that was already declared in a different bridge module.
func testSharedStructAlreadyDeclared() throws {
test_rust_calls_swift_already_declared_struct()
}
}

10 changes: 8 additions & 2 deletions crates/swift-bridge-ir/src/bridged_type.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,12 @@ mod built_in_tuple;
mod shared_enum;
pub(crate) mod shared_struct;

/// Used to declare structures in a C header file.
pub(crate) struct CFfiStruct {
pub c_ffi_type: String,
pub fields: Vec<CFfiStruct>,
}

/// Used for types that have only one possible Rust form and Swift form,
/// such as `()`, `struct UnitStruct;` and `enum SingleVariantEnum { Variant }`.
pub(crate) struct OnlyEncoding {
Expand Down Expand Up @@ -102,7 +108,7 @@ pub(crate) trait BridgeableType: Debug {
/// Some(vec![typedef enum __swift_bridge__$ResultVoidAndTransparentEnum$Tag { //... };])
/// // ...
/// Some(vec![typedef struct __swift_bridge__$ResultVoidAndTransparentEnum { //... };])
fn generate_custom_c_ffi_types(&self, types: &TypeDeclarations) -> Option<Vec<String>>;
fn generate_custom_c_ffi_types(&self, types: &TypeDeclarations) -> Option<CFfiStruct>;

/// Get the Rust representation of this type.
/// For a string this might be `std::string::String`.
Expand Down Expand Up @@ -510,7 +516,7 @@ impl BridgeableType for BridgedType {
}
}

fn generate_custom_c_ffi_types(&self, types: &TypeDeclarations) -> Option<Vec<String>> {
fn generate_custom_c_ffi_types(&self, types: &TypeDeclarations) -> Option<CFfiStruct> {
match self {
BridgedType::StdLib(ty) => match ty {
StdLibType::Result(ty) => ty.generate_custom_c_ffi_types(types),
Expand Down
4 changes: 2 additions & 2 deletions crates/swift-bridge-ir/src/bridged_type/bridgeable_pointer.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use crate::bridged_type::{
BridgeableType, BridgedType, BuiltInResult, TypePosition, UnusedOptionNoneValue,
BridgeableType, BridgedType, BuiltInResult, CFfiStruct, TypePosition, UnusedOptionNoneValue,
};
use crate::parse::TypeDeclarations;
use crate::Path;
Expand Down Expand Up @@ -53,7 +53,7 @@ impl BridgeableType for BuiltInPointer {
None
}

fn generate_custom_c_ffi_types(&self, _types: &TypeDeclarations) -> Option<Vec<String>> {
fn generate_custom_c_ffi_types(&self, _types: &TypeDeclarations) -> Option<CFfiStruct> {
None
}

Expand Down
27 changes: 13 additions & 14 deletions crates/swift-bridge-ir/src/bridged_type/bridgeable_result.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use crate::bridged_type::{BridgeableType, BridgedType, TypePosition};
use crate::bridged_type::{BridgeableType, BridgedType, CFfiStruct, TypePosition};
use crate::{TypeDeclarations, SWIFT_BRIDGE_PREFIX};
use proc_macro2::{Span, TokenStream};
use quote::{format_ident, quote, quote_spanned};
Expand Down Expand Up @@ -381,7 +381,7 @@ impl BuiltInResult {
return Some(custom_rust_ffi_types);
}

pub fn generate_custom_c_ffi_types(&self, types: &TypeDeclarations) -> Option<Vec<String>> {
pub fn generate_custom_c_ffi_types(&self, types: &TypeDeclarations) -> Option<CFfiStruct> {
if !self.is_custom_result_type() {
return None;
}
Expand All @@ -405,8 +405,7 @@ impl BuiltInResult {
let err_c_field_name = self.err_ty.to_c_type(types);
let ok_c_tag_name = self.c_ok_tag_name(types);
let err_c_tag_name = self.c_err_tag_name(types);
let mut custom_c_ffi_types = vec![];
custom_c_ffi_types.push(format!(
let c_ffi_type = format!(
"typedef enum {c_tag_name} {{{ok_c_tag_name}, {err_c_tag_name}}} {c_tag_name};
union {c_fields_name} {{{ok_c_field_name}{err_c_field_name} err;}};
typedef struct {c_enum_name}{{{c_tag_name} tag; union {c_fields_name} payload;}} {c_enum_name};",
Expand All @@ -417,18 +416,18 @@ typedef struct {c_enum_name}{{{c_tag_name} tag; union {c_fields_name} payload;}}
err_c_field_name = err_c_field_name,
ok_c_tag_name = ok_c_tag_name,
err_c_tag_name = err_c_tag_name,
));
if let Some(ok_custom_c_ffi_types) = self.ok_ty.generate_custom_c_ffi_types(types) {
for ok_custom_c_ffi_type in ok_custom_c_ffi_types {
custom_c_ffi_types.push(ok_custom_c_ffi_type);
}
);
let mut custom_c_ffi_type = CFfiStruct {
c_ffi_type,
fields: Vec::with_capacity(2),
};
if let Some(ok_custom_c_ffi_type) = self.ok_ty.generate_custom_c_ffi_types(types) {
custom_c_ffi_type.fields.push(ok_custom_c_ffi_type);
}
if let Some(err_custom_c_ffi_types) = self.err_ty.generate_custom_c_ffi_types(types) {
for err_custom_c_ffi_type in err_custom_c_ffi_types {
custom_c_ffi_types.push(err_custom_c_ffi_type);
}
if let Some(err_custom_c_ffi_type) = self.err_ty.generate_custom_c_ffi_types(types) {
custom_c_ffi_type.fields.push(err_custom_c_ffi_type);
}
return Some(custom_c_ffi_types);
return Some(custom_c_ffi_type);
}

fn is_custom_result_type(&self) -> bool {
Expand Down
6 changes: 4 additions & 2 deletions crates/swift-bridge-ir/src/bridged_type/bridgeable_string.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
use crate::bridged_type::{BridgeableType, OnlyEncoding, TypePosition, UnusedOptionNoneValue};
use crate::bridged_type::{
BridgeableType, CFfiStruct, OnlyEncoding, TypePosition, UnusedOptionNoneValue,
};
use crate::TypeDeclarations;
use proc_macro2::{Span, TokenStream};
use quote::{quote, quote_spanned, ToTokens};
Expand Down Expand Up @@ -36,7 +38,7 @@ impl BridgeableType for BridgedString {
None
}

fn generate_custom_c_ffi_types(&self, _types: &TypeDeclarations) -> Option<Vec<String>> {
fn generate_custom_c_ffi_types(&self, _types: &TypeDeclarations) -> Option<CFfiStruct> {
None
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
use crate::bridged_type::{BridgeableType, OnlyEncoding, TypePosition, UnusedOptionNoneValue};
use crate::bridged_type::{
BridgeableType, CFfiStruct, OnlyEncoding, TypePosition, UnusedOptionNoneValue,
};
use crate::parse::{HostLang, OpaqueRustTypeGenerics};
use crate::{TypeDeclarations, SWIFT_BRIDGE_PREFIX};
use proc_macro2::{Ident, Span, TokenStream};
Expand Down Expand Up @@ -46,7 +48,7 @@ impl BridgeableType for OpaqueForeignType {
None
}

fn generate_custom_c_ffi_types(&self, _types: &TypeDeclarations) -> Option<Vec<String>> {
fn generate_custom_c_ffi_types(&self, _types: &TypeDeclarations) -> Option<CFfiStruct> {
None
}

Expand Down
15 changes: 8 additions & 7 deletions crates/swift-bridge-ir/src/bridged_type/built_in_tuple.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,7 @@
use crate::bridged_type::shared_struct::UnnamedStructFields;
use crate::bridged_type::BridgeableType;
use crate::bridged_type::BuiltInResult;
use crate::bridged_type::OnlyEncoding;
use crate::bridged_type::TypePosition;
use crate::bridged_type::UnusedOptionNoneValue;
use crate::bridged_type::{
BridgeableType, BuiltInResult, CFfiStruct, OnlyEncoding, TypePosition, UnusedOptionNoneValue,
};
use crate::parse::TypeDeclarations;
use crate::SWIFT_BRIDGE_PREFIX;
use std::fmt::Debug;
Expand Down Expand Up @@ -87,13 +85,16 @@ impl BridgeableType for BuiltInTuple {
}])
}

fn generate_custom_c_ffi_types(&self, types: &TypeDeclarations) -> Option<Vec<String>> {
fn generate_custom_c_ffi_types(&self, types: &TypeDeclarations) -> Option<CFfiStruct> {
let combined_types = self.0.combine_field_types_into_ffi_name_string(types);
let fields: Vec<String> = self.0.combine_field_types_into_c_type(types);
let fields = fields.join("; ");
let fields = fields + ";";
let c_decl = format!("typedef struct __swift_bridge__$tuple${combined_types} {{ {fields} }} __swift_bridge__$tuple${combined_types};");
Some(vec![c_decl])
Some(CFfiStruct {
c_ffi_type: c_decl,
fields: vec![],
})
}

fn to_rust_type_path(&self, types: &TypeDeclarations) -> TokenStream {
Expand Down
1 change: 1 addition & 0 deletions crates/swift-bridge-ir/src/codegen/codegen_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ mod argument_label_codegen_tests;
mod async_function_codegen_tests;
mod boxed_fnonce_codegen_tests;
mod built_in_tuple_codegen_tests;
mod c_header_declaration_order_codegen_tests;
mod conditional_compilation_codegen_tests;
mod derive_attribute_codegen_tests;
mod derive_struct_attribute_codegen_tests;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
use super::{CodegenTest, ExpectedCHeader, ExpectedRustTokens, ExpectedSwiftCode};
use proc_macro2::TokenStream;
use quote::quote;

/// Verify that the type that if there is a `Result<Tuple, _>` the generated C header contains
/// the tuple's fields followed by the tuple's FFI representation followed by the Result FFI repr.
mod tuple_fields_generated_before_tuple_generated_before_result {
use super::*;

fn bridge_module_tokens() -> TokenStream {
quote! {
mod ffi {
extern "Rust" {
type ResultTestOpaqueRustType;
}

extern "Rust" {
fn rust_func_return_result_tuple_transparent_enum(
succeed: bool,
) -> Result<(i32, ResultTestOpaqueRustType, String), i32>;
}
}
}
}

fn expected_rust_tokens() -> ExpectedRustTokens {
ExpectedRustTokens::Contains(quote! {})
}

fn expected_swift_code() -> ExpectedSwiftCode {
ExpectedSwiftCode::ContainsAfterTrim(r#""#)
}

fn expected_c_header() -> ExpectedCHeader {
ExpectedCHeader::ContainsAfterTrim(
r#"
struct __swift_bridge__$ResultTupleI32ResultTestOpaqueRustTypeStringAndI32 __swift_bridge__$rust_func_return_result_tuple_transparent_enum(bool succeed);
typedef struct __swift_bridge__$tuple$I32ResultTestOpaqueRustTypeString { int32_t _0; void* _1; void* _2; } __swift_bridge__$tuple$I32ResultTestOpaqueRustTypeString;
typedef enum __swift_bridge__$ResultTupleI32ResultTestOpaqueRustTypeStringAndI32$Tag {__swift_bridge__$ResultTupleI32ResultTestOpaqueRustTypeStringAndI32$ResultOk, __swift_bridge__$ResultTupleI32ResultTestOpaqueRustTypeStringAndI32$ResultErr} __swift_bridge__$ResultTupleI32ResultTestOpaqueRustTypeStringAndI32$Tag;
union __swift_bridge__$ResultTupleI32ResultTestOpaqueRustTypeStringAndI32$Fields {struct __swift_bridge__$tuple$I32ResultTestOpaqueRustTypeString ok; int32_t err;};
typedef struct __swift_bridge__$ResultTupleI32ResultTestOpaqueRustTypeStringAndI32{__swift_bridge__$ResultTupleI32ResultTestOpaqueRustTypeStringAndI32$Tag tag; union __swift_bridge__$ResultTupleI32ResultTestOpaqueRustTypeStringAndI32$Fields payload;} __swift_bridge__$ResultTupleI32ResultTestOpaqueRustTypeStringAndI32;
"#,
)
}

#[test]
fn tuple_fields_generated_before_tuple_generated_before_result() {
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();
}
}
59 changes: 46 additions & 13 deletions crates/swift-bridge-ir/src/codegen/generate_c_header.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
//! Tests can be found in src/codegen/codegen_tests.rs and its submodules.
use crate::bridged_type::shared_struct::StructField;
use crate::bridged_type::{BridgeableType, BridgedType, StdLibType, StructFields};
use crate::bridged_type::{BridgeableType, BridgedType, CFfiStruct, StdLibType, StructFields};
use crate::codegen::CodegenConfig;
use crate::parse::{SharedTypeDeclaration, TypeDeclaration, TypeDeclarations};
use crate::parsed_extern_fn::ParsedExternFn;
Expand All @@ -16,6 +16,18 @@ struct Bookkeeping {
slice_types: HashSet<String>,
}

/// Used to manage the structures declaration order in a C header file. In the C header file, it is necessary to declare fields of a structure before declaring the structure itself.
///
/// For example, the declaration order of struct BarStruct {int32_t value;} and struct FooStruct {struct BarStruct bar_struct;} should be as follows:
/// // C-header file
/// struct BarStruct {int32_t value;};
/// struct FooStruct {struct BarStruct bar_struct;};
///
struct CFfiStructDeclarationBookkeeping {
encountered_custom_type_declarations: HashSet<String>,
custom_type_declarations: Vec<String>,
}

impl SwiftBridgeModule {
/// Generate the contents of a C header file based on the contents of this module.
pub(crate) fn generate_c_header(&self, config: &CodegenConfig) -> String {
Expand Down Expand Up @@ -327,9 +339,13 @@ typedef struct {option_ffi_name} {{ bool is_some; {ffi_name} val; }} {option_ffi
}
}
}
let mut custom_type_declarations: HashSet<String> = HashSet::new();
let mut c_ffi_struct_bookkeeping = CFfiStructDeclarationBookkeeping {
encountered_custom_type_declarations: HashSet::new(),
custom_type_declarations: Vec::new(),
};

for func in self.functions.iter() {
declare_custom_c_ffi_types(func, &self.types, &mut custom_type_declarations);
declare_custom_c_ffi_types(func, &self.types, &mut c_ffi_struct_bookkeeping);
if func.host_lang.is_swift() {
for (idx, boxed_fn) in func.args_filtered_to_boxed_fns(&self.types) {
if boxed_fn.params.is_empty() && boxed_fn.ret.is_null() {
Expand Down Expand Up @@ -364,7 +380,7 @@ typedef struct {option_ffi_name} {{ bool is_some; {ffi_name} val; }} {option_ffi
include, header
);
}
for custom_type_declaration in custom_type_declarations {
for custom_type_declaration in c_ffi_struct_bookkeeping.custom_type_declarations {
header += &custom_type_declaration;
header += "\n";
}
Expand Down Expand Up @@ -404,17 +420,36 @@ void* __swift_bridge__$Vec_{enum_name}$as_ptr(void* vec_ptr);
)
}

fn push_custom_type_declarations(
custom_type_declaration: &CFfiStruct,
c_ffi_struct_bookkeeping: &mut CFfiStructDeclarationBookkeeping,
) {
if c_ffi_struct_bookkeeping
.encountered_custom_type_declarations
.contains(&custom_type_declaration.c_ffi_type)
{
return;
}
c_ffi_struct_bookkeeping
.encountered_custom_type_declarations
.insert(custom_type_declaration.c_ffi_type.clone());
for field in &custom_type_declaration.fields {
push_custom_type_declarations(&field, c_ffi_struct_bookkeeping);
}
c_ffi_struct_bookkeeping
.custom_type_declarations
.push(custom_type_declaration.c_ffi_type.clone());
}

fn declare_custom_c_ffi_types(
func: &ParsedExternFn,
types: &TypeDeclarations,
custom_type_declarations: &mut HashSet<String>,
c_ffi_struct_bookkeeping: &mut CFfiStructDeclarationBookkeeping,
) {
if let ReturnType::Type(_, ty) = &func.func.sig.output {
if let Some(ty) = BridgedType::new_with_type(&ty, types) {
if let Some(declarations) = &ty.generate_custom_c_ffi_types(types) {
for declaration in declarations {
custom_type_declarations.insert(declaration.clone());
}
if let Some(declaration) = ty.generate_custom_c_ffi_types(types) {
push_custom_type_declarations(&declaration, c_ffi_struct_bookkeeping);
}
}
}
Expand All @@ -423,10 +458,8 @@ fn declare_custom_c_ffi_types(
FnArg::Receiver(_receiver) => {}
FnArg::Typed(pat_ty) => {
let ty = BridgedType::new_with_type(&pat_ty.ty, types).unwrap();
if let Some(declarations) = ty.generate_custom_c_ffi_types(types) {
for declaration in declarations {
custom_type_declarations.insert(declaration.clone());
}
if let Some(declaration) = ty.generate_custom_c_ffi_types(types) {
push_custom_type_declarations(&declaration, c_ffi_struct_bookkeeping);
}
}
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,5 +40,6 @@ note: required by a bound in `assert_copy`
| ^^^^ required by this bound in `assert_copy`
help: consider annotating `DoesNotImplementCopy` with `#[derive(Copy)]`
|
15| #[derive(Copy)]
15+ #[derive(Copy)]
16| pub struct DoesNotImplementCopy(u8);
|
Loading

0 comments on commit 4fbd30f

Please sign in to comment.