diff --git a/CHANGELOG.md b/CHANGELOG.md index 1323f973ccf..8fde8c6d902 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4441,6 +4441,7 @@ Released 2018-09-13 [`chars_last_cmp`]: https://rust-lang.github.io/rust-clippy/master/index.html#chars_last_cmp [`chars_next_cmp`]: https://rust-lang.github.io/rust-clippy/master/index.html#chars_next_cmp [`checked_conversions`]: https://rust-lang.github.io/rust-clippy/master/index.html#checked_conversions +[`clear_with_drain`]: https://rust-lang.github.io/rust-clippy/master/index.html#clear_with_drain [`clone_double_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#clone_double_ref [`clone_on_copy`]: https://rust-lang.github.io/rust-clippy/master/index.html#clone_on_copy [`clone_on_ref_ptr`]: https://rust-lang.github.io/rust-clippy/master/index.html#clone_on_ref_ptr diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 8ca91301472..15b557bded2 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -307,6 +307,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[ crate::methods::CASE_SENSITIVE_FILE_EXTENSION_COMPARISONS_INFO, crate::methods::CHARS_LAST_CMP_INFO, crate::methods::CHARS_NEXT_CMP_INFO, + crate::methods::CLEAR_WITH_DRAIN_INFO, crate::methods::CLONED_INSTEAD_OF_COPIED_INFO, crate::methods::CLONE_DOUBLE_REF_INFO, crate::methods::CLONE_ON_COPY_INFO, diff --git a/clippy_lints/src/methods/clear_with_drain.rs b/clippy_lints/src/methods/clear_with_drain.rs new file mode 100644 index 00000000000..24496bd4689 --- /dev/null +++ b/clippy_lints/src/methods/clear_with_drain.rs @@ -0,0 +1,28 @@ +use clippy_utils::diagnostics::span_lint_and_sugg; +use clippy_utils::is_range_full; +use clippy_utils::ty::is_type_diagnostic_item; +use rustc_errors::Applicability; +use rustc_hir::{Expr, ExprKind, QPath}; +use rustc_lint::LateContext; +use rustc_span::symbol::sym; +use rustc_span::Span; + +use super::CLEAR_WITH_DRAIN; + +pub(super) fn check(cx: &LateContext<'_>, expr: &Expr<'_>, recv: &Expr<'_>, span: Span, arg: &Expr<'_>) { + let ty = cx.typeck_results().expr_ty(recv); + if is_type_diagnostic_item(cx, ty, sym::Vec) + && let ExprKind::Path(QPath::Resolved(None, container_path)) = recv.kind + && is_range_full(cx, arg, Some(container_path)) + { + span_lint_and_sugg( + cx, + CLEAR_WITH_DRAIN, + span.with_hi(expr.span.hi()), + "`drain` used to clear a `Vec`", + "try", + "clear()".to_string(), + Applicability::MachineApplicable, + ); + } +} diff --git a/clippy_lints/src/methods/iter_with_drain.rs b/clippy_lints/src/methods/iter_with_drain.rs index 3da230e12d7..f6772c5c6b3 100644 --- a/clippy_lints/src/methods/iter_with_drain.rs +++ b/clippy_lints/src/methods/iter_with_drain.rs @@ -1,7 +1,5 @@ use clippy_utils::diagnostics::span_lint_and_sugg; -use clippy_utils::higher::Range; -use clippy_utils::is_integer_const; -use rustc_ast::ast::RangeLimits; +use clippy_utils::is_range_full; use rustc_errors::Applicability; use rustc_hir::{Expr, ExprKind, QPath}; use rustc_lint::LateContext; @@ -15,8 +13,8 @@ pub(super) fn check(cx: &LateContext<'_>, expr: &Expr<'_>, recv: &Expr<'_>, span && let Some(adt) = cx.typeck_results().expr_ty(recv).ty_adt_def() && let Some(ty_name) = cx.tcx.get_diagnostic_name(adt.did()) && matches!(ty_name, sym::Vec | sym::VecDeque) - && let Some(range) = Range::hir(arg) - && is_full_range(cx, recv, range) + && let ExprKind::Path(QPath::Resolved(None, container_path)) = recv.kind + && is_range_full(cx, arg, Some(container_path)) { span_lint_and_sugg( cx, @@ -29,19 +27,3 @@ pub(super) fn check(cx: &LateContext<'_>, expr: &Expr<'_>, recv: &Expr<'_>, span ); }; } - -fn is_full_range(cx: &LateContext<'_>, container: &Expr<'_>, range: Range<'_>) -> bool { - range.start.map_or(true, |e| is_integer_const(cx, e, 0)) - && range.end.map_or(true, |e| { - if range.limits == RangeLimits::HalfOpen - && let ExprKind::Path(QPath::Resolved(None, container_path)) = container.kind - && let ExprKind::MethodCall(name, self_arg, [], _) = e.kind - && name.ident.name == sym::len - && let ExprKind::Path(QPath::Resolved(None, path)) = self_arg.kind - { - container_path.res == path.res - } else { - false - } - }) -} diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 56e3988bf09..257bc4eccc3 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -9,6 +9,7 @@ mod chars_last_cmp; mod chars_last_cmp_with_unwrap; mod chars_next_cmp; mod chars_next_cmp_with_unwrap; +mod clear_with_drain; mod clone_on_copy; mod clone_on_ref_ptr; mod cloned_instead_of_copied; @@ -110,7 +111,7 @@ use clippy_utils::ty::{contains_ty_adt_constructor_opaque, implements_trait, is_ use clippy_utils::{contains_return, is_bool, is_trait_method, iter_input_pats, return_ty}; use if_chain::if_chain; use rustc_hir as hir; -use rustc_hir::{Expr, ExprKind, TraitItem, TraitItemKind}; +use rustc_hir::{Expr, ExprKind, Node, Stmt, StmtKind, TraitItem, TraitItemKind}; use rustc_hir_analysis::hir_ty_to_ty; use rustc_lint::{LateContext, LateLintPass, LintContext}; use rustc_middle::lint::in_external_macro; @@ -3190,6 +3191,31 @@ declare_clippy_lint! { "single command line argument that looks like it should be multiple arguments" } +declare_clippy_lint! { + /// ### What it does + /// Checks for usage of `.drain(..)` for the sole purpose of clearing a `Vec`. + /// + /// ### Why is this bad? + /// This creates an unnecessary iterator that is dropped immediately. + /// + /// Calling `.clear()` also makes the intent clearer. + /// + /// ### Example + /// ```rust + /// let mut v = vec![1, 2, 3]; + /// v.drain(..); + /// ``` + /// Use instead: + /// ```rust + /// let mut v = vec![1, 2, 3]; + /// v.clear(); + /// ``` + #[clippy::version = "1.69.0"] + pub CLEAR_WITH_DRAIN, + nursery, + "calling `drain` in order to `clear` a `Vec`" +} + pub struct Methods { avoid_breaking_exported_api: bool, msrv: Msrv, @@ -3318,6 +3344,7 @@ impl_lint_pass!(Methods => [ SEEK_TO_START_INSTEAD_OF_REWIND, NEEDLESS_COLLECT, SUSPICIOUS_COMMAND_ARG_SPACE, + CLEAR_WITH_DRAIN, ]); /// Extracts a method call name, args, and `Span` of the method name. @@ -3563,7 +3590,13 @@ impl Methods { _ => {}, }, ("drain", [arg]) => { - iter_with_drain::check(cx, expr, recv, span, arg); + if let Node::Stmt(Stmt { hir_id: _, kind, .. }) = cx.tcx.hir().get_parent(expr.hir_id) + && matches!(kind, StmtKind::Semi(_)) + { + clear_with_drain::check(cx, expr, recv, span, arg); + } else { + iter_with_drain::check(cx, expr, recv, span, arg); + } }, ("ends_with", [arg]) => { if let ExprKind::MethodCall(.., span) = expr.kind { diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index 29830557a44..2e839fdf472 100644 --- a/clippy_utils/src/lib.rs +++ b/clippy_utils/src/lib.rs @@ -78,7 +78,7 @@ use std::sync::OnceLock; use std::sync::{Mutex, MutexGuard}; use if_chain::if_chain; -use rustc_ast::ast::{self, LitKind}; +use rustc_ast::ast::{self, LitKind, RangeLimits}; use rustc_ast::Attribute; use rustc_data_structures::fx::FxHashMap; use rustc_data_structures::unhash::UnhashMap; @@ -96,6 +96,7 @@ use rustc_hir::{ use rustc_lexer::{tokenize, TokenKind}; use rustc_lint::{LateContext, Level, Lint, LintContext}; use rustc_middle::hir::place::PlaceBase; +use rustc_middle::mir::ConstantKind; use rustc_middle::ty as rustc_ty; use rustc_middle::ty::adjustment::{Adjust, Adjustment, AutoBorrow}; use rustc_middle::ty::binding::BindingMode; @@ -114,7 +115,8 @@ use rustc_span::symbol::{kw, Ident, Symbol}; use rustc_span::Span; use rustc_target::abi::Integer; -use crate::consts::{constant, Constant}; +use crate::consts::{constant, miri_to_const, Constant}; +use crate::higher::Range; use crate::ty::{can_partially_move_ty, expr_sig, is_copy, is_recursively_primitive_type, ty_is_fn_once_param}; use crate::visitors::for_each_expr; @@ -1491,6 +1493,68 @@ pub fn is_else_clause(tcx: TyCtxt<'_>, expr: &Expr<'_>) -> bool { } } +/// Checks whether the given `Expr` is a range equivalent to a `RangeFull`. +/// For the lower bound, this means that: +/// - either there is none +/// - or it is the smallest value that can be represented by the range's integer type +/// For the upper bound, this means that: +/// - either there is none +/// - or it is the largest value that can be represented by the range's integer type and is +/// inclusive +/// - or it is a call to some container's `len` method and is exclusive, and the range is passed to +/// a method call on that same container (e.g. `v.drain(..v.len())`) +/// If the given `Expr` is not some kind of range, the function returns `false`. +pub fn is_range_full(cx: &LateContext<'_>, expr: &Expr<'_>, container_path: Option<&Path<'_>>) -> bool { + let ty = cx.typeck_results().expr_ty(expr); + if let Some(Range { start, end, limits }) = Range::hir(expr) { + let start_is_none_or_min = start.map_or(true, |start| { + if let rustc_ty::Adt(_, subst) = ty.kind() + && let bnd_ty = subst.type_at(0) + && let Some(min_val) = bnd_ty.numeric_min_val(cx.tcx) + && let const_val = cx.tcx.valtree_to_const_val((bnd_ty, min_val.to_valtree())) + && let min_const_kind = ConstantKind::from_value(const_val, bnd_ty) + && let Some(min_const) = miri_to_const(cx.tcx, min_const_kind) + && let Some((start_const, _)) = constant(cx, cx.typeck_results(), start) + { + start_const == min_const + } else { + false + } + }); + let end_is_none_or_max = end.map_or(true, |end| { + match limits { + RangeLimits::Closed => { + if let rustc_ty::Adt(_, subst) = ty.kind() + && let bnd_ty = subst.type_at(0) + && let Some(max_val) = bnd_ty.numeric_max_val(cx.tcx) + && let const_val = cx.tcx.valtree_to_const_val((bnd_ty, max_val.to_valtree())) + && let max_const_kind = ConstantKind::from_value(const_val, bnd_ty) + && let Some(max_const) = miri_to_const(cx.tcx, max_const_kind) + && let Some((end_const, _)) = constant(cx, cx.typeck_results(), end) + { + end_const == max_const + } else { + false + } + }, + RangeLimits::HalfOpen => { + if let Some(container_path) = container_path + && let ExprKind::MethodCall(name, self_arg, [], _) = end.kind + && name.ident.name == sym::len + && let ExprKind::Path(QPath::Resolved(None, path)) = self_arg.kind + { + container_path.res == path.res + } else { + false + } + }, + } + }); + return start_is_none_or_min && end_is_none_or_max; + } + false +} + /// Checks whether the given expression is a constant integer of the given value. /// unlike `is_integer_literal`, this version does const folding pub fn is_integer_const(cx: &LateContext<'_>, e: &Expr<'_>, value: u128) -> bool { diff --git a/tests/ui/clear_with_drain.fixed b/tests/ui/clear_with_drain.fixed new file mode 100644 index 00000000000..9c4dc010ca7 --- /dev/null +++ b/tests/ui/clear_with_drain.fixed @@ -0,0 +1,86 @@ +// run-rustfix +#![allow(unused)] +#![warn(clippy::clear_with_drain)] + +fn range() { + let mut v = vec![1, 2, 3]; + let iter = v.drain(0..v.len()); // Yay + + let mut v = vec![1, 2, 3]; + let n = v.drain(0..v.len()).count(); // Yay + + let mut v = vec![1, 2, 3]; + let iter = v.drain(usize::MIN..v.len()); // Yay + let n = iter.count(); + + let mut v = vec![1, 2, 3]; + v.clear(); // Nay + + let mut v = vec![1, 2, 3]; + v.clear(); // Nay +} + +fn range_from() { + let mut v = vec![1, 2, 3]; + let iter = v.drain(0..); // Yay + + let mut v = vec![1, 2, 3]; + let mut iter = v.drain(0..); // Yay + let next = iter.next(); + + let mut v = vec![1, 2, 3]; + let next = v.drain(usize::MIN..).next(); // Yay + + let mut v = vec![1, 2, 3]; + v.clear(); // Nay + + let mut v = vec![1, 2, 3]; + v.clear(); // Nay +} + +fn range_full() { + let mut v = vec![1, 2, 3]; + let iter = v.drain(..); // Yay + + let mut v = vec![1, 2, 3]; + // Yay + for x in v.drain(..) { + let y = format!("x = {x}"); + } + + let mut v = vec![1, 2, 3]; + v.clear(); // Nay +} + +fn range_to() { + let mut v = vec![1, 2, 3]; + let iter = v.drain(..v.len()); // Yay + + let mut v = vec![1, 2, 3]; + let iter = v.drain(..v.len()); // Yay + for x in iter { + let y = format!("x = {x}"); + } + + let mut v = vec![1, 2, 3]; + v.clear(); // Nay +} + +fn partial_drains() { + let mut v = vec![1, 2, 3]; + v.drain(1..); // Yay + let mut v = vec![1, 2, 3]; + v.drain(1..).max(); // Yay + + let mut v = vec![1, 2, 3]; + v.drain(..v.len() - 1); // Yay + let mut v = vec![1, 2, 3]; + v.drain(..v.len() - 1).min(); // Yay + + let mut v = vec![1, 2, 3]; + v.drain(1..v.len() - 1); // Yay + let mut v = vec![1, 2, 3]; + let w: Vec = v.drain(1..v.len() - 1).collect(); // Yay +} + +fn main() {} diff --git a/tests/ui/clear_with_drain.rs b/tests/ui/clear_with_drain.rs new file mode 100644 index 00000000000..f00dbab234c --- /dev/null +++ b/tests/ui/clear_with_drain.rs @@ -0,0 +1,86 @@ +// run-rustfix +#![allow(unused)] +#![warn(clippy::clear_with_drain)] + +fn range() { + let mut v = vec![1, 2, 3]; + let iter = v.drain(0..v.len()); // Yay + + let mut v = vec![1, 2, 3]; + let n = v.drain(0..v.len()).count(); // Yay + + let mut v = vec![1, 2, 3]; + let iter = v.drain(usize::MIN..v.len()); // Yay + let n = iter.count(); + + let mut v = vec![1, 2, 3]; + v.drain(0..v.len()); // Nay + + let mut v = vec![1, 2, 3]; + v.drain(usize::MIN..v.len()); // Nay +} + +fn range_from() { + let mut v = vec![1, 2, 3]; + let iter = v.drain(0..); // Yay + + let mut v = vec![1, 2, 3]; + let mut iter = v.drain(0..); // Yay + let next = iter.next(); + + let mut v = vec![1, 2, 3]; + let next = v.drain(usize::MIN..).next(); // Yay + + let mut v = vec![1, 2, 3]; + v.drain(0..); // Nay + + let mut v = vec![1, 2, 3]; + v.drain(usize::MIN..); // Nay +} + +fn range_full() { + let mut v = vec![1, 2, 3]; + let iter = v.drain(..); // Yay + + let mut v = vec![1, 2, 3]; + // Yay + for x in v.drain(..) { + let y = format!("x = {x}"); + } + + let mut v = vec![1, 2, 3]; + v.drain(..); // Nay +} + +fn range_to() { + let mut v = vec![1, 2, 3]; + let iter = v.drain(..v.len()); // Yay + + let mut v = vec![1, 2, 3]; + let iter = v.drain(..v.len()); // Yay + for x in iter { + let y = format!("x = {x}"); + } + + let mut v = vec![1, 2, 3]; + v.drain(..v.len()); // Nay +} + +fn partial_drains() { + let mut v = vec![1, 2, 3]; + v.drain(1..); // Yay + let mut v = vec![1, 2, 3]; + v.drain(1..).max(); // Yay + + let mut v = vec![1, 2, 3]; + v.drain(..v.len() - 1); // Yay + let mut v = vec![1, 2, 3]; + v.drain(..v.len() - 1).min(); // Yay + + let mut v = vec![1, 2, 3]; + v.drain(1..v.len() - 1); // Yay + let mut v = vec![1, 2, 3]; + let w: Vec = v.drain(1..v.len() - 1).collect(); // Yay +} + +fn main() {} diff --git a/tests/ui/clear_with_drain.stderr b/tests/ui/clear_with_drain.stderr new file mode 100644 index 00000000000..c88aa1a23cb --- /dev/null +++ b/tests/ui/clear_with_drain.stderr @@ -0,0 +1,40 @@ +error: `drain` used to clear a `Vec` + --> $DIR/clear_with_drain.rs:17:7 + | +LL | v.drain(0..v.len()); // Nay + | ^^^^^^^^^^^^^^^^^ help: try: `clear()` + | + = note: `-D clippy::clear-with-drain` implied by `-D warnings` + +error: `drain` used to clear a `Vec` + --> $DIR/clear_with_drain.rs:20:7 + | +LL | v.drain(usize::MIN..v.len()); // Nay + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `clear()` + +error: `drain` used to clear a `Vec` + --> $DIR/clear_with_drain.rs:35:7 + | +LL | v.drain(0..); // Nay + | ^^^^^^^^^^ help: try: `clear()` + +error: `drain` used to clear a `Vec` + --> $DIR/clear_with_drain.rs:38:7 + | +LL | v.drain(usize::MIN..); // Nay + | ^^^^^^^^^^^^^^^^^^^ help: try: `clear()` + +error: `drain` used to clear a `Vec` + --> $DIR/clear_with_drain.rs:52:7 + | +LL | v.drain(..); // Nay + | ^^^^^^^^^ help: try: `clear()` + +error: `drain` used to clear a `Vec` + --> $DIR/clear_with_drain.rs:66:7 + | +LL | v.drain(..v.len()); // Nay + | ^^^^^^^^^^^^^^^^ help: try: `clear()` + +error: aborting due to 6 previous errors +