diff --git a/clippy_lints/src/methods/drain_collect.rs b/clippy_lints/src/methods/drain_collect.rs index 6d9df587518..53f63266073 100644 --- 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::>()` /// ^^^^^^^^^ ^^^^^^^^^^ 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, ""); 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 new file mode 100644 index 00000000000..0d40a648378 --- /dev/null +++ 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) -> BinaryHeap { + std::mem::take(b) +} + +fn binaryheap_dont_lint(b: &mut BinaryHeap) -> HashSet { + b.drain().collect() +} + +fn hashmap(b: &mut HashMap) -> HashMap { + std::mem::take(b) +} + +fn hashmap_dont_lint(b: &mut HashMap) -> Vec<(i32, i32)> { + b.drain().collect() +} + +fn hashset(b: &mut HashSet) -> HashSet { + std::mem::take(b) +} + +fn hashset_dont_lint(b: &mut HashSet) -> Vec { + b.drain().collect() +} + +fn vecdeque(b: &mut VecDeque) -> VecDeque { + std::mem::take(b) +} + +fn vecdeque_dont_lint(b: &mut VecDeque) -> HashSet { + b.drain(..).collect() +} + +fn vec(b: &mut Vec) -> Vec { + std::mem::take(b) +} + +fn vec2(b: &mut Vec) -> Vec { + std::mem::take(b) +} + +fn vec3(b: &mut Vec) -> Vec { + std::mem::take(b) +} + +fn vec4(b: &mut Vec) -> Vec { + std::mem::take(b) +} + +fn vec_no_reborrow() -> Vec { + let mut b = vec![1, 2, 3]; + std::mem::take(&mut b) +} + +fn vec_dont_lint(b: &mut Vec) -> HashSet { + b.drain(..).collect() +} + +fn string(b: &mut String) -> String { + std::mem::take(b) +} + +fn string_dont_lint(b: &mut String) -> HashSet { + b.drain(..).collect() +} + +fn main() {} diff --git a/tests/ui/drain_collect.rs b/tests/ui/drain_collect.rs index 1bedab75c03..7144a1847ca 100644 --- 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) -> Vec { b.drain(0..b.len()).collect() } +fn vec_no_reborrow() -> Vec { + let mut b = vec![1, 2, 3]; + b.drain(..).collect() +} + fn vec_dont_lint(b: &mut Vec) -> HashSet { b.drain(..).collect() } diff --git a/tests/ui/drain_collect.stderr b/tests/ui/drain_collect.stderr index d4e2b4a6fa2..0792f0254cb 100644 --- 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