Merge for_loop_over_option and for_loop_over_result lints into for_loop_over_fallible lint

CHANGELOG.md

@@ -1361,8 +1361,7 @@ Released 2018-09-13
 [`fn_to_numeric_cast`]: https://rust-lang.github.io/rust-clippy/master/index.html#fn_to_numeric_cast
 [`fn_to_numeric_cast_with_truncation`]: https://rust-lang.github.io/rust-clippy/master/index.html#fn_to_numeric_cast_with_truncation
 [`for_kv_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#for_kv_map
-[`for_loop_over_option`]: https://rust-lang.github.io/rust-clippy/master/index.html#for_loop_over_option
+[`for_loop_over_fallible`]: https://rust-lang.github.io/rust-clippy/master/index.html#for_loop_over_fallible
-[`for_loop_over_result`]: https://rust-lang.github.io/rust-clippy/master/index.html#for_loop_over_result
 [`forget_copy`]: https://rust-lang.github.io/rust-clippy/master/index.html#forget_copy
 [`forget_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#forget_ref
 [`future_not_send`]: https://rust-lang.github.io/rust-clippy/master/index.html#future_not_send

clippy_lints/src/lib.rs

@@ -615,8 +615,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         &loops::EXPLICIT_INTO_ITER_LOOP,
         &loops::EXPLICIT_ITER_LOOP,
         &loops::FOR_KV_MAP,
-        &loops::FOR_LOOP_OVER_OPTION,
+        &loops::FOR_LOOP_OVER_FALLIBLE,
-        &loops::FOR_LOOP_OVER_RESULT,
         &loops::ITER_NEXT_LOOP,
         &loops::MANUAL_MEMCPY,
         &loops::MUT_RANGE_BOUND,
@@ -1265,8 +1264,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&loops::EMPTY_LOOP),
         LintId::of(&loops::EXPLICIT_COUNTER_LOOP),
         LintId::of(&loops::FOR_KV_MAP),
-        LintId::of(&loops::FOR_LOOP_OVER_OPTION),
+        LintId::of(&loops::FOR_LOOP_OVER_FALLIBLE),
-        LintId::of(&loops::FOR_LOOP_OVER_RESULT),
         LintId::of(&loops::ITER_NEXT_LOOP),
         LintId::of(&loops::MANUAL_MEMCPY),
         LintId::of(&loops::MUT_RANGE_BOUND),
@@ -1641,8 +1639,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&inline_fn_without_body::INLINE_FN_WITHOUT_BODY),
         LintId::of(&let_underscore::LET_UNDERSCORE_LOCK),
         LintId::of(&literal_representation::MISTYPED_LITERAL_SUFFIXES),
-        LintId::of(&loops::FOR_LOOP_OVER_OPTION),
+        LintId::of(&loops::FOR_LOOP_OVER_FALLIBLE),
-        LintId::of(&loops::FOR_LOOP_OVER_RESULT),
         LintId::of(&loops::ITER_NEXT_LOOP),
         LintId::of(&loops::NEVER_LOOP),
         LintId::of(&loops::WHILE_IMMUTABLE_CONDITION),

clippy_lints/src/loops.rs

