Skip to content

dead_code lint wrongly warns about public repr(C) structs with private fields but no constructors #126169

Closed

Description

@asomers

Code

#[repr(C)] // <--
pub struct Foo {
    pub i: i16,
    align: i16
}

Current output

warning: field `align` is never read
 --> src/lib.rs:3:5
  |
1 | pub struct Foo {
  |            --- field in this struct
2 |     pub i: i16,
3 |     align: i16
  |     ^^^^^
  |
  = note: `#[warn(dead_code)]` on by default

Desired output

No warnings, or perhaps a different warning.

Rationale and extra context

This warning is probably triggering because there's no constructor that allows a user to set the align field on struct Foo. However, there is still a valid way for a user to construct it: std::mem::zeroed(). The libc crate contains many structs that must be initialized that way. libc could silence these warnings by #[allow(dead_code)], but that's too broad a brush. It would silence many much more serious warnings. Could we please either revert to the previous behavior, or else create a new lint for this that can be separately silenced?

Other cases

No response

Rust Version

rustc 1.80.0-nightly (804421dff 2024-06-07)
binary: rustc
commit-hash: 804421dff5542c9c7da5c60257b5dbc849719505
commit-date: 2024-06-07
host: x86_64-unknown-freebsd
release: 1.80.0-nightly
LLVM version: 18.1.7

Anything else?

This is a regression since rustc 1.80.0-nightly (f67a1acc0 2024-06-01). That version did not warn in such cases.

Downstream issue: rust-lang/libc#3740

Activity

  1. added
    A-diagnosticsArea: Messages for errors, warnings, and lints
    T-compilerRelevant to the compiler team, which will review and decide on the PR/issue.
    on Jun 8, 2024
  2. added
    A-lintsArea: Lints (warnings about flaws in source code) such as unused_mut.
    T-langRelevant to the language team
    C-discussionCategory: Discussion or questions that doesn't represent real issues.
    and removed
    A-diagnosticsArea: Messages for errors, warnings, and lints
    on Jun 8, 2024
  3. fmease commented on Jun 8, 2024

    @fmease
    Member

    However, there is still a valid way for a user to construct it: std::mem::zeroed()

    Personally speaking I find this argument quite weak because the private field align is not part of the public API and thus upstream (the author of Foo) is permitted to change the type of align at any time without officially breaking any contact (as an aside, the exact output of size_of, align_of, Debug is not considered to come with any stability guarantees; auto trait leakage is a slightly different story though).

    For example, the author of Foo is allowed to change the type of align to NonZeroI16 rendering zeroed::<Foo>() in the user code immediate undefined behavior. However that would be the fault of the downstream user of Foo, they shouldn't've used zeroed on Foo in the first place.

  4. asomers commented on Jun 8, 2024

    @asomers
    ContributorAuthor

    @fmease I would agree with all of that if this were all pure Rust code. But it isn't. libc has to represent structs that were defined in C, where zero-initialization is normal. Many of them already have fields used for padding, and the official C documentation requires zero-initialization. Can we please at least use a new name for this lint?

    Another case is when the only way to initialize a struct is to first allocate it uninitialized, and then populate it with a C library function or a syscall. To declare such a structure in pure safe Rust, the syscall which initializes it would have to be its constructor. But defining Rusty wrappers for such syscalls is beyond the scope of the libc crate, which is only concerned with structs and function definitions.

  5. workingjubilee commented on Jun 8, 2024

    @workingjubilee
    Member

    kinda thought the dead code group had an unused field sublint already.

  6. saethlin commented on Jun 8, 2024

    @saethlin
    Member

    The given reproducer is insufficient, compiling literally just this code in a library crate:

    pub struct Foo {
        pub i: i16,
        align: i16
    }

    has been producing warnings for years. I don't know what's different about the libc crate, but the bisection using the entire libc crate is pretty easy and points to #125572.

  7. saethlin commented on Jun 9, 2024

    @saethlin
    Member

    Ah! The difference is that the libc crate contains a zillion trait impls that aren't derived. This is a minimal reproducer:

    pub struct Foo(i16);
    
    impl Clone for Foo {
        fn clone(&self) -> Self {
            Self(self.0)
        }
    }

    The change was that trait impls don't make the struct live anymore.

    Given that, I do not think it makes a whole lot of sense to be reasoning about constructability via mem::zeroed here. Or I think that would be a new level of reasoning for this lint.

  8. saethlin commented on Jun 9, 2024

    @saethlin
    Member

    Ah! Here's a better reproducer:

    #[repr(C)]
    pub struct Foo(i16);

    I don't know enough about the lint parts of the compiler to see how the diff in the regressing PR caused repr(C) to stop making the field live, but that seems to be the bug.

  9. removed
    T-langRelevant to the language team
    C-discussionCategory: Discussion or questions that doesn't represent real issues.
    on Jun 9, 2024
  10. 22 remaining items

  11. kennykerr commented on Jun 14, 2024

    @kennykerr
    Contributor

    Externally constructed is a good way to think about it, but ideally we could just get unused to work in this case and not have to invent a new term.

  12. ChrisDenton commented on Jun 14, 2024

    @ChrisDenton
    Member

    Update: the libc crate has also opted to silence these warnings.

  13. mu001999 commented on Jun 15, 2024

    @mu001999
    Member

    Though #[allow(unused)] could play the same role, except currently that does not quite work: #126289.

    #[allow(unused)] works well now.

  14. Soveu commented on Jun 28, 2024

    @Soveu
    Contributor

    Interestingly, I found some code that technically does an exception for repr(transparent) and repr(C) structs/unions, but this is just a few minutes looking at the code, so I don't understand it yet.

    self.repr_unconditionally_treats_fields_as_live =

  15. pnkfelix commented on Jul 30, 2024

    @pnkfelix
    Contributor

    @rustbot label: I-lang-nominated

    I want T-lang to discuss the fundamental notions of "how do we determine what is dead code" that this issue and associated PR #125572 are exploring.

  16. added a commit that references this issue on Jul 31, 2024
    0c986b1
  17. added 2 commits that reference this issue on Aug 1, 2024
    bc9ccee
    3880c09
  18. added 3 commits that reference this issue on Aug 2, 2024
    2a4bed3
    9e8b082
    a966ede
  19. added a commit that references this issue on Aug 3, 2024
    1f47624
  20. removed
    I-lang-nominatedNominated for discussion during a lang team meeting.
    on Feb 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    A-lintsArea: Lints (warnings about flaws in source code) such as unused_mut.C-bugCategory: This is a bug.L-dead_codeLint: dead_codeT-compilerRelevant to the compiler team, which will review and decide on the PR/issue.

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions