diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index 22821e02fe29..731dd92c82ae 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -27,9 +27,10 @@ use syntax_pos::{BytePos, Symbol}; use crate::utils::paths; use crate::utils::{ - get_enclosing_block, get_parent_expr, has_iter_method, higher, is_integer_const, is_refutable, last_path_segment, - match_trait_method, match_type, match_var, multispan_sugg, snippet, snippet_opt, snippet_with_applicability, - span_help_and_lint, span_lint, span_lint_and_sugg, span_lint_and_then, SpanlessEq, + get_enclosing_block, get_parent_expr, get_trait_def_id, has_iter_method, higher, implements_trait, + is_integer_const, is_refutable, last_path_segment, match_trait_method, match_type, match_var, multispan_sugg, + snippet, snippet_opt, snippet_with_applicability, span_help_and_lint, span_lint, span_lint_and_sugg, + span_lint_and_then, SpanlessEq, }; declare_clippy_lint! { @@ -1460,27 +1461,26 @@ fn check_for_loop_explicit_counter<'a, 'tcx>( if visitor2.state == VarState::Warn { if let Some(name) = visitor2.name { let mut applicability = Applicability::MachineApplicable; + + // for some reason this is the only way to get the `Span` + // of the entire `for` loop + let for_span = if let ExprKind::Match(_, arms, _) = &expr.kind { + arms[0].body.span + } else { + unreachable!() + }; + span_lint_and_sugg( cx, EXPLICIT_COUNTER_LOOP, - expr.span, + for_span.with_hi(arg.span.hi()), &format!("the variable `{}` is used as a loop counter.", name), "consider using", format!( "for ({}, {}) in {}.enumerate()", name, snippet_with_applicability(cx, pat.span, "item", &mut applicability), - if higher::range(cx, arg).is_some() { - format!( - "({})", - snippet_with_applicability(cx, arg.span, "_", &mut applicability) - ) - } else { - format!( - "{}", - sugg::Sugg::hir_with_applicability(cx, arg, "_", &mut applicability).maybe_par() - ) - } + make_iterator_snippet(cx, arg, &mut applicability), ), applicability, ); @@ -1490,6 +1490,39 @@ fn check_for_loop_explicit_counter<'a, 'tcx>( } } +/// If `arg` was the argument to a `for` loop, return the "cleanest" way of writing the +/// actual `Iterator` that the loop uses. +fn make_iterator_snippet(cx: &LateContext<'_, '_>, arg: &Expr, applic_ref: &mut Applicability) -> String { + let impls_iterator = get_trait_def_id(cx, &paths::ITERATOR) + .map_or(false, |id| implements_trait(cx, cx.tables.expr_ty(arg), id, &[])); + if impls_iterator { + format!( + "{}", + sugg::Sugg::hir_with_applicability(cx, arg, "_", applic_ref).maybe_par() + ) + } else { + // (&x).into_iter() ==> x.iter() + // (&mut x).into_iter() ==> x.iter_mut() + match &arg.kind { + ExprKind::AddrOf(mutability, arg_inner) if has_iter_method(cx, cx.tables.expr_ty(&arg_inner)).is_some() => { + let meth_name = match mutability { + MutMutable => "iter_mut", + MutImmutable => "iter", + }; + format!( + "{}.{}()", + sugg::Sugg::hir_with_applicability(cx, &arg_inner, "_", applic_ref).maybe_par(), + meth_name, + ) + }, + _ => format!( + "{}.into_iter()", + sugg::Sugg::hir_with_applicability(cx, arg, "_", applic_ref).maybe_par() + ), + } + } +} + /// Checks for the `FOR_KV_MAP` lint. fn check_for_loop_over_map_kv<'a, 'tcx>( cx: &LateContext<'a, 'tcx>, diff --git a/clippy_lints/src/utils/sugg.rs b/clippy_lints/src/utils/sugg.rs index 0675c6033410..4fe41a880cc7 100644 --- a/clippy_lints/src/utils/sugg.rs +++ b/clippy_lints/src/utils/sugg.rs @@ -46,7 +46,7 @@ impl<'a> Sugg<'a> { pub fn hir_opt(cx: &LateContext<'_, '_>, expr: &hir::Expr) -> Option { snippet_opt(cx, expr.span).map(|snippet| { let snippet = Cow::Owned(snippet); - Self::hir_from_snippet(expr, snippet) + Self::hir_from_snippet(cx, expr, snippet) }) } @@ -84,12 +84,20 @@ impl<'a> Sugg<'a> { pub fn hir_with_macro_callsite(cx: &LateContext<'_, '_>, expr: &hir::Expr, default: &'a str) -> Self { let snippet = snippet_with_macro_callsite(cx, expr.span, default); - Self::hir_from_snippet(expr, snippet) + Self::hir_from_snippet(cx, expr, snippet) } /// Generate a suggestion for an expression with the given snippet. This is used by the `hir_*` /// function variants of `Sugg`, since these use different snippet functions. - fn hir_from_snippet(expr: &hir::Expr, snippet: Cow<'a, str>) -> Self { + fn hir_from_snippet(cx: &LateContext<'_, '_>, expr: &hir::Expr, snippet: Cow<'a, str>) -> Self { + if let Some(range) = higher::range(cx, expr) { + let op = match range.limits { + ast::RangeLimits::HalfOpen => AssocOp::DotDot, + ast::RangeLimits::Closed => AssocOp::DotDotEq, + }; + return Sugg::BinOp(op, snippet); + } + match expr.kind { hir::ExprKind::AddrOf(..) | hir::ExprKind::Box(..) diff --git a/tests/ui/explicit_counter_loop.rs b/tests/ui/explicit_counter_loop.rs index 71ef1e8674ac..e6fbf83a2879 100644 --- a/tests/ui/explicit_counter_loop.rs +++ b/tests/ui/explicit_counter_loop.rs @@ -12,6 +12,16 @@ fn main() { for _v in &vec { _index += 1 } + + let mut _index = 0; + for _v in &mut vec { + _index += 1; + } + + let mut _index = 0; + for _v in vec { + _index += 1; + } } mod issue_1219 { diff --git a/tests/ui/explicit_counter_loop.stderr b/tests/ui/explicit_counter_loop.stderr index 5efd51abf181..931af46efe66 100644 --- a/tests/ui/explicit_counter_loop.stderr +++ b/tests/ui/explicit_counter_loop.stderr @@ -1,34 +1,46 @@ error: the variable `_index` is used as a loop counter. - --> $DIR/explicit_counter_loop.rs:6:15 + --> $DIR/explicit_counter_loop.rs:6:5 | LL | for _v in &vec { - | ^^^^ help: consider using: `for (_index, _v) in (&vec).enumerate()` + | ^^^^^^^^^^^^^^ help: consider using: `for (_index, _v) in vec.iter().enumerate()` | = note: `-D clippy::explicit-counter-loop` implied by `-D warnings` error: the variable `_index` is used as a loop counter. - --> $DIR/explicit_counter_loop.rs:12:15 + --> $DIR/explicit_counter_loop.rs:12:5 | LL | for _v in &vec { - | ^^^^ help: consider using: `for (_index, _v) in (&vec).enumerate()` + | ^^^^^^^^^^^^^^ help: consider using: `for (_index, _v) in vec.iter().enumerate()` + +error: the variable `_index` is used as a loop counter. + --> $DIR/explicit_counter_loop.rs:17:5 + | +LL | for _v in &mut vec { + | ^^^^^^^^^^^^^^^^^^ help: consider using: `for (_index, _v) in vec.iter_mut().enumerate()` + +error: the variable `_index` is used as a loop counter. + --> $DIR/explicit_counter_loop.rs:22:5 + | +LL | for _v in vec { + | ^^^^^^^^^^^^^ help: consider using: `for (_index, _v) in vec.into_iter().enumerate()` error: the variable `count` is used as a loop counter. - --> $DIR/explicit_counter_loop.rs:51:19 + --> $DIR/explicit_counter_loop.rs:61:9 | LL | for ch in text.chars() { - | ^^^^^^^^^^^^ help: consider using: `for (count, ch) in text.chars().enumerate()` + | ^^^^^^^^^^^^^^^^^^^^^^ help: consider using: `for (count, ch) in text.chars().enumerate()` error: the variable `count` is used as a loop counter. - --> $DIR/explicit_counter_loop.rs:62:19 + --> $DIR/explicit_counter_loop.rs:72:9 | LL | for ch in text.chars() { - | ^^^^^^^^^^^^ help: consider using: `for (count, ch) in text.chars().enumerate()` + | ^^^^^^^^^^^^^^^^^^^^^^ help: consider using: `for (count, ch) in text.chars().enumerate()` error: the variable `count` is used as a loop counter. - --> $DIR/explicit_counter_loop.rs:120:19 + --> $DIR/explicit_counter_loop.rs:130:9 | LL | for _i in 3..10 { - | ^^^^^ help: consider using: `for (count, _i) in (3..10).enumerate()` + | ^^^^^^^^^^^^^^^ help: consider using: `for (count, _i) in (3..10).enumerate()` -error: aborting due to 5 previous errors +error: aborting due to 7 previous errors