migrate language server config to new config system

Cargo.lock

@@ -62,9 +62,9 @@ dependencies = [
 
 [[package]]
 name = "anyhow"
-version = "1.0.78"
+version = "1.0.79"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "ca87830a3e3fb156dc96cfbd31cb620265dd053be734723f22b760d6cc3c3051"
+checksum = "080e9890a082662b09c1ad45f567faeeb47f22b5fb23895fbe1e651e718e25ca"
 
 [[package]]
 name = "arc-swap"
@@ -1069,6 +1069,7 @@ name = "helix-core"
 version = "23.10.0"
 dependencies = [
  "ahash",
+ "anyhow",
  "arc-swap",
  "bitflags 2.4.1",
  "chrono",
@@ -1079,6 +1080,7 @@ dependencies = [
  "helix-config",
  "helix-loader",
  "imara-diff",
+ "indexmap",
  "indoc",
  "log",
  "nucleo",
@@ -1147,6 +1149,7 @@ dependencies = [
 name = "helix-lsp"
 version = "23.10.0"
 dependencies = [
+ "ahash",
  "anyhow",
  "futures-executor",
  "futures-util",
@@ -1155,6 +1158,7 @@ dependencies = [
  "helix-core",
  "helix-loader",
  "helix-parsec",
+ "indexmap",
  "log",
  "lsp-types",
  "parking_lot",
@@ -1358,12 +1362,13 @@ dependencies = [
 
 [[package]]
 name = "indexmap"
-version = "2.0.0"
+version = "2.1.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "d5477fe2230a79769d8dc68e0eabf5437907c0457a5614a9e8dddb67f65eb65d"
+checksum = "d530e1a18b1cb4c484e6e34556a0d948706958449fca0cab753d649f2bce3d1f"
 dependencies = [
  "equivalent",
  "hashbrown 0.14.3",
+ "serde",
 ]
 
 [[package]]

helix-lsp/Cargo.toml

@@ -14,6 +14,7 @@ homepage.workspace = true
 
 [dependencies]
 helix-core = { path = "../helix-core" }
+helix-config = { path = "../helix-config" }
 helix-loader = { path = "../helix-loader" }
 helix-parsec = { path = "../helix-parsec" }
 
@@ -30,3 +31,5 @@ tokio = { version = "1.35", features = ["rt", "rt-multi-thread", "io-util", "io-
 tokio-stream = "0.1.14"
 which = "5.0.0"
 parking_lot = "0.12.1"
+ahash = "0.8.6"
+indexmap = { version = "2.1.0", features = ["serde"] }

helix-lsp/src/client.rs

@@ -1,9 +1,12 @@
 use crate::{
+    config::LanguageServerConfig,
     find_lsp_workspace, jsonrpc,
     transport::{Payload, Transport},
     Call, Error, OffsetEncoding, Result,
 };
 
+use anyhow::Context;
+use helix_config::{self as config, OptionManager};
 use helix_core::{find_workspace, path, syntax::LanguageServerFeature, ChangeSet, Rope};
 use helix_loader::{self, VERSION_AND_GIT_HASH};
 use lsp::{
@@ -13,15 +16,14 @@ use lsp::{
 };
 use lsp_types as lsp;
 use parking_lot::Mutex;
-use serde::Deserialize;
 use serde_json::Value;
 use std::future::Future;
+use std::path::PathBuf;
 use std::process::Stdio;
 use std::sync::{
     atomic::{AtomicU64, Ordering},
     Arc,
 };
-use std::{collections::HashMap, path::PathBuf};
 use tokio::{
     io::{BufReader, BufWriter},
     process::{Child, Command},
@@ -50,13 +52,11 @@ pub struct Client {
     server_tx: UnboundedSender<Payload>,
     request_counter: AtomicU64,
     pub(crate) capabilities: OnceCell<lsp::ServerCapabilities>,
-    config: Option<Value>,
     root_path: std::path::PathBuf,
     root_uri: Option<lsp::Url>,
     workspace_folders: Mutex<Vec<lsp::WorkspaceFolder>>,
     initialize_notify: Arc<Notify>,
-    /// workspace folders added while the server is still initializing
+    config: Arc<OptionManager>,
-    req_timeout: u64,
 }
 
 impl Client {
@@ -170,23 +170,20 @@ impl Client {
 
     #[allow(clippy::type_complexity, clippy::too_many_arguments)]
     pub fn start(
-        cmd: &str,
+        config: Arc<OptionManager>,
-        args: &[String],
-        config: Option<Value>,
-        server_environment: HashMap<String, String>,
         root_markers: &[String],
         manual_roots: &[PathBuf],
         id: usize,
         name: String,
-        req_timeout: u64,
         doc_path: Option<&std::path::PathBuf>,
     ) -> Result<(Self, UnboundedReceiver<(usize, Call)>, Arc<Notify>)> {
         // Resolve path to the binary
-        let cmd = which::which(cmd).map_err(|err| anyhow::anyhow!(err))?;
+        let cmd = which::which(config.command().as_deref().context("no command defined")?)
+            .map_err(|err| anyhow::anyhow!(err))?;
 
         let process = Command::new(cmd)
-            .envs(server_environment)
+            .envs(config.enviorment().iter().map(|(k, v)| (&**k, &**v)))
-            .args(args)
+            .args(config.args().iter().map(|v| &**v))
             .stdin(Stdio::piped())
             .stdout(Stdio::piped())
             .stderr(Stdio::piped())
@@ -233,7 +230,6 @@ impl Client {
             request_counter: AtomicU64::new(0),
             capabilities: OnceCell::new(),
             config,
-            req_timeout,
             root_path,
             root_uri,
             workspace_folders: Mutex::new(workspace_folders),
@@ -374,8 +370,8 @@ impl Client {
             .unwrap_or_default()
     }
 
-    pub fn config(&self) -> Option<&Value> {
+    pub fn config(&self) -> config::Guard<Option<Box<Value>>> {
-        self.config.as_ref()
+        self.config.server_config()
     }
 
     pub async fn workspace_folders(
@@ -404,7 +400,7 @@ impl Client {
     where
         R::Params: serde::Serialize,
     {
-        self.call_with_timeout::<R>(params, self.req_timeout)
+        self.call_with_timeout::<R>(params, self.config.timeout())
     }
 
     fn call_with_timeout<R: lsp::request::Request>(
@@ -512,7 +508,7 @@ impl Client {
     // -------------------------------------------------------------------------------------------
 
     pub(crate) async fn initialize(&self, enable_snippets: bool) -> Result<lsp::InitializeResult> {
-        if let Some(config) = &self.config {
+        if let Some(config) = &*self.config() {
             log::info!("Using custom LSP config: {}", config);
         }
 
@@ -524,7 +520,7 @@ impl Client {
             // clients will prefer _uri if possible
             root_path: self.root_path.to_str().map(|path| path.to_owned()),
             root_uri: self.root_uri.clone(),
-            initialization_options: self.config.clone(),
+            initialization_options: self.config().as_deref().cloned(),
             capabilities: lsp::ClientCapabilities {
                 workspace: Some(lsp::WorkspaceClientCapabilities {
                     configuration: Some(true),
@@ -1152,17 +1148,12 @@ impl Client {
         };
 
         // merge FormattingOptions with 'config.format'
-        let config_format = self
+        let mut config_format = self.config.format();
-            .config
+        let options = if !config_format.is_empty() {
-            .as_ref()
-            .and_then(|cfg| cfg.get("format"))
-            .and_then(|fmt| HashMap::<String, lsp::FormattingProperty>::deserialize(fmt).ok());
-
-        let options = if let Some(mut properties) = config_format {
             // passed in options take precedence over 'config.format'
-            properties.extend(options.properties);
+            config_format.extend(options.properties);
             lsp::FormattingOptions {
-                properties,
+                properties: config_format,
                 ..options
             }
         } else {

helix-lsp/src/config.rs

@@ -0,0 +1,67 @@
+use std::collections::HashMap;
+
+use anyhow::bail;
+use helix_config::{options, List, Map, String, Ty, Value};
+
+use crate::lsp;
+
+// TODO: differentiating between Some(null) and None is not really practical
+// since the distinction is lost on a roundtrip trough config::Value.
+// Porbably better to change our code to treat null the way we currently
+// treat None
+options! {
+    struct LanguageServerConfig {
+        /// The name or path of the language server binary to execute. Binaries must be in `$PATH`
+        command: Option<String> = None,
+        /// A list of arguments to pass to the language server binary
+        #[read = deref]
+        args: List<String> = List::default(),
+        /// Any environment variables that will be used when starting the language server
+        enviorment: Map<String> = Map::default(),
+        /// LSP initialization options
+        #[name = "config"]
+        server_config: Option<Box<serde_json::Value>> = None,
+        /// LSP initialization options
+        #[read = copy]
+        timeout: u64 = 20,
+        // TODO: merge
+        /// LSP formatting options
+        #[name = "config.format"]
+        #[read = fold(HashMap::new(), fold_format_config, FormatConfig)]
+        format: Map<FormattingProperty> = Map::default()
+    }
+}
+
+type FormatConfig = HashMap<std::string::String, lsp::FormattingProperty>;
+
+fn fold_format_config(config: &Map<FormattingProperty>, mut res: FormatConfig) -> FormatConfig {
+    for (k, v) in config.iter() {
+        res.entry(k.to_string()).or_insert_with(|| v.0.clone());
+    }
+    res
+}
+
+// damm orphan rules :/
+#[derive(Debug, PartialEq, Clone)]
+struct FormattingProperty(lsp::FormattingProperty);
+
+impl Ty for FormattingProperty {
+    fn from_value(val: Value) -> anyhow::Result<Self> {
+        match val {
+            Value::Int(_) => Ok(FormattingProperty(lsp::FormattingProperty::Number(
+                i32::from_value(val)?,
+            ))),
+            Value::Bool(val) => Ok(FormattingProperty(lsp::FormattingProperty::Bool(val))),
+            Value::String(val) => Ok(FormattingProperty(lsp::FormattingProperty::String(val))),
+            _ => bail!("expected a string, boolean or integer"),
+        }
+    }
+
+    fn to_value(&self) -> Value {
+        match self.0 {
+            lsp::FormattingProperty::Bool(val) => Value::Bool(val),
+            lsp::FormattingProperty::Number(val) => Value::Int(val as _),
+            lsp::FormattingProperty::String(ref val) => Value::String(val.clone()),
+        }
+    }
+}

helix-lsp/src/lib.rs

@@ -1,4 +1,5 @@
 mod client;
+mod config;
 pub mod file_event;
 pub mod jsonrpc;
 pub mod snippet;
@@ -11,6 +12,7 @@ pub use lsp::{Position, Url};
 pub use lsp_types as lsp;
 
 use futures_util::stream::select_all::SelectAll;
+use helix_config::OptionRegistry;
 use helix_core::{
     path,
     syntax::{LanguageConfiguration, LanguageServerConfiguration, LanguageServerFeatures},
@@ -26,6 +28,8 @@ use std::{
 use thiserror::Error;
 use tokio_stream::wrappers::UnboundedReceiverStream;
 
+use crate::config::init_config;
+
 pub type Result<T> = core::result::Result<T, Error>;
 pub type LanguageServerName = String;
 
@@ -636,17 +640,25 @@ pub struct Registry {
     counter: usize,
     pub incoming: SelectAll<UnboundedReceiverStream<(usize, Call)>>,
     pub file_event_handler: file_event::Handler,
+    pub config: OptionRegistry,
 }
 
 impl Registry {
     pub fn new(syn_loader: Arc<helix_core::syntax::Loader>) -> Self {
-        Self {
+        let mut res = Self {
             inner: HashMap::new(),
             syn_loader,
             counter: 0,
             incoming: SelectAll::new(),
             file_event_handler: file_event::Handler::new(),
-        }
+            config: OptionRegistry::new(),
+        };
+        res.reset_config();
+        res
+    }
+
+    pub fn reset_config(&mut self) {
+        init_config(&mut self.config);
     }
 
     pub fn get_by_id(&self, id: usize) -> Option<&Client> {
@@ -882,15 +894,11 @@ fn start_client(
     enable_snippets: bool,
 ) -> Result<NewClient> {
     let (client, incoming, initialize_notify) = Client::start(
-        &ls_config.command,
+        todo!(),
-        &ls_config.args,
-        ls_config.config.clone(),
-        ls_config.environment.clone(),
         &config.roots,
         config.workspace_lsp_roots.as_deref().unwrap_or(root_dirs),
         id,
         name,
-        ls_config.timeout,
         doc_path,
     )?;
 

helix-term/src/application.rs

@@ -699,7 +699,7 @@ impl Application {
                         // Trigger a workspace/didChangeConfiguration notification after initialization.
                         // This might not be required by the spec but Neovim does this as well, so it's
                         // probably a good idea for compatibility.
-                        if let Some(config) = language_server.config() {
+                        if let Some(config) = language_server.config().as_deref() {
                             tokio::spawn(language_server.did_change_configuration(config.clone()));
                         }
 
@@ -1023,7 +1023,8 @@ impl Application {
                             .items
                             .iter()
                             .map(|item| {
-                                let mut config = language_server.config()?;
+                                let config = language_server.config();
+                                let mut config = config.as_deref()?;
                                 if let Some(section) = item.section.as_ref() {
                                     // for some reason some lsps send an empty string (observed in 'vscode-eslint-language-server')
                                     if !section.is_empty() {
@@ -1032,7 +1033,7 @@ impl Application {
                                         }
                                     }
                                 }
-                                Some(config)
+                                Some(config.to_owned())
                             })
                             .collect();
                         Ok(json!(result))