dead_code in 1.77 lints against tuple fields with Drop implementations and suggests removing them

[awused]

Code

I tried this code:

struct SignificantDrop {
    a: usize,
}

impl Drop for SignificantDrop {
    fn drop(&mut self) {
        println!("Doing something important to {}", self.a);
    }
}                        

struct OwnsResource(usize, SignificantDrop);              

fn main() {
    let s = OwnsResource(17, SignificantDrop { a: 999 }); 

    println!("{}", s.0);
}

Current output

field `1` is never read
`#[warn(dead_code)]` on by default (rustc dead_code)
─────────────────────────────────────────────────────────────────────────────
field in this struct (rustc dead_code)
─────────────────────────────────────────────────────────────────────────────
consider changing the field to be of unit type to suppress this warning while
preserving the field numbering, or remove the field: `()` (rustc dead_code)

Desired output

If the unused field has a drop implementation it should at least not suggest removing it entirely. I'm not sure what the best option is. For private types converting it to a struct and naming the field with an underscore works, but that doesn't work for public types.

Rationale and extra context

I use this pattern in a few cases in GUI code to ensure that an event handler is cleaned up when the containing tuple gets dropped. I can avoid the finding, but if I had just accepted the suggestion I'd have accidentally cleaned up my event handlers.

Other cases

For a struct the output is a more reasonable field b is never read (rustc dead_code) without a suggestion to remove it. I think this could, still, benefit from detecting if Drop is implemented since it might have side effects.

struct OwnsResourceTwo {
    a: usize,             
    b: SignificantDrop,   
}

Rust Version

rustc 1.77.0 (aedd173a2 2024-03-17)
binary: rustc
commit-hash: aedd173a2c086e558c2b66d3743b344f977621a7
commit-date: 2024-03-17
host: x86_64-unknown-linux-gnu
release: 1.77.0
LLVM version: 17.0.6

Anything else?

No response

[lsunsi]

I also got this on my project, several structs holding MutexGuard's were flagged and I didn't know how to please the compiler but using allow[dead_code], which feels like the wrong thing to do since the code is not dead.

[jeckersb]

Thanks for writing this up, I'm seeing this as well on 1.77, except instead of a tuple-struct it's with a tuple-style enum member like:

enum Foo {
  Bar(Option<DroppableType>),
}

[workingjubilee]

This is currently consistent across struct kinds in 1.77:

struct SignificantDrop {
    a: usize,
}

struct AnythingElse {
    a: u8,
    b: SignificantDrop,
}

impl Drop for SignificantDrop {
    fn drop(&mut self) {
        println!("Doing something important to {}", self.a);
    }
}

struct OwnsResource(u8, SignificantDrop);

fn main() {
    let s = OwnsResource(17, SignificantDrop { a: 999 });
    let b = AnythingElse {
        a: 18,
        b: SignificantDrop { a: 1000 },
    };

    println!("{}", s.0);
    println!("{}", b.a);
}

The fact that you were able to use tuple structs, specifically, to squirm around this, was a bug. Not in the sense that it should be linting on this, but that it should be linting or not linting consistently regardless of whether the fields are given letters or numbers to name them.

[workingjubilee]

In that sense, this is arguably a duplicate of #112290 but I will leave this issue open because I expect it to accumulate quite a few more comments in any case, and people will open a new copy of this issue if I do close it, so.

[khvzak]

Thanks for keeping the issue open.
After upgrading Rust to 1.77 I've got many false positives in mlua as well.
In my case inner field implements Drop that is significant.

Consider the following example:

struct MyUserdata(Arc<()>);

let rc = Arc::new(());

// Move userdata into Lua
lua.globals.set("userdata", MyUserdata(rc.clone()));
assert_eq!(Arc::strong_count(&rc), 2);
lua.globals.remove("userdata");
lua.gc_collect();
 
// Check that userdata is dropped
assert_eq!(Arc::strong_count(&rc), 1);

Using allow[dead_code] is not the best option as code is obviously not dead.

