Improve wording of the drop_bounds lint

[FabianWolff]

This PR addresses #86653. The issue is sort of a false positive of the drop_bounds lint, but I would argue that the best solution for #86653 is simply a rewording of the warning message and lint description, because even if the lint is technically wrong, it still forces the programmer to think about what they are doing, and they can always use #[allow(drop_bounds)] if they think that they really need the Drop bound.

There are two issues with the current warning message and lint description:

• First, it says that Drop bounds are "useless", which is technically incorrect because they actually do have the effect of allowing you e.g. to call methods that also have a Drop bound on their generic arguments for some reason. I have changed the wording to emphasize not that the bound is "useless", but that it is most likely not what was intended.
• Second, it claims that std::mem::needs_drop detects whether a type has a destructor. But I think this is also technically wrong: The Drop bound says whether the type has a destructor or not, whereas std::mem::needs_drop also takes nested types with destructors into account, even if the top-level type does not itself have one (although I'm not 100% sure about the exact terminology here, i.e. whether the "drop glue" of the top-level type counts as a destructor or not).

cc @jonhoo, does this solve the issue for you?

r? @GuillaumeGomez

[GuillaumeGomez]

Like you said, it doesn't solve the issue, simply improve the wording. Emitting a warning for a perfectly valid code is always an error on the lint end. We can merge this rewording as is, but we still need to address the lint issue.

[FabianWolff]

Emitting a warning for a perfectly valid code is always an error on the lint end.

That's debatable, at the very least. For instance, the trivial_bounds, dead_code, and unused_* lints all warn about code that is technically correct but looks odd/as if something else was intended. But I agree that claiming to "fix" the issue was too ambitious; I have reworded the PR description accordingly.

I have also forgotten to bless the one UI test affected by my changes; this should be fixed now.

[jonhoo]

The whole suggestion is weird to me — needs_drop is very different from T: Drop since it operates as an expression, not as a type bound. I realize they mean very different things too, but I wonder just how often a user can actually follow the suggestion to use needs_drop instead?

I also agree with @GuillaumeGomez here that this is an error in the lint — we should not suggest that the user did not mean this unless we are quite certain that is the case. I'm curious how common it is in practice for someone to add a Drop bound in cases where it truly isn't necessary — my intuition would be that people rarely add a : Drop bound unless they it useful in their particular situation. In which case the lint would be more often wrong than right.

[notriddle]

First, it says that Drop bounds are "useless", which is technically incorrect because they actually do have the effect of allowing you e.g. to call methods that also have a Drop bound on their generic arguments for some reason.

That really is passing the buck, though. Why does the function need Drop bounds?

[FabianWolff]

First, it says that Drop bounds are "useless", which is technically incorrect because they actually do have the effect of allowing you e.g. to call methods that also have a Drop bound on their generic arguments for some reason.

That really is passing the buck, though. Why does the function need Drop bounds?

In principle, I agree. But to quote from #86653:

But, the Drop bound is not useless here — removing it causes the code to no longer compile.

i.e. in this particular sense of "usefulness", the Drop bound is "useful", which is why I have suggested to change the wording of the lint to not mention this vague notion of utility.

[notriddle]

I don’t think the bounds imposed on the trait implementation are the problem, but the function is. You could write it like this, instead, at no loss of power:

trait Placeholder {}
impl<T> Placeholder for T {}
pub fn bar(_: *mut dyn Placeholder) {}

Which means the lint should probably warn on *mut dyn Drop, since this is a code smell just like T: Drop is.

[jonhoo]

I'm not sure I follow why *mut dyn Drop is code smell here — if the only thing that matters in a particular context is that it can be dropped (which all types can), then isn't *mut dyn Drop exactly the right type? Injecting an extra trait + blanket implementation as you suggest shouldn't be necessary, and only adds unnecessary complexity.

[notriddle]

The problem is that types might be droppable without implementing Drop. The standard String type is an example.

The trait with a blanket impl doesn’t have this problem. String doesn’t implement Drop, but it does implement Placeholder.

[jonhoo]

That's a good point. I still feels unfortunate though. In some sense, it feels like any type should be castable to a dyn Drop, since (from my understanding) all types have Drop::drop in their vtables when used as trait objects, even if they don't directly implement Drop.

[jackh726]

@FabianWolff @GuillaumeGomez What's the status of this?

[GuillaumeGomez]

The initial problem I pointed out is still there.

[FabianWolff]

The initial problem I pointed out is still there.

And so is my objection to it. With #86848, both the function and the trait from the example in #86653 now get a warning — rightly so, because both uses of the Drop bound look fishy, and in any case, I think it would be pretty difficult for the drop_bounds lint to figure out whether the Drop bound in the trait is necessary to satisfy some other trait bound somewhere.

My rewording tries to reflect this (without even claiming to fix #86653) by making the explanatory text for the lint slightly more conservative and less focused on the "usefulness" of a Drop bound.

@rustbot label: +S-waiting-on-review -S-waiting-on-author

[GuillaumeGomez]

Indeed, my bad.

@bors: r+ rollup

[bors]

📌 Commit 644529b has been approved by GuillaumeGomez

[arxanas]

(Commenting on this issue and not #86848, because it seems to be where the relevant parties are.)

Here's my code example that hits this, in which I conditionally return a guard object or a TrivialDrop object:

https://github.com/arxanas/git-branchless/blob/c92f939b038b7f941144de1c649a90e9a34df14e/src/main.rs#L434-L451

        match filename {
            Some(filename) => {
                let (layer, flush_guard) = ChromeLayerBuilder::new()
                    .file(filename)
                    .include_args(should_include_function_args)
                    .build();
                (Some(layer), Box::new(flush_guard))
            }
            None => {
                struct TrivialDrop;
                impl Drop for TrivialDrop {
                    fn drop(&mut self) {
                        // Do nothing.
                    }
                }
                (None, Box::new(TrivialDrop))
            }
        }

Warning message

I'm not sure if I'm missing something, but the warning message doesn't mention creating a placeholder trait. There's an explanation for the lint in the source code, but I don't know the error code, and it doesn't appear in the error index, and the compiler output message doesn't say how to explain the lint (playground):

warning: types that do not implement `Drop` can still have drop glue, consider instead using `std::mem::needs_drop` to detect whether a type is trivially dropped
 --> src/main.rs:8:32
  |
8 | fn return_a_guard() -> Box<dyn Drop> {
  |                                ^^^^
  |
  = note: `#[warn(dyn_drop)]` on by default

So it was pretty hard to figure out what the issue was.

I also think the dyn_drop lint should be worded something like this: "types which don't implement Drop can still contain members which implement Drop, consider instead using std::mem::needs_drop to decide if a type is trivially-dropped". Unfortunately, it's still not clear what trait bound I can use, but at least it's clear that the warning message doesn't apply to my case (and there should be a link to the full explanation somewhere).

Output position?

Should the dyn_drop lint really apply to dyn Drops which appear in output position? I argue that it shouldn't.

1. The output type doesn't need to "accept" an entire class of input types, in contrast to the discussion in the lint justification. The caller can check neither at compile-time nor at run-time if the type is trivially-droppable.

2. In my case, I feel like the code is a lot clearer with dyn Drop than with a placeholder trait, since it's obvious that it represents some scope guard. In the commit message for dbd4fd5, it's mentioned that you might want to make a placeholder trait, e.g. for a deferred drop implementation. And the signature makes sense for the reader:

// Reader: okay, that makes sense. how do I make my resource into a thing that can defer dropping?
// Reader: *looks up definition of DeferredDrop*
// Reader: oh, okay, my resource can already defer dropping, and this is here for technical reasons
fn defer_drop(value: Box<dyn DeferredDrop>) { ... }

In my case, it's not really even the case that the drop should be "deferred". The caller is responsible for managing it, just like any other resource it manages, which is to say that it should probably be dropped at the end of the scope it's created in:

// Reader: okay, this clearly returns a scope guard
fn make_guard1() -> Box<dyn Drop> { ... }

// Reader: why is the drop deferred for this? is the lifetime of the guard supposed to outlast something?
// Reader: *looks up definition of DeferredDrop*
// Reader: oh, I misunderstood the signature to be more significant than it really is
//         (which could happen even if it's named something like ScopeGuard)
fn make_guard2() -> Box<dyn DeferredDrop> { ... }

In the first scenario, the reader has to do some research, but the signature makes sense on the first read. In the second scenario, the reader is initially surprised. I think we should avoid the scenario where the extra trait is not just boilerplate, but actively surprising.

3. Using a name like Placeholder or an existing trait like Any would be strictly less clear.

4. It does seem like you can currently write a function which returns impl Drop without issue, just not one that returns dyn Drop. Is this intentional (since you can currently only return one actual type) or an oversight?

// Doesn't trigger a warning.
fn returns_a_guard() -> impl Drop { ... }

[notriddle]

First of all, I recommend using impl Drop in your return slot. It can be combined with the Box<dyn Drop>, so you can still return two different Drop types, but this way you're exposing less implementation details in your API.

fn install_tracing() -> eyre::Result<impl Drop> {

    // You have to make sure you cast `as Box<dyn Drop>`, because Rust won't do it automatically
    Ok(Box::new(droppable) as Box<dyn Drop>)
}

Should the dyn_drop lint really apply to dyn Drops which appear in output position? I argue that it shouldn't.

But yes, it's an oversight. dyn Drop is really only bad in the input position, because it places pointless constraints on the API consumer. In the output position, it's a lot less harmful.

[arxanas]

Thanks for the tip. Returning impl Drop works, and I agree that it's better. Here's a playground example for the future reader.