Rollup merge of #86747 - FabianWolff:issue-86653, r=GuillaumeGomez

Improve wording of the `drop_bounds` lint

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`

Author: GuillaumeGomez

compiler/rustc_lint/src/traits.rs

@@ -18,23 +18,27 @@ declare_lint! {
     ///
     /// ### Explanation
     ///
-    /// `Drop` bounds do not really accomplish anything. A type may have
-    /// compiler-generated drop glue without implementing the `Drop` trait
-    /// itself. The `Drop` trait also only has one method, `Drop::drop`, and
-    /// that function is by fiat not callable in user code. So there is really
-    /// no use case for using `Drop` in trait bounds.
+    /// A generic trait bound of the form `T: Drop` is most likely misleading
+    /// and not what the programmer intended (they probably should have used
+    /// `std::mem::needs_drop` instead).
     ///
-    /// The most likely use case of a drop bound is to distinguish between
-    /// types that have destructors and types that don't. Combined with
-    /// specialization, a naive coder would write an implementation that
-    /// assumed a type could be trivially dropped, then write a specialization
-    /// for `T: Drop` that actually calls the destructor. Except that doing so
-    /// is not correct; String, for example, doesn't actually implement Drop,
-    /// but because String contains a Vec, assuming it can be trivially dropped
-    /// will leak memory.
+    /// `Drop` bounds do not actually indicate whether a type can be trivially
+    /// dropped or not, because a composite type containing `Drop` types does
+    /// not necessarily implement `Drop` itself. Naïvely, one might be tempted
+    /// to write an implementation that assumes that a type can be trivially
+    /// dropped while also supplying a specialization for `T: Drop` that
+    /// actually calls the destructor. However, this breaks down e.g. when `T`
+    /// is `String`, which does not implement `Drop` itself but contains a
+    /// `Vec`, which does implement `Drop`, so assuming `T` can be trivially
+    /// dropped would lead to a memory leak here.
+    ///
+    /// Furthermore, the `Drop` trait only contains one method, `Drop::drop`,
+    /// which may not be called explicitly in user code (`E0040`), so there is
+    /// really no use case for using `Drop` in trait bounds, save perhaps for
+    /// some obscure corner cases, which can use `#[allow(drop_bounds)]`.
     pub DROP_BOUNDS,
     Warn,
-    "bounds of the form `T: Drop` are useless"
+    "bounds of the form `T: Drop` are most likely incorrect"
 }
 
 declare_lint! {
@@ -102,8 +106,8 @@ impl<'tcx> LateLintPass<'tcx> for DropTraitConstraints {
                         None => return,
                     };
                     let msg = format!(
-                        "bounds on `{}` are useless, consider instead \
-                         using `{}` to detect if a type has a destructor",
+                        "bounds on `{}` are most likely incorrect, consider instead \
+                         using `{}` to detect whether a type can be trivially dropped",
                         predicate,
                         cx.tcx.def_path_str(needs_drop)
                     );

src/test/ui/drop-bounds/drop-bounds.stderr

@@ -1,4 +1,4 @@
-error: bounds on `T: Drop` are useless, consider instead using `std::mem::needs_drop` to detect if a type has a destructor
+error: bounds on `T: Drop` are most likely incorrect, consider instead using `std::mem::needs_drop` to detect whether a type can be trivially dropped
   --> $DIR/drop-bounds.rs:2:11
    |
 LL | fn foo<T: Drop>() {}
@@ -10,37 +10,37 @@ note: the lint level is defined here
 LL | #![deny(drop_bounds)]
    |         ^^^^^^^^^^^
 
-error: bounds on `U: Drop` are useless, consider instead using `std::mem::needs_drop` to detect if a type has a destructor
+error: bounds on `U: Drop` are most likely incorrect, consider instead using `std::mem::needs_drop` to detect whether a type can be trivially dropped
   --> $DIR/drop-bounds.rs:5:8
    |
 LL |     U: Drop,
    |        ^^^^
 
-error: bounds on `impl Drop: Drop` are useless, consider instead using `std::mem::needs_drop` to detect if a type has a destructor
+error: bounds on `impl Drop: Drop` are most likely incorrect, consider instead using `std::mem::needs_drop` to detect whether a type can be trivially dropped
   --> $DIR/drop-bounds.rs:8:17
    |
 LL | fn baz(_x: impl Drop) {}
    |                 ^^^^
 
-error: bounds on `T: Drop` are useless, consider instead using `std::mem::needs_drop` to detect if a type has a destructor
+error: bounds on `T: Drop` are most likely incorrect, consider instead using `std::mem::needs_drop` to detect whether a type can be trivially dropped
   --> $DIR/drop-bounds.rs:9:15
    |
 LL | struct Foo<T: Drop> {
    |               ^^^^
 
-error: bounds on `U: Drop` are useless, consider instead using `std::mem::needs_drop` to detect if a type has a destructor
+error: bounds on `U: Drop` are most likely incorrect, consider instead using `std::mem::needs_drop` to detect whether a type can be trivially dropped
   --> $DIR/drop-bounds.rs:12:24
    |
 LL | struct Bar<U> where U: Drop {
    |                        ^^^^
 
-error: bounds on `Self: Drop` are useless, consider instead using `std::mem::needs_drop` to detect if a type has a destructor
+error: bounds on `Self: Drop` are most likely incorrect, consider instead using `std::mem::needs_drop` to detect whether a type can be trivially dropped
   --> $DIR/drop-bounds.rs:15:12
    |
 LL | trait Baz: Drop {
    |            ^^^^
 
-error: bounds on `T: Drop` are useless, consider instead using `std::mem::needs_drop` to detect if a type has a destructor
+error: bounds on `T: Drop` are most likely incorrect, consider instead using `std::mem::needs_drop` to detect whether a type can be trivially dropped
   --> $DIR/drop-bounds.rs:17:9
    |
 LL | impl<T: Drop> Baz for T {