Auto merge of #8941 - DevAccentor:for_loops_over_fallibles, r=llogiq

improve [`for_loops_over_fallibles`] to detect the usage of iter, iter_mut and into_iterator

fix #6762

detects code like
```rust
for _ in option.iter() {
    //..
}
```

changelog: Improve [`for_loops_over_fallibles`] to detect `for _ in option.iter() {}` or using `iter_mut()` or `into_iterator()`.

Author: bors

clippy_lints/src/loops/for_loops_over_fallibles.rs

@@ -7,9 +7,22 @@ use rustc_lint::LateContext;
 use rustc_span::symbol::sym;
 
 /// Checks for `for` loops over `Option`s and `Result`s.
-pub(super) fn check(cx: &LateContext<'_>, pat: &Pat<'_>, arg: &Expr<'_>) {
+pub(super) fn check(cx: &LateContext<'_>, pat: &Pat<'_>, arg: &Expr<'_>, method_name: Option<&str>) {
     let ty = cx.typeck_results().expr_ty(arg);
     if is_type_diagnostic_item(cx, ty, sym::Option) {
+        let help_string = if let Some(method_name) = method_name {
+            format!(
+                "consider replacing `for {0} in {1}.{method_name}()` with `if let Some({0}) = {1}`",
+                snippet(cx, pat.span, "_"),
+                snippet(cx, arg.span, "_")
+            )
+        } else {
+            format!(
+                "consider replacing `for {0} in {1}` with `if let Some({0}) = {1}`",
+                snippet(cx, pat.span, "_"),
+                snippet(cx, arg.span, "_")
+            )
+        };
         span_lint_and_help(
             cx,
             FOR_LOOPS_OVER_FALLIBLES,
@@ -20,13 +33,22 @@ pub(super) fn check(cx: &LateContext<'_>, pat: &Pat<'_>, arg: &Expr<'_>) {
                 snippet(cx, arg.span, "_")
             ),
             None,
-            &format!(
-                "consider replacing `for {0} in {1}` with `if let Some({0}) = {1}`",
-                snippet(cx, pat.span, "_"),
-                snippet(cx, arg.span, "_")
-            ),
+            &help_string,
         );
     } else if is_type_diagnostic_item(cx, ty, sym::Result) {
+        let help_string = if let Some(method_name) = method_name {
+            format!(
+                "consider replacing `for {0} in {1}.{method_name}()` with `if let Ok({0}) = {1}`",
+                snippet(cx, pat.span, "_"),
+                snippet(cx, arg.span, "_")
+            )
+        } else {
+            format!(
+                "consider replacing `for {0} in {1}` with `if let Ok({0}) = {1}`",
+                snippet(cx, pat.span, "_"),
+                snippet(cx, arg.span, "_")
+            )
+        };
         span_lint_and_help(
             cx,
             FOR_LOOPS_OVER_FALLIBLES,
@@ -37,11 +59,7 @@ pub(super) fn check(cx: &LateContext<'_>, pat: &Pat<'_>, arg: &Expr<'_>) {
                 snippet(cx, arg.span, "_")
             ),
             None,
-            &format!(
-                "consider replacing `for {0} in {1}` with `if let Ok({0}) = {1}`",
-                snippet(cx, pat.span, "_"),
-                snippet(cx, arg.span, "_")
-            ),
+            &help_string,
         );
     }
 }

clippy_lints/src/loops/mod.rs

@@ -188,6 +188,10 @@ declare_clippy_lint! {
     /// for x in &res {
     ///     // ..
     /// }
+    ///
+    /// for x in res.iter() {
+    ///     // ..
+    /// }
     /// ```
     ///
     /// Use instead:
@@ -695,10 +699,14 @@ fn check_for_loop_arg(cx: &LateContext<'_>, pat: &Pat<'_>, arg: &Expr<'_>) {
         let method_name = method.ident.as_str();
         // check for looping over x.iter() or x.iter_mut(), could use &x or &mut x
         match method_name {
-            "iter" | "iter_mut" => explicit_iter_loop::check(cx, self_arg, arg, method_name),
+            "iter" | "iter_mut" => {
+                explicit_iter_loop::check(cx, self_arg, arg, method_name);
+                for_loops_over_fallibles::check(cx, pat, self_arg, Some(method_name));
+            },
             "into_iter" => {
                 explicit_iter_loop::check(cx, self_arg, arg, method_name);
                 explicit_into_iter_loop::check(cx, self_arg, arg);
+                for_loops_over_fallibles::check(cx, pat, self_arg, Some(method_name));
             },
             "next" => {
                 next_loop_linted = iter_next_loop::check(cx, arg);
@@ -708,6 +716,6 @@ fn check_for_loop_arg(cx: &LateContext<'_>, pat: &Pat<'_>, arg: &Expr<'_>) {
     }
 
     if !next_loop_linted {
-        for_loops_over_fallibles::check(cx, pat, arg);
+        for_loops_over_fallibles::check(cx, pat, arg, None);
     }
 }

tests/ui/for_loops_over_fallibles.rs

@@ -2,19 +2,34 @@
 
 fn for_loops_over_fallibles() {
     let option = Some(1);
-    let result = option.ok_or("x not found");
+    let mut result = option.ok_or("x not found");
     let v = vec![0, 1, 2];
 
     // check over an `Option`
     for x in option {
         println!("{}", x);
     }
 
+    // check over an `Option`
+    for x in option.iter() {
+        println!("{}", x);
+    }
+
     // check over a `Result`
     for x in result {
         println!("{}", x);
     }
 
+    // check over a `Result`
+    for x in result.iter_mut() {
+        println!("{}", x);
+    }
+
+    // check over a `Result`
+    for x in result.into_iter() {
+        println!("{}", x);
+    }
+
     for x in option.ok_or("x not found") {
         println!("{}", x);
     }

tests/ui/for_loops_over_fallibles.stderr

@@ -7,48 +7,72 @@ LL |     for x in option {
    = note: `-D clippy::for-loops-over-fallibles` implied by `-D warnings`
    = help: consider replacing `for x in option` with `if let Some(x) = option`
 
-error: for loop over `result`, which is a `Result`. This is more readably written as an `if let` statement
+error: for loop over `option`, which is an `Option`. This is more readably written as an `if let` statement
   --> $DIR/for_loops_over_fallibles.rs:14:14
    |
+LL |     for x in option.iter() {
+   |              ^^^^^^
+   |
+   = help: consider replacing `for x in option.iter()` with `if let Some(x) = option`
+
+error: for loop over `result`, which is a `Result`. This is more readably written as an `if let` statement
+  --> $DIR/for_loops_over_fallibles.rs:19:14
+   |
 LL |     for x in result {
    |              ^^^^^^
    |
    = help: consider replacing `for x in result` with `if let Ok(x) = result`
 
+error: for loop over `result`, which is a `Result`. This is more readably written as an `if let` statement
+  --> $DIR/for_loops_over_fallibles.rs:24:14
+   |
+LL |     for x in result.iter_mut() {
+   |              ^^^^^^
+   |
+   = help: consider replacing `for x in result.iter_mut()` with `if let Ok(x) = result`
+
+error: for loop over `result`, which is a `Result`. This is more readably written as an `if let` statement
+  --> $DIR/for_loops_over_fallibles.rs:29:14
+   |
+LL |     for x in result.into_iter() {
+   |              ^^^^^^
+   |
+   = help: consider replacing `for x in result.into_iter()` with `if let Ok(x) = result`
+
 error: for loop over `option.ok_or("x not found")`, which is a `Result`. This is more readably written as an `if let` statement
-  --> $DIR/for_loops_over_fallibles.rs:18:14
+  --> $DIR/for_loops_over_fallibles.rs:33:14
    |
 LL |     for x in option.ok_or("x not found") {
    |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = help: consider replacing `for x in option.ok_or("x not found")` with `if let Ok(x) = option.ok_or("x not found")`
 
 error: you are iterating over `Iterator::next()` which is an Option; this will compile but is probably not what you want
-  --> $DIR/for_loops_over_fallibles.rs:24:14
+  --> $DIR/for_loops_over_fallibles.rs:39:14
    |
 LL |     for x in v.iter().next() {
    |              ^^^^^^^^^^^^^^^
    |
    = note: `#[deny(clippy::iter_next_loop)]` on by default
 
 error: for loop over `v.iter().next().and(Some(0))`, which is an `Option`. This is more readably written as an `if let` statement
-  --> $DIR/for_loops_over_fallibles.rs:29:14
+  --> $DIR/for_loops_over_fallibles.rs:44:14
    |
 LL |     for x in v.iter().next().and(Some(0)) {
    |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = help: consider replacing `for x in v.iter().next().and(Some(0))` with `if let Some(x) = v.iter().next().and(Some(0))`
 
 error: for loop over `v.iter().next().ok_or("x not found")`, which is a `Result`. This is more readably written as an `if let` statement
-  --> $DIR/for_loops_over_fallibles.rs:33:14
+  --> $DIR/for_loops_over_fallibles.rs:48:14
    |
 LL |     for x in v.iter().next().ok_or("x not found") {
    |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = help: consider replacing `for x in v.iter().next().ok_or("x not found")` with `if let Ok(x) = v.iter().next().ok_or("x not found")`
 
 error: this loop never actually loops
-  --> $DIR/for_loops_over_fallibles.rs:45:5
+  --> $DIR/for_loops_over_fallibles.rs:60:5
    |
 LL | /     while let Some(x) = option {
 LL | |         println!("{}", x);
@@ -59,13 +83,13 @@ LL | |     }
    = note: `#[deny(clippy::never_loop)]` on by default
 
 error: this loop never actually loops
-  --> $DIR/for_loops_over_fallibles.rs:51:5
+  --> $DIR/for_loops_over_fallibles.rs:66:5
    |
 LL | /     while let Ok(x) = result {
 LL | |         println!("{}", x);
 LL | |         break;
 LL | |     }
    | |_____^
 
-error: aborting due to 8 previous errors
+error: aborting due to 11 previous errors
 

tests/ui/manual_map_option.fixed

@@ -7,6 +7,7 @@
     clippy::unit_arg,
     clippy::match_ref_pats,
     clippy::redundant_pattern_matching,
+    clippy::for_loops_over_fallibles,
     dead_code
 )]
 

tests/ui/manual_map_option.rs

@@ -7,6 +7,7 @@
     clippy::unit_arg,
     clippy::match_ref_pats,
     clippy::redundant_pattern_matching,
+    clippy::for_loops_over_fallibles,
     dead_code
 )]