Migrate introduce_named_generic Assist to Use SyntaxFactory

[tareknaser]

This PR is part of #15710 and #18285.

Changes Made

• Followed the steps in Migrate assists to SyntaxEditor #18285 for migration.
• Added a new SyntaxFactory method, type_param.
• Replaced add_generic_param (originally in GenericParamList) with a new method, syntax_editor_add_generic_param. This new method is similar but takes a SyntaxEditor instance instead of ted to avoid the “immutable tree” error.

[tareknaser]

Marking this as a draft for now since I’m running into a couple of errors with this assist

1. The first issue is with cursor positioning:

---- handlers::introduce_named_generic::tests::replace_two_impl_trait_with_generic_params stdout ----
Left:
fn foo<G, $0B: Bar>(foo: impl Foo, bar: B) {}

Right:
fn foo<$0G, B: Bar>(foo: impl Foo, bar: B) {}

Diff:
fn foo<$0G, $0B: Bar>(foo: impl Foo, bar: B) {}


thread 'handlers::introduce_named_generic::tests::replace_two_impl_trait_with_generic_params' panicked at crates/ide-assists/src/handlers/introduce_named_generic.rs:105:9:
text differs

Here, only the cursor position appears to be off.

2. The second error is related to an immutable tree in rowan:

---- tests::generated::doctest_introduce_named_generic stdout ----
thread 'tests::generated::doctest_introduce_named_generic' panicked at /Users/tareknasser/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rowan-0.15.15/src/cursor.rs:810:9:
immutable tree: fn foo(bar: impl Bar) {}

[ShoyuVanilla]

This get_or_create_generic_param_list mutates syntax tree if there is no generic params - as the function name shows 😄 - and causes immutable tree panics in rowan. (You can check this with RUST_BACKTRACE=full cargo test <panicking test name> or $env:RUST_BACKTRACE="full"; cargo test <panicking test name> if you are on Windows.)
This should be modified to copying existing generic params or making fresh new generic params rather than inserting into exsisting syntax tree.
#18385 might be a good reference working on SyntaxFactory assist things

[ShoyuVanilla]

The reason for panicking is because L35: let fn_ = edit.make_mut(fn_) is being removed. Of course, it should be removed since we are migrating to SyntaxFactory from mutable syntax tree and we should modify the L45

[tareknaser]

Thank you for the review

This get_or_create_generic_param_list mutates syntax tree if there is no generic params ... This should be modified to copying existing generic params

How can I ‘copy existing generic params’ if there are none to begin with?

or making fresh new generic params

How would I go about doing that? I looked into initializing a new GenericParamList, and the only way I found was using make::generic_param_list, which is actually what’s in place now (but fails).

I defined a new ast::Fn method similar to get_or_create_generic_param_list() that calls syntax_editor_create_generic_param_list if there are no existing generic params. This uses SyntaxEditor to initialize the list (since insert_raw() isn’t available for SyntaxEditor, I used insert instead).

Unfortunately, I can’t fully test my updates due to a new error:

---- tests::generated::doctest_introduce_named_generic stdout ----
thread 'tests::generated::doctest_introduce_named_generic' panicked at crates/syntax/src/syntax_editor.rs:55:9:
assertion failed: is_ancestor_or_self(&position.parent(), &self.root)

After tracing, this seems to be related to my syntax_editor_add_generic_param implementation, though it’s identical to add_generic_param except it uses SyntaxEditor instead of ted.

[tareknaser]

By the way, I know these new functions would ideally go into a separate file, but right now I’m just focused on getting them working first

[ShoyuVanilla]

Sorry for the really late reply. I've been quite busy 'cause I moved to a new job 😅

Your code that creating fresh new generic param list seems good but as it still panics, I'll add some comments.

[tareknaser]

Sorry for the really late reply. I've been quite busy 'cause I moved to a new job 😅

No worries at all. Best of luck with your new job

[ShoyuVanilla]

This panics on the following assertion;

rust-analyzer/crates/syntax/src/syntax_editor.rs

Lines 54 to 55 in a2bb014

	pub fn insert(&mut self, position: Position, element: impl Element) {
	debug_assert!(is_ancestor_or_self(&position.parent(), &self.root));

I think that the panic happens when we are here in the following process;

1. First, we insert a new empty generic arg list because the function doesn't have one
2. And then we add a new type parameter for impl Trait thing

In 2. we don't have the new empty generic arg list made in 1. in our syntax tree yet (it will be made when the syntax edit finishes) so the position for new type parameter doesn't exist in our syntax tree yet either.
So, I think that we should directly insert a new generic arg list with new type param in 1. without separated step 2.

(Also, the current logic seems problematic beside the above because we are inserting <T> into <>, instead of T)

[tareknaser]

So just for context, there’s been some discussion about this in Zulip.

In 2. we don't have the new empty generic arg list made in 1. in our syntax tree yet (it will be made when the syntax edit finishes) so the position for new type parameter doesn't exist in our syntax tree yet either.

That was exactly the issue. I updated the code to add a method, ast::Fn::syntax_editor_add_generic_param, that handles the logic for us. Now, there’s a clear separation between the three cases in this method:

• If there is an existing generic param list and it’s not empty, we replace the entire list with a new one (existing params + the new one).
• If there’s an existing but empty generic param list, we place the new param after the left angle token.
• If there is no generic param list, we create one from scratch and place it in the right position.

This is working fine for all these cases, but the only issue left is the cursor position—it’s off in all cases.

I updated the code to use SyntaxEditor::add_annotation() as suggested #18483 (comment) , but I'm still facing the cursor issue. After tracing it a bit, it seems that this might be happening because fn_.generic_param_list() isn’t being updated. If that’s the case, how can I update it?

Update: Fixed in 69b9660. It turns out manually inserting the tokens resolves the issue.

[darichey]

Awesome! Thanks for your perseverance :)

cc @davidbarsky