Auto merge of #3776 - notriddle:drop-bounds, r=oli-obk

Add a lint to warn on `T: Drop` bounds

**What it does:** Checks for generics with `std::ops::Drop` as bounds.

**Why is this bad?** `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.

**Known problems:** None.

**Example:**
```rust
fn foo<T: Drop>() {}
```

Fixes #3773

CHANGELOG.md

@@ -785,6 +785,7 @@ All notable changes to this project will be documented in this file.
 [`double_comparisons`]: https://rust-lang.github.io/rust-clippy/master/index.html#double_comparisons
 [`double_neg`]: https://rust-lang.github.io/rust-clippy/master/index.html#double_neg
 [`double_parens`]: https://rust-lang.github.io/rust-clippy/master/index.html#double_parens
+[`drop_bounds`]: https://rust-lang.github.io/rust-clippy/master/index.html#drop_bounds
 [`drop_copy`]: https://rust-lang.github.io/rust-clippy/master/index.html#drop_copy
 [`drop_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#drop_ref
 [`duplicate_underscore_argument`]: https://rust-lang.github.io/rust-clippy/master/index.html#duplicate_underscore_argument

README.md

@@ -7,7 +7,7 @@
 
 A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code.
 
-[There are 296 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
+[There are 297 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
 
 We have a bunch of lint categories to allow you to choose how much Clippy is supposed to ~~annoy~~ help you:
 

clippy_lints/src/drop_bounds.rs

@@ -0,0 +1,79 @@
+use crate::utils::{match_def_path, paths, span_lint};
+use if_chain::if_chain;
+use rustc::hir::*;
+use rustc::lint::{LateLintPass, LintArray, LintPass};
+use rustc::{declare_tool_lint, lint_array};
+
+/// **What it does:** Checks for generics with `std::ops::Drop` as bounds.
+///
+/// **Why is this bad?** `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.
+///
+/// **Known problems:** None.
+///
+/// **Example:**
+/// ```rust
+/// fn foo<T: Drop>() {}
+/// ```
+declare_clippy_lint! {
+    pub DROP_BOUNDS,
+    correctness,
+    "Bounds of the form `T: Drop` are useless"
+}
+
+const DROP_BOUNDS_SUMMARY: &str = "Bounds of the form `T: Drop` are useless. \
+                                   Use `std::mem::needs_drop` to detect if a type has drop glue.";
+
+pub struct Pass;
+
+impl LintPass for Pass {
+    fn get_lints(&self) -> LintArray {
+        lint_array!(DROP_BOUNDS)
+    }
+
+    fn name(&self) -> &'static str {
+        "DropBounds"
+    }
+}
+
+impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass {
+    fn check_generic_param(&mut self, cx: &rustc::lint::LateContext<'a, 'tcx>, p: &'tcx GenericParam) {
+        for bound in &p.bounds {
+            lint_bound(cx, bound);
+        }
+    }
+    fn check_where_predicate(&mut self, cx: &rustc::lint::LateContext<'a, 'tcx>, p: &'tcx WherePredicate) {
+        if let WherePredicate::BoundPredicate(WhereBoundPredicate { bounds, .. }) = p {
+            for bound in bounds {
+                lint_bound(cx, bound);
+            }
+        }
+    }
+}
+
+fn lint_bound<'a, 'tcx>(cx: &rustc::lint::LateContext<'a, 'tcx>, bound: &'tcx GenericBound) {
+    if_chain! {
+        if let GenericBound::Trait(t, _) = bound;
+        if let Some(def_id) = t.trait_ref.path.def.opt_def_id();
+        if match_def_path(cx.tcx, def_id, &paths::DROP_TRAIT);
+        then {
+            span_lint(
+                cx,
+                DROP_BOUNDS,
+                t.span,
+                DROP_BOUNDS_SUMMARY
+            );
+        }
+    }
+}

clippy_lints/src/lib.rs

@@ -144,6 +144,7 @@ macro_rules! declare_clippy_lint {
 pub mod doc;
 pub mod double_comparison;
 pub mod double_parens;
+pub mod drop_bounds;
 pub mod drop_forget_ref;
 pub mod duration_subsec;
 pub mod else_if_without_else;
@@ -467,6 +468,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) {
     reg.register_late_lint_pass(box derive::Derive);
     reg.register_late_lint_pass(box types::CharLitAsU8);
     reg.register_late_lint_pass(box vec::Pass);
+    reg.register_late_lint_pass(box drop_bounds::Pass);
     reg.register_late_lint_pass(box drop_forget_ref::Pass);
     reg.register_late_lint_pass(box empty_enum::EmptyEnum);
     reg.register_late_lint_pass(box types::AbsurdExtremeComparisons);
@@ -653,6 +655,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) {
         derive::DERIVE_HASH_XOR_EQ,
         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,
@@ -1017,6 +1020,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) {
         copies::IFS_SAME_COND,
         copies::IF_SAME_THEN_ELSE,
         derive::DERIVE_HASH_XOR_EQ,
+        drop_bounds::DROP_BOUNDS,
         drop_forget_ref::DROP_COPY,
         drop_forget_ref::DROP_REF,
         drop_forget_ref::FORGET_COPY,

clippy_lints/src/utils/paths.rs

@@ -24,6 +24,7 @@
 pub const DISPLAY_FMT_METHOD: [&str; 4] = ["core", "fmt", "Display", "fmt"];
 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 FMT_ARGUMENTS_NEWV1: [&str; 4] = ["core", "fmt", "Arguments", "new_v1"];

tests/ui/drop_bounds.rs

@@ -0,0 +1,8 @@
+#![allow(unused)]
+fn foo<T: Drop>() {}
+fn bar<T>()
+where
+    T: Drop,
+{
+}
+fn main() {}

tests/ui/drop_bounds.stderr

@@ -0,0 +1,16 @@
+error: Bounds of the form `T: Drop` are useless. Use `std::mem::needs_drop` to detect if a type has drop glue.
+  --> $DIR/drop_bounds.rs:2:11
+   |
+LL | fn foo<T: Drop>() {}
+   |           ^^^^
+   |
+   = note: #[deny(clippy::drop_bounds)] on by default
+
+error: Bounds of the form `T: Drop` are useless. Use `std::mem::needs_drop` to detect if a type has drop glue.
+  --> $DIR/drop_bounds.rs:5:8
+   |
+LL |     T: Drop,
+   |        ^^^^
+
+error: aborting due to 2 previous errors
+