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

Update default implementation of LspAdapter::check_if_user_installed(...) with generic functionality that can be overridden if needed #10397

Conversation

dljsjr
Copy link

@dljsjr dljsjr commented Apr 11, 2024

The previous default implementation for the trait method was a no-op (it always returned None), but the concrete implementations for the few languages that supported it were more or less copy/paste boilerplate.

To avoid a circular dependency between the language and project crates, this PR introduces the LspBinaryConfigProvider trait that allows for a DI-like approach to getting the user's LSP override configs out of the LSP Adapter Delegate.

Through this approach, we can make the default implementation of the trait method apply to all LSP Adapters by using the LspAdapter::name(...) method to provide the key in to the config map.

LSP Adapter implementations still have the ability to implement the trait method explicitly in the event that the generalized logic is not sufficient for detecting and bootstrapping the LSP binary. An example could be cases where the PATH fallback still needs specific CLI flags/options, as the default logic passes no arguments. The gopls adapter is one such case where the concrete implementation is more-or-less the same but needs flags for the CLI.

The small number of existing explicit implementations have not been changed. This should make it easier to use local installs of LSPs for users with constrained connectivity without needing to wait for all of the various language adapters already in place to support overrides explicitly.

I think in practice the primary entry point here is going to be using the config file overrides; I feel like the automagic PATH overrides could lead to a lot of unexpected behavior for users, especially when CLI args are involved, and so it might be prudent to have the generalized version only provide an override from the config file. Adapter implementations can then choose to implement the PATH fallback logic if they want. Another option would be adding another trait method to LspAdapter such as default_args() akin to name() that would be used with the PATH fallback.

Probably worth discussing, and I can revise the PR easily.

Related Issues:

Release Notes:

  • Added a default/generic implementation for LspAdapter::check_if_user_installed(...) to enable detecting local LSP binaries for all LspAdapter implementations

The previous default implementation for the trait method was a no-op
(it always returned `None`), but the concrete implementations for the
few languages that supported it were more or less copy/paste boilerplate.

To avoid a circular dependency between the `language` and `project` crates,
this PR introduces the `LspBinaryConfigProvider` trait that allows for a DI-like
approach to getting the user's LSP override configs out of the LSP Adapter
Delegate.

Through this approach, we can make the default implementation of the trait method
apply to all LSP Adapters by using the `LspAdapter::name(...)` method to provide
the key in to the config map.

LSP Adapter implementations still have the ability to implement the trait method
explicitly in the event that the generalized logic is not sufficient for detecting
and bootstrapping the LSP binary. An example could be cases where the `PATH` fallback
still needs specific CLI flags/options, as the default logic passes no arguments, like
the `gopls` adapter.

The small number of existing explicit implementations have not been changed. This should
make it easier to use local installs of LSPs for users with constrained connectivity without
needing to wait for all of the various language adapters already in place to support
overrides explicitly.
@cla-bot cla-bot bot added the cla-signed The user has signed the Contributor License Agreement label Apr 11, 2024
@mrnugget mrnugget self-assigned this Apr 11, 2024
@maxdeviant
Copy link
Member

I don’t think this is entirely necessary, as most/all of our language servers are being pulled out into extensions, which don’t use this method on the LspAdapter.

@maxdeviant maxdeviant self-assigned this Apr 11, 2024
@mrnugget
Copy link
Member

Yeah, agree with @maxdeviant. I think the language server adapters that stay in Zed and for which it makes so have these options aren't more than a handful and for those I think it's fine to add these lines by hand, but maybe that's also my Go showing :)

@dljsjr
Copy link
Author

dljsjr commented Apr 11, 2024

@maxdeviant @mrnugget Makes sense!

The context here is that I'm currently being forced to use my phone hotspot internet for the week. I needed to work on a Lua script, and I was hitting the GitHub 403 rate limit because of the hotspot situation.

When I looked in to how to override the download process, I saw that it needed to be implemented per-language, and I figured, rule of three, lemme see if I can figure out how to do this generally. Also I've been kept away from Rust at <day job> for too long and needed to stretch my wings.

TL;DR I was nerd sniped. If this mechanism is becoming legacy, then I think it makes sense to close this PR without merging. But FWIW, I do also think that a methodology for configuring the usage of local LSP binaries is still prudent, even for the upcoming extensions.

@maxdeviant
Copy link
Member

@maxdeviant @mrnugget Makes sense!

The context here is that I'm currently being forced to use my phone hotspot internet for the week. I needed to work on a Lua script, and I was hitting the GitHub 403 rate limit because of the hotspot situation.

When I looked in to how to override the download process, I saw that it needed to be implemented per-language, and I figured, rule of three, lemme see if I can figure out how to do this generally. Also I've been kept away from Rust at for too long and needed to stretch my wings.

TL;DR I was nerd sniped. If this mechanism is becoming legacy, then I think it makes sense to close this PR without merging. But FWIW, I do also think that a methodology for configuring the usage of local LSP binaries is still prudent, even for the upcoming extensions.

Ah yea, the Lua support in particular is going to move out to an extension within the next week.

@dljsjr dljsjr closed this Apr 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed The user has signed the Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants