Auto merge of rust-lang#10835 - y21:drain-collect, r=dswij

new lint: `drain_collect`

Closes rust-lang#10818.

This adds a new lint that looks for `.drain(..).collect()` and suggests replacing it with `mem::take`.

changelog: [`drain_collect`]: new lint

Author: bors

CHANGELOG.md

@@ -4737,6 +4737,7 @@ Released 2018-09-13
 [`double_must_use`]: https://rust-lang.github.io/rust-clippy/master/index.html#double_must_use
 [`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
+[`drain_collect`]: https://rust-lang.github.io/rust-clippy/master/index.html#drain_collect
 [`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_non_drop`]: https://rust-lang.github.io/rust-clippy/master/index.html#drop_non_drop

clippy_lints/src/declared_lints.rs

@@ -324,6 +324,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
     crate::methods::CLONE_ON_COPY_INFO,
     crate::methods::CLONE_ON_REF_PTR_INFO,
     crate::methods::COLLAPSIBLE_STR_REPLACE_INFO,
+    crate::methods::DRAIN_COLLECT_INFO,
     crate::methods::ERR_EXPECT_INFO,
     crate::methods::EXPECT_FUN_CALL_INFO,
     crate::methods::EXPECT_USED_INFO,

clippy_lints/src/methods/drain_collect.rs

@@ -0,0 +1,85 @@
+use crate::methods::DRAIN_COLLECT;
+use clippy_utils::diagnostics::span_lint_and_sugg;
+use clippy_utils::is_range_full;
+use clippy_utils::source::snippet;
+use clippy_utils::ty::is_type_lang_item;
+use rustc_errors::Applicability;
+use rustc_hir::Expr;
+use rustc_hir::ExprKind;
+use rustc_hir::LangItem;
+use rustc_hir::Path;
+use rustc_hir::QPath;
+use rustc_lint::LateContext;
+use rustc_middle::query::Key;
+use rustc_middle::ty;
+use rustc_middle::ty::Ty;
+use rustc_span::sym;
+use rustc_span::Symbol;
+
+/// Checks if both types match the given diagnostic item, e.g.:
+///
+/// `vec![1,2].drain(..).collect::<Vec<_>>()`
+///  ^^^^^^^^^                     ^^^^^^   true
+/// `vec![1,2].drain(..).collect::<HashSet<_>>()`
+///  ^^^^^^^^^                     ^^^^^^^^^^  false
+fn types_match_diagnostic_item(cx: &LateContext<'_>, expr: Ty<'_>, recv: Ty<'_>, sym: Symbol) -> bool {
+    if let Some(expr_adt_did) = expr.ty_adt_id()
+        && let Some(recv_adt_did) = recv.ty_adt_id()
+    {
+        cx.tcx.is_diagnostic_item(sym, expr_adt_did) && cx.tcx.is_diagnostic_item(sym, recv_adt_did)
+    } else {
+        false
+    }
+}
+
+/// Checks `std::{vec::Vec, collections::VecDeque}`.
+fn check_vec(cx: &LateContext<'_>, args: &[Expr<'_>], expr: Ty<'_>, recv: Ty<'_>, recv_path: &Path<'_>) -> bool {
+    (types_match_diagnostic_item(cx, expr, recv, sym::Vec)
+        || types_match_diagnostic_item(cx, expr, recv, sym::VecDeque))
+        && matches!(args, [arg] if is_range_full(cx, arg, Some(recv_path)))
+}
+
+/// Checks `std::string::String`
+fn check_string(cx: &LateContext<'_>, args: &[Expr<'_>], expr: Ty<'_>, recv: Ty<'_>, recv_path: &Path<'_>) -> bool {
+    is_type_lang_item(cx, expr, LangItem::String)
+        && is_type_lang_item(cx, recv, LangItem::String)
+        && matches!(args, [arg] if is_range_full(cx, arg, Some(recv_path)))
+}
+
+/// Checks `std::collections::{HashSet, HashMap, BinaryHeap}`.
+fn check_collections(cx: &LateContext<'_>, expr: Ty<'_>, recv: Ty<'_>) -> Option<&'static str> {
+    types_match_diagnostic_item(cx, expr, recv, sym::HashSet)
+        .then_some("HashSet")
+        .or_else(|| types_match_diagnostic_item(cx, expr, recv, sym::HashMap).then_some("HashMap"))
+        .or_else(|| types_match_diagnostic_item(cx, expr, recv, sym::BinaryHeap).then_some("BinaryHeap"))
+}
+
+pub(super) fn check(cx: &LateContext<'_>, args: &[Expr<'_>], expr: &Expr<'_>, recv: &Expr<'_>) {
+    let expr_ty = cx.typeck_results().expr_ty(expr);
+    let recv_ty = cx.typeck_results().expr_ty(recv);
+    let recv_ty_no_refs = recv_ty.peel_refs();
+
+    if let ExprKind::Path(QPath::Resolved(_, recv_path)) = recv.kind
+        && let Some(typename) = check_vec(cx, args, expr_ty, recv_ty_no_refs, recv_path)
+            .then_some("Vec")
+            .or_else(|| check_string(cx, args, expr_ty, recv_ty_no_refs, recv_path).then_some("String"))
+            .or_else(|| check_collections(cx, expr_ty, recv_ty_no_refs))
+    {
+        let recv = snippet(cx, recv.span, "<expr>");
+        let sugg = if let ty::Ref(..) = recv_ty.kind() {
+            format!("std::mem::take({recv})")
+        } else {
+            format!("std::mem::take(&mut {recv})")
+        };
+
+        span_lint_and_sugg(
+            cx,
+            DRAIN_COLLECT,
+            expr.span,
+            &format!("you seem to be trying to move all elements into a new `{typename}`"),
+            "consider using `mem::take`",
+            sugg,
+            Applicability::MachineApplicable,
+        );
+    }
+}

clippy_lints/src/methods/mod.rs

@@ -14,6 +14,7 @@ mod clone_on_copy;
 mod clone_on_ref_ptr;
 mod cloned_instead_of_copied;
 mod collapsible_str_replace;
+mod drain_collect;
 mod err_expect;
 mod expect_fun_call;
 mod expect_used;
@@ -3247,6 +3248,42 @@ declare_clippy_lint! {
     "manual reverse iteration of `DoubleEndedIterator`"
 }
 
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for calls to `.drain()` that clear the collection, immediately followed by a call to `.collect()`.
+    ///
+    /// > "Collection" in this context refers to any type with a `drain` method:
+    /// > `Vec`, `VecDeque`, `BinaryHeap`, `HashSet`,`HashMap`, `String`
+    ///
+    /// ### Why is this bad?
+    /// Using `mem::take` is faster as it avoids the allocation.
+    /// When using `mem::take`, the old collection is replaced with an empty one and ownership of
+    /// the old collection is returned.
+    ///
+    /// ### Known issues
+    /// `mem::take(&mut vec)` is almost equivalent to `vec.drain(..).collect()`, except that
+    /// it also moves the **capacity**. The user might have explicitly written it this way
+    /// to keep the capacity on the original `Vec`.
+    ///
+    /// ### Example
+    /// ```rust
+    /// fn remove_all(v: &mut Vec<i32>) -> Vec<i32> {
+    ///     v.drain(..).collect()
+    /// }
+    /// ```
+    /// Use instead:
+    /// ```rust
+    /// use std::mem;
+    /// fn remove_all(v: &mut Vec<i32>) -> Vec<i32> {
+    ///     mem::take(v)
+    /// }
+    /// ```
+    #[clippy::version = "1.71.0"]
+    pub DRAIN_COLLECT,
+    perf,
+    "calling `.drain(..).collect()` to move all elements into a new collection"
+}
+
 pub struct Methods {
     avoid_breaking_exported_api: bool,
     msrv: Msrv,
@@ -3377,6 +3414,7 @@ impl_lint_pass!(Methods => [
     CLEAR_WITH_DRAIN,
     MANUAL_NEXT_BACK,
     UNNECESSARY_LITERAL_UNWRAP,
+    DRAIN_COLLECT
 ]);
 
 /// Extracts a method call name, args, and `Span` of the method name.
@@ -3606,6 +3644,9 @@ impl Methods {
                                 manual_str_repeat::check(cx, expr, recv, take_self_arg, take_arg);
                             }
                         },
