rename-file (willRenameFiles) doesn't work with Metals

[chessman]

Thank you for the bug report

• [X] I am using the latest version of lsp-mode related packages.
• [X] I checked FAQ and Troubleshooting sections
• [ ] You may also try reproduce the issue using clean environment using the following command: M-x lsp-start-plain

Bug description

When I move a scala file to another directory, Metals sends a WorkspaceEdit on willRenameFiles to change the file I'm moving. After Emacs applies this change, I see:

1. An unsaved buffer by the old path with the changes.
2. A new file by the new path without changes.

Steps to reproduce

https://github.com/scalameta/metals/issues/5137

Expected behavior

Emacs should first apply the changes from Metals and then rename the file. Not sure how Emacs should deal with the fact that after applying the changes to the buffer they are not saved yet, so rename-file moves an unmodified file. Is it possible to save the buffer after the changes have been applied?

Which Language Server did you use?

lsp-metals

OS

Linux

Error callstack

[Trace - 06:41:40 PM] Sending request 'workspace/willRenameFiles - (369)'.
Params: {
  "files": [
    {
      "oldUri": "file:///home/ea/src/metalstest/src/main/scala/com/example/first/App.scala",
      "newUri": "file:///home/ea/src/metalstest/src/main/scala/com/example/App.scala"
    }
  ]
}


[Trace - 06:41:40 PM] Received response 'workspace/willRenameFiles - (369)' in 3ms.
Result: {
  "changes": {
    "file:///home/ea/src/metalstest/src/main/scala/com/example/first/App.scala": [
      {
        "range": {
          "start": {
            "line": 0,
            "character": 0
          },
          "end": {
            "line": 0,
            "character": 25
          }
        },
        "newText": "package com.example"
      }
    ]
  }
}

Anything else?

No response

[ericdallo]

I repro the same with clojure-lsp

[Trace - 09:48:58 AM] Sending request 'workspace/willRenameFiles - (558)'.
Params: {
  "files": [
    {
      "oldUri": "file:///home/greg/dev/clojure-lsp/lib/test/clojure_lsp/feature/completion_test.clj",
      "newUri": "file:///home/greg/dev/clojure-lsp/lib/test/clojure_lsp/feature/foo/completion_test.clj"
    }
  ]
}


[Trace - 09:48:58 AM] Received response 'workspace/willRenameFiles - (558)' in 39ms.
Result: {
  "documentChanges": [
    {
      "textDocument": {
        "version": 0,
        "uri": "file:///home/greg/dev/clojure-lsp/lib/test/clojure_lsp/feature/completion_test.clj"
      },
      "edits": [
        {
          "range": {
            "start": {
              "line": 0,
              "character": 4
            },
            "end": {
              "line": 0,
              "character": 39
            }
          },
          "newText": "clojure-lsp.feature.foo.completion-test"
        }
      ]
    }
  ]
}

But I'm not sure if this is a lsp-mode bug or servers, although it works on my emacs the rename properly

[iarenaza]

Interestingly enough, if you perform an equivalent[1] operation, but use lsp-rename to rename the Clojure namespace instead of renaming the file, the response from the LSP server includes the same map with "textDocument" and "edits" entries, plus a second map with a "kind": "rename" entry plus the "oldUri" and "newUri" entries:

[Trace - 04:26:01 ] Sending request 'textDocument/rename - (10)'.
Params: {
  "textDocument": {
    "uri": "file:///home/magnet/test/clojure-lsp/lib/test/clojure_lsp/feature/completion_test.clj"
  },
  "position": {
    "line": 0,
    "character": 38
  },
  "newName": "clojure-lsp.feature.foo.completion-test"
}


[Trace - 04:26:01 ] Received response 'textDocument/rename - (10)' in 32ms.
Result: {
  "documentChanges": [
    {
      "textDocument": {
        "version": 0,
        "uri": "file:///home/magnet/test/clojure-lsp/lib/test/clojure_lsp/feature/completion_test.clj"
      },
      "edits": [
        {
          "range": {
            "start": {
              "line": 0,
              "character": 4
            },
            "end": {
              "line": 0,
              "character": 39
            }
          },
          "newText": "clojure-lsp.feature.foo.completion-test"
        }
      ]
    },
    {
      "kind": "rename",
      "oldUri": "file:///home/magnet/test/clojure-lsp/lib/test/clojure_lsp/feature/completion_test.clj",
      "newUri": "file:///home/magnet/test/clojure-lsp/lib/test/clojure_lsp/feature/foo/completion_test.clj"
    }
  ]
}

