Merge #11958

11958: Show config deseralization failures on start up r=Veykril a=Veykril

We now also show deserialization errors to the user when starting the server.
This PR also adds a small validation "pass" on the config that we will probably populate over time with more checks.

Fixes #11950

bors r+

Co-authored-by: Lukas Wirth <[email protected]>

Author: bors[bot]

crates/rust-analyzer/src/bin/main.rs

@@ -150,7 +150,17 @@ fn run_server() -> Result<()> {
 
     let mut config = Config::new(root_path, initialize_params.capabilities);
     if let Some(json) = initialize_params.initialization_options {
-        let _ = config.update(json);
+        if let Err(e) = config.update(json) {
+            use lsp_types::{
+                notification::{Notification, ShowMessage},
+                MessageType, ShowMessageParams,
+            };
+            let not = lsp_server::Notification::new(
+                ShowMessage::METHOD.to_string(),
+                ShowMessageParams { typ: MessageType::WARNING, message: e.to_string() },
+            );
+            connection.sender.send(lsp_server::Message::Notification(not)).unwrap();
+        }
     }
 
     let server_capabilities = rust_analyzer::server_capabilities(&config);
@@ -161,7 +171,11 @@ fn run_server() -> Result<()> {
             name: String::from("rust-analyzer"),
             version: Some(String::from(env!("REV"))),
         }),
-        offset_encoding: if supports_utf8(&config.caps) { Some("utf-8".to_string()) } else { None },
+        offset_encoding: if supports_utf8(config.caps()) {
+            Some("utf-8".to_string())
+        } else {
+            None
+        },
     };
 
     let initialize_result = serde_json::to_value(initialize_result).unwrap();
@@ -183,7 +197,7 @@ fn run_server() -> Result<()> {
                     .collect::<Vec<_>>()
             })
             .filter(|workspaces| !workspaces.is_empty())
-            .unwrap_or_else(|| vec![config.root_path.clone()]);
+            .unwrap_or_else(|| vec![config.root_path().clone()]);
 
         let discovered = ProjectManifest::discover_all(&workspace_roots);
         tracing::info!("discovered projects: {:?}", discovered);

crates/rust-analyzer/src/caps.rs

