diff --git a/CHANGELOG.md b/CHANGELOG.md index bc60b1c57f7d..d44341fc973d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6086,6 +6086,7 @@ Released 2018-09-13 [`iter_filter_is_ok`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_filter_is_ok [`iter_filter_is_some`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_filter_is_some [`iter_kv_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_kv_map +[`iter_last_slice`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_last_slice [`iter_next_loop`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_next_loop [`iter_next_slice`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_next_slice [`iter_not_returning_iterator`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_not_returning_iterator diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index e1cb08e361ca..6280fa85f115 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -393,6 +393,7 @@ pub static LINTS: &[&::declare_clippy_lint::LintInfo] = &[ crate::methods::ITER_FILTER_IS_OK_INFO, crate::methods::ITER_FILTER_IS_SOME_INFO, crate::methods::ITER_KV_MAP_INFO, + crate::methods::ITER_LAST_SLICE_INFO, crate::methods::ITER_NEXT_SLICE_INFO, crate::methods::ITER_NTH_INFO, crate::methods::ITER_NTH_ZERO_INFO, diff --git a/clippy_lints/src/methods/iter_last_slice.rs b/clippy_lints/src/methods/iter_last_slice.rs new file mode 100644 index 000000000000..24d3c1bd0d28 --- /dev/null +++ b/clippy_lints/src/methods/iter_last_slice.rs @@ -0,0 +1,68 @@ +use super::ITER_LAST_SLICE; +use super::utils::derefs_to_slice; +use clippy_utils::diagnostics::span_lint_and_sugg; +use clippy_utils::higher; +use clippy_utils::source::snippet_with_applicability; +use rustc_ast::ast; +use rustc_errors::Applicability; +use rustc_hir as hir; +use rustc_lint::LateContext; + +pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>, caller_expr: &'tcx hir::Expr<'_>) { + if derefs_to_slice(cx, caller_expr, cx.typeck_results().expr_ty(caller_expr)).is_some() { + // caller is a Slice + if let hir::ExprKind::Index(caller_var, index_expr, _) = &caller_expr.kind + && let Some(higher::Range { + start: _, + end: Some(end_expr), + limits, + }) = higher::Range::hir(index_expr) + && let hir::ExprKind::Lit(end_lit) = &end_expr.kind + && let ast::LitKind::Int(end_idx, _) = end_lit.node + { + let mut applicability = Applicability::MachineApplicable; + let suggest = if end_idx == 0 { + // This corresponds to `$slice[..0].iter().last()`, which + // evaluates to `None`. Maybe this is worth its own lint? + format!( + "{}.last()", + snippet_with_applicability(cx, caller_var.span, "..", &mut applicability) + ) + } else { + let idx = match limits { + ast::RangeLimits::HalfOpen => end_idx.get() - 1, + ast::RangeLimits::Closed => end_idx.get(), + }; + + format!( + "{}.get({idx})", + snippet_with_applicability(cx, caller_var.span, "..", &mut applicability) + ) + }; + span_lint_and_sugg( + cx, + ITER_LAST_SLICE, + expr.span, + "using `.iter().last()` on a slice without end index", + "try calling", + suggest, + applicability, + ); + } + } else if super::iter_next_slice::is_vec_or_array(cx, caller_expr) { + // caller is a Vec or an Array + let mut applicability = Applicability::MachineApplicable; + span_lint_and_sugg( + cx, + ITER_LAST_SLICE, + expr.span, + "using `.iter().last()` on an array", + "try calling", + format!( + "{}.last()", + snippet_with_applicability(cx, caller_expr.span, "..", &mut applicability) + ), + applicability, + ); + } +} diff --git a/clippy_lints/src/methods/iter_next_slice.rs b/clippy_lints/src/methods/iter_next_slice.rs index fd4650e1e45f..b22a60f8706f 100644 --- a/clippy_lints/src/methods/iter_next_slice.rs +++ b/clippy_lints/src/methods/iter_next_slice.rs @@ -74,7 +74,7 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>, cal } } -fn is_vec_or_array<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>) -> bool { +pub(crate) fn is_vec_or_array<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>) -> bool { is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(expr), sym::Vec) || matches!(&cx.typeck_results().expr_ty(expr).peel_refs().kind(), ty::Array(_, _)) } diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 49ca81dafc28..7bea7ef6cc4d 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -44,6 +44,7 @@ mod iter_cloned_collect; mod iter_count; mod iter_filter; mod iter_kv_map; +mod iter_last_slice; mod iter_next_slice; mod iter_nth; mod iter_nth_zero; @@ -1820,6 +1821,33 @@ declare_clippy_lint! { "using `.iter().next()` on a sliced array, which can be shortened to just `.get()`" } +declare_clippy_lint! { + /// ### What it does + /// Checks for usage of `iter().last()` on a Slice or an Array. + /// + /// ### Why is this bad? + /// These can be shortened into `.last()`. + /// + /// ### Example + /// ```no_run + /// # let a = [1, 2, 3]; + /// # let b = vec![1, 2, 3]; + /// a[..2].iter().last(); + /// b.iter().last(); + /// ``` + /// should be written as: + /// ```no_run + /// # let a = [1, 2, 3]; + /// # let b = vec![1, 2, 3]; + /// a.get(1); + /// b.get(0); + /// ``` + #[clippy::version = "1.46.0"] + pub ITER_LAST_SLICE, + style, + "using `.iter().next()` on a slice, array or `Vec`, which can be shortened to just `.get()`" +} + declare_clippy_lint! { /// ### What it does /// Warns when using `push_str`/`insert_str` with a single-character string literal @@ -4644,6 +4672,7 @@ impl_lint_pass!(Methods => [ FLAT_MAP_IDENTITY, MAP_FLATTEN, ITERATOR_STEP_BY_ZERO, + ITER_LAST_SLICE, ITER_NEXT_SLICE, ITER_COUNT, ITER_NTH, @@ -5228,17 +5257,27 @@ impl Methods { } }, (sym::last, []) => { - if let Some((sym::cloned, recv2, [], _span2, _)) = method_call(recv) { - iter_overeager_cloned::check( - cx, - expr, - recv, - recv2, - iter_overeager_cloned::Op::LaterCloned, - false, - ); + let method_call = method_call(recv); + + match method_call { + Some((sym::cloned, recv2, [], _, _)) => { + iter_overeager_cloned::check( + cx, + expr, + recv, + recv2, + iter_overeager_cloned::Op::LaterCloned, + false, + ); + double_ended_iterator_last::check(cx, expr, recv, call_span); + }, + + Some((sym::iter, recv2, [], _, _)) => iter_last_slice::check(cx, expr, recv2), + + _ => { + double_ended_iterator_last::check(cx, expr, recv, call_span); + }, } - double_ended_iterator_last::check(cx, expr, recv, call_span); }, (sym::len, []) => { if let Some((prev_method @ (sym::as_bytes | sym::bytes), prev_recv, [], _, _)) = method_call(recv) { diff --git a/tests/ui/iter_last_slice.fixed b/tests/ui/iter_last_slice.fixed new file mode 100644 index 000000000000..0548aca76f10 --- /dev/null +++ b/tests/ui/iter_last_slice.fixed @@ -0,0 +1,34 @@ +#![warn(clippy::iter_last_slice)] +#![allow(clippy::useless_vec)] + +fn main() { + // test code goes here + let s = [1, 2, 3]; + let v = vec![1, 2, 3]; + + let _ = s.last(); + //~^ iter_last_slice + // Should be replaced by s.last() + + let _ = s.get(1); + //~^ iter_last_slice + // Should be replaced by s.get(1) + let _ = s.get(2); + //~^ iter_last_slice + // Should be replaced by s.get(2) + + let _ = v.get(4); + //~^ iter_last_slice + // Should be replaced by v.get(4) + let _ = v.get(5); + //~^ iter_last_slice + // Should be replaced by v.get(5) + + let _ = v.last(); + //~^ iter_last_slice + // Should be replaced by v.last() + + let o = Some(5); + o.iter().last(); + // Shouldn't be linted since this is not a Slice or an Array +} diff --git a/tests/ui/iter_last_slice.rs b/tests/ui/iter_last_slice.rs new file mode 100644 index 000000000000..bc1cebf8543e --- /dev/null +++ b/tests/ui/iter_last_slice.rs @@ -0,0 +1,34 @@ +#![warn(clippy::iter_last_slice)] +#![allow(clippy::useless_vec)] + +fn main() { + // test code goes here + let s = [1, 2, 3]; + let v = vec![1, 2, 3]; + + let _ = s.iter().last(); + //~^ iter_last_slice + // Should be replaced by s.last() + + let _ = s[..2].iter().last(); + //~^ iter_last_slice + // Should be replaced by s.get(1) + let _ = s[..=2].iter().last(); + //~^ iter_last_slice + // Should be replaced by s.get(2) + + let _ = v[..5].iter().last(); + //~^ iter_last_slice + // Should be replaced by v.get(4) + let _ = v[..=5].iter().last(); + //~^ iter_last_slice + // Should be replaced by v.get(5) + + let _ = v.iter().last(); + //~^ iter_last_slice + // Should be replaced by v.last() + + let o = Some(5); + o.iter().last(); + // Shouldn't be linted since this is not a Slice or an Array +} diff --git a/tests/ui/iter_last_slice.stderr b/tests/ui/iter_last_slice.stderr new file mode 100644 index 000000000000..4f2fbeded623 --- /dev/null +++ b/tests/ui/iter_last_slice.stderr @@ -0,0 +1,41 @@ +error: using `.iter().last()` on an array + --> tests/ui/iter_last_slice.rs:9:13 + | +LL | let _ = s.iter().last(); + | ^^^^^^^^^^^^^^^ help: try calling: `s.last()` + | + = note: `-D clippy::iter-last-slice` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::iter_last_slice)]` + +error: using `.iter().last()` on a slice without end index + --> tests/ui/iter_last_slice.rs:13:13 + | +LL | let _ = s[..2].iter().last(); + | ^^^^^^^^^^^^^^^^^^^^ help: try calling: `s.get(1)` + +error: using `.iter().last()` on a slice without end index + --> tests/ui/iter_last_slice.rs:16:13 + | +LL | let _ = s[..=2].iter().last(); + | ^^^^^^^^^^^^^^^^^^^^^ help: try calling: `s.get(2)` + +error: using `.iter().last()` on a slice without end index + --> tests/ui/iter_last_slice.rs:20:13 + | +LL | let _ = v[..5].iter().last(); + | ^^^^^^^^^^^^^^^^^^^^ help: try calling: `v.get(4)` + +error: using `.iter().last()` on a slice without end index + --> tests/ui/iter_last_slice.rs:23:13 + | +LL | let _ = v[..=5].iter().last(); + | ^^^^^^^^^^^^^^^^^^^^^ help: try calling: `v.get(5)` + +error: using `.iter().last()` on an array + --> tests/ui/iter_last_slice.rs:27:13 + | +LL | let _ = v.iter().last(); + | ^^^^^^^^^^^^^^^ help: try calling: `v.last()` + +error: aborting due to 6 previous errors + diff --git a/tests/ui/iter_overeager_cloned.fixed b/tests/ui/iter_overeager_cloned.fixed index b0e548f17909..68d0935434e5 100644 --- a/tests/ui/iter_overeager_cloned.fixed +++ b/tests/ui/iter_overeager_cloned.fixed @@ -3,7 +3,8 @@ dead_code, clippy::let_unit_value, clippy::useless_vec, - clippy::double_ended_iterator_last + clippy::double_ended_iterator_last, + clippy::iter_last_slice )] fn main() { diff --git a/tests/ui/iter_overeager_cloned.rs b/tests/ui/iter_overeager_cloned.rs index cedf62a6b473..9c70dc90e862 100644 --- a/tests/ui/iter_overeager_cloned.rs +++ b/tests/ui/iter_overeager_cloned.rs @@ -3,7 +3,8 @@ dead_code, clippy::let_unit_value, clippy::useless_vec, - clippy::double_ended_iterator_last + clippy::double_ended_iterator_last, + clippy::iter_last_slice )] fn main() { diff --git a/tests/ui/iter_overeager_cloned.stderr b/tests/ui/iter_overeager_cloned.stderr index 1616dec95b79..0f75e323a504 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:12:29 + --> tests/ui/iter_overeager_cloned.rs:13: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:15:29 + --> tests/ui/iter_overeager_cloned.rs:16: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:18:20 + --> tests/ui/iter_overeager_cloned.rs:19: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:21:21 + --> tests/ui/iter_overeager_cloned.rs:22: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:24:21 + --> tests/ui/iter_overeager_cloned.rs:25: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:27:13 + --> tests/ui/iter_overeager_cloned.rs:28: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:30:13 + --> tests/ui/iter_overeager_cloned.rs:31:13 | LL | let _ = [Some(Some("str".to_string())), Some(Some("str".to_string()))] | _____________^ @@ -70,7 +70,7 @@ LL ~ .flatten().cloned(); | error: unnecessarily eager cloning of iterator items - --> tests/ui/iter_overeager_cloned.rs:36:13 + --> tests/ui/iter_overeager_cloned.rs:37:13 | LL | let _ = vec.iter().cloned().filter(|x| x.starts_with('2')); | ^^^^^^^^^^---------------------------------------- @@ -78,7 +78,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:39:13 + --> tests/ui/iter_overeager_cloned.rs:40:13 | LL | let _ = vec.iter().cloned().find(|x| x == "2"); | ^^^^^^^^^^---------------------------- @@ -86,7 +86,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:44:17 + --> tests/ui/iter_overeager_cloned.rs:45:17 | LL | let _ = vec.iter().cloned().filter(f); | ^^^^^^^^^^------------------- @@ -94,7 +94,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:46:17 + --> tests/ui/iter_overeager_cloned.rs:47:17 | LL | let _ = vec.iter().cloned().find(f); | ^^^^^^^^^^----------------- @@ -102,7 +102,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:53:17 + --> tests/ui/iter_overeager_cloned.rs:54:17 | LL | let _ = vec.iter().cloned().filter(f); | ^^^^^^^^^^------------------- @@ -110,7 +110,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:55:17 + --> tests/ui/iter_overeager_cloned.rs:56:17 | LL | let _ = vec.iter().cloned().find(f); | ^^^^^^^^^^----------------- @@ -118,7 +118,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:63:9 + --> tests/ui/iter_overeager_cloned.rs:64:9 | LL | iter.cloned().filter(move |&(&a, ref b)| a == 1 && b == &target) | ^^^^------------------------------------------------------------ @@ -126,7 +126,7 @@ LL | iter.cloned().filter(move |&(&a, ref b)| a == 1 && b == &target) | help: try: `.filter(move |&&(&a, ref b)| a == 1 && b == &target).cloned()` error: unnecessarily eager cloning of iterator items - --> tests/ui/iter_overeager_cloned.rs:75:13 + --> tests/ui/iter_overeager_cloned.rs:76:13 | LL | iter.cloned().filter(move |S { a, b }| **a == 1 && b == &target) | ^^^^------------------------------------------------------------ @@ -134,7 +134,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:80:13 + --> tests/ui/iter_overeager_cloned.rs:81:13 | LL | let _ = vec.iter().cloned().map(|x| x.len()); | ^^^^^^^^^^-------------------------- @@ -142,7 +142,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:86:13 + --> tests/ui/iter_overeager_cloned.rs:87:13 | LL | let _ = vec.iter().cloned().for_each(|x| assert!(!x.is_empty())); | ^^^^^^^^^^---------------------------------------------- @@ -150,7 +150,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:89:13 + --> tests/ui/iter_overeager_cloned.rs:90:13 | LL | let _ = vec.iter().cloned().all(|x| x.len() == 1); | ^^^^^^^^^^------------------------------- @@ -158,7 +158,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:92:13 + --> tests/ui/iter_overeager_cloned.rs:93:13 | LL | let _ = vec.iter().cloned().any(|x| x.len() == 1); | ^^^^^^^^^^-------------------------------