lsp4intellij icon indicating copy to clipboard operation
lsp4intellij copied to clipboard

Lookup insertion results sends 'rangeLength' ?

Open payne911 opened this issue 1 year ago • 3 comments

Typing System. and selecting out as the option to insert from the code completion lookup that appears in the IDE results in this being sent:

Notification {
  method: "textDocument/didChange",
  params: Object {
    "contentChanges": Array [
      Object {
        "range": Object {
          "end": Object {
            "character": Number(11),
            "line": Number(6)
          },
          "start": Object {
            "character": Number(11),
            "line": Number(6)
          }
        },
        "rangeLength": Number(3),
        "text": String("out")
      }
    ]
  }
}

It seems like rangeLength should not be specified given this is an insertion rather than a replacement (since range.start == range.end).

payne911 avatar Oct 18 '23 22:10 payne911

rangeLength is deprecated and can be ommited at all

nixel2007 avatar Oct 19 '23 06:10 nixel2007

In any case, I think something is wrong (source):

/**
 * An event describing a change to a text document. If only a text is provided
 * it is considered to be the full content of the document.
 */
export type [TextDocumentContentChangeEvent](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocumentContentChangeEvent) = {
	/**
	 * The range of the document that changed.
	 */
	range: [Range](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#range);

	/**
	 * The optional length of the range that got replaced.
	 *
	 * @deprecated use range instead.
	 */
	rangeLength?: [uinteger](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#uinteger);

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

Unless I'm misunderstanding what rangeLength is for, it seems like it is currently being used by lsp4intellij to designate the text.length, but the documentation says:

The optional length of the range that got replaced.

Which given it was an insertion (rather than a replacement), sounds like the value should have been 0 instead of 3 (if we look into the example I provided in the original message).

payne911 avatar Oct 20 '23 15:10 payne911

Moreover, are we certain that we shouldn't use documentChanged only for FullSync, and that beforeDocumentChanged is the one that should be used for IncrementalSync (since I believe the values should be calculated based on the "old" document version's offset values) ?

payne911 avatar Oct 20 '23 16:10 payne911