Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for multiple language servers per language #2507

Merged
merged 41 commits into from
May 19, 2023

Conversation

Philipp-M
Copy link
Contributor

@Philipp-M Philipp-M commented May 19, 2022

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, as long as the language server supports these. If it doesn't the next language server which supports the feature is tried.

The list of supported features are:

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

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 the-mikedavis linked an issue May 19, 2022 that may be closed by this pull request
@the-mikedavis the-mikedavis added the S-needs-discussion Status: Needs discussion or design. label May 19, 2022
@the-mikedavis
Copy link
Member

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
Copy link
Contributor

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.

helix-core/src/syntax.rs Outdated Show resolved Hide resolved
@Philipp-M
Copy link
Contributor Author

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.

@Philipp-M Philipp-M force-pushed the multiple-language-servers branch from 5bb5e8c to e887912 Compare May 22, 2022 11:22
@the-mikedavis
Copy link
Member

There is #1015 and #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 Philipp-M force-pushed the multiple-language-servers branch from e887912 to d8bbfe7 Compare May 24, 2022 17:29
@Philipp-M
Copy link
Contributor Author

Philipp-M commented May 24, 2022

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: 5040679#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
Copy link
Contributor Author

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 Philipp-M force-pushed the multiple-language-servers branch from d866093 to 65961c9 Compare May 26, 2022 22:44
@Philipp-M
Copy link
Contributor Author

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 Philipp-M marked this pull request as ready for review May 26, 2022 22:55
@Philipp-M Philipp-M requested a review from archseer May 26, 2022 22:56
@the-mikedavis the-mikedavis added S-waiting-on-review Status: Awaiting review from a maintainer. and removed S-needs-discussion Status: Needs discussion or design. labels May 26, 2022
@Philipp-M Philipp-M force-pushed the multiple-language-servers branch from b8cbe3b to de72a59 Compare June 2, 2022 07:56
@Philipp-M Philipp-M force-pushed the multiple-language-servers branch from de72a59 to 7d3f736 Compare June 21, 2022 20:14
@Philipp-M Philipp-M force-pushed the multiple-language-servers branch from 7d3f736 to b685db7 Compare June 29, 2022 20:22
@Philipp-M Philipp-M force-pushed the multiple-language-servers branch from b685db7 to df7ff54 Compare July 7, 2022 11:21
@Philipp-M
Copy link
Contributor Author

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
Copy link
Member

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.

@sudormrfbin sudormrfbin added S-waiting-on-pr Status: This is waiting on another PR to be merged first and removed S-waiting-on-review Status: Awaiting review from a maintainer. labels Jul 16, 2022
@robrecord
Copy link

Confused now about how to configure language servers since the "config" parameter was removed. Are there docs for this?

@Philipp-M
Copy link
Contributor Author

https://docs.helix-editor.com/master/languages.html#language-server-configuration

@MarkStruik
Copy link

Is this in the current version? because i have just copied over the typescript version from the docs and that gives me an error:

Error parsing user language config: unknown field `language-servers`, expected one of `name`, `scope`, `file-types`, `shebangs`, `roots`, `comment-token`, `text-width`, `soft-wrap`, `config`, `auto-format`, `formatter`, `diagnostic-severity`, `grammar`, `injection-regex`, `language-server`, `indent`, `debugger`, `auto-pairs`, `rulers`, `workspace-lsp-roots`
in `language`

image

@archseer
Copy link
Member

You're using an older version of hx, it still mentions the language-server key.

@benjamineskola
Copy link

@MarkStruik If I'm not mistaken this feature is only in HEAD; the latest release was in March, before this was merged.

@MarkStruik
Copy link

Thanks for responding. So if I want that I need to build from master? ( ahh I now get the version number scheme 😁 ) or is this merge planned in a release that’s coming soon?

@dvic
Copy link
Contributor

dvic commented Jul 19, 2023

Thanks for responding. So if I want that I need to build from master? ( ahh I now get the version number scheme 😁 ) or is this merge planned in a release that’s coming soon?

If you're on mac you can run brew install helix --head

@JulianH99
Copy link

When will this be on release so that it appears on scoop as well?

@dead10ck
Copy link
Member

When will this be on release so that it appears on scoop as well?

We don't maintain the scoop package, so you'll have to ask whoever does.

@MarkStruik
Copy link

When will this be on release so that it appears on scoop as well?

We don't maintain the scoop package, so you'll have to ask whoever does.

Fair enough but I would imagine they do not update it until there is a new official release. Now if I look at past releases it might suggest end of August?

@the-mikedavis
Copy link
Member

Yep, we're working on a release that will include this and the other changes in master. We try to release every few months (see the faq) but we don't commit to firm timelines / ETAs.

maveonair added a commit to maveonair/solus-packages that referenced this pull request Oct 30, 2023
**Summary**

Breaking changes:

- Support multiple language servers per language [getsolus#2507](helix-editor/helix#2507)
  - This is a breaking change to language configuration

Full release notes:

- [23.10](https://github.com/helix-editor/helix/blob/master/CHANGELOG.md#2310-2023-10-24)
@leona
Copy link

leona commented Jan 25, 2024

I know this is closed, but maybe someone has an answer. Is it possible to append a language server, rather than replace the current ones? If wanted one for every language (copilot) for example, that would be a bit tedious. Being able to just add a language server rather than overwrite would be great.

@Philipp-M
Copy link
Contributor Author

Your proposal makes sense, and I think I've thought about something like that (pretty much the use-case you've presented).

You could open an issue with a design of the configuration you think makes sense (when there isn't yet an issue with that, haven't searched for that...).

@Philipp-M Philipp-M deleted the multiple-language-servers branch January 25, 2024 21:57
@pascalkuthe
Copy link
Member

#9318 will actually allow that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-helix-term Area: Helix term improvements A-language-server Area: Language server client S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow multiple language servers for a language Support for multiple language servers for one language?