PR Followups

Author: andrewpollack

clippy_lints/src/if_let_some_result.rs

@@ -226,7 +226,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_some_else_none;
 mod implicit_hasher;
@@ -291,6 +290,7 @@ mod needless_borrow;
 mod needless_borrowed_ref;
 mod needless_continue;
 mod needless_for_each;
+mod needless_ok;
 mod needless_pass_by_value;
 mod needless_question_mark;
 mod needless_update;
@@ -376,7 +376,6 @@ mod vec;
 mod vec_init_then_push;
 mod vec_resize_to_zero;
 mod verbose_file_reads;
-mod while_let_some_result;
 mod wildcard_dependencies;
 mod wildcard_imports;
 mod write;
@@ -656,7 +655,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_some_else_none::IF_THEN_SOME_ELSE_NONE,
         implicit_hasher::IMPLICIT_HASHER,
@@ -851,6 +849,8 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         needless_borrowed_ref::NEEDLESS_BORROWED_REFERENCE,
         needless_continue::NEEDLESS_CONTINUE,
         needless_for_each::NEEDLESS_FOR_EACH,
+        needless_ok::IF_LET_SOME_RESULT,
+        needless_ok::WHILE_LET_SOME_RESULT,
         needless_pass_by_value::NEEDLESS_PASS_BY_VALUE,
         needless_question_mark::NEEDLESS_QUESTION_MARK,
         needless_update::NEEDLESS_UPDATE,
@@ -989,7 +989,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         vec_init_then_push::VEC_INIT_THEN_PUSH,
         vec_resize_to_zero::VEC_RESIZE_TO_ZERO,
         verbose_file_reads::VERBOSE_FILE_READS,
-        while_let_some_result::WHILE_LET_SOME_RESULT,
         wildcard_dependencies::WILDCARD_DEPENDENCIES,
         wildcard_imports::ENUM_GLOB_USE,
         wildcard_imports::WILDCARD_IMPORTS,
@@ -1250,7 +1249,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(indexing_slicing::OUT_OF_BOUNDS_INDEXING),
         LintId::of(infinite_iter::INFINITE_ITER),
         LintId::of(inherent_to_string::INHERENT_TO_STRING),
@@ -1380,6 +1378,8 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(needless_bool::NEEDLESS_BOOL),
         LintId::of(needless_borrow::NEEDLESS_BORROW),
         LintId::of(needless_borrowed_ref::NEEDLESS_BORROWED_REFERENCE),
+        LintId::of(needless_ok::IF_LET_SOME_RESULT),
+        LintId::of(needless_ok::WHILE_LET_SOME_RESULT),
         LintId::of(needless_question_mark::NEEDLESS_QUESTION_MARK),
         LintId::of(needless_update::NEEDLESS_UPDATE),
         LintId::of(neg_cmp_op_on_partial_ord::NEG_CMP_OP_ON_PARTIAL_ORD),
@@ -1470,7 +1470,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(vec::USELESS_VEC),
         LintId::of(vec_init_then_push::VEC_INIT_THEN_PUSH),
         LintId::of(vec_resize_to_zero::VEC_RESIZE_TO_ZERO),
-        LintId::of(while_let_some_result::WHILE_LET_SOME_RESULT),
         LintId::of(write::PRINTLN_EMPTY_STRING),
         LintId::of(write::PRINT_LITERAL),
         LintId::of(write::PRINT_WITH_NEWLINE),
@@ -1505,7 +1504,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(inherent_to_string::INHERENT_TO_STRING),
         LintId::of(len_zero::COMPARISON_TO_EMPTY),
         LintId::of(len_zero::LEN_WITHOUT_IS_EMPTY),
@@ -1560,6 +1558,8 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(mut_mutex_lock::MUT_MUTEX_LOCK),
         LintId::of(mut_reference::UNNECESSARY_MUT_PASSED),
         LintId::of(needless_borrow::NEEDLESS_BORROW),
+        LintId::of(needless_ok::IF_LET_SOME_RESULT),
+        LintId::of(needless_ok::WHILE_LET_SOME_RESULT),
         LintId::of(neg_multiply::NEG_MULTIPLY),
         LintId::of(new_without_default::NEW_WITHOUT_DEFAULT),
         LintId::of(non_copy_const::BORROW_INTERIOR_MUTABLE_CONST),
@@ -1583,7 +1583,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(unsafe_removed_from_name::UNSAFE_REMOVED_FROM_NAME),
         LintId::of(unused_unit::UNUSED_UNIT),
         LintId::of(upper_case_acronyms::UPPER_CASE_ACRONYMS),
