Support type safety for string types in `vscode-languageserver-protocol`

[remcohaszing]

vscode-languageserver-protocol is a great package for writing type-safe tests for language servers. I.e.:

import test from 'tape'
import {
  createProtocolConnection,
  InitializeRequest,
  IPCMessageReader,
  IPCMessageWriter
} from 'vscode-languageserver-protocol/node.js'

test(async (t) => {
  const proc = spawn(
    'node',
    [pathToLanguageServer, '--node-ipc'],
    { cwd, stdio: [null, 'inherit', 'inherit', 'ipc'] }
  )
  const connection = createProtocolConnection(
    new IPCMessageReader(proc),
    new IPCMessageWriter(proc)
  )
  t.teardown(() => {
    connection.end()
    proc.kill()
  })
  connection.listen()

  const initializeResponse = await connection.sendRequest(
    InitializeRequest.type,
    {
      processId: null,
      rootUri: null,
      capabilities: {},
      workspaceFolders: null
    }
  )
});

However, I personally prefer to make calls looks closer to the actual protocol:

  const initializeResponse = await connection.sendRequest(
-   InitializeRequest.type,
+   'initialize',
    {
      processId: null,
      rootUri: null,
      capabilities: {},
      workspaceFolders: null
    }
  )

This is allowed by vscode-languageserver-protocol, but at the cost of type safety.

I suggest to make requests and notifications for known types type-safe if they are passed as strings. This would work the same was as for example HTMLElementTagNameMap and document.createElement() in TypeScript’s builtin dom lib.

[dbaeumer]

If we go down that route we should basically deprecate the old RequestType based approach. The current approach got chosen when TS didn't offer a signature overload on string constants :-).

[remcohaszing]

I’d like to do this.

My suggested approach is to make 4 interfaces:

export interface ClientToServerRequests {
  /**
   * The initialize request is sent from the client to the server.
   * It is sent once as the request after starting up the server.
   * The requests parameter is of type [InitializeParams](#InitializeParams)
   * the response if of type [InitializeResult](#InitializeResult) of a Thenable that
   * resolves to such.
   */
  initialize: [InitializeParams, InitializeResult];

  // …
}

export interface ServerToClientRequests {
  /**
   * The show message request is sent from the server to the client to show a message
   * and a set of options actions to the user.
   */
  'window/showMessageRequest': [ShowMessageRequestParams, MessageActionItem | null];

  // …
}

export interface ClientToServerNotifications {
  /**
   * The document open notification is sent from the client to the server to signal
   * newly opened text documents. The document's truth is now managed by the client
   * and the server must not try to read the document's truth using the document's
   * uri. Open in this sense means it is managed by the client. It doesn't necessarily
   * mean that its content is presented in an editor. An open notification must not
   * be sent more than once without a corresponding close notification send before.
   * This means open and close notification must be balanced and the max open count
   * is one.
   */
  'textDocument/didOpen': DidOpenTextDocumentParams;
  // XXX I don’t understand what TextDocumentRegistrationOptions means in the original code.

  // …
}

export interface ServerToClientNotifications {
  /**
   * The show message notification is sent from a server to a client to ask
   * the client to display a particular message in the user interface.
   */
  'window/showMessage': ShowMessageParams;

  // …
}

How would you deprecate the RequestType based approach?

The simplest approach would be to just make the following change.

  /**
   * The initialize request is sent from the client to the server.
   * It is sent once as the request after starting up the server.
   * The requests parameter is of type [InitializeParams](#InitializeParams)
   * the response if of type [InitializeResult](#InitializeResult) of a Thenable that
   * resolves to such.
+  * 
+  * @deprecated
   */
  export namespace InitializeRequest {
    export const method: 'initialize' = 'initialize';
    export const messageDirection: MessageDirection = MessageDirection.clientToServer;
    export const type = new ProtocolRequestType<InitializeParams, InitializeResult, never, InitializeError, void>(method);
  }

