Add support for multiple language servers per language configuration (closes #1396)

[Philipp-M]

This PR adds support for multiple language servers per language.

Language Servers are now configured in a separate table in languages.toml:

[langauge-server.mylang-lsp]
command = "mylang-lsp"
args = ["--stdio"]
config = { provideFormatter = true }

[language-server.efm-lsp-prettier]
command = "efm-langserver"

[language-server.efm-lsp-prettier.config]
documentFormatting = true
languages = { typescript = [ { formatCommand ="prettier --stdin-filepath ${INPUT}", formatStdin = true } ] }

The language server for a language is configured like this (typescript-language-server is configured by default):

[[language]]
name = "typescript"
language-servers = [ { name = "efm-lsp-prettier", only-features = [ "format" ] }, "typescript-language-server" ]

or equivalent:

[[language]]
name = "typescript"
language-servers = [ { name = "typescript-language-server", except-features = [ "format" ] }, "efm-lsp-prettier" ]

Each requested LSP feature is priorized in the order of the language-servers array. For example the first goto-definition supported language server (in this case typescript-language-server) will be taken for the relevant LSP request (command goto_definition).

If no except-features or only-features is given all features for the language server are enabled. (sometimes with fallback to the next LSP depending on the supported capabilities, probably a follow-up PR should check capabilities in the order of the supported LSPs, to make this default for all LSP requests)

The list of supported features are:

• format
• goto-definition
• goto-type-definition
• goto-reference
• goto-implementation
• signature-help
• hover
• completion
• code-action
• document-symbols
• workspace-symbols
• diagnostics
• rename-symbol

Another side-effect/difference that comes with this PR, is that only one language server instance is started if different languages use the same language server.

[the-mikedavis]

It might be good to survey how other LSP clients handle multiple LSPs. I suspect that the features should be configurable so you can choose which language server configuration is used to format or request signature-help or autocompletion. Requesting auto-complete from multiple language servers and merging the values could be disirable as well as code actions and diagnostics.

[andreytkachenko]

My opinion

1. I think we need to create completion source manager where be registered different sources based on some trait(even from plugins). And LSPs should be entries in that list as well as other with priority.
2. About conflicting functionality, I think easiest way would be to perform actions in order they appear in configuration file. But better is to be able setup preferred server for some actions like formatting, etc.

[Philipp-M]

I think we need to create completion source manager where be registered different sources based on some trait(even from plugins). And LSPs should be entries in that list as well as other with priority.

Yes, I also think there needs to be something like that, currently one completion request future fills the completion menu. I'm pretty sure in the near future there will be multiple completion sources (like file-path completion, which is useful most of the time I think).

Same applies of course for code-actions and diagnostics.

Maybe this should be done before this PR?

Btw. I've tested this PR so far with typescript-language-server and eslint (for diagnostics) via efm-langserver (in this order) which seems to be working quite well already.

[the-mikedavis]

There is https://github.com/helix-editor/helix/issues/1015 and https://github.com/helix-editor/helix/pull/2403 along those lines. I don't think anyone is working on file-path completion at the moment but it should be simpler to implement than #2403 and could help with multiple completion sources as you say.

[Philipp-M]

An update:

Merging of completion results, code-action and symbols in the symbol picker(s) are working now.

I have slightly refactored the UI components Menu, Completion and (File)Picker to be able to extend options/items after the respective component has been created. I've also added support to call score without resetting the cursor inside the menu or picker (relevant, when an LSP takes some time, while already selecting options). It may make sense to put these commits ( d97da46, a376e02 and a705b70) into its/their own PR.

Merging works like this currently:

spawn all lsp-request futures,

and then collect them in a loop, the first finished future, creates the menu/picker/etc. and the next futures extend it.

This could potentially mean, that if the menu/picker etc. was closed in the meantime that a new menu/picker is spawned, if one of the LSP takes too long for a request. This could be tackled with Mutexes (see in code_action, here: https://github.com/helix-editor/helix/pull/2507/commits/5040679fd1398064e025c0e424cfaae409edc87c#diff-abe37ebb421b375471daa248c8de6541335e61dd3dfc93ee999eb108876e5837R213)

I'm not sure if it makes sense to refactor the way the ui collects its data further (e.g. via streams or something), it may get too complex this way I guess...

[Philipp-M]

Another thing I've noticed, a few other editors have separated LSPs from the languages (like neovim).

This may make sense, if an LSP should be used for multiple languages at once (e.g. efm-langserver), in the case of efm-langserver, it would mean less unnecessary spawned LSPs, but I'm not sure if it's worth the effort, to support that as well. I'm not sure if there's a case, where it is relevant that the same LSP instance is used for multiple languages (e.g. html/css/js). If that's the case, I think we should support this.

E.g. extend languages.toml like:

[[language-server]]
name="efm-frontend" # could be used as unique id
command="efm-langserver"
config = {..}

# ...

[[language]]
name = "typescript"
language-servers = [
    {
        command = "typescript-language-server",
        args = ["--stdio"],
        language-id = "javascript"
    },
    "efm-frontend"
]

[Philipp-M]

I think I got a little bit ahead of myself...

I have now implemented pretty much everything I think is necessary to support multiple language servers.

This includes the change suggested in the previous comment, with the exception, that every language server is now configured in a separate table language-server (for simplicity and consistency).

I think the rest is hopefully documented well enough in adding_languages.md.

I will update the PR message if all is good

[Philipp-M]

Is there anything I can do, that makes reviewing this PR easier? Like splitting not directly related parts into its own PR? (Since keeping this up-to-date with master is quite some work...)

[sudormrfbin]

Splitting the PR into multiple smaller ones (even dependent ones, where one PR depends on another to be merged first) would definitely make reviewing easier. Some PRs cannot do this and that's fine, but where possible breaking them down is always better.

[willparsons]

This is the only thing I'm currently missing for my workflow, thanks for this 👍🏻

[space-shell]

Hi, is there any way that I can ( as a non-dev on this project ) help review this PR?

[Philipp-M]

Hi, is there any way that I can ( as a non-dev on this project ) help review this PR?

I guess testing this as thorough as possible is probably not a bad idea. I've done a little bit of testing, but not every feature this PR should cover...

[willparsons]

Is it worth making a new feature branch for this and adding each feature as it's own PR to that branch? I think it would make this easier to digest, as it stands I think it's too hard to review.

[Philipp-M]

I think the parts that were easily separable are/were already in their own PR (#3080, #3082). The only additional thing, that may make sense to separate, I think is the refactoring of the completion menu (in particular, wrapping the lsp::CompletionItem into an enum), but I tried to separate it, and I don't think it's really worth the extra effort.

The rest is rather difficult to separate meaningfully, and I think it's better to review the whole thing at once.

[willparsons]

fair enough 👍🏻

[nrdxp]

just a heads up to interested parties, now would be a good time to attempt a review as it has just been recently rebased, I've been using this for several months now along with the path completion PR this depends on and haven't seen any issues.

Also, this feature could potentially open the door for running an lsp for injected languages (although I'm not sure if the current architecture would make that feasible). As for me, I'll see if I can find some time later today to at least run through the code, but it probably needs someone who is more familiar with helix internals.

[willparsons]

Do people mind non-maintainer reviews? (not done much OSS)

[nrdxp]

Do people mind non-maintainer reviews? (not done much OSS)

Certainly wouldn't hurt anything. Someone with merge access ultimately has to pull the trigger anyway.

[Philipp-M]

Whoops, thanks for finding the typos

[lhiamgeek]

when will this feature be in release, can't wait to try it out

[aotarola]

@lhiamgeek you can try it out by compiling this branch, I've been using it for a few days now and it is pretty solid! 👍🏼

[lhiamgeek]

@aotarola okay，I’ll give it a try

[aotarola]

I've been using this branch for the past month and I love it!

Today I stumbled against a limitation of the current config scheme: I've been trying to use two set of lsp's for the same language: Javascript.

Here are the (hopefully valid) reasons why I want to do this:

1. My company relies heavily on eslint for formatting and diagnostics coming from tsserver. I'm able to get it working with efm-langserver no problem at all !
2. I like to tinker with deno projects which are usually not eslint friendly, cos they relay in deno lsp for that task, which I totally like, cos it is a real lsp for ts/js and it is way faster 😸 .

Would it make sense to create a group of language servers that kicks off depending on the root marker? The Deno team kinda suggest that config in neovim, sth like:

language-servers = [
  {
   roots = ["package.json"], 
   servers = [
     { name = "efm-eslint", only-features = ["format", "diagnostics"] },
     { name = "typescript-lsp", except-features = ["format", "diagnostics"] }
   ]
  },
  {
   roots = ["deno.json","deno.jsonc"],
   servers = ["typescript-deno-lsp"]
  }
]

I know, it kinda looks convoluted, but it could be an optional setup?

BTW my current workaround is to comment/uncomment the config whenever I change projects 😅 .

[cd-a]

@aotarola would you be so kind to share your config file? I was trying out this PR but couldn't get it to work with TypeScript and eslint.

I got basic formatting done on ts files, but not tsx. But not the eslint-prettier formatting.

[Philipp-M]

I've been trying to use two set of lsp's for the same language: Javascript.

Yeah Deno is kind of special, as they mostly use the same language. I'm not sure though, that this should be handled in the language-servers attribute. I would rather say, that they are kind of different languages (just happen to be almost the same).

I would suggest, we may need a more sophisticated language detection (also based on the roots as you suggest), and that this is handled as a different language, which would theoretically allow also different syntax highlighting. One way or the other, I think this should probably be done in a different PR (either follow-up, to keep this PR reviewable, or in case of different language detection, could also be independent of this PR).

I got basic formatting done on ts files, but not tsx. But not the eslint-prettier formatting.

You mean the example given in the PR description (or in the docs)? I suspect that you may have a missing table in the efm-langserver config for the right language-id of tsx, something like (I've added js and jsx as well):

[language-server.efm-lsp-prettier.config.languages]
typescript = [ { formatCommand ="prettier --stdin-filepath ${INPUT}", formatStdin = true } ]
typescriptreact = [ { formatCommand ="prettier --stdin-filepath ${INPUT}", formatStdin = true } ]
javascript = [ { formatCommand ="prettier --stdin-filepath ${INPUT}", formatStdin = true } ]
javascriptreact = [ { formatCommand ="prettier --stdin-filepath ${INPUT}", formatStdin = true } ]

[aotarola]

@dahmc here it is:

[language-server.typescript-lsp]
command = "typescript-language-server"
args = ["--stdio"]
config = { provideFormatter = false }

[language-server.efm-eslint]
command = "efm-langserver"

[language-server.efm-eslint.config]
documentFormatting = true
languages = { typescript = [
  { 
    lintCommand = "eslint_d -f unix --stdin --stdin-filename=${INPUT}", 
    lintIgnoreExitCode = true, 
    lintStdin = true, 
    formatCommand = 'eslint_d --stdin --fix-to-stdout --stdin-filename=${INPUT}', 
    formatStdin = true 
   },
] }

[[language]]
name = "typescript"
auto-format = true
file-types = ["js", "mjs", "cjs", "ts"]
language-servers = [
  { name = "efm-eslint", only-features = ["format", "diagnostics"] },
  { name = "typescript-lsp", except-features = ["format", "diagnostics"] },
]
roots = [".git"]

As you probably noticed, I'm using prettier tru eslint, same for tsc diagnostics. I hope this helps!

[aotarola]

I would suggest, we may need a more sophisticated language detection (also based on the roots as you suggest), and that this is handled as a different language, which would theoretically allow also different syntax highlighting. One way or the other, I think this should probably be done in a different PR (either follow-up, to keep this PR reviewable, or in case of different language detection, could also be independent of this PR).

Yep, agreed, a higher level of abstraction for this config that allows to have a different grammar sounds neat as well, and yeah it really is something that should be discussed/tackled after this PR is merged 👍🏼 .

[Philipp-M]

[[language]]
name = "typescript"
auto-format = true
file-types = ["js", "mjs", "cjs", "ts"]
language-servers = [
  { name = "efm-eslint", only-features = ["format", "diagnostics"] },
  { name = "typescript-lsp", except-features = ["format", "diagnostics"] },
]
roots = [".git"]

Just a quick tip, I would rather use the default language configurations for js, mjs, cjs and ts, and configure each of these languages to use the same language servers, eslint-lsp-prettier configured similar as (written in my last post):

[language-server.efm-eslint.config]
documentFormatting = true
[language-server.efm-eslint.config.languages]
typescript = [ {efm-eslint-lang-config} ]
typescriptreact = [ {efm-eslint-lang-config} ]
javascript = [ {efm-eslint-lang-config} ]
javascriptreact = [ {efm-eslint-lang-config} ]

and e.g. tsx:

[[language]]
name = "tsx"
language-servers = [
  { name = "efm-eslint", only-features = ["format", "diagnostics"] },
  { name = "typescript-lsp", except-features = ["format", "diagnostics"] }, # really disable diagnostics?
]

This way the correct language-ids are sent to the language server, which may make a difference for the lsp (specifically typescript-language-server) and the correct helix language config is used (different grammars for example).

You can also skip the typescript language server configuration entirely and just use typescript-language-server (formatting is disabled anyway)

[cd-a]

Thanks @aotarola for sharing. I've played around a bit with it, and it is starting to come together nicely. Formatting with prettier using eslint rules works great.

However, a few things I noticed:

• When a line has an error and the error dot is shown in the gutter, the virtual error text on the top right only shows up when the cursor is on the first column of the row. This doesn't seem right?
• Code actions don't work at all anymore. While it does show the correct eslint error now for pretty much everything, like missing dependencies in a React useEffect, there are no actions. Furthermore, actions that worked previously (like importing a missing dependency like React itself) also don't work anymore.

[Philipp-M]

• When a line has an error and the error dot is shown in the gutter, the virtual error text on the top right only shows up when the cursor is on the first column of the row. This doesn't seem right?

Hmm weird, I haven't had that issue yet with eslint, do you have any repro, does this happen for every diagnostic, which eslint version do you use?

what does e.g. eslint -f unix src/broken.ts (with the content of the file below) tell you? Btw. I've used https://github.com/igdev116/vite-react-ts-eslint-prettier for repro (since eslint requires some configuration)

E.g. for me this:

const some thing = 1;

Correctly tells me: Parsing error: ',' expected. [Error] for the space between some and thing

• Code actions don't work at all anymore. While it does show the correct eslint error now for pretty much everything, like missing dependencies in a React useEffect, there are no actions. Furthermore, actions that worked previously (like importing a missing dependency like React itself) also don't work anymore.

Indeed, typescript-language-server needs to get the diagnostic for retrieving the code actions. I have fixed this by catching all diagnostics even when the feature diagnostics is disabled, and only disable them for rendering and commands like goto_next_diag. There was also an issue, which has overwritten diagnostics from different language servers, which I have also fixed.

I suspect you used (disabled diagnostics for typescript-language-server)

language-servers = [
  { name = "efm-eslint", only-features = ["format", "diagnostics"] },
  { name = "typescript-language-server", except-features = ["format", "diagnostics"] }, # really disable diagnostics?
]

notice the comment, I wouldn't actually recommend disabling diagnostics for typescript-language-server, also to catch possible code-actions with the diagnostics.