New lint: drop_for_static

[Mr-Leshiy]

View all comments

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 a634456

Lint	Added	Removed	Changed
clippy::drop_for_static	48	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];

[Mr-Leshiy]

@Jarcho updated implementation as you suggested, excluding raising a lint for references.

But want to add my final word on that one to my original position of raising a lint for references as well, because its still little bit concerns me.
I am totally agree that if include references we would have these false positives:

static FOO: Mutex<Option<&NeedsDrop>> = Mutex::new(None); // shouldn't lint
static FOO: NeedsDrop = NeedsDrop; // should lint
static FOO_REF: &NeedsDrop = &FOO; // shouldn't lint

But doing so, we are would miss to raise a lint for the following cases:

static FOO: Mutex<Option<&NeedsDrop>> = Mutex::new(Some(&NeedsDrop)); // should lint
static FOO_REF: &NeedsDrop = & NeedsDrop; // should lint

And in such circumstances, in my opinion, as a user, I would choose to sacrifice by raising false positives , but still catching problematic places. Because dealing with false positive is easy, you could just suppress it and that's it, and on the opposite not catching problematic place, could lead to far worse consequences.

On the other hand making a separate lint for just this case, not sure that it makes sense, because from my perspective its still tied up with the static. So it sounds like it should be a part of this lint as well.
Because such code is fully valid and we dont want to raise anything in this scenario.

let foo: &NeedsDrop = &NeedsDrop;

But I dont have a strong position here, to fight to the death 😊 Just wanted to raise my concerns and position about it as a active clippy user 😊.

[Mr-Leshiy]

@Jarcho @ada4a sorry for bothering you, but maybe you can help with it 🙏

I've been trying to raise a lint in such scenarios as well

trait FooTrait {
    type FooAssoc;
}

struct FooImpl;
impl FooTrait for FooImpl {
    type FooAssoc = FooWithDrop;
}

static FOO: <FooImpl as FooTrait>::FooAssoc = FooWithDrop; // should lint

fn main() {}

And I am trying to do so by effectively extend matching expression to precess DefKind:: AssocTy

if let Res::Def(... | DefKind::AssocTy, def_id) = path.res {
            let ty = self.cx.tcx.type_of(def_id).instantiate_identity();
            if has_drop(self.cx, ty) {
               ...
            } 
            ...
        }

but it becomes panicking inside type_of method with the following error

error: internal compiler error: /rustc-dev/eda76d9d1d133effbf7facb28168fd78d75fd434/compiler/rustc_hir_analysis/src/collect/type_of.rs:83:17: associated type missing default
 --> tests/ui/drop_for_static.rs:88:5
  |
LL |     type FooAssoc;

https://github.com/rust-lang/rust/blob/e96bb7e44fbcc23c1e6009e8d0ee8ab208668fb4/compiler/rustc_hir_analysis/src/collect/type_of.rs#L83

Is it a correct behaviour ?
I am asking because in this case, type_of is executed on the <FooImpl as FooTrait>::FooAssoc which is not actually an trait item, its a defined type which specified inside FooImpl and for which one needs_drop was returning true early on.
I was thinking that even in this scenario it should properly return a Ty for the provided def_id as it works properly for TyAlias.

So what I am trying to say, is it a bug inside type_of implementation ?
Or its a correct behaviour of this function and I am missing something ?

[Jarcho]

Once the static's type does not need to be dropped using the type is the wrong way to get the result you want. You want to check if the initializer contains a value which both had is lifetime extended to 'static and needs to be dropped.

You should be using Ty::needs_drop, not clippy_utils::ty::has_drop. You won't need the visitor this way.

[Mr-Leshiy]

@Jarcho Got it.
One final question.
In the documentation it says

is definitely non-copy and might have a destructor attached
https://doc.rust-lang.org/beta/nightly-rustc/rustc_middle/ty/struct.Ty.html#method.needs_drop

Could it be an issue that it could return false positive ? Or its not for this case ?

[Jarcho]

Not in this case. The "might" part has to do with generic parameters which isn't a problem for statics.

[Mr-Leshiy]

@Jarcho updated

[ada4a]

Fwiw the visitor allowed us to pinpoint the exact part of the type that has Drop, which made the diagnostics nicer imo. You can see the lost labels on the latest diff. WDYT @Jarcho?

[Jarcho]

That doesn't work in general. Foo<T> may need to be dropped because of T or some other reason. There's no way to tell from a HIR visitor. Tuples are the only case you can determine and it's not going to be common to have a tuple as a static and not be aware of which element causes it.

[ada4a]

Ok that's fair. Oh well

[Mr-Leshiy]

@Jarcho @ada4a
Dont want to annoying, so sorry if I am bothering you with that.
Curious, what in general would be the process for such new lint to be added int the stable version, I could assume it should pass some testing and would be a while under the unstable feature.

[Mr-Leshiy]

@Jarcho @flip1995
Friendly reminder about this PR.

[rustbot]

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[Mr-Leshiy]

@Jarcho @flip1995
Friendly reminder about this PR.