Skip to content
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.

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

Migrate assists to SyntaxEditor #18285

Open
darichey opened this issue Oct 11, 2024 · 5 comments
Open

Migrate assists to SyntaxEditor #18285

darichey opened this issue Oct 11, 2024 · 5 comments

Comments

@darichey
Copy link
Contributor

We want to migrate all assists to use the new SyntaxEditor/SyntaxFactory abstraction. This will unblock moving to immutable syntax trees (#15710). In doing so, we can choose a flat, contiguous representation that should yield significant performance wins (#17491).

@DropDemBits has outlined the process for migrating most assists:

  • Create a SyntaxFactory, optionally named make to ease migration
  • Create a SyntaxEditor using builder.make_editor with a node from the file
  • Replace all make:: with make
    • Add any missing methods to syntax::ast::syntax_factory::constructors as needed
  • Replace all ted:: with editor.
  • After all changes are complete:
    • editor.add_mappings(make.finish_with_mappings()) to add the generated mappings
    • builder.add_file_edits(ctx.file_id(), editor) to add the edits
      For assists that use snippets, check the snippet capability, and then use editor.add_annotation to associate the snippets with the elements.

They have also already migrated the extract_variable assist in #18196 which should be a useful reference.

The assists are located in crates/ide-assists/src/handlers. They should all have tests which you can use to verify that your change works properly by running:

cargo test --package ide-assists -- handlers::<ASSIST>::tests  

replacing <ASSIST> with the assist name, e.g., add_braces.

@darichey
Copy link
Contributor Author

We have two MLH fellows, @tareknaser, @davidkurilla, who are interested in working on this.

@Giga-Bowser
Copy link
Contributor

I'm not sure of the precedent on this sort of thing, but I've got two open PRs, #18458 and #18459, that both have to do with assists, and if it would help I'd gladly migrate those assists as part of those PRs.

Specifically I would be migrating:

@Giga-Bowser
Copy link
Contributor

While migrating those PRs I ran into make::ext::expr_todo(), is there any preference as far as how to migrate this? I could define an expr_todo() method on SyntaxFactory, but if there's a desire to preserve this split between make and make::ext then I could also define it as ext_expr_todo() in a separate impl block, or even do some sort of type shenanigans with an ext() method on SyntaxFactory that returns a SyntaxFactoryExt with its own methods. Let me know what is preferred.

@darichey
Copy link
Contributor Author

Not sure if anyone has a strong opinion, but just putting it directly in SyntaxFactory for now seems fine to me.

@Giga-Bowser
Copy link
Contributor

One observation while adding methods to syntax::ast::syntax_factory::constructors: I often found that the equivalent make:: functions would return more general AST types than necessary, which I think is a mistake we shouldn't repeat. expr_return should return an ast::ReturnExpr, not an ast::Expr, especially since casting an ast::ReturnExpr into an ast::Expr is infallible and the other way around is not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants