Language-server-protocol: TextDocumentContentChangeEvent#rangeLength redundant?

[svenefftinge]

It seems like 'rangeLength' is redundant, as it can be computed from the 'range'. Should be deprecated.

[svenefftinge]

DidChangeTextDocumentParams could maybe reuse TextEdit

[dbaeumer]

Agree, but the reason we sent this is to avoid unnecessary work on a server which has a [offset, length] text model instead of a [[startLine, startColumn], [endLine, endColumn]] model.

[dbaeumer]

Will look if it makes sense to reuse TextEdit

[kdvolder]

Okay so I get that it might be convenient to include redundant info. However, the fact that its redundant also makes it a little unclear what the expectations are from client/server point of view. In particular, is it okay for a client to only specify range but not rangeLength (since the info is redundant, if client didn't feel like computing it, the client implementor might assume that this is okay and the server must deal with that).

I see two possible interpretations depending on who's point of view we take re 'convenience' here:

1) Point of view favoring convenience of server implementations:

client: MUST set both range and rangelength even though rangelength is redundant.
server: MAY ignore rangelength or 'end' posisition of the range, whichever is more convenient.

2) Point of view favoring the convenience of client implementations:

client: MAY optionally set rangelength but may ommit this redundant piece of information if it wishes.
server: MUST be able to handle it when optional rangelength is missing (by computing it from the range, if they need it).

My guess is that the intention here is to use the first interpretation (based on what was said earlier, which implies this is for the convenience of server implementation). Whether I'm guessing it right or not... it would be good to make this very clear in the Spec.

Alternatively, maybe its just easier / better avoid this complexity in the Spec and just deprecate/remove the redundant rangeLength. Everything in LSP protocol is already in terms of line/col posititions so this is a bit of a weird deviation from that anyhow. And since a lsp-server that uses a 'offset-based' model instead of line/col will already need to keep a table to convert between them, it doesn't really seem to make a lot of difference if the server has two positions to convert instead of one.

[alanz]

I understand this to function something like the emacs change hooks which states

Each function receives three arguments: the beginning and end of the region just changed, and the length
of the text that existed before the change. All three arguments are integers. The buffer that has been
changed is always the current buffer when the function is called.

The length of the old text is the difference between the buffer positions before and after that text as it was
before the change. As for the changed text, its length is simply the difference between the first two
arguments.

So the changes corresponding to adding a line, and then deleting it are

2017-04-22 16:09:29 [ThreadId 11] DEBUG haskell-lsp - ---> {"jsonrpc":"2.0","method":"textDocument/didChange","params":
{"textDocument":{"uri":"file:///home/alanz/tmp/haskell-hie-test-project/src/Foo.hs","version":3}
,"contentChanges":[{"range":{"start":{"line":5,"character":6},"end":{"line":5,"character":6}},"rangeLength":0,"text":"\nbb = 5"}]}}

2017-04-22 16:09:31 [ThreadId 11] DEBUG haskell-lsp - ---> {"jsonrpc":"2.0","method":"textDocument/didChange","params":
{"textDocument":{"uri":"file:///home/alanz/tmp/haskell-hie-test-project/src/Foo.hs","version":6}
,"contentChanges":[{"range":{"start":{"line":6,"character":0},"end":{"line":7,"character":0}},"rangeLength":7,"text":""}]}}

[alanz]

And it struck me that one of TextDocumentContentChangeEvent and TextEdit is redundant, in that the same mechanism should be used for a WorkspaceEdit change and a text document change.

[albel727]

It only seems redundant, because range is defined to always have the end position. But due to the way edit hooks are in Emacs, it is not possible (or at least very hard) to correctly determine the end position. The line/character numbers can be calculated only for the newly edited state, which is not what is wanted. Only the start position and the number of affected characters can be calculated reliably in Emacs.

(@alanz so emacs/lsp-mode calculation now is incorrect, it makes deletions at the end of a line produce end position = beginning of the next line, which makes it look like deletion of EOL, i.e line join).

So, the properly universal OnChange event should split range argument, i.e. be as follows:

start?: Position;
end?: Position;
rangeLength?: number;
text: string;

Any single one of start/end/rangeLength can be omitted if the other two values are specified, because it can be calculated from them, i.e.

• rangeLength = end - start or
• end = start + rangeLength or
• start = end - rangeLength.

Any two of them could be omitted.

