From 72f343934680d27d7a38972a165582198deb3bc4 Mon Sep 17 00:00:00 2001 From: HMPerson1 Date: Fri, 18 Oct 2019 00:03:27 -0400 Subject: [PATCH 1/3] Suggest calling `iter` if needed in `explicit_counter_loop` --- clippy_lints/src/loops.rs | 52 +++++++++++++++++++-------- tests/ui/explicit_counter_loop.rs | 10 ++++++ tests/ui/explicit_counter_loop.stderr | 26 ++++++++++---- 3 files changed, 67 insertions(+), 21 deletions(-) diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index 22821e02fe29..81a8c313e9de 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! { @@ -1470,17 +1471,7 @@ fn check_for_loop_explicit_counter<'a, 'tcx>( "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 +1481,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/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..c5643a857b1d 100644 --- a/tests/ui/explicit_counter_loop.stderr +++ b/tests/ui/explicit_counter_loop.stderr @@ -2,7 +2,7 @@ error: the variable `_index` is used as a loop counter. --> $DIR/explicit_counter_loop.rs:6:15 | 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` @@ -10,25 +10,37 @@ error: the variable `_index` is used as a loop counter. --> $DIR/explicit_counter_loop.rs:12:15 | 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:15 + | +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:15 + | +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:19 | LL | for ch in text.chars() { | ^^^^^^^^^^^^ 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:19 | LL | for ch in text.chars() { | ^^^^^^^^^^^^ 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:19 | 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 From 4578e5e15e79ee800859359672fe0f4db5b164cc Mon Sep 17 00:00:00 2001 From: HMPerson1 Date: Fri, 18 Oct 2019 01:15:54 -0400 Subject: [PATCH 2/3] Fix suggestion span in `explicit_counter_loop` --- clippy_lints/src/loops.rs | 11 ++++++++++- tests/ui/explicit_counter_loop.stderr | 28 +++++++++++++-------------- 2 files changed, 24 insertions(+), 15 deletions(-) diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index 81a8c313e9de..731dd92c82ae 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -1461,10 +1461,19 @@ 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!( diff --git a/tests/ui/explicit_counter_loop.stderr b/tests/ui/explicit_counter_loop.stderr index c5643a857b1d..1853e0c054c4 100644 --- a/tests/ui/explicit_counter_loop.stderr +++ b/tests/ui/explicit_counter_loop.stderr @@ -1,46 +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.iter().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.iter().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:15 + --> $DIR/explicit_counter_loop.rs:17:5 | LL | for _v in &mut vec { - | ^^^^^^^^ help: consider using: `for (_index, _v) in vec.iter_mut().enumerate()` + | ^^^^^^^^^^^^^^^^^^ 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:15 + --> $DIR/explicit_counter_loop.rs:22:5 | LL | for _v in vec { - | ^^^ help: consider using: `for (_index, _v) in vec.into_iter().enumerate()` + | ^^^^^^^^^^^^^ 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:61: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:72: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:130: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 7 previous errors From a9cb2b9001313046e7a8ccb2fc4b1619d64cc4da Mon Sep 17 00:00:00 2001 From: HMPerson1 Date: Fri, 18 Oct 2019 12:11:15 -0400 Subject: [PATCH 3/3] Fix suggestion for ranges --- clippy_lints/src/utils/sugg.rs | 14 +++++++++++--- tests/ui/explicit_counter_loop.stderr | 2 +- 2 files changed, 12 insertions(+), 4 deletions(-) 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.stderr b/tests/ui/explicit_counter_loop.stderr index 1853e0c054c4..931af46efe66 100644 --- a/tests/ui/explicit_counter_loop.stderr +++ b/tests/ui/explicit_counter_loop.stderr @@ -40,7 +40,7 @@ error: the variable `count` is used as a loop counter. --> $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 7 previous errors