Auto merge of #7608 - andrewpollack:7594/while_let_some_result, r=Manishearth

#7594: Adding new 'while_let_some_result' linting

Excited for the opportunity to contribute to Clippy! Since the latest Bay Area Rust Meetup, I've been looking for an opportunity and found it 😄

Please let me know how I can improve this PR! Much of it is similar to ``[`if_let_some_result`]``, but I followed the description in the issue to create its own linting rules. Looking forward to feedback, and continuing to work on Clippy in the future!

changelog: Renamed Lint: `if_let_some_result` is now called [`match_result_ok`]. Now also handles `while let` case.

CHANGELOG.md

@@ -8,6 +8,10 @@ document.
 
 [74d1561...master](https://github.com/rust-lang/rust-clippy/compare/74d1561...master)
 
+### New Lints
+
+* Renamed Lint: `if_let_some_result` is now called [`match_result_ok`]. Now also handles `while let` case.
+
 ## Rust 1.55
 
 Current beta, release 2021-09-09
@@ -1491,7 +1495,7 @@ Released 2020-03-12
 * `unknown_clippy_lints` [#4963](https://github.com/rust-lang/rust-clippy/pull/4963)
 * [`explicit_into_iter_loop`] [#4978](https://github.com/rust-lang/rust-clippy/pull/4978)
 * [`useless_attribute`] [#5022](https://github.com/rust-lang/rust-clippy/pull/5022)
-* [`if_let_some_result`] [#5032](https://github.com/rust-lang/rust-clippy/pull/5032)
+* `if_let_some_result` [#5032](https://github.com/rust-lang/rust-clippy/pull/5032)
 
 ### ICE fixes
 
@@ -2685,7 +2689,6 @@ Released 2018-09-13
 [`identity_op`]: https://rust-lang.github.io/rust-clippy/master/index.html#identity_op
 [`if_let_mutex`]: https://rust-lang.github.io/rust-clippy/master/index.html#if_let_mutex
 [`if_let_redundant_pattern_matching`]: https://rust-lang.github.io/rust-clippy/master/index.html#if_let_redundant_pattern_matching
-[`if_let_some_result`]: https://rust-lang.github.io/rust-clippy/master/index.html#if_let_some_result
 [`if_not_else`]: https://rust-lang.github.io/rust-clippy/master/index.html#if_not_else
 [`if_same_then_else`]: https://rust-lang.github.io/rust-clippy/master/index.html#if_same_then_else
 [`if_then_panic`]: https://rust-lang.github.io/rust-clippy/master/index.html#if_then_panic
@@ -2775,6 +2778,7 @@ Released 2018-09-13
 [`match_on_vec_items`]: https://rust-lang.github.io/rust-clippy/master/index.html#match_on_vec_items
 [`match_overlapping_arm`]: https://rust-lang.github.io/rust-clippy/master/index.html#match_overlapping_arm
 [`match_ref_pats`]: https://rust-lang.github.io/rust-clippy/master/index.html#match_ref_pats
+[`match_result_ok`]: https://rust-lang.github.io/rust-clippy/master/index.html#match_result_ok
 [`match_same_arms`]: https://rust-lang.github.io/rust-clippy/master/index.html#match_same_arms
 [`match_single_binding`]: https://rust-lang.github.io/rust-clippy/master/index.html#match_single_binding
 [`match_wild_err_arm`]: https://rust-lang.github.io/rust-clippy/master/index.html#match_wild_err_arm

clippy_lints/src/lib.rs

@@ -225,7 +225,6 @@ mod future_not_send;
 mod get_last_with_len;
 mod identity_op;
 mod if_let_mutex;
-mod if_let_some_result;
 mod if_not_else;
 mod if_then_panic;
 mod if_then_some_else_none;
@@ -264,6 +263,7 @@ mod map_clone;
 mod map_err_ignore;
 mod map_unit_fn;
 mod match_on_vec_items;
+mod match_result_ok;
 mod matches;
 mod mem_discriminant;
 mod mem_forget;
@@ -658,7 +658,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         get_last_with_len::GET_LAST_WITH_LEN,
         identity_op::IDENTITY_OP,
         if_let_mutex::IF_LET_MUTEX,
-        if_let_some_result::IF_LET_SOME_RESULT,
         if_not_else::IF_NOT_ELSE,
         if_then_panic::IF_THEN_PANIC,
         if_then_some_else_none::IF_THEN_SOME_ELSE_NONE,
@@ -728,6 +727,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         map_unit_fn::OPTION_MAP_UNIT_FN,
         map_unit_fn::RESULT_MAP_UNIT_FN,
         match_on_vec_items::MATCH_ON_VEC_ITEMS,
+        match_result_ok::MATCH_RESULT_OK,
         matches::INFALLIBLE_DESTRUCTURING_MATCH,
         matches::MATCH_AS_REF,
         matches::MATCH_BOOL,
@@ -1259,7 +1259,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(get_last_with_len::GET_LAST_WITH_LEN),
         LintId::of(identity_op::IDENTITY_OP),
         LintId::of(if_let_mutex::IF_LET_MUTEX),
-        LintId::of(if_let_some_result::IF_LET_SOME_RESULT),
         LintId::of(if_then_panic::IF_THEN_PANIC),
         LintId::of(indexing_slicing::OUT_OF_BOUNDS_INDEXING),
         LintId::of(infinite_iter::INFINITE_ITER),
@@ -1303,6 +1302,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(map_clone::MAP_CLONE),
         LintId::of(map_unit_fn::OPTION_MAP_UNIT_FN),
         LintId::of(map_unit_fn::RESULT_MAP_UNIT_FN),
+        LintId::of(match_result_ok::MATCH_RESULT_OK),
         LintId::of(matches::INFALLIBLE_DESTRUCTURING_MATCH),
         LintId::of(matches::MATCH_AS_REF),
         LintId::of(matches::MATCH_LIKE_MATCHES_MACRO),
@@ -1513,7 +1513,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(functions::DOUBLE_MUST_USE),
         LintId::of(functions::MUST_USE_UNIT),
         LintId::of(functions::RESULT_UNIT_ERR),
-        LintId::of(if_let_some_result::IF_LET_SOME_RESULT),
         LintId::of(if_then_panic::IF_THEN_PANIC),
         LintId::of(inherent_to_string::INHERENT_TO_STRING),
         LintId::of(len_zero::COMPARISON_TO_EMPTY),
@@ -1530,6 +1529,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(manual_map::MANUAL_MAP),
         LintId::of(manual_non_exhaustive::MANUAL_NON_EXHAUSTIVE),
         LintId::of(map_clone::MAP_CLONE),
+        LintId::of(match_result_ok::MATCH_RESULT_OK),
         LintId::of(matches::INFALLIBLE_DESTRUCTURING_MATCH),
         LintId::of(matches::MATCH_LIKE_MATCHES_MACRO),
         LintId::of(matches::MATCH_OVERLAPPING_ARM),
@@ -1985,7 +1985,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_late_pass(|| Box::new(missing_doc::MissingDoc::new()));
     store.register_late_pass(|| Box::new(missing_inline::MissingInline));
     store.register_late_pass(move || Box::new(exhaustive_items::ExhaustiveItems));
-    store.register_late_pass(|| Box::new(if_let_some_result::OkIfLet));
+    store.register_late_pass(|| Box::new(match_result_ok::MatchResultOk));
     store.register_late_pass(|| Box::new(partialeq_ne_impl::PartialEqNeImpl));
     store.register_late_pass(|| Box::new(unused_io_amount::UnusedIoAmount));
     let enum_variant_size_threshold = conf.enum_variant_size_threshold;
@@ -2212,6 +2212,7 @@ pub fn register_renamed(ls: &mut rustc_lint::LintStore) {
     ls.register_renamed("clippy::identity_conversion", "clippy::useless_conversion");
     ls.register_renamed("clippy::zero_width_space", "clippy::invisible_characters");
     ls.register_renamed("clippy::single_char_push_str", "clippy::single_char_add_str");
+    ls.register_renamed("clippy::if_let_some_result", "clippy::match_result_ok");
 
     // uplifted lints
     ls.register_renamed("clippy::invalid_ref", "invalid_value");

clippy_lints/src/match_result_ok.rs

@@ -12,40 +12,51 @@ use rustc_span::sym;
 
 declare_clippy_lint! {
     /// ### What it does
-    ///* Checks for unnecessary `ok()` in if let.
+    /// Checks for unnecessary `ok()` in `while let`.
     ///
     /// ### Why is this bad?
-    /// Calling `ok()` in if let is unnecessary, instead match
+    /// Calling `ok()` in `while let` is unnecessary, instead match
     /// on `Ok(pat)`
     ///
     /// ### Example
     /// ```ignore
-    /// for i in iter {
+    /// while let Some(value) = iter.next().ok() {
-    ///     if let Some(value) = i.parse().ok() {
+    ///     vec.push(value)
-    ///         vec.push(value)
-    ///     }
     /// }
-    /// ```
-    /// Could be written:
     ///
-    /// ```ignore
+    /// if let Some(valie) = iter.next().ok() {
-    /// for i in iter {
+    ///     vec.push(value)
-    ///     if let Ok(value) = i.parse() {
-    ///         vec.push(value)
-    ///     }
     /// }
     /// ```
-    pub IF_LET_SOME_RESULT,
+    /// Use instead:
+    /// ```ignore
+    /// while let Ok(value) = iter.next() {
+    ///     vec.push(value)
+    /// }
+    ///
+    /// if let Ok(value) = iter.next() {
+    ///        vec.push_value)
+    /// }
+    /// ```
+    pub MATCH_RESULT_OK,
     style,
-    "usage of `ok()` in `if let Some(pat)` statements is unnecessary, match on `Ok(pat)` instead"
+    "usage of `ok()` in `let Some(pat)` statements is unnecessary, match on `Ok(pat)` instead"
 }
 
-declare_lint_pass!(OkIfLet => [IF_LET_SOME_RESULT]);
+declare_lint_pass!(MatchResultOk => [MATCH_RESULT_OK]);
 
-impl<'tcx> LateLintPass<'tcx> for OkIfLet {
+impl<'tcx> LateLintPass<'tcx> for MatchResultOk {
     fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
-        if_chain! { //begin checking variables
+        let (let_pat, let_expr, ifwhile) =
-            if let Some(higher::IfLet { let_pat, let_expr, .. }) = higher::IfLet::hir(cx, expr);
+            if let Some(higher::IfLet { let_pat, let_expr, .. }) = higher::IfLet::hir(cx, expr) {
+                (let_pat, let_expr, "if")
+            } else if let Some(higher::WhileLet { let_pat, let_expr, .. }) = higher::WhileLet::hir(expr) {
+                (let_pat, let_expr, "while")
+            } else {
+                return;
+            };
+
+        if_chain! {
             if let ExprKind::MethodCall(_, ok_span, [ref result_types_0, ..], _) = let_expr.kind; //check is expr.ok() has type Result<T,E>.ok(, _)
             if let PatKind::TupleStruct(QPath::Resolved(_, x), y, _)  = let_pat.kind; //get operation
             if method_chain_args(let_expr, &["ok"]).is_some(); //test to see if using ok() methoduse std::marker::Sized;
@@ -53,17 +64,19 @@ impl<'tcx> LateLintPass<'tcx> for OkIfLet {
             if rustc_hir_pretty::to_string(rustc_hir_pretty::NO_ANN, |s| s.print_path(x, false)) == "Some";
 
             then {
+
                 let mut applicability = Applicability::MachineApplicable;
                 let some_expr_string = snippet_with_applicability(cx, y[0].span, "", &mut applicability);
                 let trimmed_ok = snippet_with_applicability(cx, let_expr.span.until(ok_span), "", &mut applicability);
                 let sugg = format!(
-                    "if let Ok({}) = {}",
+                    "{} let Ok({}) = {}",
+                    ifwhile,
                     some_expr_string,
                     trimmed_ok.trim().trim_end_matches('.'),
                 );
                 span_lint_and_sugg(
                     cx,
-                    IF_LET_SOME_RESULT,
+                    MATCH_RESULT_OK,
                     expr.span.with_hi(let_expr.span.hi()),
                     "matching on `Some` with `ok()` is redundant",
                     &format!("consider matching on `Ok({})` and removing the call to `ok` instead", some_expr_string),

tests/ui/if_let_some_result.fixed

@@ -1,28 +0,0 @@
-// run-rustfix
-
-#![warn(clippy::if_let_some_result)]
-#![allow(dead_code)]
-
-fn str_to_int(x: &str) -> i32 {
-    if let Ok(y) = x.parse() { y } else { 0 }
-}
-
-fn str_to_int_ok(x: &str) -> i32 {
-    if let Ok(y) = x.parse() { y } else { 0 }
-}
-
-#[rustfmt::skip]
-fn strange_some_no_else(x: &str) -> i32 {
-    {
-        if let Ok(y) = x   .   parse()       {
-            return y;
-        };
-        0
-    }
-}
-
-fn negative() {
-    while let Some(1) = "".parse().ok() {}
-}
-
-fn main() {}

tests/ui/if_let_some_result.rs

@@ -1,28 +0,0 @@
-// run-rustfix
-
-#![warn(clippy::if_let_some_result)]
-#![allow(dead_code)]
-
-fn str_to_int(x: &str) -> i32 {
-    if let Some(y) = x.parse().ok() { y } else { 0 }
-}
-
-fn str_to_int_ok(x: &str) -> i32 {
-    if let Ok(y) = x.parse() { y } else { 0 }
-}
-
-#[rustfmt::skip]
-fn strange_some_no_else(x: &str) -> i32 {
-    {
-        if let Some(y) = x   .   parse()   .   ok   ()    {
-            return y;
-        };
-        0
-    }
-}
-
-fn negative() {
-    while let Some(1) = "".parse().ok() {}
-}
-
-fn main() {}

tests/ui/match_result_ok.fixed

@@ -0,0 +1,63 @@
+// run-rustfix
+
+#![warn(clippy::match_result_ok)]
+#![allow(clippy::boxed_local)]
+#![allow(dead_code)]
+
+// Checking `if` cases
+
+fn str_to_int(x: &str) -> i32 {
+    if let Ok(y) = x.parse() { y } else { 0 }
+}
+
+fn str_to_int_ok(x: &str) -> i32 {
+    if let Ok(y) = x.parse() { y } else { 0 }
+}
+
+#[rustfmt::skip]
+fn strange_some_no_else(x: &str) -> i32 {
+    {
+        if let Ok(y) = x   .   parse()       {
+            return y;
+        };
+        0
+    }
+}
+
+// Checking `while` cases
+
+struct Wat {
+    counter: i32,
+}
+
+impl Wat {
+    fn next(&mut self) -> Result<i32, &str> {
+        self.counter += 1;
+        if self.counter < 5 {
+            Ok(self.counter)
+        } else {
+            Err("Oh no")
+        }
+    }
+}
+
+fn base_1(x: i32) {
+    let mut wat = Wat { counter: x };
+    while let Ok(a) = wat.next() {
+        println!("{}", a);
+    }
+}
+
+fn base_2(x: i32) {
+    let mut wat = Wat { counter: x };
+    while let Ok(a) = wat.next() {
+        println!("{}", a);
+    }
+}
+
+fn base_3(test_func: Box<Result<i32, &str>>) {
+    // Expected to stay as is
+    while let Some(_b) = test_func.ok() {}
+}
+
+fn main() {}

tests/ui/match_result_ok.rs

@@ -0,0 +1,63 @@
+// run-rustfix
+
+#![warn(clippy::match_result_ok)]
+#![allow(clippy::boxed_local)]
+#![allow(dead_code)]
+
+// Checking `if` cases
+
+fn str_to_int(x: &str) -> i32 {
+    if let Some(y) = x.parse().ok() { y } else { 0 }
+}
+
+fn str_to_int_ok(x: &str) -> i32 {
+    if let Ok(y) = x.parse() { y } else { 0 }
+}
+
+#[rustfmt::skip]
+fn strange_some_no_else(x: &str) -> i32 {
+    {
+        if let Some(y) = x   .   parse()   .   ok   ()    {
+            return y;
+        };
+        0
+    }
+}
+
+// Checking `while` cases
+
+struct Wat {
+    counter: i32,
+}
+
+impl Wat {
+    fn next(&mut self) -> Result<i32, &str> {
+        self.counter += 1;
+        if self.counter < 5 {
+            Ok(self.counter)
+        } else {
+            Err("Oh no")
+        }
+    }
+}
+
+fn base_1(x: i32) {
+    let mut wat = Wat { counter: x };
+    while let Some(a) = wat.next().ok() {
+        println!("{}", a);
+    }
+}
+
+fn base_2(x: i32) {
+    let mut wat = Wat { counter: x };
+    while let Ok(a) = wat.next() {
+        println!("{}", a);
+    }
+}
+
+fn base_3(test_func: Box<Result<i32, &str>>) {
+    // Expected to stay as is
+    while let Some(_b) = test_func.ok() {}
+}
+
+fn main() {}

tests/ui/match_result_ok.stderr

@@ -1,17 +1,17 @@
 error: matching on `Some` with `ok()` is redundant
-  --> $DIR/if_let_some_result.rs:7:5
+  --> $DIR/match_result_ok.rs:10:5
    |
 LL |     if let Some(y) = x.parse().ok() { y } else { 0 }
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
-   = note: `-D clippy::if-let-some-result` implied by `-D warnings`
+   = note: `-D clippy::match-result-ok` implied by `-D warnings`
 help: consider matching on `Ok(y)` and removing the call to `ok` instead
    |
 LL |     if let Ok(y) = x.parse() { y } else { 0 }
    |     ~~~~~~~~~~~~~~~~~~~~~~~~
 
 error: matching on `Some` with `ok()` is redundant
-  --> $DIR/if_let_some_result.rs:17:9
+  --> $DIR/match_result_ok.rs:20:9
    |
 LL |         if let Some(y) = x   .   parse()   .   ok   ()    {
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
@@ -21,5 +21,16 @@ help: consider matching on `Ok(y)` and removing the call to `ok` instead
 LL |         if let Ok(y) = x   .   parse()       {
    |         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
-error: aborting due to 2 previous errors
+error: matching on `Some` with `ok()` is redundant
+  --> $DIR/match_result_ok.rs:46:5
+   |
+LL |     while let Some(a) = wat.next().ok() {
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+help: consider matching on `Ok(a)` and removing the call to `ok` instead
+   |
+LL |     while let Ok(a) = wat.next() {
+   |     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+error: aborting due to 3 previous errors