Ideally all of the extract & generate assists should still be using the add_placeholder_snippet_group for better out-of-the box support for other editor extensions, but that's an entirely separate issue 🙂

I experimented with that first and it seemed like it was a less than ideal experience. Agree that this is a separate issue.

Thanks!
@bors r

📌 Commit bfa6a5c has been approved by Veykril

It is now in the queue for this repository.

⌛ Testing commit bfa6a5c with merge e961b1a...

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing e961b1a to master...

@joshka, @Veykril, shouldn't RA check whether the LSP client announced its support for the command "rust-analyzer.rename" before sending it in a codeAction/resolve reply?

Also, I'm not sure asking the client to do a rename after the the extract is the best approach since RA can send "disjoint" snippets as well. I have not tested it, but instead of sending this:

{
  "jsonrpc": "2.0", "id": 7,
  "result": {
    "title": "Extract into variable",
    "kind": "refactor.extract",
    "command": { "title": "rename",  "command": "rust-analyzer.rename" },
    "edit": {
      "documentChanges": [
        {
          "textDocument": { "uri": "file:///tmp/rust-rename-17587/src/main.rs", "version": 0 },
          "edits": [
            {
              "range": {
                "start": { "line": 1, "character": 8 },
                "end": { "line": 1, "character": 9  }
              },
              "newText": "$0var_name",
              "insertTextFormat": 2
            },
            {
              "range": {
                "start": { "line": 1, "character": 18 },
                "end": {  "line": 1,  "character": 18 }
              },
              "newText": "\n    let a = var_name;"
            }
          ]
        }
      ]
    }
  }
}

RA could send something like this (edited):

{
  "jsonrpc": "2.0", "id": 7,
  "result": {
    "title": "Extract into variable",
    "kind": "refactor.extract",
    "edit": {
      "documentChanges": [
        {
          "textDocument": { "uri": "file:///tmp/rust-rename-17587/src/main.rs", "version": 0 },
          "edits": [
            {
              "range": {
                "start": { "line": 1, "character": 8 },
                "end": { "line": 1, "character": 9  }
              },
              "newText": "$0${1:var_name}",
              "insertTextFormat": 2
            },
            {
              "range": {
                "start": { "line": 1, "character": 18 },
                "end": {  "line": 1,  "character": 18 }
              },
              "newText": "\n    let a = $1;"
              "insertTextFormat": 2
            }
          ]
        }
      ]
    }
  }
}

I.e., the "var_name" is a just default value for the mirrored tab-stop field. The commit log of #15876 directly says:

Explicitly allow a single TextDocumentEdit to have multiple SnippetTextEdits. This allows things like renaming extracted variables and functions without having to go through a separate rename step.

@nemethf

shouldn't RA check whether the LSP client announced its support for the command "rust-analyzer.rename" before sending it in a codeAction/resolve reply?

Hmm, it should be respecting the client commands list that the client declares and only emitting it if editor.action.rename is specified there.

Also, I'm not sure asking the client to do a rename after the the extract is the best approach since RA can send "disjoint" snippets as well.

This would also be better for editors like Helix, where I forsee code action snippet support being available before a plugin system is ready.

It is possible to do it today like is done for generate_trait_from_impl, but for extract_variable there's one case where it's annoying to grab the nodes to mark as a placeholder group. Once we have something like #9649 it'll be nicer to find these nodes again, so hopefully I can scrounge up enough free time and look into that again.

shouldn't RA check whether the LSP client announced its support for the command "rust-analyzer.rename" before sending it in a codeAction/resolve reply?

Hmm, it should be respecting the client commands list that the client declares and only emitting it if editor.action.rename is specified there.

That is not what happens in my case. The client only states that it supports the most basic rename action in its RenameClientCapabilities and that's all. And yet the server sends this in its textDocument/rename reply: "command": { "title": "rename", "command": "rust-analyzer.rename" },

$ rust-analyzer --version
rust-analyzer 0.4.2041-standalone

{
  "jsonrpc": "2.0",
  "id": 1,
  "method": "initialize",
  "params": {
    "processId": 1602814,
    "clientInfo": { "name": "Eglot",  "version": "1.17" },
    "rootPath": "/tmp/rust-rename-17587/",
    "rootUri": "file:///tmp/rust-rename-17587",
    "initializationOptions": {},
    "capabilities": {
      "workspace": {
        "applyEdit": true,
        "executeCommand": { "dynamicRegistration": false },
        "workspaceEdit": { "documentChanges": true },
        "didChangeWatchedFiles": { "dynamicRegistration": true },
        "symbol": { "dynamicRegistration": false },
        "configuration": true,
        "workspaceFolders": true
      },
      "textDocument": {
        "synchronization": {
          "dynamicRegistration": false,
          "willSave": true,
          "willSaveWaitUntil": true,
          "didSave": true
        },
        "completion": {
          "dynamicRegistration": false,
          "completionItem": {
            "snippetSupport": true,
            "deprecatedSupport": true,
            "resolveSupport": {
              "properties": [ "documentation", "details", "additionalTextEdits" ]
            },
            "tagSupport": { "valueSet": [ 1 ] }
          },
          "contextSupport": true
        },
        "hover": {
          "dynamicRegistration": false,
          "contentFormat": [ "plaintext" ]
        },
        "signatureHelp": {
          "dynamicRegistration": false,
          "signatureInformation": {
            "parameterInformation": { "labelOffsetSupport": true },
            "documentationFormat": [ "plaintext" ],
            "activeParameterSupport": true
          }
        },
        "references": { "dynamicRegistration": false },
        "definition": {
          "dynamicRegistration": false,
          "linkSupport": true
        },
        "declaration": {
          "dynamicRegistration": false,
          "linkSupport": true
        },
        "implementation": {
          "dynamicRegistration": false,
          "linkSupport": true
        },
        "typeDefinition": {
          "dynamicRegistration": false,
          "linkSupport": true
        },
        "documentSymbol": {
          "dynamicRegistration": false,
          "hierarchicalDocumentSymbolSupport": true,
          "symbolKind": {
            "valueSet": [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26 ]
          }
        },
        "documentHighlight": { "dynamicRegistration": false },
        "codeAction": {
          "dynamicRegistration": false,
          "resolveSupport": {
            "properties": [ "edit", "command" ]
          },
          "dataSupport": true,
          "codeActionLiteralSupport": {
            "codeActionKind": {
              "valueSet": [
                "quickfix",
                "refactor",
                "refactor.extract",
                "refactor.inline",
                "refactor.rewrite",
                "source",
                "source.organizeImports"
              ]
            }
          },
          "isPreferredSupport": true
        },
        "formatting": { "dynamicRegistration": false },
        "rangeFormatting": { "dynamicRegistration": false },
        "rename": { "dynamicRegistration": false },
        "inlayHint": { "dynamicRegistration": false },
        "publishDiagnostics": { 
          "relatedInformation": false,
          "codeDescriptionSupport": false,
          "tagSupport": {
            "valueSet": [ 1, 2 ]
          }
        }
      },
      "window": {
        "showDocument": { "support": true },
        "workDoneProgress": true
      },
      "general": {
        "positionEncodings": [ "utf-32", "utf-8", "utf-16" ]
      },
      "experimental": {
        "snippetTextEdit": true,
        "serverStatusNotification": true,
        "colorDiagnosticOutput": true,
        "openServerLogs": true,
        "testExplorer": true,
        "localDocs": true
      },
      "offsetEncoding": [ "utf-32", "utf-16" ]
    },
    "workspaceFolders": [
      {
        "uri": "file:///tmp/rust-rename-17587",
        "name": "/tmp/rust-rename-17587/"
      }
    ]
  }
}

There is a weird lens_forceCustomCommands config that is incorrectly set to to true by default, when that is turned on we ignored the capability 🙃 That would explain this, unsure what the point of this is, we should remove it fully.

Also, I'm not sure asking the client to do a rename after the the extract is the best approach since RA can send "disjoint" snippets as well. I have not tested it, but instead of sending this:

The rename UX flow seems (subjectively) nicer than the disjoint snippets flow to me as there's no good intuitive way to finish the disjoint edit (neither Tab or Enter work, only Escape), also there's some editing weirdness:

Changing extract_variable.rs:131 (and removing the rename call):

                    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_placeholder_snippet_group(
                                    cap,
                                    vec![name.syntax().clone(), name_expr.syntax().clone()],
                                );
                            }
                        }
                    }

Regarding the client commands, the server side execution of the rename command is a noop if the editor.action.rename command is not enabled (as commands.rename == false in that case). I don't think the assists have access to the any client command config, so there's no way to filter this out.

That is not what happens in my case. The client only states that it supports the most basic rename action in its RenameClientCapabilities and that's all. And yet the server sends this in its textDocument/rename reply: "command": { "title": "rename", "command": "rust-analyzer.rename" },

Is there any impact of this other than an unnecessary round trip? I'm trying to understand what the error / problem is here, but I'm not seeing it.

Is there any impact of this other than an unnecessary round trip? I'm trying to understand what the error / problem is here, but I'm not seeing it.

That depends on the client, we are violating the LSP (or in this case the lsp extension I suppose), we shouldn't be doing that.

The problem is this config being enabled by default for some reason https://github.com/veykril/rust-analyzer/blob/09ef75c717e9ffb4cbc71ce1cfec7694244742c2/crates/rust-analyzer/src/config.rs#L564 which then force enables all of the commands here https://github.com/veykril/rust-analyzer/blob/09ef75c717e9ffb4cbc71ce1cfec7694244742c2/crates/rust-analyzer/src/config.rs#L1896-L1911
We should just get rid of lens_forceCustomCommands, I see no value in it for debugging.

Ah got it

There is one additional thing that I don't understand. The client needs to declare support for editor.action.rename, but the server sends rust-analyzer.rename instead of editor.action.rename. Is this intentional?

Thanks.

Same goes for the "editor.action.triggerParameterHints", capability. Thanks for calling that out I think we should move those over to rust-analyzer.* or similar. The names are based on the equivalent vscode commands but we should clearly not use the exact same names for those.