This means documentation is duplicated though. It could be removed.

  /**
-  * The initialize request is sent from the client to the server.
-  * It is sent once as the request after starting up the server.
-  * The requests parameter is of type [InitializeParams](#InitializeParams)
-  * the response if of type [InitializeResult](#InitializeResult) of a Thenable that
-  * resolves to such.
+  * @deprecated
   */
  export namespace InitializeRequest {
    export const method: 'initialize' = 'initialize';
    export const messageDirection: MessageDirection = MessageDirection.clientToServer;
    export const type = new ProtocolRequestType<InitializeParams, InitializeResult, never, InitializeError, void>(method);
  }

It would even be possible to replace type with their respective methods. Because they are constants strings, this still provides the same level of type safety when invoking onNotification, sendNotification, onRequest, or sendRequest. However, this may be breaking in ways I don’t understand. I.e. are people allowed to define their own request types?

  export namespace InitializeRequest {
    export const method: 'initialize' = 'initialize';
    export const messageDirection: MessageDirection = MessageDirection.clientToServer;
-   export const type = new ProtocolRequestType<InitializeParams, InitializeResult, never, InitializeError, void>(method);
+   export const type = method;
  }

There is also a script to generate protocol/metaModel.json. It looks like it also depends on the explicit current code layout. However, this file is unpublished and appears to be unused. Is this file used somehow? Should this keep working?

---

The current approach got chosen when TS didn't offer a signature overload on string constants :-).

I don’t even remember this. This must have been a while ago. :sweat_smile:

[dbaeumer]

Can you please outline how we would do this on the calling side and how we ensure type safety there.

We also need to keep in mind that we have three layers of API. protocol, client & server. Would that mean that we need to define these string overloads on all three levels?

And the jsonrpc package currently uses the request types as well to allow usages outside of LSP to have a type save calling interface. What will we recommend to these clients what they should do?

[dbaeumer]

You might want to look at TS mapped types and this to get some inspiration how an different approach could look like. Here is an example on how to use it: https://github.com/microsoft/lsif-node/blob/af85b765aa3d1a0ecfa1e0b796ae372a5c31ed87/tsc/src/common/writerMessages.ts#L6

[remcohaszing]

I was thinking of something like this (playground):

/**
 * Stripped down version of LSP initialize parameters for demo purposes.
 */
export interface InitializeParams {
    /**
     * Information about the client
     *
     * @since 3.15.0
     */
    clientInfo?: {
        /**
         * The name of the client as defined by the client.
         */
        name: string;

        /**
         * The client's version as defined by the client.
         */
        version?: string;
    };
}

/**
 * Stripped down version of LSP result returned from an initialize request.
 */
export interface InitializeResult<T = any> {
    /**
     * Information about the server.
     *
     * @since 3.15.0
     */
    serverInfo?: {
        /**
         * The name of the server as defined by the server.
         */
        name: string;

        /**
         * The server's version as defined by the server.
         */
        version?: string;
    };
}

export interface ClientToServerRequests {
  /**
   * The initialize request is sent from the client to the server.
   * It is sent once as the request after starting up the server.
   * The requests parameter is of type [InitializeParams](#InitializeParams)
   * the response if of type [InitializeResult](#InitializeResult) of a Thenable that
   * resolves to such.
   */
  initialize: [InitializeParams, InitializeResult];

  // …
}

interface ProtocolConnection {
    sendRequest<T extends string>(
        type: T, 
        params: T extends keyof ClientToServerRequests ? ClientToServerRequests[T][0] : unknown
    ): Promise<T extends keyof ClientToServerRequests ? ClientToServerRequests[T][1] : any>;
}

namespace InitializeRequest {
    export const method = 'initialize';
    export const type = method;
}

// Below would be client code.

declare const connection: ProtocolConnection;

{
    const { serverInfo, invalid } = await connection.sendRequest('initialize', {
        clientInfo: {
            name: 'VSCode',
            version: '1.2.3'
        },
        invalid: 'This option is not allowed'
    })
}

{
    // This still works
    const { serverInfo, invalid } = await connection.sendRequest(InitializeRequest.type, {
        clientInfo: {
            name: 'VSCode',
            version: '1.2.3'
        },
        invalid: 'This option is not allowed'
    })
}

const thisIsAny = await connection.sendRequest('unknown string', 'Anything is allowed here')

It would work in a very similar way when using a mapped type as you linked. Either way is fine IMO.

