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

CHANGELOG.md

@@ -5031,6 +5031,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

@@ -365,6 +365,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
     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,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);

clippy_utils/src/paths.rs

@@ -49,6 +49,7 @@ pub const IDENT: [&str; 3] = ["rustc_span", "symbol", "Ident"];
 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"];
@@ -163,3 +164,4 @@ pub const DEBUG_STRUCT: [&str; 4] = ["core", "fmt", "builders", "DebugStruct"];
 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,70 @@
+//@no-rustfix
+
+#![deny(clippy::iter_out_of_bounds)]
+#![allow(clippy::useless_vec)]
+
+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
+
+    for _ in [1; 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) {}
+
+    for _ in vec![1, 2, 3].iter().skip(4) {}
+    //~^ ERROR: this `.skip()` call skips more items than the iterator will produce
+
+    for _ in vec![1; 3].iter().skip(4) {}
+    //~^ ERROR: this `.skip()` call skips more items than the iterator will produce
+
+    let x = [1, 2, 3];
+    for _ in x.iter().skip(4) {}
+    //~^ ERROR: this `.skip()` call skips more items than the iterator will produce
+
+    let n = 4;
+    for _ in x.iter().skip(n) {}
+    //~^ ERROR: this `.skip()` call skips more items than the iterator will produce
+
+    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
+
+    for x in [].iter().take(1) {
+        //~^ ERROR: this `.take()` call takes more items than the iterator will produce
+        let _: &i32 = x;
+    }
+
+    // ok, not out of bounds
+    for _ in [1].iter().take(1) {}
+    for _ in [1, 2, 3].iter().take(2) {}
+    for _ in [1, 2, 3].iter().skip(2) {}
+}

tests/ui/iter_out_of_bounds.stderr

@@ -0,0 +1,119 @@
+error: this `.skip()` call skips more items than the iterator will produce
+  --> $DIR/iter_out_of_bounds.rs:11: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:3: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:15: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:21: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:24: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:27:14
+   |
+LL |     for _ in [1; 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 vec![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:36:14
+   |
+LL |     for _ in vec![1; 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:40:14
+   |
+LL |     for _ in x.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:44:14
+   |
+LL |     for _ in x.iter().skip(n) {}
+   |              ^^^^^^^^^^^^^^^^
+   |
+   = 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:49: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:52: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:55: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:58: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: this `.take()` call takes more items than the iterator will produce
+  --> $DIR/iter_out_of_bounds.rs:61:14
+   |
+LL |     for x in [].iter().take(1) {
+   |              ^^^^^^^^^^^^^^^^^
+   |
+   = note: this operation is useless and the returned iterator will simply yield the same items
+
+error: aborting due to 14 previous errors
+