Skip to content

Fix dead_code warning when returning Result with bridged type in error case #278

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
chinedufn merged 4 commits into from
Jun 21, 2024
Merged

Fix dead_code warning when returning Result with bridged type in error case #278

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d63b7ba
Allow unused returning transparent structs or enums in Results
sax Jun 17, 2024
227233e
Document rust issue related to allow(unused). Mark unreachable debug …
sax Jun 20, 2024
992d03e
Add integration test for returning Result<(), TransparentStruct> to S…
sax Jun 20, 2024
3da3f1c
Add codegen test for Result<(), TransparentStruct>
sax Jun 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions SwiftRustIntegrationTestRunner/SwiftRustIntegrationTestRunner/Result.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,9 @@ extension AsyncResultOpaqueRustType2: Error {}
extension ResultTransparentEnum: @unchecked Sendable {}
extension ResultTransparentEnum: Error {}

extension ResultTransparentStruct: @unchecked Sendable {}
extension ResultTransparentStruct: Error {}

extension SameEnum: @unchecked Sendable {}
extension SameEnum: Error {}

Expand Down
40 changes: 40 additions & 0 deletions SwiftRustIntegrationTestRunner/SwiftRustIntegrationTestRunnerTests/ResultTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,46 @@ class ResultTests: XCTestCase {
}
}

/// Verify that we can receive a Result<(), TransparentStruct> from Rust
func testResultNullTransparentStruct() throws {
try! rust_func_return_result_null_transparent_struct(true)

do {
try rust_func_return_result_null_transparent_struct(false)
XCTFail("The function should have returned an error.")
} catch let error as ResultTransparentStruct {
XCTAssertEqual(error.inner.toString(), "failed")
}

XCTContext.runActivity(named: "Should return a Unit type") {
_ in
do {
let _ :() = try rust_func_return_result_unit_type_enum_opaque_rust(true)
} catch {
XCTFail()
}
}

XCTContext.runActivity(named: "Should throw an error") {
_ in
do {
let _ :() = try rust_func_return_result_unit_type_enum_opaque_rust(false)
XCTFail("The function should have returned an error.")
} catch let error as ResultTransparentEnum {
switch error {
case .NamedField(let data):
XCTAssertEqual(data, 123)
case .UnnamedFields(_, _):
XCTFail()
case .NoFields:
XCTFail()
}
} catch {
XCTFail()
}
}
}

