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.

glsp/protocol_3_16/text-document-synchronization.go

Lines 105 to 117 in f4d3bef

	for _, contentChange := range value.ContentChanges {
	var value_ TextDocumentContentChangeEvent
	if err = json.Unmarshal(contentChange, &value_); err == nil {
	self.ContentChanges = append(self.ContentChanges, value_)
	} else {
	var value_ TextDocumentContentChangeEventWhole
	if err = json.Unmarshal(contentChange, &value_); err == nil {
	self.ContentChanges = append(self.ContentChanges, value_)
	} else {
	return err
	}
	}
	}

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.