Merge #9465

9465: feat: `replace_match_with_if_let` works on more 2-arm matches r=Veykril a=Veykril

Now it works on pretty much on all 2-arm matches where only up to 1 arm binds a name instead of requiring either a sad or wildcard pattern to be present.

bors r+

Co-authored-by: Lukas Wirth <[email protected]>

Author: bors[bot]

crates/ide/src/hover.rs

@@ -4137,6 +4137,7 @@ pub fn foo() {}
 
     #[test]
     fn hover_attr_path_qualifier() {
+        cov_mark::check!(name_ref_classify_attr_path_qualifier);
         check(
             r#"
 //- /foo.rs crate:foo

crates/ide_assists/src/handlers/replace_if_let_with_match.rs

@@ -86,53 +86,21 @@ pub(crate) fn replace_if_let_with_match(acc: &mut Assists, ctx: &AssistContext)
         target,
         move |edit| {
             let match_expr = {
-                let else_arm = {
-                    match else_block {
-                        Some(else_block) => {
-                            let pattern = match &*cond_bodies {
-                                [(Either::Left(pat), _)] => ctx
-                                    .sema
-                                    .type_of_pat(&pat)
-                                    .and_then(|ty| TryEnum::from_ty(&ctx.sema, &ty))
-                                    .map(|it| {
-                                        if does_pat_match_variant(&pat, &it.sad_pattern()) {
-                                            it.happy_pattern()
-                                        } else {
-                                            it.sad_pattern()
-                                        }
-                                    }),
-                                _ => None,
-                            }
-                            .unwrap_or_else(|| make::wildcard_pat().into());
-                            make::match_arm(
-                                iter::once(pattern),
-                                None,
-                                unwrap_trivial_block(else_block),
-                            )
+                let else_arm = make_else_arm(else_block, &cond_bodies, ctx);
+                let make_match_arm = |(pat, body): (_, ast::BlockExpr)| {
+                    let body = body.reset_indent().indent(IndentLevel(1));
+                    match pat {
+                        Either::Left(pat) => {
+                            make::match_arm(iter::once(pat), None, unwrap_trivial_block(body))
                         }
-                        None => make::match_arm(
+                        Either::Right(expr) => make::match_arm(
                             iter::once(make::wildcard_pat().into()),
-                            None,
-                            make::expr_unit().into(),
+                            Some(expr),
+                            unwrap_trivial_block(body),
                         ),
                     }
                 };
-                let arms = cond_bodies
-                    .into_iter()
-                    .map(|(pat, body)| {
-                        let body = body.reset_indent().indent(IndentLevel(1));
-                        match pat {
-                            Either::Left(pat) => {
-                                make::match_arm(iter::once(pat), None, unwrap_trivial_block(body))
-                            }
-                            Either::Right(expr) => make::match_arm(
-                                iter::once(make::wildcard_pat().into()),
-                                Some(expr),
-                                unwrap_trivial_block(body),
-                            ),
-                        }
-                    })
-                    .chain(iter::once(else_arm));
+                let arms = cond_bodies.into_iter().map(make_match_arm).chain(iter::once(else_arm));
                 let match_expr = make::expr_match(scrutinee_to_be_expr, make::match_arm_list(arms));
                 match_expr.indent(IndentLevel::from_node(if_expr.syntax()))
             };
@@ -150,6 +118,36 @@ pub(crate) fn replace_if_let_with_match(acc: &mut Assists, ctx: &AssistContext)
     )
 }
 
+fn make_else_arm(
+    else_block: Option<ast::BlockExpr>,
+    cond_bodies: &Vec<(Either<ast::Pat, ast::Expr>, ast::BlockExpr)>,
+    ctx: &AssistContext,
+) -> ast::MatchArm {
+    if let Some(else_block) = else_block {
+        let pattern = if let [(Either::Left(pat), _)] = &**cond_bodies {
+            ctx.sema
+                .type_of_pat(&pat)
+                .and_then(|ty| TryEnum::from_ty(&ctx.sema, &ty))
+                .zip(Some(pat))
+        } else {
+            None
+        };
+        let pattern = match pattern {
+            Some((it, pat)) => {
+                if does_pat_match_variant(&pat, &it.sad_pattern()) {
+                    it.happy_pattern()
+                } else {
+                    it.sad_pattern()
+                }
+            }
+            None => make::wildcard_pat().into(),
+        };
+        make::match_arm(iter::once(pattern), None, unwrap_trivial_block(else_block))
+    } else {
+        make::match_arm(iter::once(make::wildcard_pat().into()), None, make::expr_unit().into())
+    }
+}
+
 // Assist: replace_match_with_if_let
 //
 // Replaces a binary `match` with a wildcard pattern and no guards with an `if let` expression.
@@ -178,48 +176,47 @@ pub(crate) fn replace_if_let_with_match(acc: &mut Assists, ctx: &AssistContext)
 // ```
 pub(crate) fn replace_match_with_if_let(acc: &mut Assists, ctx: &AssistContext) -> Option<()> {
     let match_expr: ast::MatchExpr = ctx.find_node_at_offset()?;
+
     let mut arms = match_expr.match_arm_list()?.arms();
-    let first_arm = arms.next()?;
-    let second_arm = arms.next()?;
+    let (first_arm, second_arm) = (arms.next()?, arms.next()?);
     if arms.next().is_some() || first_arm.guard().is_some() || second_arm.guard().is_some() {
         return None;
     }
-    let condition_expr = match_expr.expr()?;
-    let (if_let_pat, then_expr, else_expr) = if is_pat_wildcard_or_sad(&ctx.sema, &first_arm.pat()?)
-    {
-        (second_arm.pat()?, second_arm.expr()?, first_arm.expr()?)
-    } else if is_pat_wildcard_or_sad(&ctx.sema, &second_arm.pat()?) {
-        (first_arm.pat()?, first_arm.expr()?, second_arm.expr()?)
-    } else {
-        return None;
-    };
+
+    let (if_let_pat, then_expr, else_expr) = pick_pattern_and_expr_order(
+        &ctx.sema,
+        first_arm.pat()?,
+        second_arm.pat()?,
+        first_arm.expr()?,
+        second_arm.expr()?,
+    )?;
+    let scrutinee = match_expr.expr()?;
 
     let target = match_expr.syntax().text_range();
     acc.add(
         AssistId("replace_match_with_if_let", AssistKind::RefactorRewrite),
         "Replace with if let",
         target,
         move |edit| {
-            let condition = make::condition(condition_expr, Some(if_let_pat));
+            let condition = make::condition(scrutinee, Some(if_let_pat));
             let then_block = match then_expr.reset_indent() {
                 ast::Expr::BlockExpr(block) => block,
                 expr => make::block_expr(iter::empty(), Some(expr)),
             };
             let else_expr = match else_expr {
-                ast::Expr::BlockExpr(block)
-                    if block.statements().count() == 0 && block.tail_expr().is_none() =>
-                {
-                    None
-                }
-                ast::Expr::TupleExpr(tuple) if tuple.fields().count() == 0 => None,
+                ast::Expr::BlockExpr(block) if block.is_empty() => None,
+                ast::Expr::TupleExpr(tuple) if tuple.fields().next().is_none() => None,
                 expr => Some(expr),
             };
             let if_let_expr = make::expr_if(
                 condition,
                 then_block,
-                else_expr.map(|else_expr| {
-                    ast::ElseBranch::Block(make::block_expr(iter::empty(), Some(else_expr)))
-                }),
+                else_expr
+                    .map(|expr| match expr {
+                        ast::Expr::BlockExpr(block) => block,
+                        expr => (make::block_expr(iter::empty(), Some(expr))),
+                    })
+                    .map(ast::ElseBranch::Block),
             )
             .indent(IndentLevel::from_node(match_expr.syntax()));
 
@@ -228,11 +225,51 @@ pub(crate) fn replace_match_with_if_let(acc: &mut Assists, ctx: &AssistContext)
     )
 }
 
-fn is_pat_wildcard_or_sad(sema: &hir::Semantics<RootDatabase>, pat: &ast::Pat) -> bool {
+/// Pick the pattern for the if let condition and return the expressions for the `then` body and `else` body in that order.
+fn pick_pattern_and_expr_order(
+    sema: &hir::Semantics<RootDatabase>,
+    pat: ast::Pat,
+    pat2: ast::Pat,
+    expr: ast::Expr,
+    expr2: ast::Expr,
+) -> Option<(ast::Pat, ast::Expr, ast::Expr)> {
+    let res = match (pat, pat2) {
+        (ast::Pat::WildcardPat(_), _) => return None,
+        (pat, sad_pat) if is_sad_pat(sema, &sad_pat) => (pat, expr, expr2),
+        (sad_pat, pat) if is_sad_pat(sema, &sad_pat) => (pat, expr2, expr),
+        (pat, pat2) => match (binds_name(&pat), binds_name(&pat2)) {
+            (true, true) => return None,
+            (true, false) => (pat, expr, expr2),
+            (false, true) => (pat2, expr2, expr),
+            (false, false) => (pat, expr, expr2),
+        },
+    };
+    Some(res)
+}
+
+fn binds_name(pat: &ast::Pat) -> bool {
+    let binds_name_v = |pat| binds_name(&pat);
+    match pat {
+        ast::Pat::IdentPat(_) => true,
+        ast::Pat::MacroPat(_) => true,
+        ast::Pat::OrPat(pat) => pat.pats().any(binds_name_v),
+        ast::Pat::SlicePat(pat) => pat.pats().any(binds_name_v),
+        ast::Pat::TuplePat(it) => it.fields().any(binds_name_v),
+        ast::Pat::TupleStructPat(it) => it.fields().any(binds_name_v),
+        ast::Pat::RecordPat(it) => it
+            .record_pat_field_list()
+            .map_or(false, |rpfl| rpfl.fields().flat_map(|rpf| rpf.pat()).any(binds_name_v)),
+        ast::Pat::RefPat(pat) => pat.pat().map_or(false, binds_name_v),
+        ast::Pat::BoxPat(pat) => pat.pat().map_or(false, binds_name_v),
+        ast::Pat::ParenPat(pat) => pat.pat().map_or(false, binds_name_v),
+        _ => false,
+    }
+}
+
+fn is_sad_pat(sema: &hir::Semantics<RootDatabase>, pat: &ast::Pat) -> bool {
     sema.type_of_pat(pat)
         .and_then(|ty| TryEnum::from_ty(sema, &ty))
-        .map(|it| it.sad_pattern().syntax().text() == pat.syntax().text())
-        .unwrap_or_else(|| matches!(pat, ast::Pat::WildcardPat(_)))
+        .map_or(false, |it| does_pat_match_variant(pat, &it.sad_pattern()))
 }
 
 #[cfg(test)]
@@ -664,4 +701,79 @@ fn main() {
 "#,
         )
     }
+
+    #[test]
+    fn replace_match_with_if_let_exhaustive() {
+        check_assist(
+            replace_match_with_if_let,
+            r#"
+fn print_source(def_source: ModuleSource) {
+    match def_so$0urce {
+        ModuleSource::SourceFile(..) => { println!("source file"); }
+        ModuleSource::Module(..) => { println!("module"); }
+    }
+}
+"#,
+            r#"
+fn print_source(def_source: ModuleSource) {
+    if let ModuleSource::SourceFile(..) = def_source { println!("source file"); } else { println!("module"); }
+}
+"#,
+        )
+    }
+
+    #[test]
+    fn replace_match_with_if_let_prefer_name_bind() {
+        check_assist(
+            replace_match_with_if_let,
+            r#"
+fn foo() {
+    match $0Foo(0) {
+        Foo(_) => (),
+        Bar(bar) => println!("bar {}", bar),
+    }
+}
+"#,
+            r#"
+fn foo() {
+    if let Bar(bar) = Foo(0) {
+        println!("bar {}", bar)
+    }
+}
+"#,
+        );
+        check_assist(
+            replace_match_with_if_let,
+            r#"
+fn foo() {
+    match $0Foo(0) {
+        Bar(bar) => println!("bar {}", bar),
+        Foo(_) => (),
+    }
+}
+"#,
+            r#"
+fn foo() {
+    if let Bar(bar) = Foo(0) {
+        println!("bar {}", bar)
+    }
+}
+"#,
+        );
+    }
+
+    #[test]
+    fn replace_match_with_if_let_rejects_double_name_bindings() {
+        check_assist_not_applicable(
+            replace_match_with_if_let,
+            r#"
+fn foo() {
+    match $0Foo(0) {
+        Foo(foo) => println!("bar {}", foo),
+        Bar(bar) => println!("bar {}", bar),
+    }
+}
+"#,
+        );
+    }
 }

crates/ide_db/src/defs.rs

@@ -395,6 +395,7 @@ impl NameRefClass {
                         // Don't wanna collide with builtin attributes here like `test` hence guard
                         // so only resolve to modules that aren't the last segment
                         PathResolution::Def(module @ ModuleDef::Module(_)) if path != top_path => {
+                            cov_mark::hit!(name_ref_classify_attr_path_qualifier);
                             Some(NameRefClass::Definition(Definition::ModuleDef(module)))
                         }
                         PathResolution::Macro(mac) if mac.kind() == hir::MacroKind::Attr => {

crates/syntax/src/ast/node_ext.rs

@@ -49,6 +49,10 @@ impl ast::BlockExpr {
     pub fn items(&self) -> AstChildren<ast::Item> {
         support::children(self.syntax())
     }
+
+    pub fn is_empty(&self) -> bool {
+        self.statements().next().is_none() && self.tail_expr().is_none()
+    }
 }
 
 impl ast::Expr {