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(), + }], + }) + } +}