Most of remaining suggestions from flip1995

Author: JarredAllen

clippy_lints/src/option_if_let_else.rs

@@ -1,5 +1,5 @@
 use crate::utils::sugg::Sugg;
-use crate::utils::{match_qpath, match_type, paths, span_lint_and_sugg};
+use crate::utils::{match_type, paths, span_lint_and_sugg};
 use if_chain::if_chain;
 
 use rustc_errors::Applicability;
@@ -10,40 +10,20 @@ use rustc_session::{declare_lint_pass, declare_tool_lint};
 declare_clippy_lint! {
     /// **What it does:**
     /// Lints usage of  `if let Some(v) = ... { y } else { x }` which is more 
-    /// idiomatically done with `Option::map_or` or `Option::map_or_else`.
+    /// idiomatically done with `Option::map_or` (if the else bit is a simple
+    /// expression) or `Option::map_or_else` (if the else bit is a longer
+    /// block).
     ///
     /// **Why is this bad?**
     /// Using the dedicated functions of the Option type is clearer and
     /// more concise than an if let expression.
     ///
     /// **Known problems:**
-    /// Using `Option::map_or` (as the lint currently suggests) requires
-    /// it to compute the else value every function call, even if the
-    /// value is unneeded. It might be better to use Option::map_or_else
-    /// if the code is either very expensive or if it changes some state
-    /// (i.e. mutates a value). For example,
-    /// ```rust
-    /// # let optional = Some(0);
-    /// let _ = if let Some(x) = optional {
-    ///     x
-    /// } else {
-    ///     let y = 0;
-    ///     /* Some expensive computation or state-changing operation */
-    ///     y
-    /// };
-    /// ```
-    /// would be better as
-    /// ```rust
-    /// # let optional = Some(0);
-    /// let _ = optional.map_or_else(|| { let y = 0; /* some expensive computation or
-    /// state-changing operation */ y }, |x| x);
-    /// ```
-    /// instead of the suggested
-    /// ```rust
-    /// # let optional = Some(0);
-    /// let _ = optional.map_or({ let y = 0; /* some expensive computation or state-changing
-    /// operation */ y }, |x| x);
-    /// ```
+    /// This lint uses whether the block is just an expression or if it has
+    /// more statements to decide whether to use `Option::map_or` or
+    /// `Option::map_or_else`. If you have a single expression which calls
+    /// an expensive function, then it would be more efficient to use
+    /// `Option::map_or_else`, but this lint would suggest `Option::map_or`.
     ///
     /// **Example:**
     ///
@@ -54,13 +34,23 @@ declare_clippy_lint! {
     /// } else {
     ///     5
     /// };
+    /// let _ = if let Some(foo) = optional {
+    ///     foo
+    /// } else {
+    ///     let y = do_complicated_function();
+    ///     y*y
+    /// };
     /// ```
     ///
     /// should be
     ///
     /// ```rust
     /// # let optional: Option<u32> = Some(0);
     /// let _ = optional.map_or(5, |foo| foo);
+    /// let _ = optional.map_or_else(||{
+    ///     let y = do_complicated_function;
+    ///     y*y
+    /// }, |foo| foo);
     /// ```
     pub OPTION_IF_LET_ELSE,
     style,
@@ -83,29 +73,32 @@ fn is_result_ok(cx: &LateContext<'_, '_>, expr: &'_ Expr<'_>) -> bool {
     }
 }
 
+/// A struct containing information about occurences of the
+/// `if let Some(..) = .. else` construct that this lint detects.
+struct OptionIfLetElseOccurence {
+    option: String,
+    method_sugg: String,
+    some_expr: String,
+    none_expr: String,
+}
+
 /// If this expression is the option if let/else construct we're detecting, then
-/// this function returns Some(Option<_> tested against, the method to call on
-/// the object (map_or or map_or_else), expression if Option is Some,
-/// expression if Option is None). Otherwise, it returns None.
+/// this function returns an OptionIfLetElseOccurence struct with details if
+/// this construct is found, or None if this construct is not found.
 fn detect_option_if_let_else<'a>(
     cx: &LateContext<'_, '_>,
     expr: &'a Expr<'_>,
-) -> Option<(String, String, String, String)> {
+) -> Option<OptionIfLetElseOccurence> { //(String, String, String, String)> {
     if_chain! {
         if let ExprKind::Match(let_body, arms, MatchSource::IfLetDesugar{contains_else_clause: true}) = &expr.kind;
         if arms.len() == 2;
         if match_type(cx, &cx.tables.expr_ty(let_body), &paths::OPTION);
-        if !is_result_ok(cx, let_body);
-        if let PatKind::TupleStruct(path, &[inner_pat], _) = &arms[0].pat.kind;
+        if !is_result_ok(cx, let_body); // Don't lint on Result::ok because a different lint does it already
+        if let PatKind::TupleStruct(_, &[inner_pat], _) = &arms[0].pat.kind;
         if let PatKind::Binding(_, _, id, _) = &inner_pat.kind;
         then {
-            let (some_body, none_body) = if match_qpath(path, &paths::OPTION_SOME) {
-                (arms[0].body, arms[1].body)
-            } else {
-                (arms[1].body, arms[0].body)
-            };
             let some_body = if let ExprKind::Block(Block { stmts: statements, expr: Some(expr), .. }, _)
-                = &some_body.kind {
+                = &arms[0].body.kind {
                 if let &[] = &statements {
                     expr
                 } else {
@@ -120,13 +113,13 @@ fn detect_option_if_let_else<'a>(
                     }) {
                         return None;
                     }
-                    some_body
+                    &arms[0].body
                 }
             } else {
                 return None;
             };
             let (none_body, method_sugg) = if let ExprKind::Block(Block { stmts: statements, expr: Some(expr), .. }, _)
-                = &none_body.kind {
+                = &arms[1].body.kind {
                 if let &[] = &statements {
                     (expr, "map_or")
                 } else {
@@ -140,18 +133,18 @@ fn detect_option_if_let_else<'a>(
                     }) {
                         return None;
                     }
-                    (&none_body, "map_or_else")
+                    (&arms[1].body, "map_or_else")
                 }
             } else {
                 return None;
             };
             let capture_name = id.name.to_ident_string();
-            Some((
-                format!("{}", Sugg::hir(cx, let_body, "..")),
-                format!("{}", method_sugg),
-                format!("|{}| {}", capture_name, Sugg::hir(cx, some_body, "..")),
-                format!("{}{}", if method_sugg == "map_or" { "" } else { "||" }, Sugg::hir(cx, none_body, ".."))
-            ))
+            Some(OptionIfLetElseOccurence {
+                option: format!("{}", Sugg::hir(cx, let_body, "..")),
+                method_sugg: format!("{}", method_sugg),
+                some_expr: format!("|{}| {}", capture_name, Sugg::hir(cx, some_body, "..")),
+                none_expr: format!("{}{}", if method_sugg == "map_or" { "" } else { "||" }, Sugg::hir(cx, none_body, ".."))
+            })
         } else {
             None
         }
