From bcd17dceac53e10d32415f7fddbd7d59d113802d Mon Sep 17 00:00:00 2001 From: Chinedu Francis Nwafili Date: Thu, 7 Mar 2024 21:07:21 -0500 Subject: [PATCH] Fix `improper_ctypes` warning This commit fixes an `improper_ctypes` warning when bridging transparent structs that contain non FFI safe types. While transparent structs that contained non FFI safe types were being passed in an FFI safe way before this commit, the Rust compiler could not know that what we were doing was FFI safe. This commit uses `#[allow(improper_ctypes)]` to silence the lint. ## Illustration Before this commit the following bridge module: ```rust #[swift_bridge::bridge] mod ffi { struct SharedStruct { field: String } extern "Swift" { fn swift_func() -> SharedStruct; } } ``` would lead to the following warning: ```console warning: `extern` block uses type `RustString`, which is not FFI-safe --> my_file.rs:4:12 | 4 | struct SharedStruct { | ^^^^^^^^^^^^ not FFI-safe | = help: consider adding a `#[repr(C)]` or `#[repr(transparent)]` attribute to this struct = note: this struct has unspecified layout = note: `#[warn(improper_ctypes)]` on by default ``` This warning was a bit misleading in that it made it seem like the `RustString` needed a `#[repr(C)]` annotation. The real issue was that the generated FFI representation type: ```rust #[repr(C)] struct __swift_bridge__SharedStruct { field: *mut std::string::RustString } ``` was triggering a warning because the Rust compiler can't know that the non-FFI safe `std::string::RustString` is not being dereferenced on the Swift side. --- .../SharedStruct.swift | 5 +- .../SharedStructTests.swift | 10 +++ .../function_attribute_codegen_tests.rs | 2 + .../opaque_rust_type_codegen_tests.rs | 1 + .../opaque_swift_type_codegen_tests.rs | 1 + .../codegen_tests/vec_codegen_tests.rs | 4 + .../src/codegen/generate_rust_tokens.rs | 90 +++++++++++++++++-- .../tests/ui/incorrect-return-type.stderr | 3 + .../src/expose_opaque_rust_type.rs | 12 +++ .../src/shared_types/shared_struct.rs | 17 +++- 10 files changed, 135 insertions(+), 10 deletions(-) diff --git a/SwiftRustIntegrationTestRunner/SwiftRustIntegrationTestRunner/SharedStruct.swift b/SwiftRustIntegrationTestRunner/SwiftRustIntegrationTestRunner/SharedStruct.swift index b723925d..36648391 100644 --- a/SwiftRustIntegrationTestRunner/SwiftRustIntegrationTestRunner/SharedStruct.swift +++ b/SwiftRustIntegrationTestRunner/SwiftRustIntegrationTestRunner/SharedStruct.swift @@ -11,7 +11,10 @@ func rust_calls_swift_struct_with_no_fields(arg: StructWithNoFields) -> StructWi arg } -func rust_calls_struct_repr_struct_one_primitive_field(arg: StructReprStructWithOnePrimitiveField) -> StructReprStructWithOnePrimitiveField { +func rust_calls_swift_struct_repr_struct_one_primitive_field(arg: StructReprStructWithOnePrimitiveField) -> StructReprStructWithOnePrimitiveField { arg } +func rust_calls_swift_struct_repr_struct_one_string_field(arg: StructReprStructWithOneStringField) -> StructReprStructWithOneStringField { + arg +} diff --git a/SwiftRustIntegrationTestRunner/SwiftRustIntegrationTestRunnerTests/SharedStructTests.swift b/SwiftRustIntegrationTestRunner/SwiftRustIntegrationTestRunnerTests/SharedStructTests.swift index bbf8a1b4..8d3efd57 100644 --- a/SwiftRustIntegrationTestRunner/SwiftRustIntegrationTestRunnerTests/SharedStructTests.swift +++ b/SwiftRustIntegrationTestRunner/SwiftRustIntegrationTestRunnerTests/SharedStructTests.swift @@ -34,6 +34,16 @@ class SharedStructTests: XCTestCase { XCTAssertEqual(val.named_field, 56) } + /// Verify that we can pass a transparent struct that contains a String back and forth between Rust and Swift. + /// `SharedStruct { field: String }`. + func testStructReprStructWithOneStringField() { + let val = rust_calls_swift_struct_repr_struct_one_string_field( + arg: StructReprStructWithOneStringField(field: "hello world".intoRustString()) + ); + XCTAssertEqual(val.field.toString(), "hello world") + } + + /// Verify that we can create a tuple struct. func testTupleStruct() { let val = StructReprStructTupleStruct(_0: 11, _1: 22) diff --git a/crates/swift-bridge-ir/src/codegen/codegen_tests/function_attribute_codegen_tests.rs b/crates/swift-bridge-ir/src/codegen/codegen_tests/function_attribute_codegen_tests.rs index de8e91e4..eeac35d2 100644 --- a/crates/swift-bridge-ir/src/codegen/codegen_tests/function_attribute_codegen_tests.rs +++ b/crates/swift-bridge-ir/src/codegen/codegen_tests/function_attribute_codegen_tests.rs @@ -429,6 +429,8 @@ mod function_attribute_swift_name_extern_rust { pub fn call_swift_from_rust() -> String { unsafe { Box::from_raw(unsafe {__swift_bridge__call_swift_from_rust () }).0 } } + + #[allow(improper_ctypes)] extern "C" { #[link_name = "__swift_bridge__$call_swift_from_rust"] fn __swift_bridge__call_swift_from_rust() -> * mut swift_bridge::string::RustString; diff --git a/crates/swift-bridge-ir/src/codegen/codegen_tests/opaque_rust_type_codegen_tests.rs b/crates/swift-bridge-ir/src/codegen/codegen_tests/opaque_rust_type_codegen_tests.rs index 63aacb4d..fcb4420c 100644 --- a/crates/swift-bridge-ir/src/codegen/codegen_tests/opaque_rust_type_codegen_tests.rs +++ b/crates/swift-bridge-ir/src/codegen/codegen_tests/opaque_rust_type_codegen_tests.rs @@ -422,6 +422,7 @@ mod extern_swift_freestanding_fn_with_owned_opaque_rust_type_arg { })) as *mut super::MyType ) } } + #[allow(improper_ctypes)] extern "C" { #[link_name = "__swift_bridge__$some_function"] fn __swift_bridge__some_function (arg: *mut super::MyType); diff --git a/crates/swift-bridge-ir/src/codegen/codegen_tests/opaque_swift_type_codegen_tests.rs b/crates/swift-bridge-ir/src/codegen/codegen_tests/opaque_swift_type_codegen_tests.rs index 3ef8d5d7..ffc97fd5 100644 --- a/crates/swift-bridge-ir/src/codegen/codegen_tests/opaque_swift_type_codegen_tests.rs +++ b/crates/swift-bridge-ir/src/codegen/codegen_tests/opaque_swift_type_codegen_tests.rs @@ -32,6 +32,7 @@ mod extern_swift_freestanding_fn_with_owned_opaque_swift_type_arg { } } + #[allow(improper_ctypes)] extern "C" { #[link_name = "__swift_bridge__$some_function"] fn __swift_bridge__some_function (arg: MyType); diff --git a/crates/swift-bridge-ir/src/codegen/codegen_tests/vec_codegen_tests.rs b/crates/swift-bridge-ir/src/codegen/codegen_tests/vec_codegen_tests.rs index 120e68fd..1f715509 100644 --- a/crates/swift-bridge-ir/src/codegen/codegen_tests/vec_codegen_tests.rs +++ b/crates/swift-bridge-ir/src/codegen/codegen_tests/vec_codegen_tests.rs @@ -566,6 +566,8 @@ mod extern_swift_fn_return_vec_of_primitive_rust_type { pub fn some_function() -> Vec { unsafe { *Box::from_raw(unsafe { __swift_bridge__some_function() }) } } + + #[allow(improper_ctypes)] extern "C" { #[link_name = "__swift_bridge__$some_function"] fn __swift_bridge__some_function() -> *mut Vec; @@ -623,6 +625,8 @@ mod extern_swift_fn_arg_vec_of_primitive_rust_type { pub fn some_function(arg: Vec) { unsafe { __swift_bridge__some_function(Box::into_raw(Box::new(arg))) } } + + #[allow(improper_ctypes)] extern "C" { #[link_name = "__swift_bridge__$some_function"] fn __swift_bridge__some_function(arg: *mut Vec); diff --git a/crates/swift-bridge-ir/src/codegen/generate_rust_tokens.rs b/crates/swift-bridge-ir/src/codegen/generate_rust_tokens.rs index 8e710798..5cefc103 100644 --- a/crates/swift-bridge-ir/src/codegen/generate_rust_tokens.rs +++ b/crates/swift-bridge-ir/src/codegen/generate_rust_tokens.rs @@ -259,11 +259,7 @@ impl ToTokens for SwiftBridgeModule { } let extern_swift_fn_tokens = if extern_swift_fn_tokens.len() > 0 { - quote! { - extern "C" { - #(#extern_swift_fn_tokens)* - } - } + generate_extern_c_block(extern_swift_fn_tokens) } else { quote! {} }; @@ -310,6 +306,83 @@ impl ToTokens for SwiftBridgeModule { } } +/// Generate an `extern "C"` block such as: +/// +/// ```no_run +/// extern "C" { +/// #[link_name = "some_swift_function_name"] +/// fn __swift_bridge__some_swift_function_name(); +/// } +/// ``` +/// +/// ## `improper_ctypes` lint suppression +/// +/// We suppress the `improper_ctypes` lint with `#[allow(improper_ctypes)]`. +/// +/// Given the following bridge module: +/// +/// ```ignore +/// #[swift_bridge::bridge] +/// mod ffi { +/// struct SomeStruct { +/// string: String +/// } +/// +/// extern "Swift" { +/// fn return_struct() -> SomeStruct; +/// } +/// } +/// ``` +/// +/// We would generate the following struct FFI representation and `extern "C"` block: +/// +/// ```no_run +/// struct __swift_bridge__SomeStruct { +/// string: *mut swift_bridge::string::RustString +/// } +/// +/// extern "C" { +/// #[link_name = "__swift_bridge__$rust_calls_swift_struct_repr_struct_one_string_field"] +/// fn __swift_bridge__return_struct() -> __swift_bridge__SomeStruct; +/// } +/// +/// # mod swift_bridge { pub mod string { pub struct RustString; }} +/// ``` +/// +/// The `__swift_bridge__SomeStruct` holds a pointer to a `RustString`. +/// +/// Since `RustString` is not FFI safe, and the Rust compiler cannot know that we only plan to use +/// the pointer as an opaque pointer, the Rust compiler emits an `improper_ctypes` lint. +/// +/// We silence this lint since we know that our usage is FFI safe. +/// +/// The risk in this is that if in the future we accidentally pass a truly improper ctype over FFI +/// this `#[allow(improper_ctypes)]` might prevent us from noticing. +/// +/// Given that our codegen is heavily tested we are not currently concerned about this. +/// +/// Should we become concerned about this in the future we could consider solutions such as: +/// +/// - Generate an `__swift_bridge__SomeStruct_INNER_OPAQUE` that only held opaque pointers. +/// We could then transmute the `__swift_bridge__SomeStruct` to/from this type when +/// passing/receiving it across the FFI boundary. +/// ``` +/// struct __swift_bridge__SomeStruct_INNER_OPAQUE { +/// string: *mut std::ffi::c_void +/// } +/// ``` +/// - This would involve generating an extra type, but given that they would have the same layout +/// and simply get transmuted into each other we could imagine that the optimizer would erase +/// all overhead. +fn generate_extern_c_block(extern_swift_fn_tokens: Vec) -> TokenStream { + quote! { + #[allow(improper_ctypes)] + extern "C" { + #(#extern_swift_fn_tokens)* + } + } +} + #[cfg(test)] mod tests { //! More tests can be found in src/codegen/codegen_tests.rs and its submodules. @@ -361,6 +434,7 @@ mod tests { unsafe { __swift_bridge__some_function() } } + #[allow(improper_ctypes)] extern "C" { #[link_name = "__swift_bridge__$some_function"] fn __swift_bridge__some_function (); @@ -389,6 +463,7 @@ mod tests { unsafe { __swift_bridge__some_function(start) } } + #[allow(improper_ctypes)] extern "C" { #[link_name = "__swift_bridge__$some_function"] fn __swift_bridge__some_function (start: bool); @@ -620,6 +695,7 @@ mod tests { } } + #[allow(improper_ctypes)] extern "C" { #[link_name = "__swift_bridge__$Foo$_free"] fn __swift_bridge__Foo__free (this: *mut std::ffi::c_void); @@ -685,6 +761,7 @@ mod tests { } } + #[allow(improper_ctypes)] extern "C" { #[link_name = "__swift_bridge__$Foo$new"] fn __swift_bridge__Foo_new() -> Foo; @@ -805,6 +882,7 @@ mod tests { } } + #[allow(improper_ctypes)] extern "C" { #[link_name = "__swift_bridge__$Foo$notify"] fn __swift_bridge__Foo_notify(this: swift_bridge::PointerToSwiftType); @@ -885,6 +963,7 @@ mod tests { unsafe { __swift_bridge__built_in_pointers(arg1, arg2) } } + #[allow(improper_ctypes)] extern "C" { #[link_name = "__swift_bridge__$built_in_pointers"] fn __swift_bridge__built_in_pointers ( @@ -912,6 +991,7 @@ mod tests { unsafe { __swift_bridge__void_pointers(arg1, arg2) } } + #[allow(improper_ctypes)] extern "C" { #[link_name = "__swift_bridge__$void_pointers"] fn __swift_bridge__void_pointers ( diff --git a/crates/swift-bridge-macro/tests/ui/incorrect-return-type.stderr b/crates/swift-bridge-macro/tests/ui/incorrect-return-type.stderr index 6792bf5b..b3e695c3 100644 --- a/crates/swift-bridge-macro/tests/ui/incorrect-return-type.stderr +++ b/crates/swift-bridge-macro/tests/ui/incorrect-return-type.stderr @@ -29,3 +29,6 @@ error[E0308]: mismatched types = note: expected struct `SomeType` found enum `Option` = note: this error originates in the attribute macro `swift_bridge::bridge` (in Nightly builds, run with -Z macro-backtrace for more info) +help: consider using `Option::expect` to unwrap the `Option` value, panicking if the value is an `Option::None` + | +6 | #[swift_bridge::bridge].expect("REASON") diff --git a/crates/swift-integration-tests/src/expose_opaque_rust_type.rs b/crates/swift-integration-tests/src/expose_opaque_rust_type.rs index 0414e17d..7fdf20df 100644 --- a/crates/swift-integration-tests/src/expose_opaque_rust_type.rs +++ b/crates/swift-integration-tests/src/expose_opaque_rust_type.rs @@ -1,3 +1,15 @@ +#[swift_bridge::bridge] +mod foo { + #[swift_bridge(swift_repr = "struct")] + struct SharedStruct { + field: String, + } + + extern "Swift" { + fn swift_func() -> SharedStruct; + } +} + #[swift_bridge::bridge] mod ffi { extern "Rust" { diff --git a/crates/swift-integration-tests/src/shared_types/shared_struct.rs b/crates/swift-integration-tests/src/shared_types/shared_struct.rs index 20033321..6e484173 100644 --- a/crates/swift-integration-tests/src/shared_types/shared_struct.rs +++ b/crates/swift-integration-tests/src/shared_types/shared_struct.rs @@ -17,6 +17,11 @@ mod ffi { #[swift_bridge(swift_repr = "struct")] struct StructReprStructTupleStruct(u8, u32); + #[swift_bridge(swift_repr = "struct")] + struct StructReprStructWithOneStringField { + field: String, + } + extern "Rust" { fn test_rust_calls_swift(); @@ -34,15 +39,19 @@ mod ffi { extern "Swift" { fn rust_calls_swift_struct_with_no_fields(arg: StructWithNoFields) -> StructWithNoFields; - fn rust_calls_struct_repr_struct_one_primitive_field( + fn rust_calls_swift_struct_repr_struct_one_primitive_field( arg: StructReprStructWithOnePrimitiveField, ) -> StructReprStructWithOnePrimitiveField; + + fn rust_calls_swift_struct_repr_struct_one_string_field( + arg: StructReprStructWithOneStringField, + ) -> StructReprStructWithOneStringField; } } fn test_rust_calls_swift() { self::tests::test_rust_calls_swift_struct_with_no_fields(); - self::tests::test_rust_calls_struct_repr_struct_one_primitive_field(); + self::tests::test_rust_calls_swift_struct_repr_struct_one_primitive_field(); } fn swift_calls_rust_struct_with_no_fields(arg: ffi::StructWithNoFields) -> ffi::StructWithNoFields { @@ -70,10 +79,10 @@ mod tests { ffi::rust_calls_swift_struct_with_no_fields(ffi::StructWithNoFields); } - pub(super) fn test_rust_calls_struct_repr_struct_one_primitive_field() { + pub(super) fn test_rust_calls_swift_struct_repr_struct_one_primitive_field() { let arg = ffi::StructReprStructWithOnePrimitiveField { named_field: 10 }; - let val = ffi::rust_calls_struct_repr_struct_one_primitive_field(arg); + let val = ffi::rust_calls_swift_struct_repr_struct_one_primitive_field(arg); assert_eq!(val.named_field, 10); }