dead_code suggestion for unused fields does not account for changes to auto trait implementations

[taiki-e]

I tried this code:

use std::rc::Rc;

struct Pending(Rc<()>);

fn pending() -> Pending {
    Pending(Rc::new(()))
}

fn main() {
    let _ = pending();
}

(The original code: https://github.com/rust-lang/futures-rs/blob/72e7e397cdfe7574e9b5d8845cafefc6e5dda70a/futures-executor/tests/local_pool.rs#L13)

I expected to see this happen: no warning or a warning suggests Pending(PhantomData<Rc<()>>).

Instead, this happened:

A warning suggests Pending(()).

error: field `0` is never read
  --> futures-executor/tests/local_pool.rs:13:16
   |
13 | struct Pending(Rc<()>);
   |        ------- ^^^^^^
   |        |
   |        field in this struct
   |
   = note: `-D dead-code` implied by `-D warnings`
   = help: to override `-D warnings` add `#[allow(dead_code)]`
help: consider changing the field to be of unit type to suppress this warning while preserving the field numbering, or remove the field
   |
13 | struct Pending(());
   |                ~~

Changing Rc<()> to () changes auto trait implementations of Pending, so dead_code should not suggest such a code or should suggest both code (PhantomData<Rc<()>> and ()) with explanations.

Also, if it suggests PhantomData, it should also be noted that PhantomData is not able to propagate all auto-traits (e.g., Unpin). (although this is not a problem in the above case) EDIT: see #119645 (comment)

https://github.com/rust-lang/futures-rs/actions/runs/7428509435/job/20215931270

Meta

rustc --version --verbose:

rustc 1.77.0-nightly (595bc6f00 2024-01-05)
binary: rustc
commit-hash: 595bc6f00369475047538fdae1ff8cea692ac385
commit-date: 2024-01-05
host: aarch64-apple-darwin
release: 1.77.0-nightly
LLVM version: 17.0.6

---

This has been shown in the latest nightly (nightly-2024-01-06) since #118297 (cc @shepmaster). IMO, it should not be changed to be warned by default with such a lint with a wrong suggestion.

[compiler-errors]

For the record, we also warn on named fields, though I feel like the suggestion shouldn't be given if the field has a Drop implementation at least... PhantomData is a good alternate suggestion to make though, I guess.

[shepmaster]

FWIW, the lint does actually check for and allow PhantomData, I guess the warning just doesn't mention it (although the docs for the lint do).

[shepmaster]

we also warn on named fields

Right:

use std::rc::Rc;

struct Pending {
    a: Rc<()>,
}

fn pending() -> Pending {
    Pending { a: Rc::new(()) }
}

fn main() {
    let _ = pending();
}

warning: field `a` is never read
 --> src/main.rs:4:5
  |
3 | struct Pending {
  |        ------- field in this struct
4 |     a: Rc<()>,
  |     ^
  |
  = note: `#[warn(dead_code)]` on by default

The new code is consistent with the current behavior, so I think that it's a net step forward and I'd argue about it being a regression.

The question becomes what do we want to do next?

It seems like the longer prose about "PhantomData is not able to propagate all auto-traits" shouldn't go into the lint message because it feels too wordy and niche, but should certainly go into the lint docs. Or maybe it should go in the diagnostic? Maybe we get some outside diagnostics opinion?

It does seem like the named and tuple field lint text should be more similar though.

[compiler-errors]

It does seem like the named and tuple field lint text should be more similar though.

Yeah, I agree.

Also, if it suggests PhantomData, it should also be noted that PhantomData is not able to propagate all auto-traits (e.g., Unpin).

What do you mean by this? PhantomData<T> should implement (or not) all the auto traits of T. The trait solver specifically treats PhantomData<T> as if it were PhantomData<T>(T):

rust/compiler/rustc_trait_selection/src/solve/assembly/structural_traits.rs

Lines 76 to 77 in 595bc6f

	// For `PhantomData<T>`, we pass `T`.
	ty::Adt(def, args) if def.is_phantom_data() => Ok(vec![args.type_at(0)]),

[taiki-e]

What do you mean by this? PhantomData<T> should implement (or not) all the auto traits of T. The trait solver specifically treats PhantomData<T> as if it were PhantomData<T>(T):

Sorry, I thought PhantomData always implemented Unpin, but it did not (there was a PR for it but not merged: #62786). At least at this time, it seems to propagate all (stable?) auto-traits.