New lint: drop_for_static

[Mr-Leshiy]

Description

This PR introduces a new lint to notify users when a static item contains a type that implements the Drop trait.

As per the Rust Reference, static variables have a 'static lifetime and are never destroyed. Consequently, theDrop::drop method is never executed for these items.

Rational

In scenarios where drop is used for critical resource management. Such as closing file handles, releasing external locks, or performing a "graceful" shutdown of a subsystem, relying on a static item can lead to subtle resource leaks or unexpected behaviour. Since the programmer might expect the destructor to run at the end of the program's execution, providing a diagnostic when this won't happen helps prevent "silent" failures in resource cleanup.

changelog: new lint: drop_for_static

[rustbot]

r? @dswij

rustbot has assigned @dswij.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[Jarcho]

This could be implemented as just:

if let ItemKind::Static(_, ident, ..) = item.kind
  && cx.tcx.type_of(item.owner_id.def_id).instantiate_identity().has_drop(cx.tcx, cx.typing_env())
{
  // lint
}

Note you should be using the ident's span when emitting the lint to avoid an overly large span being printed with the diagnostic.

[Mr-Leshiy]

@Jarcho thank you for your quick response and suggestions !

Just tried your approach, but it does work recursively as I originally intended.
It catches only this scenario:

static A1: FooWithDrop = FooWithDrop;

and does not identify the presence of the Drop implementation in the rest of the scenarios which I've added as a test cases:

static A3: &FooWithDrop = &FooWithDrop;
static A5: (FooWithoutDrop, FooWithDrop) = (FooWithoutDrop, FooWithDrop);
static A7: [FooWithDrop; 1] = [FooWithDrop];
static A9: &[FooWithDrop] = &[FooWithDrop];
etc.

Suggested implementation.

if let ItemKind::Static(_, _, _, _) = item.kind
    && let ty = cx.tcx.type_of(item.owner_id.def_id).instantiate_identity()
    && has_drop(cx, ty)
{
   // lint
}

Or maybe I am missing something.

[ada4a]

It could be nice to store the span of at least the first such type, so that the final lint message could put a label¹ on it -- something like:

error: static items with drop implementation
  --> tests/ui/drop_for_static.rs:36:8
   |
LL | static B1: Nested<FooWithDrop> = Nested(FooWithDrop, ());
   |        ^^         ^^^^^^^^^^^ type with drop implementation here

Or maybe just nest all the way and collect at all of them -- after all, that's what we end up doing when there isn't a type-with-drop. This would reduce the annoyance of the lint triggering again after you fix it

Footnotes

1. using Diag::span_label ↩

[Mr-Leshiy]

Thank you for suggestion !
Already applied it.
But using span_label makes it like

LL | static A1: FooWithDrop = FooWithDrop;
   |        ^^  ----------- type with drop implementation here

Have not found an alternative to print exactly how you've mentioned.

[ada4a]

Well done!

Don't worry about the underline type, I just misremembered it:)

Or maybe just nest all the way and collect at all of them -- after all, that's what we end up doing when there isn't a type-with-drop. This would reduce the annoyance of the lint triggering again after you fix it

I still think this would be a good idea.. let's see what the maintainers think

[Mr-Leshiy]

Yea, I was also thinking about that. And didn't do that because was concerned to not bee too annoying with the lint messages.
But in any case would be happy to add it as you mentioned.

[ada4a]

One small thing, otherwise LGTM:)

Now we just need to wait for an actual maintainer

View changes since this review

[Jarcho]

Sorry, had had the wrong name. You want needs_drop. This won't catch references, but I don't think you want to do that in the first place since they don't do anything when dropped..

[Mr-Leshiy]

@Jarcho got it, will take a look how to use needs_drop.

This won't catch references, but I don't think you want to do that in the first place since they don't do anything when dropped..

I am not entirely agree, because it could be the following scenario, for which one we want to emit lint in my opinion.

struct FooWithDrop;

impl Drop for FooWithDrop {
    fn drop(&mut self) {}
}

static A: &FooWithDrop = &FooWithDrop;

[dswij]

r? clippy

[github-actions]

Lintcheck changes for 2489ce9

Lint	Added	Removed	Changed
clippy::drop_for_static	14	0	0

This comment will be updated if you push new changes

[Jarcho]

struct FooWithDrop;

impl Drop for FooWithDrop {
    fn drop(&mut self) {}
}

static A: &FooWithDrop = &FooWithDrop;

This case is not due to the type of the static, but due to lifetime extension in the initializer's body. Currently you'll lint on things like static FOO: Mutex<Option<&NeedsDrop>> = Mutex::new(None); which would be a false positive. Antoher false positive would be:

static FOO: NeedsDrop = NeedsDrop; // should lint
static FOO_REF: &NeedsDrop = &FOO; // shouldn't lint

Linting undropped temporaries in the initializer is fine, but that should probably be it's own lint.

[Mr-Leshiy]

@Jarcho I see - agree, missed that.
Updating implementation ...

[Mr-Leshiy]

@Jarcho so just to clarify
In these scenarios we want raise a lint

static A1: FooWithDrop = FooWithDrop;
static A2: (FooWithoutDrop, FooWithDrop) = (FooWithoutDrop, FooWithDrop);
static A3: [FooWithDrop; 1] = [FooWithDrop];
static A4: Nested<FooWithDrop> = Nested(FooWithDrop, ());
static A5: Nested<FooWithoutDrop, Nested<FooWithDrop>> = Nested(FooWithoutDrop, Nested(FooWithDrop, ()));
static A6: Nested<(FooWithoutDrop, FooWithDrop)> = Nested((FooWithoutDrop, FooWithDrop), ());
static A7: Nested<[FooWithDrop; 1]> = Nested([FooWithDrop], ());

And in these we dont

static A1: &FooWithDrop = &FooWithDrop;
static A2: &[FooWithDrop] = &[FooWithDrop];