From c4a1c569a9e30d9a5fecb72743d04892fd30a5da Mon Sep 17 00:00:00 2001 From: Lukas Scheller <45085299+Schottkyc137@users.noreply.github.com> Date: Mon, 1 Jul 2024 11:45:43 +0200 Subject: [PATCH] Avoid sending all diagnostics on every file change (#310) This avoids sending all diagnostics on every small change by only sending diagnostics that the server has not seen before. The intent is to improve performance by reducing the communication overhead --- Cargo.lock | 1 + vhdl_lang/src/project.rs | 4 +- vhdl_ls/Cargo.toml | 1 + vhdl_ls/src/rpc_channel.rs | 53 +++++- vhdl_ls/src/vhdl_server.rs | 145 ++------------- vhdl_ls/src/vhdl_server/diagnostics.rs | 235 +++++++++++++++++++++++++ 6 files changed, 306 insertions(+), 133 deletions(-) create mode 100644 vhdl_ls/src/vhdl_server/diagnostics.rs diff --git a/Cargo.lock b/Cargo.lock index 4ceebc44..cdf7153b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1036,6 +1036,7 @@ dependencies = [ "lsp-server", "lsp-types", "pretty_assertions", + "regex", "serde", "serde_json", "tempfile", diff --git a/vhdl_lang/src/project.rs b/vhdl_lang/src/project.rs index c1ee5409..16701e1f 100644 --- a/vhdl_lang/src/project.rs +++ b/vhdl_lang/src/project.rs @@ -229,9 +229,7 @@ impl Project { self.root.add_design_file(library_name.clone(), design_file); } - for diagnostic in source_file.parser_diagnostics.iter().cloned() { - diagnostics.push(diagnostic); - } + diagnostics.extend(source_file.parser_diagnostics.iter().cloned()); } for library_name in self.empty_libraries.iter() { diff --git a/vhdl_ls/Cargo.toml b/vhdl_ls/Cargo.toml index a464e5da..bc71c935 100644 --- a/vhdl_ls/Cargo.toml +++ b/vhdl_ls/Cargo.toml @@ -29,6 +29,7 @@ fuzzy-matcher = "0.3.7" [dev-dependencies] tempfile = "3" pretty_assertions = "1" +regex = "1.10.4" [features] default = [] diff --git a/vhdl_ls/src/rpc_channel.rs b/vhdl_ls/src/rpc_channel.rs index c7cb6a01..c9c96919 100644 --- a/vhdl_ls/src/rpc_channel.rs +++ b/vhdl_ls/src/rpc_channel.rs @@ -48,6 +48,7 @@ impl SharedRpcChannel { pub mod test_support { use pretty_assertions::assert_eq; + use regex::Regex; use serde_json::Value; use std::cell::RefCell; use std::collections::VecDeque; @@ -60,7 +61,14 @@ pub mod test_support { notification: serde_json::Value, }, /// Check that the string representation of the notification contains a string - NotificationContainsString { method: String, contains: String }, + NotificationContainsString { + method: String, + contains: String, + }, + NotificationContainsRegex { + method: String, + contains: Regex, + }, Request { method: String, params: serde_json::Value, @@ -92,7 +100,7 @@ pub mod test_support { }); } - fn expect_notification_contains( + pub fn expect_notification_contains( &self, method: impl Into, contains: impl Into, @@ -105,6 +113,19 @@ pub mod test_support { }); } + pub fn expect_notification_contains_regex( + &self, + method: impl Into, + contains: Regex, + ) { + self.expected + .borrow_mut() + .push_back(RpcExpected::NotificationContainsRegex { + method: method.into(), + contains, + }); + } + pub fn expect_request( &self, method: impl Into, @@ -159,6 +180,21 @@ pub mod test_support { } } + fn contains_regex(value: &serde_json::Value, regex: &Regex) -> bool { + match value { + serde_json::Value::Array(values) => { + values.iter().any(|value| contains_regex(value, regex)) + } + serde_json::Value::Object(map) => { + map.values().any(|value| contains_regex(value, regex)) + } + serde_json::Value::String(got_string) => regex.is_match(got_string), + serde_json::Value::Null => false, + serde_json::Value::Bool(..) => false, + serde_json::Value::Number(..) => false, + } + } + impl super::RpcChannel for RpcMock { fn send_notification(&self, method: String, notification: Value) { let expected = self @@ -190,6 +226,19 @@ pub mod test_support { panic!("{notification:?} does not contain sub-string {contains:?}"); } } + RpcExpected::NotificationContainsRegex { + method: exp_method, + contains, + } => { + assert_eq!( + method, exp_method, + "{:?} contains {:?}", + notification, contains + ); + if !contains_regex(¬ification, &contains) { + panic!("{notification:?} does not match regex {contains:?}"); + } + } _ => panic!("Expected {expected:?}, got notification {method} {notification:?}"), } } diff --git a/vhdl_ls/src/vhdl_server.rs b/vhdl_ls/src/vhdl_server.rs index 7f675abb..9b2e4796 100644 --- a/vhdl_ls/src/vhdl_server.rs +++ b/vhdl_ls/src/vhdl_server.rs @@ -5,6 +5,7 @@ // Copyright (c) 2018, Olof Kraigher olof.kraigher@gmail.com mod completion; +mod diagnostics; mod lifecycle; mod rename; mod text_document; @@ -13,7 +14,6 @@ mod workspace; use lsp_types::*; use fnv::FnvHashMap; -use std::collections::hash_map::Entry; use vhdl_lang::ast::ObjectClass; use crate::rpc_channel::SharedRpcChannel; @@ -22,8 +22,8 @@ use std::io; use std::io::ErrorKind; use std::path::{Path, PathBuf}; use vhdl_lang::{ - AnyEntKind, Concurrent, Config, Diagnostic, EntHierarchy, EntRef, Message, MessageHandler, - Object, Overloaded, Project, Severity, SeverityMap, SrcPos, Token, Type, VHDLStandard, + AnyEntKind, Concurrent, Config, EntHierarchy, EntRef, Message, MessageHandler, Object, + Overloaded, Project, SeverityMap, SrcPos, Token, Type, VHDLStandard, }; /// Defines how the language server handles files @@ -61,7 +61,7 @@ pub struct VHDLServer { // To have well defined unit tests that are not affected by environment use_external_config: bool, project: Project, - files_with_notifications: FnvHashMap, + diagnostic_cache: FnvHashMap>, init_params: Option, config_file: Option, severity_map: SeverityMap, @@ -75,7 +75,7 @@ impl VHDLServer { settings, use_external_config: true, project: Project::new(VHDLStandard::default()), - files_with_notifications: FnvHashMap::default(), + diagnostic_cache: FnvHashMap::default(), init_params: None, config_file: None, severity_map: SeverityMap::default(), @@ -90,7 +90,7 @@ impl VHDLServer { settings: Default::default(), use_external_config, project: Project::new(VHDLStandard::default()), - files_with_notifications: FnvHashMap::default(), + diagnostic_cache: Default::default(), init_params: None, config_file: None, severity_map: SeverityMap::default(), @@ -230,56 +230,6 @@ impl VHDLServer { try_fun().unwrap_or(false) } - fn publish_diagnostics(&mut self) { - let diagnostics = self.project.analyse(); - - if self.settings.no_lint { - return; - } - - let supports_related_information = self.client_supports_related_information(); - let diagnostics = { - if supports_related_information { - diagnostics - } else { - flatten_related(diagnostics) - } - }; - - let mut files_with_notifications = std::mem::take(&mut self.files_with_notifications); - for (file_uri, diagnostics) in diagnostics_by_uri(diagnostics).into_iter() { - let lsp_diagnostics = diagnostics - .into_iter() - .filter_map(|diag| to_lsp_diagnostic(diag, &self.severity_map)) - .collect(); - - let publish_diagnostics = PublishDiagnosticsParams { - uri: file_uri.clone(), - diagnostics: lsp_diagnostics, - version: None, - }; - - self.rpc - .send_notification("textDocument/publishDiagnostics", publish_diagnostics); - - self.files_with_notifications.insert(file_uri.clone(), ()); - } - - for (file_uri, _) in files_with_notifications.drain() { - // File has no longer any diagnostics, publish empty notification to clear them - if !self.files_with_notifications.contains_key(&file_uri) { - let publish_diagnostics = PublishDiagnosticsParams { - uri: file_uri.clone(), - diagnostics: vec![], - version: None, - }; - - self.rpc - .send_notification("textDocument/publishDiagnostics", publish_diagnostics); - } - } - } - pub fn document_symbol(&self, params: &DocumentSymbolParams) -> Option { let source = self .project @@ -449,32 +399,6 @@ fn from_lsp_range(range: lsp_types::Range) -> vhdl_lang::Range { } } -fn diagnostics_by_uri(diagnostics: Vec) -> FnvHashMap> { - let mut map: FnvHashMap> = FnvHashMap::default(); - - for diagnostic in diagnostics { - let uri = file_name_to_uri(diagnostic.pos.source.file_name()); - match map.entry(uri) { - Entry::Occupied(mut entry) => entry.get_mut().push(diagnostic), - Entry::Vacant(entry) => { - let vec = vec![diagnostic]; - entry.insert(vec); - } - } - } - - map -} - -fn flatten_related(diagnostics: Vec) -> Vec { - let mut flat_diagnostics = Vec::new(); - for mut diagnostic in diagnostics { - flat_diagnostics.extend(diagnostic.drain_related()); - flat_diagnostics.push(diagnostic); - } - flat_diagnostics -} - fn file_name_to_uri(file_name: &Path) -> Url { // @TODO return error to client Url::from_file_path(file_name).unwrap() @@ -485,45 +409,6 @@ fn uri_to_file_name(uri: &Url) -> PathBuf { uri.to_file_path().unwrap() } -fn to_lsp_diagnostic( - diagnostic: Diagnostic, - severity_map: &SeverityMap, -) -> Option { - let severity = match severity_map[diagnostic.code]? { - Severity::Error => DiagnosticSeverity::ERROR, - Severity::Warning => DiagnosticSeverity::WARNING, - Severity::Info => DiagnosticSeverity::INFORMATION, - Severity::Hint => DiagnosticSeverity::HINT, - }; - - let related_information = if !diagnostic.related.is_empty() { - let mut related_information = Vec::new(); - for (pos, msg) in diagnostic.related { - let uri = file_name_to_uri(pos.source.file_name()); - related_information.push(DiagnosticRelatedInformation { - location: Location { - uri: uri.to_owned(), - range: to_lsp_range(pos.range()), - }, - message: msg, - }) - } - Some(related_information) - } else { - None - }; - - Some(lsp_types::Diagnostic { - range: to_lsp_range(diagnostic.pos.range()), - severity: Some(severity), - code: Some(NumberOrString::String(format!("{}", diagnostic.code))), - source: Some("vhdl ls".to_owned()), - message: diagnostic.message, - related_information, - ..Default::default() - }) -} - fn overloaded_kind(overloaded: &Overloaded) -> SymbolKind { match overloaded { Overloaded::SubprogramDecl(_) => SymbolKind::FUNCTION, @@ -615,7 +500,7 @@ mod tests { use super::*; use crate::rpc_channel::test_support::*; - fn initialize_server(server: &mut VHDLServer, root_uri: Url) { + pub(crate) fn initialize_server(server: &mut VHDLServer, root_uri: Url) { let capabilities = ClientCapabilities::default(); #[allow(deprecated)] @@ -636,13 +521,13 @@ mod tests { server.initialized_notification(); } - fn temp_root_uri() -> (tempfile::TempDir, Url) { + pub(crate) fn temp_root_uri() -> (tempfile::TempDir, Url) { let tempdir = tempfile::tempdir().unwrap(); let root_uri = Url::from_file_path(tempdir.path().canonicalize().unwrap()).unwrap(); (tempdir, root_uri) } - fn expect_loaded_config_messages(mock: &RpcMock, config_uri: &Url) { + pub(crate) fn expect_loaded_config_messages(mock: &RpcMock, config_uri: &Url) { let file_name = config_uri .to_file_path() .unwrap() @@ -666,7 +551,7 @@ mod tests { } /// Create RpcMock and VHDLServer - fn setup_server() -> (Rc, VHDLServer) { + pub(crate) fn setup_server() -> (Rc, VHDLServer) { let mock = Rc::new(RpcMock::new()); let server = VHDLServer::new_external_config(SharedRpcChannel::new(mock.clone()), false); (mock, server) @@ -787,13 +672,17 @@ end entity ent; server.text_document_did_change_notification(&did_change); } - fn write_file(root_uri: &Url, file_name: impl AsRef, contents: impl AsRef) -> Url { + pub(crate) fn write_file( + root_uri: &Url, + file_name: impl AsRef, + contents: impl AsRef, + ) -> Url { let path = root_uri.to_file_path().unwrap().join(file_name.as_ref()); std::fs::write(&path, contents.as_ref()).unwrap(); Url::from_file_path(path).unwrap() } - fn write_config(root_uri: &Url, contents: impl AsRef) -> Url { + pub(crate) fn write_config(root_uri: &Url, contents: impl AsRef) -> Url { write_file(root_uri, "vhdl_ls.toml", contents) } @@ -1086,8 +975,8 @@ lib.files = [ mock.expect_notification("textDocument/publishDiagnostics", publish_diagnostics1); mock.expect_message_contains("Configuration file has changed, reloading project..."); expect_loaded_config_messages(&mock, &config_uri); - mock.expect_notification("textDocument/publishDiagnostics", publish_diagnostics2a); mock.expect_notification("textDocument/publishDiagnostics", publish_diagnostics2b); + mock.expect_notification("textDocument/publishDiagnostics", publish_diagnostics2a); initialize_server(&mut server, root_uri.clone()); diff --git a/vhdl_ls/src/vhdl_server/diagnostics.rs b/vhdl_ls/src/vhdl_server/diagnostics.rs new file mode 100644 index 00000000..7fbb059e --- /dev/null +++ b/vhdl_ls/src/vhdl_server/diagnostics.rs @@ -0,0 +1,235 @@ +use crate::vhdl_server::{file_name_to_uri, to_lsp_range, VHDLServer}; +use fnv::FnvHashMap; +use lsp_types::{ + DiagnosticRelatedInformation, DiagnosticSeverity, Location, NumberOrString, + PublishDiagnosticsParams, Url, +}; +use std::collections::hash_map::Entry; +use vhdl_lang::{Diagnostic, Severity, SeverityMap}; + +impl VHDLServer { + pub fn publish_diagnostics(&mut self) { + let diagnostics = self.project.analyse(); + + if self.settings.no_lint { + return; + } + + let supports_related_information = self.client_supports_related_information(); + let diagnostics = { + if supports_related_information { + diagnostics + } else { + flatten_related(diagnostics) + } + }; + + let mut by_uri = diagnostics_by_uri(diagnostics); + for (file_uri, cached_diagnostics) in self.diagnostic_cache.iter_mut() { + let Some(new_diagnostics) = by_uri.remove(file_uri) else { + // Diagnostics are in the cache, but not in the newly created diagnostics. + // This means that there are no longer any diagnostics in the given file. + // As a consequence, the client needs to be updated + let publish_diagnostics = PublishDiagnosticsParams { + uri: file_uri.clone(), + diagnostics: vec![], + version: None, + }; + self.rpc + .send_notification("textDocument/publishDiagnostics", publish_diagnostics); + cached_diagnostics.clear(); + continue; + }; + // Diagnostics are in the cache, but they are not equivalent to the old diagnostics. + // This means that we need to update the client, i.e., send a notification. + if &new_diagnostics != cached_diagnostics { + let lsp_diagnostics = new_diagnostics + .iter() + .filter_map(|diag| to_lsp_diagnostic(diag.clone(), &self.severity_map)) + .collect(); + let publish_diagnostics = PublishDiagnosticsParams { + uri: file_uri.clone(), + diagnostics: lsp_diagnostics, + version: None, + }; + self.rpc + .send_notification("textDocument/publishDiagnostics", publish_diagnostics); + } + // else: diagnostics are the same in the cache and the new analysis state. + // No need to update. + } + + // These are new diagnostics that weren't in the cache before. + for (file_uri, diagnostics) in by_uri.into_iter() { + let lsp_diagnostics = diagnostics + .iter() + .filter_map(|diag| to_lsp_diagnostic(diag.clone(), &self.severity_map)) + .collect(); + let publish_diagnostics = PublishDiagnosticsParams { + uri: file_uri.clone(), + diagnostics: lsp_diagnostics, + version: None, + }; + self.rpc + .send_notification("textDocument/publishDiagnostics", publish_diagnostics); + self.diagnostic_cache.insert(file_uri, diagnostics); + } + } +} + +fn diagnostics_by_uri(diagnostics: Vec) -> FnvHashMap> { + let mut map: FnvHashMap> = FnvHashMap::default(); + + for diagnostic in diagnostics { + let uri = file_name_to_uri(diagnostic.pos.source.file_name()); + match map.entry(uri) { + Entry::Occupied(mut entry) => entry.get_mut().push(diagnostic), + Entry::Vacant(entry) => { + let vec = vec![diagnostic]; + entry.insert(vec); + } + } + } + + map +} + +fn flatten_related(diagnostics: Vec) -> Vec { + let mut flat_diagnostics = Vec::new(); + for mut diagnostic in diagnostics { + flat_diagnostics.extend(diagnostic.drain_related()); + flat_diagnostics.push(diagnostic); + } + flat_diagnostics +} + +fn to_lsp_diagnostic( + diagnostic: Diagnostic, + severity_map: &SeverityMap, +) -> Option { + let severity = match severity_map[diagnostic.code]? { + Severity::Error => DiagnosticSeverity::ERROR, + Severity::Warning => DiagnosticSeverity::WARNING, + Severity::Info => DiagnosticSeverity::INFORMATION, + Severity::Hint => DiagnosticSeverity::HINT, + }; + + let related_information = if !diagnostic.related.is_empty() { + let mut related_information = Vec::new(); + for (pos, msg) in diagnostic.related { + let uri = file_name_to_uri(pos.source.file_name()); + related_information.push(DiagnosticRelatedInformation { + location: Location { + uri: uri.to_owned(), + range: to_lsp_range(pos.range()), + }, + message: msg, + }) + } + Some(related_information) + } else { + None + }; + + Some(lsp_types::Diagnostic { + range: to_lsp_range(diagnostic.pos.range()), + severity: Some(severity), + code: Some(NumberOrString::String(format!("{}", diagnostic.code))), + source: Some("vhdl ls".to_owned()), + message: diagnostic.message, + related_information, + ..Default::default() + }) +} + +#[cfg(test)] +pub mod tests { + use crate::vhdl_server::tests::{ + expect_loaded_config_messages, initialize_server, setup_server, temp_root_uri, + write_config, write_file, + }; + use lsp_types::{ + DiagnosticSeverity, DidChangeTextDocumentParams, NumberOrString, Position, + PublishDiagnosticsParams, Range, TextDocumentContentChangeEvent, + VersionedTextDocumentIdentifier, + }; + use regex::Regex; + + #[test] + fn only_send_diagnostics_once() { + let (mock, mut server) = setup_server(); + let (_tempdir, root_uri) = temp_root_uri(); + write_file( + &root_uri, + "file1.vhd", + "\ +architecture rtl of ent1 is +begin +end; +", + ); + let file2_uri = write_file( + &root_uri, + "file2.vhd", + "\ +architecture rtl of ent2 is +begin +end; +", + ); + let config_uri = write_config( + &root_uri, + " +[libraries] +lib.files = [ + 'file1.vhd', + 'file2.vhd' +] +", + ); + + let publish_diagnostics3 = PublishDiagnosticsParams { + uri: file2_uri.clone(), + diagnostics: vec![lsp_types::Diagnostic { + range: Range::new( + Position::new(0, "architecture rtl of ".len() as u32), + Position::new(0, "architecture rtl of ent3".len() as u32), + ), + code: Some(NumberOrString::String("unresolved".to_owned())), + severity: Some(DiagnosticSeverity::ERROR), + source: Some("vhdl ls".to_owned()), + message: "No primary unit \'ent3\' within library \'lib\'".to_owned(), + ..Default::default() + }], + version: None, + }; + + expect_loaded_config_messages(&mock, &config_uri); + // Initially, we get two reports for both files. + mock.expect_notification_contains_regex( + "textDocument/publishDiagnostics", + Regex::new(r#"No primary unit 'ent\d' within library 'lib'"#).unwrap(), + ); + mock.expect_notification_contains_regex( + "textDocument/publishDiagnostics", + Regex::new(r#"No primary unit 'ent\d' within library 'lib'"#).unwrap(), + ); + // Only expect one new notification after changing file2 + mock.expect_notification("textDocument/publishDiagnostics", publish_diagnostics3); + + initialize_server(&mut server, root_uri.clone()); + + // Change "ent2" to "ent3" + server.text_document_did_change_notification(&DidChangeTextDocumentParams { + text_document: VersionedTextDocumentIdentifier::new(file2_uri, 0), + content_changes: vec![TextDocumentContentChangeEvent { + range: Some(Range::new( + Position::new(0, "architecture rtl of ent".len() as u32), + Position::new(0, "architecture rtl of ent2".len() as u32), + )), + range_length: None, + text: "3".to_string(), + }], + }) + } +}