[clechasseur]

I believe this is also the same issue as #119645.

[workingjubilee]

@khvzak This is a question with a deeply subjective answer, but:

You say, as I understand it, that #[allow(dead_code)] is confusing because you consider the code to be "not-dead". But that doesn't answer the question of whether you would be okay with using #[allow(some_other_lint)]. If there was a lint named, say, drop_only_field, would you prefer that? i.e. to write something like

struct MyUserdata(
    #[allow(drop_only_field)] Arc<()>,
);

I am asking because I have not formed an opinion yet on whether the lint is correct to, in fact, lint at all, or whether this should be indicated some other way.

[workingjubilee]

@clechasseur #119645 is subtly different because it's about "auto traits", whereas Drop is very much not an auto trait, and indeed "whether dropping this type calls significant drop glue or not" is not really indicated via the trait system (except in a recursion-on-fields way, and in the negation way indicated by T: Copy).

[clechasseur]

@clechasseur #119645 is subtly different because it's about "auto traits", whereas Drop is very much not an auto trait, and indeed "whether dropping this type calls significant drop glue or not" is not really indicated via the trait system (except in a recursion-on-fields way, and in the negation way indicated by T: Copy).

It was me that was confused then, because I posted about the Drop behavior in that other issue - sorry about that.

If there was a lint named, say, drop_only_field, would you prefer that?

If I may offer my opinion: I don't think it would be better (IMHO). I realize that conceptually, lints applied to structs with named fields should also apply to structs with unnamed fields. However, in the case of Drop-only fields, if the field has a name, there's an easy way out that is generally well-understood (prepend an underscore to the name). For unnamed fields however, that's not possible so you have to silence the lint "manually". This in fact makes the experience of structs with named fields different from the experience of structs with unnamed fields.

[khvzak]

But that doesn't answer the question of whether you would be okay with using #[allow(some_other_lint)].

I discovered that adding #[allow(unused)] suppresses the warning as well and I'm okay with it.

I am asking because I have not formed an opinion yet on whether the lint is correct to, in fact, lint at all, or whether this should be indicated some other way.

What is really confusing, is the lint advice:

help: consider changing the field to be of unit type to suppress this warning while preserving the field numbering, or remove the field

both suggested options are wrong as would break the program.

[iazel]

I'm experiencing a similar issue, even though it isn't directly related to Drop trait, but rather to how I handle errors.

Imagine having something like:

#[derive(Debug)]
struct ErrorA;
fn doA() -> Result<(), ErrorA> {
    todo!()
}

#[derive(Debug)]
struct ErrorB;
fn doB() -> Result<(), ErrorB> {
    todo!()
}

#[derive(Debug)]
enum ErrorAB {
    A(ErrorA),
    B(ErrorB),
}
fn doAB() -> Result<(), ErrorAB> {
    doA().map_err(ErrorAB::A)?;
    doB().map_err(ErrorAB::B)?;
}

I'm only interested in ErrorA and ErrorB for log purposes, so I never really read them except when I display them in their debug representation.

With the new change, it will complain that ErrorAB variants' arguments are never read, which is technically true, however using allow(dead_code) will cause issues during refactoring, because I may remove some error variant but forget to remove it in the enum.

It would be best if we could introduce allow(never_read) or something similar, that will trigger if and only if the value is never read. In case the value is never used (no read and no write), then dead_code should be triggered. In the end, dead_code could be just a composition of never_read and never_wrote.

I believe this arrangement may be a good solution also for the issue described here. What do you think?

[Danvil]

@workingjubilee Even if a field has a custom drop, it might still be an oversight to not use it anywhere else. dead_code feels like a very strong statement. IMHO I would be fine with a lint like "field_not_read" with a message like "This field is never read, except for a potential custom Drop". Two separate lints are of course also an option but might introduce more noise.

[awused]

dead_code is even more eager in 1.79, it's now hitting some error enums that only really exist so that I can debug failures - so the fact that the error was only read in the Debug implementation was intended. It's getting pretty annoying.