Using NoInfer for better typing

[itrapashko]

There are some generic methods that accept parameter named type, which type defines types of request handler, including parameter types and result type.

Here is an example of such method:

onRequest<P, R, PR, E, RO>(type: ProtocolRequestType<P, R, PR, E, RO>, handler: RequestHandler<P, R, E>): Disposable

TypeScript compiler infers generic arguments from both the type and handler parameters. This can result in the inferred types being wider than what is allowed by ProtocolRequestType.

For example, the following code compiles but should not, because the return type of the handler, ls.Hover | undefined | string, is not assignable to the type Hover | null specified in ls.HoverRequest.type.

const connection: Connection = null as any;
const handler: (params: HoverParams) => Promise<Hover | undefined | string> = null as any;
connection.onRequest(HoverRequest.type, handler); // no error

If we wrap type of the handler parameter in NoInfer, TypeScript will not infer onRequest method generic arguments from handler parameter and will correctly report an error.

Here is another example:

const params: HoverParams | string = null as any;
await connection.sendRequest(HoverRequest.type, params); // no error

It’s also necessary to adjust the HandlerResult type to allow returning undefined wherever returning null is permitted, since the library code automatically replaces all undefined values with null.
This can be done as follows:

export type HandlerResult<R, E, R_ = R extends null ? (R | undefined) : R> = R_ | ResponseError<E> | Thenable<R_> | Thenable<ResponseError<E>> | Thenable<R_ | ResponseError<E>>

After applying these changes locally, I was able to find several incorrect return types in my request handlers.

If you agree with the proposed changes, I would like to make a PR implementing them.

[dbaeumer]

@itrapashko thanks for the detailed analysis. Can you outline to me how that change would actually look like that you are proposing?

[itrapashko]

@itrapashko thanks for the detailed analysis. Can you outline to me how that change would actually look like that you are proposing?

@dbaeumer Here is an example.

Current Connection.onRequest methods:

onRequest<R, PR, E, RO>(type: ProtocolRequestType0<R, PR, E, RO>, handler: RequestHandler0<R, E>): Disposable;
onRequest<P, R, PR, E, RO>(type: ProtocolRequestType<P, R, PR, E, RO>, handler: RequestHandler<P, R, E>): Disposable;
onRequest<R, PR, E, RO>(type: RequestType0<R, E>, handler: RequestHandler0<R, E>): Disposable;
onRequest<P, R, E>(type: RequestType<P, R, E>, handler: RequestHandler<P, R, E>): Disposable;

Connection.onRequest methods with NoInfer:

onRequest<R, PR, E, RO>(type: ProtocolRequestType0<R, PR, E, RO>, handler: NoInfer<RequestHandler0<R, E>>): Disposable;
onRequest<P, R, PR, E, RO>(type: ProtocolRequestType<P, R, PR, E, RO>, handler: NoInfer<RequestHandler<P, R, E>>): Disposable;
onRequest<R, PR, E, RO>(type: RequestType0<R, E>, handler: NoInfer<RequestHandler0<R, E>>): Disposable;
onRequest<P, R, E>(type: RequestType<P, R, E>, handler: NoInfer<RequestHandler<P, R, E>>): Disposable;

As you can see, the only difference is that types of handler parameters are wrapped in NoInfer type.

This should be applied to all generic methods that accept type as first parameter: onRequest, sendRequest, onNotification, sendNotification, onProgress, sendProgress. The following classes will be affected: _Connection in server.ts, ProtocolConnection, MessageConnection, FeatureClient, BaseLanguageClient.

[dbaeumer]

Thanks. That make sense. Looking forward to the PR.