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

sax commented 3 months ago

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 in swift-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 to swift-bridge annotates generated result enums with #[allow(unused)].

Fixes #270

error: field `0` is never read
   |
1  | #[swift_bridge::bridge]
   | ----------------------- field in this variant
...
3  |     enum ResultTransparentEnum {
   |          ^^^^^^^^^^^^^^^^^^^^^
   |
help: consider changing the field to be of unit type to suppress this warning while preserving the field numbering, or remove the field
   |
3  |     enum () {
   |          ~~

Example bridge

The following bridge code is the minimal reproduction case:

#[swift_bridge::bridge]
mod ffi {
    #[swift_bridge(swift_repr = "struct")]
    struct TransparentErrorStruct(pub String);

    extern "Rust" {
        fn rust_func_returns_result_transparent_struct(
            succeed: bool
        ) -> Result<(), TransparentErrorStruct>;
    }
}

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

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

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

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

chinedufn commented 3 months ago

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:

an example bridge module that would have left to a warning before this commit
an exampe of how the warning message looks

All PRs (PR body gets squashed into the commit message) should include an example of what they now enable/fix. We don't want contributors to have to click through links or read the diff to get a basic understanding of what is being enabled.

We use the example in the PR body when:

writing release notes
linking new contributors to older similar PRs

So it's nice if they can be understood at a glance.

I looked at the Announcing Rust 1.79.0 post and I don't see anything about changes to dead code lints. Where did you find that it was introduced in 1.79.0?

I ask because I want to understand whether this is a regression or an intentional change in rustc.

We are running 1.79.0 in CI, yet CI is passing. Here's a run from 3 days ago.

https://github.com/chinedufn/swift-bridge/actions/runs/9532352837/job/26274367654

In CI we deny warnings: https://github.com/chinedufn/swift-bridge/blob/cc8746c98ef7d176e11a216d4acf4e9fc07397de/.github/workflows/test.yml#L27-L29

We'll want to ensure that the test suite fails without the #[allow(unused)] that was introduced in this commit. This would make it possible for us to remove it in the future and be sure that we haven't re-introduced the warning.

We can modify the crates/swift-integration-tests/src/result.rs such that we get this warning without the #[allow(unused)].

All unusual code should be documented. If no one knows why the #[allow(unused)] was added they'll be afraid to try to remove it.

Can document it here https://github.com/chinedufn/swift-bridge/blob/58f4a40f96bb050607c746376422ab3c62e0e771/crates/swift-bridge-ir/src/bridged_type/bridgeable_result.rs#L353 .

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. litmus test: "could a contributor understand this documentation/explanation if they were offline".

Finally, why would the Err variant need #[allow(unused)], but not the Ok variant? That should be made clear in:

the commit message
near the test case and/or near the implementation code

sax commented 3 months ago

Sure thing and thanks for the feedback. All of this makes sense.

I don't 100% understand why the Err case generates a warning but the Ok does not. I'll see if I can figure out why this only started happening with 1.79.0. I definitely see issues and pull requests on other projects that have started facing this issue over the past few months, and in some cases having to #[allow(unused)] across dozens of enums across their codebases. But that still doesn't explain why it started here with 1.79.0.

sax commented 3 months ago

I can't find the specific change that introduced this in the rust linters, but I see issues around this going back to 2021: https://github.com/rust-lang/rust/issues/88900. More recently there are https://github.com/rust-lang/rust/issues/123068 and https://github.com/rust-lang/rust/issues/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.

sax commented 3 months ago

Maybe it's this: https://github.com/rust-lang/rust/pull/120845/. -Z debug-macros was stabilized and enabled by default. I see in a comment here (https://github.com/rust-lang/rust/issues/100758#issuecomment-1935815625) that #[collapse_debuginfo] is not supported for procedural macros, which might be why for swift-bridge the warning is terse and just points to the definition of the bridged error struct without any information about the generated result enum.

chinedufn commented 3 months ago

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 1

This one stands out: https://github.com/rust-lang/rust/issues/119545

Here's the commit that closes that issue https://github.com/rust-lang/rust/commit/a0fe4138ed83b9c3f6e1c85ef6e6a0846b1e542d

And here's the PR ( I haven't looked at the PR, just linking for reference https://github.com/rust-lang/rust/pull/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
    }
}

error: field `f` is never read
  --> $DIR/pub-field-in-priv-mod.rs:9:13
   |
