From 6d7055e3223a33e3df7a8dd0c01e9dec4b91ac38 Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Mon, 11 Apr 2022 13:05:34 +0200 Subject: [PATCH 1/2] Show config deseralization failures on start up --- crates/rust-analyzer/src/bin/main.rs | 20 +++- crates/rust-analyzer/src/caps.rs | 4 +- crates/rust-analyzer/src/cargo_target_spec.rs | 2 +- crates/rust-analyzer/src/config.rs | 106 ++++++++++-------- crates/rust-analyzer/src/main_loop.rs | 13 +-- 5 files changed, 84 insertions(+), 61 deletions(-) diff --git a/crates/rust-analyzer/src/bin/main.rs b/crates/rust-analyzer/src/bin/main.rs index 9a2e4355915..a4e985f43b3 100644 --- a/crates/rust-analyzer/src/bin/main.rs +++ b/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::>() }) .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); diff --git a/crates/rust-analyzer/src/caps.rs b/crates/rust-analyzer/src/caps.rs index ca42bf321e5..85e3d500538 100644 --- a/crates/rust-analyzer/src/caps.rs +++ b/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() { diff --git a/crates/rust-analyzer/src/cargo_target_spec.rs b/crates/rust-analyzer/src/cargo_target_spec.rs index ec5dd16d001..35f8f61ef44 100644 --- a/crates/rust-analyzer/src/cargo_target_spec.rs +++ b/crates/rust-analyzer/src/cargo_target_spec.rs @@ -123,7 +123,7 @@ pub(crate) fn for_file( file_id: FileId, ) -> Result> { 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) { diff --git a/crates/rust-analyzer/src/config.rs b/crates/rust-analyzer/src/config.rs index 958ff1fcee1..4d02e896534 100644 --- a/crates/rust-analyzer/src/config.rs +++ b/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 @@ 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 @@ 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 @@ fn default() -> Self { #[derive(Debug, Clone)] pub struct Config { - pub caps: lsp_types::ClientCapabilities, + pub discovered_projects: Option>, + caps: lsp_types::ClientCapabilities, + root_path: AbsPathBuf, data: ConfigData, detached_files: Vec, - pub discovered_projects: Option>, - pub root_path: AbsPathBuf, snippets: Vec, } @@ -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 @@ pub fn new(root_path: AbsPathBuf, caps: ClientCapabilities) -> Self { 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(()); @@ -556,13 +574,25 @@ pub fn update( if errors.is_empty() { Ok(()) } else { - Err(errors) + Err(ConfigUpdateError { errors }) } } 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 +608,31 @@ macro_rules! try_or { impl Config { pub fn linked_projects(&self) -> Vec { - 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()); diff --git a/crates/rust-analyzer/src/main_loop.rs b/crates/rust-analyzer/src/main_loop.rs index 27ec32b1f41..2f0e5c64b98 100644 --- a/crates/rust-analyzer/src/main_loop.rs +++ b/crates/rust-analyzer/src/main_loop.rs @@ -9,7 +9,6 @@ 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 @@ fn on_notification(&mut self, not: Notification) -> Result<()> { // 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); } From b90df7997d891d89e658a7781cc4a52fba38aa40 Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Mon, 11 Apr 2022 13:10:43 +0200 Subject: [PATCH 2/2] Add simplistic config validation --- crates/rust-analyzer/src/config.rs | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/crates/rust-analyzer/src/config.rs b/crates/rust-analyzer/src/config.rs index 4d02e896534..ab9ad4a5431 100644 --- a/crates/rust-analyzer/src/config.rs +++ b/crates/rust-analyzer/src/config.rs @@ -571,6 +571,9 @@ pub fn update(&mut self, mut json: serde_json::Value) -> Result<(), ConfigUpdate None => tracing::info!("Invalid snippet {}", name), } } + + self.validate(&mut errors); + if errors.is_empty() { Ok(()) } else { @@ -578,6 +581,16 @@ pub fn update(&mut self, mut json: serde_json::Value) -> Result<(), ConfigUpdate } } + 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() }