Skip to content

internal: Remove mutable syntax tree usages from add_missing_match_arms assist #19155

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Feb 16, 2025
Merged

internal: Remove mutable syntax tree usages from add_missing_match_arms assist #19155

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8b947d7
internal: Remove mutable syntax tree usages from `add_missing_match_a…
ShoyuVanilla Feb 13, 2025
7d74f2b
Temporarily ignore tests with comments
ShoyuVanilla Feb 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
136 changes: 70 additions & 66 deletions crates/ide-assists/src/handlers/add_missing_match_arms.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,9 @@ use ide_db::syntax_helpers::suggest_name;
use ide_db::RootDatabase;
use ide_db::{famous_defs::FamousDefs, helpers::mod_path_to_ast};
use itertools::Itertools;
use syntax::ast::edit_in_place::Removable;
use syntax::ast::edit::IndentLevel;
use syntax::ast::edit_in_place::Indent;
use syntax::ast::syntax_factory::SyntaxFactory;
use syntax::ast::{self, make, AstNode, MatchArmList, MatchExpr, Pat};

use crate::{utils, AssistContext, AssistId, AssistKind, Assists};
Expand Down Expand Up @@ -200,8 +202,8 @@ pub(crate) fn add_missing_match_arms(acc: &mut Assists, ctx: &AssistContext<'_>)
AssistId("add_missing_match_arms", AssistKind::QuickFix),
"Fill match arms",
ctx.sema.original_range(match_expr.syntax()).range,
|edit| {
let new_match_arm_list = match_arm_list.clone_for_update();
|builder| {
let make = SyntaxFactory::new();

// having any hidden variants means that we need a catch-all arm
needs_catch_all_arm |= has_hidden_variants;
Expand All @@ -211,89 +213,85 @@ pub(crate) fn add_missing_match_arms(acc: &mut Assists, ctx: &AssistContext<'_>)
// filter out hidden patterns because they're handled by the catch-all arm
!hidden
})
.map(|(pat, _)| {
make::match_arm(pat, None, make::ext::expr_todo()).clone_for_update()
});
.map(|(pat, _)| make.match_arm(pat, None, make::ext::expr_todo()));

let catch_all_arm = new_match_arm_list
let mut arms: Vec<_> = match_arm_list
.arms()
.find(|arm| matches!(arm.pat(), Some(ast::Pat::WildcardPat(_))));
if let Some(arm) = catch_all_arm {
let is_empty_expr = arm.expr().is_none_or(|e| match e {
ast::Expr::BlockExpr(b) => {
b.statements().next().is_none() && b.tail_expr().is_none()
.filter(|arm| {
if matches!(arm.pat(), Some(ast::Pat::WildcardPat(_))) {
let is_empty_expr = arm.expr().is_none_or(|e| match e {
ast::Expr::BlockExpr(b) => {
b.statements().next().is_none() && b.tail_expr().is_none()
}
ast::Expr::TupleExpr(t) => t.fields().next().is_none(),
_ => false,
});
if is_empty_expr {
false
} else {
cov_mark::hit!(add_missing_match_arms_empty_expr);
true
}
} else {
true
}
ast::Expr::TupleExpr(t) => t.fields().next().is_none(),
_ => false,
});
if is_empty_expr {
arm.remove();
} else {
cov_mark::hit!(add_missing_match_arms_empty_expr);
}
}
})
.collect();

let mut added_arms = Vec::new();
let mut todo_placeholders = Vec::new();
for arm in missing_arms {
todo_placeholders.push(arm.expr().unwrap());
added_arms.push(arm);
}
let first_new_arm_idx = arms.len();
arms.extend(missing_arms);

if needs_catch_all_arm && !has_catch_all_arm {
cov_mark::hit!(added_wildcard_pattern);
let arm =
make::match_arm(make::wildcard_pat().into(), None, make::ext::expr_todo())
.clone_for_update();
todo_placeholders.push(arm.expr().unwrap());
added_arms.push(arm);
}

let first_new_arm = added_arms.first().cloned();
let last_new_arm = added_arms.last().cloned();

for arm in added_arms {
new_match_arm_list.add_arm(arm);
let arm = make.match_arm(make::wildcard_pat().into(), None, make::ext::expr_todo());
arms.push(arm);
}

if let Some(cap) = ctx.config.snippet_cap {
if let Some(it) = first_new_arm
.and_then(|arm| arm.syntax().descendants().find_map(ast::WildcardPat::cast))
{
edit.add_placeholder_snippet(cap, it);
}

for placeholder in todo_placeholders {
edit.add_placeholder_snippet(cap, placeholder);
}

if let Some(arm) = last_new_arm {
edit.add_tabstop_after(cap, arm);
}
}
let new_match_arm_list = make.match_arm_list(arms);

// FIXME: Hack for mutable syntax trees not having great support for macros
// FIXME: Hack for syntax trees not having great support for macros
// Just replace the element that the original range came from
let old_place = {
// Find the original element
let file = ctx.sema.parse(arm_list_range.file_id);
let old_place = file.syntax().covering_element(arm_list_range.range);

// Make `old_place` mut
match old_place {
syntax::SyntaxElement::Node(it) => {
syntax::SyntaxElement::from(edit.make_syntax_mut(it))
}
syntax::SyntaxElement::Node(it) => it,
syntax::SyntaxElement::Token(it) => {
// If a token is found, it is '{' or '}'
// The parent is `{ ... }`
let parent = it.parent().expect("Token must have a parent.");
syntax::SyntaxElement::from(edit.make_syntax_mut(parent))
it.parent().expect("Token must have a parent.")
}
}
};

syntax::ted::replace(old_place, new_match_arm_list.syntax());
let mut editor = builder.make_editor(&old_place);
new_match_arm_list.indent(IndentLevel::from_node(&old_place));
editor.replace(old_place, new_match_arm_list.syntax());

if let Some(cap) = ctx.config.snippet_cap {
if let Some(it) = new_match_arm_list
.arms()
.nth(first_new_arm_idx)
.and_then(|arm| arm.syntax().descendants().find_map(ast::WildcardPat::cast))
{
editor.add_annotation(it.syntax(), builder.make_placeholder_snippet(cap));
}

for arm in new_match_arm_list.arms().skip(first_new_arm_idx) {
if let Some(expr) = arm.expr() {
editor.add_annotation(expr.syntax(), builder.make_placeholder_snippet(cap));
}
}

if let Some(arm) = new_match_arm_list.arms().skip(first_new_arm_idx).last() {
editor.add_annotation(arm.syntax(), builder.make_tabstop_after(cap));
}
}

editor.add_mappings(make.finish_with_mappings());
builder.add_file_edits(ctx.file_id(), editor);
},
)
}
Expand Down Expand Up @@ -1377,6 +1375,9 @@ fn main() {
);
}

// FIXME: Preserving comments is quite hard in the current transitional syntax editing model.
// Once we migrate to new trivia model addressed in #6854, remove the ignore attribute.
#[ignore]
#[test]
fn add_missing_match_arms_preserves_comments() {
check_assist(
Expand Down Expand Up @@ -1405,6 +1406,9 @@ fn foo(a: A) {
);
}

// FIXME: Preserving comments is quite hard in the current transitional syntax editing model.
// Once we migrate to new trivia model addressed in #6854, remove the ignore attribute.
#[ignore]
#[test]
fn add_missing_match_arms_preserves_comments_empty() {
check_assist(
Expand Down Expand Up @@ -1502,10 +1506,10 @@ enum Test {

fn foo(t: Test) {
m!(match t {
Test::A => ${1:todo!()},
Test::B => ${2:todo!()},
Test::C => ${3:todo!()},$0
});
Test::A => ${1:todo!()},
Test::B => ${2:todo!()},
Test::C => ${3:todo!()},$0
});
}"#,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The indentation got a bit better

);
}
Expand Down
46 changes: 0 additions & 46 deletions crates/syntax/src/ast/edit_in_place.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -710,52 +710,6 @@ impl ast::Fn {
}
}

impl Removable for ast::MatchArm {
fn remove(&self) {
if let Some(sibling) = self.syntax().prev_sibling_or_token() {
if sibling.kind() == SyntaxKind::WHITESPACE {
ted::remove(sibling);
}
}
if let Some(sibling) = self.syntax().next_sibling_or_token() {
if sibling.kind() == T![,] {
ted::remove(sibling);
}
}
ted::remove(self.syntax());
}
}

impl ast::MatchArmList {
pub fn add_arm(&self, arm: ast::MatchArm) {
normalize_ws_between_braces(self.syntax());
let mut elements = Vec::new();
let position = match self.arms().last() {
Some(last_arm) => {
if needs_comma(&last_arm) {
ted::append_child(last_arm.syntax(), make::token(SyntaxKind::COMMA));
}
Position::after(last_arm.syntax().clone())
}
None => match self.l_curly_token() {
Some(it) => Position::after(it),
None => Position::last_child_of(self.syntax()),
},
};
let indent = IndentLevel::from_node(self.syntax()) + 1;
elements.push(make::tokens::whitespace(&format!("\n{indent}")).into());
elements.push(arm.syntax().clone().into());
if needs_comma(&arm) {
ted::append_child(arm.syntax(), make::token(SyntaxKind::COMMA));
}
ted::insert_all(position, elements);

fn needs_comma(arm: &ast::MatchArm) -> bool {
arm.expr().is_some_and(|e| !e.is_block_like()) && arm.comma_token().is_none()
}
}
}

impl ast::LetStmt {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These impls utilizes mutable syntax trees and as we are not using these anywhere after this PR, they should be removed, I think

pub fn set_ty(&self, ty: Option<ast::Type>) {
match ty {
Expand Down
3 changes: 2 additions & 1 deletion crates/syntax/src/ast/make.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -837,7 +837,8 @@ pub fn match_guard(condition: ast::Expr) -> ast::MatchGuard {

pub fn match_arm_list(arms: impl IntoIterator<Item = ast::MatchArm>) -> ast::MatchArmList {
let arms_str = arms.into_iter().fold(String::new(), |mut acc, arm| {
let needs_comma = arm.expr().is_none_or(|it| !it.is_block_like());
let needs_comma =
arm.comma_token().is_none() && arm.expr().is_none_or(|it| !it.is_block_like());
let comma = if needs_comma { "," } else { "" };
let arm = arm.syntax();
format_to_acc!(acc, " {arm}{comma}\n")
Expand Down