new lint: iter_out_of_bounds

CHANGELOG.md

@@ -5030,6 +5030,7 @@ Released 2018-09-13
 [`iter_nth_zero`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_nth_zero
 [`iter_on_empty_collections`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_on_empty_collections
 [`iter_on_single_items`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_on_single_items
+[`iter_out_of_bounds`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_out_of_bounds
 [`iter_overeager_cloned`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_overeager_cloned
 [`iter_skip_next`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_skip_next
 [`iter_skip_zero`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_skip_zero

clippy_lints/src/declared_lints.rs

@@ -364,6 +364,7 @@
     crate::methods::ITER_NTH_ZERO_INFO,
     crate::methods::ITER_ON_EMPTY_COLLECTIONS_INFO,
     crate::methods::ITER_ON_SINGLE_ITEMS_INFO,
+    crate::methods::ITER_OUT_OF_BOUNDS_INFO,
     crate::methods::ITER_OVEREAGER_CLONED_INFO,
     crate::methods::ITER_SKIP_NEXT_INFO,
     crate::methods::ITER_SKIP_ZERO_INFO,

clippy_lints/src/methods/iter_out_of_bounds.rs

@@ -0,0 +1,91 @@
+use clippy_utils::diagnostics::span_lint_and_note;
+use clippy_utils::{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;
+
+/// 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 iter_ty = cx.typeck_results().expr_ty(iter);
+
+    if let ty::Adt(adt, substs) = iter_ty.kind() {
+        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
+            && let ExprKind::Array(array) = recv.peel_borrows().kind
+        {
+            // For slice::Iter<'_, T>, the receiver might be an array literal: [1,2,3].iter().skip(..)
+            array.len().try_into().ok()
+        } 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
+        }
+    } 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 ExprKind::Lit(lit) = arg.kind
+        && let LitKind::Int(skip, _) = lit.node
+        && skip > 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_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 @@
     "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.73.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 @@ pub fn new(
     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.
@@ -4136,6 +4162,7 @@ fn check_methods<'tcx>(&self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
                 },
                 ("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,
@@ -4163,7 +4190,8 @@ fn check_methods<'tcx>(&self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
                     }
                 },
                 ("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);

clippy_utils/src/paths.rs

@@ -49,6 +49,7 @@
 pub const IDENT_AS_STR: [&str; 4] = ["rustc_span", "symbol", "Ident", "as_str"];
 pub const INSERT_STR: [&str; 4] = ["alloc", "string", "String", "insert_str"];
 pub const ITER_EMPTY: [&str; 5] = ["core", "iter", "sources", "empty", "Empty"];
+pub const ITER_ONCE: [&str; 5] = ["core", "iter", "sources", "once", "Once"];
 pub const ITERTOOLS_NEXT_TUPLE: [&str; 3] = ["itertools", "Itertools", "next_tuple"];
 #[cfg(feature = "internal")]
 pub const KW_MODULE: [&str; 3] = ["rustc_span", "symbol", "kw"];
@@ -166,3 +167,4 @@
 pub const ORD_CMP: [&str; 4] = ["core", "cmp", "Ord", "cmp"];
 #[expect(clippy::invalid_paths)] // not sure why it thinks this, it works so
 pub const BOOL_THEN: [&str; 4] = ["core", "bool", "<impl bool>", "then"];
+pub const ARRAY_INTO_ITER: [&str; 4] = ["core", "array", "iter", "IntoIter"];

tests/ui/iter_out_of_bounds.rs

@@ -0,0 +1,44 @@
+#![deny(clippy::iter_out_of_bounds)]
+
+fn opaque_empty_iter() -> impl Iterator<Item = ()> {
+    std::iter::empty()
+}
+
+fn main() {
+    for _ in [1, 2, 3].iter().skip(4) {
+        //~^ ERROR: this `.skip()` call skips more items than the iterator will produce
+        unreachable!();
+    }
+    for (i, _) in [1, 2, 3].iter().take(4).enumerate() {
+        //~^ ERROR: this `.take()` call takes more items than the iterator will produce
+        assert!(i <= 2);
+    }
+
+    #[allow(clippy::needless_borrow)]
+    for _ in (&&&&&&[1, 2, 3]).iter().take(4) {}
+    //~^ ERROR: this `.take()` call takes more items than the iterator will produce
+
+    for _ in [1, 2, 3].iter().skip(4) {}
+    //~^ ERROR: this `.skip()` call skips more items than the iterator will produce
+
+    // Should not lint
+    for _ in opaque_empty_iter().skip(1) {}
+
+    // Should not lint
+    let empty: [i8; 0] = [];
+    for _ in empty.iter().skip(1) {}
+
+    let empty = std::iter::empty::<i8>;
+
+    for _ in empty().skip(1) {}
+    //~^ ERROR: this `.skip()` call skips more items than the iterator will produce
+
+    for _ in empty().take(1) {}
+    //~^ ERROR: this `.take()` call takes more items than the iterator will produce
+
+    for _ in std::iter::once(1).skip(2) {}
+    //~^ ERROR: this `.skip()` call skips more items than the iterator will produce
+
+    for _ in std::iter::once(1).take(2) {}
+    //~^ ERROR: this `.take()` call takes more items than the iterator will produce
+}

tests/ui/iter_out_of_bounds.stderr

@@ -0,0 +1,71 @@
+error: this `.skip()` call skips more items than the iterator will produce
+  --> $DIR/iter_out_of_bounds.rs:8:14
+   |
+LL |     for _ in [1, 2, 3].iter().skip(4) {
+   |              ^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = note: this operation is useless and will create an empty iterator
+note: the lint level is defined here
+  --> $DIR/iter_out_of_bounds.rs:1:9
+   |
+LL | #![deny(clippy::iter_out_of_bounds)]
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: this `.take()` call takes more items than the iterator will produce
+  --> $DIR/iter_out_of_bounds.rs:12:19
+   |
+LL |     for (i, _) in [1, 2, 3].iter().take(4).enumerate() {
+   |                   ^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = note: this operation is useless and the returned iterator will simply yield the same items
+
+error: this `.take()` call takes more items than the iterator will produce
+  --> $DIR/iter_out_of_bounds.rs:18:14
+   |
+LL |     for _ in (&&&&&&[1, 2, 3]).iter().take(4) {}
+   |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = note: this operation is useless and the returned iterator will simply yield the same items
+
+error: this `.skip()` call skips more items than the iterator will produce
+  --> $DIR/iter_out_of_bounds.rs:21:14
+   |
+LL |     for _ in [1, 2, 3].iter().skip(4) {}
+   |              ^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = note: this operation is useless and will create an empty iterator
+
+error: this `.skip()` call skips more items than the iterator will produce
+  --> $DIR/iter_out_of_bounds.rs:33:14
+   |
+LL |     for _ in empty().skip(1) {}
+   |              ^^^^^^^^^^^^^^^
+   |
+   = note: this operation is useless and will create an empty iterator
+
+error: this `.take()` call takes more items than the iterator will produce
+  --> $DIR/iter_out_of_bounds.rs:36:14
+   |
+LL |     for _ in empty().take(1) {}
+   |              ^^^^^^^^^^^^^^^
+   |
+   = note: this operation is useless and the returned iterator will simply yield the same items
+
+error: this `.skip()` call skips more items than the iterator will produce
+  --> $DIR/iter_out_of_bounds.rs:39:14
+   |
+LL |     for _ in std::iter::once(1).skip(2) {}
+   |              ^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = note: this operation is useless and will create an empty iterator
+
+error: this `.take()` call takes more items than the iterator will produce
+  --> $DIR/iter_out_of_bounds.rs:42:14
+   |
+LL |     for _ in std::iter::once(1).take(2) {}
+   |              ^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = note: this operation is useless and the returned iterator will simply yield the same items
+
+error: aborting due to 8 previous errors
+

tests/ui/iter_skip_zero.fixed

@@ -1,5 +1,5 @@
 //@aux-build:proc_macros.rs
-#![allow(clippy::useless_vec, unused)]
+#![allow(clippy::useless_vec, clippy::iter_out_of_bounds, unused)]
 #![warn(clippy::iter_skip_zero)]
 
 #[macro_use]

tests/ui/iter_skip_zero.rs

@@ -1,5 +1,5 @@
 //@aux-build:proc_macros.rs
-#![allow(clippy::useless_vec, unused)]
+#![allow(clippy::useless_vec, clippy::iter_out_of_bounds, unused)]
 #![warn(clippy::iter_skip_zero)]
 
 #[macro_use]