@@ -27,7 +27,7 @@ pub fn server_capabilities(config: &Config) -> ServerCapabilities {
         })),
         hover_provider: Some(HoverProviderCapability::Simple(true)),
         completion_provider: Some(CompletionOptions {
-            resolve_provider: completions_resolve_provider(&config.caps),
+            resolve_provider: completions_resolve_provider(config.caps()),
             trigger_characters: Some(vec![":".to_string(), ".".to_string(), "'".to_string()]),
             all_commit_characters: None,
             completion_item: None,
@@ -46,7 +46,7 @@ pub fn server_capabilities(config: &Config) -> ServerCapabilities {
         document_highlight_provider: Some(OneOf::Left(true)),
         document_symbol_provider: Some(OneOf::Left(true)),
         workspace_symbol_provider: Some(OneOf::Left(true)),
-        code_action_provider: Some(code_action_capabilities(&config.caps)),
+        code_action_provider: Some(code_action_capabilities(config.caps())),
         code_lens_provider: Some(CodeLensOptions { resolve_provider: Some(true) }),
         document_formatting_provider: Some(OneOf::Left(true)),
         document_range_formatting_provider: match config.rustfmt() {

crates/rust-analyzer/src/cargo_target_spec.rs

@@ -123,7 +123,7 @@ impl CargoTargetSpec {
         file_id: FileId,
     ) -> Result<Option<CargoTargetSpec>> {
         let crate_id = match global_state_snapshot.analysis.crate_for(file_id)?.first() {
-            Some(crate_id) => *crate_id,
+            Some(&crate_id) => crate_id,
             None => return Ok(None),
         };
         let (cargo_ws, target) = match global_state_snapshot.cargo_target_for_crate_root(crate_id) {

crates/rust-analyzer/src/config.rs

@@ -7,7 +7,7 @@
 //! configure the server itself, feature flags are passed into analysis, and
 //! tweak things like automatic insertion of `()` in completions.
 
-use std::{ffi::OsString, iter, path::PathBuf};
+use std::{ffi::OsString, fmt, iter, path::PathBuf};
 
 use flycheck::FlycheckConfig;
 use ide::{
@@ -19,6 +19,7 @@ use ide_db::{
     imports::insert_use::{ImportGranularity, InsertUseConfig, PrefixKind},
     SnippetCap,
 };
+use itertools::Itertools;
 use lsp_types::{ClientCapabilities, MarkupKind};
 use project_model::{
     CargoConfig, ProjectJson, ProjectJsonData, ProjectManifest, RustcSource, UnsetTestCrates,
@@ -31,9 +32,7 @@ use crate::{
     caps::completion_item_edit_resolve,
     diagnostics::DiagnosticsMapConfig,
     line_index::OffsetEncoding,
-    lsp_ext::supports_utf8,
-    lsp_ext::WorkspaceSymbolSearchScope,
-    lsp_ext::{self, WorkspaceSymbolSearchKind},
+    lsp_ext::{self, supports_utf8, WorkspaceSymbolSearchKind, WorkspaceSymbolSearchScope},
 };
 
 // Defines the server-side configuration of the rust-analyzer. We generate
@@ -369,11 +368,11 @@ impl Default for ConfigData {
 
 #[derive(Debug, Clone)]
 pub struct Config {
-    pub caps: lsp_types::ClientCapabilities,
+    pub discovered_projects: Option<Vec<ProjectManifest>>,
+    caps: lsp_types::ClientCapabilities,
+    root_path: AbsPathBuf,
     data: ConfigData,
     detached_files: Vec<AbsPathBuf>,
-    pub discovered_projects: Option<Vec<ProjectManifest>>,
-    pub root_path: AbsPathBuf,
     snippets: Vec<Snippet>,
 }
 
@@ -505,6 +504,27 @@ pub struct ClientCommandsConfig {
     pub trigger_parameter_hints: bool,
 }
 
+pub struct ConfigUpdateError {
+    errors: Vec<(String, serde_json::Error)>,
+}
+
+impl fmt::Display for ConfigUpdateError {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        let errors = self.errors.iter().format_with("\n", |(key, e), f| {
+            f(key)?;
+            f(&": ")?;
+            f(e)
+        });
+        write!(
+            f,
+            "rust-analyzer found {} invalid config value{}:\n{}",
+            self.errors.len(),
+            if self.errors.len() == 1 { "" } else { "s" },
+            errors
+        )
+    }
+}
+
 impl Config {
     pub fn new(root_path: AbsPathBuf, caps: ClientCapabilities) -> Self {
         Config {
@@ -516,10 +536,8 @@ impl Config {
             snippets: Default::default(),
         }
     }
-    pub fn update(
-        &mut self,
-        mut json: serde_json::Value,
-    ) -> Result<(), Vec<(String, serde_json::Error)>> {
+
+    pub fn update(&mut self, mut json: serde_json::Value) -> Result<(), ConfigUpdateError> {
         tracing::info!("updating config from JSON: {:#}", json);
         if json.is_null() || json.as_object().map_or(false, |it| it.is_empty()) {
             return Ok(());
@@ -553,16 +571,41 @@ impl Config {
                 None => tracing::info!("Invalid snippet {}", name),
             }
         }
+
+        self.validate(&mut errors);
+
         if errors.is_empty() {
             Ok(())
         } else {
-            Err(errors)
+            Err(ConfigUpdateError { errors })
+        }
+    }
+
+    fn validate(&self, error_sink: &mut Vec<(String, serde_json::Error)>) {
+        use serde::de::Error;
+        if self.data.checkOnSave_command.is_empty() {
+            error_sink.push((
+                "/checkOnSave/command".to_string(),
+                serde_json::Error::custom("expected a non-empty string"),
+            ));
         }
     }
 
     pub fn json_schema() -> serde_json::Value {
         ConfigData::json_schema()
     }
+
+    pub fn root_path(&self) -> &AbsPathBuf {
+        &self.root_path
+    }
+
+    pub fn caps(&self) -> &lsp_types::ClientCapabilities {
+        &self.caps
+    }
+
+    pub fn detached_files(&self) -> &[AbsPathBuf] {
+        &self.detached_files
+    }
 }
 
 macro_rules! try_ {
@@ -578,43 +621,31 @@ macro_rules! try_or {
 
 impl Config {
     pub fn linked_projects(&self) -> Vec<LinkedProject> {
-        if self.data.linkedProjects.is_empty() {
-            self.discovered_projects
-                .as_ref()
-                .into_iter()
-                .flatten()
-                .cloned()
-                .map(LinkedProject::from)
-                .collect()
-        } else {
-            self.data
-                .linkedProjects
+        match self.data.linkedProjects.as_slice() {
+            [] => match self.discovered_projects.as_ref() {
+                Some(discovered_projects) => {
+                    discovered_projects.iter().cloned().map(LinkedProject::from).collect()
+                }
+                None => Vec::new(),
+            },
+            linked_projects => linked_projects
                 .iter()
-                .filter_map(|linked_project| {
-                    let res = match linked_project {
-                        ManifestOrProjectJson::Manifest(it) => {
-                            let path = self.root_path.join(it);
-                            ProjectManifest::from_manifest_file(path)
-                                .map_err(|e| {
-                                    tracing::error!("failed to load linked project: {}", e)
-                                })
-                                .ok()?
-                                .into()
-                        }
-                        ManifestOrProjectJson::ProjectJson(it) => {
-                            ProjectJson::new(&self.root_path, it.clone()).into()
-                        }
-                    };
-                    Some(res)
+                .filter_map(|linked_project| match linked_project {
+                    ManifestOrProjectJson::Manifest(it) => {
+                        let path = self.root_path.join(it);
+                        ProjectManifest::from_manifest_file(path)
+                            .map_err(|e| tracing::error!("failed to load linked project: {}", e))
+                            .ok()
+                            .map(Into::into)
+                    }
+                    ManifestOrProjectJson::ProjectJson(it) => {
+                        Some(ProjectJson::new(&self.root_path, it.clone()).into())
+                    }
                 })
-                .collect()
+                .collect(),
         }
     }
 
-    pub fn detached_files(&self) -> &[AbsPathBuf] {
-        &self.detached_files
-    }
-
     pub fn did_save_text_document_dynamic_registration(&self) -> bool {
         let caps =
             try_or!(self.caps.text_document.as_ref()?.synchronization.clone()?, Default::default());

crates/rust-analyzer/src/main_loop.rs

@@ -9,7 +9,6 @@ use std::{
 use always_assert::always;
 use crossbeam_channel::{select, Receiver};
 use ide_db::base_db::{SourceDatabaseExt, VfsPath};
-use itertools::Itertools;
 use lsp_server::{Connection, Notification, Request};
 use lsp_types::notification::Notification as _;
 use vfs::{ChangeKind, FileId};
@@ -747,16 +746,8 @@ impl GlobalState {
                                     // Note that json can be null according to the spec if the client can't
                                     // provide a configuration. This is handled in Config::update below.
                                     let mut config = Config::clone(&*this.config);
-                                    if let Err(errors) = config.update(json.take()) {
-                                        let errors = errors
-                                            .iter()
-                                            .format_with("\n", |(key, e),f| {
-                                                f(key)?;
-                                                f(&": ")?;
-                                                f(e)
-                                            });
-                                        let msg= format!("Failed to deserialize config key(s):\n{}", errors);
-                                        this.show_message(lsp_types::MessageType::WARNING, msg);
+                                    if let Err(error) = config.update(json.take()) {
+                                        this.show_message(lsp_types::MessageType::WARNING, error.to_string());
                                     }
                                     this.update_configuration(config);
                                 }