Auto merge of #86848 - notriddle:notriddle/drop-dyn, r=varkor

feat(rustc_lint): add `dyn_drop`

Based on the conversation in #86747.

Explanation
-----------

A trait object bound of the form `dyn Drop` is most likely misleading and not what the programmer intended.

`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 a deferred drop system, to pull cleaning up memory out of a latency-sensitive code path, using `dyn Drop` trait objects. However, this breaks down e.g. when `T` is `String`, which does not implement `Drop`, but should probably be accepted.

To write a trait object bound that accepts anything, use a placeholder trait with a blanket implementation.

```rust
trait Placeholder {}
impl<T> Placeholder for T {}
fn foo(_x: Box<dyn Placeholder>) {}
```

Author: bors

compiler/rustc_lint/src/traits.rs

@@ -37,10 +37,47 @@ declare_lint! {
     "bounds of the form `T: Drop` are useless"
 }
 
+declare_lint! {
+    /// The `dyn_drop` lint checks for trait objects with `std::ops::Drop`.
+    ///
+    /// ### Example
+    ///
+    /// ```rust
+    /// fn foo(_x: Box<dyn Drop>) {}
+    /// ```
+    ///
+    /// {{produces}}
+    ///
+    /// ### Explanation
+    ///
+    /// A trait object bound of the form `dyn Drop` is most likely misleading
+    /// and not what the programmer intended.
+    ///
+    /// `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 a deferred drop system, to pull cleaning up memory out of a
+    /// latency-sensitive code path, using `dyn Drop` trait objects. However,
+    /// this breaks down e.g. when `T` is `String`, which does not implement
+    /// `Drop`, but should probably be accepted.
+    ///
+    /// To write a trait object bound that accepts anything, use a placeholder
+    /// trait with a blanket implementation.
+    ///
+    /// ```rust
+    /// trait Placeholder {}
+    /// impl<T> Placeholder for T {}
+    /// fn foo(_x: Box<dyn Placeholder>) {}
+    /// ```
+    pub DYN_DROP,
+    Warn,
+    "trait objects of the form `dyn Drop` are useless"
+}
+
 declare_lint_pass!(
     /// Lint for bounds of the form `T: Drop`, which usually
     /// indicate an attempt to emulate `std::mem::needs_drop`.
-    DropTraitConstraints => [DROP_BOUNDS]
+    DropTraitConstraints => [DROP_BOUNDS, DYN_DROP]
 );
 
 impl<'tcx> LateLintPass<'tcx> for DropTraitConstraints {
@@ -75,4 +112,28 @@ impl<'tcx> LateLintPass<'tcx> for DropTraitConstraints {
             }
         }
     }
+
+    fn check_ty(&mut self, cx: &LateContext<'_>, ty: &'tcx hir::Ty<'tcx>) {
+        let bounds = match &ty.kind {
+            hir::TyKind::TraitObject(bounds, _lifetime, _syntax) => bounds,
+            _ => return,
+        };
+        for bound in &bounds[..] {
+            let def_id = bound.trait_ref.trait_def_id();
+            if cx.tcx.lang_items().drop_trait() == def_id {
+                cx.struct_span_lint(DYN_DROP, bound.span, |lint| {
+                    let needs_drop = match cx.tcx.get_diagnostic_item(sym::needs_drop) {
+                        Some(needs_drop) => needs_drop,
+                        None => return,
+                    };
+                    let msg = format!(
+                        "types that do not implement `Drop` can still have drop glue, consider \
+                        instead using `{}` to detect whether a type is trivially dropped",
+                        cx.tcx.def_path_str(needs_drop)
+                    );
+                    lint.build(&msg).emit()
+                });
+            }
+        }
+    }
 }

src/test/ui/dyn-drop/dyn-drop.rs

@@ -0,0 +1,16 @@
+#![deny(dyn_drop)]
+#![allow(bare_trait_objects)]
+fn foo(_: Box<dyn Drop>) {} //~ ERROR
+fn bar(_: &dyn Drop) {} //~ERROR
+fn baz(_: *mut Drop) {} //~ ERROR
+struct Foo {
+  _x: Box<dyn Drop> //~ ERROR
+}
+trait Bar {
+  type T: ?Sized;
+}
+struct Baz {}
+impl Bar for Baz {
+  type T = dyn Drop; //~ ERROR
+}
+fn main() {}

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

