Suggest `into_iter()` over `drain(..)`

Add doc

Add description

iter_with_drain dogfood

Disable emiting on struct field.

Fix clippy

Add eq_path for SpanlessEq

Fix tests

Better error message

Fix doc test

Fix version

Apply suggestions

16 files changed, 276 insertions, 14 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 1ab879ba9ee..7f18a00a9dd 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -3225,6 +3225,7 @@ Released 2018-09-13
 [`iter_nth_zero`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_nth_zero
 [`iter_overeager_cloned`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_overeager_cloned
 [`iter_skip_next`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_skip_next
+[`iter_with_drain`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_with_drain
 [`iterator_step_by_zero`]: https://rust-lang.github.io/rust-clippy/master/index.html#iterator_step_by_zero
 [`just_underscores_and_digits`]: https://rust-lang.github.io/rust-clippy/master/index.html#just_underscores_and_digits
 [`large_const_arrays`]: https://rust-lang.github.io/rust-clippy/master/index.html#large_const_arrays
diff --git a/clippy_lints/src/lib.register_all.rs b/clippy_lints/src/lib.register_all.rs
index 4484f6e1960..f388ae7ce60 100644
--- a/clippy_lints/src/lib.register_all.rs
+++ b/clippy_lints/src/lib.register_all.rs
@@ -164,6 +164,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![
     LintId::of(methods::ITER_NTH_ZERO),
     LintId::of(methods::ITER_OVEREAGER_CLONED),
     LintId::of(methods::ITER_SKIP_NEXT),
+    LintId::of(methods::ITER_WITH_DRAIN),
     LintId::of(methods::MANUAL_FILTER_MAP),
     LintId::of(methods::MANUAL_FIND_MAP),
     LintId::of(methods::MANUAL_SATURATING_ARITHMETIC),
diff --git a/clippy_lints/src/lib.register_lints.rs b/clippy_lints/src/lib.register_lints.rs
index c34d8f46e09..d248c2a41ee 100644
--- a/clippy_lints/src/lib.register_lints.rs
+++ b/clippy_lints/src/lib.register_lints.rs
@@ -299,6 +299,7 @@ store.register_lints(&[
     methods::ITER_NTH_ZERO,
     methods::ITER_OVEREAGER_CLONED,
     methods::ITER_SKIP_NEXT,
+    methods::ITER_WITH_DRAIN,
     methods::MANUAL_FILTER_MAP,
     methods::MANUAL_FIND_MAP,
     methods::MANUAL_SATURATING_ARITHMETIC,
diff --git a/clippy_lints/src/lib.register_perf.rs b/clippy_lints/src/lib.register_perf.rs
index f2f5c988d8f..6e9c0ee33a1 100644
--- a/clippy_lints/src/lib.register_perf.rs
+++ b/clippy_lints/src/lib.register_perf.rs
@@ -16,6 +16,7 @@ store.register_group(true, "clippy::perf", Some("clippy_perf"), vec![
     LintId::of(methods::EXTEND_WITH_DRAIN),
     LintId::of(methods::ITER_NTH),
     LintId::of(methods::ITER_OVEREAGER_CLONED),
+    LintId::of(methods::ITER_WITH_DRAIN),
     LintId::of(methods::MANUAL_STR_REPEAT),
     LintId::of(methods::OR_FUN_CALL),
     LintId::of(methods::SINGLE_CHAR_PATTERN),
diff --git a/clippy_lints/src/methods/iter_with_drain.rs b/clippy_lints/src/methods/iter_with_drain.rs
new file mode 100644
index 00000000000..958c3773087
--- /dev/null
+++ b/clippy_lints/src/methods/iter_with_drain.rs
@@ -0,0 +1,72 @@
+use clippy_utils::diagnostics::span_lint_and_sugg;
+use clippy_utils::is_integer_const;
+use clippy_utils::ty::is_type_diagnostic_item;
+use clippy_utils::{
+    higher::{self, Range},
+    SpanlessEq,
+};
+use rustc_ast::ast::RangeLimits;
+use rustc_errors::Applicability;
+use rustc_hir::{Expr, ExprKind, QPath};
+use rustc_lint::LateContext;
+use rustc_span::symbol::{sym, Symbol};
+use rustc_span::Span;
+
+use super::ITER_WITH_DRAIN;
+
+const DRAIN_TYPES: &[Symbol] = &[sym::Vec, sym::VecDeque];
+
+pub(super) fn check(cx: &LateContext<'_>, expr: &Expr<'_>, recv: &Expr<'_>, span: Span, arg: &Expr<'_>) {
+    let ty = cx.typeck_results().expr_ty(recv).peel_refs();
+    if let Some(drained_type) = DRAIN_TYPES.iter().find(|&&sym| is_type_diagnostic_item(cx, ty, sym)) {
+        // Refuse to emit `into_iter` suggestion on draining struct fields due
+        // to the strong possibility of processing unmovable field.
+        if let ExprKind::Field(..) = recv.kind {
+            return;
+        }
+
+        if let Some(range) = higher::Range::hir(arg) {
+            let left_full = match range {
+                Range { start: Some(start), .. } if is_integer_const(cx, start, 0) => true,
+                Range { start: None, .. } => true,
+                _ => false,
+            };
+            let full = left_full
+                && match range {
+                    Range {
+                        end: Some(end),
+                        limits: RangeLimits::HalfOpen,
+                        ..
+                    } => {
+                        // `x.drain(..x.len())` call
+                        if_chain! {
+                            if let ExprKind::MethodCall(len_path, len_args, _) = end.kind;
+                            if len_path.ident.name == sym::len && len_args.len() == 1;
+                            if let ExprKind::Path(QPath::Resolved(_, drain_path)) = recv.kind;
+                            if let ExprKind::Path(QPath::Resolved(_, len_path)) = len_args[0].kind;
+                            if SpanlessEq::new(cx).eq_path(drain_path, len_path);
+                            then { true }
+                            else { false }
+                        }
+                    },
+                    Range {
+                        end: None,
+                        limits: RangeLimits::HalfOpen,
+                        ..
+                    } => true,
+                    _ => false,
+                };
+            if full {
+                span_lint_and_sugg(
+                    cx,
+                    ITER_WITH_DRAIN,
+                    span.with_hi(expr.span.hi()),
+                    &format!("`drain(..)` used on a `{}`", drained_type),
+                    "try this",
+                    "into_iter()".to_string(),
+                    Applicability::MaybeIncorrect,
+                );
+            }
+        }
+    }
+}
diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs
index 581f4c8b0e2..5edd22cd14c 100644
--- a/clippy_lints/src/methods/mod.rs
+++ b/clippy_lints/src/methods/mod.rs
@@ -32,6 +32,7 @@ mod iter_nth;
 mod iter_nth_zero;
 mod iter_overeager_cloned;
 mod iter_skip_next;
+mod iter_with_drain;
 mod iterator_step_by_zero;
 mod manual_saturating_arithmetic;
 mod manual_str_repeat;
@@ -1120,6 +1121,31 @@ declare_clippy_lint! {
 
 declare_clippy_lint! {
     /// ### What it does
+    /// Checks for use of `.drain(..)` on `Vec` and `VecDeque` for iteration.
+    ///
+    /// ### Why is this bad?
+    /// `.into_iter()` is simpler with better performance.
+    ///
+    /// ### Example
+    /// ```rust
+    /// # use std::collections::HashSet;
+    /// let mut foo = vec![0, 1, 2, 3];
+    /// let bar: HashSet<usize> = foo.drain(..).collect();
+    /// ```
+    /// Use instead:
+    /// ```rust
+    /// # use std::collections::HashSet;
+    /// let foo = vec![0, 1, 2, 3];
+    /// let bar: HashSet<usize> = foo.into_iter().collect();
+    /// ```
+    #[clippy::version = "1.61.0"]
+    pub ITER_WITH_DRAIN,
+    perf,
+    "replace `.drain(..)` with `.into_iter()`"
+}
+
+declare_clippy_lint! {
+    /// ### What it does
     /// Checks for use of `.get().unwrap()` (or
     /// `.get_mut().unwrap`) on a standard library type which implements `Index`
     ///
@@ -2047,6 +2073,7 @@ impl_lint_pass!(Methods => [
     GET_UNWRAP,
     STRING_EXTEND_CHARS,
     ITER_CLONED_COLLECT,
+    ITER_WITH_DRAIN,
     USELESS_ASREF,
     UNNECESSARY_FOLD,
     UNNECESSARY_FILTER_MAP,
@@ -2327,6 +2354,9 @@ fn check_methods<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, msrv: Optio
                 Some(("map", [_, arg], _)) => suspicious_map::check(cx, expr, recv, arg),
                 _ => {},
             },
+            ("drain", [arg]) => {
+                iter_with_drain::check(cx, expr, recv, span, arg);
+            },
             ("expect", [_]) => match method_call(recv) {
                 Some(("ok", [recv], _)) => ok_expect::check(cx, expr, recv),
                 _ => expect_used::check(cx, expr, recv),
diff --git a/clippy_lints/src/suspicious_operation_groupings.rs b/clippy_lints/src/suspicious_operation_groupings.rs
index c3d984fb5ae..940a8428f77 100644
--- a/clippy_lints/src/suspicious_operation_groupings.rs
+++ b/clippy_lints/src/suspicious_operation_groupings.rs
@@ -399,9 +399,9 @@ fn if_statment_binops(kind: &ExprKind) -> Option<Vec<BinaryOp<'_>>> {
 
 fn append_opt_vecs<A>(target_opt: Option<Vec<A>>, source_opt: Option<Vec<A>>) -> Option<Vec<A>> {
     match (target_opt, source_opt) {
-        (Some(mut target), Some(mut source)) => {
+        (Some(mut target), Some(source)) => {
             target.reserve(source.len());
-            for op in source.drain(..) {
+            for op in source {
                 target.push(op);
             }
             Some(target)
@@ -436,9 +436,9 @@ fn chained_binops_helper<'expr>(left_outer: &'expr Expr, right_outer: &'expr Exp
             chained_binops_helper(left_left, left_right),
             chained_binops_helper(right_left, right_right),
         ) {
-            (Some(mut left_ops), Some(mut right_ops)) => {
+            (Some(mut left_ops), Some(right_ops)) => {
                 left_ops.reserve(right_ops.len());
-                for op in right_ops.drain(..) {
+                for op in right_ops {
                     left_ops.push(op);
                 }
                 Some(left_ops)
diff --git a/clippy_lints/src/utils/internal_lints/metadata_collector.rs b/clippy_lints/src/utils/internal_lints/metadata_collector.rs
index 56633490eaa..a617422bbeb 100644
--- a/clippy_lints/src/utils/internal_lints/metadata_collector.rs
+++ b/clippy_lints/src/utils/internal_lints/metadata_collector.rs
@@ -473,7 +473,7 @@ impl<'hir> LateLintPass<'hir> for MetadataCollector {
     /// ```
     fn check_expr(&mut self, cx: &LateContext<'hir>, expr: &'hir hir::Expr<'_>) {
         if let Some(args) = match_lint_emission(cx, expr) {
-            let mut emission_info = extract_emission_info(cx, args);
+            let emission_info = extract_emission_info(cx, args);
             if emission_info.is_empty() {
                 // See:
                 // - src/misc.rs:734:9
@@ -483,7 +483,7 @@ impl<'hir> LateLintPass<'hir> for MetadataCollector {
                 return;
             }
 
-            for (lint_name, applicability, is_multi_part) in emission_info.drain(..) {
+            for (lint_name, applicability, is_multi_part) in emission_info {
                 let app_info = self.applicability_info.entry(lint_name).or_default();
                 app_info.applicability = applicability;
                 app_info.is_multi_part_suggestion = is_multi_part;
@@ -693,7 +693,7 @@ fn extract_emission_info<'hir>(
     }
 
     lints
-        .drain(..)
+        .into_iter()
         .map(|lint_name| (lint_name, applicability, multi_part))
         .collect()
 }
diff --git a/clippy_utils/src/hir_utils.rs b/clippy_utils/src/hir_utils.rs
index 9654895060f..00594f4d42a 100644
--- a/clippy_utils/src/hir_utils.rs
+++ b/clippy_utils/src/hir_utils.rs
@@ -74,6 +74,10 @@ impl<'a, 'tcx> SpanlessEq<'a, 'tcx> {
         self.inter_expr().eq_expr(left, right)
     }
 
+    pub fn eq_path(&mut self, left: &Path<'_>, right: &Path<'_>) -> bool {
+        self.inter_expr().eq_path(left, right)
+    }
+
     pub fn eq_path_segment(&mut self, left: &PathSegment<'_>, right: &PathSegment<'_>) -> bool {
         self.inter_expr().eq_path_segment(left, right)
     }
@@ -362,7 +366,7 @@ impl HirEqInterExpr<'_, '_, '_> {
         }
     }
 
-    fn eq_path(&mut self, left: &Path<'_>, right: &Path<'_>) -> bool {
+    pub fn eq_path(&mut self, left: &Path<'_>, right: &Path<'_>) -> bool {
         match (left.res, right.res) {
             (Res::Local(l), Res::Local(r)) => l == r || self.locals.get(&l) == Some(&r),
             (Res::Local(_), _) | (_, Res::Local(_)) => false,
diff --git a/tests/ui/extend_with_drain.fixed b/tests/ui/extend_with_drain.fixed
index e863870e7d6..71ebad24c16 100644
--- a/tests/ui/extend_with_drain.fixed
+++ b/tests/ui/extend_with_drain.fixed
@@ -1,11 +1,11 @@
 // run-rustfix
 #![warn(clippy::extend_with_drain)]
+#![allow(clippy::iter_with_drain)]
 use std::collections::BinaryHeap;
 fn main() {
     //gets linted
     let mut vec1 = vec![0u8; 1024];
     let mut vec2: std::vec::Vec<u8> = Vec::new();
-
     vec2.append(&mut vec1);
 
     let mut vec3 = vec![0u8; 1024];
@@ -17,7 +17,7 @@ fn main() {
 
     vec11.append(&mut return_vector());
 
-    //won't get linted it dosen't move the entire content of a vec into another
+    //won't get linted it doesn't move the entire content of a vec into another
     let mut test1 = vec![0u8, 10];
     let mut test2: std::vec::Vec<u8> = Vec::new();
 
diff --git a/tests/ui/extend_with_drain.rs b/tests/ui/extend_with_drain.rs
index dcb36b5951c..e9f011abb0e 100644
--- a/tests/ui/extend_with_drain.rs
+++ b/tests/ui/extend_with_drain.rs
@@ -1,11 +1,11 @@
 // run-rustfix
 #![warn(clippy::extend_with_drain)]
+#![allow(clippy::iter_with_drain)]
 use std::collections::BinaryHeap;
 fn main() {
     //gets linted
     let mut vec1 = vec![0u8; 1024];
     let mut vec2: std::vec::Vec<u8> = Vec::new();
-
     vec2.extend(vec1.drain(..));
 
     let mut vec3 = vec![0u8; 1024];
@@ -17,7 +17,7 @@ fn main() {
 
     vec11.extend(return_vector().drain(..));
 
-    //won't get linted it dosen't move the entire content of a vec into another
+    //won't get linted it doesn't move the entire content of a vec into another
     let mut test1 = vec![0u8, 10];
     let mut test2: std::vec::Vec<u8> = Vec::new();
 
diff --git a/tests/ui/iter_with_drain.fixed b/tests/ui/iter_with_drain.fixed
new file mode 100644
index 00000000000..aea4dba9dd5
--- /dev/null
+++ b/tests/ui/iter_with_drain.fixed
@@ -0,0 +1,56 @@
+// run-rustfix
+// will emits unused mut warnings after fixing
+#![allow(unused_mut)]
+// will emits needless collect warnings after fixing
+#![allow(clippy::needless_collect)]
+#![warn(clippy::iter_with_drain)]
+use std::collections::{BinaryHeap, HashMap, HashSet, VecDeque};
+
+fn full() {
+    let mut a = vec!["aaa".to_string(), "bbb".to_string()];
+    let mut a: BinaryHeap<_> = a.into_iter().collect();
+    let mut a: HashSet<_> = a.drain().collect();
+    let mut a: VecDeque<_> = a.drain().collect();
+    let mut a: Vec<_> = a.into_iter().collect();
+    let mut a: HashMap<_, _> = a.into_iter().map(|x| (x.clone(), x)).collect();
+    let _: Vec<(String, String)> = a.drain().collect();
+}
+
+fn closed() {
+    let mut a = vec!["aaa".to_string(), "bbb".to_string()];
+    let mut a: BinaryHeap<_> = a.into_iter().collect();
+    let mut a: HashSet<_> = a.drain().collect();
+    let mut a: VecDeque<_> = a.drain().collect();
+    let mut a: Vec<_> = a.into_iter().collect();
+    let mut a: HashMap<_, _> = a.into_iter().map(|x| (x.clone(), x)).collect();
+    let _: Vec<(String, String)> = a.drain().collect();
+}
+
+fn should_not_help() {
+    let mut a = vec!["aaa".to_string(), "bbb".to_string()];
+    let mut a: BinaryHeap<_> = a.drain(1..).collect();
+    let mut a: HashSet<_> = a.drain().collect();
+    let mut a: VecDeque<_> = a.drain().collect();
+    let mut a: Vec<_> = a.drain(..a.len() - 1).collect();
+    let mut a: HashMap<_, _> = a.drain(1..a.len() - 1).map(|x| (x.clone(), x)).collect();
+    let _: Vec<(String, String)> = a.drain().collect();
+
+    let mut b = vec!["aaa".to_string(), "bbb".to_string()];
+    let _: Vec<_> = b.drain(0..a.len()).collect();
+}
+
+#[derive(Default)]
+struct Bomb {
+    fire: Vec<u8>,
+}
+
+fn should_not_help_0(bomb: &mut Bomb) {
+    let _: Vec<u8> = bomb.fire.drain(..).collect();
+}
+
+fn main() {
+    full();
+    closed();
+    should_not_help();
+    should_not_help_0(&mut Bomb::default());
+}
diff --git a/tests/ui/iter_with_drain.rs b/tests/ui/iter_with_drain.rs
new file mode 100644
index 00000000000..271878cffb4
--- /dev/null
+++ b/tests/ui/iter_with_drain.rs
@@ -0,0 +1,56 @@
+// run-rustfix
+// will emits unused mut warnings after fixing
+#![allow(unused_mut)]
+// will emits needless collect warnings after fixing
+#![allow(clippy::needless_collect)]
+#![warn(clippy::iter_with_drain)]
+use std::collections::{BinaryHeap, HashMap, HashSet, VecDeque};
+
+fn full() {
+    let mut a = vec!["aaa".to_string(), "bbb".to_string()];
+    let mut a: BinaryHeap<_> = a.drain(..).collect();
+    let mut a: HashSet<_> = a.drain().collect();
+    let mut a: VecDeque<_> = a.drain().collect();
+    let mut a: Vec<_> = a.drain(..).collect();
+    let mut a: HashMap<_, _> = a.drain(..).map(|x| (x.clone(), x)).collect();
+    let _: Vec<(String, String)> = a.drain().collect();
+}
+
+fn closed() {
+    let mut a = vec!["aaa".to_string(), "bbb".to_string()];
+    let mut a: BinaryHeap<_> = a.drain(0..).collect();
+    let mut a: HashSet<_> = a.drain().collect();
+    let mut a: VecDeque<_> = a.drain().collect();
+    let mut a: Vec<_> = a.drain(..a.len()).collect();
+    let mut a: HashMap<_, _> = a.drain(0..a.len()).map(|x| (x.clone(), x)).collect();
+    let _: Vec<(String, String)> = a.drain().collect();
+}
+
+fn should_not_help() {
+    let mut a = vec!["aaa".to_string(), "bbb".to_string()];
+    let mut a: BinaryHeap<_> = a.drain(1..).collect();
+    let mut a: HashSet<_> = a.drain().collect();
+    let mut a: VecDeque<_> = a.drain().collect();
+    let mut a: Vec<_> = a.drain(..a.len() - 1).collect();
+    let mut a: HashMap<_, _> = a.drain(1..a.len() - 1).map(|x| (x.clone(), x)).collect();
+    let _: Vec<(String, String)> = a.drain().collect();
+
+    let mut b = vec!["aaa".to_string(), "bbb".to_string()];
+    let _: Vec<_> = b.drain(0..a.len()).collect();
+}
+
+#[derive(Default)]
+struct Bomb {
+    fire: Vec<u8>,
+}
+
+fn should_not_help_0(bomb: &mut Bomb) {
+    let _: Vec<u8> = bomb.fire.drain(..).collect();
+}
+
+fn main() {
+    full();
+    closed();
+    should_not_help();
+    should_not_help_0(&mut Bomb::default());
+}
diff --git a/tests/ui/iter_with_drain.stderr b/tests/ui/iter_with_drain.stderr
new file mode 100644
index 00000000000..aa394439fa6
--- /dev/null
+++ b/tests/ui/iter_with_drain.stderr
@@ -0,0 +1,40 @@
+error: `drain(..)` used on a `Vec`
+  --> $DIR/iter_with_drain.rs:11:34
+   |
+LL |     let mut a: BinaryHeap<_> = a.drain(..).collect();
+   |                                  ^^^^^^^^^ help: try this: `into_iter()`
+   |
+   = note: `-D clippy::iter-with-drain` implied by `-D warnings`
+
+error: `drain(..)` used on a `VecDeque`
+  --> $DIR/iter_with_drain.rs:14:27
+   |
+LL |     let mut a: Vec<_> = a.drain(..).collect();
+   |                           ^^^^^^^^^ help: try this: `into_iter()`
+
+error: `drain(..)` used on a `Vec`
+  --> $DIR/iter_with_drain.rs:15:34
+   |
+LL |     let mut a: HashMap<_, _> = a.drain(..).map(|x| (x.clone(), x)).collect();
+   |                                  ^^^^^^^^^ help: try this: `into_iter()`
+
+error: `drain(..)` used on a `Vec`
+  --> $DIR/iter_with_drain.rs:21:34
+   |
+LL |     let mut a: BinaryHeap<_> = a.drain(0..).collect();
+   |                                  ^^^^^^^^^^ help: try this: `into_iter()`
+
+error: `drain(..)` used on a `VecDeque`
+  --> $DIR/iter_with_drain.rs:24:27
+   |
+LL |     let mut a: Vec<_> = a.drain(..a.len()).collect();
+   |                           ^^^^^^^^^^^^^^^^ help: try this: `into_iter()`
+
+error: `drain(..)` used on a `Vec`
+  --> $DIR/iter_with_drain.rs:25:34
+   |
+LL |     let mut a: HashMap<_, _> = a.drain(0..a.len()).map(|x| (x.clone(), x)).collect();
+   |                                  ^^^^^^^^^^^^^^^^^ help: try this: `into_iter()`
+
+error: aborting due to 6 previous errors
+
diff --git a/tests/ui/range_plus_minus_one.fixed b/tests/ui/range_plus_minus_one.fixed
index 19b253b0fe2..40d7791df28 100644
--- a/tests/ui/range_plus_minus_one.fixed
+++ b/tests/ui/range_plus_minus_one.fixed
@@ -1,7 +1,7 @@
 // run-rustfix
 
 #![allow(unused_parens)]
-
+#![allow(clippy::iter_with_drain)]
 fn f() -> usize {
     42
 }
diff --git a/tests/ui/range_plus_minus_one.rs b/tests/ui/range_plus_minus_one.rs
index 7d034117547..a8ddd9b5f75 100644
--- a/tests/ui/range_plus_minus_one.rs
+++ b/tests/ui/range_plus_minus_one.rs
@@ -1,7 +1,7 @@
 // run-rustfix
 
 #![allow(unused_parens)]
-
+#![allow(clippy::iter_with_drain)]
 fn f() -> usize {
     42
 }