search

clangd / vscode-clangd

Visual Studio Code extension for clangd
https://marketplace.visualstudio.com/items?itemName=llvm-vs-code-extensions.vscode-clangd
MIT License
641 stars 113 forks source link

The language client exposed by the plugin's API can be null #721

Closed HighCommander4 closed 5 days ago

HighCommander4 commented 1 week ago

While reviewing #636, I realized that even though the extension's public API is specified to expose a non-null BaseLanguageClient object (rather than BaseLanguageClient | null), the value can in fact be null if the server binary cannot be found and this codepath is taken during plugin activation.

(This technically shouldn't type-check, but it does because ClangdContext.client is declared as client!: ClangdLanguageClient;, which I guess overrides the type checker.)

In #636, we are adding a second codepath where client can be null (the patch is adding a "clangd.enable" setting that disables operation of the server if set to false).

@thegecko do you have any thoughts on how the API should deal with this? Should we change the type of ClangdApiV1.languageClient to BaseLanguageClient | null, to give API users a heads up that the value could be null? Or alternatively should we have activate() return ClangdExtension | null, where if we don't have a client we return null there?

thegecko commented 1 week ago

My personal opinion is to undertake the following:

  1. remove the forced client!. This is bad form and could lead to other issues
  2. AFAICT the languageClient can never be populated without a client, so we need to decide how to indicate this to the user

I can see two approaches to the last situation:

  1. Make the languageClient type optional or BaseLanguageClient | null. This effectively changes the API and we should be good citizens and bump the API version (or is it really we are reflecting reality?)
  2. Throw an error when accessing languageClient and client is undefined. This approach keeps the API and allows the user to catch and read an error why it failed. However I think the addition of a try..catch complicates the API usage.

My preference I think is to use the first option.

LMK if you want a PR.

HighCommander4 commented 1 week ago
  1. Make the languageClient type optional or BaseLanguageClient | null.

+1

This effectively changes the API and we should be good citizens and bump the API version (or is it really we are reflecting reality?)

I think we can keep things simple and do it without bumping the API version; the API is unlikely to have a lot of consumers to break at this point.

LMK if you want a PR.

That would be great, thanks.

thegecko commented 1 week ago

Opened #727. Quite a bit of churn removing the non-null assertions, though.

HighCommander4 commented 5 days ago

Opened #727. Quite a bit of churn removing the non-null assertions, though.

Indeed, it would be preferable to avoid having to check the client in so many places.

I wrote up a different approach at https://github.com/clangd/vscode-clangd/pull/728, perhaps you could have a look?