Skip to content
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

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

Merged
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 1 commit
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
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 allowances when rustc no longer issues dead code warnings for `#[repr(C)]`
Copy link
Owner

Choose a reason for hiding this comment

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

Let's explicitly say "#[allow(unused]" instead of "allowances".

Usually I'd say this is a minor nit that you could feel free to ignore, but because the codegen is fairly complex it's nice to leave as little room for misinterpretation as possible so let's make this change.

// 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
13 changes: 13 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,15 @@ 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)]`.
Copy link
Owner

Choose a reason for hiding this comment

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

Let's:

  • link to the Rust issue
  • make it clear that this can be deleted when that issue is closed

This way no matter where you run into this #[allow(unused) you can easily check whether it can be removed.

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 +488,14 @@ mod extern_rust_fn_return_result_transparent_enum_type_and_opaque_rust_type {
}
}

// Allows unused to avoid dead_code warnings in Rust 1.79.0 or later.
Copy link
Owner

Choose a reason for hiding this comment

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

Let's:

  • link to the Rust issue
  • make it clear that this can be deleted when that issue is closed

This way no matter where you run into this #[allow(unused) you can easily check whether it can be removed.

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 +565,14 @@ mod extern_rust_fn_return_result_unit_type_and_transparent_enum_type {
}
}

// Allows unused to avoid dead_code warnings in Rust 1.79.0 or later.
Copy link
Owner

Choose a reason for hiding this comment

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

Let's:

  • link to the Rust issue
  • make it clear that this can be deleted when that issue is closed

This way no matter where you run into this #[allow(unused) you can easily check whether it can be removed.

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 +638,15 @@ mod extern_rust_fn_return_result_tuple_type_and_transparent_enum_type {
}
}

// Allows unused to avoid dead_code warnings in Rust 1.79.0 or later.
Copy link
Owner

Choose a reason for hiding this comment

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

Let's:

  • link to the Rust issue
  • make it clear that this can be deleted when that issue is closed

This way no matter where you run into this #[allow(unused) you can easily check whether it can be removed.

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
37 changes: 33 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,15 @@ mod ffi {
fn val(&self) -> u32;
}

#[swift_bridge(swift_repr = "struct")]
struct ResultTestTransparentStruct(pub String);

extern "Rust" {
fn rust_func_returns_result_null_transparent_struct(
succeed: bool,
) -> Result<(), ResultTestTransparentStruct>;
}
Copy link
Owner

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

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

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 +146,30 @@ fn rust_func_return_result_unit_struct_opaque_rust(
}
}

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

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

impl std::fmt::Debug for ffi::ResultTestTransparentStruct {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
write!(f, "{}", self)
Copy link
Owner

Choose a reason for hiding this comment

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

Let's avoid implying that these implementations are being used.

unreachable!("Debug impl was added to pass type checking")

}
}

impl std::fmt::Display for ffi::ResultTestTransparentStruct {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
write!(f, "{}", self.0)
Copy link
Owner

Choose a reason for hiding this comment

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

unreachable!("Display impl was added to pass type checking")

}
}

pub struct ResultTestOpaqueRustType {
val: u32,
}
Expand Down