`VersionedTextDocumentIdentifier` version is always `null`

[ckipp01]

Describe the bug According to the spec when a VersionedTextDocumentIdentifier is used like we do in in the RenameProvider.scala, the version can either be null to indicate the version is known or a number to indicate another change. From the spec:

/**
     * The version number of this document. If a versioned text document identifier
     * is sent from the server to the client and the file is not open in the editor
     * (the server has not received an open notification before) the server can send
     * `null` to indicate that the version is known and the content on disk is the
     * master (as speced with document content ownership).
     *
     * The version number of a document will increase after each change, including
     * undo/redo. The number doesn't need to be consecutive.
     */

In the rename provider we always just return null no matter what. Even if we undo the change, redo it again, or just submit a whole other rename. This doesn't seem to be a problem for us for a couple reasons.

1. We don't use VersionedTextDocumentIdentifiers all over the place.
2. Most clients handle this as expected, since they account for null, and if null, then they just assume everything is fine and that they have "master", and do the edit.

Here is the relevant code:

https://github.com/scalameta/metals/blob/411c792e144a28d3c35f7a3ef4846b775e139de0/metals/src/main/scala/scala/meta/internal/rename/RenameProvider.scala#L177-L187

I ran into this because currently there is a bug in the built-in lsp support in Nvim, where they weren't accounting for this being null, and renames fail with Metals. Again, that's not necessarily Metal's fault, but it does point to use never actually setting the version.

To Reproduce Steps to reproduce the behavior:

1. Issue a rename and watch the trace file. You'll see something like I have below.

[Trace - 00:06:58 PM] Sending response 'textDocument/rename - (9)'. Processing request took 19ms
Result: {
  "documentChanges": [
    {
      "textDocument": {
        "version": null, <-- notice the null
        "uri": "file:///Users/ckipp/Documents/scala-workspace/test-project/example/src/Main.scala"
      },
      "edits": [
        {
          "range": {
            "start": {
              "line": 18,
              "character": 10
            },
            "end": {
              "line": 18,
              "character": 13
            }
          },
          "newText": "hi"
        },
        {
          "range": {
            "start": {
              "line": 17,
              "character": 6
            },
            "end": {
              "line": 17,
              "character": 9
            }
          },
          "newText": "hi"
        }
      ]
    }
  ]
}

Expected behavior That after some changes, undos, renames etc, that the VersionedTextDocumentIdentifier would indeed be versioned.

Search terms LSP, VersionedTextDocumentIdentifier, rename, nvim

[tgodzik]

Thanks for bringing that up! We don't track the version numbers.

What would be the expected behavior here in case of renames? Would we just increase the number by one and return that? Or should it be just the same number as we received?

Anyway, it should be easy to implement, though I don't think we've seen a purpose to do it yet.

[ckipp01]

So looking at the internals of the Nvim LSP implementation it looks like if we triggered a rename and document locally had been changed in-between, then the buffer version locally would be higher than the version we are sending back. It'd then reject that edit.

https://github.com/neovim/neovim/blob/3de9452abf182519fd9a015fe489a37d14aa78d9/runtime/lua/vim/lsp/util.lua#L159-L167

So I think that's a sensible use-case.

[tgodzik]

So looking at the internals of the Nvim LSP implementation it looks like if we triggered a rename and document locally had been changed in-between, then the buffer version locally would be higher than the version we are sending back. It'd then reject that edit.

https://github.com/neovim/neovim/blob/3de9452abf182519fd9a015fe489a37d14aa78d9/runtime/lua/vim/lsp/util.lua#L159-L167

So I think that's a sensible use-case.

We most likely haven't been hit by this since the renames are usually quick.

[ckipp01]

We most likely haven't been hit by this since the renames are usually quick.

Metals is that fast. 😎