Auto merge of #8992 - kyoto7250:fix_8753, r=flip1995

feat(fix): Do not lint if the target code is inside a loop

close #8753

we consider the following code.

```rust
fn main() {
    let vec = vec![1];
    let w: Vec<usize> = vec.iter().map(|i| i * i).collect();  // <- once.

    for i in 0..2 {
        let _ = w.contains(&i);
    }
}
```

and the clippy will issue the following warning.

```rust
warning: avoid using `collect()` when not needed
 --> src/main.rs:3:51
  |
3 |     let w: Vec<usize> = vec.iter().map(|i| i * i).collect();
  |                                                   ^^^^^^^
...
6 |         let _ = w.contains(&i);
  |                 -------------- the iterator could be used here instead
  |
  = note: `#[warn(clippy::needless_collect)]` on by default
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_collect
help: check if the original Iterator contains an element instead of collecting then checking
  |
3 ~
4 |
5 |     for i in 0..2 {
6 ~         let _ = vec.iter().map(|i| i * i).any(|x| x == i);
```

Rewrite the code as indicated.

```rust
fn main() {
    let vec = vec![1];

    for i in 0..2 {
        let _ = vec.iter().map(|i| i * i).any(|x| x == i);  // <- execute `map` every loop.
    }
}
```

this code is valid in the compiler, but, it is different from the code before the rewrite.
So, we should not lint, If `collect` is outside of a loop.

Thank you in advance.

---

changelog: Do not lint if the target code is inside a loop in `needless_collect`

Author: bors

clippy_lints/src/loops/needless_collect.rs

@@ -1,5 +1,6 @@
 use super::NEEDLESS_COLLECT;
 use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_hir_and_then};
+use clippy_utils::higher;
 use clippy_utils::source::{snippet, snippet_with_applicability};
 use clippy_utils::sugg::Sugg;
 use clippy_utils::ty::is_type_diagnostic_item;
@@ -184,10 +185,19 @@ struct IterFunctionVisitor<'a, 'tcx> {
 impl<'tcx> Visitor<'tcx> for IterFunctionVisitor<'_, 'tcx> {
     fn visit_block(&mut self, block: &'tcx Block<'tcx>) {
         for (expr, hir_id) in block.stmts.iter().filter_map(get_expr_and_hir_id_from_stmt) {
+            if check_loop_kind(expr).is_some() {
+                continue;
+            }
             self.visit_block_expr(expr, hir_id);
         }
         if let Some(expr) = block.expr {
-            self.visit_block_expr(expr, None);
+            if let Some(loop_kind) = check_loop_kind(expr) {
+                if let LoopKind::Conditional(block_expr) = loop_kind {
+                    self.visit_block_expr(block_expr, None);
+                }
+            } else {
+                self.visit_block_expr(expr, None);
+            }
         }
     }
 
@@ -264,6 +274,28 @@ impl<'tcx> Visitor<'tcx> for IterFunctionVisitor<'_, 'tcx> {
     }
 }
 
+enum LoopKind<'tcx> {
+    Conditional(&'tcx Expr<'tcx>),
+    Loop,
+}
+
+fn check_loop_kind<'tcx>(expr: &Expr<'tcx>) -> Option<LoopKind<'tcx>> {
+    if let Some(higher::WhileLet { let_expr, .. }) = higher::WhileLet::hir(expr) {
+        return Some(LoopKind::Conditional(let_expr));
+    }
+    if let Some(higher::While { condition, .. }) = higher::While::hir(expr) {
+        return Some(LoopKind::Conditional(condition));
+    }
+    if let Some(higher::ForLoop { arg, .. }) = higher::ForLoop::hir(expr) {
+        return Some(LoopKind::Conditional(arg));
+    }
+    if let ExprKind::Loop { .. } = expr.kind {
+        return Some(LoopKind::Loop);
+    }
+
+    None
+}
+
 impl<'tcx> IterFunctionVisitor<'_, 'tcx> {
     fn visit_block_expr(&mut self, expr: &'tcx Expr<'tcx>, hir_id: Option<HirId>) {
         self.current_statement_hir_id = hir_id;

tests/compile-test.rs

@@ -413,19 +413,21 @@ fn check_rustfix_coverage() {
     if let Ok(missing_coverage_contents) = std::fs::read_to_string(missing_coverage_path) {
         assert!(RUSTFIX_COVERAGE_KNOWN_EXCEPTIONS.iter().is_sorted_by_key(Path::new));
 
-        for rs_path in missing_coverage_contents.lines() {
-            if Path::new(rs_path).starts_with("tests/ui/crashes") {
+        for rs_file in missing_coverage_contents.lines() {
+            let rs_path = Path::new(rs_file);
+            if rs_path.starts_with("tests/ui/crashes") {
                 continue;
             }
-            let filename = Path::new(rs_path).strip_prefix("tests/ui/").unwrap();
+            assert!(rs_path.starts_with("tests/ui/"), "{:?}", rs_file);
+            let filename = rs_path.strip_prefix("tests/ui/").unwrap();
             assert!(
                 RUSTFIX_COVERAGE_KNOWN_EXCEPTIONS
                     .binary_search_by_key(&filename, Path::new)
                     .is_ok(),
                 "`{}` runs `MachineApplicable` diagnostics but is missing a `run-rustfix` annotation. \
                 Please either add `// run-rustfix` at the top of the file or add the file to \
                 `RUSTFIX_COVERAGE_KNOWN_EXCEPTIONS` in `tests/compile-test.rs`.",
-                rs_path,
+                rs_file,
             );
         }
     }