• If only start is present, then replacement is assumed to be from start and to the end of the document.
• If only end is present, then replacement is assumed to be from the beginning of the document to the end.
• If only rangeLength is present for some reason, the whole document is to be replaced, and rangeLength should be equal to the length of the whole document.

Or they all can be omitted if the whole document is to be replaced.

[albel727]

PS. That said, this is probably an overgeneralization. I think it would be general enough to always demand presence of start and either one of end/rangeLength to cover all realistic uses.

[astahlman]

I agree with the analysis from @albel727, though it took me a while to fully understand the problem with Emacs Change Hooks. Here are some details that I hope someone else will find useful.

Consider the case of a single-line deletion. Here are the buffer contents at time 1.

1234
6789

And here are the buffer contents at time 2, after line 1 is deleted. (Note that a newline character is at position 5).

6789

From the Emacs documentation on Change Hooks:

Three arguments are passed to each function: the positions of
the beginning and end of the range of changed text,
and the length in chars of the pre-change text replaced by that range.
(For an insertion, the pre-change length is zero;
for a deletion, that length is the number of chars deleted,
and the post-change beginning and end are at the same place.)

So after the line deletion, our after-change-function is invoked with the following arguments:

• start = 1
• end = 1
• length = 5

The desired payload of the TextDocumentContentChangeEvent for this edit looks like this:

{
    'range': {
        'start': {'line': 0, 'character': 0},
        'end': {'line': 1, 'character': 0}
    },
  'rangeLength': 5,
  'text': ''
}

range.start can be calculated given the value of start and the current buffer contents. rangeLength is equal to the value of length. The problem is in calculating range.end; there is no way to reconstruct the pre-edit state of the buffer given the information available.

As proof, instead of

1234
6789

consider the following, alternate, prior state of the buffer:

1
34
6789

Deleting the first 2 lines would produce the same buffer state as before:

6789

And the arguments to our after-change-function would be the same:

• start: 1
• end: 1
• length: 5

But the correct payload for the TextDocumentContentChangeEvent corresponding to this change is:

{
    'range': {
        'start': {'line': 0, 'character': 0},
        'end': {'line': 2, 'character': 0}
    },
  'rangeLength': 5,
  'text': ''
}

(Note the difference in the range.end field - line: 2 instead of line: 1.)

Thus, the best we can do using Emacs Text Hooks is to correctly specify the range.start and rangeLength and populate range.end with some garbage value.

Given a range.start, knowledge of either rangeLength or range.end should be sufficient for the server to update its representation of the document. However, I don't think it's reasonable to expect that each server implementation will ignore range.end unless the specification is amended.

[kdvolder]

However, I don't think it's reasonable to expect that each server implementation will ignore range.end unless the specification is amended

I concur, this not at all implied by the current wording. Our language servers handling of change events, for example, totally ignores rangeLength. It instead solely uses the range (start and end). The spec currently seems to say (in not so many words) that a client should either include both (for a partial document replacement) or neither (for a whole doument replacement). So, if I'm reading that correctly, then it should be fine for a server to use whichever of the 'redundant' bits of info they want to use and ignore the rest.

The emacs situation, gives a strong argument in favor of allowing more flexibility for the client to omit one of the redundant infos if its hard for them to compute it. But that should be made clear in the spec, so server implementors know how to properly deal with that.

[alanz]

Given the state of emacs, I would like to suggest that we disallow the range end for a delete, as this is not available in emacs, and is redundant for managing the changes.

See also https://github.com/emacs-lsp/lsp-mode/issues/114

[vincentwoo]

I just want to chime that I, as a LSP client writer, think that this is bonkers also

[dbaeumer]

I agree that this is confusing and today I would not add rangeLength again to make this more clear.

If we want to allow range.start & rangeLength as a proper alternative then we need to make this a server capability. I am pretty sure that all servers right now expect range.end to be set properly.

[dbaeumer]

To get out of this I changed it as follows:

/**
 * An event describing a change to a text document. If range and rangeLength are omitted
 * the new text is considered to be the full content of the document.
 */
export type TextDocumentContentChangeEvent = {
    /**
     * The range of the document that changed.
     */
    range: Range;

    /**
     * The optional length of the range that got replaced.
     *
     * @deprecated use range instead.
     */
    rangeLength?: number;

    /**
     * The new text for the provided range.
     */
    text: string;
} | {
    /**
     * The new text of the whole document.
     */
    text: string;
}

I think that better con ways what we want to do here.

[dbaeumer]

Closing since in 3.15 the value is deprecated.