New by_ref_peekable_peek lint (#17042)

*[View all
comments](https://triagebot.infra.rust-lang.org/gh-comments/rust-lang/rust-clippy/pull/17042)*

changelog: [`by_ref_peekable_peek`]: new lint

Closes #17020

Author: Jarcho

CHANGELOG.md

@@ -6670,6 +6670,7 @@ Released 2018-09-13
 [`boxed_local`]: https://rust-lang.github.io/rust-clippy/master/index.html#boxed_local
 [`branches_sharing_code`]: https://rust-lang.github.io/rust-clippy/master/index.html#branches_sharing_code
 [`builtin_type_shadow`]: https://rust-lang.github.io/rust-clippy/master/index.html#builtin_type_shadow
+[`by_ref_peekable_peek`]: https://rust-lang.github.io/rust-clippy/master/index.html#by_ref_peekable_peek
 [`byte_char_slices`]: https://rust-lang.github.io/rust-clippy/master/index.html#byte_char_slices
 [`bytes_count_to_len`]: https://rust-lang.github.io/rust-clippy/master/index.html#bytes_count_to_len
 [`bytes_nth`]: https://rust-lang.github.io/rust-clippy/master/index.html#bytes_nth

clippy_lints/src/declared_lints.rs

@@ -361,6 +361,7 @@ pub static LINTS: &[&::declare_clippy_lint::LintInfo] = &[
     crate::mem_replace::MEM_REPLACE_WITH_DEFAULT_INFO,
     crate::mem_replace::MEM_REPLACE_WITH_UNINIT_INFO,
     crate::methods::BIND_INSTEAD_OF_MAP_INFO,
+    crate::methods::BY_REF_PEEKABLE_PEEK_INFO,
     crate::methods::BYTES_COUNT_TO_LEN_INFO,
     crate::methods::BYTES_NTH_INFO,
     crate::methods::CASE_SENSITIVE_FILE_EXTENSION_COMPARISONS_INFO,

clippy_lints/src/methods/by_ref_peekable_peek.rs

@@ -0,0 +1,84 @@
+use crate::clippy_utils::res::MaybeTypeckRes;
+use clippy_utils::diagnostics::span_lint_and_then;
+use clippy_utils::res::{MaybeDef, MaybeResPath as _};
+use clippy_utils::sugg::Sugg;
+use clippy_utils::sym;
+use clippy_utils::ty::implements_trait;
+use rustc_ast::BindingMode;
+use rustc_errors::Applicability;
+use rustc_hir::{Expr, ExprKind, LetStmt, Node, PatKind};
+use rustc_lint::LateContext;
+use rustc_middle::ty;
+
+use super::BY_REF_PEEKABLE_PEEK;
+
+pub(super) fn check(cx: &LateContext<'_>, expr: &Expr<'_>, recv: &Expr<'_>) {
+    if let ExprKind::MethodCall(maybe_peekable, peekable_recv, [], _) = recv.kind
+        && maybe_peekable.ident.name == sym::peekable
+        && !peekable_recv.span.from_expansion()
+        && let ExprKind::MethodCall(maybe_by_ref, by_ref_recv, [], _) = peekable_recv.kind
+        && maybe_by_ref.ident.name == sym::by_ref
+        && !by_ref_recv.span.from_expansion()
+        && [peekable_recv, recv]
+            .into_iter()
+            .all(|e| cx.ty_based_def(e).opt_parent(cx).is_diag_item(cx, sym::Iterator))
+    {
+        span_lint_and_then(
+            cx,
+            BY_REF_PEEKABLE_PEEK,
+            expr.span,
+            "calling `.by_ref().peekable().peek()` will advance the underlying iterator and consume its first output",
+            |diag| {
+                let span = by_ref_recv.span.shrink_to_hi().with_hi(expr.span.hi());
+                if let ty::Ref(_, iter_ty, _) = cx.typeck_results().expr_ty_adjusted(by_ref_recv).kind()
+                    && let Some(clone_trait) = cx.tcx.lang_items().clone_trait()
+                    && implements_trait(cx, *iter_ty, clone_trait, &[])
+                {
+                    diag.span_suggestion_verbose(
+                        span,
+                        "to peek the first item without advancing the underlying iterator, use",
+                        ".clone().next().as_ref()",
+                        Applicability::MaybeIncorrect,
+                    );
+                }
+                diag.span_suggestion_verbose(
+                    span,
+                    "to advance the underlying iterator, use",
+                    ".next().as_ref()",
+                    Applicability::MaybeIncorrect,
+                );
+                // If the iterator is a local variable, initialized through a simple binding with an inferred
+                // initialization expression, suggest making the initialization expression peekable.
+                if let Some(iter_local_id) = by_ref_recv.res_local_id()
+                    && let Node::LetStmt(LetStmt {
+                        pat: let_pat,
+                        ty: None,
+                        init: Some(init_expr),
+                        els: None,
+                        span: let_stmt_span,
+                        ..
+                    }) = cx.tcx.parent_hir_node(iter_local_id)
+                    && let PatKind::Binding(BindingMode::MUT, _, _, None) = let_pat.kind
+                    && !let_stmt_span.from_expansion()
+                    // Changing the type of the iterator may prevent the code from compiling
+                    && let mut app = Applicability::MaybeIncorrect
+                    && let sugg =
+                        Sugg::hir_with_context(cx, init_expr, let_stmt_span.ctxt(), "_", &mut app).maybe_paren()
+                {
+                    diag.multipart_suggestion(
+                        "to make the iterator peekable, use",
+                        vec![
+                            (init_expr.span.source_callsite(), format!("{sugg}.peekable()")),
+                            (recv.span.with_lo(by_ref_recv.span.hi()), String::new()),
+                        ],
+                        app,
+                    );
+                } else {
+                    diag.help(
+                        "you might want to transform the iterator itself using `.peekable()` without using `.by_ref()`",
+                    );
+                }
+            },
+        );
+    }
+}

clippy_lints/src/methods/mod.rs

@@ -1,4 +1,5 @@
 mod bind_instead_of_map;
+mod by_ref_peekable_peek;
 mod bytecount;
 mod bytes_count_to_len;
 mod bytes_nth;
@@ -201,6 +202,39 @@ declare_clippy_lint! {
     "using `Option.and_then(|x| Some(y))`, which is more succinctly expressed as `map(|x| y)`"
 }
 
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for usages of `Iterator::by_ref().peekable().peek()`.
+    ///
+    /// ### Why is this bad?
+    /// While it might look like this will allow peeking on the first
+    /// element of an iterator without consuming it and without consuming
+    /// the iterator itself, it will in practice consume the first element.
+    ///
+    /// The implementation of `Peekable::peek()` produces the first element
+    /// of the underlying iterator, and stores it internally so that it can
+    /// be later produced. As a consequence, it advances the underlying
+    /// iterator, whose `.next()` method will now produce its second element.
+    ///
+    /// ### Example
+    /// ```no_run
+    /// let mut iter = [1, 2, 3].into_iter();
+    /// let x = iter.by_ref().peekable().peek();  // 1
+    /// let y = iter.by_ref().peekable().peek();  // 2
+    /// ```
+    /// If this does what you intended, use the following instead, which is
+    /// shorter and clearer:
+    /// ```no_run
+    /// let mut iter = [1, 2, 3].into_iter();
+    /// let x = iter.next().as_ref();  // 1
+    /// let y = iter.next().as_ref();  // 2
+    /// ```
+    #[clippy::version = "1.98.0"]
+    pub BY_REF_PEEKABLE_PEEK,
+    suspicious,
+    "Using `.by_ref().peekable().peek()` on an iterator"
+}
+
 declare_clippy_lint! {
     /// ### What it does
     /// It checks for `str::bytes().count()` and suggests replacing it with
@@ -4835,6 +4869,7 @@ impl_lint_pass!(Methods => [
     BIND_INSTEAD_OF_MAP,
     BYTES_COUNT_TO_LEN,
     BYTES_NTH,
+    BY_REF_PEEKABLE_PEEK,
     CASE_SENSITIVE_FILE_EXTENSION_COMPARISONS,
     CHARS_LAST_CMP,
     CHARS_NEXT_CMP,
@@ -5604,6 +5639,9 @@ impl Methods {
                         unnecessary_lazy_eval::check(cx, expr, recv, arg, "or");
                     }
                 },
+                (sym::peek, []) => {
+                    by_ref_peekable_peek::check(cx, expr, recv);
+                },
                 (sym::push, [arg]) => {
                     path_buf_push_overwrite::check(cx, expr, arg);
                 },

tests/ui/by_ref_peekable_peek.1.fixed

@@ -0,0 +1,55 @@
+#![warn(clippy::by_ref_peekable_peek)]
+
+struct S;
+
+impl S {
+    fn by_ref(&mut self) -> impl Iterator<Item = i32> {
+        std::iter::empty()
+    }
+}
+
+macro_rules! mac {
+    ($x:expr) => {
+        $x.by_ref().peekable().peek()
+    };
+}
+
+fn with_non_clone_parameter(i: &mut impl Iterator<Item = i32>) {
+    // This won't suggest `.clone().next().as_ref()` as `i` is not `Clone`
+    let _: Option<&i32> = i.next().as_ref();
+    //~^ by_ref_peekable_peek
+}
+
+fn with_cloneable_local_iterator(a: Vec<i32>) {
+    let mut i = a.into_iter();
+    let _: Option<&i32> = i.clone().next().as_ref();
+    //~^ by_ref_peekable_peek
+}
+
+fn with_cloneable_local_iterator_from_macro() {
+    macro_rules! mac {
+        () => {
+            [1, 2, 3].into_iter()
+        };
+    }
+    let mut i = mac!();
+    let _: Option<&i32> = i.clone().next().as_ref();
+    //~^ by_ref_peekable_peek
+}
+
+fn main() {
+    let mut iter = [1, 2, 3].into_iter();
+    let _: Option<&i32> = iter.clone().next().as_ref();
+    //~^ by_ref_peekable_peek
+    #[expect(clippy::needless_borrow)]
+    #[allow(clippy::unnecessary_mut_passed)] // For the `.clone().next().as_ref()` suggestion
+    let _: Option<&i32> = (&mut iter).clone().next().as_ref();
+    //~^ by_ref_peekable_peek
+
+    // Do not lint if `by_ref()` is not the one on `Iterator`
+    let _: Option<&i32> = S.by_ref().peekable().peek();
+
+    // Do not lint if coming from a macro, as we cannot ensure
+    // that all uses of `.by_ref()` will be `Iterator::by_ref()`.
+    let _: Option<&i32> = mac!(iter);
+}

tests/ui/by_ref_peekable_peek.2.fixed

@@ -0,0 +1,55 @@
+#![warn(clippy::by_ref_peekable_peek)]
+
+struct S;
+
+impl S {
+    fn by_ref(&mut self) -> impl Iterator<Item = i32> {
+        std::iter::empty()
+    }
+}
+
+macro_rules! mac {
+    ($x:expr) => {
+        $x.by_ref().peekable().peek()
+    };
+}
+
+fn with_non_clone_parameter(i: &mut impl Iterator<Item = i32>) {
+    // This won't suggest `.clone().next().as_ref()` as `i` is not `Clone`
+    let _: Option<&i32> = i.next().as_ref();
+    //~^ by_ref_peekable_peek
+}
+
+fn with_cloneable_local_iterator(a: Vec<i32>) {
+    let mut i = a.into_iter();
+    let _: Option<&i32> = i.next().as_ref();
+    //~^ by_ref_peekable_peek
+}
+
+fn with_cloneable_local_iterator_from_macro() {
+    macro_rules! mac {
+        () => {
+            [1, 2, 3].into_iter()
+        };
+    }
+    let mut i = mac!();
+    let _: Option<&i32> = i.next().as_ref();
+    //~^ by_ref_peekable_peek
+}
+
+fn main() {
+    let mut iter = [1, 2, 3].into_iter();
+    let _: Option<&i32> = iter.next().as_ref();
+    //~^ by_ref_peekable_peek
+    #[expect(clippy::needless_borrow)]
+    #[allow(clippy::unnecessary_mut_passed)] // For the `.clone().next().as_ref()` suggestion
+    let _: Option<&i32> = (&mut iter).next().as_ref();
+    //~^ by_ref_peekable_peek
+
+    // Do not lint if `by_ref()` is not the one on `Iterator`
+    let _: Option<&i32> = S.by_ref().peekable().peek();
+
+    // Do not lint if coming from a macro, as we cannot ensure
+    // that all uses of `.by_ref()` will be `Iterator::by_ref()`.
+    let _: Option<&i32> = mac!(iter);
+}

tests/ui/by_ref_peekable_peek.3.fixed

@@ -0,0 +1,55 @@
+#![warn(clippy::by_ref_peekable_peek)]
+
+struct S;
+
+impl S {
+    fn by_ref(&mut self) -> impl Iterator<Item = i32> {
+        std::iter::empty()
+    }
+}
+
+macro_rules! mac {
+    ($x:expr) => {
+        $x.by_ref().peekable().peek()
+    };
+}
+
+fn with_non_clone_parameter(i: &mut impl Iterator<Item = i32>) {
+    // This won't suggest `.clone().next().as_ref()` as `i` is not `Clone`
+    let _: Option<&i32> = i.next().as_ref();
+    //~^ by_ref_peekable_peek
+}
+
+fn with_cloneable_local_iterator(a: Vec<i32>) {
+    let mut i = a.into_iter().peekable();
+    let _: Option<&i32> = i.peek();
+    //~^ by_ref_peekable_peek
+}
+
+fn with_cloneable_local_iterator_from_macro() {
+    macro_rules! mac {
+        () => {
+            [1, 2, 3].into_iter()
+        };
+    }
+    let mut i = mac!().peekable();
+    let _: Option<&i32> = i.peek();
+    //~^ by_ref_peekable_peek
+}
+
+fn main() {
+    let mut iter = [1, 2, 3].into_iter().peekable();
+    let _: Option<&i32> = iter.peek();
+    //~^ by_ref_peekable_peek
+    #[expect(clippy::needless_borrow)]
+    #[allow(clippy::unnecessary_mut_passed)] // For the `.clone().next().as_ref()` suggestion
+    let _: Option<&i32> = (&mut iter).clone().next().as_ref();
+    //~^ by_ref_peekable_peek
+
+    // Do not lint if `by_ref()` is not the one on `Iterator`
+    let _: Option<&i32> = S.by_ref().peekable().peek();
+
+    // Do not lint if coming from a macro, as we cannot ensure
+    // that all uses of `.by_ref()` will be `Iterator::by_ref()`.
+    let _: Option<&i32> = mac!(iter);
+}

tests/ui/by_ref_peekable_peek.rs

@@ -0,0 +1,55 @@
+#![warn(clippy::by_ref_peekable_peek)]
+
+struct S;
+
+impl S {
+    fn by_ref(&mut self) -> impl Iterator<Item = i32> {
+        std::iter::empty()
+    }
+}
+
+macro_rules! mac {
+    ($x:expr) => {
+        $x.by_ref().peekable().peek()
+    };
+}
+
+fn with_non_clone_parameter(i: &mut impl Iterator<Item = i32>) {
+    // This won't suggest `.clone().next().as_ref()` as `i` is not `Clone`
+    let _: Option<&i32> = i.by_ref().peekable().peek();
+    //~^ by_ref_peekable_peek
+}
+
+fn with_cloneable_local_iterator(a: Vec<i32>) {
+    let mut i = a.into_iter();
+    let _: Option<&i32> = i.by_ref().peekable().peek();
+    //~^ by_ref_peekable_peek
+}
+
+fn with_cloneable_local_iterator_from_macro() {
+    macro_rules! mac {
+        () => {
+            [1, 2, 3].into_iter()
+        };
+    }
+    let mut i = mac!();
+    let _: Option<&i32> = i.by_ref().peekable().peek();
+    //~^ by_ref_peekable_peek
+}
+
+fn main() {
+    let mut iter = [1, 2, 3].into_iter();
+    let _: Option<&i32> = iter.by_ref().peekable().peek();
+    //~^ by_ref_peekable_peek
+    #[expect(clippy::needless_borrow)]
+    #[allow(clippy::unnecessary_mut_passed)] // For the `.clone().next().as_ref()` suggestion
+    let _: Option<&i32> = (&mut iter).by_ref().peekable().peek();
+    //~^ by_ref_peekable_peek
+
+    // Do not lint if `by_ref()` is not the one on `Iterator`
+    let _: Option<&i32> = S.by_ref().peekable().peek();
+
+    // Do not lint if coming from a macro, as we cannot ensure
+    // that all uses of `.by_ref()` will be `Iterator::by_ref()`.
+    let _: Option<&i32> = mac!(iter);
+}