Rollup merge of #75699 - notriddle:drop-bounds-lint, r=petrochenkov

Uplift drop-bounds lint from clippy

Bounds on `T: Drop` do nothing, so they should warn.

Author: JohnTitor

compiler/rustc_lint/src/lib.rs

@@ -53,6 +53,7 @@ mod non_ascii_idents;
 mod nonstandard_style;
 mod passes;
 mod redundant_semicolon;
+mod traits;
 mod types;
 mod unused;
 
@@ -75,6 +76,7 @@ use internal::*;
 use non_ascii_idents::*;
 use nonstandard_style::*;
 use redundant_semicolon::*;
+use traits::*;
 use types::*;
 use unused::*;
 
@@ -157,6 +159,7 @@ macro_rules! late_lint_passes {
                 MissingDebugImplementations: MissingDebugImplementations::default(),
                 ArrayIntoIter: ArrayIntoIter,
                 ClashingExternDeclarations: ClashingExternDeclarations::new(),
+                DropTraitConstraints: DropTraitConstraints,
             ]
         );
     };

compiler/rustc_lint/src/traits.rs

@@ -0,0 +1,79 @@
+use crate::LateContext;
+use crate::LateLintPass;
+use crate::LintContext;
+use rustc_hir as hir;
+use rustc_span::symbol::sym;
+
+declare_lint! {
+    /// The `drop_bounds` lint checks for generics with `std::ops::Drop` as
+    /// bounds.
+    ///
+    /// ### Example
+    ///
+    /// ```rust
+    /// fn foo<T: Drop>() {}
+    /// ```
+    ///
+    /// {{produces}}
+    ///
+    /// ### 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.
+    ///
+    /// 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.
+    pub DROP_BOUNDS,
+    Warn,
+    "bounds of the form `T: 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]
+);
+
+impl<'tcx> LateLintPass<'tcx> for DropTraitConstraints {
+    fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx hir::Item<'tcx>) {
+        use rustc_middle::ty::PredicateAtom::*;
+
+        let def_id = cx.tcx.hir().local_def_id(item.hir_id);
+        let predicates = cx.tcx.explicit_predicates_of(def_id);
+        for &(predicate, span) in predicates.predicates {
+            let trait_predicate = match predicate.skip_binders() {
+                Trait(trait_predicate, _constness) => trait_predicate,
+                _ => continue,
+            };
+            let def_id = trait_predicate.trait_ref.def_id;
+            if cx.tcx.lang_items().drop_trait() == Some(def_id) {
+                // Explicitly allow `impl Drop`, a drop-guards-as-Voldemort-type pattern.
+                if trait_predicate.trait_ref.self_ty().is_impl_trait() {
+                    continue;
+                }
+                cx.struct_span_lint(DROP_BOUNDS, span, |lint| {
+                    let needs_drop = match cx.tcx.get_diagnostic_item(sym::needs_drop) {
+                        Some(needs_drop) => needs_drop,
+                        None => return,
+                    };
+                    let msg = format!(
+                        "bounds on `{}` are useless, consider instead \
+                         using `{}` to detect if a type has a destructor",
+                        predicate,
+                        cx.tcx.def_path_str(needs_drop)
+                    );
+                    lint.build(&msg).emit()
+                });
+            }
+        }
+    }
+}

library/core/src/mem/mod.rs

@@ -568,6 +568,7 @@ pub unsafe fn align_of_val_raw<T: ?Sized>(val: *const T) -> usize {
 #[inline]
 #[stable(feature = "needs_drop", since = "1.21.0")]
 #[rustc_const_stable(feature = "const_needs_drop", since = "1.36.0")]
+#[rustc_diagnostic_item = "needs_drop"]
 pub const fn needs_drop<T>() -> bool {
     intrinsics::needs_drop::<T>()
 }

src/test/ui/drop-bounds/drop-bounds-impl-drop.rs

@@ -0,0 +1,14 @@
+// run-pass
+#![deny(drop_bounds)]
+// As a special exemption, `impl Drop` in the return position raises no error.
+// This allows a convenient way to return an unnamed drop guard.
+fn voldemort_type() -> impl Drop {
+  struct Voldemort;
+  impl Drop for Voldemort {
+    fn drop(&mut self) {}
+  }
+  Voldemort
+}
+fn main() {
+  let _ = voldemort_type();
+}

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

@@ -0,0 +1,19 @@
+#![deny(drop_bounds)]
+fn foo<T: Drop>() {} //~ ERROR
+fn bar<U>()
+where
+    U: Drop, //~ ERROR
+{
+}
+fn baz(_x: impl Drop) {} //~ ERROR
+struct Foo<T: Drop> { //~ ERROR
+  _x: T
+}
+struct Bar<U> where U: Drop { //~ ERROR
+  _x: U
+}
+trait Baz: Drop { //~ ERROR
+}
+impl<T: Drop> Baz for T { //~ ERROR
+}
+fn main() {}

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

@@ -0,0 +1,50 @@
+error: bounds on `T: Drop` are useless, consider instead using `std::mem::needs_drop` to detect if a type has a destructor
+  --> $DIR/drop-bounds.rs:2:11
+   |
+LL | fn foo<T: Drop>() {}
+   |           ^^^^
+   |
+note: the lint level is defined here
+  --> $DIR/drop-bounds.rs:1:9
+   |
+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
+  --> $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
+  --> $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
+  --> $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
+  --> $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
+  --> $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
+  --> $DIR/drop-bounds.rs:17:9
+   |
+LL | impl<T: Drop> Baz for T {
+   |         ^^^^
+
+error: aborting due to 7 previous errors
+

src/tools/clippy/clippy_lints/src/deprecated_lints.rs

@@ -163,3 +163,12 @@ declare_deprecated_lint! {
     pub REGEX_MACRO,
     "the regex! macro has been removed from the regex crate in 2018"
 }
+
+declare_deprecated_lint! {
+    /// **What it does:** Nothing. This lint has been deprecated.
+    ///
+    /// **Deprecation reason:** This lint has been uplifted to rustc and is now called
+    /// `drop_bounds`.
+    pub DROP_BOUNDS,
+    "this lint has been uplifted to rustc and is now called `drop_bounds`"
+}

src/tools/clippy/clippy_lints/src/drop_bounds.rs

@@ -179,7 +179,6 @@ mod derive;
 mod doc;
 mod double_comparison;
 mod double_parens;
-mod drop_bounds;
 mod drop_forget_ref;
 mod duration_subsec;
 mod else_if_without_else;
@@ -478,6 +477,10 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         "clippy::regex_macro",
         "the regex! macro has been removed from the regex crate in 2018",
     );
+    store.register_removed(
+        "clippy::drop_bounds",
+        "this lint has been uplifted to rustc and is now called `drop_bounds`",
+    );
     // end deprecated lints, do not remove this comment, it’s used in `update_lints`
 
     // begin register lints, do not remove this comment, it’s used in `update_lints`
@@ -532,7 +535,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         &doc::NEEDLESS_DOCTEST_MAIN,
         &double_comparison::DOUBLE_COMPARISONS,
         &double_parens::DOUBLE_PARENS,
-        &drop_bounds::DROP_BOUNDS,
         &drop_forget_ref::DROP_COPY,
         &drop_forget_ref::DROP_REF,
         &drop_forget_ref::FORGET_COPY,
@@ -959,7 +961,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_late_pass(|| box strings::StringLitAsBytes);
     store.register_late_pass(|| box derive::Derive);
     store.register_late_pass(|| box types::CharLitAsU8);
-    store.register_late_pass(|| box drop_bounds::DropBounds);
     store.register_late_pass(|| box get_last_with_len::GetLastWithLen);
     store.register_late_pass(|| box drop_forget_ref::DropForgetRef);
     store.register_late_pass(|| box empty_enum::EmptyEnum);
@@ -1282,7 +1283,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&doc::NEEDLESS_DOCTEST_MAIN),
         LintId::of(&double_comparison::DOUBLE_COMPARISONS),
         LintId::of(&double_parens::DOUBLE_PARENS),