LL |     pub struct S {
   |                - field in this struct
LL |         pub f: bool,
   |             ^
   |
note: the lint level is defined here
  --> $DIR/pub-field-in-priv-mod.rs:1:9
   |
LL | #![deny(dead_code)]
   |         ^^^^^^^^^

error: aborting due to 1 previous error

Same error as the one you reported in https://github.com/chinedufn/swift-bridge/issues/270

So, from a quick scan, I'm thinking:

BEFORE: pub enum inside of a private module would not get a dead code lint
AFTER: pub enum inside a private module gets a dead code lint if rustc does not think that it is used

Candidate 2

This commit makes unused tuple struct fields a warning, instead of allowed https://github.com/rust-lang/rust/commit/9fcf9c141068984ffcbb4cb00c83d891043761e8

Conclusion

Seems like the issue might potentially be a combination of Candidate 1 and Candidate 2... Or maybe something else .. not sure ... I'd have to bisect ...

Regardless, the generated enum FFI representations that this PR currently modifies all have #[repr(C)] on them, which I would expect the compiler to treat as used.

Looks like there was a recent discussion about this: https://github.com/rust-lang/rust/issues/119659#issuecomment-1879866540 https://github.com/rust-lang/rust/issues/119659#issuecomment-1885172045

Also, when you add the fact that the enum is used, this seems like a regression in rustc: https://github.com/chinedufn/swift-bridge/blob/852ece728d3f362f3ada4368e037fe9ebb2a913f/crates/swift-bridge-ir/src/codegen/codegen_tests/result_codegen_tests.rs#L412-L428

Note that the pub extern "C" fn __swift_bridge__some_function that I linked to above is making use of the pub enum ResultSomeOkTypeAndSomeErrEnum.

So I think the real fix here lies in rustc, not swift-bridge.

chinedufn commented 3 months ago

Heres a minimal example that generates the warning in rust 1.79.0

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=3f5c81c0749130a6e46fbea14cbb055f

fn main() {}

mod private_mod {
    #[repr(C)]
    pub enum MyEnum {
        Variant(u32)
    }

    #[no_mangle]
    pub extern "C" fn some_function() -> MyEnum {
        MyEnum::Variant(5)
    }
}

   Compiling playground v0.0.1 (/playground)
warning: field `0` is never read
 --> src/main.rs:6:17
  |
6 |         Variant(u32)
  |         ------- ^^^
  |         |
  |         field in this variant
  |
  = note: `#[warn(dead_code)]` on by default
help: consider changing the field to be of unit type to suppress this warning while preserving the field numbering, or remove the field
  |
6 |         Variant(())
  |                 ~~

warning: `playground` (bin "playground") generated 1 warning
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.85s
     Running `target/debug/playground`

Notice that the warning is incorrect since the enum variant is being used.

This is a regression in rustc's dead code pass ( this file -> https://github.com/rust-lang/rust/commits/master/compiler/rustc_passes/src/dead.rs?before=894f7a4ba6554d3797404bbf550d9919df060b97+35 ).

sax commented 3 months ago

Does this mean you don't think this swift-bridge should #[allow(unused)], even pending a fix in rustc? I'm finding that when running the swift-bridge tests while removing the #![allow(dead_code)] from the result integration test, warnings are generated for both transparent enums and structs.

When I allow unused just for the Err variant, transparent structs pass through without warnings (so the Ok variant is somehow ok). When I allow unused for both Ok and Err, then the transparent enums also succeed without warnings.

chinedufn commented 3 months ago

I opened an issue here: https://github.com/rust-lang/rust/issues/126706

Temporarily adding #[allow(unused)] is ok as long as:

we introduce a failing test case (I think you already did in a recent commit, but I haven't read it yet)
we mention and link to the issue in a way that makes it easy for a future maintainer to realize that we can delete the #[allow(unused)] when the issue is closed.

Let me know if you disagree or have another plan in mind.

EDIT fixed the issue link

sax commented 3 months ago

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 #[allow(unused)] reintroduces the warning on transparent enums.

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.

sax commented 3 months ago

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.

chinedufn commented 3 months ago

Just fixed the link -> https://github.com/rust-lang/rust/issues/126706

Great, I'll review today or tomorrow.

chinedufn commented 3 months ago

Looks like you added -> Result<(), TransStruct> and we already had a codegen test for that?

So, we're good? Anything else you're waiting for?

If not I can merge this once tests pass.

sax commented 3 months ago

I'm adding one more codegen test that seems like it's missing for Result<(), TransStruct>, and then seems good.

sax commented 3 months ago

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.

CGamesPlay commented 1 month ago

Is it possible to make a new release with this change? It doesn't seem possible to work around from user code. Thank you!

chinedufn commented 1 month ago

Released in 0.1.56 https://crates.io/crates/swift-bridge/0.1.56 https://github.com/chinedufn/swift-bridge/releases/tag/0.1.56

CGamesPlay commented 1 month ago

Thanks! Can confirm it fixes the warning for my transparent enum.

chinedufn / swift-bridge