Improve elm-language-server configuration (#7342)

Hi folks! @absynce and I paired a bit to improve the
`elm-language-server` configuration. We have realised that sometimes
`elm-language-server` settings were being reset to default. We had been
configuring `elm-language-server` like this:

```json
"lsp": {
  "elm-language-server": {
    "initialization_options": {
      "disableElmLSDiagnostics": true,
      "onlyUpdateDiagnosticsOnSave": true,
      "elmReviewDiagnostics": "warning"
    }
  }
}
```

And then we noticed that the following communication happened:

```
// Send:
{"jsonrpc":"2.0","method":"workspace/didChangeConfiguration","params":{"settings":{}}}
// Receive:
{"jsonrpc":"2.0","id":5,"method":"workspace/configuration","params":{"items":[{"section":"elmLS"}]}}
// Send:
{"jsonrpc":"2.0","id":5,"result":[null],"error":null}
```

In `elm-language-server` the settings from `didChangeConfiguration`
[replace the initial
settings](edd6813388/src/common/providers/diagnostics/diagnosticsProvider.ts (L188)).
Setting the value to `{}` effectively resets the configuration options
to defaults.

In Zed, `initialization_options` and `workspace_configuration` are two
different things, but in `elm-language-server` they are coupled.
Additionally, `elm-language-server` is requesting workspace
configuration for the `elmLS` section that doesn't exist.

This PR: 

1. Fixes settings reset on `didChangeConfiguration` by populating
`workspace_configuration` from `initialization_options`
2. Makes workspace configuration requests work by inserting an extra
copy of the settings under the `elmLS` key in `workspace_configuration`
— this is a bit ugly, but we're not sure how to make both kinds of
configuration messages work in the current setup.

This is how communication looks like after the proposed changes:

```
// Send:
{
  "jsonrpc": "2.0",
  "method": "workspace/didChangeConfiguration",
  "params": {
    "settings": {
      "disableElmLSDiagnostics": true,
      "onlyUpdateDiagnosticsOnSave": true,
      "elmReviewDiagnostics": "warning",
      "elmLS": {
        "disableElmLSDiagnostics": true,
        "onlyUpdateDiagnosticsOnSave": true,
        "elmReviewDiagnostics": "warning"
      }
    }
  }
}
// Receive:
{
  "jsonrpc": "2.0",
  "id": 4,
  "method": "workspace/configuration",
  "params": {
    "items": [
      {
        "section": "elmLS"
      }
    ]
  }
}
// Send:
{
  "jsonrpc": "2.0",
  "id": 4,
  "result": [
    {
      "disableElmLSDiagnostics": true,
      "onlyUpdateDiagnosticsOnSave": true,
      "elmReviewDiagnostics": "warning"
    }
  ],
  "error": null
}
```

Things we have considered:

1. Extracting the `elm-language-server` settings into a separate
section: we haven't found this being widely used in Zed, seems that all
language server configuration should fall under the top level `lsp`
section
2. Changing the way `elm-language-server` configuration works:
`elm-language-server` has got integrations with multiple editors,
changing the configuration behaviour would mean updating all the
existing integrations. Plus we are not exactly sure if it's doing
anything wrong.

Release Notes:

- Improved elm-language-server configuration options

Co-authored-by: Jared M. Smith <absynce@gmail.com>
This commit is contained in:
Andrey Kuzmin 2024-02-04 00:57:24 +01:00 committed by GitHub
parent ae2c23bd8e
commit ac74a72a9e
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
2 changed files with 48 additions and 1 deletions

View File

@ -1,9 +1,13 @@
use anyhow::{anyhow, Result};
use async_trait::async_trait;
use futures::StreamExt;
use gpui::AppContext;
use language::{LanguageServerName, LspAdapter, LspAdapterDelegate};
use lsp::LanguageServerBinary;
use node_runtime::NodeRuntime;
use project::project_settings::ProjectSettings;
use serde_json::Value;
use settings::Settings;
use smol::fs;
use std::{
any::Any,
@ -13,6 +17,7 @@ use std::{
};
use util::ResultExt;
const SERVER_NAME: &'static str = "elm-language-server";
const SERVER_PATH: &'static str = "node_modules/@elm-tooling/elm-language-server/out/node/index.js";
fn server_binary_arguments(server_path: &Path) -> Vec<OsString> {
@ -32,7 +37,7 @@ impl ElmLspAdapter {
#[async_trait]
impl LspAdapter for ElmLspAdapter {
fn name(&self) -> LanguageServerName {
LanguageServerName("elm-language-server".into())
LanguageServerName(SERVER_NAME.into())
}
fn short_name(&self) -> &'static str {
@ -88,6 +93,27 @@ impl LspAdapter for ElmLspAdapter {
) -> Option<LanguageServerBinary> {
get_cached_server_binary(container_dir, &*self.node).await
}
fn workspace_configuration(&self, _workspace_root: &Path, cx: &mut AppContext) -> Value {
// elm-language-server expects workspace didChangeConfiguration notification
// params to be the same as lsp initialization_options
let override_options = ProjectSettings::get_global(cx)
.lsp
.get(SERVER_NAME)
.and_then(|s| s.initialization_options.clone())
.unwrap_or_default();
match override_options.clone().as_object_mut() {
Some(op) => {
// elm-language-server requests workspace configuration
// for the `elmLS` section, so we have to nest
// another copy of initialization_options there
op.insert("elmLS".into(), override_options);
serde_json::to_value(op).unwrap_or_default()
}
None => override_options,
}
}
}
async fn get_cached_server_binary(

View File

@ -2,3 +2,24 @@
- Tree Sitter: [tree-sitter-elm](https://github.com/elm-tooling/tree-sitter-elm)
- Language Server: [elm-language-server](https://github.com/elm-tooling/elm-language-server)
### Setting up `elm-language-server`
Elm language server can be configured in your `settings.json`, e.g.:
```json
{
"lsp": {
"elm-language-server": {
"initialization_options": {
"disableElmLSDiagnostics": true,
"onlyUpdateDiagnosticsOnSave": false,
"elmReviewDiagnostics": "warning"
}
}
}
}
```
`elm-format`, `elm-review` and `elm` need to be installed and made available in the environment
or configured in the settings. See the [full list of server settings here](https://github.com/elm-tooling/elm-language-server?tab=readme-ov-file#server-settings).