So maybe clojure-lsp should be returning that second map too[2], for this type of operation? (willRenameFiles)

Funnily enough, the processing of that second extra map (by the "rename" op, in lsp--apply-text-document-edit), executes rename-file. Which triggers the advice set by lsp-mode on rename-file.

But before rename-file/lsp--on-rename-file is run, lsp--apply-text-document-edit has already done all of the required work for the file on disk to be up to date (applied the edits to the original buffer attached to the file, saved it, notified the server that it saved and closed the file, etc). So by the time lsp--on-rename-file runs, the only thing that really needs to be done is the file renaming itself. And then lsp--apply-text-document-edit finishes the "rename" operation by reattaching the buffer to the new file path, etc.

[1] Equivalent for Clojure, as both result in the namespace change plus the file renaming (as there is usually a 1 to 1 mapping in Clojure between the namespace and the file name).

[2] Or a third, fourth, etc if more than one file renaming is involved in the requested renaming. E.g., if we are renaming a directory and we need to recursively rename all the files inside of it.

[iarenaza]

So maybe clojure-lsp should be returning that second map too[2], for this type of operation? (willRenameFiles)

Humm it seems I was wrog. If I'm reading https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#workspace_willRenameFiles correctly, the returned WorkspaceEdit contains the edits "which will be applied to workspace before the files are renamed.". Thus it seems to imply that the renaming itself is not part of the returned WorkspaceEdit, and that the renaming should be done client-side doing other methods.

[iarenaza]

These changes seem to make it work as expected:

Unstaged changes (1)
modified   lsp-mode.el
@@ -4310,20 +4310,8 @@ interface TextDocumentEdit {
                 (f-delete (lsp--uri-to-path uri) recursive?)))
     ("rename" (-let* (((&RenameFile :old-uri :new-uri :options? (&RenameFileOptions? :overwrite?)) edit)
                       (old-file-name (lsp--uri-to-path old-uri))
-                      (new-file-name (lsp--uri-to-path new-uri))
-                      (buf (find-buffer-visiting old-file-name)))
-                (when buf
-                  (lsp-with-current-buffer buf
-                    (save-buffer)
-                    (lsp--text-document-did-close)))
-                (mkdir (f-dirname new-file-name) t)
-                (rename-file old-file-name new-file-name overwrite?)
-                (when buf
-                  (lsp-with-current-buffer buf
-                    (set-buffer-modified-p nil)
-                    (setq lsp-buffer-uri nil)
-                    (set-visited-file-name new-file-name)
-                    (lsp)))))
+                      (new-file-name (lsp--uri-to-path new-uri)))
+                (rename-file old-file-name new-file-name overwrite?)))
     (_ (let ((file-name (->> edit
                              (lsp:text-document-edit-text-document)
                              (lsp:versioned-text-document-identifier-uri)
@@ -6345,7 +6333,19 @@ to check if server wants to apply any workspaceEdits after renamed."
                                      :newUri (lsp--path-to-uri new-name))))))
         (when-let ((edits (lsp-request "workspace/willRenameFiles" params)))
           (lsp--apply-workspace-edit edits 'rename-file)
-          (funcall old-func old-name new-name ok-if-already-exists?)
+          (let ((buf (find-buffer-visiting old-name)))
+            (when buf
+              (lsp-with-current-buffer buf
+                (save-buffer)
+                (lsp--text-document-did-close)))
+            (mkdir (f-dirname new-name) t)
+            (funcall old-func old-name new-name ok-if-already-exists?)
+            (when buf
+              (lsp-with-current-buffer buf
+                (set-buffer-modified-p nil)
+                (setq lsp-buffer-uri nil)
+                (set-visited-file-name new-name)
+                (lsp))))
           (when (lsp--send-did-rename-files-p)
             (lsp-notify "workspace/didRenameFiles" params))))
     (funcall old-func old-name new-name ok-if-already-exists?)))

@chessman could you try those changes and see if they fix the issue for you?