take into account reborrowing when inserting &mut in sugg

y21 · y21 · commit d2a6ec2d4d79 · 2023-06-15T20:23:12.000+02:00
diff --git a/clippy_lints/src/methods/drain_collect.rs b/clippy_lints/src/methods/drain_collect.rs
@@ -11,7 +11,7 @@ use rustc_hir::Path;
 use rustc_hir::QPath;
 use rustc_lint::LateContext;
 use rustc_middle::query::Key;
-use rustc_middle::ty::Ty;
+use rustc_middle::ty;
 use rustc_span::sym;
 use rustc_span::Symbol;
 
@@ -21,7 +21,7 @@ use rustc_span::Symbol;
 ///  ^^^^^^^^^                     ^^^^^^   true
 /// `vec![1,2].drain(..).collect::<HashSet<_>>()`
 ///  ^^^^^^^^^                     ^^^^^^^^^^  false
-fn types_match_diagnostic_item(cx: &LateContext<'_>, expr: Ty<'_>, recv: Ty<'_>, sym: Symbol) -> bool {
+fn types_match_diagnostic_item(cx: &LateContext<'_>, expr: ty::Ty<'_>, recv: ty::Ty<'_>, sym: Symbol) -> bool {
     if let Some(expr_adt_did) = expr.ty_adt_id()
         && let Some(recv_adt_did) = recv.ty_adt_id()
     {
@@ -32,21 +32,33 @@ fn types_match_diagnostic_item(cx: &LateContext<'_>, expr: Ty<'_>, recv: Ty<'_>,
 }
 
 /// Checks `std::{vec::Vec, collections::VecDeque}`.
-fn check_vec(cx: &LateContext<'_>, args: &[Expr<'_>], expr: Ty<'_>, recv: Ty<'_>, recv_path: &Path<'_>) -> bool {
+fn check_vec(
+    cx: &LateContext<'_>,
+    args: &[Expr<'_>],
+    expr: ty::Ty<'_>,
+    recv: ty::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 {
+fn check_string(
+    cx: &LateContext<'_>,
+    args: &[Expr<'_>],
+    expr: ty::Ty<'_>,
+    recv: ty::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> {
+fn check_collections(cx: &LateContext<'_>, expr: ty::Ty<'_>, recv: ty::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"))
@@ -55,13 +67,14 @@ fn check_collections(cx: &LateContext<'_>, expr: Ty<'_>, recv: Ty<'_>) -> Option
 
 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).peel_refs();
+    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, recv_path)
+        && 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, recv_path).then_some("String"))
-            .or_else(|| check_collections(cx, expr_ty, recv_ty))
+            .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>");
         span_lint_and_sugg(
@@ -70,7 +83,10 @@ pub(super) fn check(cx: &LateContext<'_>, args: &[Expr<'_>], expr: &Expr<'_>, re
             expr.span,
             &format!("you seem to be trying to move all elements into a new `{typename}`"),
             "consider using `mem::take`",
-            format!("std::mem::take(&mut {recv})"),
+            match recv_ty.kind() {
+                ty::Ref(..) => format!("std::mem::take({recv})"),
+                _ => format!("std::mem::take(&mut {recv})"),
+            },
             Applicability::MachineApplicable,
         );
     }
diff --git a/tests/ui/drain_collect.fixed b/tests/ui/drain_collect.fixed
@@ -0,0 +1,73 @@
+//@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 main() {}
diff --git a/tests/ui/drain_collect.rs b/tests/ui/drain_collect.rs
@@ -1,4 +1,7 @@
+//@run-rustfix
+
 #![deny(clippy::drain_collect)]
+#![allow(dead_code)]
 
 use std::collections::{BinaryHeap, HashMap, HashSet, VecDeque};
 
@@ -50,6 +53,11 @@ 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()
 }
diff --git a/tests/ui/drain_collect.stderr b/tests/ui/drain_collect.stderr
@@ -1,62 +1,68 @@
 error: you seem to be trying to move all elements into a new `BinaryHeap`
-  --> $DIR/drain_collect.rs:6:5
+  --> $DIR/drain_collect.rs:9:5
    |
 LL |     b.drain().collect()
-   |     ^^^^^^^^^^^^^^^^^^^ help: consider using `mem::take`: `std::mem::take(&mut b)`
+   |     ^^^^^^^^^^^^^^^^^^^ help: consider using `mem::take`: `std::mem::take(b)`
    |
 note: the lint level is defined here
-  --> $DIR/drain_collect.rs:1:9
+  --> $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:14:5
+  --> $DIR/drain_collect.rs:17:5
    |
 LL |     b.drain().collect()
-   |     ^^^^^^^^^^^^^^^^^^^ help: consider using `mem::take`: `std::mem::take(&mut b)`
+   |     ^^^^^^^^^^^^^^^^^^^ 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:22:5
+  --> $DIR/drain_collect.rs:25:5
    |
 LL |     b.drain().collect()
-   |     ^^^^^^^^^^^^^^^^^^^ help: consider using `mem::take`: `std::mem::take(&mut b)`
+   |     ^^^^^^^^^^^^^^^^^^^ 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:30:5
+  --> $DIR/drain_collect.rs:33:5
    |
 LL |     b.drain(..).collect()
-   |     ^^^^^^^^^^^^^^^^^^^^^ help: consider using `mem::take`: `std::mem::take(&mut b)`
+   |     ^^^^^^^^^^^^^^^^^^^^^ 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:38:5
+  --> $DIR/drain_collect.rs:41:5
    |
 LL |     b.drain(..).collect()
-   |     ^^^^^^^^^^^^^^^^^^^^^ help: consider using `mem::take`: `std::mem::take(&mut b)`
+   |     ^^^^^^^^^^^^^^^^^^^^^ 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:42:5
+  --> $DIR/drain_collect.rs:45:5
    |
 LL |     b.drain(0..).collect()
-   |     ^^^^^^^^^^^^^^^^^^^^^^ help: consider using `mem::take`: `std::mem::take(&mut b)`
+   |     ^^^^^^^^^^^^^^^^^^^^^^ 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:46:5
+  --> $DIR/drain_collect.rs:49:5
    |
 LL |     b.drain(..b.len()).collect()
-   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `mem::take`: `std::mem::take(&mut b)`
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 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:50:5
+  --> $DIR/drain_collect.rs:53:5
    |
 LL |     b.drain(0..b.len()).collect()
-   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `mem::take`: `std::mem::take(&mut b)`
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `mem::take`: `std::mem::take(b)`
 
-error: you seem to be trying to move all elements into a new `String`
+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: aborting due to 9 previous errors
+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
 
