Auto merge of rust-lang#88163 - camsteffen:collapsible-match-fix, r=Manishearth

Fix clippy::collapsible_match with let expressions

This fixes rust-lang/rust-clippy#7575 which is a regression from rust-lang#80357. I am fixing the bug here instead of in the clippy repo (if that's okay) because a) the regression has not been synced yet and b) I would like to land the fix on nightly asap.

The fix is basically to re-generalize `match` and `if let` for the lint implementation (they were split because `if let` no longer desugars to `match` in the HIR).

Also fixes rust-lang/rust-clippy#7586 and fixes rust-lang/rust-clippy#7591
cc `@rust-lang/clippy`
`@xFrednet` do you want to review this?

Author: bors

src/tools/clippy/clippy_lints/src/collapsible_match.rs

@@ -1,9 +1,9 @@
 use clippy_utils::diagnostics::span_lint_and_then;
 use clippy_utils::visitors::LocalUsedVisitor;
-use clippy_utils::{higher, is_lang_ctor, path_to_local, peel_ref_operators, SpanlessEq};
+use clippy_utils::{higher, is_lang_ctor, is_unit_expr, path_to_local, peel_ref_operators, SpanlessEq};
 use if_chain::if_chain;
 use rustc_hir::LangItem::OptionNone;
-use rustc_hir::{Expr, ExprKind, Guard, HirId, Pat, PatKind, StmtKind};
+use rustc_hir::{Arm, Expr, ExprKind, Guard, HirId, MatchSource, Pat, PatKind, StmtKind};
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_session::{declare_lint_pass, declare_tool_lint};
 use rustc_span::{MultiSpan, Span};