tests/ui/needless_collect_indirect.rs

@@ -112,3 +112,192 @@ fn allow_test() {
     let v = [1].iter().collect::<Vec<_>>();
     v.into_iter().collect::<HashSet<_>>();
 }
+
+mod issue_8553 {
+    fn test_for() {
+        let vec = vec![1, 2];
+        let w: Vec<usize> = vec.iter().map(|i| i * i).collect();
+
+        for i in 0..2 {
+            // Do not lint, because this method call is in the loop
+            w.contains(&i);
+        }
+
+        for i in 0..2 {
+            let y: Vec<usize> = vec.iter().map(|k| k * k).collect();
+            let z: Vec<usize> = vec.iter().map(|k| k * k).collect();
+            // Do lint
+            y.contains(&i);
+            for j in 0..2 {
+                // Do not lint, because this method call is in the loop
+                z.contains(&j);
+            }
+        }
+
+        // Do not lint, because this variable is used.
+        w.contains(&0);
+    }
+
+    fn test_while() {
+        let vec = vec![1, 2];
+        let x: Vec<usize> = vec.iter().map(|i| i * i).collect();
+        let mut n = 0;
+        while n > 1 {
+            // Do not lint, because this method call is in the loop
+            x.contains(&n);
+            n += 1;
+        }
+
+        while n > 2 {
+            let y: Vec<usize> = vec.iter().map(|k| k * k).collect();
+            let z: Vec<usize> = vec.iter().map(|k| k * k).collect();
+            // Do lint
+            y.contains(&n);
+            n += 1;
+            while n > 4 {
+                // Do not lint, because this method call is in the loop
+                z.contains(&n);
+                n += 1;
+            }
+        }
+    }
+
+    fn test_loop() {
+        let vec = vec![1, 2];
+        let x: Vec<usize> = vec.iter().map(|i| i * i).collect();
+        let mut n = 0;
+        loop {
+            if n < 1 {
+                // Do not lint, because this method call is in the loop
+                x.contains(&n);
+                n += 1;
+            } else {
+                break;
+            }
+        }
+
+        loop {
+            if n < 2 {
+                let y: Vec<usize> = vec.iter().map(|k| k * k).collect();
+                let z: Vec<usize> = vec.iter().map(|k| k * k).collect();
+                // Do lint
+                y.contains(&n);
+                n += 1;
+                loop {
+                    if n < 4 {
+                        // Do not lint, because this method call is in the loop
+                        z.contains(&n);
+                        n += 1;
+                    } else {
+                        break;
+                    }
+                }
+            } else {
+                break;
+            }
+        }
+    }
+
+    fn test_while_let() {
+        let vec = vec![1, 2];
+        let x: Vec<usize> = vec.iter().map(|i| i * i).collect();
+        let optional = Some(0);
+        let mut n = 0;
+        while let Some(value) = optional {
+            if n < 1 {
+                // Do not lint, because this method call is in the loop
+                x.contains(&n);
+                n += 1;
+            } else {
+                break;
+            }
+        }
+
+        while let Some(value) = optional {
+            let y: Vec<usize> = vec.iter().map(|k| k * k).collect();
+            let z: Vec<usize> = vec.iter().map(|k| k * k).collect();
+            if n < 2 {
+                // Do lint
+                y.contains(&n);
+                n += 1;
+            } else {
+                break;
+            }
+
+            while let Some(value) = optional {
+                if n < 4 {
+                    // Do not lint, because this method call is in the loop
+                    z.contains(&n);
+                    n += 1;
+                } else {
+                    break;
+                }
+            }
+        }
+    }
+
+    fn test_if_cond() {
+        let vec = vec![1, 2];
+        let v: Vec<usize> = vec.iter().map(|i| i * i).collect();
+        let w = v.iter().collect::<Vec<_>>();
+        // Do lint
+        for _ in 0..w.len() {
+            todo!();
+        }
+    }
+
+    fn test_if_cond_false_case() {
+        let vec = vec![1, 2];
+        let v: Vec<usize> = vec.iter().map(|i| i * i).collect();
+        let w = v.iter().collect::<Vec<_>>();
+        // Do not lint, because w is used.
+        for _ in 0..w.len() {
+            todo!();
+        }
+
+        w.len();
+    }
+
+    fn test_while_cond() {
+        let mut vec = vec![1, 2];
+        let mut v: Vec<usize> = vec.iter().map(|i| i * i).collect();
+        let mut w = v.iter().collect::<Vec<_>>();
+        // Do lint
+        while 1 == w.len() {
+            todo!();
+        }
+    }
+
+    fn test_while_cond_false_case() {
+        let mut vec = vec![1, 2];
+        let mut v: Vec<usize> = vec.iter().map(|i| i * i).collect();
+        let mut w = v.iter().collect::<Vec<_>>();
+        // Do not lint, because w is used.
+        while 1 == w.len() {
+            todo!();
+        }
+
+        w.len();
+    }
+
+    fn test_while_let_cond() {
+        let mut vec = vec![1, 2];
+        let mut v: Vec<usize> = vec.iter().map(|i| i * i).collect();
+        let mut w = v.iter().collect::<Vec<_>>();
+        // Do lint
+        while let Some(i) = Some(w.len()) {
+            todo!();
+        }
+    }
+
+    fn test_while_let_cond_false_case() {
+        let mut vec = vec![1, 2];
+        let mut v: Vec<usize> = vec.iter().map(|i| i * i).collect();
+        let mut w = v.iter().collect::<Vec<_>>();
+        // Do not lint, because w is used.
+        while let Some(i) = Some(w.len()) {
+            todo!();
+        }
+        w.len();
+    }
+}

