Use proper rules for merging language server settings (#61546)

Closes https://github.com/zed-industries/zed/issues/61524

https://github.com/zed-industries/zed/issues/60763 was a bogus issue:
the semantic of root `language_settings` is the **default settings for
all languages**, not a "magic string that gets applied when an `!x`
exclusion is mentioned there".

Thus, reverts https://github.com/zed-industries/zed/pull/60984 and
redoes https://github.com/zed-industries/zed/pull/60000 in a correct
way: more custom settings replace less custom ones, same applies to the
language server settings.
Instead of doing some odd `!x` manipulations, the right fix is to remove
the old state as

```diff
 impl merge_from::MergeFrom for AllLanguageSettingsContent {
     fn merge_from(&mut self, other: &Self) {
         ...
         self.defaults.merge_from(&other.defaults);
         for language_settings in self.languages.0.values_mut() {
+            let language_servers = language_settings.language_servers.take();
             language_settings.merge_from(&other.defaults);
+            language_settings.language_servers = language_servers;
         }

         for (language_name, user_language_settings) in &other.languages.0 {
             if let Some(existing) = self.languages.0.get_mut(language_name) {
                 existing.merge_from(&user_language_settings);
             } else {
                 let mut new_settings = self.defaults.clone();
+                new_settings.language_servers = None;
                 new_settings.merge_from(&user_language_settings);
```

After the final override is determined, `...` is expanded and `!x`
entries are removed from the final list.

Project settings override user settings, so it's possible that a certain
language server is disabled by the user and re-enabled in the project:
worktree trust mechanism is supposed to mitigate the security issues.

More tests were added to cover all the cases.

Release Notes:

- Fixed project settings not re-enabling language servers

crates/language/src/language_settings.rs

@@ -940,6 +940,7 @@ pub struct JsxTagAutoCloseSettings {
 mod tests {
     use super::*;
     use gpui::TestAppContext;
+    use settings::{LocalSettingsKind, LocalSettingsPath, WorktreeId};
     use util::rel_path::rel_path;
 
     #[gpui::test]
@@ -1159,4 +1160,353 @@ mod tests {
             ])
         );
     }
