Auto merge of #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 #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> {