Repository navigation
Unactionable "field is never read" warning for printed structs with the Debug derive #88900
Description
Activity
First off: Denying warnings is considered harmful [0] [1] [2] [3] and should not be default in a project. You may use it for testing purposes, but not actively as a blocker.
Second: I can see, that you might think that the lint is wrong, but it is actually right. You never read the field
a.
It is used in the Debug output, yes, but not actively in your code.
For the warning to silence you can prepend a underscore (e.g._a) but that's it.The same goes for
Clonebtw.Here's the relevant PR + a lot of discussion: #85200
Reacted by FabianWolff, Martin Habovštiak, Raman Kumar and CaptainEmoReacted by Tobias Bieniek, Robert, Danny, lexi-brt, Gaurish Gangwar, Adam Sindelar, Jude Southworth, ben, microsoft-is-bad-123, Aaron Manning and 4 more- We don't hard-code
deny(warnings), we pass in-D warningsas a rustc flag as part of our CI, which doesn't suffer from the "forward compatibility" issues called out in the four links you shared. I think most people would agree that warnings should be either resolved or suppressed, especially in public libraries. - This doesn't change the fact that something uses it. If I were to implement a new Debug trait in a new crate (with an equivalent proc macro derive), then use that crate and derive that version of Debug on my
Footype, I wouldn't get the warning. This isn't about whether or not I directly touch the field. Its about whether something uses the field. As discussed in Ignore derived Clone and Debug implementations during dead code analysis #85200, the fact that Debug doesn't count is a new exception to a rule that applies to literally everything else.
It sounds like ignoring Debug impls was a pragmatic choice to make this lint more useful. Debug is a common / recommended derive for structs, which meant that most structs weren't getting "unused field" lints. Working around this seems reasonable.
However I think it is worth calling out that this specific case is relatively common / many people would consider it "valid". At the very least, I imagine some users would be confused, especially given that this isn't consistent with other traits and printing debug derives is super common. Warnings should be actionable. It is the compiler telling the user that something should be fixed / isn't recommended. In my ideal world either:
- The code above is considered valid by rustc: when a Debug impl is actually used via something like println, unused field lints are suppressed (because the fields are actually used by code the user chose to write).
- The code above is considered invalid by rustc, but equivalent tools are recommended / provided: if using Debug in this context is incorrect (ex: Display should be used instead), ideally there is a Debug-like derive for Display, and ideally rustc recommends that in these cases.
Of course both of those solutions are complicated / controversial and I'm sure this was supposed to be an easy win. I'm not demanding any action here / this isn't a high priority for me. I'm just reporting a confusing / unintuitive new behavior. I doubt I'll be the last person to hit this.
Reacted by IceSentry, Denis Lisov, Parker Timmerman, Matthew Planchard, Valeriy V. Vorotyntsev, lexi-brt, Konrad Frysiak, Gaurish Gangwar, student020341, Steven Gutstein and 6 more- We don't hard-code
Nominating this for T-lang awareness, and possible discussion.
We have another datapoint at synth. In our case, the code in question is not derived; the
type_namefield is actually read in a trait impl (though to be fair it's in anasync fn, so there may be expansion involved, too).@llogiq The piece of code that you link to reads the
data_typefield, not thetype_namefield, so the latter may very well be unused.In general, only automatically derived impls should be ignored; if you find an exception to this, it's a bug (whereas ignoring derived implementations is intended).
I’ve been getting these warnings as well. The struct actually contains a Child (tokio::process::Child) handle. So at drop time of the struct the Child handle drops and eventually cleanups the child process. So it’s not technically true that the field is never read, because the drop glue certainly does.
Edit: On second thought, this warning might have been lurking there all along because the struct has a Debug derive.
Edit: On second thought, this warning might have been lurking there all along because the struct has a Debug derive.
That's what I was going to say. Nothing was changed with regards to the
Droptrait, so the behavior you're complaining about must have been there before (it was just hidden by theDebugderive).For those wondering about disruptiveness of the change, we had to add this to
93153 fields in the main Fuchsia repo. I'd say this is among the most disruptive warnings changes we've had to fix. Most of these structs seem to only deriveDebug; I haven't dug into what they're used for or whether these fields should exist yet.https://fuchsia-review.googlesource.com/c/fuchsia/+/581062
https://fuchsia-review.googlesource.com/c/fuchsia/+/581066Reacted by Tobias Bieniek, Matthew Planchard, Robert, Gaurish Gangwar and Dot32From the disruptiveness angle, I think it shouldn't matter how disruptive a warning change is, because that's the deal of
-D warningsor#![deny(warnings)]: By enabling them, you get the duty of having to fix newly occurring warnings. I'm not going as far as @hellow554 that you shouldn't enable them, but that if you do, you lose your right to complain about breakage. If you don't like having to fix new warnings, that's your choice, no need to add-D warnings.Separate from this is the question whether the lint is a good idea. I lean towards yes. Some thoughts:
- ➕ From the practical angle, it has been lauded to have found many unused fields in the compiler.
- ➕ From a theoretical angle, the purpose of
Debugderives is to aid printf debugging. Their purpose is not to extract data from a structure in an automatted way, by say parsing its output, so there won't be proper "use" of a field somewhere in the code base even if you debug-print it. - ➖ However, one can make a case that printf debugging is a usage of its own, and even if the debug impl is never used, it might be one day.
- ➕ On the other hand, when I add a private function to print some internal state, but don't use it because I'm not debugging right now, then the compiler has no idea it's actually meant for debugging purposes so justifiably warns about it. That's what silencing the lint is for.
- ➕ It's recommended to implement
Debugin the API guidelines in both C-COMMON-TRAITS and in C-DEBUG. So you'll have lots of structs that implement the trait. If all members of these structs enjoy immunity from the "field is never read warning", won't that severely weaken the power of the lint? - This is more a consistency question, Feeding a variable to the
dbg!()macro counts as use of the variable. People often create variables for the sole purpose of printf debugging, should they be reminded by warnings that those variables should go? Why treat fields differently?
Reacted by Marcel Hellwig, Jonas Platte, Soumya, Martin Habovštiak, Valeriy V. Vorotyntsev and ⭐️NINIKA⭐️Reacted by Marcel Hellwig, Alex Kladov, Frank Steffahn, scottmcm and Matthew PlanchardI agree that just deriving Debug shouldn't hide unused warnings and making that change is nice. The issue is that if you actually use the trait for printing, it should count as using the value. Adding an
#[allow(unused)]on a property that is both assigned and then printed using Debug doesn't feel like the right solution to this.Reacted by Rolf Karp, Dot32 and jalbiero25 remaining items
A very common case (from looking at these errors in my codebase) is informational fields on error types. These are not really intended to be used as such but will show when the error is logged.
I think a better compromise would be to add a new Printable trait to replace debug. Printable would essentially be intended for structs who's sole purpose is to be printed. Of course, you would lose the "unused" warnings on any struct that derived from Printable.
Another might've been converting the dead_code lint into a group, so that people could readily add
#![allow(dead_code_in_derived_impls)]to their file or something like that (presumably temporarily).Was something like this ever added? Seems weird that rustc_trivial_field_reads is internal unstable. It doesn't make sense to lock this down, I have a use-case where the derive macro will generate reads and writes for specific fields. But I want to ensure the user of this macro still uses the field in their code.
- marked warning of dead code against live enum field #134168 as a duplicate of this issue
on Dec 11, 2024 - marked Unexpected dead_code warning when returning Result type from main #123418 as a duplicate of this issue
on Dec 11, 2024
The latest nightly (rustc 1.57.0-nightly (8c2b6ea 2021-09-11)) introduces a new warning that I consider to be invalid.
Given the following code: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=14d790d2f286f58543c42d915f60e182
The current output is:
This feels very wrong to me. It means any struct created with the intent to be printed is "invalid" according to the compiler. This warning is not actionable. My only option is to suppress it, live with it, or insert some sort of "dummy read". This has broken our CI builds for Bevy (because we treat warnings as errors).