Unactionable "field is never read" warning for printed structs with the Debug derive

[cart]

The latest nightly (rustc 1.57.0-nightly (8c2b6ea37 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

#[derive(Debug)]
struct Foo {
    a: String,
}

fn main() {
  let foo = Foo {
      a: "hello".to_string(),
  };

  println!("{:?}", foo);
}

The current output is:

warning: field is never read: `a`
 --> src/main.rs:3:5
  |
3 |     a: String,
  |     ^^^^^^^^^
  |
  = note: `#[warn(dead_code)]` on by default

warning: `playground` (bin "playground") generated 1 warning
    Finished dev [unoptimized + debuginfo] target(s) in 1.56s

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).

[hellow554]

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 Clone btw.

Here's the relevant PR + a lot of discussion: https://github.com/rust-lang/rust/pull/85200

[cart]

1. We don't hard-code deny(warnings), we pass in -D warnings as 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.
2. 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 Foo type, 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 #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:

1. 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).
2. 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.

[llogiq]

We have another datapoint at synth. In our case, the code in question is not derived; the type_name field is actually read in a trait impl (though to be fair it's in an async fn, so there may be expansion involved, too).

[Mark-Simulacrum]

Nominating this for T-lang awareness, and possible discussion.

[FabianWolff]

We have another datapoint at synth. In our case, the code in question is not derived; the type_name field is actually read in a trait impl (though to be fair it's in an async fn, so there may be expansion involved, too).

@llogiq The piece of code that you link to reads the data_type field, not the type_name field, 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).

[cynecx]

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.

[FabianWolff]

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 Drop trait, so the behavior you're complaining about must have been there before (it was just hidden by the Debug derive).

[tmandry]

For those wondering about disruptiveness of the change, we had to add this to 93 153 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 derive Debug; 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/+/581066

[est31]

From the disruptiveness angle, I think it shouldn't matter how disruptive a warning change is, because that's the deal of -D warnings or #![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 Debug derives 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 Debug in 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?

[IceSentry]

I 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.

[matklad]

Effect of this lint on rust-analyzer: https://github.com/rust-analyzer/rust-analyzer/pull/10248/files

There were 8 cases of this firing. 3 of them were due to field being needed by Drop (true warning with low utility), and the fix was to add _ to field's name, as is idiomatic to signify that the stuff is only needed for drop. 5 were true warnings with high utility where dead fields were removed, and in 2 cases that transitively triggered removal of some dead code.

[matklad]

I want to take a closer look at the bevy issue (https://github.com/bevyengine/bevy/pull/2811)! I want to understand why it can be considered a false warning -- to me, it seems like a case where the lint emits a true warning. More generally, I don't think that a mere fact of printing a struct with #[derive(Debug)] should be considered a usage.

Bevy usage is from a literal example, so I'd like to start with a different, more "real world" case. The code could look roughly like this:

#[derive(Debug)]
struct MyEvent {
    pub message: String,
    pub random_value: f32,
}

fn receiving_system(mut event_reader: EventReader<MyEvent>) {
    for my_event in event_reader.iter() {
        log::debug!("processing {:?}", *my_event);
        do_work(my_event)
    }
}

Here, we have an event processing loop, which logs an event, and then processes it. In this case, I would argue that it makes sense to emit a warning if, eg random_value isn't used by anything except the logging call. Although we do literary print every field, it doesn't mean that they are not dead, they might have become unused after refactors.

What makes the actual bevy case different is that it's an example, so the do_work is actually left empty. And examples are known for triggering unused warnings -- that's why doctest allow(unused) by default, for example. It seems reasonable that the warning fires here: the fix would be to either spice up the example to actually do something with the fields inside the loop, or, if we literally just want to display them to the user (and not as a debug aid), use a manually-formatted output.

That being said: I agree that there are cases where you do want to use #[derive(Debug)] just as a quick way to render something to strings. It's just that I don't think that's very common, and I don't think that bevy is an example of that -- it is rather the more general issue that examples tend to fire unused warnings.

[matklad]

Thoughts about blast radius: it's very clearly suboptimal if the rollout path for big consumers like Fuchsia is to just allow the warning on every call site (changeset link). Even if each one of those 93 cases is a true warning, and there's indeed a bunch of deadcode in Fuchsia, the experience should somehow be nicer.

I think an ideal story would be something like this: John at MonorepoInc is tasked with upgrading rust compiler. The build after upgrade contains 128 new warnings. Upon examining a dozen of then, a cold sweat runs down John's spine, as every case does look like a potential issue. Nonetheless, John is able to proceed with upgrade by selectively allowing a new warning in the central build file. After the upgrade, John creates a series of tickets to fix the warning and re-enable it on per-subsystem basis. A week later, the warning is re-enabled globally, and the code a MonorepoInc contains no problematic code.

The problem with this change is that it's not a new lint, its an extension to an existing one, so you can't selectively disable just the bit about derive(Debug).

[scottmcm]

The problem with this change is that it's not a new lint, its an extension to an existing one

This seems like the core, to me. unused is already a lint group, so one possibility would be to have unused_field and unused_outside_derive as separate sub-lints, so one could be allowed in the build system during the upgrade to unblock.

[Thomasdezeeuw]

As another data point that is slightly different: I have a use case where I have a bunch of structs that are only written to and then logged using their (derived) fmt::Debug implementation and the warning still triggers. Basically the following code.

#[derive(Debug)]
struct S {
    f: usize,
}

fn main() {
    let s = S { f: 0 };
    println!("{:?}", s);
}

Would it be possible to make an exception for this case as the fields are actually read?

[est31]

Aren't the unused lints influencing each other? Right now, when I initialize an unused field of a struct with the result of a function, the function does not show up as unused. But often there is an avalanche of lints firing. E.g. when function foo calls bar, and foo is unused, then both foo and bar are shown as unused. So I'm not sure if splitting up the dead code lint group is a good strategy in general, although it might be helpful for the migration.

I think additions of allow like the one in the linked fuchsia commit could be automatted somehow, e.g. with rustfix (note that it allowed the entire unused lint group, which wasn't actually neccessary. enabling dead_code would have been enough). Even if not, I don't think it's too harmful in general to allow lints like dead_code on a project wide basis if the lint became more strict during an upgrade, and then let sub-teams enable the lint again. Even if that means that some unrelated dead code might get undetected for a while, is it really a big deal?

If you add a lint to support some migration, can you remove it after a while? IIRC the stability rules allow removal of lints, right? It has to land in stable because some users never use nightly and those should have the same level of comfort during a compiler upgrade.

[tmandry]

Thoughts about blast radius: it's very clearly suboptimal if the rollout path for big consumers like Fuchsia is to just allow the warning on every call site (changeset link). Even if each one of those 93 cases is a true warning, and there's indeed a bunch of deadcode in Fuchsia, the experience should somehow be nicer.

We ended up adding a comment above every #[allow(unused)] with a link to a bug describing what to do. It was a fairly time consuming issue to deal with.

Also, the true number of cases ended up being ~150.

With regard to -Dwarnings, it's not really a choice in a large monorepo. Either warnings are denied or developers get overwhelmed with warnings from code they don't control each time they build.

[nikomatsakis]

Side note that it is possible to put the #[allow] on the field itself, which feels very precise:

#[derive(Debug)]
struct Foo {
    #[allow(dead_code)]
    x: u32
}

fn main() {
    let _foo = Foo { x: 22 };
}

I realize it's still work to add those lints, I wonder if we could make this a "cargo fix"'able thing.

Playground

(I think this was suggested before, but I wanted to note it anyway :)

[nikomatsakis]

A few more expansive thoughts here.

There are two questions at play. First, was the decision to expand dead_code the right one?

• I still think that, on balance, the decision to make a warning here was the right one. There are lots of ways that things which appear Dead may not be, and we've seen a lot of evidence that derive(Debug) winds up making fields appear used which the authors would not consider used.
• That said, it will obviously have false warnings, and it is good to have a nice way to identify and silence those. I personally feel that tagging the field with allow is tolerable, but I recognize that other people don't like that. I also recognize that doing that for 150+ fields is no fun.
• In general, I think it's great that we evolve lints to fix bugs and be more precise. I think Rust would not be well served by "freezing" lint definitions for all time. This is why we've engineered things like "cap-lints".
• I've found that the dead-code lint in particular often catches bugs for me, often of the variety ("wait, why is that dead? I should definitely be reading that field...."), so I believe that having it be as aggressive as we can, while annoying is an active contributor to Rust's feeling of reliability.

This brings us to the second question: can we find less disruptive ways of rolling these lint changes out?

• It seems obviously bad that us making an improvement here broke bevy's CI in a difficult way. It's also clear that bevy would like to guarantee a warning-free codebase. There is probably some work to identify the best way to configure CI to get the intended effect while giving time to respond to changes like this without massive disruption.
• Similarly, it is too bad that Fuchsia had to make a bunch of manual edits. No fun. (I suspect, though, that a substantial fraction of those 150+ fields are in fact dead code...).
• It seems clear that the way this roll-out played out in practice worked against Productivity, in short, and I think we can probably do better!

All that said, I'm not sure what the best fix is. One option might be having some version of cargo fix that can be used to "fixup" warnings. 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). Another might be making a documented (and not too painful) CI workflow for bevy that would let them "become aware" of changes like this without the need to immediately act on them (@Mark-Simulacrum suggested pinning nightlies as one possibility).

[tmandry]

Making lints fixable with #[allow] attributes would be a huge help! At that point we'd just want to add a comment with an explanation or link to a bug. (Maybe that can be a command-line option, but it's also pretty easy to script outside the tool.)

[Mark-Simulacrum]

Closing -- we're not expecting further action here. Per Niko's comment, we're not expecting further action on this particular issue but it seems like there's probably more work to do around better rollouts for similar changes in the future.

[matklad]

there's probably more work to do around better rollouts for similar changes in the future.

I wonder if we could write a test which gives us a heads up when we start emitting significantly more warnings by default? I suspected that rollout of this would not be perfectly smooth and suggested that t-compiler looks at it. There was a lot of attention to the PR indeed, and we knew that in Rustc we had to fix quite a few cases.

So it's not that we didn't have data about the impact, it's just that nobody explicitly posed a question of "are we sure that emiting a bunch of new true warnings is OK without providing cargo fix/lint group".

OTOH, I feel that such cases are going to be pretty rare, so perhaps we should just store this as a bit of tribal knowledge.

[Turbo87]

FWIW the crates.io codebase also ran into this issue (see https://github.com/rust-lang/crates.io/pull/4030), though luckily only in a single case.

It still feels strange to me to exchange false-negatives for false-positives though. This might be naive, but how hard would it be to disable the lint if we detect that the Debug impl itself is actually used somewhere?

[Turbo87]

alternatively, if we want to keep it, it would be helpful for everyone that is surprised by this new behavior if the compiler added a hint like:

These fields appear to only be used by the derived Debug implementation. If you want to keep the fields and disable the warning you can annotate them with #[allow(dead_code)].

[Ten0]

You never read the field a. It is used in the Debug output, yes, but not actively in your code.

That is not correct for us either. Our use-case is that the debug output of some of these fields is purposely used within production applications:

• As error context to Sentry events, to help developers with some debugging context in case of error
• In gRPC Status message, again to help developers understand errors returned by the application in case of invalid usage.

• 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.

It seems reasonable that this is a warning when the Debug implementation is never used. However, when it's actually used, it seems to be incorrect, as per the existence of the use-case above.

• I still think that, on balance, the decision to make a warning here was the right one. There are lots of ways that things which appear Dead may not be, and we've seen a lot of evidence that derive(Debug) winds up making fields appear used which the authors would not consider used.

I would agree with that. How about having a separate lint - close to dead code, for these kind of implementations? Projects that make heavy use of this could silence that particular lint, with others still benefiting of it by default.

[pmetzger]

Even if this warning is reasonable, it would be a good idea if the message explained that derive(Debug) is ignored for purposes of the lint. As it happens, I spent a lot of time tearing my hair out trying to figure out why the warning was being triggered. (In my case I have structs that exist only to be dumped to a log using debug prints.)

[jpalus]

Might this also be the reason why rust 1.57.0 fails to build itself?

error: field is never read: `id`
   --> src/bootstrap/lib.rs:280:5
    |
280 |     id: String,
    |     ^^^^^^^^^^
    |
    = note: `-D dead-code` implied by `-D warnings`

error: could not compile `bootstrap` due to previous error

[est31]

@jpalus yeah looks like it. FTR the commit that fixed it was aca8bcb48feca8c87b9af4e440835992d3f6d470, so you might want to backport it.

[peterjoel]

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.

[lexi-brt]

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.

[hellow554]

@lexi-brt that's what Display is for.

[d-sauter]

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.