+
+    #[gpui::test]
+    fn test_language_servers_across_settings_files(cx: &mut TestAppContext) {
+        cx.update(|cx| {
+            let mut store = SettingsStore::new(cx, &settings::default_settings());
+            store.register_setting::<AllLanguageSettings>();
+
+            let worktree_id = WorktreeId::from_usize(1);
+            let root = LocalSettingsPath::InWorktree(rel_path("root").into());
+            let subdir = LocalSettingsPath::InWorktree(rel_path("root/subdir").into());
+            let root_location = Some(SettingsLocation {
+                worktree_id,
+                path: rel_path("root/a.ts"),
+            });
+            let subdir_location = Some(SettingsLocation {
+                worktree_id,
+                path: rel_path("root/subdir/b.ts"),
+            });
+
+            assert_eq!(
+                resolved_language_servers(&store, None, "TypeScript"),
+                servers(&["vtsls", "eslint"]),
+                "default settings should disable typescript-language-server for TypeScript"
+            );
+            assert_eq!(
+                resolved_language_servers(&store, None, "Rust"),
+                servers(&["typescript-language-server", "vtsls", "eslint"]),
+            );
+
+            store
+                .set_user_settings(r#"{"language_servers": ["!vtsls", "..."]}"#, cx)
+                .unwrap();
+            assert_eq!(
+                resolved_language_servers(&store, None, "TypeScript"),
+                servers(&["vtsls", "eslint"]),
+                "the per-language list should fully replace the user's global list"
+            );
+            assert_eq!(
+                resolved_language_servers(&store, None, "Rust"),
+                servers(&["typescript-language-server", "eslint"]),
+                "user's global disable should apply to languages without their own list"
+            );
+
+            store
+                .set_local_settings(
+                    worktree_id,
+                    root.clone(),
+                    LocalSettingsKind::Settings,
+                    Some(r#"{"language_servers": ["..."]}"#),
+                    cx,
+                )
+                .unwrap();
+            assert_eq!(
+                resolved_language_servers(&store, root_location, "Rust"),
+                servers(&["typescript-language-server", "vtsls", "eslint"]),
+                "project settings enabling all servers should undo the user's global disable (#61524)"
+            );
+            assert_eq!(
+                resolved_language_servers(&store, None, "Rust"),
+                servers(&["typescript-language-server", "eslint"]),
+                "user's global disable should still apply outside of the project"
+            );
+
+            store
+                .set_local_settings(
+                    worktree_id,
+                    root.clone(),
+                    LocalSettingsKind::Settings,
+                    Some(r#"{"language_servers": ["!eslint", "..."]}"#),
+                    cx,
+                )
+                .unwrap();
+            assert_eq!(
+                resolved_language_servers(&store, root_location, "Rust"),
+                servers(&["typescript-language-server", "vtsls"]),
+                "project's global list should replace the user's global list"
+            );
+            assert_eq!(
+                resolved_language_servers(&store, root_location, "TypeScript"),
+                servers(&["vtsls", "eslint"]),
+                "the per-language list should fully replace the project's global list"
+            );
+
+            store
+                .set_local_settings(
+                    worktree_id,
+                    subdir.clone(),
+                    LocalSettingsKind::Settings,
+                    Some(r#"{"language_servers": ["..."]}"#),
+                    cx,
+                )
+                .unwrap();
+            assert_eq!(
+                resolved_language_servers(&store, subdir_location, "Rust"),
+                servers(&["typescript-language-server", "vtsls", "eslint"]),
+                "nested project settings should replace the outer global list"
+            );
+            assert_eq!(
+                resolved_language_servers(&store, root_location, "Rust"),
+                servers(&["typescript-language-server", "vtsls"]),
+            );
+            store
+                .set_local_settings(worktree_id, subdir, LocalSettingsKind::Settings, None, cx)
+                .unwrap();
+
+            store
+                .set_local_settings(
+                    worktree_id,
+                    root.clone(),
+                    LocalSettingsKind::Settings,
+                    Some(
+                        r#"{"languages": {"TypeScript": {"language_servers": ["vtsls", "..."]}}}"#,
+                    ),
+                    cx,
+                )
+                .unwrap();
+            assert_eq!(
+                resolved_language_servers(&store, root_location, "TypeScript"),
+                servers(&["vtsls", "typescript-language-server", "eslint"]),
+                "project's per-language configuration should override the user's global disable"
+            );
+            assert_eq!(
+                resolved_language_servers(&store, root_location, "Rust"),
+                servers(&["typescript-language-server", "eslint"]),
+                "user's global disable should still apply to other languages"
+            );
+
+            store
+                .set_user_settings(
+                    r#"{
+                        "language_servers": ["!vtsls", "..."],
+                        "languages": {"TypeScript": {"language_servers": ["vtsls", "..."]}}
+                    }"#,
+                    cx,
+                )
+                .unwrap();
+            assert_eq!(
+                resolved_language_servers(&store, None, "TypeScript"),
+                servers(&["vtsls", "typescript-language-server", "eslint"]),
+                "per-language configuration should win over a global disable from the same file"
+            );
+            assert_eq!(
+                resolved_language_servers(&store, None, "Rust"),
+                servers(&["typescript-language-server", "eslint"]),
+            );
+
+            store
+                .set_local_settings(
+                    worktree_id,
+                    root.clone(),
+                    LocalSettingsKind::Settings,
+                    Some(r#"{"language_servers": ["!vtsls", "..."]}"#),
+                    cx,
+                )
+                .unwrap();
+            assert_eq!(
+                resolved_language_servers(&store, root_location, "TypeScript"),
+                servers(&["vtsls", "typescript-language-server", "eslint"]),
+                "user's per-language list should win over the project's global disable"
+            );
+            assert_eq!(
+                resolved_language_servers(&store, root_location, "Rust"),
+                servers(&["typescript-language-server", "eslint"]),
+                "project's global disable should still apply to languages without their own list"
+            );
+
+            store
+                .set_user_settings(
+                    r#"{"languages": {"JavaScript": {"language_servers": ["!vtsls", "..."]}}}"#,
+                    cx,
+                )
+                .unwrap();
+            store
+                .set_local_settings(
+                    worktree_id,
+                    root,
+                    LocalSettingsKind::Settings,
+                    Some(r#"{"languages": {"JavaScript": {"language_servers": ["..."]}}}"#),
+                    cx,
+                )
+                .unwrap();
+            assert_eq!(
+                resolved_language_servers(&store, root_location, "JavaScript"),
+                servers(&["typescript-language-server", "vtsls", "eslint"]),
+                "project's per-language list should re-enable a server disabled by the user (#61524)"
+            );
+            assert_eq!(
+                resolved_language_servers(&store, None, "JavaScript"),
+                servers(&["typescript-language-server", "eslint"]),
+                "user's per-language disable should still apply outside of the project"
+            );
+        });
+    }
+
+    #[gpui::test]
+    fn test_language_servers_combined_restrictions(cx: &mut TestAppContext) {
+        cx.update(|cx| {
+            let mut store = SettingsStore::new(cx, &settings::default_settings());
+            store.register_setting::<AllLanguageSettings>();
+
+            let worktree_id = WorktreeId::from_usize(1);
+            let root = LocalSettingsPath::InWorktree(rel_path("root").into());
+            let root_location = Some(SettingsLocation {
+                worktree_id,
+                path: rel_path("root/a.ts"),
+            });
+
+            store
+                .set_user_settings(r#"{"language_servers": ["!vtsls", "..."]}"#, cx)
+                .unwrap();
+            store
+                .set_local_settings(
+                    worktree_id,
+                    root,
+                    LocalSettingsKind::Settings,
+                    Some(r#"{"language_servers": ["!eslint", "..."]}"#),
+                    cx,
+                )
+                .unwrap();
+            assert_eq!(
+                resolved_language_servers(&store, root_location, "Rust"),
+                servers(&["typescript-language-server", "vtsls"]),
+                "global lists replace wholesale: disables from different files must not accumulate"
+            );
+            assert_eq!(
+                resolved_language_servers(&store, None, "Rust"),
+                servers(&["typescript-language-server", "eslint"]),
+                "user's own disable should still apply outside of the project"
+            );
+            assert_eq!(
+                resolved_language_servers(&store, root_location, "TypeScript"),
+                servers(&["vtsls", "eslint"]),
+                "a language with its own list should ignore global lists from every file"
+            );
+
+            store
+                .set_user_settings(r#"{"language_servers": ["...", "!vtsls"]}"#, cx)
+                .unwrap();
+            assert_eq!(
+                resolved_language_servers(&store, None, "Rust"),
+                servers(&["typescript-language-server", "eslint"]),
+                "the position of a disabled entry relative to '...' should not matter"
+            );
+
+            store
+                .set_user_settings(r#"{"language_servers": ["eslint", "..."]}"#, cx)
+                .unwrap();
+            assert_eq!(
+                resolved_language_servers(&store, None, "Rust"),
+                servers(&["eslint", "typescript-language-server", "vtsls"]),
+                "the position of '...' should determine the priority order of enabled servers"
+            );
+
+            store
+                .set_user_settings(r#"{"language_servers": ["eslint"]}"#, cx)
+                .unwrap();
+            assert_eq!(
+                resolved_language_servers(&store, None, "Rust"),
+                servers(&["eslint"]),
+                "a global list without '...' should be exhaustive for languages without their own list"
+            );
+            assert_eq!(
+                resolved_language_servers(&store, None, "TypeScript"),
+                servers(&["vtsls", "eslint"]),
+                "a global list, exhaustive or not, never applies to languages with their own list"
+            );
+
+            store
+                .set_user_settings(
+                    r#"{
+                        "language_servers": ["!eslint", "..."],
+                        "languages": {"TypeScript": {"language_servers": ["vtsls", "..."]}}
+                    }"#,
+                    cx,
+                )
+                .unwrap();
+            assert_eq!(
+                resolved_language_servers(&store, None, "TypeScript"),
+                servers(&["vtsls", "typescript-language-server", "eslint"]),
+                "authoring a per-language list opts the language out of the global list entirely"
+            );
+            assert_eq!(
+                resolved_language_servers(&store, None, "Rust"),
+                servers(&["typescript-language-server", "vtsls"]),
+            );
+
+            // Issue #60763: to disable a server that the shipped list explicitly
+            // enables, the spelling is per-language, restating the shipped
+            // exclusions.
+            store
+                .set_user_settings(
+                    r#"{"languages": {"TypeScript": {"language_servers": ["!typescript-language-server", "!vtsls", "..."]}}}"#,
+                    cx,
+                )
+                .unwrap();
+            assert_eq!(
+                resolved_language_servers(&store, None, "TypeScript"),
+                servers(&["eslint"]),
+                "a per-language disable should win over the shipped explicit enable"
+            );
+            store
+                .set_user_settings(
+                    r#"{"languages": {"TypeScript": {"language_servers": ["!vtsls", "..."]}}}"#,
+                    cx,
+                )
+                .unwrap();
+            assert_eq!(
+                resolved_language_servers(&store, None, "TypeScript"),
+                servers(&["typescript-language-server", "eslint"]),
+                "a per-language list not restating the shipped exclusions should lift them"
+            );
+
+            store
+                .set_user_settings(
+                    r#"{"languages": {"TypeScript": {"language_servers": ["eslint"]}}}"#,
+                    cx,
+                )
+                .unwrap();
+            assert_eq!(
+                resolved_language_servers(&store, None, "TypeScript"),
+                servers(&["eslint"]),
+                "an exhaustive per-language list should pin exactly the listed servers"
+            );
+        });
+    }
+
+    fn servers(names: &[&str]) -> Vec<LanguageServerName> {
+        names
+            .iter()
+            .map(|name| LanguageServerName(name.to_string().into()))
+            .collect::<Vec<_>>()
+    }
+
+    fn resolved_language_servers(
+        store: &SettingsStore,
+        location: Option<SettingsLocation>,
+        language: &str,
+    ) -> Vec<LanguageServerName> {
+        let all_settings = store.get::<AllLanguageSettings>(location);
+        let language_settings = all_settings
+            .languages
+            .get(&LanguageName::new(language))
+            .unwrap_or(&all_settings.defaults);
+        language_settings.customized_language_servers(&servers(&[
+            "typescript-language-server",
+            "vtsls",
+            "eslint",
+        ]))
+    }
 }

crates/settings_content/src/language.rs

@@ -55,36 +55,10 @@ impl merge_from::MergeFrom for AllLanguageSettingsContent {
         // A user's global settings override the default global settings and
         // all default language-specific settings.
         self.defaults.merge_from(&other.defaults);
-        let globally_disabled_servers = other.defaults.language_servers.as_ref().map(|servers| {
-            servers
-                .iter()
-                .filter(|entry| entry.disabled)
-                .cloned()
-                .collect::<Vec<_>>()
-        });
         for language_settings in self.languages.0.values_mut() {
-            let language_server_overrides = language_settings.language_servers.clone();
+            let language_servers = language_settings.language_servers.take();
             language_settings.merge_from(&other.defaults);
-            if let Some(mut language_server_overrides) = language_server_overrides {
-                if let Some(disabled) = &globally_disabled_servers {
-                    for disabled_server in disabled {
-                        language_server_overrides
-                            .retain(|entry| entry.disabled || entry.name != disabled_server.name);
-                    }
-
-                    let insert_before = language_server_overrides
-                        .iter()
-                        .position(|entry| entry.name.as_ref() == REST_OF_LANGUAGE_SERVERS)
-                        .unwrap_or(language_server_overrides.len());
-                    for disabled_server in disabled {
-                        if !language_server_overrides.contains(disabled_server) {
-                            language_server_overrides
-                                .insert(insert_before, disabled_server.clone());
-                        }
-                    }
-                }
-                language_settings.language_servers = Some(language_server_overrides);
-            }
+            language_settings.language_servers = language_servers;
         }
 
         // A user's language-specific settings override default language-specific settings.
@@ -93,6 +67,7 @@ impl merge_from::MergeFrom for AllLanguageSettingsContent {
                 existing.merge_from(&user_language_settings);
             } else {
                 let mut new_settings = self.defaults.clone();
+                new_settings.language_servers = None;
                 new_settings.merge_from(&user_language_settings);
 
                 self.languages.0.insert(language_name.clone(), new_settings);
@@ -1364,7 +1339,12 @@ mod test {
     }
 
     #[test]
-    fn test_language_servers_merge_preserves_per_language_config() {
+    fn test_language_servers_merge_keeps_per_language_lists_pure() {
+        let default_typescript_servers = vec![
+            ConfiguredLanguageServer::new_disabled("typescript-language-server"),
+            ConfiguredLanguageServer::new("vtsls"),
+            ConfiguredLanguageServer::new(REST_OF_LANGUAGE_SERVERS),
+        ];
         let mut base = AllLanguageSettingsContent {
             defaults: LanguageSettingsContent {
                 language_servers: Some(vec![REST_OF_LANGUAGE_SERVERS.into()]),
@@ -1374,76 +1354,7 @@ mod test {
                 [(
                     "TypeScript".into(),
                     LanguageSettingsContent {
-                        language_servers: Some(vec![
-                            "!typescript-language-server".into(),
-                            "vtsls".into(),
-                            REST_OF_LANGUAGE_SERVERS.into(),
-                        ]),
-                        ..LanguageSettingsContent::default()
-                    },
-                )]
-                .into_iter()
-                .collect(),
-            ),
-            ..AllLanguageSettingsContent::default()
-        };
-
-        let user = AllLanguageSettingsContent {
-            defaults: LanguageSettingsContent {
-                language_servers: Some(vec![
-                    "!tailwindcss-language-server".into(),
-                    "!eslint".into(),
-                    REST_OF_LANGUAGE_SERVERS.into(),
-                ]),
-                ..LanguageSettingsContent::default()
-            },
-            ..AllLanguageSettingsContent::default()
-        };
-
-        base.merge_from(&user);
-
-        let ts_servers = base.languages.0["TypeScript"]
-            .language_servers
-            .as_ref()
-            .unwrap();
-        assert_eq!(
-            ts_servers,
-            &vec![
-                ConfiguredLanguageServer::new_disabled("typescript-language-server"),
-                ConfiguredLanguageServer::new("vtsls"),
-                ConfiguredLanguageServer::new_disabled("eslint"),
-                ConfiguredLanguageServer::new_disabled("tailwindcss-language-server"),
-                ConfiguredLanguageServer::new(REST_OF_LANGUAGE_SERVERS),
-            ]
-        );
-
-        let default_servers = base.defaults.language_servers.as_ref().unwrap();
-        assert_eq!(
-            default_servers,
-            &vec![
-                ConfiguredLanguageServer::new_disabled("tailwindcss-language-server"),
-                ConfiguredLanguageServer::new_disabled("eslint"),
-                ConfiguredLanguageServer::new(REST_OF_LANGUAGE_SERVERS),
-            ]
-        );
-    }
-
-    #[test]
-    fn test_global_language_server_disable_overrides_per_language_enable() {
-        let mut base = AllLanguageSettingsContent {
-            defaults: LanguageSettingsContent {
-                language_servers: Some(vec![REST_OF_LANGUAGE_SERVERS.into()]),
-                ..LanguageSettingsContent::default()
-            },
-            languages: LanguageToSettingsMap(
-                [(
-                    "TypeScript".into(),
-                    LanguageSettingsContent {
-                        language_servers: Some(vec![
-                            "!typescript-language-server".into(),
-                            "vtsls".into(),
-                            REST_OF_LANGUAGE_SERVERS.into(),
-                        ]),
+                        language_servers: Some(default_typescript_servers.clone()),
                         ..LanguageSettingsContent::default()
                     },
                 )]
@@ -1460,25 +1371,50 @@ mod test {
             },
             ..AllLanguageSettingsContent::default()
         };
-
         base.merge_from(&user);
-
-        let expected = vec![
-            ConfiguredLanguageServer::new_disabled("typescript-language-server"),
-            ConfiguredLanguageServer::new_disabled("vtsls"),
-            ConfiguredLanguageServer::new(REST_OF_LANGUAGE_SERVERS),
-        ];
         assert_eq!(
-            base.languages
-                .0
-                .get("TypeScript")
-                .and_then(|settings| settings.language_servers.as_deref()),
-            Some(expected.as_slice()),
+            base.languages.0["TypeScript"].language_servers.as_ref(),
+            Some(&default_typescript_servers),
+            "a global list must not rewrite per-language lists"
+        );
+        assert_eq!(
+            base.defaults.language_servers.as_ref(),
+            Some(&vec![
+                ConfiguredLanguageServer::new_disabled("vtsls"),
+                ConfiguredLanguageServer::new(REST_OF_LANGUAGE_SERVERS),
+            ])
+        );
+
+        let project = AllLanguageSettingsContent {
+            languages: LanguageToSettingsMap(
+                [(
+                    "TypeScript".into(),
+                    LanguageSettingsContent {
+                        language_servers: Some(vec![
+                            "vtsls".into(),
+                            REST_OF_LANGUAGE_SERVERS.into(),
+                        ]),
+                        ..LanguageSettingsContent::default()
+                    },
+                )]
+                .into_iter()
+                .collect(),
+            ),
+            ..AllLanguageSettingsContent::default()
+        };
+        base.merge_from(&project);
+        assert_eq!(
+            base.languages.0["TypeScript"].language_servers.as_ref(),
+            Some(&vec![
+                ConfiguredLanguageServer::new("vtsls"),
+                ConfiguredLanguageServer::new(REST_OF_LANGUAGE_SERVERS),
+            ]),
+            "a per-language list must replace the older one wholesale"
         );
     }
 
     #[test]
-    fn test_language_servers_merge_no_per_language_config_uses_global() {
+    fn test_language_servers_merge_no_per_language_config_stays_unset() {
         let mut base = AllLanguageSettingsContent {
             defaults: LanguageSettingsContent {
                 language_servers: Some(vec![REST_OF_LANGUAGE_SERVERS.into()]),
@@ -1508,13 +1444,16 @@ mod test {
 
         base.merge_from(&user);
 
-        let rust_servers = base.languages.0["Rust"].language_servers.as_ref().unwrap();
         assert_eq!(
-            rust_servers,
-            &vec![
+            base.languages.0["Rust"].language_servers, None,
+            "languages without their own list must not get a stale copy of the global one"
+        );
+        assert_eq!(
+            base.defaults.language_servers.as_ref(),
+            Some(&vec![
                 ConfiguredLanguageServer::new_disabled("eslint"),
                 ConfiguredLanguageServer::new(REST_OF_LANGUAGE_SERVERS),
-            ]
+            ])
         );
     }