rust-lang / rust-analyzer

A Rust compiler front-end for IDEs
https://rust-analyzer.github.io/
Apache License 2.0
13.99k stars 1.55k forks source link

Prompt for name when using extract refactoring assists #17579

Open joshka opened 1 month ago

joshka commented 1 month ago

In typescript, extracting a variable / function / etc. allows you to enter a name for the just created extraction. It would be nice to do the same in RA for functions, modules, variables, etc.

image image

Playing with the extract_variable:

if let Some(cap) = ctx.config.snippet_cap {
    if let Some(ast::Pat::IdentPat(ident_pat)) = let_stmt.pat() {
        if let Some(name) = ident_pat.name() {
-            edit.add_tabstop_before(cap, name);
+            edit.add_placeholder_snippet_group(
+                cap,
+                vec![name.syntax().clone(), name_expr.syntax().clone()],
            )
        }
    }
}

SourceChangeBuilder::add_placeholder_snippet_token doesn't seem quite right for this. Instead of a single edit location which applies to both places when the edit is confirmed, this method makes two edit locations that are edited simultaneously.

image

I'm not quite sure what the right terminology to use to find the functionality here, so seeking some guidance on what to do to make this work.

Edit: this UX is provided by the rename symbol function, but I'm not sure how that would fit in to the extract refactoring.

DropDemBits commented 1 month ago

It seems that the Typescript language server is sending a command to the client (in this case, VSCode) to trigger a rename dialog, as the dialog that shows up for renaming a symbol is shown by the client, which then sends the new name as part of a textDocument/rename request.

rust-analyzer does have custom commands (see https://github.com/rust-lang/rust-analyzer/blob/5577e4e3b1d87eb3a3e45b649ff73e021a10dba5/editors/code/src/main.ts#L113), but you'll have to look into the VSCode Typescript extension to see how they do it.

joshka commented 1 month ago

Thanks for the guidance on where to look.

Looking at tsserver, it seems that every refactoring method returns (possibly empty) information on the file and location to use in presenting the rename command after the refactor finishes.

In RA, currently there's a trigger_signature_help bool, which controls whether a command is returned from the code action to the vscode client (and I assume is then run by the client). I think this can possibly be implemented by replacing the trigger_signature_help bool with an enum that specifies what command will run after the refactoring. I'll experiment a bit more soon.

joshka commented 1 month ago

Added #17587 which triggers editor.action.rename after performing variable extraction. I'd also add function and module extraction. Are there any other assists I might have missed that would also need this behavior?

DropDemBits commented 1 month ago

From looking at ide-assists/src/handlers, extract_type_alias would be another good candidate. The other good candidates would be the generate assists (e.g. generate_trait_from_impl, generate_function).

Note that not all of the generate assists use the correct snippet APIs (see #17332), and to do so would require migrating them to use both mutable AST and the appropriate add_placeholder_snippet methods.

joshka commented 1 month ago

Thanks. I took a brief look at the folder and also saw

Note that not all of the generate assists use the correct snippet APIs (see #17332), and to do so would require migrating them to use both mutable AST and the appropriate add_placeholder_snippet methods.

I wondered about that - good to know that these don't implement the right methods. With 1.5K issues, it seems like it would be difficult to build up the context of what's a problem and what's intended easily.

DropDemBits commented 1 month ago
  • [ ] extract_struct_from_enum_variant

I'd also classify this one as a maybe as you'd typically want the extracted struct to have the same name as the variant name.

With 1.5K issues, it seems like it would be difficult to build up the context of what's a problem and what's intended easily.

Guess you're in luck for this domain, since I was the one who moved the assists to using the add_placeholder_snippet method 😁

joshka commented 1 month ago

I'd also classify this one as a maybe as you'd typically want the extracted struct to have the same name as the variant name.

makes sense.

Guess you're in luck for this domain, since I was the one who moved the assists to using the add_placeholder_snippet method 😁

Neat - it's good to be lucky :)