Refactor doc language servers to a HashMap, and the config to use a Vec to retain order

helix-core/src/syntax.rs

@@ -112,11 +112,11 @@ pub struct LanguageConfiguration {
     // tags_config OnceCell<> https://github.com/tree-sitter/tree-sitter/pull/583
     #[serde(
         default,
-        skip_serializing_if = "HashMap::is_empty",
+        skip_serializing_if = "Vec::is_empty",
         serialize_with = "serialize_lang_features",
         deserialize_with = "deserialize_lang_features"
     )]
-    pub language_servers: HashMap<String, LanguageServerFeatures>,
+    pub language_servers: Vec<LanguageServerFeatures>,
     #[serde(skip_serializing_if = "Option::is_none")]
     pub indent: Option<IndentationConfiguration>,
 
@@ -282,19 +282,20 @@ enum LanguageServerFeatureConfiguration {
 
 #[derive(Debug, Default)]
 pub struct LanguageServerFeatures {
+    pub name: String,
     pub only: HashSet<LanguageServerFeature>,
     pub excluded: HashSet<LanguageServerFeature>,
 }
 
 impl LanguageServerFeatures {
     pub fn has_feature(&self, feature: LanguageServerFeature) -> bool {
-        self.only.is_empty() || self.only.contains(&feature) && !self.excluded.contains(&feature)
+        (self.only.is_empty() || self.only.contains(&feature)) && !self.excluded.contains(&feature)
     }
 }
 
 fn deserialize_lang_features<'de, D>(
     deserializer: D,
-) -> Result<HashMap<String, LanguageServerFeatures>, D::Error>
+) -> Result<Vec<LanguageServerFeatures>, D::Error>
 where
     D: serde::Deserializer<'de>,
 {
@@ -302,40 +303,39 @@ where
     let res = raw
         .into_iter()
         .map(|config| match config {
-            LanguageServerFeatureConfiguration::Simple(name) => {
-                (name, LanguageServerFeatures::default())
-            }
+            LanguageServerFeatureConfiguration::Simple(name) => LanguageServerFeatures {
+                name,
+                ..Default::default()
+            },
             LanguageServerFeatureConfiguration::Features {
                 only_features,
                 except_features,
                 name,
-            } => (
+            } => LanguageServerFeatures {
                 name,
-                LanguageServerFeatures {
-                    only: only_features,
-                    excluded: except_features,
-                },
-            ),
+                only: only_features,
+                excluded: except_features,
+            },
         })
         .collect();
     Ok(res)
 }
 fn serialize_lang_features<S>(
-    map: &HashMap<String, LanguageServerFeatures>,
+    map: &Vec<LanguageServerFeatures>,
     serializer: S,
 ) -> Result<S::Ok, S::Error>
 where
     S: serde::Serializer,
 {
     let mut serializer = serializer.serialize_seq(Some(map.len()))?;
-    for (name, features) in map {
+    for features in map {
         let features = if features.only.is_empty() && features.excluded.is_empty() {
-            LanguageServerFeatureConfiguration::Simple(name.to_owned())
+            LanguageServerFeatureConfiguration::Simple(features.name.to_owned())
         } else {
             LanguageServerFeatureConfiguration::Features {
                 only_features: features.only.clone(),
                 except_features: features.excluded.clone(),
-                name: name.to_owned(),
+                name: features.name.to_owned(),
             }
         };
         serializer.serialize_element(&features)?;

helix-lsp/src/lib.rs

@@ -12,7 +12,7 @@ pub use lsp_types as lsp;
 use futures_util::stream::select_all::SelectAll;
 use helix_core::{
     path,
-    syntax::{LanguageConfiguration, LanguageServerConfiguration},
+    syntax::{LanguageConfiguration, LanguageServerConfiguration, LanguageServerFeatures},
 };
 use tokio::sync::mpsc::UnboundedReceiver;
 
@@ -26,7 +26,7 @@ use thiserror::Error;
 use tokio_stream::wrappers::UnboundedReceiverStream;
 
 pub type Result<T> = core::result::Result<T, Error>;
-type LanguageServerName = String;
+pub type LanguageServerName = String;
 
 #[derive(Error, Debug)]
 pub enum Error {
@@ -689,9 +689,9 @@ impl Registry {
     ) -> Result<Vec<Arc<Client>>> {
         language_config
             .language_servers
-            .keys()
-            .filter_map(|name| {
-                #[allow(clippy::map_entry)]
+            .iter()
+            .filter_map(|LanguageServerFeatures { name, .. }| {
+                // #[allow(clippy::map_entry)]
                 if self.inner.contains_key(name) {
                     let client = match self.start_client(
                         name.clone(),
@@ -740,17 +740,20 @@ impl Registry {
         doc_path: Option<&std::path::PathBuf>,
         root_dirs: &[PathBuf],
         enable_snippets: bool,
-    ) -> Result<Vec<Arc<Client>>> {
+    ) -> Result<HashMap<LanguageServerName, Arc<Client>>> {
         language_config
             .language_servers
-            .keys()
-            .map(|name| {
+            .iter()
+            .map(|LanguageServerFeatures { name, .. }| {
                 if let Some(clients) = self.inner.get_mut(name) {
+                    // clients.find(
+
                     if let Some((_, client)) = clients.iter_mut().enumerate().find(|(i, client)| {
                         client.try_add_doc(&language_config.roots, root_dirs, doc_path, *i == 0)
                     }) {
-                        return Ok(client.clone());
+                        return Ok((name.to_owned(), client.clone()));
                     }
+                    // return Ok((name.clone(), clients.clone()));
                 }
                 let client = self.start_client(
                     name.clone(),
@@ -761,7 +764,7 @@ impl Registry {
                 )?;
                 let clients = self.inner.entry(name.clone()).or_default();
                 clients.push(client.clone());
-                Ok(client)
+                Ok((name.clone(), client))
             })
             .collect()
     }

helix-term/src/health.rs

@@ -194,10 +194,10 @@ pub fn languages_all() -> std::io::Result<()> {
 
         // TODO multiple language servers (check binary for each supported language server, not just the first)
 
-        let lsp = lang.language_servers.keys().next().and_then(|ls_name| {
+        let lsp = lang.language_servers.first().and_then(|ls| {
             syn_loader_conf
                 .language_server
-                .get(ls_name)
+                .get(&ls.name)
                 .map(|config| config.command.clone())
         });
         check_binary(lsp);
@@ -271,10 +271,10 @@ pub fn language(lang_str: String) -> std::io::Result<()> {
     // TODO multiple language servers
     probe_protocol(
         "language server",
-        lang.language_servers.keys().next().and_then(|ls_name| {
+        lang.language_servers.first().and_then(|ls| {
             syn_loader_conf
                 .language_server
-                .get(ls_name)
+                .get(&ls.name)
                 .map(|config| config.command.clone())
         }),
     )?;

helix-view/src/document.rs

@@ -180,7 +180,7 @@ pub struct Document {
     pub(crate) modified_since_accessed: bool,
 
     diagnostics: Vec<Diagnostic>,
-    language_servers: Vec<Arc<helix_lsp::Client>>,
+    pub(crate) language_servers: HashMap<LanguageServerName, Arc<Client>>,
 
     diff_handle: Option<DiffHandle>,
     version_control_head: Option<Arc<ArcSwap<Box<str>>>>,
@@ -580,7 +580,7 @@ where
     *mut_ref = f(mem::take(mut_ref));
 }
 
-use helix_lsp::{lsp, Client, OffsetEncoding};
+use helix_lsp::{lsp, Client, LanguageServerName, OffsetEncoding};
 use url::Url;
 
 impl Document {
@@ -616,7 +616,7 @@ impl Document {
             last_saved_time: SystemTime::now(),
             last_saved_revision: 0,
             modified_since_accessed: false,
-            language_servers: Vec::new(),
+            language_servers: HashMap::new(),
             diff_handle: None,
             config,
             version_control_head: None,
@@ -850,7 +850,7 @@ impl Document {
                 text: text.clone(),
             };
 
-            for language_server in language_servers {
+            for (_, language_server) in language_servers {
                 if !language_server.is_initialized() {
                     return Ok(event);
                 }
@@ -1006,11 +1006,6 @@ impl Document {
         Ok(())
     }
 
-    /// Set the LSP.
-    pub fn set_language_servers(&mut self, language_servers: Vec<Arc<helix_lsp::Client>>) {
-        self.language_servers = language_servers;
-    }
-
     /// Select text within the [`Document`].
     pub fn set_selection(&mut self, view_id: ViewId, selection: Selection) {
         // TODO: use a transaction?
@@ -1437,16 +1432,17 @@ impl Document {
     }
 
     pub fn language_servers(&self) -> impl Iterator<Item = &helix_lsp::Client> {
-        self.language_servers
-            .iter()
-            .filter_map(|l| if l.is_initialized() { Some(&**l) } else { None })
+        self.language_servers.values().filter_map(|l| {
+            if l.is_initialized() {
+                Some(&**l)
+            } else {
+                None
+            }
+        })
     }
 
     pub fn remove_language_server_by_name(&mut self, name: &str) -> Option<Arc<Client>> {
-        match self.language_servers.iter().position(|l| l.name() == name) {
-            Some(index) => Some(self.language_servers.remove(index)),
-            None => None,
-        }
+        self.language_servers.remove(name)
     }
 
     // TODO filter also based on LSP capabilities?
@@ -1454,12 +1450,15 @@ impl Document {
         &self,
         feature: LanguageServerFeature,
     ) -> impl Iterator<Item = &helix_lsp::Client> {
-        self.language_servers().filter(move |server| {
-            self.language_config()
-                .and_then(|config| config.language_servers.get(server.name()))
-                .map_or(false, |server_features| {
-                    server_features.has_feature(feature)
-                })
+        self.language_config().into_iter().flat_map(move |config| {
+            config.language_servers.iter().filter_map(move |features| {
+                let ls = &**self.language_servers.get(&features.name)?;
+                if ls.is_initialized() && features.has_feature(feature) {
+                    Some(ls)
+                } else {
+                    None
+                }
+            })
         })
     }
 
@@ -1610,7 +1609,10 @@ impl Document {
                 .find(|ls| ls.id() == d.language_server_id)
                 .and_then(|ls| {
                     let config = self.language_config()?;
-                    let features = config.language_servers.get(ls.name())?;
+                    let features = config
+                        .language_servers
+                        .iter()
+                        .find(|features| features.name == ls.name())?;
                     Some(features.has_feature(LanguageServerFeature::Diagnostics))
                 })
                 == Some(true)

helix-view/src/editor.rs

@@ -689,7 +689,7 @@ pub struct WhitespaceCharacters {
 impl Default for WhitespaceCharacters {
     fn default() -> Self {
         Self {
-            space: '·',   // U+00B7
+            space: '·',    // U+00B7
             nbsp: '⍽',    // U+237D
             tab: '→',     // U+2192
             newline: '⏎', // U+23CE
@@ -1103,9 +1103,9 @@ impl Editor {
         if !self.config().lsp.enable {
             return None;
         }
-
         // if doc doesn't have a URL it's a scratch buffer, ignore it
-        let doc = self.document(doc_id)?;
+        let doc = self.documents.get_mut(&doc_id)?;
+        let doc_url = doc.url()?;
         let (lang, path) = (doc.language.clone(), doc.path().cloned());
         let config = doc.config.load();
         let root_dirs = &config.workspace_lsp_roots;
@@ -1124,37 +1124,37 @@ impl Editor {
                 .ok()
         });
 
-        let doc = self.document_mut(doc_id)?;
-        let doc_url = doc.url()?;
-
         if let Some(language_servers) = language_servers {
-            // only spawn new lang servers if the servers aren't the same
-            // TODO simplify?
-            let doc_language_servers = doc.language_servers().collect::<Vec<_>>();
-            let spawn_new_servers = language_servers.len() != doc_language_servers.len()
-                || language_servers
-                    .iter()
-                    .zip(doc_language_servers.iter())
-                    .any(|(l, dl)| l.id() != dl.id());
-            if spawn_new_servers {
-                for doc_language_server in doc_language_servers {
-                    tokio::spawn(doc_language_server.text_document_did_close(doc.identifier()));
-                }
+            let language_id = doc.language_id().map(ToOwned::to_owned).unwrap_or_default();
 
-                let language_id = doc.language_id().map(ToOwned::to_owned).unwrap_or_default();
+            // only spawn new language servers if the servers aren't the same
 
-                for language_server in &language_servers {
-                    // TODO: this now races with on_init code if the init happens too quickly
-                    tokio::spawn(language_server.text_document_did_open(
-                        doc_url.clone(),
-                        doc.version(),
-                        doc.text(),
-                        language_id.clone(),
-                    ));
-                }
+            let doc_language_servers_not_in_registry =
+                doc.language_servers.iter().filter(|(name, doc_ls)| {
+                    !language_servers.contains_key(*name)
+                        || language_servers[*name].id() != doc_ls.id()
+                });
 
-                doc.set_language_servers(language_servers);
+            for (_, language_server) in doc_language_servers_not_in_registry {
+                tokio::spawn(language_server.text_document_did_close(doc.identifier()));
             }
+
+            let language_servers_not_in_doc = language_servers.iter().filter(|(name, ls)| {
+                !doc.language_servers.contains_key(*name)
+                    || doc.language_servers[*name].id() != ls.id()
+            });
+
+            for (_, language_server) in language_servers_not_in_doc {
+                // TODO: this now races with on_init code if the init happens too quickly
+                tokio::spawn(language_server.text_document_did_open(
+                    doc_url.clone(),
+                    doc.version(),
+                    doc.text(),
+                    language_id.clone(),
+                ));
+            }
+
+            doc.language_servers = language_servers;
         }
         Some(())
     }