@@ -0,0 +1,38 @@
+error: 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
+  --> $DIR/dyn-drop.rs:3:19
+   |
+LL | fn foo(_: Box<dyn Drop>) {}
+   |                   ^^^^
+   |
+note: the lint level is defined here
+  --> $DIR/dyn-drop.rs:1:9
+   |
+LL | #![deny(dyn_drop)]
+   |         ^^^^^^^^
+
+error: 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
+  --> $DIR/dyn-drop.rs:4:16
+   |
+LL | fn bar(_: &dyn Drop) {}
+   |                ^^^^
+
+error: 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
+  --> $DIR/dyn-drop.rs:5:16
+   |
+LL | fn baz(_: *mut Drop) {}
+   |                ^^^^
+
+error: 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
+  --> $DIR/dyn-drop.rs:7:15
+   |
+LL |   _x: Box<dyn Drop>
+   |               ^^^^
+
+error: 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
+  --> $DIR/dyn-drop.rs:14:16
+   |
+LL |   type T = dyn Drop;
+   |                ^^^^
+
+error: aborting due to 5 previous errors
+

src/test/ui/dyn-keyword/issue-56327-dyn-trait-in-macro-is-okay.rs

@@ -10,6 +10,7 @@
 // anything.
 
 #![deny(rust_2018_compatibility)]
+#![allow(dyn_drop)]
 
 macro_rules! foo {
     () => {

src/test/ui/traits/object/issue-33140-traitobject-crate.rs

@@ -1,6 +1,7 @@
 // check-pass
 
 #![warn(order_dependent_trait_objects)]
+#![allow(dyn_drop)]
 
 // Check that traitobject 0.1.0 compiles
 

src/test/ui/traits/object/issue-33140-traitobject-crate.stderr

@@ -1,5 +1,5 @@
 warning: conflicting implementations of trait `Trait` for type `(dyn std::marker::Send + std::marker::Sync + 'static)`: (E0119)
-  --> $DIR/issue-33140-traitobject-crate.rs:85:1
+  --> $DIR/issue-33140-traitobject-crate.rs:86:1
    |
 LL | unsafe impl Trait for dyn (::std::marker::Send) + Sync { }
    | ------------------------------------------------------ first implementation here
@@ -15,7 +15,7 @@ LL | #![warn(order_dependent_trait_objects)]
    = note: for more information, see issue #56484 <https://github.com/rust-lang/rust/issues/56484>
 
 warning: conflicting implementations of trait `Trait` for type `(dyn std::marker::Send + std::marker::Sync + 'static)`: (E0119)
-  --> $DIR/issue-33140-traitobject-crate.rs:88:1
+  --> $DIR/issue-33140-traitobject-crate.rs:89:1
    |
 LL | unsafe impl Trait for dyn (::std::marker::Send) + Send + Sync { }
    | ------------------------------------------------------------- first implementation here
@@ -27,7 +27,7 @@ LL | unsafe impl Trait for dyn (::std::marker::Sync) + Send { }
    = note: for more information, see issue #56484 <https://github.com/rust-lang/rust/issues/56484>
 
 warning: conflicting implementations of trait `Trait` for type `(dyn std::marker::Send + std::marker::Sync + 'static)`: (E0119)
-  --> $DIR/issue-33140-traitobject-crate.rs:92:1
+  --> $DIR/issue-33140-traitobject-crate.rs:93:1
    |
 LL | unsafe impl Trait for dyn (::std::marker::Sync) + Send { }
    | ------------------------------------------------------ first implementation here

src/tools/clippy/tests/ui/needless_lifetimes.rs

@@ -1,5 +1,5 @@
 #![warn(clippy::needless_lifetimes)]
-#![allow(dead_code, clippy::needless_pass_by_value, clippy::unnecessary_wraps)]
+#![allow(dead_code, clippy::needless_pass_by_value, clippy::unnecessary_wraps, dyn_drop)]
 
 fn distinct_lifetimes<'a, 'b>(_x: &'a u8, _y: &'b u8, _z: u8) {}