From 707653f268a2e8911751b738671bae8da9eb7225 Mon Sep 17 00:00:00 2001 From: Quentin Santos Date: Wed, 1 Jan 2025 18:47:23 +0100 Subject: [PATCH 1/7] Add lint for calling last() on DoubleEndedIterator --- CHANGELOG.md | 1 + clippy_lints/src/declared_lints.rs | 1 + .../src/methods/double_ended_iterator_last.rs | 42 +++++++++++++++++++ clippy_lints/src/methods/mod.rs | 29 +++++++++++++ tests/ui/double_ended_iterator_last.fixed | 35 ++++++++++++++++ tests/ui/double_ended_iterator_last.rs | 35 ++++++++++++++++ tests/ui/double_ended_iterator_last.stderr | 17 ++++++++ 7 files changed, 160 insertions(+) create mode 100644 clippy_lints/src/methods/double_ended_iterator_last.rs create mode 100644 tests/ui/double_ended_iterator_last.fixed create mode 100644 tests/ui/double_ended_iterator_last.rs create mode 100644 tests/ui/double_ended_iterator_last.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 664a7e766304..3e68b46cb0f6 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 7451fb909ef8..3ff10d850f82 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/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, diff --git a/clippy_lints/src/methods/double_ended_iterator_last.rs b/clippy_lints/src/methods/double_ended_iterator_last.rs new file mode 100644 index 000000000000..1322108e6fef --- /dev/null +++ b/clippy_lints/src/methods/double_ended_iterator_last.rs @@ -0,0 +1,42 @@ +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_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(); + + // Check if the current "last" method is that of the Iterator trait + if !is_trait_method(cx, expr, sym::Iterator) { + return; + } + + // Find id for DoubleEndedIterator trait + let Some(deiter_id) = cx.tcx.get_diagnostic_item(sym::DoubleEndedIterator) else { + return; + }; + + // Find the type of self + let self_type = typeck.expr_ty(self_expr).peel_refs(); + + // Check that the object implements the DoubleEndedIterator trait + if !implements_trait(cx, self_type, deiter_id, &[]) { + return; + } + + // Emit lint + 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, + ); +} diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 810287fa5416..51351f6b7cd1 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/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) { diff --git a/tests/ui/double_ended_iterator_last.fixed b/tests/ui/double_ended_iterator_last.fixed new file mode 100644 index 000000000000..cfafd1045764 --- /dev/null +++ b/tests/ui/double_ended_iterator_last.fixed @@ -0,0 +1,35 @@ +#![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 { + Some(()) + } + } + impl DoubleEndedIterator for DeIterator { + fn next_back(&mut self) -> Option { + 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 { + Some(()) + } + } + let _ = SimpleIterator.last(); +} diff --git a/tests/ui/double_ended_iterator_last.rs b/tests/ui/double_ended_iterator_last.rs new file mode 100644 index 000000000000..ae80fb648389 --- /dev/null +++ b/tests/ui/double_ended_iterator_last.rs @@ -0,0 +1,35 @@ +#![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 { + Some(()) + } + } + impl DoubleEndedIterator for DeIterator { + fn next_back(&mut self) -> Option { + 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 { + Some(()) + } + } + let _ = SimpleIterator.last(); +} diff --git a/tests/ui/double_ended_iterator_last.stderr b/tests/ui/double_ended_iterator_last.stderr new file mode 100644 index 000000000000..b795c18a736e --- /dev/null +++ b/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 + From 458c955ae3a6f297510a86ab7efac4c7db5cc964 Mon Sep 17 00:00:00 2001 From: Quentin Santos Date: Wed, 1 Jan 2025 18:58:53 +0100 Subject: [PATCH 2/7] Fix conflicts with double_ended_iterator_last --- tests/ui/infinite_iter.rs | 2 +- tests/ui/iter_overeager_cloned.fixed | 7 ++++- tests/ui/iter_overeager_cloned.rs | 7 ++++- tests/ui/iter_overeager_cloned.stderr | 38 +++++++++++++-------------- tests/ui/starts_ends_with.fixed | 2 +- tests/ui/starts_ends_with.rs | 2 +- 6 files changed, 34 insertions(+), 24 deletions(-) diff --git a/tests/ui/infinite_iter.rs b/tests/ui/infinite_iter.rs index da95ba04b821..178e300ff5bf 100644 --- a/tests/ui/infinite_iter.rs +++ b/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 { diff --git a/tests/ui/iter_overeager_cloned.fixed b/tests/ui/iter_overeager_cloned.fixed index 7d8a584b0224..d7d3d299349e 100644 --- a/tests/ui/iter_overeager_cloned.fixed +++ b/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()]; diff --git a/tests/ui/iter_overeager_cloned.rs b/tests/ui/iter_overeager_cloned.rs index 58c374ab8cd1..45e1349febd0 100644 --- a/tests/ui/iter_overeager_cloned.rs +++ b/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()]; diff --git a/tests/ui/iter_overeager_cloned.stderr b/tests/ui/iter_overeager_cloned.stderr index 7a822a79494b..e6680266f107 100644 --- a/tests/ui/iter_overeager_cloned.stderr +++ b/tests/ui/iter_overeager_cloned.stderr @@ -1,5 +1,5 @@ error: unnecessarily eager cloning of iterator items - --> tests/ui/iter_overeager_cloned.rs:7:29 + --> tests/ui/iter_overeager_cloned.rs:12:29 | LL | let _: Option = vec.iter().cloned().last(); | ^^^^^^^^^^---------------- @@ -10,7 +10,7 @@ LL | let _: Option = vec.iter().cloned().last(); = help: to override `-D warnings` add `#[allow(clippy::iter_overeager_cloned)]` error: unnecessarily eager cloning of iterator items - --> tests/ui/iter_overeager_cloned.rs:9:29 + --> tests/ui/iter_overeager_cloned.rs:14:29 | LL | let _: Option = vec.iter().chain(vec.iter()).cloned().next(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^---------------- @@ -18,7 +18,7 @@ LL | let _: Option = vec.iter().chain(vec.iter()).cloned().next(); | help: try: `.next().cloned()` error: unneeded cloning of iterator items - --> tests/ui/iter_overeager_cloned.rs:11:20 + --> tests/ui/iter_overeager_cloned.rs:16:20 | LL | let _: usize = vec.iter().filter(|x| x == &"2").cloned().count(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^----------------- @@ -29,7 +29,7 @@ LL | let _: usize = vec.iter().filter(|x| x == &"2").cloned().count(); = help: to override `-D warnings` add `#[allow(clippy::redundant_clone)]` error: unnecessarily eager cloning of iterator items - --> tests/ui/iter_overeager_cloned.rs:13:21 + --> tests/ui/iter_overeager_cloned.rs:18:21 | LL | let _: Vec<_> = vec.iter().cloned().take(2).collect(); | ^^^^^^^^^^----------------- @@ -37,7 +37,7 @@ LL | let _: Vec<_> = vec.iter().cloned().take(2).collect(); | help: try: `.take(2).cloned()` error: unnecessarily eager cloning of iterator items - --> tests/ui/iter_overeager_cloned.rs:15:21 + --> tests/ui/iter_overeager_cloned.rs:20:21 | LL | let _: Vec<_> = vec.iter().cloned().skip(2).collect(); | ^^^^^^^^^^----------------- @@ -45,7 +45,7 @@ LL | let _: Vec<_> = vec.iter().cloned().skip(2).collect(); | help: try: `.skip(2).cloned()` error: unnecessarily eager cloning of iterator items - --> tests/ui/iter_overeager_cloned.rs:17:13 + --> tests/ui/iter_overeager_cloned.rs:22:13 | LL | let _ = vec.iter().filter(|x| x == &"2").cloned().nth(2); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^---------------- @@ -53,7 +53,7 @@ LL | let _ = vec.iter().filter(|x| x == &"2").cloned().nth(2); | help: try: `.nth(2).cloned()` error: unnecessarily eager cloning of iterator items - --> tests/ui/iter_overeager_cloned.rs:19:13 + --> tests/ui/iter_overeager_cloned.rs:24:13 | LL | let _ = [Some(Some("str".to_string())), Some(Some("str".to_string()))] | _____________^ @@ -69,7 +69,7 @@ LL ~ .flatten().cloned(); | error: unnecessarily eager cloning of iterator items - --> tests/ui/iter_overeager_cloned.rs:24:13 + --> tests/ui/iter_overeager_cloned.rs:29:13 | LL | let _ = vec.iter().cloned().filter(|x| x.starts_with('2')); | ^^^^^^^^^^---------------------------------------- @@ -77,7 +77,7 @@ LL | let _ = vec.iter().cloned().filter(|x| x.starts_with('2')); | help: try: `.filter(|&x| x.starts_with('2')).cloned()` error: unnecessarily eager cloning of iterator items - --> tests/ui/iter_overeager_cloned.rs:26:13 + --> tests/ui/iter_overeager_cloned.rs:31:13 | LL | let _ = vec.iter().cloned().find(|x| x == "2"); | ^^^^^^^^^^---------------------------- @@ -85,7 +85,7 @@ LL | let _ = vec.iter().cloned().find(|x| x == "2"); | help: try: `.find(|&x| x == "2").cloned()` error: unnecessarily eager cloning of iterator items - --> tests/ui/iter_overeager_cloned.rs:30:17 + --> tests/ui/iter_overeager_cloned.rs:35:17 | LL | let _ = vec.iter().cloned().filter(f); | ^^^^^^^^^^------------------- @@ -93,7 +93,7 @@ LL | let _ = vec.iter().cloned().filter(f); | help: try: `.filter(|&x| f(x)).cloned()` error: unnecessarily eager cloning of iterator items - --> tests/ui/iter_overeager_cloned.rs:31:17 + --> tests/ui/iter_overeager_cloned.rs:36:17 | LL | let _ = vec.iter().cloned().find(f); | ^^^^^^^^^^----------------- @@ -101,7 +101,7 @@ LL | let _ = vec.iter().cloned().find(f); | help: try: `.find(|&x| f(x)).cloned()` error: unnecessarily eager cloning of iterator items - --> tests/ui/iter_overeager_cloned.rs:37:17 + --> tests/ui/iter_overeager_cloned.rs:42:17 | LL | let _ = vec.iter().cloned().filter(f); | ^^^^^^^^^^------------------- @@ -109,7 +109,7 @@ LL | let _ = vec.iter().cloned().filter(f); | help: try: `.filter(|&x| f(x)).cloned()` error: unnecessarily eager cloning of iterator items - --> tests/ui/iter_overeager_cloned.rs:38:17 + --> tests/ui/iter_overeager_cloned.rs:43:17 | LL | let _ = vec.iter().cloned().find(f); | ^^^^^^^^^^----------------- @@ -117,7 +117,7 @@ LL | let _ = vec.iter().cloned().find(f); | help: try: `.find(|&x| f(x)).cloned()` error: unnecessarily eager cloning of iterator items - --> tests/ui/iter_overeager_cloned.rs:45:9 + --> tests/ui/iter_overeager_cloned.rs:50:9 | LL | iter.cloned().filter(move |(&a, b)| a == 1 && b == &target) | ^^^^------------------------------------------------------- @@ -125,7 +125,7 @@ LL | iter.cloned().filter(move |(&a, b)| a == 1 && b == &target) | help: try: `.filter(move |&(&a, b)| a == 1 && b == &target).cloned()` error: unnecessarily eager cloning of iterator items - --> tests/ui/iter_overeager_cloned.rs:56:13 + --> tests/ui/iter_overeager_cloned.rs:61:13 | LL | iter.cloned().filter(move |S { a, b }| **a == 1 && b == &target) | ^^^^------------------------------------------------------------ @@ -133,7 +133,7 @@ LL | iter.cloned().filter(move |S { a, b }| **a == 1 && b == &target | help: try: `.filter(move |&S { a, b }| **a == 1 && b == &target).cloned()` error: unneeded cloning of iterator items - --> tests/ui/iter_overeager_cloned.rs:60:13 + --> tests/ui/iter_overeager_cloned.rs:65:13 | LL | let _ = vec.iter().cloned().map(|x| x.len()); | ^^^^^^^^^^-------------------------- @@ -141,7 +141,7 @@ LL | let _ = vec.iter().cloned().map(|x| x.len()); | help: try: `.map(|x| x.len())` error: unneeded cloning of iterator items - --> tests/ui/iter_overeager_cloned.rs:65:13 + --> tests/ui/iter_overeager_cloned.rs:70:13 | LL | let _ = vec.iter().cloned().for_each(|x| assert!(!x.is_empty())); | ^^^^^^^^^^---------------------------------------------- @@ -149,7 +149,7 @@ LL | let _ = vec.iter().cloned().for_each(|x| assert!(!x.is_empty())); | help: try: `.for_each(|x| assert!(!x.is_empty()))` error: unneeded cloning of iterator items - --> tests/ui/iter_overeager_cloned.rs:67:13 + --> tests/ui/iter_overeager_cloned.rs:72:13 | LL | let _ = vec.iter().cloned().all(|x| x.len() == 1); | ^^^^^^^^^^------------------------------- @@ -157,7 +157,7 @@ LL | let _ = vec.iter().cloned().all(|x| x.len() == 1); | help: try: `.all(|x| x.len() == 1)` error: unneeded cloning of iterator items - --> tests/ui/iter_overeager_cloned.rs:69:13 + --> tests/ui/iter_overeager_cloned.rs:74:13 | LL | let _ = vec.iter().cloned().any(|x| x.len() == 1); | ^^^^^^^^^^------------------------------- diff --git a/tests/ui/starts_ends_with.fixed b/tests/ui/starts_ends_with.fixed index 4a66ca7ec91a..252b6e5a98c0 100644 --- a/tests/ui/starts_ends_with.fixed +++ b/tests/ui/starts_ends_with.fixed @@ -1,4 +1,4 @@ -#![allow(clippy::needless_if, dead_code, unused_must_use)] +#![allow(clippy::needless_if, dead_code, unused_must_use, clippy::double_ended_iterator_last)] fn main() {} diff --git a/tests/ui/starts_ends_with.rs b/tests/ui/starts_ends_with.rs index 16a68e02d66d..6c5655f31782 100644 --- a/tests/ui/starts_ends_with.rs +++ b/tests/ui/starts_ends_with.rs @@ -1,4 +1,4 @@ -#![allow(clippy::needless_if, dead_code, unused_must_use)] +#![allow(clippy::needless_if, dead_code, unused_must_use, clippy::double_ended_iterator_last)] fn main() {} From 09c5d34f98f53d4f0b4181404608d4c1bcede4d5 Mon Sep 17 00:00:00 2001 From: Quentin Santos Date: Wed, 1 Jan 2025 19:43:27 +0100 Subject: [PATCH 3/7] Dogfood double_ended_iterator_last --- clippy_lints/src/attrs/mixed_attributes_style.rs | 2 +- clippy_lints/src/collapsible_if.rs | 2 +- clippy_lints/src/doc/empty_line_after.rs | 2 +- clippy_lints/src/doc/markdown.rs | 2 +- clippy_lints/src/let_if_seq.rs | 2 +- clippy_lints/src/returns.rs | 4 ++-- clippy_lints/src/unit_types/unit_arg.rs | 2 +- clippy_lints/src/vec.rs | 2 +- clippy_utils/src/msrvs.rs | 2 +- 9 files changed, 10 insertions(+), 10 deletions(-) diff --git a/clippy_lints/src/attrs/mixed_attributes_style.rs b/clippy_lints/src/attrs/mixed_attributes_style.rs index 32c28c09c360..8c91c65eaf76 100644 --- a/clippy_lints/src/attrs/mixed_attributes_style.rs +++ b/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; diff --git a/clippy_lints/src/collapsible_if.rs b/clippy_lints/src/collapsible_if.rs index e73bfc6ebf7a..6122fcb28da5 100644 --- a/clippy_lints/src/collapsible_if.rs +++ b/clippy_lints/src/collapsible_if.rs @@ -110,7 +110,7 @@ fn check_collapsible_maybe_if_let(cx: &EarlyContext<'_>, then_span: Span, else_: // Prevent "elseif" // Check that the "else" is followed by whitespace let up_to_else = then_span.between(block.span); - let requires_space = if let Some(c) = snippet(cx, up_to_else, "..").chars().last() { + let requires_space = if let Some(c) = snippet(cx, up_to_else, "..").chars().next_back() { !c.is_whitespace() } else { false diff --git a/clippy_lints/src/doc/empty_line_after.rs b/clippy_lints/src/doc/empty_line_after.rs index 099194d4e746..74058c2fbf58 100644 --- a/clippy_lints/src/doc/empty_line_after.rs +++ b/clippy_lints/src/doc/empty_line_after.rs @@ -227,7 +227,7 @@ fn check_gaps(cx: &LateContext<'_>, gaps: &[Gap<'_>]) -> bool { span_lint_and_then( cx, lint, - first_gap.prev_stop.span.to(empty_lines().last().unwrap()), + first_gap.prev_stop.span.to(empty_lines().next_back().unwrap()), format!("empty {lines} after {kind_desc}"), |diag| { if let Some(owner) = cx.last_node_with_lint_attrs.as_owner() { diff --git a/clippy_lints/src/doc/markdown.rs b/clippy_lints/src/doc/markdown.rs index 8cdaba88e509..0c8e50445d65 100644 --- a/clippy_lints/src/doc/markdown.rs +++ b/clippy_lints/src/doc/markdown.rs @@ -89,7 +89,7 @@ fn check_word(cx: &LateContext<'_>, word: &str, span: Span, code_level: isize, b let s = if let Some(prefix) = s.strip_suffix("es") && prefix.chars().all(|c| c.is_ascii_uppercase()) - && matches!(prefix.chars().last(), Some('S' | 'X')) + && matches!(prefix.chars().next_back(), Some('S' | 'X')) { prefix } else if let Some(prefix) = s.strip_suffix("ified") diff --git a/clippy_lints/src/let_if_seq.rs b/clippy_lints/src/let_if_seq.rs index 5db28e9ae9b8..502e6f3a08d0 100644 --- a/clippy_lints/src/let_if_seq.rs +++ b/clippy_lints/src/let_if_seq.rs @@ -149,7 +149,7 @@ fn check_assign<'tcx>( block: &'tcx hir::Block<'_>, ) -> Option<&'tcx hir::Expr<'tcx>> { if block.expr.is_none() - && let Some(expr) = block.stmts.iter().last() + && let Some(expr) = block.stmts.iter().next_back() && let hir::StmtKind::Semi(expr) = expr.kind && let hir::ExprKind::Assign(var, value, _) = expr.kind && path_to_local_id(var, decl) diff --git a/clippy_lints/src/returns.rs b/clippy_lints/src/returns.rs index aeff31d02d26..5ed6e3fead5f 100644 --- a/clippy_lints/src/returns.rs +++ b/clippy_lints/src/returns.rs @@ -228,7 +228,7 @@ impl<'tcx> LateLintPass<'tcx> for Return { fn check_block(&mut self, cx: &LateContext<'tcx>, block: &'tcx Block<'_>) { // we need both a let-binding stmt and an expr if let Some(retexpr) = block.expr - && let Some(stmt) = block.stmts.iter().last() + && let Some(stmt) = block.stmts.iter().next_back() && let StmtKind::Let(local) = &stmt.kind && local.ty.is_none() && cx.tcx.hir().attrs(local.hir_id).is_empty() @@ -315,7 +315,7 @@ fn check_block_return<'tcx>(cx: &LateContext<'tcx>, expr_kind: &ExprKind<'tcx>, if let ExprKind::Block(block, _) = expr_kind { if let Some(block_expr) = block.expr { check_final_expr(cx, block_expr, semi_spans, RetReplacement::Empty, None); - } else if let Some(stmt) = block.stmts.iter().last() { + } else if let Some(stmt) = block.stmts.iter().next_back() { match stmt.kind { StmtKind::Expr(expr) => { check_final_expr(cx, expr, semi_spans, RetReplacement::Empty, None); diff --git a/clippy_lints/src/unit_types/unit_arg.rs b/clippy_lints/src/unit_types/unit_arg.rs index 47d6fe7db766..5e49eae14046 100644 --- a/clippy_lints/src/unit_types/unit_arg.rs +++ b/clippy_lints/src/unit_types/unit_arg.rs @@ -78,7 +78,7 @@ fn lint_unit_args(cx: &LateContext<'_>, expr: &Expr<'_>, args_to_recover: &[&Exp .filter_map(|arg| { if let ExprKind::Block(block, _) = arg.kind && block.expr.is_none() - && let Some(last_stmt) = block.stmts.iter().last() + && let Some(last_stmt) = block.stmts.iter().next_back() && let StmtKind::Semi(last_expr) = last_stmt.kind && let Some(snip) = last_expr.span.get_source_text(cx) { diff --git a/clippy_lints/src/vec.rs b/clippy_lints/src/vec.rs index ef1c46154d29..2c7e0fea17d7 100644 --- a/clippy_lints/src/vec.rs +++ b/clippy_lints/src/vec.rs @@ -175,7 +175,7 @@ impl UselessVec { } }, higher::VecArgs::Vec(args) => { - let args_span = if let Some(last) = args.iter().last() { + let args_span = if let Some(last) = args.iter().next_back() { if args.len() as u64 * size_of(cx, last) > self.too_large_for_stack { return; } diff --git a/clippy_utils/src/msrvs.rs b/clippy_utils/src/msrvs.rs index 98bcedecccc6..2169a5fdd63b 100644 --- a/clippy_utils/src/msrvs.rs +++ b/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") From 27acfd8a5b9dd1eb91e044e07201b4a9044a2bbd Mon Sep 17 00:00:00 2001 From: Quentin Santos Date: Wed, 1 Jan 2025 20:33:58 +0100 Subject: [PATCH 4/7] Prefer if chain to let-else --- .../src/methods/double_ended_iterator_last.rs | 42 ++++++------------- 1 file changed, 13 insertions(+), 29 deletions(-) diff --git a/clippy_lints/src/methods/double_ended_iterator_last.rs b/clippy_lints/src/methods/double_ended_iterator_last.rs index 1322108e6fef..dec59b1c34ef 100644 --- a/clippy_lints/src/methods/double_ended_iterator_last.rs +++ b/clippy_lints/src/methods/double_ended_iterator_last.rs @@ -9,34 +9,18 @@ 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(); - - // Check if the current "last" method is that of the Iterator trait - if !is_trait_method(cx, expr, sym::Iterator) { - return; - } - - // Find id for DoubleEndedIterator trait - let Some(deiter_id) = cx.tcx.get_diagnostic_item(sym::DoubleEndedIterator) else { - return; - }; - - // Find the type of self - let self_type = typeck.expr_ty(self_expr).peel_refs(); - - // Check that the object implements the DoubleEndedIterator trait - if !implements_trait(cx, self_type, deiter_id, &[]) { - return; + if is_trait_method(cx, expr, sym::Iterator) + && let Some(deiter_id) = cx.tcx.get_diagnostic_item(sym::DoubleEndedIterator) + && implements_trait(cx, cx.typeck_results().expr_ty(self_expr).peel_refs(), deiter_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, + ); } - - // Emit lint - 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, - ); } From 7331cc0f81d97fbc4f4eb47890cb10fe7c275696 Mon Sep 17 00:00:00 2001 From: Quentin Santos Date: Wed, 1 Jan 2025 22:14:17 +0100 Subject: [PATCH 5/7] Only complain about default Iterator::last() --- .../src/methods/double_ended_iterator_last.rs | 17 ++++++++++++++++- tests/ui/double_ended_iterator_last.fixed | 18 ++++++++++++++++++ tests/ui/double_ended_iterator_last.rs | 18 ++++++++++++++++++ 3 files changed, 52 insertions(+), 1 deletion(-) diff --git a/clippy_lints/src/methods/double_ended_iterator_last.rs b/clippy_lints/src/methods/double_ended_iterator_last.rs index dec59b1c34ef..118ab7cd93b0 100644 --- a/clippy_lints/src/methods/double_ended_iterator_last.rs +++ b/clippy_lints/src/methods/double_ended_iterator_last.rs @@ -4,14 +4,29 @@ 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) - && implements_trait(cx, cx.typeck_results().expr_ty(self_expr).peel_refs(), deiter_id, &[]) + && 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 == sym!(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, diff --git a/tests/ui/double_ended_iterator_last.fixed b/tests/ui/double_ended_iterator_last.fixed index cfafd1045764..06c48e337537 100644 --- a/tests/ui/double_ended_iterator_last.fixed +++ b/tests/ui/double_ended_iterator_last.fixed @@ -32,4 +32,22 @@ fn main() { } } let _ = SimpleIterator.last(); + + // Should not apply to custom implementations of last() + struct CustomLast; + impl Iterator for CustomLast { + type Item = (); + fn next(&mut self) -> Option { + Some(()) + } + fn last(self) -> Option { + Some(()) + } + } + impl DoubleEndedIterator for CustomLast { + fn next_back(&mut self) -> Option { + Some(()) + } + } + let _ = CustomLast.last(); } diff --git a/tests/ui/double_ended_iterator_last.rs b/tests/ui/double_ended_iterator_last.rs index ae80fb648389..9c13b496d117 100644 --- a/tests/ui/double_ended_iterator_last.rs +++ b/tests/ui/double_ended_iterator_last.rs @@ -32,4 +32,22 @@ fn main() { } } let _ = SimpleIterator.last(); + + // Should not apply to custom implementations of last() + struct CustomLast; + impl Iterator for CustomLast { + type Item = (); + fn next(&mut self) -> Option { + Some(()) + } + fn last(self) -> Option { + Some(()) + } + } + impl DoubleEndedIterator for CustomLast { + fn next_back(&mut self) -> Option { + Some(()) + } + } + let _ = CustomLast.last(); } From 0d213aa09aa263474d52fce5bcab9d3f662f7d72 Mon Sep 17 00:00:00 2001 From: Quentin Santos Date: Wed, 1 Jan 2025 22:16:33 +0100 Subject: [PATCH 6/7] Revert "Dogfood double_ended_iterator_last" This reverts commit 09c5d34f98f53d4f0b4181404608d4c1bcede4d5. --- clippy_lints/src/attrs/mixed_attributes_style.rs | 2 +- clippy_lints/src/collapsible_if.rs | 2 +- clippy_lints/src/doc/empty_line_after.rs | 2 +- clippy_lints/src/doc/markdown.rs | 2 +- clippy_lints/src/let_if_seq.rs | 2 +- clippy_lints/src/returns.rs | 4 ++-- clippy_lints/src/unit_types/unit_arg.rs | 2 +- clippy_lints/src/vec.rs | 2 +- clippy_utils/src/msrvs.rs | 2 +- 9 files changed, 10 insertions(+), 10 deletions(-) diff --git a/clippy_lints/src/attrs/mixed_attributes_style.rs b/clippy_lints/src/attrs/mixed_attributes_style.rs index 8c91c65eaf76..32c28c09c360 100644 --- a/clippy_lints/src/attrs/mixed_attributes_style.rs +++ b/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.next_back()) { + let span = if let (Some(first), Some(last)) = (attrs_iter.next(), attrs_iter.last()) { first.span.with_hi(last.span.hi()) } else { return; diff --git a/clippy_lints/src/collapsible_if.rs b/clippy_lints/src/collapsible_if.rs index 6122fcb28da5..e73bfc6ebf7a 100644 --- a/clippy_lints/src/collapsible_if.rs +++ b/clippy_lints/src/collapsible_if.rs @@ -110,7 +110,7 @@ fn check_collapsible_maybe_if_let(cx: &EarlyContext<'_>, then_span: Span, else_: // Prevent "elseif" // Check that the "else" is followed by whitespace let up_to_else = then_span.between(block.span); - let requires_space = if let Some(c) = snippet(cx, up_to_else, "..").chars().next_back() { + let requires_space = if let Some(c) = snippet(cx, up_to_else, "..").chars().last() { !c.is_whitespace() } else { false diff --git a/clippy_lints/src/doc/empty_line_after.rs b/clippy_lints/src/doc/empty_line_after.rs index 74058c2fbf58..099194d4e746 100644 --- a/clippy_lints/src/doc/empty_line_after.rs +++ b/clippy_lints/src/doc/empty_line_after.rs @@ -227,7 +227,7 @@ fn check_gaps(cx: &LateContext<'_>, gaps: &[Gap<'_>]) -> bool { span_lint_and_then( cx, lint, - first_gap.prev_stop.span.to(empty_lines().next_back().unwrap()), + first_gap.prev_stop.span.to(empty_lines().last().unwrap()), format!("empty {lines} after {kind_desc}"), |diag| { if let Some(owner) = cx.last_node_with_lint_attrs.as_owner() { diff --git a/clippy_lints/src/doc/markdown.rs b/clippy_lints/src/doc/markdown.rs index 0c8e50445d65..8cdaba88e509 100644 --- a/clippy_lints/src/doc/markdown.rs +++ b/clippy_lints/src/doc/markdown.rs @@ -89,7 +89,7 @@ fn check_word(cx: &LateContext<'_>, word: &str, span: Span, code_level: isize, b let s = if let Some(prefix) = s.strip_suffix("es") && prefix.chars().all(|c| c.is_ascii_uppercase()) - && matches!(prefix.chars().next_back(), Some('S' | 'X')) + && matches!(prefix.chars().last(), Some('S' | 'X')) { prefix } else if let Some(prefix) = s.strip_suffix("ified") diff --git a/clippy_lints/src/let_if_seq.rs b/clippy_lints/src/let_if_seq.rs index 502e6f3a08d0..5db28e9ae9b8 100644 --- a/clippy_lints/src/let_if_seq.rs +++ b/clippy_lints/src/let_if_seq.rs @@ -149,7 +149,7 @@ fn check_assign<'tcx>( block: &'tcx hir::Block<'_>, ) -> Option<&'tcx hir::Expr<'tcx>> { if block.expr.is_none() - && let Some(expr) = block.stmts.iter().next_back() + && let Some(expr) = block.stmts.iter().last() && let hir::StmtKind::Semi(expr) = expr.kind && let hir::ExprKind::Assign(var, value, _) = expr.kind && path_to_local_id(var, decl) diff --git a/clippy_lints/src/returns.rs b/clippy_lints/src/returns.rs index 5ed6e3fead5f..aeff31d02d26 100644 --- a/clippy_lints/src/returns.rs +++ b/clippy_lints/src/returns.rs @@ -228,7 +228,7 @@ impl<'tcx> LateLintPass<'tcx> for Return { fn check_block(&mut self, cx: &LateContext<'tcx>, block: &'tcx Block<'_>) { // we need both a let-binding stmt and an expr if let Some(retexpr) = block.expr - && let Some(stmt) = block.stmts.iter().next_back() + && let Some(stmt) = block.stmts.iter().last() && let StmtKind::Let(local) = &stmt.kind && local.ty.is_none() && cx.tcx.hir().attrs(local.hir_id).is_empty() @@ -315,7 +315,7 @@ fn check_block_return<'tcx>(cx: &LateContext<'tcx>, expr_kind: &ExprKind<'tcx>, if let ExprKind::Block(block, _) = expr_kind { if let Some(block_expr) = block.expr { check_final_expr(cx, block_expr, semi_spans, RetReplacement::Empty, None); - } else if let Some(stmt) = block.stmts.iter().next_back() { + } else if let Some(stmt) = block.stmts.iter().last() { match stmt.kind { StmtKind::Expr(expr) => { check_final_expr(cx, expr, semi_spans, RetReplacement::Empty, None); diff --git a/clippy_lints/src/unit_types/unit_arg.rs b/clippy_lints/src/unit_types/unit_arg.rs index 5e49eae14046..47d6fe7db766 100644 --- a/clippy_lints/src/unit_types/unit_arg.rs +++ b/clippy_lints/src/unit_types/unit_arg.rs @@ -78,7 +78,7 @@ fn lint_unit_args(cx: &LateContext<'_>, expr: &Expr<'_>, args_to_recover: &[&Exp .filter_map(|arg| { if let ExprKind::Block(block, _) = arg.kind && block.expr.is_none() - && let Some(last_stmt) = block.stmts.iter().next_back() + && let Some(last_stmt) = block.stmts.iter().last() && let StmtKind::Semi(last_expr) = last_stmt.kind && let Some(snip) = last_expr.span.get_source_text(cx) { diff --git a/clippy_lints/src/vec.rs b/clippy_lints/src/vec.rs index 2c7e0fea17d7..ef1c46154d29 100644 --- a/clippy_lints/src/vec.rs +++ b/clippy_lints/src/vec.rs @@ -175,7 +175,7 @@ impl UselessVec { } }, higher::VecArgs::Vec(args) => { - let args_span = if let Some(last) = args.iter().next_back() { + let args_span = if let Some(last) = args.iter().last() { if args.len() as u64 * size_of(cx, last) > self.too_large_for_stack { return; } diff --git a/clippy_utils/src/msrvs.rs b/clippy_utils/src/msrvs.rs index 2169a5fdd63b..98bcedecccc6 100644 --- a/clippy_utils/src/msrvs.rs +++ b/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.next_back() { + if let Some(duplicate) = msrv_attrs.last() { sess.dcx() .struct_span_err(duplicate.span(), "`clippy::msrv` is defined multiple times") .with_span_note(msrv_attr.span(), "first definition found here") From d67c00f7232b104817cd30f76ad7089a3be5a389 Mon Sep 17 00:00:00 2001 From: Quentin Santos Date: Wed, 1 Jan 2025 22:18:08 +0100 Subject: [PATCH 7/7] Dogfood double_ended_iterator_last --- clippy_lints/src/attrs/mixed_attributes_style.rs | 2 +- clippy_lints/src/methods/double_ended_iterator_last.rs | 2 +- clippy_utils/src/msrvs.rs | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/clippy_lints/src/attrs/mixed_attributes_style.rs b/clippy_lints/src/attrs/mixed_attributes_style.rs index 32c28c09c360..8c91c65eaf76 100644 --- a/clippy_lints/src/attrs/mixed_attributes_style.rs +++ b/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; diff --git a/clippy_lints/src/methods/double_ended_iterator_last.rs b/clippy_lints/src/methods/double_ended_iterator_last.rs index 118ab7cd93b0..208172980c9f 100644 --- a/clippy_lints/src/methods/double_ended_iterator_last.rs +++ b/clippy_lints/src/methods/double_ended_iterator_last.rs @@ -24,7 +24,7 @@ pub(super) fn check(cx: &LateContext<'_>, expr: &'_ Expr<'_>, self_expr: &'_ Exp && 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 == sym!(last)) + && 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 { diff --git a/clippy_utils/src/msrvs.rs b/clippy_utils/src/msrvs.rs index 98bcedecccc6..2169a5fdd63b 100644 --- a/clippy_utils/src/msrvs.rs +++ b/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")