From 2748ab9565100dedfe170c899fe1f7f0791677d2 Mon Sep 17 00:00:00 2001 From: y21 <30553356+y21@users.noreply.github.com> Date: Sat, 27 May 2023 00:58:44 +0200 Subject: [PATCH 1/5] new lint: `drain_collect` --- CHANGELOG.md | 1 + clippy_lints/src/declared_lints.rs | 1 + clippy_lints/src/methods/drain_collect.rs | 77 +++++++++++++++++++++++ clippy_lints/src/methods/mod.rs | 41 ++++++++++++ tests/ui/drain_collect.rs | 65 +++++++++++++++++++ tests/ui/drain_collect.stderr | 62 ++++++++++++++++++ tests/ui/iter_with_drain.fixed | 2 +- tests/ui/iter_with_drain.rs | 2 +- 8 files changed, 249 insertions(+), 2 deletions(-) create mode 100644 clippy_lints/src/methods/drain_collect.rs create mode 100644 tests/ui/drain_collect.rs create mode 100644 tests/ui/drain_collect.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 218bca40cb0..ad7a101d38a 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 9509e6d016a..b726f343ffe 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/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, diff --git a/clippy_lints/src/methods/drain_collect.rs b/clippy_lints/src/methods/drain_collect.rs new file mode 100644 index 00000000000..6d9df587518 --- /dev/null +++ b/clippy_lints/src/methods/drain_collect.rs @@ -0,0 +1,77 @@ +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::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::>()` +/// ^^^^^^^^^ ^^^^^^ true +/// `vec![1,2].drain(..).collect::>()` +/// ^^^^^^^^^ ^^^^^^^^^^ 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).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) + .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)) + { + let recv = snippet(cx, recv.span, ""); + 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`", + format!("std::mem::take(&mut {recv})"), + Applicability::MachineApplicable, + ); + } +} diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 183bd582a48..faddb036eaa 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/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. + /// + /// ### Drawback + /// `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) -> Vec { + /// v.drain(..).collect() + /// } + /// ``` + /// Use instead: + /// ```rust + /// use std::mem; + /// fn remove_all(v: &mut Vec) -> Vec { + /// mem::take(v) + /// } + /// ``` + #[clippy::version = "1.71.0"] + pub DRAIN_COLLECT, + perf, + "description" +} + 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); + } _ => {}, } }, diff --git a/tests/ui/drain_collect.rs b/tests/ui/drain_collect.rs new file mode 100644 index 00000000000..1bedab75c03 --- /dev/null +++ b/tests/ui/drain_collect.rs @@ -0,0 +1,65 @@ +#![deny(clippy::drain_collect)] + +use std::collections::{BinaryHeap, HashMap, HashSet, VecDeque}; + +fn binaryheap(b: &mut BinaryHeap) -> BinaryHeap { + b.drain().collect() +} + +fn binaryheap_dont_lint(b: &mut BinaryHeap) -> HashSet { + b.drain().collect() +} + +fn hashmap(b: &mut HashMap) -> HashMap { + b.drain().collect() +} + +fn hashmap_dont_lint(b: &mut HashMap) -> Vec<(i32, i32)> { + b.drain().collect() +} + +fn hashset(b: &mut HashSet) -> HashSet { + b.drain().collect() +} + +fn hashset_dont_lint(b: &mut HashSet) -> Vec { + b.drain().collect() +} + +fn vecdeque(b: &mut VecDeque) -> VecDeque { + b.drain(..).collect() +} + +fn vecdeque_dont_lint(b: &mut VecDeque) -> HashSet { + b.drain(..).collect() +} + +fn vec(b: &mut Vec) -> Vec { + b.drain(..).collect() +} + +fn vec2(b: &mut Vec) -> Vec { + b.drain(0..).collect() +} + +fn vec3(b: &mut Vec) -> Vec { + b.drain(..b.len()).collect() +} + +fn vec4(b: &mut Vec) -> Vec { + b.drain(0..b.len()).collect() +} + +fn vec_dont_lint(b: &mut Vec) -> HashSet { + b.drain(..).collect() +} + +fn string(b: &mut String) -> String { + b.drain(..).collect() +} + +fn string_dont_lint(b: &mut String) -> HashSet { + b.drain(..).collect() +} + +fn main() {} diff --git a/tests/ui/drain_collect.stderr b/tests/ui/drain_collect.stderr new file mode 100644 index 00000000000..d4e2b4a6fa2 --- /dev/null +++ b/tests/ui/drain_collect.stderr @@ -0,0 +1,62 @@ +error: you seem to be trying to move all elements into a new `BinaryHeap` + --> $DIR/drain_collect.rs:6:5 + | +LL | b.drain().collect() + | ^^^^^^^^^^^^^^^^^^^ help: consider using `mem::take`: `std::mem::take(&mut b)` + | +note: the lint level is defined here + --> $DIR/drain_collect.rs:1: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 + | +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 `HashSet` + --> $DIR/drain_collect.rs:22: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 `Vec` + --> $DIR/drain_collect.rs:30: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 `Vec` + --> $DIR/drain_collect.rs:38: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 `Vec` + --> $DIR/drain_collect.rs:42:5 + | +LL | b.drain(0..).collect() + | ^^^^^^^^^^^^^^^^^^^^^^ help: consider using `mem::take`: `std::mem::take(&mut b)` + +error: you seem to be trying to move all elements into a new `Vec` + --> $DIR/drain_collect.rs:46:5 + | +LL | b.drain(..b.len()).collect() + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `mem::take`: `std::mem::take(&mut b)` + +error: you seem to be trying to move all elements into a new `Vec` + --> $DIR/drain_collect.rs:50:5 + | +LL | b.drain(0..b.len()).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:58:5 + | +LL | b.drain(..).collect() + | ^^^^^^^^^^^^^^^^^^^^^ help: consider using `mem::take`: `std::mem::take(&mut b)` + +error: aborting due to 9 previous errors + diff --git a/tests/ui/iter_with_drain.fixed b/tests/ui/iter_with_drain.fixed index 24a95c4d0fe..7a8c6770101 100644 --- a/tests/ui/iter_with_drain.fixed +++ b/tests/ui/iter_with_drain.fixed @@ -2,7 +2,7 @@ // will emits unused mut warnings after fixing #![allow(unused_mut)] // will emits needless collect warnings after fixing -#![allow(clippy::needless_collect)] +#![allow(clippy::needless_collect, clippy::drain_collect)] #![warn(clippy::iter_with_drain)] use std::collections::{BinaryHeap, HashMap, HashSet, VecDeque}; diff --git a/tests/ui/iter_with_drain.rs b/tests/ui/iter_with_drain.rs index a118c981ee3..cf3a935c349 100644 --- a/tests/ui/iter_with_drain.rs +++ b/tests/ui/iter_with_drain.rs @@ -2,7 +2,7 @@ // will emits unused mut warnings after fixing #![allow(unused_mut)] // will emits needless collect warnings after fixing -#![allow(clippy::needless_collect)] +#![allow(clippy::needless_collect, clippy::drain_collect)] #![warn(clippy::iter_with_drain)] use std::collections::{BinaryHeap, HashMap, HashSet, VecDeque}; From d2a6ec2d4d7930401d2600edfdac1cd964ebe55c Mon Sep 17 00:00:00 2001 From: y21 <30553356+y21@users.noreply.github.com> Date: Sat, 27 May 2023 01:31:55 +0200 Subject: [PATCH 2/5] take into account reborrowing when inserting `&mut` in sugg --- clippy_lints/src/methods/drain_collect.rs | 36 +++++++---- tests/ui/drain_collect.fixed | 73 +++++++++++++++++++++++ tests/ui/drain_collect.rs | 8 +++ tests/ui/drain_collect.stderr | 44 ++++++++------ 4 files changed, 132 insertions(+), 29 deletions(-) create mode 100644 tests/ui/drain_collect.fixed 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 From 3f3657a3e4ae745339e42e377bef57f2f56ed5ff Mon Sep 17 00:00:00 2001 From: y21 <30553356+y21@users.noreply.github.com> Date: Sat, 27 May 2023 01:50:36 +0200 Subject: [PATCH 3/5] make clippy happy --- clippy_lints/src/methods/drain_collect.rs | 28 ++++++++--------------- 1 file changed, 9 insertions(+), 19 deletions(-) diff --git a/clippy_lints/src/methods/drain_collect.rs b/clippy_lints/src/methods/drain_collect.rs index 53f63266073..8bd8f88a214 100644 --- a/clippy_lints/src/methods/drain_collect.rs +++ b/clippy_lints/src/methods/drain_collect.rs @@ -12,6 +12,7 @@ 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; @@ -21,7 +22,7 @@ use rustc_span::Symbol; /// ^^^^^^^^^ ^^^^^^ true /// `vec![1,2].drain(..).collect::>()` /// ^^^^^^^^^ ^^^^^^^^^^ false -fn types_match_diagnostic_item(cx: &LateContext<'_>, expr: ty::Ty<'_>, recv: ty::Ty<'_>, sym: Symbol) -> bool { +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() { @@ -32,33 +33,21 @@ fn types_match_diagnostic_item(cx: &LateContext<'_>, expr: ty::Ty<'_>, recv: ty: } /// Checks `std::{vec::Vec, collections::VecDeque}`. -fn check_vec( - cx: &LateContext<'_>, - args: &[Expr<'_>], - expr: ty::Ty<'_>, - recv: ty::Ty<'_>, - recv_path: &Path<'_>, -) -> bool { +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::Ty<'_>, - recv: ty::Ty<'_>, - recv_path: &Path<'_>, -) -> bool { +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::Ty<'_>, recv: ty::Ty<'_>) -> Option<&'static str> { +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")) @@ -83,9 +72,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`", - match recv_ty.kind() { - ty::Ref(..) => format!("std::mem::take({recv})"), - _ => format!("std::mem::take(&mut {recv})"), + if let ty::Ref(..) = recv_ty.kind() { + format!("std::mem::take({recv})") + } else { + format!("std::mem::take(&mut {recv})") }, Applicability::MachineApplicable, ); From 20ae597ec4dc593bb07bcd436b3ca42fcb64df42 Mon Sep 17 00:00:00 2001 From: y21 <30553356+y21@users.noreply.github.com> Date: Thu, 8 Jun 2023 19:54:33 +0200 Subject: [PATCH 4/5] add a description --- clippy_lints/src/methods/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index faddb036eaa..bed70207f28 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -3281,7 +3281,7 @@ declare_clippy_lint! { #[clippy::version = "1.71.0"] pub DRAIN_COLLECT, perf, - "description" + "calling `.drain(..).collect()` to move all elements into a new collection" } pub struct Methods { From 5821fbbc3072c16bc941a5e207bfd3a5e401304b Mon Sep 17 00:00:00 2001 From: y21 <30553356+y21@users.noreply.github.com> Date: Thu, 15 Jun 2023 14:16:39 +0200 Subject: [PATCH 5/5] add test case for not whole length, move sugg into variable --- clippy_lints/src/methods/drain_collect.rs | 12 +++++++----- clippy_lints/src/methods/mod.rs | 2 +- tests/ui/drain_collect.fixed | 4 ++++ tests/ui/drain_collect.rs | 4 ++++ 4 files changed, 16 insertions(+), 6 deletions(-) diff --git a/clippy_lints/src/methods/drain_collect.rs b/clippy_lints/src/methods/drain_collect.rs index 8bd8f88a214..d0c79dc11b0 100644 --- a/clippy_lints/src/methods/drain_collect.rs +++ b/clippy_lints/src/methods/drain_collect.rs @@ -66,17 +66,19 @@ pub(super) fn check(cx: &LateContext<'_>, args: &[Expr<'_>], expr: &Expr<'_>, re .or_else(|| check_collections(cx, expr_ty, recv_ty_no_refs)) { let recv = snippet(cx, recv.span, ""); + 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`", - if let ty::Ref(..) = recv_ty.kind() { - format!("std::mem::take({recv})") - } else { - format!("std::mem::take(&mut {recv})") - }, + sugg, Applicability::MachineApplicable, ); } diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index bed70207f28..99c984ba65a 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -3260,7 +3260,7 @@ declare_clippy_lint! { /// When using `mem::take`, the old collection is replaced with an empty one and ownership of /// the old collection is returned. /// - /// ### Drawback + /// ### 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`. diff --git a/tests/ui/drain_collect.fixed b/tests/ui/drain_collect.fixed index 0d40a648378..11001bd319f 100644 --- a/tests/ui/drain_collect.fixed +++ b/tests/ui/drain_collect.fixed @@ -70,4 +70,8 @@ fn string_dont_lint(b: &mut String) -> HashSet { b.drain(..).collect() } +fn not_whole_length(v: &mut Vec) -> Vec { + v.drain(1..).collect() +} + fn main() {} diff --git a/tests/ui/drain_collect.rs b/tests/ui/drain_collect.rs index 7144a1847ca..373a3ca3506 100644 --- a/tests/ui/drain_collect.rs +++ b/tests/ui/drain_collect.rs @@ -70,4 +70,8 @@ fn string_dont_lint(b: &mut String) -> HashSet { b.drain(..).collect() } +fn not_whole_length(v: &mut Vec) -> Vec { + v.drain(1..).collect() +} + fn main() {}