Fix manual_map suggestion for if let.. else ... if let.. else chain

Author: Jarcho

clippy_lints/src/manual_map.rs

@@ -1,17 +1,14 @@
-use crate::{
-    map_unit_fn::OPTION_MAP_UNIT_FN,
-    matches::MATCH_AS_REF,
-    utils::{is_allowed, match_def_path, match_var, paths, peel_hir_expr_refs},
-};
+use crate::{map_unit_fn::OPTION_MAP_UNIT_FN, matches::MATCH_AS_REF};
 use clippy_utils::diagnostics::span_lint_and_sugg;
 use clippy_utils::source::{snippet_with_applicability, snippet_with_context};
 use clippy_utils::ty::{can_partially_move_ty, is_type_diagnostic_item, peel_mid_ty_refs_is_mutable};
+use clippy_utils::{is_allowed, is_else_clause_of_if_let_else, match_def_path, match_var, paths, peel_hir_expr_refs};
 use rustc_ast::util::parser::PREC_POSTFIX;
 use rustc_errors::Applicability;
 use rustc_hir::{
     def::Res,
     intravisit::{walk_expr, ErasedMap, NestedVisitorMap, Visitor},
-    Arm, BindingAnnotation, Block, Expr, ExprKind, Mutability, Pat, PatKind, Path, QPath,
+    Arm, BindingAnnotation, Block, Expr, ExprKind, MatchSource, Mutability, Pat, PatKind, Path, QPath,
 };
 use rustc_lint::{LateContext, LateLintPass, LintContext};
 use rustc_middle::lint::in_external_macro;
@@ -54,8 +51,11 @@ impl LateLintPass<'_> for ManualMap {
             return;
         }
 
-        if let ExprKind::Match(scrutinee, [arm1 @ Arm { guard: None, .. }, arm2 @ Arm { guard: None, .. }], _) =
-            expr.kind
+        if let ExprKind::Match(
+            scrutinee,
+            [arm1 @ Arm { guard: None, .. }, arm2 @ Arm { guard: None, .. }],
+            match_kind,
+        ) = expr.kind
         {
             let (scrutinee_ty, ty_ref_count, ty_mutability) =
                 peel_mid_ty_refs_is_mutable(cx.typeck_results().expr_ty(scrutinee));
@@ -181,7 +181,12 @@ impl LateLintPass<'_> for ManualMap {
                 expr.span,
                 "manual implementation of `Option::map`",
                 "try this",
-                format!("{}{}.map({})", scrutinee_str, as_ref_str, body_str),
+                if matches!(match_kind, MatchSource::IfLetDesugar { .. }) && is_else_clause_of_if_let_else(cx.tcx, expr)
+                {
+                    format!("{{ {}{}.map({}) }}", scrutinee_str, as_ref_str, body_str)
+                } else {
+                    format!("{}{}.map({})", scrutinee_str, as_ref_str, body_str)
+                },
                 app,
             );
         }

clippy_utils/src/lib.rs

@@ -798,6 +798,26 @@ pub fn get_parent_as_impl(tcx: TyCtxt<'_>, id: HirId) -> Option<&Impl<'_>> {
     }
 }
 
+/// Checks if the given expression is the else clause in the expression `if let .. {} else {}`
+pub fn is_else_clause_of_if_let_else(tcx: TyCtxt<'_>, expr: &Expr<'_>) -> bool {
+    let map = tcx.hir();
+    let mut iter = map.parent_iter(expr.hir_id);
+    let arm_id = match iter.next() {
+        Some((id, Node::Arm(..))) => id,
+        _ => return false,
+    };
+    match iter.next() {
+        Some((
+            _,
+            Node::Expr(Expr {
+                kind: ExprKind::Match(_, [_, else_arm], kind),
+                ..
+            }),
+        )) => else_arm.hir_id == arm_id && matches!(kind, MatchSource::IfLetDesugar { .. }),
+        _ => false,
+    }
+}
+
 /// Checks whether the given expression is a constant integer of the given value.
 /// unlike `is_integer_literal`, this version does const folding
 pub fn is_integer_const(cx: &LateContext<'_>, e: &Expr<'_>, value: u128) -> bool {

tests/ui/manual_map_option.fixed

@@ -128,4 +128,9 @@ fn main() {
             None => None,
         };
     }
+
+    // #6847
+    if Some(0).is_some() {
+        Some(0)
+    } else { Some(0).map(|x| x + 1) };
 }

tests/ui/manual_map_option.rs

@@ -186,4 +186,13 @@ fn main() {
             None => None,
         };
     }
+
+    // #6847
+    if let Some(_) = Some(0) {
+        Some(0)
+    } else if let Some(x) = Some(0) {
+        Some(x + 1)
+    } else {
+        None
+    };
 }

tests/ui/manual_map_option.stderr

@@ -172,5 +172,24 @@ LL | |         None => None,
 LL | |     };
    | |_____^ help: try this: `option_env!("").map(String::from)`
 
-error: aborting due to 19 previous errors
+error: redundant pattern matching, consider using `is_some()`
+  --> $DIR/manual_map_option.rs:191:12
+   |
+LL |     if let Some(_) = Some(0) {
+   |     -------^^^^^^^---------- help: try this: `if Some(0).is_some()`
+   |
+   = note: `-D clippy::redundant-pattern-matching` implied by `-D warnings`
+
+error: manual implementation of `Option::map`
+  --> $DIR/manual_map_option.rs:193:12
+   |
+LL |       } else if let Some(x) = Some(0) {
+   |  ____________^
+LL | |         Some(x + 1)
+LL | |     } else {
+LL | |         None
+LL | |     };
+   | |_____^ help: try this: `{ Some(0).map(|x| x + 1) }`
+
+error: aborting due to 21 previous errors