If customized connection types are needed for vscode-jsonrpc, then MessageConnection could accept a generic to make it typed. This would have the added benefit that all method invocations of that instance would be typed.

interface MessageConnection<ClientToServerRequest, ServerToClientRequest, ClientToServerNotification, ServerToClientNotification> {
    sendRequest<T extends string>(
        type: T, 
        params: T extends keyof ClientToServerRequest ? ClientToServerRequest[T][0] : unknown
    ): Promise<T extends keyof ClientToServerRequest ? ClientToServerRequest[T][1] : any>;
}

In this case ProtocolConnection could be typed as:

interface ProtocolConnection extends MessageConnection<LSPClientToServerRequest, LSPServerToClientRequest, LSPClientToServerNotification, LSPServerToClientNotification> {}

[dbaeumer]

Actually that all goes into the right direction. I would actually prefer a solution that is more in line with what I liked to. Especially since I use the same approach here now as well: https://github.com/microsoft/vscode-wasm/blob/86da64653a6234f3dfe78ed4f609347ed10f4a28/sync-api-common/src/common/connection.ts#L598

Makes things overall more consistent.

[remcohaszing]

I gave this a try, but I’m having an issue dealing with unknown methods.

interface InitializeParams {
    foo: 'bar';
    fooz?: 'baz';
}

interface InitializeResult {
    ready: true
}

export interface InitializeRequest {
    method: 'initialize';
    params: InitializeParams;
    result: InitializeResult;
}

export interface ShutdownRequest {
    method: 'shutdown';
    result: 'shutdown success';
}

export interface CancellationToken {
    readonly isCancellationRequested: boolean;
}

export type ClientToServerRequest = InitializeRequest | ShutdownRequest;

declare function sendRequest<U, M extends string>(
    method: M,
    params: M extends Extract<ClientToServerRequest, { method: M }>['method'] ? Extract<ClientToServerRequest, { method: M, params: any }>['params'] : unknown,
    token?: CancellationToken
): Promise<M extends Extract<ClientToServerRequest, { method: M }>['method'] ? Extract<ClientToServerRequest, { method: M, params: any }>['result'] : unknown>;
declare function sendRequest<U, M extends string>(
    method: M extends Extract<ClientToServerRequest, { method: M, params: any }>['method'] ? never : M,
    token?: CancellationToken
): Promise<M extends Extract<ClientToServerRequest, { method: M }>['method'] ? Extract<ClientToServerRequest, { method: M }>['result'] : unknown>;
declare function sendRequest<R>(
    type: string,
    params: unknown,
    token?: CancellationToken
): Promise<R>;

sendRequest('initialize', {foo: 'bar', }, { isCancellationRequested: true })
// @ts-expect-error
sendRequest('initialize', { isCancellationRequested: true })
// @ts-expect-error
sendRequest('shutdown', {foo: 'bar'}, { isCancellationRequested: true })
sendRequest('shutdown')
sendRequest('shutdown', { isCancellationRequested: true })
// @ts-expect-error
sendRequest('initialize', {fpp: 'bar'})
sendRequest('asd')

playground link

In this case if the params are invalid, the type definition falls through to the one that accepts any string and unknown params. I tried a couple of things, but couldn’t figure it out. Do we still need this signature?

[dbaeumer]

I need to think a little about this. Out of the box I have no good idea and we can't remove the string signature easily since it is used for custom untyped message.

[remcohaszing]

I noticed the same issue I was encountering applies to the usage of ProtocolRequestType.

const result = await connection.sendRequest(InitializeRequest.type, 'This can be anything')

Just like with my fiddling in the playground, it means result is of type any. So this doesn’t really have to block this issue.

[dbaeumer]

I do get a proper error from TS (at least on the client):

[dbaeumer]

I can do

void clientConnection!.sendRequest(InitializeRequest.type, 'this can be anything');

on a protocol connection however, this is a typing oversight. The signatures

sendRequest<R>(method: MessageSignature | string, token?: CancellationToken): Promise<R>;

is actually bogus since MessageSignature is not very usefull since it is an interface without any params and return value typing. I should fix this on the RPC layer.

[dbaeumer]

Opened https://github.com/microsoft/vscode-languageserver-node/issues/1130