workspaceEdit textDocument version is always 0

[mfussenegger]

In the neovim lsp client we recently added resourceOperations support and that changed the workspaceEdit result from eclipse.jdt.ls to include documentChanges with a versioned textDocument where the version is always 0.

For example, if I open a file and type something like int x = 10 + 20 and then trigger a extract variable refactoring on 20, previously there was the following message flow:

...
[2021-03-19T12:26:15+0100] "didChange version", 16
[2021-03-19T12:26:16+0100] "didChange version", 17
[2021-03-19T12:26:16+0100] "didChange version", 18
[2021-03-19T12:26:16+0100] "didChange version", 19
[2021-03-19T12:26:16+0100] "didChange version", 20
[2021-03-19T12:26:16+0100] "didChange version", 21
[2021-03-19T12:26:19+0100] "workspaceEdit", {
  changes = {
    ["file:///path/to/Third.java"] = { {
        newText = "int i = 20;\n        int x = 10 + i",
        range = {
          end = {
            character = 23,
            line = 6
          },
          start = {
            character = 8,
            line = 6
          }
        }
      } }
  }
}
[2021-03-19T12:26:19+0100] "didChange version", 22

Now this changed to:

[2021-03-19T12:21:36+0100] "didChange version", 16
[2021-03-19T12:21:36+0100] "didChange version", 17
[2021-03-19T12:21:36+0100] "didChange version", 18
[2021-03-19T12:21:36+0100] "didChange version", 19
[2021-03-19T12:21:37+0100] "didChange version", 20
[2021-03-19T12:21:37+0100] "didChange version", 21
[2021-03-19T12:21:41+0100] "workspaceEdit", {
  changes = {},
  documentChanges = { {
      edits = { {
          newText = "int i = 20;\n        int x = 10 + i",
          range = {
            end = {
              character = 23,
              line = 6
            },
            start = {
              character = 8,
              line = 6
            }
          }
        } },
      textDocument = {
        uri = "file:///path/to/Third.java",
        version = 0
      }
    } }
}

Neovim outputs an error if the version is present and if it is lower than the expected buffer version.

If I'm not mistaken the version is fixed to 0 in eclipse.jdt.ls

https://github.com/eclipse/eclipse.jdt.ls/blob/8028f3ffc04e7f4695ffa08d058912e2003a0116/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/ChangeUtil.java#L280

Based on my interpretation of the spec, that neovim rejects the workspace edit as outdated is legitimate and eclipse.jdt.ls is not supposed to hardcode it to 0
I think given that textDocument is a OptionalVersionedTextDocumentIdentifier, eclipse.jdtls could send null as version instead of 0 if the version information is not available internally

See https://microsoft.github.io/language-server-protocol/specification#textDocumentEdit

[mfussenegger]

Any inputs on this? Do you consider this a bug in eclipse.jdt.ls or do you think that clients should accept/ignore version 0?

[rgrunber]

In VSCode, it looks like the text document starts at version 1 from the initial didOpen(..) and various code actions do specify a version of 0 without issue. However, I can't see where it says this is required, or that 0 is special ( https://microsoft.github.io/language-server-protocol/specification#versionedTextDocumentIdentifier ). In fact, it looks like the version should always be increasing so I think you're right that this likely needs to become null if we don't have access to the actual version.

@testforstephen any specific reason 0 is used as opposed to null (vscode-specific implementation?), and is it likely safe to switch over ?

[testforstephen]

any specific reason 0 is used as opposed to null (vscode-specific implementation?), and is it likely safe to switch over ?

Yes, "null" is better. On JDT language server side, we don't actually maintain version information for the files, so it's impossible to specify a correct version number in the workspace edit response. Having said that, it is more appropriate to give it "null".

However, the current LSP4j model only accepts "VersionedTextDocumentIdentifier" in TextDocumentEdit, which does not support null values. According to the LSP, it should be "OptionalVersionedTextDocumentIdentifier". This is a bug in LSP4j and we have to fix it in LSP4j first.

https://github.com/eclipse/lsp4j/blob/d44525140da577dbb69fcd851d7336ce9e633b74/org.eclipse.lsp4j/src/main/java/org/eclipse/lsp4j/Protocol.xtend#L6081

[rgrunber]

@testforstephen , when I look at https://github.com/eclipse/lsp4j/blob/593cdb65217fd406267812ee5394342617e32f71/org.eclipse.lsp4j/src/main/java/org/eclipse/lsp4j/adapters/VersionedTextDocumentIdentifierTypeAdapter.xtend#L26 , shouldn't this support a 'null' value even if the "OptionalVersionedTextDocumentIdentifier" isn't actually being used as a class ?

I tried replacing "0" with null and it seems to be working as expected.

[mfussenegger]

We figured out that the type got changed in 3.16 of the protocol to OptionalVersionedTextDocumentIdentifier and before that the version wasn't allowed to be null. So neovim now also treats 0 the same as null.

Might still make sense to change it but I wouldn't consider this as bug anymore.

[testforstephen]

@testforstephen , when I look at https://github.com/eclipse/lsp4j/blob/593cdb65217fd406267812ee5394342617e32f71/org.eclipse.lsp4j/src/main/java/org/eclipse/lsp4j/adapters/VersionedTextDocumentIdentifierTypeAdapter.xtend#L26 , shouldn't this support a 'null' value even if the "OptionalVersionedTextDocumentIdentifier" isn't actually being used as a class ?

I tried replacing "0" with null and it seems to be working as expected.

Ok, it uses the JSON Adapter to deal with null, not straightforward, but should work.