From 2603a1020e28b73cabb8b80559784146af3d4397 Mon Sep 17 00:00:00 2001 From: Tatounee <66035819+Tatounee@users.noreply.github.com> Date: Sun, 21 Apr 2024 07:04:14 +0200 Subject: [PATCH 1/3] neddless_range_loop: improve documentation --- clippy_lints/src/loops/needless_range_loop.rs | 174 +++++++++++++++--- 1 file changed, 153 insertions(+), 21 deletions(-) diff --git a/clippy_lints/src/loops/needless_range_loop.rs b/clippy_lints/src/loops/needless_range_loop.rs index de7ec81bc010..f1208781dd47 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,17 @@ 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 +209,17 @@ 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 +228,76 @@ 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: Option, + end_val: Option, + array_len: 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 +311,42 @@ 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) -> u128 { + match full_int { + FullInt::U(u) => u, + FullInt::S(s) => s as u128, + } } - false + let start_val = constant_full_int(cx, cx.typeck_results(), start).map(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.into()) + } else { + None + }; + + let end_val = end.as_ref().and_then(|end| { + constant_full_int(cx, cx.typeck_results(), end) + .map(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> { From 5c0c5e3fef66977d92cd5dd8e9ee98b79707b4f2 Mon Sep 17 00:00:00 2001 From: Tatounee <66035819+Tatounee@users.noreply.github.com> Date: Sun, 21 Apr 2024 07:04:40 +0200 Subject: [PATCH 2/3] bless --- tests/ui/needless_range_loop.stderr | 8 ++++++++ tests/ui/needless_range_loop2.stderr | 3 +++ 2 files changed, 11 insertions(+) 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) { From f083c9c42602f8004b75c1dd60d6baea8420c252 Mon Sep 17 00:00:00 2001 From: Tatounee Date: Sun, 21 Apr 2024 20:35:50 +0200 Subject: [PATCH 3/3] fixing dogfood test --- clippy_lints/src/loops/needless_range_loop.rs | 41 ++++++++----------- 1 file changed, 16 insertions(+), 25 deletions(-) diff --git a/clippy_lints/src/loops/needless_range_loop.rs b/clippy_lints/src/loops/needless_range_loop.rs index f1208781dd47..1937e8d5e91e 100644 --- a/clippy_lints/src/loops/needless_range_loop.rs +++ b/clippy_lints/src/loops/needless_range_loop.rs @@ -91,7 +91,7 @@ pub(super) fn check<'tcx>( }; let (start_val, end_val, array_len) = - try_get_start_end_and_array_len_vals(cx, start, end, limits, indexed_ty); + 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; @@ -182,13 +182,11 @@ pub(super) fn check<'tcx>( diag, indexed, end_is_start_plus_val, - start_val, - end_val, - array_len, + (start_val, end_val, array_len), sugg_skip_val, sugg_take_val, end_snippet, - ) + ); }, ); } else { @@ -213,13 +211,11 @@ pub(super) fn check<'tcx>( diag, indexed, end_is_start_plus_val, - start_val, - end_val, - array_len, + (start_val, end_val, array_len), sugg_skip_val, sugg_take_val, end_snippet, - ) + ); }, ); } @@ -232,9 +228,7 @@ fn note_suggestion_might_skip_iteration( diag: &mut rustc_errors::Diag<'_, ()>, indexed: Symbol, end_is_start_plus_val: bool, - start_val: Option, - end_val: Option, - array_len: Option, + (start_val, end_val, array_len): (Option, Option, Option), sugg_skip_val: Option>, sugg_take_val: Option>, end_snippet: Option>, @@ -263,7 +257,7 @@ fn note_suggestion_might_skip_iteration( if let Some(sugg_skip_val) = sugg_skip_val && (start_val.is_none() || array_len.is_none()) { - bounds.push(sugg_skip_val) + bounds.push(sugg_skip_val); } // Case where the loop have the form `for _ in /* .. */..(=)a {}` @@ -271,18 +265,18 @@ fn note_suggestion_might_skip_iteration( && !end_is_start_plus_val && (end_val.is_none() || array_len.is_none()) { - bounds.push(sugg_take_val) + 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))) + 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()) + bounds.push(end_snippet.unwrap()); } if !bounds.is_empty() { @@ -316,30 +310,27 @@ fn is_len_call(expr: &Expr<'_>, var: Symbol) -> bool { fn try_get_start_end_and_array_len_vals<'tcx>( cx: &LateContext<'tcx>, start: &Expr<'_>, - end: &Option<&Expr<'_>>, + end: Option<&Expr<'_>>, limits: ast::RangeLimits, indexed_ty: Ty<'tcx>, ) -> (Option, Option, Option) { - fn full_int_to_u128(full_int: FullInt) -> u128 { - match full_int { - FullInt::U(u) => u, - FullInt::S(s) => s as u128, - } + fn full_int_to_u128(full_int: FullInt) -> Option { + if let FullInt::U(u) = full_int { Some(u) } else { None } } - let start_val = constant_full_int(cx, cx.typeck_results(), start).map(full_int_to_u128); + 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.into()) + .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) - .map(full_int_to_u128) + .and_then(full_int_to_u128) .map(|end_val| match limits { ast::RangeLimits::Closed => end_val + 1, ast::RangeLimits::HalfOpen => end_val,