diff --git a/clippy_lints/src/loops/needless_range_loop.rs b/clippy_lints/src/loops/needless_range_loop.rs index de7ec81bc010..1937e8d5e91e 100644 --- a/clippy_lints/src/loops/needless_range_loop.rs +++ b/clippy_lints/src/loops/needless_range_loop.rs @@ -1,4 +1,5 @@ use super::NEEDLESS_RANGE_LOOP; +use clippy_utils::consts::{constant_full_int, FullInt}; use clippy_utils::diagnostics::{multispan_sugg, span_lint_and_then}; use clippy_utils::source::snippet; use clippy_utils::ty::has_iter_method; @@ -13,6 +14,7 @@ use rustc_lint::LateContext; use rustc_middle::middle::region; use rustc_middle::ty::{self, Ty}; use rustc_span::symbol::{sym, Symbol}; +use std::borrow::Cow; use std::{iter, mem}; /// Checks for looping over a range and then indexing a sequence with it. @@ -75,6 +77,7 @@ pub(super) fn check<'tcx>( return; } + let mut sugg_skip_val = None; let starts_at_zero = is_integer_const(cx, start, 0); let skip = if starts_at_zero { @@ -82,9 +85,14 @@ pub(super) fn check<'tcx>( } else if visitor.indexed_mut.contains(&indexed) && contains_name(indexed, start, cx) { return; } else { - format!(".skip({})", snippet(cx, start.span, "..")) + let skip_val = snippet(cx, start.span, ".."); + sugg_skip_val = Some(skip_val); + format!(".skip({})", sugg_skip_val.as_ref().unwrap()) }; + let (start_val, end_val, array_len) = + try_get_start_end_and_array_len_vals(cx, start, *end, limits, indexed_ty); + let mut sugg_take_val = None; let mut end_is_start_plus_val = false; let take = if let Some(end) = *end { @@ -105,20 +113,26 @@ pub(super) fn check<'tcx>( } } - if is_len_call(end, indexed) || is_end_eq_array_len(cx, end, limits, indexed_ty) { + let is_end_ge_array_len = if let (Some(end), Some(array_len)) = (end_val, array_len) { + end >= array_len + } else { + false + }; + if is_len_call(end, indexed) || is_end_ge_array_len { String::new() } else if visitor.indexed_mut.contains(&indexed) && contains_name(indexed, take_expr, cx) { return; } else { - match limits { + let take_val = match limits { ast::RangeLimits::Closed => { let take_expr = sugg::Sugg::hir(cx, take_expr, ""); - format!(".take({})", take_expr + sugg::ONE) - }, - ast::RangeLimits::HalfOpen => { - format!(".take({})", snippet(cx, take_expr.span, "..")) + Cow::Owned((take_expr + sugg::ONE).to_string()) }, - } + ast::RangeLimits::HalfOpen => snippet(cx, take_expr.span, ".."), + }; + + sugg_take_val = Some(take_val); + format!(".take({})", sugg_take_val.as_ref().unwrap()) } } else { String::new() @@ -138,6 +152,14 @@ pub(super) fn check<'tcx>( mem::swap(&mut method_1, &mut method_2); } + let end_snippet = end.map(|end| { + if matches!(limits, ast::RangeLimits::Closed) { + snippet(cx, end.span, "..") + " + 1" + } else { + snippet(cx, end.span, "..") + } + }); + if visitor.nonindex { span_lint_and_then( cx, @@ -156,6 +178,15 @@ pub(super) fn check<'tcx>( ), ], ); + note_suggestion_might_skip_iteration( + diag, + indexed, + end_is_start_plus_val, + (start_val, end_val, array_len), + sugg_skip_val, + sugg_take_val, + end_snippet, + ); }, ); } else { @@ -176,6 +207,15 @@ pub(super) fn check<'tcx>( "consider using an iterator", vec![(pat.span, "".to_string()), (arg.span, repl)], ); + note_suggestion_might_skip_iteration( + diag, + indexed, + end_is_start_plus_val, + (start_val, end_val, array_len), + sugg_skip_val, + sugg_take_val, + end_snippet, + ); }, ); } @@ -184,6 +224,74 @@ pub(super) fn check<'tcx>( } } +fn note_suggestion_might_skip_iteration( + diag: &mut rustc_errors::Diag<'_, ()>, + indexed: Symbol, + end_is_start_plus_val: bool, + (start_val, end_val, array_len): (Option, Option, Option), + sugg_skip_val: Option>, + sugg_take_val: Option>, + end_snippet: Option>, +) { + // If the array length is known, we can check if the suggestion will skip iterations + if let Some(array_len) = array_len { + if let Some(start_val) = start_val { + if start_val >= array_len { + diag.note(format!( + "suggestion will skip iterations because `{indexed}` is of length {array_len} which is less than {start_val}", + )); + } + } + if let Some(end_val) = end_val { + if end_val > array_len { + diag.note(format!( + "suggestion will skip iterations because `{indexed}` is of length {array_len} which is less than {end_val}", + )); + } + } + } + + let mut bounds = Vec::new(); + + // Case where the loop have the form `for _ in a../* .. */ {}` + if let Some(sugg_skip_val) = sugg_skip_val + && (start_val.is_none() || array_len.is_none()) + { + bounds.push(sugg_skip_val); + } + + // Case where the loop have the form `for _ in /* .. */..(=)a {}` + if let Some(sugg_take_val) = sugg_take_val + && !end_is_start_plus_val + && (end_val.is_none() || array_len.is_none()) + { + bounds.push(sugg_take_val); + } + + // Case where the loop have the form `for _ in /* .. */.. {}` + if end_snippet.is_none() { + bounds.push(Cow::Owned(format!("{}::MAX", sym::usize))); + } + + // Case where the loop have the form `for _ in a..(=)a+b {}` + if end_is_start_plus_val && (end_val.is_none() || array_len.is_none()) { + // Here `end_snippet` is `Some(a+b)` + bounds.push(end_snippet.unwrap()); + } + + if !bounds.is_empty() { + let bounds = bounds + .iter() + .map(|bound| format!("`{bound}`")) + .intersperse(" or ".to_owned()) + .collect::(); + + diag.note(format!( + "suggestion might skip expected iterations if `{indexed}.iter().count()` is less than {bounds}" + )); + } +} + fn is_len_call(expr: &Expr<'_>, var: Symbol) -> bool { if let ExprKind::MethodCall(method, recv, [], _) = expr.kind && method.ident.name == sym::len @@ -197,24 +305,39 @@ fn is_len_call(expr: &Expr<'_>, var: Symbol) -> bool { false } -fn is_end_eq_array_len<'tcx>( +/// Get the start, end and array length values from the range loop if they are constant. +/// `(start_val, end_val, array_len)` +fn try_get_start_end_and_array_len_vals<'tcx>( cx: &LateContext<'tcx>, - end: &Expr<'_>, + start: &Expr<'_>, + end: Option<&Expr<'_>>, limits: ast::RangeLimits, indexed_ty: Ty<'tcx>, -) -> bool { - if let ExprKind::Lit(lit) = end.kind - && let ast::LitKind::Int(end_int, _) = lit.node - && let ty::Array(_, arr_len_const) = indexed_ty.kind() - && let Some(arr_len) = arr_len_const.try_eval_target_usize(cx.tcx, cx.param_env) - { - return match limits { - ast::RangeLimits::Closed => end_int.get() + 1 >= arr_len.into(), - ast::RangeLimits::HalfOpen => end_int.get() >= arr_len.into(), - }; +) -> (Option, Option, Option) { + fn full_int_to_u128(full_int: FullInt) -> Option { + if let FullInt::U(u) = full_int { Some(u) } else { None } } - false + let start_val = constant_full_int(cx, cx.typeck_results(), start).and_then(full_int_to_u128); + + let array_len = if let ty::Array(_, arr_len_const) = indexed_ty.kind() { + arr_len_const + .try_eval_target_usize(cx.tcx, cx.param_env) + .map(|array_len| array_len as u128) + } else { + None + }; + + let end_val = end.as_ref().and_then(|end| { + constant_full_int(cx, cx.typeck_results(), end) + .and_then(full_int_to_u128) + .map(|end_val| match limits { + ast::RangeLimits::Closed => end_val + 1, + ast::RangeLimits::HalfOpen => end_val, + }) + }); + + (start_val, end_val, array_len) } struct VarVisitor<'a, 'tcx> { diff --git a/tests/ui/needless_range_loop.stderr b/tests/ui/needless_range_loop.stderr index dc2cf437e02e..c64c9334d206 100644 --- a/tests/ui/needless_range_loop.stderr +++ b/tests/ui/needless_range_loop.stderr @@ -61,6 +61,7 @@ error: the loop variable `i` is only used to index `vec2` LL | for i in 0..vec.len() { | ^^^^^^^^^^^^ | + = note: suggestion might skip expected iterations if `vec2.iter().count()` is less than `vec.len()` help: consider using an iterator | LL | for in vec2.iter().take(vec.len()) { @@ -72,6 +73,7 @@ error: the loop variable `i` is only used to index `vec` LL | for i in 5..vec.len() { | ^^^^^^^^^^^^ | + = note: suggestion might skip expected iterations if `vec.iter().count()` is less than `5` help: consider using an iterator | LL | for in vec.iter().skip(5) { @@ -83,6 +85,7 @@ error: the loop variable `i` is only used to index `vec` LL | for i in 0..MAX_LEN { | ^^^^^^^^^^ | + = note: suggestion might skip expected iterations if `vec.iter().count()` is less than `MAX_LEN` help: consider using an iterator | LL | for in vec.iter().take(MAX_LEN) { @@ -94,6 +97,7 @@ error: the loop variable `i` is only used to index `vec` LL | for i in 0..=MAX_LEN { | ^^^^^^^^^^^ | + = note: suggestion might skip expected iterations if `vec.iter().count()` is less than `MAX_LEN + 1` help: consider using an iterator | LL | for in vec.iter().take(MAX_LEN + 1) { @@ -105,6 +109,7 @@ error: the loop variable `i` is only used to index `vec` LL | for i in 5..10 { | ^^^^^ | + = note: suggestion might skip expected iterations if `vec.iter().count()` is less than `5` or `10` help: consider using an iterator | LL | for in vec.iter().take(10).skip(5) { @@ -116,6 +121,7 @@ error: the loop variable `i` is only used to index `vec` LL | for i in 5..=10 { | ^^^^^^ | + = note: suggestion might skip expected iterations if `vec.iter().count()` is less than `5` or `10 + 1` help: consider using an iterator | LL | for in vec.iter().take(10 + 1).skip(5) { @@ -127,6 +133,7 @@ error: the loop variable `i` is used to index `vec` LL | for i in 5..vec.len() { | ^^^^^^^^^^^^ | + = note: suggestion might skip expected iterations if `vec.iter().count()` is less than `5` help: consider using an iterator and enumerate() | LL | for (i, ) in vec.iter().enumerate().skip(5) { @@ -138,6 +145,7 @@ error: the loop variable `i` is used to index `vec` LL | for i in 5..10 { | ^^^^^ | + = note: suggestion might skip expected iterations if `vec.iter().count()` is less than `5` or `10` help: consider using an iterator and enumerate() | LL | for (i, ) in vec.iter().enumerate().take(10).skip(5) { diff --git a/tests/ui/needless_range_loop2.stderr b/tests/ui/needless_range_loop2.stderr index 353f30b1b26d..704088486436 100644 --- a/tests/ui/needless_range_loop2.stderr +++ b/tests/ui/needless_range_loop2.stderr @@ -4,6 +4,7 @@ error: the loop variable `i` is only used to index `ns` LL | for i in 3..10 { | ^^^^^ | + = note: suggestion might skip expected iterations if `ns.iter().count()` is less than `3` or `10` = note: `-D clippy::needless-range-loop` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(clippy::needless_range_loop)]` help: consider using an iterator @@ -39,6 +40,7 @@ error: the loop variable `i` is only used to index `vec` LL | for i in x..x + 4 { | ^^^^^^^^ | + = note: suggestion might skip expected iterations if `vec.iter().count()` is less than `x` or `x + 4` help: consider using an iterator | LL | for in vec.iter_mut().skip(x).take(4) { @@ -50,6 +52,7 @@ error: the loop variable `i` is only used to index `vec` LL | for i in x..=x + 4 { | ^^^^^^^^^ | + = note: suggestion might skip expected iterations if `vec.iter().count()` is less than `x` or `x + 4 + 1` help: consider using an iterator | LL | for in vec.iter_mut().skip(x).take(4 + 1) {