-
Notifications
You must be signed in to change notification settings - Fork 62
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.
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
Conversation
Thank you for tracking this down, figuring out a fix, and submitting a PR. I appreciate you taking the time for this. I have a few small requests to be able to land this: Can you update your PR body with:
All PRs (PR body gets squashed into the commit message) should include an example of what they now enable/fix. We use the example in the PR body when:
So it's nice if they can be understood at a glance. I looked at the I ask because I want to understand whether this is a regression or an intentional change in We are running https://github.com/chinedufn/swift-bridge/actions/runs/9532352837/job/26274367654 In CI we deny warnings: swift-bridge/.github/workflows/test.yml Lines 27 to 29 in cc8746c
We'll want to ensure that the test suite fails without the We can modify the All unusual code should be documented. If no one knows why the Can document it here
OR can document it on/near the test case. Or mention in both places. Whatever you think makes the most sense. A contributor should not need to click the link to get a basic understanding of the problem/solution, so any links should be supplemental. Finally, why would the
|
Sure thing and thanks for the feedback. All of this makes sense. I don't 100% understand why the |
cc562e4
to
327d02a
Compare
I can't find the specific change that introduced this in the rust linters, but I see issues around this going back to 2021: rust-lang/rust#88900. More recently there are rust-lang/rust#123068 and rust-lang/rust#123418. My supposition is that even though the lints go back multiple years, until recently they were hidden when generated from proc macros. I'll keep looking. |
Maybe it's this: rust-lang/rust#120845. |
327d02a
to
879d9ca
Compare
Code generation happens before the linting pass so it's unlikely to be related to proc macros. I took a look at some of the recent changes to the dead code lint https://github.com/rust-lang/rust/commits/master/compiler/rustc_passes/src/dead.rs?before=894f7a4ba6554d3797404bbf550d9919df060b97+35 Candidate 1This one stands out: rust-lang/rust#119545 Here's the commit that closes that issue rust-lang/rust@a0fe413 And here's the PR ( I haven't looked at the PR, just linking for reference rust-lang/rust#119552 ) Note this ui test in the commit linked above: #![deny(dead_code)]
fn main() {
let _ = foo::S{f: false};
}
mod foo {
pub struct S {
pub f: bool, //~ ERROR field `f` is never read
}
}
Same error as the one you reported in #270 So, from a quick scan, I'm thinking:
Candidate 2This commit makes unused tuple struct fields a warning, instead of allowed rust-lang/rust@9fcf9c1 ConclusionSeems like the issue might potentially be a combination of Regardless, the generated enum FFI representations that this PR currently modifies all have Looks like there was a recent discussion about this: rust-lang/rust#119659 (comment) rust-lang/rust#119659 (comment) Also, when you add the fact that the enum is used, this seems like a regression in swift-bridge/crates/swift-bridge-ir/src/codegen/codegen_tests/result_codegen_tests.rs Lines 412 to 428 in 852ece7
Note that the So I think the real fix here lies in |
Heres a minimal example that generates the warning in fn main() {}
mod private_mod {
#[repr(C)]
pub enum MyEnum {
Variant(u32)
}
#[no_mangle]
pub extern "C" fn some_function() -> MyEnum {
MyEnum::Variant(5)
}
}
Notice that the warning is incorrect since the enum variant is being used. This is a regression in |
879d9ca
to
a9530a2
Compare
Does this mean you don't think this swift-bridge should When I allow unused just for the |
I opened an issue here: rust-lang/rust#126706 Temporarily adding
Let me know if you disagree or have another plan in mind. EDIT fixed the issue link |
Did you mean to open that on rustc? Looks like you opened it on swift-bridge. I have a new test for transparent structs, and removed an allowance from the integration tests so that removing the I'll work on adding a minimal bridge reproduction case to the description of this PR, and add a TODO to the code once I'm totally sure which issue to link to. Thanks for the taking the time to look at all the commits on rust. With all the recent issues related to these sorts of warnings, I had assumed that this was an intentional change to the linter. |
a9530a2
to
0fd3075
Compare
Please take a look again at your convenience! After understanding that the PR description gets squashed onto the merged commit, I added more context to the PR itself and made the text on the commit itself more terse. One thing to note is that in the code I'm linking to #279 on swift-bridge, but if you were intending that to be an issue on rustc I can change it (or you can modify this PR to save time) link to a different issue. |
Just fixed the link -> rust-lang/rust#126706 Great, I'll review today or tomorrow. |
0fd3075
to
d63b7ba
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks great.
Let some minor requests around making it easier for a maintainer from 5 years from now to understand all of this.
I left notes about what information to include, so hopefully you can just take that, massage it, and then plop it into the comments.
After that this looks good to me and we can land it.
Thanks again for researching and solving this. This is our first time papering over a Rust
regression so it was nice to iron out how we should do this if it ever happens again.
Good idea regarding landing the #[allow(unused)]
for now and then removing it in the future vs. my original idea of just closing this out.
Cheers.
@@ -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)]` |
There was a problem hiding this comment.
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.
@@ -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)]`. |
There was a problem hiding this comment.
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.
@@ -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. |
There was a problem hiding this comment.
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.
@@ -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. |
There was a problem hiding this comment.
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.
@@ -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. |
There was a problem hiding this comment.
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.
#[swift_bridge(swift_repr = "struct")] | ||
struct ResultTestTransparentStruct(pub String); | ||
|
||
extern "Rust" { | ||
fn rust_func_returns_result_null_transparent_struct( | ||
succeed: bool, | ||
) -> Result<(), ResultTestTransparentStruct>; | ||
} |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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>
|
||
impl std::fmt::Debug for ffi::ResultTestTransparentStruct { | ||
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||
write!(f, "{}", self) |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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")
…code in test rather than implement it.
Looks like you added So, we're good? Anything else you're waiting for? If not I can merge this once tests pass. |
I'm adding one more codegen test that seems like it's missing for |
Ok seems good to go! Thanks for the reviews. Also feels good to add a bit more test coverage to using transparent structs in results. |
Is it possible to make a new release with this change? It doesn't seem possible to work around from user code. Thank you! |
Thanks! Can confirm it fixes the warning for my transparent enum. |
In Rust 1.79.0, dead code warnings are emitted from enums where the compiler does not infer that wrapped data is used, even when the enum has been annotated with
#[repr(C]
. This warning triggers inswift-bridge
when defining transparent structs or enums that will be returned in error results.This appears to be a regression in the
dead_code
rustc lint. Pending upstream fixes to rustc, this change toswift-bridge
annotates generated result enums with#[allow(unused)]
.Fixes #270
Example bridge
The following bridge code is the minimal reproduction case: