Skip to content

Commit

Permalink
Fix improper_ctypes warning
Browse files Browse the repository at this point in the history
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 since the generated code has its `Span` set to the `SharedStruct`'s span.

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.
  • Loading branch information
chinedufn committed Mar 8, 2024
1 parent 48195b5 commit c63d18c
Show file tree
Hide file tree
Showing 9 changed files with 130 additions and 10 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -566,6 +566,8 @@ mod extern_swift_fn_return_vec_of_primitive_rust_type {
pub fn some_function() -> Vec<u8> {
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<u8>;
Expand Down Expand Up @@ -623,6 +625,8 @@ mod extern_swift_fn_arg_vec_of_primitive_rust_type {
pub fn some_function(arg: Vec<u8>) {
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<u8>);
Expand Down
88 changes: 83 additions & 5 deletions crates/swift-bridge-ir/src/codegen/generate_rust_tokens.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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! {}
};
Expand Down Expand Up @@ -310,6 +306,81 @@ 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.
///
/// The risk is that if we ever do actually attempt to do something that is not FFI safe the
/// `#[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>) -> 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.
Expand Down Expand Up @@ -361,6 +432,7 @@ mod tests {
unsafe { __swift_bridge__some_function() }
}

#[allow(improper_ctypes)]
extern "C" {
#[link_name = "__swift_bridge__$some_function"]
fn __swift_bridge__some_function ();
Expand Down Expand Up @@ -389,6 +461,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);
Expand Down Expand Up @@ -620,6 +693,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);
Expand Down Expand Up @@ -685,6 +759,7 @@ mod tests {
}
}

#[allow(improper_ctypes)]
extern "C" {
#[link_name = "__swift_bridge__$Foo$new"]
fn __swift_bridge__Foo_new() -> Foo;
Expand Down Expand Up @@ -805,6 +880,7 @@ mod tests {
}
}

#[allow(improper_ctypes)]
extern "C" {
#[link_name = "__swift_bridge__$Foo$notify"]
fn __swift_bridge__Foo_notify(this: swift_bridge::PointerToSwiftType);
Expand Down Expand Up @@ -885,6 +961,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 (
Expand Down Expand Up @@ -912,6 +989,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 (
Expand Down
12 changes: 12 additions & 0 deletions crates/swift-integration-tests/src/expose_opaque_rust_type.rs
Original file line number Diff line number Diff line change
@@ -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" {
Expand Down
17 changes: 13 additions & 4 deletions crates/swift-integration-tests/src/shared_types/shared_struct.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand All @@ -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 {
Expand Down Expand Up @@ -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);
}
Expand Down

0 comments on commit c63d18c

Please sign in to comment.