Bad parsing of DidChangeTextDocumentParams ContentChanges

[AlexGustafsson]

In order to parse either a TextDocumentContentChangeEvent or TextDocumentContentChangeEventWhole from the ContentChanges, the code uses the assumption that the marshaling fails if there are missing fields. This is not true.

https://github.com/tliron/glsp/blob/f4d3befef740ff5a47ecc04727761f371c2a6dda/protocol_3_16/text-document-synchronization.go#L105-L117

Example code where this assumption is shown.

for _, change := range parameters.ContentChanges {
    switch cast := change.(type) {
    case protocol.TextDocumentContentChangeEvent:
        // This will always occur, with zeroed ranges if sync kind is TextDocumentSyncKindFull
        return fmt.Errorf("incremental changes are not supported")
    case protocol.TextDocumentContentChangeEventWhole:
        // This never occurs
        document.Content = cast.Text
    }
}

A better approach for detecting would be to first have a common, private, struct much like TextDocumentContentChangeEvent, but where Range is a pointer. If the pointer is nil after marshaling, then return a TextDocumentContentChangeEventWhole, else return a TextDcoumentContentChangeEvent. That mechanism uses the documented behavior for the changes - range is never set for whole updates.

[AlexGustafsson]

The current workaround is to have an additional check to see if the range is all zeroes.

for _, change := range parameters.ContentChanges {
    switch cast := change.(type) {
    case protocol.TextDocumentContentChangeEvent:
        // https://github.com/tliron/glsp/issues/9
        if cast.Range.Start.Line == 0 && cast.Range.Start.Character == 0 && cast.Range.End.Line == 0 && cast.Range.End.Character == 0 {
            document.Content = cast.Text
            continue
        }
        // TODO: For now, don't support incremental changes
        return fmt.Errorf("incremental changes are not supported")
    case protocol.TextDocumentContentChangeEventWhole:
        document.Content = cast.Text
    }
}

[tliron]

You are correct in that we can only check for "Range" as the differentiator, because "RangeLength" can be omitted. However, I think there might be a better solution: define the Range as a pointer, like so:

type TextDocumentContentChangeEvent struct {
    Range *Range `json:"range"`
    RangeLength *UInteger `json:"rangeLength,omitempty"`
    Text string `json:"text"`
}

I did some experimentation and it seems the go/json will leave the pointer as nil if there is no "range" field in the JSON, so we can just check for nil. What do you think?

[AlexGustafsson]

That is how the source graph LSP project solves it, which has worked for me before. It’s already documented for the type as well that if range is nil, the update is for the entire document.

[tliron]

Please let me know if my fix works!

[AlexGustafsson]

This works. Thanks for fixing it so fast! Closing.