@@ -49,104 +49,87 @@ declare_lint_pass!(CollapsibleMatch => [COLLAPSIBLE_MATCH]);
 
 impl<'tcx> LateLintPass<'tcx> for CollapsibleMatch {
     fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &Expr<'tcx>) {
-        if let Some(higher::IfLet {
-            let_pat,
-            if_then,
-            if_else,
-            ..
-        }) = higher::IfLet::hir(expr)
-        {
-            check_arm(cx, if_then, None, let_pat, if_else);
-
-            check_if_let(cx, if_then, let_pat);
-        }
-
-        if let ExprKind::Match(_expr, arms, _source) = expr.kind {
-            if let Some(wild_arm) = arms.iter().rfind(|arm| is_wild_like(cx, &arm.pat.kind, &arm.guard)) {
-                for arm in arms {
-                    check_arm(cx, arm.body, arm.guard.as_ref(), arm.pat, Some(wild_arm.body));
+        match IfLetOrMatch::parse(cx, expr) {
+            Some(IfLetOrMatch::Match(_, arms, _)) => {
+                if let Some(els_arm) = arms.iter().rfind(|arm| arm_is_wild_like(cx, arm)) {
+                    for arm in arms {
+                        check_arm(cx, true, arm.pat, arm.body, arm.guard.as_ref(), Some(els_arm.body));
+                    }
                 }
             }
-
-            if let Some(first_arm) = arms.get(0) {
-                check_if_let(cx, &first_arm.body, &first_arm.pat);
+            Some(IfLetOrMatch::IfLet(_, pat, body, els)) => {
+                check_arm(cx, false, pat, body, None, els);
             }
+            None => {}
         }
     }
 }
 
 fn check_arm<'tcx>(
     cx: &LateContext<'tcx>,
-    outer_block: &'tcx Expr<'tcx>,
-    outer_guard: Option<&Guard<'tcx>>,
+    outer_is_match: bool,
     outer_pat: &'tcx Pat<'tcx>,
-    wild_outer_block: Option<&'tcx Expr<'tcx>>,
+    outer_then_body: &'tcx Expr<'tcx>,
+    outer_guard: Option<&'tcx Guard<'tcx>>,
+    outer_else_body: Option<&'tcx Expr<'tcx>>
 ) {
-    let expr = strip_singleton_blocks(outer_block);
+    let inner_expr = strip_singleton_blocks(outer_then_body);
     if_chain! {
-        if let ExprKind::Match(expr_in, arms_inner, _) = expr.kind;
-        // the outer arm pattern and the inner match
-        if expr_in.span.ctxt() == outer_pat.span.ctxt();
-        // there must be no more than two arms in the inner match for this lint
-        if arms_inner.len() == 2;
-        // no if guards on the inner match
-        if arms_inner.iter().all(|arm| arm.guard.is_none());
+        if let Some(inner) = IfLetOrMatch::parse(cx, inner_expr);
+        if let Some((inner_scrutinee, inner_then_pat, inner_else_body)) = match inner {
+            IfLetOrMatch::IfLet(scrutinee, pat, _, els) => Some((scrutinee, pat, els)),
+            IfLetOrMatch::Match(scrutinee, arms, ..) => if_chain! {
+                // if there are more than two arms, collapsing would be non-trivial
+                if arms.len() == 2 && arms.iter().all(|a| a.guard.is_none());
+                // one of the arms must be "wild-like"
+                if let Some(wild_idx) = arms.iter().rposition(|a| arm_is_wild_like(cx, a));
+                then {
+                    let (then, els) = (&arms[1 - wild_idx], &arms[wild_idx]);
+                    Some((scrutinee, then.pat, Some(els.body)))
+                } else {
+                    None
+                }
+            },
+        };
+        if outer_pat.span.ctxt() == inner_scrutinee.span.ctxt();
         // match expression must be a local binding
         // match <local> { .. }
-        if let Some(binding_id) = path_to_local(peel_ref_operators(cx, expr_in));
-        // one of the branches must be "wild-like"
-        if let Some(wild_inner_arm_idx) = arms_inner.iter().rposition(|arm_inner| is_wild_like(cx, &arm_inner.pat.kind, &arm_inner.guard));
-        let (wild_inner_arm, non_wild_inner_arm) =
-            (&arms_inner[wild_inner_arm_idx], &arms_inner[1 - wild_inner_arm_idx]);
-        if !pat_contains_or(non_wild_inner_arm.pat);
+        if let Some(binding_id) = path_to_local(peel_ref_operators(cx, inner_scrutinee));
+        if !pat_contains_or(inner_then_pat);
         // the binding must come from the pattern of the containing match arm
         // ..<local>.. => match <local> { .. }
         if let Some(binding_span) = find_pat_binding(outer_pat, binding_id);
-        // the "wild-like" branches must be equal
-        if wild_outer_block.map(|el| SpanlessEq::new(cx).eq_expr(wild_inner_arm.body, el)).unwrap_or(true);
+        // the "else" branches must be equal
+        if match (outer_else_body, inner_else_body) {
+            (None, None) => true,
+            (None, Some(e)) | (Some(e), None) => is_unit_expr(e),
+            (Some(a), Some(b)) => SpanlessEq::new(cx).eq_expr(a, b),
+        };
         // the binding must not be used in the if guard
         let mut used_visitor = LocalUsedVisitor::new(cx, binding_id);
-        if match outer_guard {
-            None => true,
-            Some(Guard::If(expr) | Guard::IfLet(_, expr)) => !used_visitor.check_expr(expr),
+        if outer_guard.map_or(true, |(Guard::If(e) | Guard::IfLet(_, e))| !used_visitor.check_expr(e));
+        // ...or anywhere in the inner expression
+        if match inner {
+            IfLetOrMatch::IfLet(_, _, body, els) => {
+                !used_visitor.check_expr(body) && els.map_or(true, |e| !used_visitor.check_expr(e))
+            },
+            IfLetOrMatch::Match(_, arms, ..) => !arms.iter().any(|arm| used_visitor.check_arm(arm)),
         };
-        // ...or anywhere in the inner match
-        if !arms_inner.iter().any(|arm| used_visitor.check_arm(arm));
         then {
-            span_lint_and_then(
-                cx,
-                COLLAPSIBLE_MATCH,
-                expr.span,
-                "unnecessary nested match",
-                |diag| {
-                    let mut help_span = MultiSpan::from_spans(vec![binding_span, non_wild_inner_arm.pat.span]);
-                    help_span.push_span_label(binding_span, "replace this binding".into());
-                    help_span.push_span_label(non_wild_inner_arm.pat.span, "with this pattern".into());
-                    diag.span_help(help_span, "the outer pattern can be modified to include the inner pattern");
-                },
+            let msg = format!(
+                "this `{}` can be collapsed into the outer `{}`",
+                if matches!(inner, IfLetOrMatch::Match(..)) { "match" } else { "if let" },
+                if outer_is_match { "match" } else { "if let" },
             );
-        }
-    }
-}
-
-fn check_if_let<'tcx>(cx: &LateContext<'tcx>, outer_expr: &'tcx Expr<'tcx>, outer_pat: &'tcx Pat<'tcx>) {
-    let block_inner = strip_singleton_blocks(outer_expr);
-    if_chain! {
-        if let Some(higher::IfLet { if_then: inner_if_then, let_expr: inner_let_expr, let_pat: inner_let_pat, .. }) = higher::IfLet::hir(block_inner);
-        if let Some(binding_id) = path_to_local(peel_ref_operators(cx, inner_let_expr));
-        if let Some(binding_span) = find_pat_binding(outer_pat, binding_id);
-        let mut used_visitor = LocalUsedVisitor::new(cx, binding_id);
-        if !used_visitor.check_expr(inner_if_then);
-        then {
             span_lint_and_then(
                 cx,
                 COLLAPSIBLE_MATCH,
-                block_inner.span,
-                "unnecessary nested `if let` or `match`",
+                inner_expr.span,
+                &msg,
                 |diag| {
-                    let mut help_span = MultiSpan::from_spans(vec![binding_span, inner_let_pat.span]);
+                    let mut help_span = MultiSpan::from_spans(vec![binding_span, inner_then_pat.span]);
                     help_span.push_span_label(binding_span, "replace this binding".into());
-                    help_span.push_span_label(inner_let_pat.span, "with this pattern".into());
+                    help_span.push_span_label(inner_then_pat.span, "with this pattern".into());
                     diag.span_help(help_span, "the outer pattern can be modified to include the inner pattern");
                 },
             );
@@ -168,14 +151,30 @@ fn strip_singleton_blocks<'hir>(mut expr: &'hir Expr<'hir>) -> &'hir Expr<'hir>
     expr
 }
 
-/// A "wild-like" pattern is wild ("_") or `None`.
-/// For this lint to apply, both the outer and inner patterns
-/// must have "wild-like" branches that can be combined.
-fn is_wild_like(cx: &LateContext<'_>, pat_kind: &PatKind<'_>, arm_guard: &Option<Guard<'_>>) -> bool {
-    if arm_guard.is_some() {
+enum IfLetOrMatch<'hir> {
+    Match(&'hir Expr<'hir>, &'hir [Arm<'hir>], MatchSource),
+    /// scrutinee, pattern, then block, else block
+    IfLet(&'hir Expr<'hir>, &'hir Pat<'hir>, &'hir Expr<'hir>, Option<&'hir Expr<'hir>>),
+}
+
+impl<'hir> IfLetOrMatch<'hir> {
+    fn parse(cx: &LateContext<'_>, expr: &Expr<'hir>) -> Option<Self> {
+        match expr.kind {
+            ExprKind::Match(expr, arms, source) => Some(Self::Match(expr, arms, source)),
+            _ => higher::IfLet::hir(cx, expr).map(|higher::IfLet { let_expr, let_pat, if_then, if_else }| {
+                Self::IfLet(let_expr, let_pat, if_then, if_else)
+            })
+        }
+    }
+}
+
+/// A "wild-like" arm has a wild (`_`) or `None` pattern and no guard. Such arms can be "collapsed"
+/// into a single wild arm without any significant loss in semantics or readability.
+fn arm_is_wild_like(cx: &LateContext<'_>, arm: &Arm<'_>) -> bool {
+    if arm.guard.is_some() {
         return false;
     }
-    match pat_kind {
+    match arm.pat.kind {
         PatKind::Binding(..) | PatKind::Wild => true,
         PatKind::Path(ref qpath) => is_lang_ctor(cx, qpath, OptionNone),
         _ => false,

src/tools/clippy/clippy_lints/src/if_let_mutex.rs

@@ -59,7 +59,7 @@ impl<'tcx> LateLintPass<'tcx> for IfLetMutex {
             if_then,
             if_else: Some(if_else),
             ..
-        }) = higher::IfLet::hir(expr)
+        }) = higher::IfLet::hir(cx, expr)
         {
             op_visit.visit_expr(let_expr);
             if op_visit.mutex_lock_called {

src/tools/clippy/clippy_lints/src/if_let_some_result.rs

@@ -45,7 +45,7 @@ declare_lint_pass!(OkIfLet => [IF_LET_SOME_RESULT]);
 impl<'tcx> LateLintPass<'tcx> for OkIfLet {
     fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
         if_chain! { //begin checking variables
-            if let Some(higher::IfLet { let_pat, let_expr, .. }) = higher::IfLet::hir(expr);
+            if let Some(higher::IfLet { let_pat, let_expr, .. }) = higher::IfLet::hir(cx, expr);
             if let ExprKind::MethodCall(_, ok_span, ref result_types, _) = let_expr.kind; //check is expr.ok() has type Result<T,E>.ok(, _)
             if let PatKind::TupleStruct(QPath::Resolved(_, ref x), ref 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;

src/tools/clippy/clippy_lints/src/loops/manual_flatten.rs

@@ -37,7 +37,7 @@ pub(super) fn check<'tcx>(
 
         if_chain! {
             if let Some(inner_expr) = inner_expr;
-            if let Some(higher::IfLet { let_pat, let_expr, if_else: None, .. }) = higher::IfLet::hir(inner_expr);
+            if let Some(higher::IfLet { let_pat, let_expr, if_else: None, .. }) = higher::IfLet::hir(cx, inner_expr);
             // Ensure match_expr in `if let` statement is the same as the pat from the for-loop
             if let PatKind::Binding(_, pat_hir_id, _, _) = pat.kind;
             if path_to_local_id(let_expr, pat_hir_id);

src/tools/clippy/clippy_lints/src/loops/while_let_loop.rs

@@ -17,7 +17,7 @@ pub(super) fn check(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, loop_block: &'
             let_expr,
             if_else: Some(if_else),
             ..
-        }) = higher::IfLet::hir(inner)
+        }) = higher::IfLet::hir(cx, inner)
         {
             if is_simple_break_expr(if_else) {
                 could_be_while_let(cx, expr, let_pat, let_expr);

src/tools/clippy/clippy_lints/src/manual_map.rs

@@ -49,7 +49,7 @@ impl LateLintPass<'_> for ManualMap {
             let_expr,
             if_then,
             if_else: Some(if_else),
-        }) = higher::IfLet::hir(expr)
+        }) = higher::IfLet::hir(cx, expr)
         {
             manage_lint(cx, expr, (&let_pat.kind, if_then), (&PatKind::Wild, if_else), let_expr);
         }

src/tools/clippy/clippy_lints/src/matches.rs

@@ -8,9 +8,9 @@ use clippy_utils::sugg::Sugg;
 use clippy_utils::ty::{implements_trait, is_type_diagnostic_item, match_type, peel_mid_ty_refs};
 use clippy_utils::visitors::LocalUsedVisitor;
 use clippy_utils::{
-    get_parent_expr, in_macro, is_expn_of, is_lang_ctor, is_lint_allowed, is_refutable, is_wild, meets_msrv, msrvs,
-    path_to_local, path_to_local_id, peel_hir_pat_refs, peel_n_hir_expr_refs, recurse_or_patterns, remove_blocks,
-    strip_pat_refs,
+    get_parent_expr, in_macro, is_expn_of, is_lang_ctor, is_lint_allowed, is_refutable, is_unit_expr, is_wild,
+    meets_msrv, msrvs, path_to_local, path_to_local_id, peel_hir_pat_refs, peel_n_hir_expr_refs, recurse_or_patterns,
+    remove_blocks, strip_pat_refs,
 };
 use clippy_utils::{paths, search_same, SpanlessEq, SpanlessHash};
 use core::array;
@@ -634,7 +634,7 @@ impl<'tcx> LateLintPass<'tcx> for Matches {
         if let ExprKind::Match(ref ex, ref arms, _) = expr.kind {
             check_match_ref_pats(cx, ex, arms.iter().map(|el| el.pat), expr);
         }
-        if let Some(higher::IfLet { let_pat, let_expr, .. }) = higher::IfLet::hir(expr) {
+        if let Some(higher::IfLet { let_pat, let_expr, .. }) = higher::IfLet::hir(cx, expr) {
             check_match_ref_pats(cx, let_expr, once(let_pat), expr);
         }
     }
@@ -1298,7 +1298,7 @@ fn check_match_like_matches<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>)
         let_expr,
         if_then,
         if_else: Some(if_else),
-    }) = higher::IfLet::hir(expr)
+    }) = higher::IfLet::hir(cx, expr)
     {
         return find_matches_sugg(
             cx,
@@ -1672,14 +1672,6 @@ fn type_ranges(ranges: &[SpannedRange<Constant>]) -> TypedRanges {
         .collect()
 }
 
-fn is_unit_expr(expr: &Expr<'_>) -> bool {
-    match expr.kind {
-        ExprKind::Tup(v) if v.is_empty() => true,
-        ExprKind::Block(b, _) if b.stmts.is_empty() && b.expr.is_none() => true,
-        _ => false,
-    }
-}
-
 // Checks if arm has the form `None => None`
 fn is_none_arm(cx: &LateContext<'_>, arm: &Arm<'_>) -> bool {
     matches!(arm.pat.kind, PatKind::Path(ref qpath) if is_lang_ctor(cx, qpath, OptionNone))
@@ -1835,7 +1827,7 @@ mod redundant_pattern_match {
             let_pat,
             let_expr,
             ..
-        }) = higher::IfLet::ast(cx, expr)
+        }) = higher::IfLet::hir(cx, expr)
         {
             find_sugg_for_if_let(cx, expr, let_pat, let_expr, "if", if_else.is_some())
         }

src/tools/clippy/clippy_lints/src/option_if_let_else.rs

@@ -125,7 +125,7 @@ fn format_option_in_sugg(cx: &LateContext<'_>, cond_expr: &Expr<'_>, as_ref: boo
 fn detect_option_if_let_else<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>) -> Option<OptionIfLetElseOccurence> {
     if_chain! {
         if !in_macro(expr.span); // Don't lint macros, because it behaves weirdly
-        if let Some(higher::IfLet { let_pat, let_expr, if_then, if_else: Some(if_else) }) = higher::IfLet::hir(expr);
+        if let Some(higher::IfLet { let_pat, let_expr, if_then, if_else: Some(if_else) }) = higher::IfLet::hir(cx, expr);
         if !is_else_clause(cx.tcx, expr);
         if !is_result_ok(cx, let_expr); // Don't lint on Result::ok because a different lint does it already
         if let PatKind::TupleStruct(struct_qpath, [inner_pat], _) = &let_pat.kind;

src/tools/clippy/clippy_lints/src/question_mark.rs

@@ -97,7 +97,7 @@ impl QuestionMark {
 
     fn check_if_let_some_and_early_return_none(cx: &LateContext<'_>, expr: &Expr<'_>) {
         if_chain! {
-            if let Some(higher::IfLet { let_pat, let_expr, if_then, if_else: Some(if_else) }) = higher::IfLet::hir(expr);
+            if let Some(higher::IfLet { let_pat, let_expr, if_then, if_else: Some(if_else) }) = higher::IfLet::hir(cx, expr);
             if Self::is_option(cx, let_expr);
 
             if let PatKind::TupleStruct(ref path1, fields, None) = let_pat.kind;

src/tools/clippy/clippy_utils/src/higher.rs

@@ -79,15 +79,7 @@ pub struct IfLet<'hir> {
 }
 
 impl<'hir> IfLet<'hir> {
-    #[inline]
-    pub fn ast(cx: &LateContext<'tcx>, expr: &Expr<'hir>) -> Option<Self> {
-        let rslt = Self::hir(expr)?;
-        Self::is_not_within_while_context(cx, expr)?;
-        Some(rslt)
-    }
-
-    #[inline]
-    pub const fn hir(expr: &Expr<'hir>) -> Option<Self> {
+    pub fn hir(cx: &LateContext<'_>, expr: &Expr<'hir>) -> Option<Self> {
         if let ExprKind::If(
             Expr {
                 kind: ExprKind::Let(let_pat, let_expr, _),
@@ -97,6 +89,14 @@ impl<'hir> IfLet<'hir> {
             if_else,
         ) = expr.kind
         {
+            let hir = cx.tcx.hir();
+            let mut iter = hir.parent_iter(expr.hir_id);
+            if let Some((_, Node::Block(Block { stmts: [], .. }))) = iter.next() {
+                if let Some((_, Node::Expr(Expr { kind: ExprKind::Loop(_, _, LoopSource::While, _), .. }))) = iter.next() {
+                    // while loop desugar
+                    return None;
+                }
+            }
             return Some(Self {
                 let_pat,
                 let_expr,
@@ -106,22 +106,6 @@ impl<'hir> IfLet<'hir> {
         }
         None
     }
-
-    #[inline]
-    fn is_not_within_while_context(cx: &LateContext<'tcx>, expr: &Expr<'hir>) -> Option<()> {
-        let hir = cx.tcx.hir();
-        let parent = hir.get_parent_node(expr.hir_id);
-        let parent_parent = hir.get_parent_node(parent);
-        let parent_parent_node = hir.get(parent_parent);
-        if let Node::Expr(Expr {
-            kind: ExprKind::Loop(_, _, LoopSource::While, _),
-            ..
-        }) = parent_parent_node
-        {
-            return None;
-        }
-        Some(())
-    }
 }
 
 pub struct IfOrIfLet<'hir> {

src/tools/clippy/clippy_utils/src/lib.rs

@@ -254,6 +254,10 @@ pub fn in_macro(span: Span) -> bool {
     }
 }
 
+pub fn is_unit_expr(expr: &Expr<'_>) -> bool {
+    matches!(expr.kind, ExprKind::Block(Block { stmts: [], expr: None, .. }, _) | ExprKind::Tup([]))
+}
+
 /// Checks if given pattern is a wildcard (`_`)
 pub fn is_wild(pat: &Pat<'_>) -> bool {
     matches!(pat.kind, PatKind::Wild)