From a7d61ddba4726ce941fabe594a947c70ffb7a811 Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Fri, 2 Jul 2021 19:34:49 +0200 Subject: [PATCH 1/4] Add cov_mark --- crates/ide/src/hover.rs | 1 + crates/ide_db/src/defs.rs | 1 + 2 files changed, 2 insertions(+) diff --git a/crates/ide/src/hover.rs b/crates/ide/src/hover.rs index f5fa850808..855220449b 100644 --- a/crates/ide/src/hover.rs +++ b/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 diff --git a/crates/ide_db/src/defs.rs b/crates/ide_db/src/defs.rs index a6c6db6c06..2accc735fe 100644 --- a/crates/ide_db/src/defs.rs +++ b/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 => { From f1819525f57c31da6632cd81d16f7eb0f54805e6 Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Fri, 2 Jul 2021 19:50:37 +0200 Subject: [PATCH 2/4] Simplify --- .../src/handlers/replace_if_let_with_match.rs | 82 +++++++++---------- 1 file changed, 40 insertions(+), 42 deletions(-) diff --git a/crates/ide_assists/src/handlers/replace_if_let_with_match.rs b/crates/ide_assists/src/handlers/replace_if_let_with_match.rs index 4934bb4187..a84f8f1207 100644 --- a/crates/ide_assists/src/handlers/replace_if_let_with_match.rs +++ b/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, + cond_bodies: &Vec<(Either, 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. From 251f0c6090e98a2367e14a5505e849d9c883b5a5 Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Fri, 2 Jul 2021 21:05:10 +0200 Subject: [PATCH 3/4] `replace_match_with_if_let` works on more binary matches --- .../src/handlers/replace_if_let_with_match.rs | 161 +++++++++++++++--- crates/syntax/src/ast/node_ext.rs | 4 + 2 files changed, 141 insertions(+), 24 deletions(-) diff --git a/crates/ide_assists/src/handlers/replace_if_let_with_match.rs b/crates/ide_assists/src/handlers/replace_if_let_with_match.rs index a84f8f1207..b1c5f44213 100644 --- a/crates/ide_assists/src/handlers/replace_if_let_with_match.rs +++ b/crates/ide_assists/src/handlers/replace_if_let_with_match.rs @@ -176,21 +176,21 @@ fn make_else_arm( // ``` 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( @@ -198,26 +198,25 @@ pub(crate) fn replace_match_with_if_let(acc: &mut Assists, ctx: &AssistContext) "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())); @@ -226,11 +225,50 @@ pub(crate) fn replace_match_with_if_let(acc: &mut Assists, ctx: &AssistContext) ) } -fn is_pat_wildcard_or_sad(sema: &hir::Semantics, 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, + 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, 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| it.sad_pattern().syntax().text() == pat.syntax().text()) } #[cfg(test)] @@ -662,4 +700,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), + } +} +"#, + ); + } } diff --git a/crates/syntax/src/ast/node_ext.rs b/crates/syntax/src/ast/node_ext.rs index 49c478b817..d8c5c4e76f 100644 --- a/crates/syntax/src/ast/node_ext.rs +++ b/crates/syntax/src/ast/node_ext.rs @@ -49,6 +49,10 @@ impl ast::BlockExpr { pub fn items(&self) -> AstChildren { support::children(self.syntax()) } + + pub fn is_empty(&self) -> bool { + self.statements().next().is_none() && self.tail_expr().is_none() + } } impl ast::Expr { From eb3f90b301144434c197e09bd190fa8d234ef24d Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Fri, 2 Jul 2021 21:10:38 +0200 Subject: [PATCH 4/4] Don't check sad pattern equality by text --- crates/ide_assists/src/handlers/replace_if_let_with_match.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/ide_assists/src/handlers/replace_if_let_with_match.rs b/crates/ide_assists/src/handlers/replace_if_let_with_match.rs index b1c5f44213..9ce3ea694b 100644 --- a/crates/ide_assists/src/handlers/replace_if_let_with_match.rs +++ b/crates/ide_assists/src/handlers/replace_if_let_with_match.rs @@ -265,10 +265,11 @@ fn binds_name(pat: &ast::Pat) -> bool { _ => false, } } + fn is_sad_pat(sema: &hir::Semantics, pat: &ast::Pat) -> bool { sema.type_of_pat(pat) .and_then(|ty| TryEnum::from_ty(sema, &ty)) - .map_or(false, |it| it.sad_pattern().syntax().text() == pat.syntax().text()) + .map_or(false, |it| does_pat_match_variant(pat, &it.sad_pattern())) } #[cfg(test)]