-        LintId::of(&drop_bounds::DROP_BOUNDS),
         LintId::of(&drop_forget_ref::DROP_COPY),
         LintId::of(&drop_forget_ref::DROP_REF),
         LintId::of(&drop_forget_ref::FORGET_COPY),
@@ -1714,7 +1714,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&copies::IF_SAME_THEN_ELSE),
         LintId::of(&derive::DERIVE_HASH_XOR_EQ),
         LintId::of(&derive::DERIVE_ORD_XOR_PARTIAL_ORD),
-        LintId::of(&drop_bounds::DROP_BOUNDS),
         LintId::of(&drop_forget_ref::DROP_COPY),
         LintId::of(&drop_forget_ref::DROP_REF),
         LintId::of(&drop_forget_ref::FORGET_COPY),

src/tools/clippy/clippy_lints/src/lib.rs

@@ -31,7 +31,6 @@ pub const DISPLAY_FMT_METHOD: [&str; 4] = ["core", "fmt", "Display", "fmt"];
 pub const DISPLAY_TRAIT: [&str; 3] = ["core", "fmt", "Display"];
 pub const DOUBLE_ENDED_ITERATOR: [&str; 4] = ["core", "iter", "traits", "DoubleEndedIterator"];
 pub const DROP: [&str; 3] = ["core", "mem", "drop"];
-pub const DROP_TRAIT: [&str; 4] = ["core", "ops", "drop", "Drop"];
 pub const DURATION: [&str; 3] = ["core", "time", "Duration"];
 pub const EARLY_CONTEXT: [&str; 4] = ["rustc", "lint", "context", "EarlyContext"];
 pub const EXIT: [&str; 3] = ["std", "process", "exit"];

src/tools/clippy/clippy_lints/src/utils/paths.rs

@@ -423,13 +423,6 @@ pub static ref ALL_LINTS: Vec<Lint> = vec![
         deprecation: None,
         module: "double_parens",
     },
-    Lint {
-        name: "drop_bounds",
-        group: "correctness",
-        desc: "bounds of the form `T: Drop` are useless",
-        deprecation: None,
-        module: "drop_bounds",
-    },
     Lint {
         name: "drop_copy",
         group: "correctness",

src/tools/clippy/src/lintlist/mod.rs

@@ -8,5 +8,6 @@
 #[warn(clippy::into_iter_on_array)]
 #[warn(clippy::unused_label)]
 #[warn(clippy::regex_macro)]
+#[warn(clippy::drop_bounds)]
 
 fn main() {}

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

@@ -60,11 +60,17 @@ error: lint `clippy::regex_macro` has been removed: `the regex! macro has been r
 LL | #[warn(clippy::regex_macro)]
    |        ^^^^^^^^^^^^^^^^^^^
 
+error: lint `clippy::drop_bounds` has been removed: `this lint has been uplifted to rustc and is now called `drop_bounds``
+  --> $DIR/deprecated.rs:11:8
+   |
+LL | #[warn(clippy::drop_bounds)]
+   |        ^^^^^^^^^^^^^^^^^^^
+
 error: lint `clippy::str_to_string` has been removed: `using `str::to_string` is common even today and specialization will likely happen soon`
   --> $DIR/deprecated.rs:1:8
    |
 LL | #[warn(clippy::str_to_string)]
    |        ^^^^^^^^^^^^^^^^^^^^^
 
-error: aborting due to 11 previous errors
+error: aborting due to 12 previous errors