@@ -168,7 +168,7 @@ declare_clippy_lint! {
 }
 
 declare_clippy_lint! {
-    /// **What it does:** Checks for `for` loops over `Option` values.
+    /// **What it does:** Checks for `for` loops over `Option` or `Result` values.
     ///
     /// **Why is this bad?** Readability. This is more clearly expressed as an `if
     /// let`.
@@ -176,47 +176,38 @@ declare_clippy_lint! {
     /// **Known problems:** None.
     ///
     /// **Example:**
-    /// ```ignore
+    /// ```rust
-    /// for x in option {
+    /// # let opt = Some(1);
-    ///     ..
+    ///
+    /// // Bad
+    /// for x in opt {
+    ///     // ..
+    /// }
+    ///
+    /// // Good
+    /// if let Some(x) = opt {
+    ///     // ..
     /// }
     /// ```
     ///
-    /// This should be
+    /// // or
-    /// ```ignore
+    ///
-    /// if let Some(x) = option {
+    /// ```rust
-    ///     ..
+    /// # let res: Result<i32, std::io::Error> = Ok(1);
+    ///
+    /// // Bad
+    /// for x in &res {
+    ///     // ..
+    /// }
+    ///
+    /// // Good
+    /// if let Ok(x) = res {
+    ///     // ..
     /// }
     /// ```
-    pub FOR_LOOP_OVER_OPTION,
+    pub FOR_LOOP_OVER_FALLIBLE,
     correctness,
-    "for-looping over an `Option`, which is more clearly expressed as an `if let`"
+    "for-looping over an `Option` or a `Result`, which is more clearly expressed as an `if let`"
-}
-
-declare_clippy_lint! {
-    /// **What it does:** Checks for `for` loops over `Result` values.
-    ///
-    /// **Why is this bad?** Readability. This is more clearly expressed as an `if
-    /// let`.
-    ///
-    /// **Known problems:** None.
-    ///
-    /// **Example:**
-    /// ```ignore
-    /// for x in result {
-    ///     ..
-    /// }
-    /// ```
-    ///
-    /// This should be
-    /// ```ignore
-    /// if let Ok(x) = result {
-    ///     ..
-    /// }
-    /// ```
-    pub FOR_LOOP_OVER_RESULT,
-    correctness,
-    "for-looping over a `Result`, which is more clearly expressed as an `if let`"
 }
 
 declare_clippy_lint! {
@@ -435,8 +426,7 @@ declare_lint_pass!(Loops => [
     EXPLICIT_ITER_LOOP,
     EXPLICIT_INTO_ITER_LOOP,
     ITER_NEXT_LOOP,
-    FOR_LOOP_OVER_RESULT,
+    FOR_LOOP_OVER_FALLIBLE,
-    FOR_LOOP_OVER_OPTION,
     WHILE_LET_LOOP,
     NEEDLESS_COLLECT,
     EXPLICIT_COUNTER_LOOP,
@@ -1283,7 +1273,7 @@ fn check_for_loop_arg(cx: &LateContext<'_, '_>, pat: &Pat<'_>, arg: &Expr<'_>, e
                     ITER_NEXT_LOOP,
                     expr.span,
                     "you are iterating over `Iterator::next()` which is an Option; this will compile but is \
-                     probably not what you want",
+                    probably not what you want",
                 );
                 next_loop_linted = true;
             }
@@ -1300,11 +1290,11 @@ fn check_arg_type(cx: &LateContext<'_, '_>, pat: &Pat<'_>, arg: &Expr<'_>) {
     if is_type_diagnostic_item(cx, ty, sym!(option_type)) {
         span_lint_and_help(
             cx,
-            FOR_LOOP_OVER_OPTION,
+            FOR_LOOP_OVER_FALLIBLE,
             arg.span,
             &format!(
                 "for loop over `{0}`, which is an `Option`. This is more readably written as an \
-                 `if let` statement.",
+                `if let` statement.",
                 snippet(cx, arg.span, "_")
             ),
             None,
@@ -1317,11 +1307,11 @@ fn check_arg_type(cx: &LateContext<'_, '_>, pat: &Pat<'_>, arg: &Expr<'_>) {
     } else if is_type_diagnostic_item(cx, ty, sym!(result_type)) {
         span_lint_and_help(
             cx,
-            FOR_LOOP_OVER_RESULT,
+            FOR_LOOP_OVER_FALLIBLE,
             arg.span,
             &format!(
                 "for loop over `{0}`, which is a `Result`. This is more readably written as an \
-                 `if let` statement.",
+                `if let` statement.",
                 snippet(cx, arg.span, "_")
             ),
             None,

src/lintlist/mod.rs

@@ -676,16 +676,9 @@ pub static ref ALL_LINTS: Vec<Lint> = vec![
         module: "loops",
     },
     Lint {
-        name: "for_loop_over_option",
+        name: "for_loop_over_fallible",
         group: "correctness",
-        desc: "for-looping over an `Option`, which is more clearly expressed as an `if let`",
+        desc: "for-looping over an `Option` or a `Result`, which is more clearly expressed as an `if let`",
-        deprecation: None,
-        module: "loops",
-    },
-    Lint {
-        name: "for_loop_over_result",
-        group: "correctness",
-        desc: "for-looping over a `Result`, which is more clearly expressed as an `if let`",
         deprecation: None,
         module: "loops",
     },

tests/ui/for_loop_over_fallible.rs

@@ -1,18 +1,16 @@
-#![warn(clippy::for_loop_over_option, clippy::for_loop_over_result)]
+#![warn(clippy::for_loop_over_fallible)]
 
-/// Tests for_loop_over_result and for_loop_over_option
+fn for_loop_over_fallible() {
-
-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
+    // check over an `Option`
     for x in option {
         println!("{}", x);
     }
 
-    // check FOR_LOOP_OVER_RESULT lint
+    // check over a `Result`
     for x in result {
         println!("{}", x);
     }

tests/ui/for_loop_over_fallible.stderr

@@ -1,23 +1,22 @@
 error: for loop over `option`, which is an `Option`. This is more readably written as an `if let` statement.
-  --> $DIR/for_loop_over_option_result.rs:11:14
+  --> $DIR/for_loop_over_fallible.rs:9:14
    |
 LL |     for x in option {
    |              ^^^^^^
    |
-   = note: `-D clippy::for-loop-over-option` implied by `-D warnings`
+   = note: `-D clippy::for-loop-over-fallible` 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.
-  --> $DIR/for_loop_over_option_result.rs:16:14
+  --> $DIR/for_loop_over_fallible.rs:14:14
    |
 LL |     for x in result {
    |              ^^^^^^
    |
-   = note: `-D clippy::for-loop-over-result` implied by `-D warnings`
    = help: consider replacing `for x in result` 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_loop_over_option_result.rs:20:14
+  --> $DIR/for_loop_over_fallible.rs:18:14
    |
 LL |     for x in option.ok_or("x not found") {
    |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^
@@ -25,7 +24,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_loop_over_option_result.rs:26:14
+  --> $DIR/for_loop_over_fallible.rs:24:14
    |
 LL |     for x in v.iter().next() {
    |              ^^^^^^^^^^^^^^^
@@ -33,7 +32,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_loop_over_option_result.rs:31:14
+  --> $DIR/for_loop_over_fallible.rs:29:14
    |
 LL |     for x in v.iter().next().and(Some(0)) {
    |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
@@ -41,7 +40,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_loop_over_option_result.rs:35:14
+  --> $DIR/for_loop_over_fallible.rs:33:14
    |
 LL |     for x in v.iter().next().ok_or("x not found") {
    |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
@@ -49,7 +48,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_loop_over_option_result.rs:47:5
+  --> $DIR/for_loop_over_fallible.rs:45:5
    |
 LL | /     while let Some(x) = option {
 LL | |         println!("{}", x);
@@ -60,7 +59,7 @@ LL | |     }
    = note: `#[deny(clippy::never_loop)]` on by default
 
 error: this loop never actually loops
-  --> $DIR/for_loop_over_option_result.rs:53:5
+  --> $DIR/for_loop_over_fallible.rs:51:5
    |
 LL | /     while let Ok(x) = result {
 LL | |         println!("{}", x);