/// Verify that we can receive a Result<Vec<>, OpaqueRust> from Rust
func testSwiftCallRustResultVecUInt32Rust() throws {
let vec = try! rust_func_return_result_of_vec_u32()
Expand Down
4 changes: 4 additions & 0 deletions crates/swift-bridge-ir/src/bridged_type/bridgeable_result.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -375,10 +375,14 @@ impl BuiltInResult {
.err_ty
.to_ffi_compatible_rust_type(swift_bridge_path, types);
let mut custom_rust_ffi_types = vec![];
// TODO: remove `#[allow(unused)]` when rustc no longer issues dead code warnings for `#[repr(C)]`
// structs or enums: https://github.com/rust-lang/rust/issues/126706
custom_rust_ffi_types.push(quote! {
#[repr(C)]
pub enum #ty {
#[allow(unused)]
Ok #ok,
#[allow(unused)]
Err(#err),
}
});
Expand Down
20 changes: 20 additions & 0 deletions crates/swift-bridge-ir/src/codegen/codegen_tests/result_codegen_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -407,11 +407,16 @@ mod extern_rust_fn_return_result_opaque_rust_type_and_transparent_enum_type {
}
}

// In Rust 1.79.0 dead_code warnings are issued for wrapped data in enums in spite of the enum
// having `#[repr(C)]`. `#[allow(unused)]` can be removed following resolution and release of this
// issue: https://github.com/rust-lang/rust/issues/126706
fn expected_rust_tokens() -> ExpectedRustTokens {
ExpectedRustTokens::Contains(quote! {
#[repr(C)]
pub enum ResultSomeOkTypeAndSomeErrEnum{
#[allow(unused)]
Ok(*mut super::SomeOkType),
#[allow(unused)]
Err(__swift_bridge__SomeErrEnum),
}

Expand Down Expand Up @@ -484,11 +489,16 @@ mod extern_rust_fn_return_result_transparent_enum_type_and_opaque_rust_type {
}
}

// In Rust 1.79.0 dead_code warnings are issued for wrapped data in enums in spite of the enum
// having `#[repr(C)]`. `#[allow(unused)]` can be removed following resolution and release of this
// issue: https://github.com/rust-lang/rust/issues/126706
fn expected_rust_tokens() -> ExpectedRustTokens {
ExpectedRustTokens::Contains(quote! {
#[repr(C)]
pub enum ResultSomeOkEnumAndSomeErrType{
#[allow(unused)]
Ok(__swift_bridge__SomeOkEnum),
#[allow(unused)]
Err(*mut super::SomeErrType),
}

Expand Down Expand Up @@ -558,11 +568,16 @@ mod extern_rust_fn_return_result_unit_type_and_transparent_enum_type {
}
}

// In Rust 1.79.0 dead_code warnings are issued for wrapped data in enums in spite of the enum
// having `#[repr(C)]`. `#[allow(unused)]` can be removed following resolution and release of this
// issue: https://github.com/rust-lang/rust/issues/126706
fn expected_rust_tokens() -> ExpectedRustTokens {
ExpectedRustTokens::Contains(quote! {
#[repr(C)]
pub enum ResultVoidAndSomeErrEnum{
#[allow(unused)]
Ok,
#[allow(unused)]
Err(__swift_bridge__SomeErrEnum),
}

Expand Down Expand Up @@ -628,12 +643,17 @@ mod extern_rust_fn_return_result_tuple_type_and_transparent_enum_type {
}
}

// In Rust 1.79.0 dead_code warnings are issued for wrapped data in enums in spite of the enum
// having `#[repr(C)]`. `#[allow(unused)]` can be removed following resolution and release of this
// issue: https://github.com/rust-lang/rust/issues/126706
fn expected_rust_tokens() -> ExpectedRustTokens {
ExpectedRustTokens::ContainsMany(vec![
quote! {
#[repr(C)]
pub enum ResultTupleI32U32AndSomeErrEnum{
#[allow(unused)]
Ok(__swift_bridge__tuple_I32U32),
#[allow(unused)]
Err(__swift_bridge__SomeErrEnum),
}
},
Expand Down
41 changes: 37 additions & 4 deletions crates/swift-integration-tests/src/result.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,4 @@
//! See also: crates/swift-bridge-ir/src/codegen/codegen_tests/result_codegen_tests.rs
// This is a temporary workaround until https://github.com/chinedufn/swift-bridge/issues/270
// is closed. When tests are compiled they have `-D warnings` (deny warnings) enabled, so
// tests won't even compile unless this warning is ignored.
#![allow(dead_code)]

#[swift_bridge::bridge]
mod ffi {
Expand Down Expand Up @@ -41,6 +37,17 @@ mod ffi {
fn val(&self) -> u32;
}

#[swift_bridge(swift_repr = "struct")]
struct ResultTransparentStruct {
pub inner: String,
}

extern "Rust" {
fn rust_func_return_result_null_transparent_struct(
succeed: bool,
) -> Result<(), ResultTransparentStruct>;
}
Comment on lines 40 to 49
Copy link
Owner

@chinedufn chinedufn Jun 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As it stands now it's unclear why this is here. A maintainer 5 years from now would look at this and be confused as to why we aren't calling it from the Swift side.
This is here for a special reason (to prevent regressions when we remove this #[allow(unused)], but this will not be clear to a maintainer in 5 years)

Let's explain why this was put here.

  • In Rust 1.79.0 we noticed a warning when we returned -> Result<*, TransparentStruct> when the struct has at least one field
  • This is a regression in Rust that should be fixed in a future version
  • At some point after that regression is fixed we'll remove the #[allow(unused)] from the coegen
  • This test is meant to help us be sure that after removing the #[allow(unused)] the warning does not come back
  • We do not call this function from Swift since only exists here as a test that no warning is generated in Rust

Copy link
Contributor Author

@sax sax Jun 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Funny thing... I didn't even see the Swift integration tests. I'll do one better and add a test for results with transparent structs there. It seems like I'm probably not the only person who will want to use that feature.

Copy link
Owner

@chinedufn chinedufn Jun 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That works, but if that's the case need to also add a codegen test for a Rust function that returns -> Result<TransparentStruct, TransparentStruct>

Similar to this but for transparent structs

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

Lines 129 to 200 in 852ece7

/// Test code generation for Rust function that accepts a Result<T, E> where T and E are
/// opaque Rust types.
mod extern_rust_fn_return_result_opaque_rust {
use super::*;
fn bridge_module_tokens() -> TokenStream {
quote! {
mod ffi {
extern "Rust" {
type SomeType;
fn some_function () -> Result<SomeType, SomeType>;
}
}
}
}
fn expected_rust_tokens() -> ExpectedRustTokens {
ExpectedRustTokens::Contains(quote! {
#[export_name = "__swift_bridge__$some_function"]
pub extern "C" fn __swift_bridge__some_function() -> swift_bridge::result::ResultPtrAndPtr {
match super::some_function() {
Ok(ok) => {
swift_bridge::result::ResultPtrAndPtr {
is_ok: true,
ok_or_err: Box::into_raw(Box::new({
let val: super::SomeType = ok;
val
})) as *mut super::SomeType as *mut std::ffi::c_void
}
}
Err(err) => {
swift_bridge::result::ResultPtrAndPtr {
is_ok: false,
ok_or_err: Box::into_raw(Box::new({
let val: super::SomeType = err;
val
})) as *mut super::SomeType as *mut std::ffi::c_void
}
}
}
}
})
}
fn expected_swift_code() -> ExpectedSwiftCode {
ExpectedSwiftCode::ContainsAfterTrim(
r#"
public func some_function() throws -> SomeType {
try { let val = __swift_bridge__$some_function(); if val.is_ok { return SomeType(ptr: val.ok_or_err!) } else { throw SomeType(ptr: val.ok_or_err!) } }()
}
"#,
)
}
const EXPECTED_C_HEADER: ExpectedCHeader = ExpectedCHeader::ContainsAfterTrim(
r#"
struct __private__ResultPtrAndPtr __swift_bridge__$some_function(void);
"#,
);
#[test]
fn extern_rust_fn_return_result_opaque_rust() {
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();
}
}

Anytime we add support for a new type we add a codegen test and an integration test.

In this case the new type that we are explicitly supporting (now that you're adding this test) is -> Result<TransStruct, TransStruct>


enum ResultTransparentEnum {
NamedField { data: i32 },
UnnamedFields(u8, String),
Expand Down Expand Up @@ -141,6 +148,32 @@ fn rust_func_return_result_unit_struct_opaque_rust(
}
}

fn rust_func_return_result_null_transparent_struct(
succeed: bool,
) -> Result<(), ffi::ResultTransparentStruct> {
if succeed {
Ok(())
} else {
Err(ffi::ResultTransparentStruct {
inner: "failed".to_string(),
})
}
}

impl std::error::Error for ffi::ResultTransparentStruct {}

impl std::fmt::Debug for ffi::ResultTransparentStruct {
fn fmt(&self, _f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
unreachable!("Debug impl was added to pass `Error: Debug + Display` type checking")
}
}

impl std::fmt::Display for ffi::ResultTransparentStruct {
fn fmt(&self, _f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
unreachable!("Display impl was added to pass `Error: Debug + Display` type checking")
}
}

pub struct ResultTestOpaqueRustType {
val: u32,
}
Expand Down