From 1cece36fa5d9e35406b22e8d6ed94bfc7fc5621a Mon Sep 17 00:00:00 2001 From: David Sherret Date: Wed, 19 Jan 2022 11:38:40 -0500 Subject: [PATCH] refactor(lsp): store the `LspUrlMap`'s state inside a mutex (#13416) --- cli/lsp/analysis.rs | 4 ++-- cli/lsp/code_lens.rs | 6 +++--- cli/lsp/language_server.rs | 32 +++++++++++++++++--------------- cli/lsp/tsc.rs | 20 ++++++++++---------- cli/lsp/urls.rs | 38 +++++++++++++++++++++++--------------- 5 files changed, 55 insertions(+), 45 deletions(-) diff --git a/cli/lsp/analysis.rs b/cli/lsp/analysis.rs index 182b90488b..bbc30043d4 100644 --- a/cli/lsp/analysis.rs +++ b/cli/lsp/analysis.rs @@ -341,7 +341,7 @@ fn is_preferred( /// for an LSP CodeAction. pub(crate) async fn ts_changes_to_edit( changes: &[tsc::FileTextChanges], - language_server: &mut language_server::Inner, + language_server: &language_server::Inner, ) -> Result, AnyError> { let mut text_document_edits = Vec::new(); for change in changes { @@ -607,7 +607,7 @@ impl CodeActionCollection { specifier: &ModuleSpecifier, action: &tsc::CodeFixAction, diagnostic: &lsp::Diagnostic, - language_server: &mut language_server::Inner, + language_server: &language_server::Inner, ) -> Result<(), AnyError> { if action.commands.is_some() { // In theory, tsc can return actions that require "commands" to be applied diff --git a/cli/lsp/code_lens.rs b/cli/lsp/code_lens.rs index 853a16f1f8..885e8cb1fd 100644 --- a/cli/lsp/code_lens.rs +++ b/cli/lsp/code_lens.rs @@ -239,7 +239,7 @@ impl Visit for DenoTestCollector { async fn resolve_implementation_code_lens( code_lens: lsp::CodeLens, data: CodeLensData, - language_server: &mut language_server::Inner, + language_server: &language_server::Inner, ) -> Result { let asset_or_doc = language_server.get_cached_asset_or_document(&data.specifier)?; @@ -308,7 +308,7 @@ async fn resolve_implementation_code_lens( async fn resolve_references_code_lens( code_lens: lsp::CodeLens, data: CodeLensData, - language_server: &mut language_server::Inner, + language_server: &language_server::Inner, ) -> Result { let asset_or_document = language_server.get_cached_asset_or_document(&data.specifier)?; @@ -378,7 +378,7 @@ async fn resolve_references_code_lens( pub(crate) async fn resolve_code_lens( code_lens: lsp::CodeLens, - language_server: &mut language_server::Inner, + language_server: &language_server::Inner, ) -> Result { let data: CodeLensData = serde_json::from_value(code_lens.data.clone().unwrap())?; diff --git a/cli/lsp/language_server.rs b/cli/lsp/language_server.rs index 95d2bbf710..c38b36c691 100644 --- a/cli/lsp/language_server.rs +++ b/cli/lsp/language_server.rs @@ -398,6 +398,8 @@ impl Inner { maybe_lint_config: self.maybe_lint_config.clone(), maybe_fmt_config: self.maybe_fmt_config.clone(), module_registries: self.module_registries.clone(), + // it's ok to get an Arc to the url map because the url_map is a cache + // that does not change once the mapping for a specifier is set url_map: self.url_map.clone(), })) } @@ -1147,7 +1149,7 @@ impl Inner { } async fn code_action( - &mut self, + &self, params: CodeActionParams, ) -> LspResult> { let specifier = self.url_map.normalize_url(¶ms.text_document.uri); @@ -1321,7 +1323,7 @@ impl Inner { } async fn code_action_resolve( - &mut self, + &self, params: CodeAction, ) -> LspResult { if params.kind.is_none() || params.data.is_none() { @@ -1460,7 +1462,7 @@ impl Inner { } async fn code_lens_resolve( - &mut self, + &self, code_lens: CodeLens, ) -> LspResult { let mark = self.performance.mark("code_lens_resolve", Some(&code_lens)); @@ -1481,7 +1483,7 @@ impl Inner { } async fn document_highlight( - &mut self, + &self, params: DocumentHighlightParams, ) -> LspResult>> { let specifier = self @@ -1526,7 +1528,7 @@ impl Inner { } async fn references( - &mut self, + &self, params: ReferenceParams, ) -> LspResult>> { let specifier = self @@ -1581,7 +1583,7 @@ impl Inner { } async fn goto_definition( - &mut self, + &self, params: GotoDefinitionParams, ) -> LspResult> { let specifier = self @@ -1620,7 +1622,7 @@ impl Inner { } async fn goto_type_definition( - &mut self, + &self, params: GotoTypeDefinitionParams, ) -> LspResult> { let specifier = self @@ -1668,7 +1670,7 @@ impl Inner { } async fn completion( - &mut self, + &self, params: CompletionParams, ) -> LspResult> { let specifier = self @@ -1746,7 +1748,7 @@ impl Inner { } async fn completion_resolve( - &mut self, + &self, params: CompletionItem, ) -> LspResult { let mark = self.performance.mark("completion_resolve", Some(¶ms)); @@ -1793,7 +1795,7 @@ impl Inner { } async fn goto_implementation( - &mut self, + &self, params: GotoImplementationParams, ) -> LspResult> { let specifier = self @@ -1841,7 +1843,7 @@ impl Inner { } async fn folding_range( - &mut self, + &self, params: FoldingRangeParams, ) -> LspResult>> { let specifier = self.url_map.normalize_url(¶ms.text_document.uri); @@ -1885,7 +1887,7 @@ impl Inner { } async fn incoming_calls( - &mut self, + &self, params: CallHierarchyIncomingCallsParams, ) -> LspResult>> { let specifier = self.url_map.normalize_url(¶ms.item.uri); @@ -1934,7 +1936,7 @@ impl Inner { } async fn outgoing_calls( - &mut self, + &self, params: CallHierarchyOutgoingCallsParams, ) -> LspResult>> { let specifier = self.url_map.normalize_url(¶ms.item.uri); @@ -1984,7 +1986,7 @@ impl Inner { } async fn prepare_call_hierarchy( - &mut self, + &self, params: CallHierarchyPrepareParams, ) -> LspResult>> { let specifier = self @@ -2058,7 +2060,7 @@ impl Inner { } async fn rename( - &mut self, + &self, params: RenameParams, ) -> LspResult> { let specifier = self diff --git a/cli/lsp/tsc.rs b/cli/lsp/tsc.rs index 91a1de900c..c61f7c2157 100644 --- a/cli/lsp/tsc.rs +++ b/cli/lsp/tsc.rs @@ -714,7 +714,7 @@ impl DocumentSpan { pub(crate) async fn to_link( &self, line_index: Arc, - language_server: &mut language_server::Inner, + language_server: &language_server::Inner, ) -> Option { let target_specifier = normalize_specifier(&self.file_name).ok()?; let target_asset_or_doc = language_server @@ -991,7 +991,7 @@ impl ImplementationLocation { pub(crate) fn to_location( &self, line_index: Arc, - language_server: &mut language_server::Inner, + language_server: &language_server::Inner, ) -> lsp::Location { let specifier = normalize_specifier(&self.document_span.file_name) .unwrap_or_else(|_| ModuleSpecifier::parse("deno://invalid").unwrap()); @@ -1008,7 +1008,7 @@ impl ImplementationLocation { pub(crate) async fn to_link( &self, line_index: Arc, - language_server: &mut language_server::Inner, + language_server: &language_server::Inner, ) -> Option { self .document_span @@ -1035,7 +1035,7 @@ impl RenameLocations { pub(crate) async fn into_workspace_edit( self, new_name: &str, - language_server: &mut language_server::Inner, + language_server: &language_server::Inner, ) -> Result { let mut text_document_edit_map: HashMap = HashMap::new(); @@ -1126,7 +1126,7 @@ impl DefinitionInfoAndBoundSpan { pub(crate) async fn to_definition( &self, line_index: Arc, - language_server: &mut language_server::Inner, + language_server: &language_server::Inner, ) -> Option { if let Some(definitions) = &self.definitions { let mut location_links = Vec::::new(); @@ -1528,7 +1528,7 @@ impl ReferenceEntry { pub(crate) fn to_location( &self, line_index: Arc, - language_server: &mut language_server::Inner, + language_server: &language_server::Inner, ) -> lsp::Location { let specifier = normalize_specifier(&self.document_span.file_name) .unwrap_or_else(|_| INVALID_SPECIFIER.clone()); @@ -1560,7 +1560,7 @@ pub struct CallHierarchyItem { impl CallHierarchyItem { pub(crate) async fn try_resolve_call_hierarchy_item( &self, - language_server: &mut language_server::Inner, + language_server: &language_server::Inner, maybe_root_path: Option<&Path>, ) -> Option { let target_specifier = normalize_specifier(&self.file).ok()?; @@ -1579,7 +1579,7 @@ impl CallHierarchyItem { pub(crate) fn to_call_hierarchy_item( &self, line_index: Arc, - language_server: &mut language_server::Inner, + language_server: &language_server::Inner, maybe_root_path: Option<&Path>, ) -> lsp::CallHierarchyItem { let target_specifier = normalize_specifier(&self.file) @@ -1661,7 +1661,7 @@ pub struct CallHierarchyIncomingCall { impl CallHierarchyIncomingCall { pub(crate) async fn try_resolve_call_hierarchy_incoming_call( &self, - language_server: &mut language_server::Inner, + language_server: &language_server::Inner, maybe_root_path: Option<&Path>, ) -> Option { let target_specifier = normalize_specifier(&self.from.file).ok()?; @@ -1696,7 +1696,7 @@ impl CallHierarchyOutgoingCall { pub(crate) async fn try_resolve_call_hierarchy_outgoing_call( &self, line_index: Arc, - language_server: &mut language_server::Inner, + language_server: &language_server::Inner, maybe_root_path: Option<&Path>, ) -> Option { let target_specifier = normalize_specifier(&self.to.file).ok()?; diff --git a/cli/lsp/urls.rs b/cli/lsp/urls.rs index 74fcc757ab..781fc8035b 100644 --- a/cli/lsp/urls.rs +++ b/cli/lsp/urls.rs @@ -6,11 +6,13 @@ use data_url::DataUrl; use deno_ast::MediaType; use deno_core::error::uri_error; use deno_core::error::AnyError; +use deno_core::parking_lot::Mutex; use deno_core::url::Position; use deno_core::url::Url; use deno_core::ModuleSpecifier; use once_cell::sync::Lazy; use std::collections::HashMap; +use std::sync::Arc; /// Used in situations where a default URL needs to be used where otherwise a /// panic is undesired. @@ -57,16 +59,13 @@ fn hash_data_specifier(specifier: &ModuleSpecifier) -> String { crate::checksum::gen(&[file_name_str.as_bytes()]) } -/// A bi-directional map of URLs sent to the LSP client and internal module -/// specifiers. We need to map internal specifiers into `deno:` schema URLs -/// to allow the Deno language server to manage these as virtual documents. -#[derive(Debug, Default, Clone)] -pub struct LspUrlMap { +#[derive(Debug, Default)] +struct LspUrlMapInner { specifier_to_url: HashMap, url_to_specifier: HashMap, } -impl LspUrlMap { +impl LspUrlMapInner { fn put(&mut self, specifier: ModuleSpecifier, url: Url) { self.specifier_to_url.insert(specifier.clone(), url.clone()); self.url_to_specifier.insert(url, specifier); @@ -79,15 +78,24 @@ impl LspUrlMap { fn get_specifier(&self, url: &Url) -> Option<&ModuleSpecifier> { self.url_to_specifier.get(url) } +} +/// A bi-directional map of URLs sent to the LSP client and internal module +/// specifiers. We need to map internal specifiers into `deno:` schema URLs +/// to allow the Deno language server to manage these as virtual documents. +#[derive(Debug, Default, Clone)] +pub struct LspUrlMap(Arc>); + +impl LspUrlMap { /// Normalize a specifier that is used internally within Deno (or tsc) to a /// URL that can be handled as a "virtual" document by an LSP client. pub fn normalize_specifier( - &mut self, + &self, specifier: &ModuleSpecifier, ) -> Result { - if let Some(url) = self.get_url(specifier) { - Ok(url.clone()) + let mut inner = self.0.lock(); + if let Some(url) = inner.get_url(specifier).cloned() { + Ok(url) } else { let url = if specifier.scheme() == "file" { specifier.clone() @@ -123,7 +131,7 @@ impl LspUrlMap { format!("deno:/{}", path) }; let url = Url::parse(&specifier_str)?; - self.put(specifier.clone(), url.clone()); + inner.put(specifier.clone(), url.clone()); url }; Ok(url) @@ -135,8 +143,8 @@ impl LspUrlMap { /// where the client encodes a file URL differently than Rust does by default /// causing issues with string matching of URLs. pub fn normalize_url(&self, url: &Url) -> ModuleSpecifier { - if let Some(specifier) = self.get_specifier(url) { - return specifier.clone(); + if let Some(specifier) = self.0.lock().get_specifier(url).cloned() { + return specifier; } if url.scheme() == "file" { if let Ok(path) = url.to_file_path() { @@ -164,7 +172,7 @@ mod tests { #[test] fn test_lsp_url_map() { - let mut map = LspUrlMap::default(); + let map = LspUrlMap::default(); let fixture = resolve_url("https://deno.land/x/pkg@1.0.0/mod.ts").unwrap(); let actual_url = map .normalize_specifier(&fixture) @@ -180,7 +188,7 @@ mod tests { #[test] fn test_lsp_url_map_complex_encoding() { // Test fix for #9741 - not properly encoding certain URLs - let mut map = LspUrlMap::default(); + let map = LspUrlMap::default(); let fixture = resolve_url("https://cdn.skypack.dev/-/postcss@v8.2.9-E4SktPp9c0AtxrJHp8iV/dist=es2020,mode=types/lib/postcss.d.ts").unwrap(); let actual_url = map .normalize_specifier(&fixture) @@ -194,7 +202,7 @@ mod tests { #[test] fn test_lsp_url_map_data() { - let mut map = LspUrlMap::default(); + let map = LspUrlMap::default(); let fixture = resolve_url("data:application/typescript;base64,ZXhwb3J0IGNvbnN0IGEgPSAiYSI7CgpleHBvcnQgZW51bSBBIHsKICBBLAogIEIsCiAgQywKfQo=").unwrap(); let actual_url = map .normalize_specifier(&fixture)