From 70a91cf218ef5ff48930dddace7c85de38671bb8 Mon Sep 17 00:00:00 2001 From: Giga Bowser <45986823+Giga-Bowser@users.noreply.github.com> Date: Wed, 20 Nov 2024 11:56:23 -0500 Subject: [PATCH 1/3] minor: Take `ast::MatchGuard` instead of `ast::Expr` in `make::match_arm` --- .../ide-assists/src/handlers/replace_if_let_with_match.rs | 4 ++-- crates/ide-assists/src/handlers/unmerge_match_arm.rs | 8 ++------ crates/syntax/src/ast/make.rs | 8 ++++++-- 3 files changed, 10 insertions(+), 10 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 f13b0b0713d5..07904dd5658c 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 @@ -90,7 +90,7 @@ pub(crate) fn replace_if_let_with_match(acc: &mut Assists, ctx: &AssistContext<' } // Multiple `let`, unsupported. None if is_pattern_cond(cond.clone()) => return None, - None => Either::Right(cond), + None => Either::Right(make::match_guard(cond)), }; let body = if_expr.then_branch()?; cond_bodies.push((cond, body)); @@ -150,7 +150,7 @@ pub(crate) fn replace_if_let_with_match(acc: &mut Assists, ctx: &AssistContext<' fn make_else_arm( ctx: &AssistContext<'_>, else_block: Option, - conditionals: &[(Either, ast::BlockExpr)], + conditionals: &[(Either, ast::BlockExpr)], ) -> ast::MatchArm { let (pattern, expr) = if let Some(else_block) = else_block { let pattern = match conditionals { diff --git a/crates/ide-assists/src/handlers/unmerge_match_arm.rs b/crates/ide-assists/src/handlers/unmerge_match_arm.rs index 648bf358b4bb..a6eb38295ea2 100644 --- a/crates/ide-assists/src/handlers/unmerge_match_arm.rs +++ b/crates/ide-assists/src/handlers/unmerge_match_arm.rs @@ -55,12 +55,8 @@ pub(crate) fn unmerge_match_arm(acc: &mut Assists, ctx: &AssistContext<'_>) -> O .siblings_with_tokens(Direction::Next) .filter_map(|it| ast::Pat::cast(it.into_node()?)); // FIXME: We should add a leading pipe if the original arm has one. - let new_match_arm = make::match_arm( - pats_after, - match_arm.guard().and_then(|guard| guard.condition()), - match_arm_body, - ) - .clone_for_update(); + let new_match_arm = + make::match_arm(pats_after, match_arm.guard(), match_arm_body).clone_for_update(); let mut pipe_index = pipe_token.index(); if pipe_token diff --git a/crates/syntax/src/ast/make.rs b/crates/syntax/src/ast/make.rs index 2ec83d23b27c..2f117f835ee8 100644 --- a/crates/syntax/src/ast/make.rs +++ b/crates/syntax/src/ast/make.rs @@ -786,12 +786,12 @@ pub fn path_pat(path: ast::Path) -> ast::Pat { pub fn match_arm( pats: impl IntoIterator, - guard: Option, + guard: Option, expr: ast::Expr, ) -> ast::MatchArm { let pats_str = pats.into_iter().join(" | "); return match guard { - Some(guard) => from_text(&format!("{pats_str} if {guard} => {expr}")), + Some(guard) => from_text(&format!("{pats_str} {guard} => {expr}")), None => from_text(&format!("{pats_str} => {expr}")), }; @@ -800,6 +800,10 @@ pub fn match_arm( } } +pub fn match_guard(condition: ast::Expr) -> ast::MatchGuard { + ast_from_text(&format!("fn f() {{ match () {{() if {condition} => ()}} }}")) +} + pub fn match_arm_with_guard( pats: impl IntoIterator, guard: ast::Expr, From ef32eba01b759a3cdf7c2c63b7a34776e3b31ddd Mon Sep 17 00:00:00 2001 From: Giga Bowser <45986823+Giga-Bowser@users.noreply.github.com> Date: Wed, 20 Nov 2024 12:04:25 -0500 Subject: [PATCH 2/3] minor: Handle match arm commas in `make::match_arm` --- .../src/handlers/unmerge_match_arm.rs | 23 ++++--------------- crates/syntax/src/ast/make.rs | 7 +++--- crates/syntax/src/syntax_editor.rs | 2 +- 3 files changed, 9 insertions(+), 23 deletions(-) diff --git a/crates/ide-assists/src/handlers/unmerge_match_arm.rs b/crates/ide-assists/src/handlers/unmerge_match_arm.rs index a6eb38295ea2..354f1af8e7ed 100644 --- a/crates/ide-assists/src/handlers/unmerge_match_arm.rs +++ b/crates/ide-assists/src/handlers/unmerge_match_arm.rs @@ -1,5 +1,4 @@ use syntax::{ - algo::neighbor, ast::{self, edit::IndentLevel, make, AstNode}, ted::{self, Position}, Direction, SyntaxKind, T, @@ -80,15 +79,8 @@ pub(crate) fn unmerge_match_arm(acc: &mut Assists, ctx: &AssistContext<'_>) -> O // body is a block, but we don't bother to check that. // - Missing after the arm with arms after, if the arm body is a block. In this case // we don't want to insert a comma at all. - let has_comma_after = - std::iter::successors(match_arm.syntax().last_child_or_token(), |it| { - it.prev_sibling_or_token() - }) - .map(|it| it.kind()) - .find(|it| !it.is_trivia()) - == Some(T![,]); - let has_arms_after = neighbor(&match_arm, Direction::Next).is_some(); - if !has_comma_after && !has_arms_after { + let has_comma_after = match_arm.comma_token().is_some(); + if !has_comma_after && !match_arm.expr().unwrap().is_block_like() { insert_after_old_arm.push(make::token(T![,]).into()); } @@ -99,13 +91,6 @@ pub(crate) fn unmerge_match_arm(acc: &mut Assists, ctx: &AssistContext<'_>) -> O ted::insert_all_raw(Position::after(match_arm.syntax()), insert_after_old_arm); - if has_comma_after { - ted::insert_raw( - Position::last_child_of(new_match_arm.syntax()), - make::token(T![,]), - ); - } - edit.replace(old_parent_range, new_parent.to_string()); }, ) @@ -252,7 +237,7 @@ fn main() { let x = X::A; let y = match x { X::A => 1i32, - X::B => 1i32 + X::B => 1i32, }; } "#, @@ -282,7 +267,7 @@ fn main() { let x = X::A; match x { X::A => {}, - X::B => {}, + X::B => {} } } "#, diff --git a/crates/syntax/src/ast/make.rs b/crates/syntax/src/ast/make.rs index 2f117f835ee8..1499750eabe9 100644 --- a/crates/syntax/src/ast/make.rs +++ b/crates/syntax/src/ast/make.rs @@ -789,10 +789,11 @@ pub fn match_arm( guard: Option, expr: ast::Expr, ) -> ast::MatchArm { + let comma_str = if expr.is_block_like() { "" } else { "," }; let pats_str = pats.into_iter().join(" | "); return match guard { - Some(guard) => from_text(&format!("{pats_str} {guard} => {expr}")), - None => from_text(&format!("{pats_str} => {expr}")), + Some(guard) => from_text(&format!("{pats_str} {guard} => {expr}{comma_str}")), + None => from_text(&format!("{pats_str} => {expr}{comma_str}")), }; fn from_text(text: &str) -> ast::MatchArm { @@ -820,7 +821,7 @@ pub fn match_arm_with_guard( pub fn match_arm_list(arms: impl IntoIterator) -> ast::MatchArmList { let arms_str = arms.into_iter().fold(String::new(), |mut acc, arm| { let needs_comma = arm.expr().map_or(true, |it| !it.is_block_like()); - let comma = if needs_comma { "," } else { "" }; + let comma = if needs_comma && arm.comma_token().is_none() { "," } else { "" }; let arm = arm.syntax(); format_to_acc!(acc, " {arm}{comma}\n") }); diff --git a/crates/syntax/src/syntax_editor.rs b/crates/syntax/src/syntax_editor.rs index 714f5a991114..72584981231c 100644 --- a/crates/syntax/src/syntax_editor.rs +++ b/crates/syntax/src/syntax_editor.rs @@ -380,7 +380,7 @@ mod tests { _ => { let var_name = 2 + 2; (var_name, true) - }"#]]; + },"#]]; expect.assert_eq(&edit.new_root.to_string()); assert_eq!(edit.find_annotation(placeholder_snippet).len(), 2); From 1f033e3194bcbda1667dfe40e965e066b713e9c1 Mon Sep 17 00:00:00 2001 From: Giga Bowser <45986823+Giga-Bowser@users.noreply.github.com> Date: Wed, 20 Nov 2024 12:54:46 -0500 Subject: [PATCH 3/3] feat: Add assist to remove braces from closures and match arms --- crates/ide-assists/src/handlers/add_braces.rs | 159 --------- .../ide-assists/src/handlers/change_braces.rs | 310 ++++++++++++++++++ crates/ide-assists/src/lib.rs | 4 +- crates/ide-assists/src/tests/generated.rs | 25 ++ .../src/ast/syntax_factory/constructors.rs | 24 ++ 5 files changed, 361 insertions(+), 161 deletions(-) delete mode 100644 crates/ide-assists/src/handlers/add_braces.rs create mode 100644 crates/ide-assists/src/handlers/change_braces.rs diff --git a/crates/ide-assists/src/handlers/add_braces.rs b/crates/ide-assists/src/handlers/add_braces.rs deleted file mode 100644 index 42f615e71daf..000000000000 --- a/crates/ide-assists/src/handlers/add_braces.rs +++ /dev/null @@ -1,159 +0,0 @@ -use syntax::{ - ast::{self, edit_in_place::Indent, syntax_factory::SyntaxFactory}, - AstNode, -}; - -use crate::{AssistContext, AssistId, AssistKind, Assists}; - -// Assist: add_braces -// -// Adds braces to lambda and match arm expressions. -// -// ``` -// fn foo(n: i32) -> i32 { -// match n { -// 1 =>$0 n + 1, -// _ => 0 -// } -// } -// ``` -// -> -// ``` -// fn foo(n: i32) -> i32 { -// match n { -// 1 => { -// n + 1 -// }, -// _ => 0 -// } -// } -// ``` -pub(crate) fn add_braces(acc: &mut Assists, ctx: &AssistContext<'_>) -> Option<()> { - let (expr_type, expr) = get_replacement_node(ctx)?; - - acc.add( - AssistId("add_braces", AssistKind::RefactorRewrite), - match expr_type { - ParentType::ClosureExpr => "Add braces to closure body", - ParentType::MatchArmExpr => "Add braces to arm expression", - }, - expr.syntax().text_range(), - |builder| { - let make = SyntaxFactory::new(); - let mut editor = builder.make_editor(expr.syntax()); - - let block_expr = make.block_expr(None, Some(expr.clone())); - block_expr.indent(expr.indent_level()); - - editor.replace(expr.syntax(), block_expr.syntax()); - - editor.add_mappings(make.finish_with_mappings()); - builder.add_file_edits(ctx.file_id(), editor); - }, - ) -} - -enum ParentType { - MatchArmExpr, - ClosureExpr, -} - -fn get_replacement_node(ctx: &AssistContext<'_>) -> Option<(ParentType, ast::Expr)> { - if let Some(match_arm) = ctx.find_node_at_offset::() { - let match_arm_expr = match_arm.expr()?; - - if matches!(match_arm_expr, ast::Expr::BlockExpr(_)) { - return None; - } - - return Some((ParentType::MatchArmExpr, match_arm_expr)); - } else if let Some(closure_expr) = ctx.find_node_at_offset::() { - let body = closure_expr.body()?; - - if matches!(body, ast::Expr::BlockExpr(_)) { - return None; - } - - return Some((ParentType::ClosureExpr, body)); - } - - None -} - -#[cfg(test)] -mod tests { - use crate::tests::{check_assist, check_assist_not_applicable}; - - use super::*; - - #[test] - fn suggest_add_braces_for_closure() { - check_assist( - add_braces, - r#" -fn foo() { - t(|n|$0 n + 100); -} -"#, - r#" -fn foo() { - t(|n| { - n + 100 - }); -} -"#, - ); - } - - #[test] - fn no_assist_for_closures_with_braces() { - check_assist_not_applicable( - add_braces, - r#" -fn foo() { - t(|n|$0 { n + 100 }); -} -"#, - ); - } - - #[test] - fn suggest_add_braces_for_match() { - check_assist( - add_braces, - r#" -fn foo() { - match n { - Some(n) $0=> 29, - _ => () - }; -} -"#, - r#" -fn foo() { - match n { - Some(n) => { - 29 - }, - _ => () - }; -} -"#, - ); - } - - #[test] - fn no_assist_for_match_with_braces() { - check_assist_not_applicable( - add_braces, - r#" -fn foo() { - match n { - Some(n) $0=> { return 29; }, - _ => () - }; -} -"#, - ); - } -} diff --git a/crates/ide-assists/src/handlers/change_braces.rs b/crates/ide-assists/src/handlers/change_braces.rs new file mode 100644 index 000000000000..5f39d1f88ada --- /dev/null +++ b/crates/ide-assists/src/handlers/change_braces.rs @@ -0,0 +1,310 @@ +use syntax::{ + ast::{self, edit_in_place::Indent, syntax_factory::SyntaxFactory}, + AstNode, SyntaxNode, +}; + +use crate::{AssistContext, AssistId, AssistKind, Assists}; + +// Assist: add_braces +// +// Adds braces to lambda and match arm expressions. +// +// ``` +// fn foo(n: i32) -> i32 { +// match n { +// 1 =>$0 n + 1, +// _ => 0 +// } +// } +// ``` +// -> +// ``` +// fn foo(n: i32) -> i32 { +// match n { +// 1 => { +// n + 1 +// }, +// _ => 0 +// } +// } +// ``` + +// Assist: remove_braces +// +// Removes braces from lambda and match arm expressions. +// +// ``` +// fn foo(n: i32) -> i32 { +// match n { +// 1 =>$0 { +// n + 1 +// }, +// _ => 0 +// } +// } +// ``` +// -> +// ``` +// fn foo(n: i32) -> i32 { +// match n { +// 1 => n + 1, +// _ => 0 +// } +// } +// ``` +pub(crate) fn change_braces(acc: &mut Assists, ctx: &AssistContext<'_>) -> Option<()> { + let change = get_change(ctx)?; + acc.add(change.assist_id(), change.label(), change.root_syntax().text_range(), |builder| { + let mut editor = builder.make_editor(change.root_syntax()); + let make = SyntaxFactory::new(); + editor.replace(change.root_syntax(), change.replacement(&make)); + editor.add_mappings(make.finish_with_mappings()); + builder.add_file_edits(ctx.file_id(), editor); + }) +} + +fn get_change(ctx: &AssistContext<'_>) -> Option { + let (expr, parent) = if let Some(match_arm) = ctx.find_node_at_offset::() { + (match_arm.expr().unwrap(), ParentKind::MatchArmExpr(match_arm)) + } else if let Some(closure_expr) = ctx.find_node_at_offset::() { + (closure_expr.body().unwrap(), ParentKind::ClosureExpr) + } else { + return None; + }; + + let change = match expr { + ast::Expr::BlockExpr(block_expr) => { + if block_expr.statements().next().is_some() { + return None; + } + + let tail_expr = block_expr.tail_expr()?; + + ChangeKind::RemoveBraces { block_expr, tail_expr } + } + other => ChangeKind::AddBraces(other), + }; + + Some(ChangeBraces { kind: change, parent }) +} + +struct ChangeBraces { + kind: ChangeKind, + parent: ParentKind, +} + +impl ChangeBraces { + fn assist_id(&self) -> AssistId { + let s = match &self.kind { + ChangeKind::AddBraces(_) => "add_braces", + ChangeKind::RemoveBraces { .. } => "remove_braces", + }; + + AssistId(s, AssistKind::RefactorRewrite) + } + + fn label(&self) -> String { + let change_part = match &self.kind { + ChangeKind::AddBraces(_) => "Add braces to", + ChangeKind::RemoveBraces { .. } => "Remove braces from", + }; + + let parent_str = match &self.parent { + ParentKind::MatchArmExpr(_) => "arm expression", + ParentKind::ClosureExpr => "closure body", + }; + + format!("{change_part} {parent_str}") + } + + fn root_syntax(&self) -> &SyntaxNode { + match &self.kind { + ChangeKind::AddBraces(expr) => expr.syntax(), + ChangeKind::RemoveBraces { block_expr, .. } => match &self.parent { + ParentKind::MatchArmExpr(match_arm) => match_arm.syntax(), + _ => block_expr.syntax(), + }, + } + } + + fn replacement(&self, make: &SyntaxFactory) -> SyntaxNode { + match &self.kind { + ChangeKind::AddBraces(expr) => { + let block_expr = make.block_expr([], Some(expr.clone())); + block_expr.indent(expr.indent_level()); + block_expr.syntax().clone() + } + ChangeKind::RemoveBraces { tail_expr, .. } => match &self.parent { + ParentKind::MatchArmExpr(match_arm) => make + .match_arm(match_arm.pat().unwrap(), match_arm.guard(), tail_expr.clone()) + .syntax() + .clone(), + _ => tail_expr.syntax().clone_for_update(), + }, + } + } +} + +enum ChangeKind { + AddBraces(ast::Expr), + RemoveBraces { block_expr: ast::BlockExpr, tail_expr: ast::Expr }, +} + +// We have to keep track of the match arm in case we need to add a comma +enum ParentKind { + MatchArmExpr(ast::MatchArm), + ClosureExpr, +} + +#[cfg(test)] +mod tests { + use super::*; + + use crate::tests::{check_assist, check_assist_not_applicable}; + + #[test] + fn suggest_add_braces_for_closure() { + check_assist( + change_braces, + r#" +fn foo() { +t(|n|$0 n + 100); +} +"#, + r#" +fn foo() { +t(|n| { + n + 100 +}); +} +"#, + ); + } + + #[test] + fn suggest_add_braces_for_match() { + check_assist( + change_braces, + r#" +fn foo() { +match n { + Some(n) $0=> 29, + _ => () +}; +} +"#, + r#" +fn foo() { +match n { + Some(n) => { + 29 + }, + _ => () +}; +} +"#, + ); + } + + #[test] + pub(crate) fn suggest_remove_braces_for_closure() { + check_assist( + change_braces, + r#" +fn foo() { +t(|n|$0 { + n + 100 +}); +} +"#, + r#" +fn foo() { +t(|n| n + 100); +} +"#, + ); + } + + #[test] + pub(crate) fn no_assist_for_closures_with_statements() { + check_assist_not_applicable( + change_braces, + r#" +fn foo() { +t(|n|$0 { + panic!(); + n + 100 +}); +} +"#, + ); + } + + #[test] + pub(crate) fn suggest_remove_braces_for_match() { + check_assist( + change_braces, + r#" +fn foo() { +match n { + Some(n) $0=> { + 29 + }, + _ => () +}; +} +"#, + r#" +fn foo() { +match n { + Some(n) => 29, + _ => () +}; +} +"#, + ); + } + + #[test] + pub(crate) fn no_assist_for_match_with_statements() { + check_assist_not_applicable( + change_braces, + r#" +fn foo() { +match n { + Some(n) $0=> { + panic!(); + 29 + }, + _ => () +}; +} +"#, + ); + } + + #[test] + pub(crate) fn handle_adding_comma() { + check_assist( + change_braces, + r#" +fn foo() { +match n { + Some(n) $0=> { + 29 + } + _ => () +}; +} +"#, + r#" +fn foo() { +match n { + Some(n) => 29, + _ => () +}; +} +"#, + ); + } +} diff --git a/crates/ide-assists/src/lib.rs b/crates/ide-assists/src/lib.rs index 5c95b25f28dd..cbeb78b7e688 100644 --- a/crates/ide-assists/src/lib.rs +++ b/crates/ide-assists/src/lib.rs @@ -104,7 +104,6 @@ mod handlers { pub(crate) type Handler = fn(&mut Assists, &AssistContext<'_>) -> Option<()>; - mod add_braces; mod add_explicit_type; mod add_label_to_loop; mod add_lifetime_to_type; @@ -116,6 +115,7 @@ mod handlers { mod auto_import; mod bind_unused_param; mod bool_to_enum; + mod change_braces; mod change_visibility; mod convert_bool_then; mod convert_closure_to_fn; @@ -232,7 +232,6 @@ mod handlers { pub(crate) fn all() -> &'static [Handler] { &[ // These are alphabetic for the foolish consistency - add_braces::add_braces, add_explicit_type::add_explicit_type, add_label_to_loop::add_label_to_loop, add_missing_match_arms::add_missing_match_arms, @@ -244,6 +243,7 @@ mod handlers { auto_import::auto_import, bind_unused_param::bind_unused_param, bool_to_enum::bool_to_enum, + change_braces::change_braces, change_visibility::change_visibility, convert_bool_then::convert_bool_then_to_if, convert_bool_then::convert_if_to_bool_then, diff --git a/crates/ide-assists/src/tests/generated.rs b/crates/ide-assists/src/tests/generated.rs index eef4da55e94a..756d1915cc48 100644 --- a/crates/ide-assists/src/tests/generated.rs +++ b/crates/ide-assists/src/tests/generated.rs @@ -2559,6 +2559,31 @@ const _: i32 = 1_012_345; ) } +#[test] +fn doctest_remove_braces() { + check_doc_test( + "remove_braces", + r#####" +fn foo(n: i32) -> i32 { + match n { + 1 =>$0 { + n + 1 + }, + _ => 0 + } +} +"#####, + r#####" +fn foo(n: i32) -> i32 { + match n { + 1 => n + 1, + _ => 0 + } +} +"#####, + ) +} + #[test] fn doctest_remove_dbg() { check_doc_test( diff --git a/crates/syntax/src/ast/syntax_factory/constructors.rs b/crates/syntax/src/ast/syntax_factory/constructors.rs index 9f88add0f787..47a5758f756a 100644 --- a/crates/syntax/src/ast/syntax_factory/constructors.rs +++ b/crates/syntax/src/ast/syntax_factory/constructors.rs @@ -83,6 +83,30 @@ impl SyntaxFactory { ast.into() } + pub fn match_arm( + &self, + pat: ast::Pat, + guard: Option, + expr: ast::Expr, + ) -> ast::MatchArm { + let ast = make::match_arm([pat.clone()], guard.clone(), expr.clone()).clone_for_update(); + + if let Some(mut mappings) = self.mappings() { + let mut builder = SyntaxMappingBuilder::new(ast.syntax().clone()); + + builder.map_node(pat.syntax().clone(), ast.pat().unwrap().syntax().clone()); + builder.map_node(expr.syntax().clone(), ast.expr().unwrap().syntax().clone()); + + if let Some(input) = guard { + builder.map_node(input.syntax().clone(), ast.guard().unwrap().syntax().clone()); + } + + builder.finish(&mut mappings); + } + + ast + } + pub fn let_stmt( &self, pattern: ast::Pat,