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.

[traviscross]

@rustbot labels +T-lang +I-lang-nominated

Nominating so we can discuss the proposal to mention PhantomData in the help message, both for field and tuple structs.

[joshtriplett]

We talked about this in today's @rust-lang/lang meeting. Some of us were generally skeptical about making this suggestion in the general case. At most, we felt that it might be worth adding a note to the default suggestion if the removal would actually affect the auto traits of the struct (if we can tell that). If it wouldn't affect the auto traits we should preserve the suggestion. If it would, we could add a note like "Note that this removal will make the struct Send; if you don't want that, consider PhantomData."

[traviscross]

@rustbot labels -I-lang-nominated

In the end, there seemed general consensus to suggest PhantomData only if we could do it only when removing the field would change the auto traits of the struct.

Let's go ahead and unnominate the issue. If a PR is filed to add the suggestion to the lint in this manner, please nominate that for T-lang.

[compiler-errors]

At most, we felt that it might be worth adding a note to the default suggestion if the removal would actually affect the auto traits of the struct (if we can tell that).

I don't believe it's possible in general to detect whether or not the auto traits of a struct are going to change in the presence of generics, normalizable associated types, and the fact that the list of auto-traits is not hard-coded into the compiler 😓

[tmandry]

Generics are another valid use case for PhantomData, making them used when they would otherwise be unused. Otherwise most uses of PhantomData to affect auto traits don't depend on generics in my experience.

My proposal would be: If we had a list of auto traits, we could ask whether the type implements each of those auto traits.

• If the answer is definitely no for any auto trait, suggest PhantomData.
• If the answer is ambiguous for any auto trait, suggest PhantomData. This ensures that auto traits will definitely be preserved under (one of) the compiler suggestions.
• If the answer is definitely yes for all auto traits, elide the PhantomData suggestion.

[clechasseur]

I see that implementations of Drop are mentioned... this code warns on Nightly but not on Stable:

struct Foo(&'static ());

impl Foo {
    fn new() -> Self {
        Self(&())
    }
}

impl Drop for Foo {
    fn drop(&mut self) {
        println!("Foo dropped: {:?}", self.0);
    }
}

struct Bar(Foo);

impl Bar {
    fn new() -> Self {
        Self(Foo::new())
    }
}

fn main() {
    let _ = Bar::new();
}

playground

In this case, removing the field causes the Drop not to work properly. Is there a way to avoid the warning while keeping the field so it is correctly Droped? If I used a named field, I could prefix the field name with _ to avoid the warning, but I don't know if there's a way to do that for fields of a tuple struct.

[glandium]

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.

PhantomData doesn't work as an alternative in the case where the field type has a Drop impl (e.g. in the example from #119645 (comment))

Note this is now a stable-to-beta regression.

[workingjubilee]

@clechasseur

In this case, removing the field causes the Drop not to work properly. Is there a way to avoid the warning while keeping the field so it is correctly Droped? If I used a named field, I could prefix the field name with _ to avoid the warning, but I don't know if there's a way to do that for fields of a tuple struct.

You can simply #[allow(dead_code)] on the struct.

[clechasseur]

You can simply #[allow(dead_code)] on the struct.

Thanks, it's good to know! If this makes it into stable, I guess that's what I'll end up doing, although I still feel like the suggestion is a bid misleading (the warning text mentions that the field is "never read", which is true, but suggesting to remove the field or change its type actually changes the behavior).