-        LintId::of(while_let_some_result::WHILE_LET_SOME_RESULT),
         LintId::of(write::PRINTLN_EMPTY_STRING),
         LintId::of(write::PRINT_LITERAL),
         LintId::of(write::PRINT_WITH_NEWLINE),
@@ -1975,8 +1974,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_late_pass(|| box missing_doc::MissingDoc::new());
     store.register_late_pass(|| box missing_inline::MissingInline);
     store.register_late_pass(move || box exhaustive_items::ExhaustiveItems);
-    store.register_late_pass(|| box if_let_some_result::OkIfLet);
-    store.register_late_pass(|| box while_let_some_result::WhileLetSomeResult);
+    store.register_late_pass(|| box needless_ok::NeedlessOk);
     store.register_late_pass(|| box partialeq_ne_impl::PartialEqNeImpl);
     store.register_late_pass(|| box unused_io_amount::UnusedIoAmount);
     let enum_variant_size_threshold = conf.enum_variant_size_threshold;

clippy_lints/src/lib.rs

@@ -9,6 +9,36 @@ use rustc_lint::{LateContext, LateLintPass};
 use rustc_session::{declare_lint_pass, declare_tool_lint};
 use rustc_span::sym;
 
+declare_clippy_lint! {
+    /// ### What it does
+    ///* Checks for unnecessary `ok()` in if let.
+    ///
+    /// ### Why is this bad?
+    /// Calling `ok()` in if let is unnecessary, instead match
+    /// on `Ok(pat)`
+    ///
+    /// ### Example
+    /// ```ignore
+    /// for i in iter {
+    ///     if let Some(value) = i.parse().ok() {
+    ///         vec.push(value)
+    ///     }
+    /// }
+    /// ```
+    /// Could be written:
+    ///
+    /// ```ignore
+    /// for i in iter {
+    ///     if let Ok(value) = i.parse() {
+    ///         vec.push(value)
+    ///     }
+    /// }
+    /// ```
+    pub IF_LET_SOME_RESULT,
+    style,
+    "usage of `ok()` in `if let Some(pat)` statements is unnecessary, match on `Ok(pat)` instead"
+}
+
 declare_clippy_lint! {
     /// ### What it does
     /// Checks for unnecessary `ok()` in `while let`.
@@ -34,11 +64,40 @@ declare_clippy_lint! {
     "usage of `ok()` in `while let Some(pat)` statements is unnecessary, match on `Ok(pat)` instead"
 }
 
-declare_lint_pass!(WhileLetSomeResult => [WHILE_LET_SOME_RESULT]);
+declare_lint_pass!(NeedlessOk => [IF_LET_SOME_RESULT, WHILE_LET_SOME_RESULT]);
 
-impl<'tcx> LateLintPass<'tcx> for WhileLetSomeResult {
+impl<'tcx> LateLintPass<'tcx> for NeedlessOk {
     fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
-        if_chain! {
+        if_chain! { // IF_LET_SOME_RESULT check
+            if let ExprKind::Match(op, body, MatchSource::IfLetDesugar { .. }) = expr.kind; // test if expr is if let
+            if let ExprKind::MethodCall(_, ok_span, result_types, _) = op.kind;
+            if let PatKind::TupleStruct(QPath::Resolved(_, x), y, _)  = body[0].pat.kind; // get operation
+            if method_chain_args(op, &["ok"]).is_some(); // test to see if using ok() methoduse std::marker::Sized;
+            if is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(&result_types[0]), sym::result_type);
+            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, op.span.until(ok_span), "", &mut applicability);
+                let sugg = format!(
+                    "if let Ok({}) = {}",
+                    some_expr_string,
+                    trimmed_ok.trim().trim_end_matches('.'),
+                );
+                span_lint_and_sugg(
+                    cx,
+                    IF_LET_SOME_RESULT,
+                    expr.span.with_hi(op.span.hi()),
+                    "matching on `Some` with `ok()` is redundant",
+                    &format!("consider matching on `Ok({})` and removing the call to `ok` instead", some_expr_string),
+                    sugg,
+                    applicability,
+                );
+            }
+        }
+
+        if_chain! { // WHILE_LET_SOME_RESULT check
             if let ExprKind::Match(op, body, MatchSource::WhileLetDesugar { .. }) = expr.kind; // test if expr is `while let`
             if let ExprKind::MethodCall(_, ok_span, result_types, _) = op.kind;
             if let PatKind::TupleStruct(QPath::Resolved(_, x), y, _)  = body[0].pat.kind; // get operation