+                        Some(("drain", recv, args, ..)) => {
+                            drain_collect::check(cx, args, expr, recv);
+                        }
                         _ => {},
                     }
                 },

tests/ui/drain_collect.fixed

@@ -0,0 +1,77 @@
+//@run-rustfix
+
+#![deny(clippy::drain_collect)]
+#![allow(dead_code)]
+
+use std::collections::{BinaryHeap, HashMap, HashSet, VecDeque};
+
+fn binaryheap(b: &mut BinaryHeap<i32>) -> BinaryHeap<i32> {
+    std::mem::take(b)
+}
+
+fn binaryheap_dont_lint(b: &mut BinaryHeap<i32>) -> HashSet<i32> {
+    b.drain().collect()
+}
+
+fn hashmap(b: &mut HashMap<i32, i32>) -> HashMap<i32, i32> {
+    std::mem::take(b)
+}
+
+fn hashmap_dont_lint(b: &mut HashMap<i32, i32>) -> Vec<(i32, i32)> {
+    b.drain().collect()
+}
+
+fn hashset(b: &mut HashSet<i32>) -> HashSet<i32> {
+    std::mem::take(b)
+}
+
+fn hashset_dont_lint(b: &mut HashSet<i32>) -> Vec<i32> {
+    b.drain().collect()
+}
+
+fn vecdeque(b: &mut VecDeque<i32>) -> VecDeque<i32> {
+    std::mem::take(b)
+}
+
+fn vecdeque_dont_lint(b: &mut VecDeque<i32>) -> HashSet<i32> {
+    b.drain(..).collect()
+}
+
+fn vec(b: &mut Vec<i32>) -> Vec<i32> {
+    std::mem::take(b)
+}
+
+fn vec2(b: &mut Vec<i32>) -> Vec<i32> {
+    std::mem::take(b)
+}
+
+fn vec3(b: &mut Vec<i32>) -> Vec<i32> {
+    std::mem::take(b)
+}
+
+fn vec4(b: &mut Vec<i32>) -> Vec<i32> {
+    std::mem::take(b)
+}
+
+fn vec_no_reborrow() -> Vec<i32> {
+    let mut b = vec![1, 2, 3];
+    std::mem::take(&mut b)
+}
+
+fn vec_dont_lint(b: &mut Vec<i32>) -> HashSet<i32> {
+    b.drain(..).collect()
+}
+
+fn string(b: &mut String) -> String {
+    std::mem::take(b)
+}
+
+fn string_dont_lint(b: &mut String) -> HashSet<char> {
+    b.drain(..).collect()
+}
+
+fn not_whole_length(v: &mut Vec<i32>) -> Vec<i32> {
+    v.drain(1..).collect()
+}
+
+fn main() {}

