From 4b668159d2d790dce8868c36ed591394eeb2cf7c Mon Sep 17 00:00:00 2001 From: Josh Mcguigan Date: Wed, 5 Sep 2018 19:14:01 -0700 Subject: [PATCH 1/5] Closes #1219 false positive for explicit_counter_loop --- tests/ui/for_loop.rs | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/tests/ui/for_loop.rs b/tests/ui/for_loop.rs index 39eee6488..76bc96f91 100644 --- a/tests/ui/for_loop.rs +++ b/tests/ui/for_loop.rs @@ -571,3 +571,19 @@ mod issue_2496 { unimplemented!() } } + +mod issue_1219 { + // potential false positive for explicit_counter_loop + pub fn test() { + let thing = 5; + let text = "banana"; + let mut count = 0; + for ch in text.chars() { + if ch == 'a' { + continue; + } + count += 1 + } + println!("{}", count); + } +} From edfa9feac2d86b4c775c12b155b06efe1efe9d5a Mon Sep 17 00:00:00 2001 From: Josh Mcguigan Date: Thu, 6 Sep 2018 06:20:25 -0700 Subject: [PATCH 2/5] Corrected explicit_counter_loop missing lints if variable used after loop --- clippy_lints/src/loops.rs | 10 ++++------ tests/ui/for_loop.rs | 35 ++++++++++++++++++++++++++++++++--- 2 files changed, 36 insertions(+), 9 deletions(-) diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index 8a12530cb..6dfe0a7ec 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -1996,6 +1996,9 @@ impl<'a, 'tcx> Visitor<'tcx> for InitializeVisitor<'a, 'tcx> { if self.state == VarState::DontWarn { return; } + if self.past_loop { + return; + } if SpanlessEq::new(self.cx).eq_expr(&expr, self.end_expr) { self.past_loop = true; return; @@ -2024,12 +2027,7 @@ impl<'a, 'tcx> Visitor<'tcx> for InitializeVisitor<'a, 'tcx> { _ => (), } } - - if self.past_loop { - self.state = VarState::DontWarn; - return; - } - } else if !self.past_loop && is_loop(expr) { + } else if is_loop(expr) { self.state = VarState::DontWarn; return; } else if is_conditional(expr) { diff --git a/tests/ui/for_loop.rs b/tests/ui/for_loop.rs index 76bc96f91..286ef190c 100644 --- a/tests/ui/for_loop.rs +++ b/tests/ui/for_loop.rs @@ -573,16 +573,45 @@ mod issue_2496 { } mod issue_1219 { - // potential false positive for explicit_counter_loop + #[warn(clippy::explicit_counter_loop)] pub fn test() { - let thing = 5; + // should not trigger the lint, because of the continue statement let text = "banana"; let mut count = 0; for ch in text.chars() { if ch == 'a' { continue; } - count += 1 + count += 1; + } + println!("{}", count); + + // should trigger the lint + let text = "banana"; + let mut count = 0; + for ch in text.chars() { + if ch == 'a' { + println!("abc") + } + count += 1; + } + println!("{}", count); + + // should not trigger the lint + let text = "banana"; + let mut count = 0; + for ch in text.chars() { + if ch == 'a' { + count += 1; + } + } + println!("{}", count); + + // should trigger the lint + let text = "banana"; + let mut count = 0; + for _ch in text.chars() { + count += 1; } println!("{}", count); } From ce554267b816692204324b26441cee0f25877072 Mon Sep 17 00:00:00 2001 From: Josh Mcguigan Date: Fri, 7 Sep 2018 05:32:56 -0700 Subject: [PATCH 3/5] Updated explicit_counter_loop tests based on discussion in #3135 --- clippy_lints/src/loops.rs | 10 ++++++---- tests/ui/for_loop.rs | 33 ++++++++++----------------------- 2 files changed, 16 insertions(+), 27 deletions(-) diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index 6dfe0a7ec..8a12530cb 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -1996,9 +1996,6 @@ impl<'a, 'tcx> Visitor<'tcx> for InitializeVisitor<'a, 'tcx> { if self.state == VarState::DontWarn { return; } - if self.past_loop { - return; - } if SpanlessEq::new(self.cx).eq_expr(&expr, self.end_expr) { self.past_loop = true; return; @@ -2027,7 +2024,12 @@ impl<'a, 'tcx> Visitor<'tcx> for InitializeVisitor<'a, 'tcx> { _ => (), } } - } else if is_loop(expr) { + + if self.past_loop { + self.state = VarState::DontWarn; + return; + } + } else if !self.past_loop && is_loop(expr) { self.state = VarState::DontWarn; return; } else if is_conditional(expr) { diff --git a/tests/ui/for_loop.rs b/tests/ui/for_loop.rs index 286ef190c..029b6f7aa 100644 --- a/tests/ui/for_loop.rs +++ b/tests/ui/for_loop.rs @@ -575,7 +575,13 @@ mod issue_2496 { mod issue_1219 { #[warn(clippy::explicit_counter_loop)] pub fn test() { - // should not trigger the lint, because of the continue statement + // should not trigger the lint because variable is used after the loop #473 + let vec = vec![1,2,3]; + let mut index = 0; + for _v in &vec { index += 1 } + println!("index: {}", index); + + // should not trigger the lint because the count is conditional #1219 let text = "banana"; let mut count = 0; for ch in text.chars() { @@ -583,36 +589,17 @@ mod issue_1219 { continue; } count += 1; + println!("{}", count); } - println!("{}", count); - // should trigger the lint - let text = "banana"; - let mut count = 0; - for ch in text.chars() { - if ch == 'a' { - println!("abc") - } - count += 1; - } - println!("{}", count); - - // should not trigger the lint + // should not trigger the lint because the count is conditional let text = "banana"; let mut count = 0; for ch in text.chars() { if ch == 'a' { count += 1; } + println!("{}", count); } - println!("{}", count); - - // should trigger the lint - let text = "banana"; - let mut count = 0; - for _ch in text.chars() { - count += 1; - } - println!("{}", count); } } From 53c262048c4d7cbffd32074f8037430ca803fbff Mon Sep 17 00:00:00 2001 From: Josh Mcguigan Date: Fri, 7 Sep 2018 19:58:19 -0700 Subject: [PATCH 4/5] Fix #1219 false positive for explicit_counter_loop --- clippy_lints/src/loops.rs | 3 +++ tests/ui/for_loop.rs | 11 +++++++++++ tests/ui/for_loop.stderr | 8 +++++++- 3 files changed, 21 insertions(+), 1 deletion(-) diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index 8a12530cb..091b477e3 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -1950,6 +1950,9 @@ impl<'a, 'tcx> Visitor<'tcx> for IncrementVisitor<'a, 'tcx> { walk_expr(self, expr); self.depth -= 1; return; + } else if let ExprKind::Continue(_) = expr.node { + self.done = true; + return; } walk_expr(self, expr); } diff --git a/tests/ui/for_loop.rs b/tests/ui/for_loop.rs index 029b6f7aa..a53b9cd28 100644 --- a/tests/ui/for_loop.rs +++ b/tests/ui/for_loop.rs @@ -601,5 +601,16 @@ mod issue_1219 { } println!("{}", count); } + + // should trigger the lint because the count is not conditional + let text = "banana"; + let mut count = 0; + for ch in text.chars() { + count += 1; + if ch == 'a' { + continue; + } + println!("{}", count); + } } } diff --git a/tests/ui/for_loop.stderr b/tests/ui/for_loop.stderr index bab7bdc77..bbd663dc8 100644 --- a/tests/ui/for_loop.stderr +++ b/tests/ui/for_loop.stderr @@ -487,5 +487,11 @@ error: it looks like you're manually copying between slices 547 | for i in 0..src.len() { | ^^^^^^^^^^^^ help: try replacing the loop by: `dst[..src.len()].clone_from_slice(&src[..])` -error: aborting due to 59 previous errors +error: the variable `count` is used as a loop counter. Consider using `for (count, item) in text.chars().enumerate()` or similar iterators + --> $DIR/for_loop.rs:608:19 + | +608 | for ch in text.chars() { + | ^^^^^^^^^^^^ + +error: aborting due to 60 previous errors From 9168746c385776b4f76216de9a9884f4a6b3265f Mon Sep 17 00:00:00 2001 From: Josh Mcguigan Date: Fri, 7 Sep 2018 20:46:36 -0700 Subject: [PATCH 5/5] Corrected explicit_counter_loop behavior with nested loops --- clippy_lints/src/loops.rs | 3 +-- tests/ui/for_loop.rs | 22 ++++++++++++++++++++++ tests/ui/for_loop.stderr | 8 +++++++- 3 files changed, 30 insertions(+), 3 deletions(-) diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index 091b477e3..6d9519911 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -1942,8 +1942,7 @@ impl<'a, 'tcx> Visitor<'tcx> for IncrementVisitor<'a, 'tcx> { } } } else if is_loop(expr) { - self.states.clear(); - self.done = true; + walk_expr(self, expr); return; } else if is_conditional(expr) { self.depth += 1; diff --git a/tests/ui/for_loop.rs b/tests/ui/for_loop.rs index a53b9cd28..55060b076 100644 --- a/tests/ui/for_loop.rs +++ b/tests/ui/for_loop.rs @@ -612,5 +612,27 @@ mod issue_1219 { } println!("{}", count); } + + // should trigger the lint because the count is not conditional + let text = "banana"; + let mut count = 0; + for ch in text.chars() { + count += 1; + for i in 0..2 { + let _ = 123; + } + println!("{}", count); + } + + // should not trigger the lint because the count is incremented multiple times + let text = "banana"; + let mut count = 0; + for ch in text.chars() { + count += 1; + for i in 0..2 { + count += 1; + } + println!("{}", count); + } } } diff --git a/tests/ui/for_loop.stderr b/tests/ui/for_loop.stderr index bbd663dc8..d82914754 100644 --- a/tests/ui/for_loop.stderr +++ b/tests/ui/for_loop.stderr @@ -493,5 +493,11 @@ error: the variable `count` is used as a loop counter. Consider using `for (coun 608 | for ch in text.chars() { | ^^^^^^^^^^^^ -error: aborting due to 60 previous errors +error: the variable `count` is used as a loop counter. Consider using `for (count, item) in text.chars().enumerate()` or similar iterators + --> $DIR/for_loop.rs:619:19 + | +619 | for ch in text.chars() { + | ^^^^^^^^^^^^ + +error: aborting due to 61 previous errors