Centralize client options validation (astral-sh#18623)

Author: MichaReiser

crates/ruff_server/src/lib.rs

@@ -3,7 +3,7 @@
 pub use edit::{DocumentKey, NotebookDocument, PositionEncoding, TextDocument};
 use lsp_types::CodeActionKind;
 pub use server::Server;
-pub use session::{ClientSettings, DocumentQuery, DocumentSnapshot, Session};
+pub use session::{ClientOptions, DocumentQuery, DocumentSnapshot, GlobalOptions, Session};
 pub use workspace::{Workspace, Workspaces};
 
 #[macro_use]

crates/ruff_server/src/server.rs

@@ -30,7 +30,7 @@ use self::schedule::Scheduler;
 use self::schedule::Task;
 use self::schedule::event_loop_thread;
 use crate::PositionEncoding;
-use crate::session::AllSettings;
+use crate::session::AllOptions;
 use crate::session::Session;
 use crate::workspace::Workspaces;
 
@@ -77,39 +77,36 @@ impl Server {
             ..
         } = init_params;
 
-        let mut all_settings = AllSettings::from_value(
+        let mut all_options = AllOptions::from_value(
             initialization_options
                 .unwrap_or_else(|| serde_json::Value::Object(serde_json::Map::default())),
         );
         if let Some(preview) = preview {
-            all_settings.set_preview(preview);
+            all_options.set_preview(preview);
         }
-        let AllSettings {
-            global_settings,
-            workspace_settings,
-        } = all_settings;
+        let AllOptions {
+            global: global_options,
+            workspace: workspace_options,
+        } = all_options;
 
         crate::logging::init_logging(
-            global_settings.tracing.log_level.unwrap_or_default(),
-            global_settings.tracing.log_file.as_deref(),
+            global_options.tracing.log_level.unwrap_or_default(),
+            global_options.tracing.log_file.as_deref(),
         );
 
         let workspaces = Workspaces::from_workspace_folders(
             workspace_folders,
-            workspace_settings.unwrap_or_default(),
+            workspace_options.unwrap_or_default(),
         )?;
 
         tracing::debug!("Negotiated position encoding: {position_encoding:?}");
 
+        let global = global_options.into_settings();
+
         Ok(Self {
             connection,
             worker_threads,
-            session: Session::new(
-                &client_capabilities,
-                position_encoding,
-                global_settings,
-                &workspaces,
-            )?,
+            session: Session::new(&client_capabilities, position_encoding, global, &workspaces)?,
             client_capabilities,
         })
     }

crates/ruff_server/src/session.rs

@@ -4,19 +4,21 @@ use std::path::Path;
 use std::sync::Arc;
 
 use lsp_types::{ClientCapabilities, FileEvent, NotebookDocumentCellChange, Url};
-use settings::ResolvedClientSettings;
+use settings::ClientSettings;
 
 use crate::edit::{DocumentKey, DocumentVersion, NotebookDocument};
+use crate::session::settings::GlobalClientSettings;
 use crate::workspace::Workspaces;
 use crate::{PositionEncoding, TextDocument};
 
 pub(crate) use self::capabilities::ResolvedClientCapabilities;
 pub use self::index::DocumentQuery;
-pub use self::settings::ClientSettings;
-pub(crate) use self::settings::{AllSettings, WorkspaceSettingsMap};
+pub(crate) use self::options::{AllOptions, WorkspaceOptionsMap};
+pub use self::options::{ClientOptions, GlobalOptions};
 
 mod capabilities;
 mod index;
+mod options;
 mod settings;
 
 /// The global state for the LSP
@@ -26,7 +28,8 @@ pub struct Session {
     /// The global position encoding, negotiated during LSP initialization.
     position_encoding: PositionEncoding,
     /// Global settings provided by the client.
-    global_settings: ClientSettings,
+    global_settings: GlobalClientSettings,
+
     /// Tracks what LSP features the client supports and doesn't support.
     resolved_client_capabilities: Arc<ResolvedClientCapabilities>,
 }
@@ -35,7 +38,7 @@ pub struct Session {
 /// a specific document.
 pub struct DocumentSnapshot {
     resolved_client_capabilities: Arc<ResolvedClientCapabilities>,
-    client_settings: settings::ResolvedClientSettings,
+    client_settings: Arc<settings::ClientSettings>,
     document_ref: index::DocumentQuery,
     position_encoding: PositionEncoding,
 }
@@ -44,13 +47,13 @@ impl Session {
     pub fn new(
         client_capabilities: &ClientCapabilities,
         position_encoding: PositionEncoding,
-        global_settings: ClientSettings,
+        global: GlobalClientSettings,
         workspaces: &Workspaces,
     ) -> crate::Result<Self> {
         Ok(Self {
             position_encoding,
-            index: index::Index::new(workspaces, &global_settings)?,
-            global_settings,
+            index: index::Index::new(workspaces, &global)?,
+            global_settings: global,
             resolved_client_capabilities: Arc::new(ResolvedClientCapabilities::new(
                 client_capabilities,
             )),
@@ -66,7 +69,10 @@ impl Session {
         let key = self.key_from_url(url);
         Some(DocumentSnapshot {
             resolved_client_capabilities: self.resolved_client_capabilities.clone(),
-            client_settings: self.index.client_settings(&key, &self.global_settings),
+            client_settings: self
+                .index
+                .client_settings(&key)
+                .unwrap_or_else(|| self.global_settings.to_settings_arc()),
             document_ref: self.index.make_document_ref(key, &self.global_settings)?,
             position_encoding: self.position_encoding,
         })
@@ -163,8 +169,8 @@ impl Session {
     }
 
     /// Returns the resolved global client settings.
-    pub(crate) fn global_client_settings(&self) -> ResolvedClientSettings {
-        ResolvedClientSettings::global(&self.global_settings)
+    pub(crate) fn global_client_settings(&self) -> &ClientSettings {
+        self.global_settings.to_settings()
     }
 
     /// Returns the number of open documents in the session.
@@ -183,7 +189,7 @@ impl DocumentSnapshot {
         &self.resolved_client_capabilities
     }
 
-    pub(crate) fn client_settings(&self) -> &settings::ResolvedClientSettings {
+    pub(crate) fn client_settings(&self) -> &settings::ClientSettings {
         &self.client_settings
     }
 

crates/ruff_server/src/session/index.rs

@@ -11,13 +11,15 @@ use thiserror::Error;
 pub(crate) use ruff_settings::RuffSettings;
 
 use crate::edit::LanguageId;
+use crate::session::options::Combine;
+use crate::session::settings::GlobalClientSettings;
 use crate::workspace::{Workspace, Workspaces};
 use crate::{
     PositionEncoding, TextDocument,
     edit::{DocumentKey, DocumentVersion, NotebookDocument},
 };
 
-use super::{ClientSettings, settings::ResolvedClientSettings};
+use super::settings::ClientSettings;
 
 mod ruff_settings;
 
@@ -36,7 +38,7 @@ pub(crate) struct Index {
 
 /// Settings associated with a workspace.
 struct WorkspaceSettings {
-    client_settings: ResolvedClientSettings,
+    client_settings: Arc<ClientSettings>,
     ruff_settings: ruff_settings::RuffSettingsIndex,
 }
 
@@ -70,11 +72,11 @@ pub enum DocumentQuery {
 impl Index {
     pub(super) fn new(
         workspaces: &Workspaces,
-        global_settings: &ClientSettings,
+        global: &GlobalClientSettings,
     ) -> crate::Result<Self> {
         let mut settings = WorkspaceSettingsIndex::default();
         for workspace in &**workspaces {
-            settings.register_workspace(workspace, global_settings)?;
+            settings.register_workspace(workspace, global)?;
         }
 
         Ok(Self {
@@ -170,11 +172,11 @@ impl Index {
     pub(super) fn open_workspace_folder(
         &mut self,
         url: Url,
-        global_settings: &ClientSettings,
+        global: &GlobalClientSettings,
     ) -> crate::Result<()> {
         // TODO(jane): Find a way for workspace client settings to be added or changed dynamically.
         self.settings
-            .register_workspace(&Workspace::new(url), global_settings)
+            .register_workspace(&Workspace::new(url), global)
     }
 
     pub(super) fn close_workspace_folder(&mut self, workspace_url: &Url) -> crate::Result<()> {
@@ -201,7 +203,7 @@ impl Index {
     pub(super) fn make_document_ref(
         &self,
         key: DocumentKey,
-        global_settings: &ClientSettings,
+        global: &GlobalClientSettings,
     ) -> Option<DocumentQuery> {
         let url = self.url_for_key(&key)?.clone();
 
@@ -230,13 +232,12 @@ impl Index {
                     "No settings available for {} - falling back to default settings",
                     url
                 );
-                let resolved_global = ResolvedClientSettings::global(global_settings);
                 // The path here is only for completeness, it's okay to use a non-existing path
                 // in case this is an unsaved (untitled) document.
                 let path = Path::new(url.path());
                 let root = path.parent().unwrap_or(path);
                 Arc::new(RuffSettings::fallback(
-                    resolved_global.editor_settings(),
+                    global.to_settings().editor_settings(),
                     root,
                 ))
             });
@@ -330,21 +331,12 @@ impl Index {
         Ok(())
     }
 
-    pub(super) fn client_settings(
-        &self,
-        key: &DocumentKey,
-        global_settings: &ClientSettings,
-    ) -> ResolvedClientSettings {
-        let Some(url) = self.url_for_key(key) else {
-            return ResolvedClientSettings::global(global_settings);
-        };
-        let Some(WorkspaceSettings {
+    pub(super) fn client_settings(&self, key: &DocumentKey) -> Option<Arc<ClientSettings>> {
+        let url = self.url_for_key(key)?;
+        let WorkspaceSettings {
             client_settings, ..
-        }) = self.settings_for_url(url)
-        else {
-            return ResolvedClientSettings::global(global_settings);
-        };
-        client_settings.clone()
+        } = self.settings_for_url(url)?;
+        Some(client_settings.clone())
     }
 
     fn document_controller_for_key(
@@ -422,7 +414,7 @@ impl WorkspaceSettingsIndex {
     fn register_workspace(
         &mut self,
         workspace: &Workspace,
-        global_settings: &ClientSettings,
+        global: &GlobalClientSettings,
     ) -> crate::Result<()> {
         let workspace_url = workspace.url();
         if workspace_url.scheme() != "file" {
@@ -434,10 +426,21 @@ impl WorkspaceSettingsIndex {
             anyhow!("Failed to convert workspace URL to file path: {workspace_url}")
         })?;
 
-        let client_settings = if let Some(workspace_settings) = workspace.settings() {
-            ResolvedClientSettings::with_workspace(workspace_settings, global_settings)
+        let client_settings = if let Some(workspace_options) = workspace.options() {
+            let options = workspace_options.clone().combine(global.options().clone());
+            let settings = match options.into_settings() {
+                Ok(settings) => settings,
+                Err(settings) => {
+                    show_err_msg!(
+                        "The settings for the workspace {workspace_path} are invalid. Refer to the logs for more information.",
+                        workspace_path = workspace_path.display()
+                    );
+                    settings
+                }
+            };
+            Arc::new(settings)
         } else {
-            ResolvedClientSettings::global(global_settings)
+            global.to_settings_arc()
         };
 
         let workspace_settings_index = ruff_settings::RuffSettingsIndex::new(

crates/ruff_server/src/session/index/ruff_settings.rs

@@ -18,9 +18,8 @@ use ruff_workspace::{
     resolver::ConfigurationTransformer,
 };
 
-use crate::session::settings::{
-    ConfigurationPreference, ResolvedConfiguration, ResolvedEditorSettings,
-};
+use crate::session::options::ConfigurationPreference;
+use crate::session::settings::{EditorSettings, ResolvedConfiguration};
 
 #[derive(Debug)]
 pub struct RuffSettings {
@@ -64,7 +63,7 @@ impl RuffSettings {
     ///
     /// In the absence of a valid configuration file, it gracefully falls back to
     /// editor-only settings.
-    pub(crate) fn fallback(editor_settings: &ResolvedEditorSettings, root: &Path) -> RuffSettings {
+    pub(crate) fn fallback(editor_settings: &EditorSettings, root: &Path) -> RuffSettings {
         struct FallbackTransformer<'a> {
             inner: EditorConfigurationTransformer<'a>,
         }
@@ -122,14 +121,14 @@ impl RuffSettings {
 
     /// Constructs [`RuffSettings`] by merging the editor-defined settings with the
     /// default configuration.
-    fn editor_only(editor_settings: &ResolvedEditorSettings, root: &Path) -> RuffSettings {
+    fn editor_only(editor_settings: &EditorSettings, root: &Path) -> RuffSettings {
         Self::with_editor_settings(editor_settings, root, Configuration::default())
             .expect("editor configuration should merge successfully with default configuration")
     }
 
     /// Merges the `configuration` with the editor defined settings.
     fn with_editor_settings(
-        editor_settings: &ResolvedEditorSettings,
+        editor_settings: &EditorSettings,
         root: &Path,
         configuration: Configuration,
     ) -> anyhow::Result<RuffSettings> {
@@ -157,7 +156,7 @@ impl RuffSettingsIndex {
     /// skipping (3).
     pub(super) fn new(
         root: &Path,
-        editor_settings: &ResolvedEditorSettings,
+        editor_settings: &EditorSettings,
         is_default_workspace: bool,
     ) -> Self {
         if editor_settings.configuration_preference == ConfigurationPreference::EditorOnly {
@@ -392,11 +391,11 @@ impl RuffSettingsIndex {
     }
 }
 
-struct EditorConfigurationTransformer<'a>(&'a ResolvedEditorSettings, &'a Path);
+struct EditorConfigurationTransformer<'a>(&'a EditorSettings, &'a Path);
 
 impl ConfigurationTransformer for EditorConfigurationTransformer<'_> {
     fn transform(&self, filesystem_configuration: Configuration) -> Configuration {
-        let ResolvedEditorSettings {
+        let EditorSettings {
             configuration,
             format_preview,
             lint_preview,
@@ -515,7 +514,7 @@ mod tests {
     /// This test ensures that the inline configuration is correctly applied to the configuration.
     #[test]
     fn inline_settings() {
-        let editor_settings = ResolvedEditorSettings {
+        let editor_settings = EditorSettings {
             configuration: Some(ResolvedConfiguration::Inline(Box::new(Options {
                 line_length: Some(LineLength::try_from(120).unwrap()),
                 ..Default::default()
@@ -533,7 +532,7 @@ mod tests {
     /// settings is prioritized.
     #[test]
     fn inline_and_specific_settings_resolution_order() {
-        let editor_settings = ResolvedEditorSettings {
+        let editor_settings = EditorSettings {
             configuration: Some(ResolvedConfiguration::Inline(Box::new(Options {
                 line_length: Some(LineLength::try_from(120).unwrap()),
                 ..Default::default()