Skip to content

Commit

Permalink
Add new map_with_unused_argument_over_ranges lint
Browse files Browse the repository at this point in the history
This lint checks for code that looks like
```rust
  let something : Vec<_> = (0..100).map(|_| {
    1 + 2 + 3
  }).collect();
```
which is more clear as
```rust
  let something : Vec<_> = std::iter::repeat_with(|| {
    1 + 2 + 3
  }).take(100).collect();
```
or
```rust
  let something : Vec<_> =
      std::iter::repeat_n(1 + 2 + 3, 100)
      .collect();
```

That is, a map over a range which does nothing with the parameter
passed to it is simply a function (or closure) being called `n`
times and could be more semantically expressed using `take`.
  • Loading branch information
rspencer01 committed Oct 29, 2024
1 parent 35a7095 commit acc3842
Show file tree
Hide file tree
Showing 16 changed files with 558 additions and 8 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5697,6 +5697,7 @@ Released 2018-09-13
[`map_flatten`]: https://rust-lang.github.io/rust-clippy/master/index.html#map_flatten
[`map_identity`]: https://rust-lang.github.io/rust-clippy/master/index.html#map_identity
[`map_unwrap_or`]: https://rust-lang.github.io/rust-clippy/master/index.html#map_unwrap_or
[`map_with_unused_argument_over_ranges`]: https://rust-lang.github.io/rust-clippy/master/index.html#map_with_unused_argument_over_ranges
[`match_as_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#match_as_ref
[`match_bool`]: https://rust-lang.github.io/rust-clippy/master/index.html#match_bool
[`match_like_matches_macro`]: https://rust-lang.github.io/rust-clippy/master/index.html#match_like_matches_macro
Expand Down
1 change: 1 addition & 0 deletions book/src/lint_configuration.md
Original file line number Diff line number Diff line change
Expand Up @@ -710,6 +710,7 @@ The minimum rust version that the project supports. Defaults to the `rust-versio
* [`manual_try_fold`](https://rust-lang.github.io/rust-clippy/master/index.html#manual_try_fold)
* [`map_clone`](https://rust-lang.github.io/rust-clippy/master/index.html#map_clone)
* [`map_unwrap_or`](https://rust-lang.github.io/rust-clippy/master/index.html#map_unwrap_or)
* [`map_with_unused_argument_over_ranges`](https://rust-lang.github.io/rust-clippy/master/index.html#map_with_unused_argument_over_ranges)
* [`match_like_matches_macro`](https://rust-lang.github.io/rust-clippy/master/index.html#match_like_matches_macro)
* [`mem_replace_with_default`](https://rust-lang.github.io/rust-clippy/master/index.html#mem_replace_with_default)
* [`missing_const_for_fn`](https://rust-lang.github.io/rust-clippy/master/index.html#missing_const_for_fn)
Expand Down
1 change: 1 addition & 0 deletions clippy_config/src/conf.rs
Original file line number Diff line number Diff line change
Expand Up @@ -573,6 +573,7 @@ define_Conf! {
manual_try_fold,
map_clone,
map_unwrap_or,
map_with_unused_argument_over_ranges,
match_like_matches_macro,
mem_replace_with_default,
missing_const_for_fn,
Expand Down
4 changes: 2 additions & 2 deletions clippy_config/src/msrvs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ macro_rules! msrv_aliases {
// names may refer to stabilized feature flags or library items
msrv_aliases! {
1,83,0 { CONST_EXTERN_FN, CONST_FLOAT_BITS_CONV, CONST_FLOAT_CLASSIFY }
1,82,0 { IS_NONE_OR }
1,82,0 { IS_NONE_OR, REPEAT_N }
1,81,0 { LINT_REASONS_STABILIZATION }
1,80,0 { BOX_INTO_ITER}
1,77,0 { C_STR_LITERALS }
Expand Down Expand Up @@ -55,7 +55,7 @@ msrv_aliases! {
1,33,0 { UNDERSCORE_IMPORTS }
1,30,0 { ITERATOR_FIND_MAP, TOOL_ATTRIBUTES }
1,29,0 { ITER_FLATTEN }
1,28,0 { FROM_BOOL }
1,28,0 { FROM_BOOL, REPEAT_WITH }
1,27,0 { ITERATOR_TRY_FOLD }
1,26,0 { RANGE_INCLUSIVE, STRING_RETAIN }
1,24,0 { IS_ASCII_DIGIT }
Expand Down
1 change: 1 addition & 0 deletions clippy_lints/src/declared_lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -423,6 +423,7 @@ pub static LINTS: &[&crate::LintInfo] = &[
crate::methods::MAP_FLATTEN_INFO,
crate::methods::MAP_IDENTITY_INFO,
crate::methods::MAP_UNWRAP_OR_INFO,
crate::methods::MAP_WITH_UNUSED_ARGUMENT_OVER_RANGES_INFO,
crate::methods::MUT_MUTEX_LOCK_INFO,
crate::methods::NAIVE_BYTECOUNT_INFO,
crate::methods::NEEDLESS_AS_BYTES_INFO,
Expand Down
5 changes: 4 additions & 1 deletion clippy_lints/src/matches/single_match.rs
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,10 @@ impl<'a> PatState<'a> {
let states = match self {
Self::Wild => return None,
Self::Other => {
*self = Self::StdEnum(cx.arena.alloc_from_iter((0..adt.variants().len()).map(|_| Self::Other)));
*self = Self::StdEnum(
cx.arena
.alloc_from_iter(std::iter::repeat_with(|| Self::Other).take(adt.variants().len())),
);
let Self::StdEnum(x) = self else {
unreachable!();
};
Expand Down
134 changes: 134 additions & 0 deletions clippy_lints/src/methods/map_with_unused_argument_over_ranges.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,134 @@
use crate::methods::MAP_WITH_UNUSED_ARGUMENT_OVER_RANGES;
use clippy_config::msrvs::{self, Msrv};
use clippy_utils::diagnostics::span_lint_and_then;
use clippy_utils::source::snippet_with_applicability;
use clippy_utils::sugg::Sugg;
use clippy_utils::{eager_or_lazy, higher, usage};
use rustc_ast::LitKind;
use rustc_ast::ast::RangeLimits;
use rustc_data_structures::packed::Pu128;
use rustc_errors::Applicability;
use rustc_hir::{Body, Closure, Expr, ExprKind};
use rustc_lint::LateContext;
use rustc_span::Span;

fn extract_count_with_applicability(
cx: &LateContext<'_>,
range: higher::Range<'_>,
applicability: &mut Applicability,
) -> Option<String> {
let start = range.start?;
let end = range.end?;
// TODO: This doens't handle if either the start or end are negative literals, or if the start is
// not a literal. In the first case, we need to be careful about how we handle computing the
// count to avoid overflows. In the second, we may need to add parenthesis to make the
// suggestion correct.
if let ExprKind::Lit(lit) = start.kind
&& let LitKind::Int(Pu128(lower_bound), _) = lit.node
{
if let ExprKind::Lit(lit) = end.kind
&& let LitKind::Int(Pu128(upper_bound), _) = lit.node
{
// Here we can explicitly calculate the number of iterations
let count = if upper_bound >= lower_bound {
match range.limits {
RangeLimits::HalfOpen => upper_bound - lower_bound,
RangeLimits::Closed => (upper_bound - lower_bound).checked_add(1)?,
}
} else {
0
};
return Some(format!("{count}"));
}
let end_snippet = Sugg::hir_with_applicability(cx, end, "...", applicability)
.maybe_par()
.into_string();
if lower_bound == 0 {
if range.limits == RangeLimits::Closed {
return Some(format!("{end_snippet} + 1"));
}
return Some(end_snippet);
}
if range.limits == RangeLimits::Closed {
return Some(format!("{end_snippet} - {}", lower_bound - 1));
}
return Some(format!("{end_snippet} - {lower_bound}"));
}
None
}

pub(super) fn check(
cx: &LateContext<'_>,
ex: &Expr<'_>,
receiver: &Expr<'_>,
arg: &Expr<'_>,
msrv: &Msrv,
method_call_span: Span,
) {
let mut applicability = Applicability::MaybeIncorrect;
if let Some(range) = higher::Range::hir(receiver)
&& let ExprKind::Closure(Closure { body, .. }) = arg.kind
&& let body_hir = cx.tcx.hir().body(*body)
&& let Body {
params: [param],
value: body_expr,
} = body_hir
&& !usage::BindingUsageFinder::are_params_used(cx, body_hir)
&& let Some(count) = extract_count_with_applicability(cx, range, &mut applicability)
{
let method_to_use_name;
let new_span;
let use_take;

if eager_or_lazy::switch_to_eager_eval(cx, body_expr) {
if msrv.meets(msrvs::REPEAT_N) {
method_to_use_name = "repeat_n";
let body_snippet = snippet_with_applicability(cx, body_expr.span, "..", &mut applicability);
new_span = (arg.span, format!("{body_snippet}, {count}"));
use_take = false;
} else {
method_to_use_name = "repeat";
let body_snippet = snippet_with_applicability(cx, body_expr.span, "..", &mut applicability);
new_span = (arg.span, body_snippet.to_string());
use_take = true;
}
} else if msrv.meets(msrvs::REPEAT_WITH) {
method_to_use_name = "repeat_with";
new_span = (param.span, String::new());
use_take = true;
} else {
return;
}

// We need to provide nonempty parts to diag.multipart_suggestion so we
// collate all our parts here and then remove those that are empty.
let mut parts = vec![
(
receiver.span.to(method_call_span),
format!("std::iter::{method_to_use_name}"),
),
new_span,
];
if use_take {
parts.push((ex.span.shrink_to_hi(), format!(".take({count})")));
}

span_lint_and_then(
cx,
MAP_WITH_UNUSED_ARGUMENT_OVER_RANGES,
ex.span,
"map of a closure that does not depend on its parameter over a range",
|diag| {
diag.multipart_suggestion(
if use_take {
format!("remove the explicit range and use `{method_to_use_name}` and `take`")
} else {
format!("remove the explicit range and use `{method_to_use_name}`")
},
parts,
applicability,
);
},
);
}
}
37 changes: 37 additions & 0 deletions clippy_lints/src/methods/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ mod map_err_ignore;
mod map_flatten;
mod map_identity;
mod map_unwrap_or;
mod map_with_unused_argument_over_ranges;
mod mut_mutex_lock;
mod needless_as_bytes;
mod needless_character_iteration;
Expand Down Expand Up @@ -4218,6 +4219,40 @@ declare_clippy_lint! {
"combine `.map(_)` followed by `.all(identity)`/`.any(identity)` into a single call"
}

declare_clippy_lint! {
/// ### What it does
///
/// Checks for `Iterator::map` over ranges without using the parameter which
/// could be more clearly expressed using `std::iter::repeat(...).take(...)`
/// or `std::iter::repeat_n`.
///
/// ### Why is this bad?
///
/// It expresses 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::repeat( 3 + 1 ).take(10).collect();
/// ```
///
/// ### Known Issues
///
/// This lint may suggest replacing a `Map<Range>` with a `Take<RepeatWith>`.
/// The former implements some traits that the latter does not, such as
/// `DoubleEndedIterator`.
#[clippy::version = "1.84.0"]
pub MAP_WITH_UNUSED_ARGUMENT_OVER_RANGES,
restriction,
"map of a trivial closure (not dependent on parameter) over a range"
}

pub struct Methods {
avoid_breaking_exported_api: bool,
msrv: Msrv,
Expand Down Expand Up @@ -4381,6 +4416,7 @@ impl_lint_pass!(Methods => [
UNNECESSARY_MIN_OR_MAX,
NEEDLESS_AS_BYTES,
MAP_ALL_ANY_IDENTITY,
MAP_WITH_UNUSED_ARGUMENT_OVER_RANGES,
]);

/// Extracts a method call name, args, and `Span` of the method name.
Expand Down Expand Up @@ -4876,6 +4912,7 @@ impl Methods {
if name == "map" {
unused_enumerate_index::check(cx, expr, recv, m_arg);
map_clone::check(cx, expr, recv, m_arg, &self.msrv);
map_with_unused_argument_over_ranges::check(cx, expr, recv, m_arg, &self.msrv, span);
match method_call(recv) {
Some((map_name @ ("iter" | "into_iter"), recv2, _, _, _)) => {
iter_kv_map::check(cx, map_name, expr, recv2, m_arg, &self.msrv);
Expand Down
73 changes: 73 additions & 0 deletions tests/ui/map_with_unused_argument_over_ranges.fixed
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
#![allow(
unused,
clippy::redundant_closure,
clippy::reversed_empty_ranges,
clippy::identity_op
)]
#![warn(clippy::map_with_unused_argument_over_ranges)]

fn do_something() -> usize {
todo!()
}

fn do_something_interesting(x: usize, y: usize) -> usize {
todo!()
}

macro_rules! gen {
() => {
(0..10).map(|_| do_something());
};
}

fn main() {
// These should be raised
std::iter::repeat_with(|| do_something()).take(10);
std::iter::repeat_with(|| do_something()).take(10);
std::iter::repeat_with(|| do_something()).take(11);
std::iter::repeat_with(|| do_something()).take(7);
std::iter::repeat_with(|| do_something()).take(8);
std::iter::repeat_n(3, 10);
std::iter::repeat_with(|| {
let x = 3;
x + 2
}).take(10);
std::iter::repeat_with(|| do_something()).take(10).collect::<Vec<_>>();
let upper = 4;
std::iter::repeat_with(|| do_something()).take(upper);
let upper_fn = || 4;
std::iter::repeat_with(|| do_something()).take(upper_fn());
std::iter::repeat_with(|| do_something()).take(upper_fn() + 1);
std::iter::repeat_with(|| do_something()).take(upper_fn() - 2);
std::iter::repeat_with(|| do_something()).take(upper_fn() - 1);
(-3..9).map(|_| do_something());
std::iter::repeat_with(|| do_something()).take(0);
std::iter::repeat_with(|| do_something()).take(1);
std::iter::repeat_with(|| do_something()).take((1 << 4) - 0);
// These should not be raised
gen!();
let lower = 2;
let lower_fn = || 2;
(lower..upper_fn()).map(|_| do_something()); // Ranges not starting at zero not yet handled
(lower_fn()..upper_fn()).map(|_| do_something()); // Ranges not starting at zero not yet handled
(lower_fn()..=upper_fn()).map(|_| do_something()); // Ranges not starting at zero not yet handled
(0..10).map(|x| do_something_interesting(x, 4)); // Actual map over range
"Foobar".chars().map(|_| do_something()); // Not a map over range
// i128::MAX == 340282366920938463463374607431768211455
(0..=340282366920938463463374607431768211455).map(|_: u128| do_something()); // Can't be replaced due to overflow
}

#[clippy::msrv = "1.27"]
fn msrv_1_27() {
(0..10).map(|_| do_something());
}

#[clippy::msrv = "1.28"]
fn msrv_1_28() {
std::iter::repeat_with(|| do_something()).take(10);
}

#[clippy::msrv = "1.81"]
fn msrv_1_82() {
std::iter::repeat(3).take(10);
}
Loading

0 comments on commit acc3842

Please sign in to comment.