Add for_loop_over_result lint

README.md

@@ -6,7 +6,7 @@ A collection of lints to catch common mistakes and improve your Rust code.
 [Jump to usage instructions](#usage)
 
 ##Lints
-There are 102 lints included in this crate:
+There are 103 lints included in this crate:
 
 name                                                                                                           | default | meaning
 ---------------------------------------------------------------------------------------------------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
@@ -37,7 +37,8 @@ name
 [extend_from_slice](https://github.com/Manishearth/rust-clippy/wiki#extend_from_slice)                         | warn    | `.extend_from_slice(_)` is a faster way to extend a Vec by a slice
 [filter_next](https://github.com/Manishearth/rust-clippy/wiki#filter_next)                                     | warn    | using `filter(p).next()`, which is more succinctly expressed as `.find(p)`
 [float_cmp](https://github.com/Manishearth/rust-clippy/wiki#float_cmp)                                         | warn    | using `==` or `!=` on float values (as floating-point operations usually involve rounding errors, it is always better to check for approximate equality within small bounds)
-[for_loop_over_option](https://github.com/Manishearth/rust-clippy/wiki#for_loop_over_option)                   | warn    | for-looping over an Option, which is more clear as an `if let`
+[for_loop_over_option](https://github.com/Manishearth/rust-clippy/wiki#for_loop_over_option)                   | warn    | for-looping over an `Option`, which is more clearly expressed as an `if let`
+[for_loop_over_result](https://github.com/Manishearth/rust-clippy/wiki#for_loop_over_result)                   | warn    | for-looping over a `Result`, which is more clearly expressed as an `if let`
 [identity_op](https://github.com/Manishearth/rust-clippy/wiki#identity_op)                                     | warn    | using identity operations, e.g. `x + 0` or `y / 1`
 [ineffective_bit_mask](https://github.com/Manishearth/rust-clippy/wiki#ineffective_bit_mask)                   | warn    | expressions where a bit mask will be rendered useless by a comparison, e.g. `(x | 1) > 2`
 [inline_always](https://github.com/Manishearth/rust-clippy/wiki#inline_always)                                 | warn    | `#[inline(always)]` is a bad idea in most cases

src/lib.rs

@@ -193,6 +193,7 @@ pub fn plugin_registrar(reg: &mut Registry) {
         loops::EXPLICIT_COUNTER_LOOP,
         loops::EXPLICIT_ITER_LOOP,
         loops::FOR_LOOP_OVER_OPTION,
+        loops::FOR_LOOP_OVER_RESULT,
         loops::ITER_NEXT_LOOP,
         loops::NEEDLESS_RANGE_LOOP,
         loops::REVERSE_RANGE_LOOP,

src/loops.rs

@@ -11,7 +11,7 @@ use std::collections::{HashSet, HashMap};
 
 use utils::{snippet, span_lint, get_parent_expr, match_trait_method, match_type, in_external_macro, expr_block,
             span_help_and_lint, is_integer_literal, get_enclosing_block};
-use utils::{HASHMAP_PATH, VEC_PATH, LL_PATH, OPTION_PATH};
+use utils::{HASHMAP_PATH, VEC_PATH, LL_PATH, OPTION_PATH, RESULT_PATH};
 
 /// **What it does:** This lint checks for looping over the range of `0..len` of some collection just to get the values by index. It is `Warn` by default.
 ///
@@ -48,7 +48,7 @@ declare_lint!{ pub EXPLICIT_ITER_LOOP, Warn,
 declare_lint!{ pub ITER_NEXT_LOOP, Warn,
                "for-looping over `_.next()` which is probably not intended" }
 
-/// **What it does:** This lint checks for `for` loops over Option values. It is `Warn` by default.
+/// **What it does:** This lint checks for `for` loops over `Option` values. It is `Warn` by default.
 ///
 /// **Why is this bad?** Readability. This is more clearly expressed as an `if let`.
 ///
@@ -56,7 +56,17 @@ declare_lint!{ pub ITER_NEXT_LOOP, Warn,
 ///
 /// **Example:** `for x in option { .. }`. This should be `if let Some(x) = option { .. }`.
 declare_lint!{ pub FOR_LOOP_OVER_OPTION, Warn,
-               "for-looping over an Option, which is more clear as an `if let`" }
+               "for-looping over an `Option`, which is more clearly expressed as an `if let`" }
+
+/// **What it does:** This lint checks for `for` loops over `Result` values. It is `Warn` by default.
+///
+/// **Why is this bad?** Readability. This is more clearly expressed as an `if let`.
+///
+/// **Known problems:** None
+///
+/// **Example:** `for x in result { .. }`. This should be `if let Ok(x) = result { .. }`.
+declare_lint!{ pub FOR_LOOP_OVER_RESULT, Warn,
+               "for-looping over a `Result`, which is more clearly expressed as an `if let`" }
 
 /// **What it does:** This lint detects `loop + match` combinations that are easier written as a `while let` loop. It is `Warn` by default.
 ///
@@ -417,24 +427,35 @@ fn check_for_loop_arg(cx: &LateContext, pat: &Pat, arg: &Expr, expr: &Expr) {
         }
     }
     if !next_loop_linted {
-        check_option_looping(cx, pat, arg);
+        check_arg_type(cx, pat, arg);
     }
 }
 
-/// Check for `for` loops over `Option`s
+/// Check for `for` loops over `Option`s and `Results`
-fn check_option_looping(cx: &LateContext, pat: &Pat, arg: &Expr) {
+fn check_arg_type(cx: &LateContext, pat: &Pat, arg: &Expr) {
     let ty = cx.tcx.expr_ty(arg);
     if match_type(cx, ty, &OPTION_PATH) {
         span_help_and_lint(
             cx,
             FOR_LOOP_OVER_OPTION,
             arg.span,
-            &format!("for loop over `{0}`, which is an Option. This is more readably written as \
+            &format!("for loop over `{0}`, which is an `Option`. This is more readably written as \
                       an `if let` statement.", snippet(cx, arg.span, "_")),
             &format!("consider replacing `for {0} in {1}` with `if let Some({0}) = {1}`",
                      snippet(cx, pat.span, "_"), snippet(cx, arg.span, "_"))
         );
     }
+    else if match_type(cx, ty, &RESULT_PATH) {
+        span_help_and_lint(
+            cx,
+            FOR_LOOP_OVER_RESULT,
+            arg.span,
+            &format!("for loop over `{0}`, which is a `Result`. This is more readably written as \
+                      an `if let` statement.", snippet(cx, arg.span, "_")),
+            &format!("consider replacing `for {0} in {1}` with `if let Ok({0}) = {1}`",
+                     snippet(cx, pat.span, "_"), snippet(cx, arg.span, "_"))
+        );
+    }
 }
 
 fn check_for_loop_explicit_counter(cx: &LateContext, arg: &Expr, body: &Expr, expr: &Expr) {

tests/compile-fail/for_loop.rs

@@ -4,21 +4,51 @@
 use std::collections::*;
 
 #[deny(clippy)]
-fn for_loop_over_option() {
+fn for_loop_over_option_and_result() {
     let option = Some(1);
+    let result = option.ok_or("x not found");
     let v = vec![0,1,2];
 
     // check FOR_LOOP_OVER_OPTION lint
+
     for x in option {
-        //~^ ERROR for loop over `option`, which is an Option.
+        //~^ ERROR for loop over `option`, which is an `Option`.
         //~| HELP consider replacing `for x in option` with `if let Some(x) = option`
         println!("{}", x);
     }
 
-    // make sure LOOP_OVER_NEXT lint takes precedence
+    // check FOR_LOOP_OVER_RESULT lint
+
+    for x in result {
+        //~^ ERROR for loop over `result`, which is a `Result`.
+        //~| HELP consider replacing `for x in result` with `if let Ok(x) = result`
+        println!("{}", x);
+    }
+
+    for x in option.ok_or("x not found") {
+        //~^ ERROR for loop over `option.ok_or("x not found")`, which is a `Result`.
+        //~| HELP consider replacing `for x in option.ok_or("x not found")` with `if let Ok(x) = option.ok_or("x not found")`
+        println!("{}", x);
+    }
+
+    // make sure LOOP_OVER_NEXT lint takes precedence when next() is the last call in the chain
+
     for x in v.iter().next() {
         //~^ ERROR you are iterating over `Iterator::next()` which is an Option
-        // TODO: make sure we don't lint twice
+        println!("{}", x);
+    }
+
+    // make sure we lint when next() is not the last call in the chain
+
+    for x in v.iter().next().and(Some(0)) {
+        //~^ ERROR for loop over `v.iter().next().and(Some(0))`, which is an `Option`
+        //~| HELP consider replacing `for x in v.iter().next().and(Some(0))` with `if let Some(x) = v.iter().next().and(Some(0))`
+        println!("{}", x);
+    }
+
+    for x in v.iter().next().ok_or("x not found") {
+        //~^ ERROR for loop over `v.iter().next().ok_or("x not found")`, which is a `Result`
+        //~| 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")`
         println!("{}", x);
     }
 
@@ -29,11 +59,17 @@ fn for_loop_over_option() {
         println!("{}", x);
     }
 
-    // while let false positive
+    // while let false positive for Option
     while let Some(x) = option {
         println!("{}", x);
         break;
     }
+
+    // while let false positive for Option
+    while let Ok(x) = result {
+        println!("{}", x);
+        break;
+    }
 }
 
 struct Unrelated(Vec<u8>);
@@ -243,5 +279,5 @@ fn main() {
     for _v in &vec { index += 1 }
     println!("index: {}", index);
 
-    for_loop_over_option();
+    for_loop_over_option_and_result();
 }