Add lint for calling Iterator::last() on DoubleEndedIterator (#13922

)

I [recently realized that `.last()` might not call `next_back()` when it
is
available](https://qsantos.fr/2025/01/01/rust-gotcha-last-on-doubleendediterator/).
Although the implementor could make sure to implement `last()` to do so,
this is not what will happen by default. As a result, I think it is
useful to add a lint to promote using `.next_back()` over `.last()` on
`DoubleEndedIterator`.

If this is merged, we might want to close #1822.

changelog: [`double_ended_iterator_last`]: Add lint for calling
`Iterator::last()` on `DoubleEndedIterator`

CHANGELOG.md

@@ -5493,6 +5493,7 @@ Released 2018-09-13
 [`doc_markdown`]: https://rust-lang.github.io/rust-clippy/master/index.html#doc_markdown
 [`doc_nested_refdefs`]: https://rust-lang.github.io/rust-clippy/master/index.html#doc_nested_refdefs
 [`double_comparisons`]: https://rust-lang.github.io/rust-clippy/master/index.html#double_comparisons
+[`double_ended_iterator_last`]: https://rust-lang.github.io/rust-clippy/master/index.html#double_ended_iterator_last
 [`double_must_use`]: https://rust-lang.github.io/rust-clippy/master/index.html#double_must_use
 [`double_neg`]: https://rust-lang.github.io/rust-clippy/master/index.html#double_neg
 [`double_parens`]: https://rust-lang.github.io/rust-clippy/master/index.html#double_parens

clippy_lints/src/attrs/mixed_attributes_style.rs

@@ -66,7 +66,7 @@ pub(super) fn check(cx: &EarlyContext<'_>, item_span: Span, attrs: &[Attribute])
 
 fn lint_mixed_attrs(cx: &EarlyContext<'_>, attrs: &[Attribute]) {
     let mut attrs_iter = attrs.iter().filter(|attr| !attr.span.from_expansion());
-    let span = if let (Some(first), Some(last)) = (attrs_iter.next(), attrs_iter.last()) {
+    let span = if let (Some(first), Some(last)) = (attrs_iter.next(), attrs_iter.next_back()) {
         first.span.with_hi(last.span.hi())
     } else {
         return;

clippy_lints/src/declared_lints.rs

@@ -372,6 +372,7 @@ pub static LINTS: &[&crate::LintInfo] = &[
     crate::methods::CLONE_ON_REF_PTR_INFO,
     crate::methods::COLLAPSIBLE_STR_REPLACE_INFO,
     crate::methods::CONST_IS_EMPTY_INFO,
+    crate::methods::DOUBLE_ENDED_ITERATOR_LAST_INFO,
     crate::methods::DRAIN_COLLECT_INFO,
     crate::methods::ERR_EXPECT_INFO,
     crate::methods::EXPECT_FUN_CALL_INFO,

clippy_lints/src/methods/double_ended_iterator_last.rs

@@ -0,0 +1,41 @@
+use clippy_utils::diagnostics::span_lint_and_sugg;
+use clippy_utils::is_trait_method;
+use clippy_utils::ty::implements_trait;
+use rustc_errors::Applicability;
+use rustc_hir::Expr;
+use rustc_lint::LateContext;
+use rustc_middle::ty::Instance;
+use rustc_span::{Span, sym};
+
+use super::DOUBLE_ENDED_ITERATOR_LAST;
+
+pub(super) fn check(cx: &LateContext<'_>, expr: &'_ Expr<'_>, self_expr: &'_ Expr<'_>, call_span: Span) {
+    let typeck = cx.typeck_results();
+
+    // if the "last" method is that of Iterator
+    if is_trait_method(cx, expr, sym::Iterator)
+        // if self implements DoubleEndedIterator
+        && let Some(deiter_id) = cx.tcx.get_diagnostic_item(sym::DoubleEndedIterator)
+        && let self_type = cx.typeck_results().expr_ty(self_expr)
+        && implements_trait(cx, self_type.peel_refs(), deiter_id, &[])
+        // resolve the method definition
+        && let id = typeck.type_dependent_def_id(expr.hir_id).unwrap()
+        && let args = typeck.node_args(expr.hir_id)
+        && let Ok(Some(fn_def)) = Instance::try_resolve(cx.tcx, cx.typing_env(), id, args)
+        // find the provided definition of Iterator::last
+        && let Some(item) = cx.tcx.get_diagnostic_item(sym::Iterator)
+        && let Some(last_def) = cx.tcx.provided_trait_methods(item).find(|m| m.name.as_str() == "last")
+        // if the resolved method is the same as the provided definition
+        && fn_def.def_id() == last_def.def_id
+    {
+        span_lint_and_sugg(
+            cx,
+            DOUBLE_ENDED_ITERATOR_LAST,
+            call_span,
+            "called `Iterator::last` on a `DoubleEndedIterator`; this will needlessly iterate the entire iterator",
+            "try",
+            "next_back()".to_string(),
+            Applicability::MachineApplicable,
+        );
+    }
+}

clippy_lints/src/methods/mod.rs

@@ -14,6 +14,7 @@ mod clone_on_copy;
 mod clone_on_ref_ptr;
 mod cloned_instead_of_copied;
 mod collapsible_str_replace;
+mod double_ended_iterator_last;
 mod drain_collect;
 mod err_expect;
 mod expect_fun_call;
@@ -4284,6 +4285,32 @@ declare_clippy_lint! {
     "map of a trivial closure (not dependent on parameter) over a range"
 }
 
+declare_clippy_lint! {
+    /// ### What it does
+    ///
+    /// Checks for `Iterator::last` being called on a  `DoubleEndedIterator`, which can be replaced
+    /// with `DoubleEndedIterator::next_back`.
+    ///
+    /// ### Why is this bad?
+    ///
+    /// `Iterator::last` is implemented by consuming the iterator, which is unnecessary if
+    /// the iterator is a `DoubleEndedIterator`. Since Rust traits do not allow specialization,
+    /// `Iterator::last` cannot be optimized for `DoubleEndedIterator`.
+    ///
+    /// ### Example
+    /// ```no_run
+    /// let last_arg = "echo hello world".split(' ').last();
+    /// ```
+    /// Use instead:
+    /// ```no_run
+    /// let last_arg = "echo hello world".split(' ').next_back();
+    /// ```
+    #[clippy::version = "1.85.0"]
+    pub DOUBLE_ENDED_ITERATOR_LAST,
+    perf,
+    "using `Iterator::last` on a `DoubleEndedIterator`"
+}
+
 pub struct Methods {
     avoid_breaking_exported_api: bool,
     msrv: Msrv,
@@ -4449,6 +4476,7 @@ impl_lint_pass!(Methods => [
     MAP_ALL_ANY_IDENTITY,
     MAP_WITH_UNUSED_ARGUMENT_OVER_RANGES,
     UNNECESSARY_MAP_OR,
+    DOUBLE_ENDED_ITERATOR_LAST,
 ]);
 
 /// Extracts a method call name, args, and `Span` of the method name.
@@ -4931,6 +4959,7 @@ impl Methods {
                             false,
                         );
                     }
+                    double_ended_iterator_last::check(cx, expr, recv, call_span);
                 },
                 ("len", []) => {
                     if let Some(("as_bytes", prev_recv, [], _, _)) = method_call(recv) {

clippy_utils/src/msrvs.rs

@@ -130,7 +130,7 @@ impl Msrv {
         let mut msrv_attrs = attrs.iter().filter(|attr| attr.path_matches(&[sym::clippy, sym_msrv]));
 
         if let Some(msrv_attr) = msrv_attrs.next() {
-            if let Some(duplicate) = msrv_attrs.last() {
+            if let Some(duplicate) = msrv_attrs.next_back() {
                 sess.dcx()
                     .struct_span_err(duplicate.span(), "`clippy::msrv` is defined multiple times")
                     .with_span_note(msrv_attr.span(), "first definition found here")

tests/ui/double_ended_iterator_last.fixed

@@ -0,0 +1,53 @@
+#![warn(clippy::double_ended_iterator_last)]
+
+// Typical case
+pub fn last_arg(s: &str) -> Option<&str> {
+    s.split(' ').next_back()
+}
+
+fn main() {
+    // General case
+    struct DeIterator;
+    impl Iterator for DeIterator {
+        type Item = ();
+        fn next(&mut self) -> Option<Self::Item> {
+            Some(())
+        }
+    }
+    impl DoubleEndedIterator for DeIterator {
+        fn next_back(&mut self) -> Option<Self::Item> {
+            Some(())
+        }
+    }
+    let _ = DeIterator.next_back();
+    // Should not apply to other methods of Iterator
+    let _ = DeIterator.count();
+
+    // Should not apply to simple iterators
+    struct SimpleIterator;
+    impl Iterator for SimpleIterator {
+        type Item = ();
+        fn next(&mut self) -> Option<Self::Item> {
+            Some(())
+        }
+    }
+    let _ = SimpleIterator.last();
+
+    // Should not apply to custom implementations of last()
+    struct CustomLast;
+    impl Iterator for CustomLast {
+        type Item = ();
+        fn next(&mut self) -> Option<Self::Item> {
+            Some(())
+        }
+        fn last(self) -> Option<Self::Item> {
+            Some(())
+        }
+    }
+    impl DoubleEndedIterator for CustomLast {
+        fn next_back(&mut self) -> Option<Self::Item> {
+            Some(())
+        }
+    }
+    let _ = CustomLast.last();
+}

tests/ui/double_ended_iterator_last.rs

@@ -0,0 +1,53 @@
+#![warn(clippy::double_ended_iterator_last)]
+
+// Typical case
+pub fn last_arg(s: &str) -> Option<&str> {
+    s.split(' ').last()
+}
+
+fn main() {
+    // General case
+    struct DeIterator;
+    impl Iterator for DeIterator {
+        type Item = ();
+        fn next(&mut self) -> Option<Self::Item> {
+            Some(())
+        }
+    }
+    impl DoubleEndedIterator for DeIterator {
+        fn next_back(&mut self) -> Option<Self::Item> {
+            Some(())
+        }
+    }
+    let _ = DeIterator.last();
+    // Should not apply to other methods of Iterator
+    let _ = DeIterator.count();
+
+    // Should not apply to simple iterators
+    struct SimpleIterator;
+    impl Iterator for SimpleIterator {
+        type Item = ();
+        fn next(&mut self) -> Option<Self::Item> {
+            Some(())
+        }
+    }
+    let _ = SimpleIterator.last();
+
+    // Should not apply to custom implementations of last()
+    struct CustomLast;
+    impl Iterator for CustomLast {
+        type Item = ();
+        fn next(&mut self) -> Option<Self::Item> {
+            Some(())
+        }
+        fn last(self) -> Option<Self::Item> {
+            Some(())
+        }
+    }
+    impl DoubleEndedIterator for CustomLast {
+        fn next_back(&mut self) -> Option<Self::Item> {
+            Some(())
+        }
+    }
+    let _ = CustomLast.last();
+}

tests/ui/double_ended_iterator_last.stderr

@@ -0,0 +1,17 @@
+error: called `Iterator::last` on a `DoubleEndedIterator`; this will needlessly iterate the entire iterator
+  --> tests/ui/double_ended_iterator_last.rs:5:18
+   |
+LL |     s.split(' ').last()
+   |                  ^^^^^^ help: try: `next_back()`
+   |
+   = note: `-D clippy::double-ended-iterator-last` implied by `-D warnings`
+   = help: to override `-D warnings` add `#[allow(clippy::double_ended_iterator_last)]`
+
+error: called `Iterator::last` on a `DoubleEndedIterator`; this will needlessly iterate the entire iterator
+  --> tests/ui/double_ended_iterator_last.rs:22:24
+   |
+LL |     let _ = DeIterator.last();
+   |                        ^^^^^^ help: try: `next_back()`
+
+error: aborting due to 2 previous errors
+

tests/ui/infinite_iter.rs

@@ -1,4 +1,4 @@
-#![allow(clippy::uninlined_format_args)]
+#![allow(clippy::uninlined_format_args, clippy::double_ended_iterator_last)]
 
 use std::iter::repeat;
 fn square_is_lower_64(x: &u32) -> bool {

tests/ui/iter_overeager_cloned.fixed

@@ -1,5 +1,10 @@
 #![warn(clippy::iter_overeager_cloned, clippy::redundant_clone, clippy::filter_next)]
-#![allow(dead_code, clippy::let_unit_value, clippy::useless_vec)]
+#![allow(
+    dead_code,
+    clippy::let_unit_value,
+    clippy::useless_vec,
+    clippy::double_ended_iterator_last
+)]
 
 fn main() {
     let vec = vec!["1".to_string(), "2".to_string(), "3".to_string()];

tests/ui/iter_overeager_cloned.rs

@@ -1,5 +1,10 @@
 #![warn(clippy::iter_overeager_cloned, clippy::redundant_clone, clippy::filter_next)]
-#![allow(dead_code, clippy::let_unit_value, clippy::useless_vec)]
+#![allow(
+    dead_code,
+    clippy::let_unit_value,
+    clippy::useless_vec,
+    clippy::double_ended_iterator_last
+)]
 
 fn main() {
     let vec = vec!["1".to_string(), "2".to_string(), "3".to_string()];