Update the textDocumentSync capability to spec v3

[dariooddenino]

Currently the language servers sends TextDocumentSyncKind as its textDocumentSync capability.

The current LSP spec (https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#serverCapabilities) expects a TextDocumentSyncOptions object instead.

TextDocumentSyncKind doesn't appear to be deprecated, but it's still accepted only for backwards compatibility.

This prevents some clients (for example the helix editor) from working with the purescript language server.

I have made some tests, and this looks like the minimal working config that vscode, emacs and helix accept happily. I'm a bit confused, as these options should correspond to a TextDocumentSyncKind.None and the server is sending TextDocumentSyncKind.Full now.

[oati]

see also: https://github.com/helix-editor/helix/pull/3541

[nwolverson]

Certainly no objection to updating to the newer object version but not before understanding the sync kind, which I agree seems to correspond to TextDocumentSyncKind.None, this is just not right.

[dariooddenino]

Some loud thinking here, hoping it can help with figuring this out.

These are the the config options from the current spec:

export interface TextDocumentSyncOptions{
    /**
     * Open and close notifications are sent to the server. If omitted open
     * close notification should not be sent.
     */
    openClose?: boolean;
    /**
     * Change notifications are sent to the server. See
     * TextDocumentSyncKind.None, TextDocumentSyncKind.Full and
     * TextDocumentSyncKind.Incremental. If omitted it defaults to
     * TextDocumentSyncKind.None.
     */
    change?: TextDocumentSyncKind;
    /**
     * If present will save notifications are sent to the server. If omitted
     * the notification should not be sent.
     */
    willSave?: boolean;
    /**
     * If present will save wait until requests are sent to the server. If
     * omitted the request should not be sent.
     */
    willSaveWaitUntil?: boolean;
    /**
     * If present save notifications are sent to the server. If omitted the
     * notification should not be sent.
     */
    save?: boolean | SaveOptions;
}
export interface SaveOptions {
    /**
     * The client is supposed to include the content on save.
     */
    includeText?: boolean;
}

The old specification is not very detailed on this regard, so I'm not sure how the old value translates to the new config.

Looking at the server's code: handleDocumentSave passes the document object to the fast rebuild, and following from there it looks like the actual file content is used in a few places. I wonder if those functions are actually ever reached or if it's just legacy code, since everything seems to work fine regardless of the includeText value passed.

From a brief look at the code the server also has a handleDocumentChange and handleDocumentClose functions, but apparently no handleDocumentOpen.

From the specification the server must either implement all three of them or none:

Client support for textDocument/didOpen, textDocument/didChange and textDocument/didClose notifications is mandatory in the protocol and clients can not opt out supporting them. This includes both full and incremental synchronization in the textDocument/didChange notification. In addition a server must either implement all three of them or none.

Aside from that, what confuses me is that omitting the openClose and change values from textDocumentSync should correspond to no notification sent for those events, but it looks like they're still triggered unless explicitly set to false and None. I.e.

textDocumentSync: {
  openClose: false,
  change: TextDocumentSyncKind.None
}

I wonder if this pr should be closed and this discussion moved to an issue.

[nwolverson]

Please leave the PR open, I expect we should just explicitly list change: TextDocumentSyncKind.Full to maintain the current behaviour. I am minded to move to the new object form, but specify the set of options that we understand to be the current behaviour.

At a glance it looks like not specifying the server capabilities may be simply using the client capabilities if specified, though I also see the document store directly overriding the actual sync mode. I'd like to test this myself.

From the specification the server must either implement all three of them or none:

These are implemented, but the document store subscribes to them, and the handlers you see in the purescript-language-server code are on the document store (essntially proxying the events as well as keeping the document store updated).

The document store is the main concern regarding the SyncKind - so long as that is correctly updated, all is hopefully fine.

I wonder if those functions are actually ever reached or if it's just legacy code, since everything seems to work fine regardless of the includeText value passed.

If there is a guarantee that a change notification is always sent before a save notification (and never eg debounced), I could imagine this is not necessary. I would rather not make a change to that without a good reason (observed performance or a client compatibility issue) and determining that the save notification never has an updated document version.

[Maskhjarna]

Any recent developments on this front? I'd love to be able to use Helix with a LS for Purescript, but I don't feel confident touching anything here.

[dariooddenino]

Any recent developments on this front? I'd love to be able to use Helix with a LS for Purescript, but I don't feel confident touching anything here.

Until there's an official solution you can pretty safely use this PR. I'm using it daily without issues

[nwolverson]

I can confirm the initial patch does not work as-is with vscode, because text documents are not synced (per the comment, "If omitted it defaults to TextDocumentSyncKind.None") - really no features work for this reason. If helix is still syncing documents with the original capabilities

 textDocumentSync: {
            save: { includeText: false }
          },

I think this is a bug that should be reported in helix.

In addition the documentsstore requires openClose: true, again in vscode lacking this capability the open/close events are not sent and basically nothing works.