Auto merge of #11396 - y21:issue11345, r=Jarcho

new lint: `iter_out_of_bounds`

Closes #11345

The original idea in the linked issue seemed to be just about arrays afaict, but I extended this to catch some other iterator sources such as `iter::once` or `iter::empty`.

I'm not entirely sure if this name makes a lot of sense now that it's not just about arrays anymore (specifically, not sure if you can call `.take(1)` on an `iter::Empty` to be "out of bounds"?).

changelog: [`iter_out_of_bounds`]: new lint

clippy_lints/src/methods/iter_out_of_bounds.rs

@@ -0,0 +1,106 @@
+use clippy_utils::diagnostics::span_lint_and_note;
+use clippy_utils::higher::VecArgs;
+use clippy_utils::{expr_or_init, is_trait_method, match_def_path, paths};
+use rustc_ast::LitKind;
+use rustc_hir::{Expr, ExprKind};
+use rustc_lint::LateContext;
+use rustc_middle::ty::{self};
+use rustc_span::sym;
+
+use super::ITER_OUT_OF_BOUNDS;
+
+fn expr_as_u128(cx: &LateContext<'_>, e: &Expr<'_>) -> Option<u128> {
+    if let ExprKind::Lit(lit) = expr_or_init(cx, e).kind
+        && let LitKind::Int(n, _) = lit.node
+    {
+        Some(n)
+    } else {
+        None
+    }
+}
+
+/// Attempts to extract the length out of an iterator expression.
+fn get_iterator_length<'tcx>(cx: &LateContext<'tcx>, iter: &'tcx Expr<'tcx>) -> Option<u128> {
+    let ty::Adt(adt, substs) = cx.typeck_results().expr_ty(iter).kind() else {
+        return None;
+    };
+    let did = adt.did();
+
+    if match_def_path(cx, did, &paths::ARRAY_INTO_ITER) {
+        // For array::IntoIter<T, const N: usize>, the length is the second generic
+        // parameter.
+        substs
+            .const_at(1)
+            .try_eval_target_usize(cx.tcx, cx.param_env)
+            .map(u128::from)
+    } else if match_def_path(cx, did, &paths::SLICE_ITER)
+        && let ExprKind::MethodCall(_, recv, ..) = iter.kind
+    {
+        if let ty::Array(_, len) = cx.typeck_results().expr_ty(recv).peel_refs().kind() {
+            // For slice::Iter<'_, T>, the receiver might be an array literal: [1,2,3].iter().skip(..)
+            len.try_eval_target_usize(cx.tcx, cx.param_env).map(u128::from)
+        } else if let Some(args) = VecArgs::hir(cx, expr_or_init(cx, recv)) {
+            match args {
+                VecArgs::Vec(vec) => vec.len().try_into().ok(),
+                VecArgs::Repeat(_, len) => expr_as_u128(cx, len),
+            }
+        } else {
+            None
+        }
+    } else if match_def_path(cx, did, &paths::ITER_EMPTY) {
+        Some(0)
+    } else if match_def_path(cx, did, &paths::ITER_ONCE) {
+        Some(1)
+    }  else {
+        None
+    }
+}
+
+fn check<'tcx>(
+    cx: &LateContext<'tcx>,
+    expr: &'tcx Expr<'tcx>,
+    recv: &'tcx Expr<'tcx>,
+    arg: &'tcx Expr<'tcx>,
+    message: &'static str,
+    note: &'static str,
+) {
+    if is_trait_method(cx, expr, sym::Iterator)
+        && let Some(len) = get_iterator_length(cx, recv)
+        && let Some(skipped) = expr_as_u128(cx, arg)
+        && skipped > len
+    {
+        span_lint_and_note(cx, ITER_OUT_OF_BOUNDS, expr.span, message, None, note);
+    }
+}
+
+pub(super) fn check_skip<'tcx>(
+    cx: &LateContext<'tcx>,
+    expr: &'tcx Expr<'tcx>,
+    recv: &'tcx Expr<'tcx>,
+    arg: &'tcx Expr<'tcx>,
+) {
+    check(
+        cx,
+        expr,
+        recv,
+        arg,
+        "this `.skip()` call skips more items than the iterator will produce",
+        "this operation is useless and will create an empty iterator",
+    );
+}
+
+pub(super) fn check_take<'tcx>(
+    cx: &LateContext<'tcx>,
+    expr: &'tcx Expr<'tcx>,
+    recv: &'tcx Expr<'tcx>,
+    arg: &'tcx Expr<'tcx>,
+) {
+    check(
+        cx,
+        expr,
+        recv,
+        arg,
+        "this `.take()` call takes more items than the iterator will produce",
+        "this operation is useless and the returned iterator will simply yield the same items",
+    );
+}

clippy_lints/src/methods/mod.rs

@@ -43,6 +43,7 @@ mod iter_next_slice;
 mod iter_nth;
 mod iter_nth_zero;
 mod iter_on_single_or_empty_collections;
+mod iter_out_of_bounds;
 mod iter_overeager_cloned;
 mod iter_skip_next;
 mod iter_skip_zero;
@@ -3538,6 +3539,30 @@ declare_clippy_lint! {
     "acquiring a write lock when a read lock would work"
 }
 
+declare_clippy_lint! {
+    /// ### What it does
+    /// Looks for iterator combinator calls such as `.take(x)` or `.skip(x)`
+    /// where `x` is greater than the amount of items that an iterator will produce.
+    ///
+    /// ### Why is this bad?
+    /// Taking or skipping more items than there are in an iterator either creates an iterator
+    /// with all items from the original iterator or an iterator with no items at all.
+    /// This is most likely not what the user intended to do.
+    ///
+    /// ### Example
+    /// ```rust
+    /// for _ in [1, 2, 3].iter().take(4) {}
+    /// ```
+    /// Use instead:
+    /// ```rust
+    /// for _ in [1, 2, 3].iter() {}
+    /// ```
+    #[clippy::version = "1.74.0"]
+    pub ITER_OUT_OF_BOUNDS,
+    suspicious,
+    "calls to `.take()` or `.skip()` that are out of bounds"
+}
+
 pub struct Methods {
     avoid_breaking_exported_api: bool,
     msrv: Msrv,
@@ -3676,7 +3701,8 @@ impl_lint_pass!(Methods => [
     STRING_LIT_CHARS_ANY,
     ITER_SKIP_ZERO,
     FILTER_MAP_BOOL_THEN,
-    READONLY_WRITE_LOCK
+    READONLY_WRITE_LOCK,
+    ITER_OUT_OF_BOUNDS,
 ]);
 
 /// Extracts a method call name, args, and `Span` of the method name.
@@ -4146,6 +4172,7 @@ impl Methods {
                 },
                 ("skip", [arg]) => {
                     iter_skip_zero::check(cx, expr, arg);
+                    iter_out_of_bounds::check_skip(cx, expr, recv, arg);
 
                     if let Some(("cloned", recv2, [], _span2, _)) = method_call(recv) {
                         iter_overeager_cloned::check(cx, expr, recv, recv2,
@@ -4173,7 +4200,8 @@ impl Methods {
                     }
                 },
                 ("step_by", [arg]) => iterator_step_by_zero::check(cx, expr, arg),
-                ("take", [_arg]) => {
+                ("take", [arg]) => {
+                    iter_out_of_bounds::check_take(cx, expr, recv, arg);
                     if let Some(("cloned", recv2, [], _span2, _)) = method_call(recv) {
                         iter_overeager_cloned::check(cx, expr, recv, recv2,
                                 iter_overeager_cloned::Op::LaterCloned, false);