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

clippy_lints/src/loops/for_loops_over_fallibles.rs

@@ -7,9 +7,22 @@
 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!(
+            &help_string,
-                "consider replacing `for {0} in {1}` with `if let Some({0}) = {1}`",
-                snippet(cx, pat.span, "_"),
-                snippet(cx, arg.span, "_")
-            ),
         );
     } 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!(
+            &help_string,
-                "consider replacing `for {0} in {1}` with `if let Ok({0}) = {1}`",
-                snippet(cx, pat.span, "_"),
-                snippet(cx, arg.span, "_")
-            ),
         );
     }
 }

clippy_lints/src/loops/mod.rs

@@ -188,6 +188,10 @@
     /// 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,7 +2,7 @@
 
 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`
@@ -10,11 +10,26 @@ fn for_loops_over_fallibles() {
         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.into_iter() {
+        println!("{}", x);
+    }
+
+    // check over a `Result`
+    for x in result.iter_mut() {
+        println!("{}", x);
+    }
+
     for x in option.ok_or("x not found") {
         println!("{}", x);
     }

tests/ui/for_loops_over_fallibles.stderr

@@ -7,16 +7,40 @@ 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.into_iter() {
+   |              ^^^^^^
+   |
+   = help: consider replacing `for x in result.into_iter()` 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.iter_mut() {
+   |              ^^^^^^
+   |
+   = help: consider replacing `for x in result.iter_mut()` 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") {
    |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^
@@ -24,7 +48,7 @@ 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() {
    |              ^^^^^^^^^^^^^^^
@@ -32,7 +56,7 @@ 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)) {
    |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
@@ -40,7 +64,7 @@ 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") {
    |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
@@ -48,7 +72,7 @@ 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,7 +83,7 @@ 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);
@@ -67,5 +91,5 @@ 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
 )]
 

tests/ui/manual_map_option.stderr

@@ -1,5 +1,5 @@
 error: manual implementation of `Option::map`
-  --> $DIR/manual_map_option.rs:14:5
+  --> $DIR/manual_map_option.rs:15:5
    |
 LL | /     match Some(0) {
 LL | |         Some(_) => Some(2),
@@ -10,7 +10,7 @@ LL | |     };
    = note: `-D clippy::manual-map` implied by `-D warnings`
 
 error: manual implementation of `Option::map`
-  --> $DIR/manual_map_option.rs:19:5
+  --> $DIR/manual_map_option.rs:20:5
    |
 LL | /     match Some(0) {
 LL | |         Some(x) => Some(x + 1),
@@ -19,7 +19,7 @@ LL | |     };
    | |_____^ help: try this: `Some(0).map(|x| x + 1)`
 
 error: manual implementation of `Option::map`
-  --> $DIR/manual_map_option.rs:24:5
+  --> $DIR/manual_map_option.rs:25:5
    |
 LL | /     match Some("") {
 LL | |         Some(x) => Some(x.is_empty()),
@@ -28,7 +28,7 @@ LL | |     };
    | |_____^ help: try this: `Some("").map(|x| x.is_empty())`
 
 error: manual implementation of `Option::map`
-  --> $DIR/manual_map_option.rs:29:5
+  --> $DIR/manual_map_option.rs:30:5
    |
 LL | /     if let Some(x) = Some(0) {
 LL | |         Some(!x)
@@ -38,7 +38,7 @@ LL | |     };
    | |_____^ help: try this: `Some(0).map(|x| !x)`
 
 error: manual implementation of `Option::map`
-  --> $DIR/manual_map_option.rs:36:5
+  --> $DIR/manual_map_option.rs:37:5
    |
 LL | /     match Some(0) {
 LL | |         Some(x) => { Some(std::convert::identity(x)) }
@@ -47,7 +47,7 @@ LL | |     };
    | |_____^ help: try this: `Some(0).map(std::convert::identity)`
 
 error: manual implementation of `Option::map`
-  --> $DIR/manual_map_option.rs:41:5
+  --> $DIR/manual_map_option.rs:42:5
    |
 LL | /     match Some(&String::new()) {
 LL | |         Some(x) => Some(str::len(x)),
@@ -56,7 +56,7 @@ LL | |     };
    | |_____^ help: try this: `Some(&String::new()).map(|x| str::len(x))`
 
 error: manual implementation of `Option::map`
-  --> $DIR/manual_map_option.rs:51:5
+  --> $DIR/manual_map_option.rs:52:5
    |
 LL | /     match &Some([0, 1]) {
 LL | |         Some(x) => Some(x[0]),
@@ -65,7 +65,7 @@ LL | |     };
    | |_____^ help: try this: `Some([0, 1]).as_ref().map(|x| x[0])`
 
 error: manual implementation of `Option::map`
-  --> $DIR/manual_map_option.rs:56:5
+  --> $DIR/manual_map_option.rs:57:5
    |
 LL | /     match &Some(0) {
 LL | |         &Some(x) => Some(x * 2),
@@ -74,7 +74,7 @@ LL | |     };
    | |_____^ help: try this: `Some(0).map(|x| x * 2)`
 
 error: manual implementation of `Option::map`
-  --> $DIR/manual_map_option.rs:61:5
+  --> $DIR/manual_map_option.rs:62:5
    |
 LL | /     match Some(String::new()) {
 LL | |         Some(ref x) => Some(x.is_empty()),
@@ -83,7 +83,7 @@ LL | |     };
    | |_____^ help: try this: `Some(String::new()).as_ref().map(|x| x.is_empty())`
 
 error: manual implementation of `Option::map`
-  --> $DIR/manual_map_option.rs:66:5
+  --> $DIR/manual_map_option.rs:67:5
    |
 LL | /     match &&Some(String::new()) {
 LL | |         Some(x) => Some(x.len()),
@@ -92,7 +92,7 @@ LL | |     };
    | |_____^ help: try this: `Some(String::new()).as_ref().map(|x| x.len())`
 
 error: manual implementation of `Option::map`
-  --> $DIR/manual_map_option.rs:71:5
+  --> $DIR/manual_map_option.rs:72:5
    |
 LL | /     match &&Some(0) {
 LL | |         &&Some(x) => Some(x + x),
@@ -101,7 +101,7 @@ LL | |     };
    | |_____^ help: try this: `Some(0).map(|x| x + x)`
 
 error: manual implementation of `Option::map`
-  --> $DIR/manual_map_option.rs:84:9
+  --> $DIR/manual_map_option.rs:85:9
    |
 LL | /         match &mut Some(String::new()) {
 LL | |             Some(x) => Some(x.push_str("")),
@@ -110,7 +110,7 @@ LL | |         };
    | |_________^ help: try this: `Some(String::new()).as_mut().map(|x| x.push_str(""))`
 
 error: manual implementation of `Option::map`
-  --> $DIR/manual_map_option.rs:90:5
+  --> $DIR/manual_map_option.rs:91:5
    |
 LL | /     match &mut Some(String::new()) {
 LL | |         Some(ref x) => Some(x.len()),
@@ -119,7 +119,7 @@ LL | |     };
    | |_____^ help: try this: `Some(String::new()).as_ref().map(|x| x.len())`
 
 error: manual implementation of `Option::map`
-  --> $DIR/manual_map_option.rs:95:5
+  --> $DIR/manual_map_option.rs:96:5
    |
 LL | /     match &mut &Some(String::new()) {
 LL | |         Some(x) => Some(x.is_empty()),
@@ -128,7 +128,7 @@ LL | |     };
    | |_____^ help: try this: `Some(String::new()).as_ref().map(|x| x.is_empty())`
 
 error: manual implementation of `Option::map`
-  --> $DIR/manual_map_option.rs:100:5
+  --> $DIR/manual_map_option.rs:101:5
    |
 LL | /     match Some((0, 1, 2)) {
 LL | |         Some((x, y, z)) => Some(x + y + z),
@@ -137,7 +137,7 @@ LL | |     };
    | |_____^ help: try this: `Some((0, 1, 2)).map(|(x, y, z)| x + y + z)`
 
 error: manual implementation of `Option::map`
-  --> $DIR/manual_map_option.rs:105:5
+  --> $DIR/manual_map_option.rs:106:5
    |
 LL | /     match Some([1, 2, 3]) {
 LL | |         Some([first, ..]) => Some(first),
@@ -146,7 +146,7 @@ LL | |     };
    | |_____^ help: try this: `Some([1, 2, 3]).map(|[first, ..]| first)`
 
 error: manual implementation of `Option::map`
-  --> $DIR/manual_map_option.rs:110:5
+  --> $DIR/manual_map_option.rs:111:5
    |
 LL | /     match &Some((String::new(), "test")) {
 LL | |         Some((x, y)) => Some((y, x)),
@@ -155,7 +155,7 @@ LL | |     };
    | |_____^ help: try this: `Some((String::new(), "test")).as_ref().map(|(x, y)| (y, x))`
 
 error: manual implementation of `Option::map`
-  --> $DIR/manual_map_option.rs:168:5
+  --> $DIR/manual_map_option.rs:169:5
    |
 LL | /     match Some(0) {
 LL | |         Some(x) => Some(vec![x]),
@@ -164,7 +164,7 @@ LL | |     };
    | |_____^ help: try this: `Some(0).map(|x| vec![x])`
 
 error: manual implementation of `Option::map`
-  --> $DIR/manual_map_option.rs:173:5
+  --> $DIR/manual_map_option.rs:174:5
    |
 LL | /     match option_env!("") {
 LL | |         Some(x) => Some(String::from(x)),
@@ -173,7 +173,7 @@ LL | |     };
    | |_____^ help: try this: `option_env!("").map(String::from)`
 
 error: manual implementation of `Option::map`
-  --> $DIR/manual_map_option.rs:193:12
+  --> $DIR/manual_map_option.rs:194:12
    |
 LL |       } else if let Some(x) = Some(0) {
    |  ____________^
@@ -184,7 +184,7 @@ LL | |     };
    | |_____^ help: try this: `{ Some(0).map(|x| x + 1) }`
 
 error: manual implementation of `Option::map`
-  --> $DIR/manual_map_option.rs:201:12
+  --> $DIR/manual_map_option.rs:202:12
    |
 LL |       } else if let Some(x) = Some(0) {
    |  ____________^