Skip to content

Migrate reorder_fields Assist to Use SyntaxFactory #18495

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 1 commit into from
Nov 15, 2024
Merged

Migrate reorder_fields Assist to Use SyntaxFactory #18495

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
29 changes: 21 additions & 8 deletions crates/ide-assists/src/handlers/reorder_fields.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use either::Either;
use ide_db::FxHashMap;
use itertools::Itertools;
use syntax::{ast, ted, AstNode, SmolStr, ToSmolStr};
use syntax::{ast, syntax_editor::SyntaxEditor, AstNode, SmolStr, SyntaxElement, ToSmolStr};

use crate::{AssistContext, AssistId, AssistKind, Assists};

Expand All @@ -24,6 +24,11 @@ pub(crate) fn reorder_fields(acc: &mut Assists, ctx: &AssistContext<'_>) -> Opti
let record =
path.syntax().parent().and_then(<Either<ast::RecordExpr, ast::RecordPat>>::cast)?;

let parent_node = match ctx.covering_element() {
SyntaxElement::Node(n) => n,
SyntaxElement::Token(t) => t.parent()?,
};

let ranks = compute_fields_ranks(&path, ctx)?;
let get_rank_of_field = |of: Option<SmolStr>| {
*ranks.get(of.unwrap_or_default().trim_start_matches("r#")).unwrap_or(&usize::MAX)
Expand Down Expand Up @@ -65,23 +70,31 @@ pub(crate) fn reorder_fields(acc: &mut Assists, ctx: &AssistContext<'_>) -> Opti
AssistId("reorder_fields", AssistKind::RefactorRewrite),
"Reorder record fields",
target,
|builder| match fields {
Either::Left((sorted, field_list)) => {
replace(builder.make_mut(field_list).fields(), sorted)
}
Either::Right((sorted, field_list)) => {
replace(builder.make_mut(field_list).fields(), sorted)
|builder| {
let mut editor = builder.make_editor(&parent_node);

match fields {
Either::Left((sorted, field_list)) => {
replace(&mut editor, field_list.fields(), sorted)
}
Either::Right((sorted, field_list)) => {
replace(&mut editor, field_list.fields(), sorted)
}
}

builder.add_file_edits(ctx.file_id(), editor);
},
)
}

fn replace<T: AstNode + PartialEq>(
editor: &mut SyntaxEditor,
fields: impl Iterator<Item = T>,
sorted_fields: impl IntoIterator<Item = T>,
) {
fields.zip(sorted_fields).for_each(|(field, sorted_field)| {
ted::replace(field.syntax(), sorted_field.syntax().clone_for_update())
// FIXME: remove `clone_for_update` when `SyntaxEditor` handles it for us
editor.replace(field.syntax(), sorted_field.syntax().clone_for_update())
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to eliminate this clone_for_update using SyntaxFactory. It looks like we will need to add constructors for record_expr_field and record_pat_field. For now, I would inline this replace function so you don't need to worry about how to abstract it over the two cases. We can try to clean it up afterwards.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Inlined the editor.replace() calls, but the clone_for_update() calls are still needed. Removing them causes the assist to fail tests with an ‘immutable tree’ error

Copy link
Contributor

Choose a reason for hiding this comment

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

Removing them causes the assist to fail tests with an ‘immutable tree’ error

Right, we do still need a mutable node, but my understanding is that we want to hide this under SyntaxFactory so it is abstracted away and so that we track the mappings, hence my suggestion of adding those constructors. That said, in these cases where we do just want to clone, those constructors might not be very convenient to use. @DropDemBits can you check my understanding here?

Copy link
Contributor

Choose a reason for hiding this comment

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

@darichey Generally we'd like to hide the details of creating new nodes under SyntaxFactory. In this case though, we're picking nodes from the original syntax tree so we wouldn't need to go through a SyntaxFactory.

SyntaxEditor should handle transform any node in an edit that reference the original syntax tree into their mutable equivalents, but seems like I forgot this case for the new nodes in replace edits 😅. For now it should be okay to use clone_for_update and add a fixme, and then we can remove those fixmes once SyntaxEditor is fixed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it, thank you! Sorry about that, @tareknaser, you can revert inlining replace and just add a FIXME for the clone_for_update :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All set 5c41c20

});
}

Expand Down