Auto merge of #10528 - bluthej:clear-with-drain, r=llogiq

Clear with drain

changelog: [`clear_with_drain`]: Add new lint

Fixes #9339

Author: bors

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

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,

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,
+        );
+    }
+}

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
-            }
-        })
-}

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 {

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 {

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<i8> = v.drain(1..v.len() - 1).collect(); // Yay
+}
+
+fn main() {}