You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
dead_code lint wrongly warns about public repr(C) structs with private fields but no constructors #126169
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?
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.
Reacted by Chayim Refael Friedman, J. Frimmel and Johannes Dahlström
@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.
Reacted by Jubilee, ptrca, Michael Yang and Nathaniel Bennett
The given reproducer is insufficient, compiling literally just this code in a library crate:
pubstructFoo{pubi: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.
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.
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.
Reacted by León Orell Valerian Liehr and Michael Yang
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.
Activity
fmease commentedon Jun 8, 2024
Personally speaking I find this argument quite weak because the private field
alignis not part of the public API and thus upstream (the author ofFoo) is permitted to change the type ofalignat any time without officially breaking any contact (as an aside, the exact output ofsize_of,align_of,Debugis not considered to come with any stability guarantees; auto trait leakage is a slightly different story though).For example, the author of
Foois allowed to change the type ofaligntoNonZeroI16renderingzeroed::<Foo>()in the user code immediate undefined behavior. However that would be the fault of the downstream user ofFoo, they shouldn't've usedzeroedonFooin the first place.asomers commentedon Jun 8, 2024
@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.
workingjubilee commentedon Jun 8, 2024
kinda thought the dead code group had an unused field sublint already.
saethlin commentedon Jun 8, 2024
The given reproducer is insufficient, compiling literally just this code in a library crate:
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.
saethlin commentedon Jun 9, 2024
Ah! The difference is that the libc crate contains a zillion trait impls that aren't derived. This is a minimal reproducer:
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 viamem::zeroedhere. Or I think that would be a new level of reasoning for this lint.saethlin commentedon Jun 9, 2024
Ah! Here's a better reproducer:
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.22 remaining items
kennykerr commentedon Jun 14, 2024
Externally constructed is a good way to think about it, but ideally we could just get
unusedto work in this case and not have to invent a new term.ChrisDenton commentedon Jun 14, 2024
Update: the libc crate has also opted to silence these warnings.
mu001999 commentedon Jun 15, 2024
#[allow(unused)]works well now.Soveu commentedon Jun 28, 2024
Interestingly, I found some code that technically does an exception for
repr(transparent)andrepr(C)structs/unions, but this is just a few minutes looking at the code, so I don't understand it yet.rust/compiler/rustc_passes/src/dead.rs
Line 465 in 89a0cfe
pnkfelix commentedon Jul 30, 2024
@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.
Rollup merge of rust-lang#128404 - compiler-errors:revert-dead-code-c…
Rollup merge of rust-lang#128404 - compiler-errors:revert-dead-code-c…
Rollup merge of rust-lang#128404 - compiler-errors:revert-dead-code-c…
Rollup merge of rust-lang#128404 - compiler-errors:revert-dead-code-c…
Rollup merge of rust-lang#128404 - compiler-errors:revert-dead-code-c…
Rollup merge of rust-lang#128404 - compiler-errors:revert-dead-code-c…
Auto merge of rust-lang#128404 - compiler-errors:revert-dead-code-cha…