diff --git a/CHANGELOG.md b/CHANGELOG.md index 0974631ac5dc..24d718e0d18c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5884,6 +5884,7 @@ Released 2018-09-13 [`transmutes_expressible_as_ptr_casts`]: https://rust-lang.github.io/rust-clippy/master/index.html#transmutes_expressible_as_ptr_casts [`transmuting_null`]: https://rust-lang.github.io/rust-clippy/master/index.html#transmuting_null [`trim_split_whitespace`]: https://rust-lang.github.io/rust-clippy/master/index.html#trim_split_whitespace +[`trivial_map_over_range`]: https://rust-lang.github.io/rust-clippy/master/index.html#trivial_map_over_range [`trivial_regex`]: https://rust-lang.github.io/rust-clippy/master/index.html#trivial_regex [`trivially_copy_pass_by_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#trivially_copy_pass_by_ref [`try_err`]: https://rust-lang.github.io/rust-clippy/master/index.html#try_err diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 53f346c04e78..01de94875807 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -705,6 +705,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[ crate::transmute::UNSOUND_COLLECTION_TRANSMUTE_INFO, crate::transmute::USELESS_TRANSMUTE_INFO, crate::transmute::WRONG_TRANSMUTE_INFO, + crate::trivial_map_over_range::TRIVIAL_MAP_OVER_RANGE_INFO, crate::tuple_array_conversions::TUPLE_ARRAY_CONVERSIONS_INFO, crate::types::BORROWED_BOX_INFO, crate::types::BOX_COLLECTION_INFO, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index fa4c7084f4dd..6b4a815bda86 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -345,6 +345,7 @@ mod to_string_trait_impl; mod trailing_empty_array; mod trait_bounds; mod transmute; +mod trivial_map_over_range; mod tuple_array_conversions; mod types; mod unconditional_recursion; @@ -1172,6 +1173,7 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) { }); store.register_late_pass(move |_| Box::new(string_patterns::StringPatterns::new(msrv()))); store.register_early_pass(|| Box::new(field_scoped_visibility_modifiers::FieldScopedVisibilityModifiers)); + store.register_late_pass(|_| Box::new(trivial_map_over_range::TrivialMapOverRange)); // add lints here, do not remove this comment, it's used in `new_lint` } diff --git a/clippy_lints/src/trivial_map_over_range.rs b/clippy_lints/src/trivial_map_over_range.rs new file mode 100644 index 000000000000..444bdb1e4229 --- /dev/null +++ b/clippy_lints/src/trivial_map_over_range.rs @@ -0,0 +1,83 @@ +use clippy_utils::diagnostics::span_lint_and_sugg; +use clippy_utils::source::snippet_with_applicability; +use rustc_ast::LitKind; +use rustc_data_structures::packed::Pu128; +use rustc_errors::Applicability; +use rustc_hir::{Body, Closure, Expr, ExprKind, LangItem, PatKind, QPath}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_session::declare_lint_pass; + +declare_clippy_lint! { + /// ### What it does + /// + /// Lint to prevent trivial `map`s over ranges when one could use `take`. + /// + /// ### Why is this bad? + /// + /// It expreses the intent more clearly to `take` the correct number of times + /// from a generating function than to apply a closure to each number in a + /// range only to discard them. + /// + /// ### Example + /// ```no_run + /// let random_numbers : Vec<_> = (0..10).map(|_| { 3 + 1 }).collect(); + /// ``` + /// Use instead: + /// ```no_run + /// let f : Vec<_> = std::iter::from_fn(|| { 3 + 1 }).take(10).collect(); + /// ``` + #[clippy::version = "1.81.0"] + pub TRIVIAL_MAP_OVER_RANGE, + style, + "map of a trivial closure (not dependent on parameter) over a range" +} + +declare_lint_pass!(TrivialMapOverRange => [TRIVIAL_MAP_OVER_RANGE]); + +impl LateLintPass<'_> for TrivialMapOverRange { + fn check_expr(&mut self, cx: &LateContext<'_>, ex: &Expr<'_>) { + if let ExprKind::MethodCall(path, receiver, [map_arg_expr], _call_span) = ex.kind + && path.ident.name == rustc_span::sym::map + && let ExprKind::Struct(qpath, fields, _) = receiver.kind + && matches!(qpath, QPath::LangItem(LangItem::Range, _)) + && fields.len() == 2 + && let ExprKind::Closure(Closure { body, .. }) = map_arg_expr.kind + && let Body { params: [param], .. } = cx.tcx.hir().body(*body) + && matches!(param.pat.kind, PatKind::Wild) + && let ExprKind::Lit(lit) = fields[0].expr.kind + && let LitKind::Int(Pu128(lower_bound), _) = lit.node + { + if let ExprKind::Lit(lit) = fields[1].expr.kind + && let LitKind::Int(Pu128(upper_bound), _) = lit.node + { + let count = upper_bound - lower_bound; + let mut applicability = Applicability::MaybeIncorrect; + let snippet = snippet_with_applicability(cx, map_arg_expr.span, "|| { ... }", &mut applicability) + .replacen("|_|", "||", 1); + span_lint_and_sugg( + cx, + TRIVIAL_MAP_OVER_RANGE, + ex.span, + "map of a trivial closure (not dependent on parameter) over a range", + "use", + format!("std::iter::repeat_with({snippet}).take({count})"), + applicability, + ); + } else if lower_bound == 0 { + let mut applicability = Applicability::MaybeIncorrect; + let count = snippet_with_applicability(cx, fields[1].expr.span, "...", &mut applicability); + let snippet = snippet_with_applicability(cx, map_arg_expr.span, "|| { ... }", &mut applicability) + .replacen("|_|", "||", 1); + span_lint_and_sugg( + cx, + TRIVIAL_MAP_OVER_RANGE, + ex.span, + "map of a trivial closure (not dependent on parameter) over a range", + "use", + format!("std::iter::repeat_with({snippet}).take({count})"), + applicability, + ); + } + } + } +} diff --git a/tests/ui/repeat_vec_with_capacity.fixed b/tests/ui/repeat_vec_with_capacity.fixed index 2afe2f433258..4928bbb49ee2 100644 --- a/tests/ui/repeat_vec_with_capacity.fixed +++ b/tests/ui/repeat_vec_with_capacity.fixed @@ -1,3 +1,4 @@ +#![allow(clippy::trivial_map_over_range)] #![warn(clippy::repeat_vec_with_capacity)] fn main() { diff --git a/tests/ui/repeat_vec_with_capacity.rs b/tests/ui/repeat_vec_with_capacity.rs index 659f2a3953dd..612884304f25 100644 --- a/tests/ui/repeat_vec_with_capacity.rs +++ b/tests/ui/repeat_vec_with_capacity.rs @@ -1,3 +1,4 @@ +#![allow(clippy::trivial_map_over_range)] #![warn(clippy::repeat_vec_with_capacity)] fn main() { diff --git a/tests/ui/repeat_vec_with_capacity.stderr b/tests/ui/repeat_vec_with_capacity.stderr index cec9c6ea84a2..43027c9cb892 100644 --- a/tests/ui/repeat_vec_with_capacity.stderr +++ b/tests/ui/repeat_vec_with_capacity.stderr @@ -1,5 +1,5 @@ error: repeating `Vec::with_capacity` using `vec![x; n]`, which does not retain capacity - --> tests/ui/repeat_vec_with_capacity.rs:5:9 + --> tests/ui/repeat_vec_with_capacity.rs:6:9 | LL | vec![Vec::<()>::with_capacity(42); 123]; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -13,7 +13,7 @@ LL | (0..123).map(|_| Vec::<()>::with_capacity(42)).collect::>(); | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ error: repeating `Vec::with_capacity` using `vec![x; n]`, which does not retain capacity - --> tests/ui/repeat_vec_with_capacity.rs:11:9 + --> tests/ui/repeat_vec_with_capacity.rs:12:9 | LL | vec![Vec::<()>::with_capacity(42); n]; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -25,7 +25,7 @@ LL | (0..n).map(|_| Vec::<()>::with_capacity(42)).collect::>(); | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ error: repeating `Vec::with_capacity` using `iter::repeat`, which does not retain capacity - --> tests/ui/repeat_vec_with_capacity.rs:26:9 + --> tests/ui/repeat_vec_with_capacity.rs:27:9 | LL | std::iter::repeat(Vec::<()>::with_capacity(42)); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ diff --git a/tests/ui/trivial_map_over_range.fixed b/tests/ui/trivial_map_over_range.fixed new file mode 100644 index 000000000000..00b01ecd8b6c --- /dev/null +++ b/tests/ui/trivial_map_over_range.fixed @@ -0,0 +1,32 @@ +#![allow(unused, clippy::redundant_closure)] +#![warn(clippy::trivial_map_over_range)] + +fn do_something() -> usize { + todo!() +} + +fn do_something_interesting(x: usize, y: usize) -> usize { + todo!() +} + +fn main() { + // These should be raised + std::iter::repeat_with(|| do_something()).take(10); + std::iter::repeat_with(|| do_something()).take(7); + std::iter::repeat_with(|| 3).take(10); + std::iter::repeat_with(|| { + let x = 3; + x + 2 + }).take(10); + std::iter::repeat_with(|| do_something()).take(10).collect::>(); + let upper = 4; + std::iter::repeat_with(|| do_something()).take(upper); + let upper_fn = || 4; + std::iter::repeat_with(|| do_something()).take(upper_fn()); + // These should not be raised + (0..=10).map(|_| do_something()); // Inclusive bounds not yet handled + (0..10).map(|x| do_something()); // We do not detect unused parameters + (0..10).map(|x| do_something()).collect::>(); // We do not detect unused parameters + (0..10).map(|x| do_something_interesting(x, 4)); // Actual map over range + "Foobar".chars().map(|_| do_something()); // Not a map over range +} diff --git a/tests/ui/trivial_map_over_range.rs b/tests/ui/trivial_map_over_range.rs new file mode 100644 index 000000000000..da981d76f44a --- /dev/null +++ b/tests/ui/trivial_map_over_range.rs @@ -0,0 +1,32 @@ +#![allow(unused, clippy::redundant_closure)] +#![warn(clippy::trivial_map_over_range)] + +fn do_something() -> usize { + todo!() +} + +fn do_something_interesting(x: usize, y: usize) -> usize { + todo!() +} + +fn main() { + // These should be raised + (0..10).map(|_| do_something()); + (3..10).map(|_| do_something()); + (0..10).map(|_| 3); + (0..10).map(|_| { + let x = 3; + x + 2 + }); + (0..10).map(|_| do_something()).collect::>(); + let upper = 4; + (0..upper).map(|_| do_something()); + let upper_fn = || 4; + (0..upper_fn()).map(|_| do_something()); + // These should not be raised + (0..=10).map(|_| do_something()); // Inclusive bounds not yet handled + (0..10).map(|x| do_something()); // We do not detect unused parameters + (0..10).map(|x| do_something()).collect::>(); // We do not detect unused parameters + (0..10).map(|x| do_something_interesting(x, 4)); // Actual map over range + "Foobar".chars().map(|_| do_something()); // Not a map over range +} diff --git a/tests/ui/trivial_map_over_range.stderr b/tests/ui/trivial_map_over_range.stderr new file mode 100644 index 000000000000..0f96a52f54c6 --- /dev/null +++ b/tests/ui/trivial_map_over_range.stderr @@ -0,0 +1,58 @@ +error: map of a trivial closure (not dependent on parameter) over a range + --> tests/ui/trivial_map_over_range.rs:14:5 + | +LL | (0..10).map(|_| do_something()); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `std::iter::repeat_with(|| do_something()).take(10)` + | + = note: `-D clippy::trivial-map-over-range` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::trivial_map_over_range)]` + +error: map of a trivial closure (not dependent on parameter) over a range + --> tests/ui/trivial_map_over_range.rs:15:5 + | +LL | (3..10).map(|_| do_something()); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `std::iter::repeat_with(|| do_something()).take(7)` + +error: map of a trivial closure (not dependent on parameter) over a range + --> tests/ui/trivial_map_over_range.rs:16:5 + | +LL | (0..10).map(|_| 3); + | ^^^^^^^^^^^^^^^^^^ help: use: `std::iter::repeat_with(|| 3).take(10)` + +error: map of a trivial closure (not dependent on parameter) over a range + --> tests/ui/trivial_map_over_range.rs:17:5 + | +LL | / (0..10).map(|_| { +LL | | let x = 3; +LL | | x + 2 +LL | | }); + | |______^ + | +help: use + | +LL ~ std::iter::repeat_with(|| { +LL + let x = 3; +LL + x + 2 +LL ~ }).take(10); + | + +error: map of a trivial closure (not dependent on parameter) over a range + --> tests/ui/trivial_map_over_range.rs:21:5 + | +LL | (0..10).map(|_| do_something()).collect::>(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `std::iter::repeat_with(|| do_something()).take(10)` + +error: map of a trivial closure (not dependent on parameter) over a range + --> tests/ui/trivial_map_over_range.rs:23:5 + | +LL | (0..upper).map(|_| do_something()); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `std::iter::repeat_with(|| do_something()).take(upper)` + +error: map of a trivial closure (not dependent on parameter) over a range + --> tests/ui/trivial_map_over_range.rs:25:5 + | +LL | (0..upper_fn()).map(|_| do_something()); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `std::iter::repeat_with(|| do_something()).take(upper_fn())` + +error: aborting due to 7 previous errors +