@@ -160,14 +153,14 @@ fn detect_option_if_let_else<'a>(
 
 impl<'a, 'tcx> LateLintPass<'a, 'tcx> for OptionIfLetElse {
     fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr<'_>) {
-        if let Some((option, method, map, else_func)) = detect_option_if_let_else(cx, expr) {
+        if let Some(detection) = detect_option_if_let_else(cx, expr) {
             span_lint_and_sugg(
                 cx,
                 OPTION_IF_LET_ELSE,
                 expr.span,
-                format!("use Option::{} instead of an if let/else", method).as_str(),
+                format!("use Option::{} instead of an if let/else", detection.method_sugg).as_str(),
                 "try",
-                format!("{}.{}({}, {})", option, method, else_func, map),
+                format!("{}.{}({}, {})", detection.option, detection.method_sugg, detection.none_expr, detection.some_expr),
                 Applicability::MachineApplicable,
             );
         }

tests/ui/author/blocks.rs

@@ -1,5 +1,5 @@
 #![feature(stmt_expr_attributes)]
-#![allow(redundant_semicolon, clippy::no_effect)]
+#![allow(redundant_semicolons, clippy::no_effect)]
 
 #[rustfmt::skip]
 fn main() {

tests/ui/option_if_let_else.fixed

@@ -1,5 +1,5 @@
-#![warn(clippy::option_if_let_else)]
 // run-rustfix
+#![warn(clippy::option_if_let_else)]
 
 fn bad1(string: Option<&str>) -> (bool, &str) {
     string.map_or((false, "hello"), |x| (true, x))

tests/ui/option_if_let_else.stderr

@@ -47,7 +47,7 @@ help: try
 LL |     let _ = arg.map_or_else(||{
 LL |         let mut y = 1;
 LL |         for _ in 0..10 {
-LL |             y = (y + 2 / y) / 2;
+LL |             y = (y + 2/y) / 2;
 LL |         }
 LL |         y
  ...