tests/ui/drain_collect.rs

@@ -0,0 +1,77 @@
+//@run-rustfix
+
+#![deny(clippy::drain_collect)]
+#![allow(dead_code)]
+
+use std::collections::{BinaryHeap, HashMap, HashSet, VecDeque};
+
+fn binaryheap(b: &mut BinaryHeap<i32>) -> BinaryHeap<i32> {
+    b.drain().collect()
+}
+
+fn binaryheap_dont_lint(b: &mut BinaryHeap<i32>) -> HashSet<i32> {
+    b.drain().collect()
+}
+
+fn hashmap(b: &mut HashMap<i32, i32>) -> HashMap<i32, i32> {
+    b.drain().collect()
+}
+
+fn hashmap_dont_lint(b: &mut HashMap<i32, i32>) -> Vec<(i32, i32)> {
+    b.drain().collect()
+}
+
+fn hashset(b: &mut HashSet<i32>) -> HashSet<i32> {
+    b.drain().collect()
+}
+
+fn hashset_dont_lint(b: &mut HashSet<i32>) -> Vec<i32> {
+    b.drain().collect()
+}
+
+fn vecdeque(b: &mut VecDeque<i32>) -> VecDeque<i32> {
+    b.drain(..).collect()
+}
+
+fn vecdeque_dont_lint(b: &mut VecDeque<i32>) -> HashSet<i32> {
+    b.drain(..).collect()
+}
+
+fn vec(b: &mut Vec<i32>) -> Vec<i32> {
+    b.drain(..).collect()
+}
+
+fn vec2(b: &mut Vec<i32>) -> Vec<i32> {
+    b.drain(0..).collect()
+}
+
+fn vec3(b: &mut Vec<i32>) -> Vec<i32> {
+    b.drain(..b.len()).collect()
+}
+
+fn vec4(b: &mut Vec<i32>) -> Vec<i32> {
+    b.drain(0..b.len()).collect()
+}
+
+fn vec_no_reborrow() -> Vec<i32> {
+    let mut b = vec![1, 2, 3];
+    b.drain(..).collect()
+}
+
+fn vec_dont_lint(b: &mut Vec<i32>) -> HashSet<i32> {
+    b.drain(..).collect()
+}
+
+fn string(b: &mut String) -> String {
+    b.drain(..).collect()
+}
+
+fn string_dont_lint(b: &mut String) -> HashSet<char> {
+    b.drain(..).collect()
+}
+
+fn not_whole_length(v: &mut Vec<i32>) -> Vec<i32> {
+    v.drain(1..).collect()
+}
+
+fn main() {}

