diff options
| -rw-r--r-- | CHANGELOG.md | 1 | ||||
| -rw-r--r-- | clippy_lints/src/declared_lints.rs | 1 | ||||
| -rw-r--r-- | clippy_lints/src/methods/drain_collect.rs | 77 | ||||
| -rw-r--r-- | clippy_lints/src/methods/mod.rs | 41 | ||||
| -rw-r--r-- | tests/ui/drain_collect.rs | 65 | ||||
| -rw-r--r-- | tests/ui/drain_collect.stderr | 62 | ||||
| -rw-r--r-- | tests/ui/iter_with_drain.fixed | 2 | ||||
| -rw-r--r-- | tests/ui/iter_with_drain.rs | 2 |
8 files changed, 249 insertions, 2 deletions
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::<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).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, "<expr>"); + 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<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, + "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<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_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 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}; |