tests/ui/needless_collect_indirect.stderr

@@ -125,5 +125,122 @@ LL ~
 LL ~         sample.iter().count()
    |
 
-error: aborting due to 9 previous errors
+error: avoid using `collect()` when not needed
+  --> $DIR/needless_collect_indirect.rs:127:59
+   |
+LL |             let y: Vec<usize> = vec.iter().map(|k| k * k).collect();
+   |                                                           ^^^^^^^
+...
+LL |             y.contains(&i);
+   |             -------------- the iterator could be used here instead
+   |
+help: check if the original Iterator contains an element instead of collecting then checking
+   |
+LL ~             
+LL |             let z: Vec<usize> = vec.iter().map(|k| k * k).collect();
+LL |             // Do lint
+LL ~             vec.iter().map(|k| k * k).any(|x| x == i);
+   |
+
+error: avoid using `collect()` when not needed
+  --> $DIR/needless_collect_indirect.rs:152:59
+   |
+LL |             let y: Vec<usize> = vec.iter().map(|k| k * k).collect();
+   |                                                           ^^^^^^^
+...
+LL |             y.contains(&n);
+   |             -------------- the iterator could be used here instead
+   |
+help: check if the original Iterator contains an element instead of collecting then checking
+   |
+LL ~             
+LL |             let z: Vec<usize> = vec.iter().map(|k| k * k).collect();
+LL |             // Do lint
+LL ~             vec.iter().map(|k| k * k).any(|x| x == n);
+   |
+
+error: avoid using `collect()` when not needed
+  --> $DIR/needless_collect_indirect.rs:181:63
+   |
+LL |                 let y: Vec<usize> = vec.iter().map(|k| k * k).collect();
+   |                                                               ^^^^^^^
+...
+LL |                 y.contains(&n);
+   |                 -------------- the iterator could be used here instead
+   |
+help: check if the original Iterator contains an element instead of collecting then checking
+   |
+LL ~                 
+LL |                 let z: Vec<usize> = vec.iter().map(|k| k * k).collect();
+LL |                 // Do lint
+LL ~                 vec.iter().map(|k| k * k).any(|x| x == n);
+   |
+
+error: avoid using `collect()` when not needed
+  --> $DIR/needless_collect_indirect.rs:217:59
+   |
+LL |             let y: Vec<usize> = vec.iter().map(|k| k * k).collect();
+   |                                                           ^^^^^^^
+...
+LL |                 y.contains(&n);
+   |                 -------------- the iterator could be used here instead
+   |
+help: check if the original Iterator contains an element instead of collecting then checking
+   |
+LL ~             
+LL |             let z: Vec<usize> = vec.iter().map(|k| k * k).collect();
+LL |             if n < 2 {
+LL |                 // Do lint
+LL ~                 vec.iter().map(|k| k * k).any(|x| x == n);
+   |
+
+error: avoid using `collect()` when not needed
+  --> $DIR/needless_collect_indirect.rs:242:26
+   |
+LL |         let w = v.iter().collect::<Vec<_>>();
+   |                          ^^^^^^^
+LL |         // Do lint
+LL |         for _ in 0..w.len() {
+   |                     ------- the iterator could be used here instead
+   |
+help: take the original Iterator's count instead of collecting it and finding the length
+   |
+LL ~         
+LL |         // Do lint
+LL ~         for _ in 0..v.iter().count() {
+   |
+
+error: avoid using `collect()` when not needed
+  --> $DIR/needless_collect_indirect.rs:264:30
+   |
+LL |         let mut w = v.iter().collect::<Vec<_>>();
+   |                              ^^^^^^^
+LL |         // Do lint
+LL |         while 1 == w.len() {
+   |                    ------- the iterator could be used here instead
+   |
+help: take the original Iterator's count instead of collecting it and finding the length
+   |
+LL ~         
+LL |         // Do lint
+LL ~         while 1 == v.iter().count() {
+   |
+
+error: avoid using `collect()` when not needed
+  --> $DIR/needless_collect_indirect.rs:286:30
+   |
+LL |         let mut w = v.iter().collect::<Vec<_>>();
+   |                              ^^^^^^^
+LL |         // Do lint
+LL |         while let Some(i) = Some(w.len()) {
+   |                                  ------- the iterator could be used here instead
+   |
+help: take the original Iterator's count instead of collecting it and finding the length
+   |
+LL ~         
+LL |         // Do lint
+LL ~         while let Some(i) = Some(v.iter().count()) {
+   |
+
+error: aborting due to 16 previous errors