diff --git a/CHANGELOG.md b/CHANGELOG.md index 9209d11feb34..4054a7a95d5e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6259,6 +6259,7 @@ Released 2018-09-13 [`unused_async`]: https://rust-lang.github.io/rust-clippy/master/index.html#unused_async [`unused_collect`]: https://rust-lang.github.io/rust-clippy/master/index.html#unused_collect [`unused_enumerate_index`]: https://rust-lang.github.io/rust-clippy/master/index.html#unused_enumerate_index +[`unused_enumerate_value`]: https://rust-lang.github.io/rust-clippy/master/index.html#unused_enumerate_value [`unused_format_specs`]: https://rust-lang.github.io/rust-clippy/master/index.html#unused_format_specs [`unused_io_amount`]: https://rust-lang.github.io/rust-clippy/master/index.html#unused_io_amount [`unused_label`]: https://rust-lang.github.io/rust-clippy/master/index.html#unused_label diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index b9f9f4e4fe0b..e80a05883eb3 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -308,6 +308,7 @@ pub static LINTS: &[&crate::LintInfo] = &[ crate::loops::SAME_ITEM_PUSH_INFO, crate::loops::SINGLE_ELEMENT_LOOP_INFO, crate::loops::UNUSED_ENUMERATE_INDEX_INFO, + crate::loops::UNUSED_ENUMERATE_VALUE_INFO, crate::loops::WHILE_FLOAT_INFO, crate::loops::WHILE_IMMUTABLE_CONDITION_INFO, crate::loops::WHILE_LET_LOOP_INFO, diff --git a/clippy_lints/src/loops/mod.rs b/clippy_lints/src/loops/mod.rs index 2b66827e82ee..ac7b03ce642e 100644 --- a/clippy_lints/src/loops/mod.rs +++ b/clippy_lints/src/loops/mod.rs @@ -18,6 +18,7 @@ mod never_loop; mod same_item_push; mod single_element_loop; mod unused_enumerate_index; +mod unused_enumerate_value; mod utils; mod while_float; mod while_immutable_condition; @@ -784,6 +785,37 @@ declare_clippy_lint! { "using the character position yielded by `.chars().enumerate()` in a context where a byte index is expected" } +declare_clippy_lint! { + /// ### What it does + /// Checks for uses of the `enumerate` method where the value is unused (`_`) on iterators + /// implementing `ExactSizeIterator`. + /// + /// ### Why is this bad? + /// Just iterating a range of indices is more idiomatic and is probably faster because it + /// avoids consuming the iterator. + /// + /// ### Example + /// ```no_run + /// fn example(iter: impl ExactSizeIterator) { + /// for (i, _) in iter.enumerate() { + /// ..; + /// } + /// } + /// ``` + /// Use instead: + /// ```no_run + /// fn example(iter: impl ExactSizeIterator) { + /// for i in 0..iter.len() { + /// ..; + /// } + /// } + /// ``` + #[clippy::version = "1.87.0"] + pub UNUSED_ENUMERATE_VALUE, + nursery, + "using `.enumerate()` and immediately dropping the value" +} + pub struct Loops { msrv: Msrv, enforce_iter_loop_reborrow: bool, @@ -822,6 +854,7 @@ impl_lint_pass!(Loops => [ INFINITE_LOOP, MANUAL_SLICE_FILL, CHAR_INDICES_AS_BYTE_INDICES, + UNUSED_ENUMERATE_VALUE, ]); impl<'tcx> LateLintPass<'tcx> for Loops { @@ -906,6 +939,7 @@ impl Loops { manual_find::check(cx, pat, arg, body, span, expr); unused_enumerate_index::check(cx, pat, arg, body); char_indices_as_byte_indices::check(cx, pat, arg, body); + unused_enumerate_value::check(cx, pat, arg, body); } fn check_for_loop_arg(&self, cx: &LateContext<'_>, _: &Pat<'_>, arg: &Expr<'_>) { diff --git a/clippy_lints/src/loops/unused_enumerate_value.rs b/clippy_lints/src/loops/unused_enumerate_value.rs new file mode 100644 index 000000000000..bc26f3ea0deb --- /dev/null +++ b/clippy_lints/src/loops/unused_enumerate_value.rs @@ -0,0 +1,87 @@ +use super::UNUSED_ENUMERATE_VALUE; +use clippy_utils::diagnostics::span_lint_and_then; +use clippy_utils::source::snippet; +use clippy_utils::sugg::Sugg; +use clippy_utils::ty::{get_adt_inherent_method, implements_trait}; +use clippy_utils::{get_trait_def_id, pat_is_wild, paths}; +use rustc_errors::Applicability; +use rustc_hir::def::DefKind; +use rustc_hir::{Expr, ExprKind, Pat, PatKind}; +use rustc_lint::LateContext; +use rustc_middle::ty::{self, Ty}; +use rustc_span::sym; + +/// Checks for the `UNUSED_ENUMERATE_VALUE` lint. +/// +/// TODO: Extend this lint to cover iterator chains. +pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, pat: &Pat<'tcx>, arg: &'tcx Expr<'_>, body: &'tcx Expr<'tcx>) { + if let PatKind::Tuple([index, elem], _) = pat.kind + && let ExprKind::MethodCall(_method, recv, [], _) = arg.kind + && pat_is_wild(cx, &elem.kind, body) + && let arg_ty = cx.typeck_results().expr_ty(arg) + && let ty::Adt(base, _) = *arg_ty.kind() + && cx.tcx.is_diagnostic_item(sym::Enumerate, base.did()) + && let Some((DefKind::AssocFn, call_id)) = cx.typeck_results().type_dependent_def(arg.hir_id) + && cx.tcx.is_diagnostic_item(sym::enumerate_method, call_id) + && let receiver_ty = cx.typeck_results().expr_ty(recv) + // TODO: Replace with `sym` when it's available + && let Some(exact_size_iter) = get_trait_def_id(cx.tcx, &paths::ITER_EXACT_SIZE_ITERATOR) + && implements_trait(cx, receiver_ty, exact_size_iter, &[]) + { + let (recv, applicability) = remove_trailing_iter(cx, recv); + span_lint_and_then( + cx, + UNUSED_ENUMERATE_VALUE, + arg.span, + "you seem to use `.enumerate()` and immediately discard the value", + |diag| { + let range_end = Sugg::hir(cx, recv, ".."); + if applicability != Applicability::MachineApplicable { + diag.help(format!("consider using `0..{range_end}.len()` instead")); + return; + } + + diag.multipart_suggestion( + format!("replace `{}` with `0..{range_end}.len()`", snippet(cx, arg.span, "..")), + vec![ + (pat.span, snippet(cx, index.span, "..").into_owned()), + (arg.span, format!("0..{range_end}.len()")), + ], + applicability, + ); + }, + ); + } +} + +/// Removes trailing `.iter()`, `.iter_mut()`, or `.into_iter()` calls from the given expression if +/// `len` can be called directly on the receiver. Note that this may be incorrect if the receiver is +/// a user-defined type whose `len` method has a different meaning than the standard library. +fn remove_trailing_iter<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> (&'tcx Expr<'tcx>, Applicability) { + let mut app = Applicability::MachineApplicable; + if let ExprKind::MethodCall(iter_path, iter_recv, _, _) = expr.kind + && matches!(iter_path.ident.name, sym::iter | sym::iter_mut | sym::into_iter) + && let iter_recv_ty = cx.typeck_results().expr_ty(iter_recv).peel_refs() + && let ty_is_builtin_container = is_builtin_container(cx, iter_recv_ty) + && (ty_is_builtin_container || get_adt_inherent_method(cx, iter_recv_ty, sym::len).is_some()) + { + if !ty_is_builtin_container { + app = Applicability::MaybeIncorrect; + } + + return (iter_recv, app); + } + + (expr, app) +} + +/// Return `true` if the given type is a built-in container type (array, slice, or collection). +fn is_builtin_container(cx: &LateContext<'_>, ty: Ty<'_>) -> bool { + ty.is_array() + || ty.is_slice() + || ty.is_array_slice() + || (matches!(*ty.kind(), ty::Adt(iter_base, _) + if [sym::Vec, sym::VecDeque, sym::LinkedList, sym::BTreeMap, sym::BTreeSet, sym::HashMap, sym::HashSet, sym::BinaryHeap] + .iter() + .any(|sym| cx.tcx.is_diagnostic_item(*sym, iter_base.did())))) +} diff --git a/clippy_utils/src/paths.rs b/clippy_utils/src/paths.rs index a960a65ead2f..853a26c91b90 100644 --- a/clippy_utils/src/paths.rs +++ b/clippy_utils/src/paths.rs @@ -31,6 +31,7 @@ pub const CHAR_IS_ASCII: [&str; 5] = ["core", "char", "methods", "", pub const IO_ERROR_NEW: [&str; 5] = ["std", "io", "error", "Error", "new"]; pub const IO_ERRORKIND_OTHER: [&str; 5] = ["std", "io", "error", "ErrorKind", "Other"]; pub const ALIGN_OF: [&str; 3] = ["core", "mem", "align_of"]; +pub const ITER_EXACT_SIZE_ITERATOR: [&str; 3] = ["core", "iter", "ExactSizeIterator"]; // Paths in clippy itself pub const MSRV_STACK: [&str; 3] = ["clippy_utils", "msrvs", "MsrvStack"]; diff --git a/tests/ui/unused_enumerate_value.fixed b/tests/ui/unused_enumerate_value.fixed new file mode 100644 index 000000000000..965f88de6d75 --- /dev/null +++ b/tests/ui/unused_enumerate_value.fixed @@ -0,0 +1,20 @@ +#![warn(clippy::unused_enumerate_value)] + +fn main() { + let mut array = [1, 2, 3]; + for index in 0..array.len() { + //~^ unused_enumerate_value + todo!(); + } + + let my_iter = vec![1, 2, 3].into_iter(); + for index in 0..my_iter.len() { + //~^ unused_enumerate_value + todo!(); + } + + let another_iter = vec![1, 2, 3].into_iter(); + for (index, _) in another_iter.enumerate().map(|(index, x)| (index, x + 1)) { + todo!(); + } +} diff --git a/tests/ui/unused_enumerate_value.rs b/tests/ui/unused_enumerate_value.rs new file mode 100644 index 000000000000..6441cfc5e06f --- /dev/null +++ b/tests/ui/unused_enumerate_value.rs @@ -0,0 +1,20 @@ +#![warn(clippy::unused_enumerate_value)] + +fn main() { + let mut array = [1, 2, 3]; + for (index, _) in array.iter_mut().enumerate() { + //~^ unused_enumerate_value + todo!(); + } + + let my_iter = vec![1, 2, 3].into_iter(); + for (index, _) in my_iter.enumerate() { + //~^ unused_enumerate_value + todo!(); + } + + let another_iter = vec![1, 2, 3].into_iter(); + for (index, _) in another_iter.enumerate().map(|(index, x)| (index, x + 1)) { + todo!(); + } +} diff --git a/tests/ui/unused_enumerate_value.stderr b/tests/ui/unused_enumerate_value.stderr new file mode 100644 index 000000000000..138bd0b145d4 --- /dev/null +++ b/tests/ui/unused_enumerate_value.stderr @@ -0,0 +1,28 @@ +error: you seem to use `.enumerate()` and immediately discard the value + --> tests/ui/unused_enumerate_value.rs:5:23 + | +LL | for (index, _) in array.iter_mut().enumerate() { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: `-D clippy::unused-enumerate-value` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::unused_enumerate_value)]` +help: replace `array.iter_mut().enumerate()` with `0..array.len()` + | +LL - for (index, _) in array.iter_mut().enumerate() { +LL + for index in 0..array.len() { + | + +error: you seem to use `.enumerate()` and immediately discard the value + --> tests/ui/unused_enumerate_value.rs:11:23 + | +LL | for (index, _) in my_iter.enumerate() { + | ^^^^^^^^^^^^^^^^^^^ + | +help: replace `my_iter.enumerate()` with `0..my_iter.len()` + | +LL - for (index, _) in my_iter.enumerate() { +LL + for index in 0..my_iter.len() { + | + +error: aborting due to 2 previous errors + diff --git a/tests/ui/unused_enumerate_value_unfixable.rs b/tests/ui/unused_enumerate_value_unfixable.rs new file mode 100644 index 000000000000..ca09e36f500d --- /dev/null +++ b/tests/ui/unused_enumerate_value_unfixable.rs @@ -0,0 +1,27 @@ +//@no-rustfix +#![warn(clippy::unused_enumerate_value)] + +fn main() { + struct Length(usize); + + impl IntoIterator for Length { + type Item = usize; + type IntoIter = std::iter::Once; + + fn into_iter(self) -> Self::IntoIter { + std::iter::once(self.0) + } + } + + impl Length { + fn len(&self) -> usize { + self.0 + } + } + + let length = Length(3); + for (index, _) in length.into_iter().enumerate() { + //~^ unused_enumerate_value + todo!(); + } +} diff --git a/tests/ui/unused_enumerate_value_unfixable.stderr b/tests/ui/unused_enumerate_value_unfixable.stderr new file mode 100644 index 000000000000..90d02103956f --- /dev/null +++ b/tests/ui/unused_enumerate_value_unfixable.stderr @@ -0,0 +1,12 @@ +error: you seem to use `.enumerate()` and immediately discard the value + --> tests/ui/unused_enumerate_value_unfixable.rs:23:23 + | +LL | for (index, _) in length.into_iter().enumerate() { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: consider using `0..length.len()` instead + = note: `-D clippy::unused-enumerate-value` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::unused_enumerate_value)]` + +error: aborting due to 1 previous error +