dead_code lint shouldn't warn about unused fields that implement Drop

[simias]

I have a use case similar to this:

struct S(u32);

impl Drop for S {
    fn drop(&mut self) {
        println!("dropping {}", self.0);
    }
}

struct Container {
    s: S,
}

impl Container {
    fn new() -> Container {
        let s = S(4);

        Container { s: s } 
    }
}

fn main() {
    Container::new();
}

The struct S is just a thin wrapper around a foreign object, it's just used to free the object in the Drop. The dead_code lint says:

warning: struct field is never used: `s`, #[warn(dead_code)] on by default

IMHO that's bogus because it seems to imply that the member could be removed without changing the behaviour of the code which is clearly not the case. The whole point is that I want s to live as long as the Container object.

Therefore I think the lint shouldn't trigger when a seemingly unused field implements Drop.

[ghost]

This should go even further - the lint should not trigger if the field is of a type that hierarchically contains any fields implementing Drop.

[kmcallister]

Yeah I ran into this the other day, holding on to a mutex guard in a struct.

[kmcallister]

@jakub-: I think there's some "needs cleanup glue" predicate on types that we could query, since linting happens after type checking.

[llogiq]

@kmcallister We should be able to query the rustc::middle::ty::destructor_for_type map, which should work on both structs and enums. However, I'm not sure if it will pick up structs of which parts need a destructor or enums of which only some variants need a destructor.

[TimNN]

How would this interact with generics?

Let's say:

struct Container<T> {
    s: T,
}

Should this warn because T may not implement drop or should this not warn because T may implement drop?

[llogiq]

@TimNN: this lint checks at use site, so unless the code using it is generic itself, T will be replaced by some concrete type.

I think the lint should warn on fully generic instances, though possibly with another lint name and message.

[Manishearth]

Code:

rust/src/librustc/middle/dead.rs

Line 456 in be3d390

fn should_warn_about_field(&mut self, node: &hir::StructField_) -> bool {

(should_warn_about_field)

Useful methods: TyS::ty_adt_def, AdtDefData::has_dtor()

[cristicbz]

Isn't the right approach for this to just name the field _some_guard? This is similar to what one does inside a function let _guard = foo.lock() or whatever. Otherwise once could also argue that PhantomData fields shouldn't warn, but doing _dummy: PhantomData<T> instead is easy enough.

[simias]

@cristicbz that's reasonable IMO but the error message itself is still technically incorrect and potentially misleading.

[brson]

Discussion on IRC looks like it leans toward leaving the warning for Drop fields, but mentioning that _ prefix will quiet it. I also feel like this is a prudent, incremental improvement.

[cristicbz]

In general or only for Drop fields? I lean towards "in general".

[Manishearth]

Only for Drop fields. In general this will occur during development when warnings are okayish. And the allow() solution is already hinted at.

[cristicbz]

Sounds good, I'll have a go at fixing this then.

[steveklabnik]

Since new lints have a big impact on users of rustc, the policy is that they should go through the RFC process like other user-facing changes. As such, I'm going to give this one a close, but if anyone comes across this ticket and wants this lint, consider adding it to clippy and/or writing up an RFC. Thanks!