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 3b815a467bc0..b7e5344712eb 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 @@ -1,15 +1,10 @@ use std::iter::successors; -use either::Either; -use ide_db::{ - RootDatabase, - defs::NameClass, - syntax_helpers::node_ext::{is_pattern_cond, single_let}, - ty_filter::TryEnum, -}; +use ide_db::{RootDatabase, defs::NameClass, ty_filter::TryEnum}; use syntax::{ - AstNode, Edition, T, TextRange, + AstNode, Edition, SyntaxKind, T, TextRange, ast::{self, HasName, edit::IndentLevel, edit_in_place::Indent, syntax_factory::SyntaxFactory}, + syntax_editor::SyntaxEditor, }; use crate::{ @@ -54,42 +49,46 @@ pub(crate) fn replace_if_let_with_match(acc: &mut Assists, ctx: &AssistContext<' return None; } let mut else_block = None; + let indent = if_expr.indent_level(); let if_exprs = successors(Some(if_expr.clone()), |expr| match expr.else_branch()? { ast::ElseBranch::IfExpr(expr) => Some(expr), ast::ElseBranch::Block(block) => { + let block = unwrap_trivial_block(block).clone_for_update(); + block.reindent_to(IndentLevel(1)); else_block = Some(block); None } }); let scrutinee_to_be_expr = if_expr.condition()?; - let scrutinee_to_be_expr = match single_let(scrutinee_to_be_expr.clone()) { - Some(cond) => cond.expr()?, - None => scrutinee_to_be_expr, + let scrutinee_to_be_expr = match let_and_guard(&scrutinee_to_be_expr) { + (Some(let_expr), _) => let_expr.expr()?, + (None, cond) => cond?, }; let mut pat_seen = false; let mut cond_bodies = Vec::new(); for if_expr in if_exprs { let cond = if_expr.condition()?; - let cond = match single_let(cond.clone()) { - Some(let_) => { + let (cond, guard) = match let_and_guard(&cond) { + (None, guard) => (None, Some(guard?)), + (Some(let_), guard) => { let pat = let_.pat()?; let expr = let_.expr()?; - // FIXME: If one `let` is wrapped in parentheses and the second is not, - // we'll exit here. if scrutinee_to_be_expr.syntax().text() != expr.syntax().text() { // Only if all condition expressions are equal we can merge them into a match return None; } pat_seen = true; - Either::Left(pat) + (Some(pat), guard) } - // Multiple `let`, unsupported. - None if is_pattern_cond(cond.clone()) => return None, - None => Either::Right(cond), }; - let body = if_expr.then_branch()?; - cond_bodies.push((cond, body)); + if let Some(guard) = &guard { + guard.dedent(indent); + guard.indent(IndentLevel(1)); + } + let body = if_expr.then_branch()?.clone_for_update(); + body.indent(IndentLevel(1)); + cond_bodies.push((cond, guard, body)); } if !pat_seen && cond_bodies.len() != 1 { @@ -106,27 +105,25 @@ pub(crate) fn replace_if_let_with_match(acc: &mut Assists, ctx: &AssistContext<' available_range, move |builder| { let make = SyntaxFactory::with_mappings(); - let match_expr = { + let match_expr: ast::Expr = { let else_arm = make_else_arm(ctx, &make, else_block, &cond_bodies); - let make_match_arm = |(pat, body): (_, ast::BlockExpr)| { - let body = make.block_expr(body.statements(), body.tail_expr()); - body.indent(IndentLevel::from(1)); - let body = unwrap_trivial_block(body); - match pat { - Either::Left(pat) => make.match_arm(pat, None, body), - Either::Right(_) if !pat_seen => { - make.match_arm(make.literal_pat("true").into(), None, body) + let make_match_arm = + |(pat, guard, body): (_, Option, ast::BlockExpr)| { + body.reindent_to(IndentLevel::single()); + let body = unwrap_trivial_block(body); + match (pat, guard.map(|it| make.match_guard(it))) { + (Some(pat), guard) => make.match_arm(pat, guard, body), + (None, _) if !pat_seen => { + make.match_arm(make.literal_pat("true").into(), None, body) + } + (None, guard) => { + make.match_arm(make.wildcard_pat().into(), guard, body) + } } - Either::Right(expr) => make.match_arm( - make.wildcard_pat().into(), - Some(make.match_guard(expr)), - body, - ), - } - }; + }; let arms = cond_bodies.into_iter().map(make_match_arm).chain([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())); + match_expr.indent(indent); match_expr.into() }; @@ -134,7 +131,11 @@ pub(crate) fn replace_if_let_with_match(acc: &mut Assists, ctx: &AssistContext<' if_expr.syntax().parent().is_some_and(|it| ast::IfExpr::can_cast(it.kind())); let expr = if has_preceding_if_expr { // make sure we replace the `else if let ...` with a block so we don't end up with `else expr` - make.block_expr([], Some(match_expr)).into() + match_expr.dedent(indent); + match_expr.indent(IndentLevel(1)); + let block_expr = make.block_expr([], Some(match_expr)); + block_expr.indent(indent); + block_expr.into() } else { match_expr }; @@ -150,13 +151,13 @@ pub(crate) fn replace_if_let_with_match(acc: &mut Assists, ctx: &AssistContext<' fn make_else_arm( ctx: &AssistContext<'_>, make: &SyntaxFactory, - else_block: Option, - conditionals: &[(Either, ast::BlockExpr)], + else_expr: Option, + conditionals: &[(Option, Option, ast::BlockExpr)], ) -> ast::MatchArm { - let (pattern, expr) = if let Some(else_block) = else_block { + let (pattern, expr) = if let Some(else_expr) = else_expr { let pattern = match conditionals { - [(Either::Right(_), _)] => make.literal_pat("false").into(), - [(Either::Left(pat), _)] => match ctx + [(None, Some(_), _)] => make.literal_pat("false").into(), + [(Some(pat), _, _)] => match ctx .sema .type_of_pat(pat) .and_then(|ty| TryEnum::from_ty(&ctx.sema, &ty.adjusted())) @@ -174,10 +175,10 @@ fn make_else_arm( }, _ => make.wildcard_pat().into(), }; - (pattern, unwrap_trivial_block(else_block)) + (pattern, else_expr) } else { let pattern = match conditionals { - [(Either::Right(_), _)] => make.literal_pat("false").into(), + [(None, Some(_), _)] => make.literal_pat("false").into(), _ => make.wildcard_pat().into(), }; (pattern, make.expr_unit()) @@ -266,7 +267,10 @@ pub(crate) fn replace_match_with_if_let(acc: &mut Assists, ctx: &AssistContext<' // wrap them in another BlockExpr. match expr { ast::Expr::BlockExpr(block) if block.modifier().is_none() => block, - expr => make.block_expr([], Some(expr)), + expr => { + expr.indent(IndentLevel(1)); + make.block_expr([], Some(expr)) + } } }; @@ -289,7 +293,9 @@ pub(crate) fn replace_match_with_if_let(acc: &mut Assists, ctx: &AssistContext<' condition }; let then_expr = then_expr.clone_for_update(); + let else_expr = else_expr.clone_for_update(); then_expr.reindent_to(IndentLevel::single()); + else_expr.reindent_to(IndentLevel::single()); let then_block = make_block_expr(then_expr); let else_expr = if is_empty_expr(&else_expr) { None } else { Some(else_expr) }; let if_let_expr = make.expr_if( @@ -382,6 +388,48 @@ fn is_sad_pat(sema: &hir::Semantics<'_, RootDatabase>, pat: &ast::Pat) -> bool { .is_some_and(|it| does_pat_match_variant(pat, &it.sad_pattern())) } +fn let_and_guard(cond: &ast::Expr) -> (Option, Option) { + if let ast::Expr::ParenExpr(expr) = cond + && let Some(sub_expr) = expr.expr() + { + let_and_guard(&sub_expr) + } else if let ast::Expr::LetExpr(let_expr) = cond { + (Some(let_expr.clone()), None) + } else if let ast::Expr::BinExpr(bin_expr) = cond + && let Some(ast::Expr::LetExpr(let_expr)) = and_bin_expr_left(bin_expr).lhs() + { + let new_expr = bin_expr.clone_subtree(); + let mut edit = SyntaxEditor::new(new_expr.syntax().clone()); + + let left_bin = and_bin_expr_left(&new_expr); + if let Some(rhs) = left_bin.rhs() { + edit.replace(left_bin.syntax(), rhs.syntax()); + } else { + if let Some(next) = left_bin.syntax().next_sibling_or_token() + && next.kind() == SyntaxKind::WHITESPACE + { + edit.delete(next); + } + edit.delete(left_bin.syntax()); + } + + let new_expr = edit.finish().new_root().clone(); + (Some(let_expr), ast::Expr::cast(new_expr)) + } else { + (None, Some(cond.clone())) + } +} + +fn and_bin_expr_left(expr: &ast::BinExpr) -> ast::BinExpr { + if expr.op_kind() == Some(ast::BinaryOp::LogicOp(ast::LogicOp::And)) + && let Some(ast::Expr::BinExpr(left)) = expr.lhs() + { + and_bin_expr_left(&left) + } else { + expr.clone() + } +} + #[cfg(test)] mod tests { use super::*; @@ -452,6 +500,45 @@ pub fn foo(foo: bool) { ) } + #[test] + fn test_if_with_match_comments() { + check_assist( + replace_if_let_with_match, + r#" +pub fn foo(foo: i32) { + $0if let 1 = foo { + // some comment + self.foo(); + } else if let 2 = foo { + // some comment 2 + self.bar() + } else { + // some comment 3 + self.baz(); + } +} +"#, + r#" +pub fn foo(foo: i32) { + match foo { + 1 => { + // some comment + self.foo(); + } + 2 => { + // some comment 2 + self.bar() + } + _ => { + // some comment 3 + self.baz(); + } + } +} +"#, + ) + } + #[test] fn test_if_let_with_match_no_else() { check_assist( @@ -514,14 +601,151 @@ impl VariantData { #[test] fn test_if_let_with_match_let_chain() { - check_assist_not_applicable( + check_assist( replace_if_let_with_match, r#" +#![feature(if_let_guard)] +fn main() { + if $0let true = true && let Some(1) = None {} else { other() } +} +"#, + r#" +#![feature(if_let_guard)] +fn main() { + match true { + true if let Some(1) = None => {} + _ => other(), + } +} +"#, + ); + + check_assist( + replace_if_let_with_match, + r#" +#![feature(if_let_guard)] +fn main() { + if true { + $0if let ParenExpr(expr) = cond + && let Some(sub_expr) = expr.expr() + { + branch1( + "..." + ) + } else if let LetExpr(let_expr) = cond { + branch2( + "..." + ) + } else if let BinExpr(bin_expr) = cond + && let Some(kind) = bin_expr.op_kind() + && let Some(LetExpr(let_expr)) = foo(bin_expr) + { + branch3() + } else { + branch4( + "..." + ) + } + } +} +"#, + r#" +#![feature(if_let_guard)] +fn main() { + if true { + match cond { + ParenExpr(expr) if let Some(sub_expr) = expr.expr() => { + branch1( + "..." + ) + } + LetExpr(let_expr) => { + branch2( + "..." + ) + } + BinExpr(bin_expr) if let Some(kind) = bin_expr.op_kind() + && let Some(LetExpr(let_expr)) = foo(bin_expr) => branch3(), + _ => { + branch4( + "..." + ) + } + } + } +} +"#, + ); + + check_assist( + replace_if_let_with_match, + r#" +fn main() { + if $0let true = true + && true + && false + { + code() + } else { + other() + } +} +"#, + r#" +fn main() { + match true { + true if true + && false => code(), + _ => other(), + } +} +"#, + ); + } + + #[test] + fn test_if_let_with_match_let_chain_no_else() { + check_assist( + replace_if_let_with_match, + r#" +#![feature(if_let_guard)] fn main() { if $0let true = true && let Some(1) = None {} } "#, - ) + r#" +#![feature(if_let_guard)] +fn main() { + match true { + true if let Some(1) = None => {} + _ => (), + } +} +"#, + ); + + check_assist( + replace_if_let_with_match, + r#" +fn main() { + if $0let true = true + && true + && false + { + code() + } +} +"#, + r#" +fn main() { + match true { + true if true + && false => code(), + _ => (), + } +} +"#, + ); } #[test] @@ -553,10 +777,10 @@ impl VariantData { VariantData::Tuple(..) => false, _ if cond() => true, _ => { - bar( - 123 - ) - } + bar( + 123 + ) + } } } } @@ -587,11 +811,11 @@ impl VariantData { if let VariantData::Struct(..) = *self { true } else { - match *self { - VariantData::Tuple(..) => false, - _ => false, + match *self { + VariantData::Tuple(..) => false, + _ => false, + } } -} } } "#, @@ -706,9 +930,12 @@ fn foo(x: Result) { fn main() { if true { $0if let Ok(rel_path) = path.strip_prefix(root_path) { - let rel_path = RelativePathBuf::from_path(rel_path).ok()?; + let rel_path = RelativePathBuf::from_path(rel_path) + .ok()?; Some((*id, rel_path)) } else { + let _ = some_code() + .clone(); None } } @@ -719,10 +946,52 @@ fn main() { if true { match path.strip_prefix(root_path) { Ok(rel_path) => { - let rel_path = RelativePathBuf::from_path(rel_path).ok()?; + let rel_path = RelativePathBuf::from_path(rel_path) + .ok()?; Some((*id, rel_path)) } - _ => None, + _ => { + let _ = some_code() + .clone(); + None + } + } + } +} +"#, + ); + + check_assist( + replace_if_let_with_match, + r#" +fn main() { + if true { + $0if let Ok(rel_path) = path.strip_prefix(root_path) { + Foo { + x: 1 + } + } else { + Foo { + x: 2 + } + } + } +} +"#, + r#" +fn main() { + if true { + match path.strip_prefix(root_path) { + Ok(rel_path) => { + Foo { + x: 1 + } + } + _ => { + Foo { + x: 2 + } + } } } } @@ -1581,13 +1850,51 @@ fn foo(x: Result) { replace_match_with_if_let, r#" fn main() { + if true { + $0match path.strip_prefix(root_path) { + Ok(rel_path) => Foo { + x: 2 + } + _ => Foo { + x: 3 + }, + } + } +} +"#, + r#" +fn main() { + if true { + if let Ok(rel_path) = path.strip_prefix(root_path) { + Foo { + x: 2 + } + } else { + Foo { + x: 3 + } + } + } +} +"#, + ); + + check_assist( + replace_match_with_if_let, + r#" +fn main() { if true { $0match path.strip_prefix(root_path) { Ok(rel_path) => { - let rel_path = RelativePathBuf::from_path(rel_path).ok()?; + let rel_path = RelativePathBuf::from_path(rel_path) + .ok()?; Some((*id, rel_path)) } - _ => None, + _ => { + let _ = some_code() + .clone(); + None + }, } } } @@ -1596,15 +1903,18 @@ fn main() { fn main() { if true { if let Ok(rel_path) = path.strip_prefix(root_path) { - let rel_path = RelativePathBuf::from_path(rel_path).ok()?; + let rel_path = RelativePathBuf::from_path(rel_path) + .ok()?; Some((*id, rel_path)) } else { + let _ = some_code() + .clone(); None } } } "#, - ) + ); } #[test] diff --git a/crates/ide-assists/src/utils.rs b/crates/ide-assists/src/utils.rs index e43516f6b963..fbdd0667b94c 100644 --- a/crates/ide-assists/src/utils.rs +++ b/crates/ide-assists/src/utils.rs @@ -57,6 +57,14 @@ pub fn extract_trivial_expression(block_expr: &ast::BlockExpr) -> Option