Migrate reorder_fields Assist to Use SyntaxFactory
#18495
Conversation
rustbot
added
the
S-waiting-on-review
| 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()) | ||
| editor.replace(field.syntax(), sorted_field.syntax().clone_for_update()) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
Got it, thank you! Sorry about that, @tareknaser, you can revert inlining replace and just add a FIXME for the clone_for_update :)
| fn replace<T: AstNode + PartialEq>( | ||
| 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()) | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
So just bring this back and add like
// FIXME: remove clone_for_update when SyntaxEditor handles it for us
editor.replace(field.syntax(), sorted_field.syntax().clone_for_update());
Signed-off-by: Tarek <tareknaser360@gmail.com>
tareknaser
force-pushed
the
syntax_factory_reorder_fields
branch
from
bef15fc to
5c41c20
Compare
This PR is part of #15710 and #18285