tests/ui/drain_collect.stderr

@@ -0,0 +1,68 @@
+error: you seem to be trying to move all elements into a new `BinaryHeap`
+  --> $DIR/drain_collect.rs:9:5
+   |
+LL |     b.drain().collect()
+   |     ^^^^^^^^^^^^^^^^^^^ help: consider using `mem::take`: `std::mem::take(b)`
+   |
+note: the lint level is defined here
+  --> $DIR/drain_collect.rs:3:9
+   |
+LL | #![deny(clippy::drain_collect)]
+   |         ^^^^^^^^^^^^^^^^^^^^^
+
+error: you seem to be trying to move all elements into a new `HashMap`
+  --> $DIR/drain_collect.rs:17:5
+   |
+LL |     b.drain().collect()
+   |     ^^^^^^^^^^^^^^^^^^^ help: consider using `mem::take`: `std::mem::take(b)`
+
+error: you seem to be trying to move all elements into a new `HashSet`
+  --> $DIR/drain_collect.rs:25:5
+   |
+LL |     b.drain().collect()
+   |     ^^^^^^^^^^^^^^^^^^^ help: consider using `mem::take`: `std::mem::take(b)`
+
+error: you seem to be trying to move all elements into a new `Vec`
+  --> $DIR/drain_collect.rs:33:5
+   |
+LL |     b.drain(..).collect()
+   |     ^^^^^^^^^^^^^^^^^^^^^ help: consider using `mem::take`: `std::mem::take(b)`
+
+error: you seem to be trying to move all elements into a new `Vec`
+  --> $DIR/drain_collect.rs:41:5
+   |
+LL |     b.drain(..).collect()
+   |     ^^^^^^^^^^^^^^^^^^^^^ help: consider using `mem::take`: `std::mem::take(b)`
+
+error: you seem to be trying to move all elements into a new `Vec`
+  --> $DIR/drain_collect.rs:45:5
+   |
+LL |     b.drain(0..).collect()
+   |     ^^^^^^^^^^^^^^^^^^^^^^ help: consider using `mem::take`: `std::mem::take(b)`
+
+error: you seem to be trying to move all elements into a new `Vec`
+  --> $DIR/drain_collect.rs:49:5
+   |
+LL |     b.drain(..b.len()).collect()
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `mem::take`: `std::mem::take(b)`
+
+error: you seem to be trying to move all elements into a new `Vec`
+  --> $DIR/drain_collect.rs:53:5
+   |
+LL |     b.drain(0..b.len()).collect()
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `mem::take`: `std::mem::take(b)`
+
+error: you seem to be trying to move all elements into a new `Vec`
+  --> $DIR/drain_collect.rs:58:5
+   |
+LL |     b.drain(..).collect()
+   |     ^^^^^^^^^^^^^^^^^^^^^ help: consider using `mem::take`: `std::mem::take(&mut b)`
+
+error: you seem to be trying to move all elements into a new `String`
+  --> $DIR/drain_collect.rs:66:5
+   |
+LL |     b.drain(..).collect()
+   |     ^^^^^^^^^^^^^^^^^^^^^ help: consider using `mem::take`: `std::mem::take(b)`
+
+error: aborting due to 10 previous errors
+