From 6125671efa33c96bb7c15d3327bb6a7aab0db431 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Tue, 28 Feb 2023 15:43:13 -0500 Subject: [PATCH 01/11] fix(lsp): add all documents to the lsp on initialize --- cli/lsp/documents.rs | 42 ++++++++++++++++++++++++++++++++++++++ cli/lsp/language_server.rs | 6 ++++++ 2 files changed, 48 insertions(+) diff --git a/cli/lsp/documents.rs b/cli/lsp/documents.rs index 0acfdbe1fe3c5a..46639cebe8a9d6 100644 --- a/cli/lsp/documents.rs +++ b/cli/lsp/documents.rs @@ -852,6 +852,48 @@ impl Documents { } } + pub fn open_root(&mut self, root_dir: &Path) { + let mut fs_docs = self.file_system_docs.lock(); + let resolver = self.resolver.as_graph_resolver(); + + let mut pending_dirs = VecDeque::new(); + pending_dirs.push_back(root_dir.to_path_buf()); + + while let Some(dir_path) = pending_dirs.pop_front() { + if let Ok(read_dir) = std::fs::read_dir(&dir_path) { + for entry in read_dir { + let entry = match entry { + Ok(entry) => entry, + Err(_) => continue, + }; + let file_type = match entry.file_type() { + Ok(file_type) => file_type, + Err(_) => continue, + }; + let new_path = dir_path.join(entry.file_name()); + if file_type.is_dir() { + pending_dirs.push_back(new_path); + } else if file_type.is_file() { + if let Some(ext) = new_path.extension() { + let ext = ext.to_string_lossy().to_lowercase(); + // todo: use media type here? + if matches!(ext.as_str(), "ts" | "js" | "mts" | "tsx") { + if let Ok(specifier) = ModuleSpecifier::from_file_path(new_path) + { + if !self.open_docs.contains_key(&specifier) + && !fs_docs.docs.contains_key(&specifier) + { + fs_docs.refresh_document(&self.cache, resolver, &specifier); + } + } + } + } + } + } + } + } + } + /// "Open" a document from the perspective of the editor, meaning that /// requests for information from the document will come from the in-memory /// representation received from the language server client, versus reading diff --git a/cli/lsp/language_server.rs b/cli/lsp/language_server.rs index ee91e045ba9c8e..1b0d4637ac32b0 100644 --- a/cli/lsp/language_server.rs +++ b/cli/lsp/language_server.rs @@ -995,6 +995,12 @@ impl Inner { self.assets.intitialize(self.snapshot()).await; + if let Some(root_uri) = &self.config.root_uri { + if let Ok(dir_path) = root_uri.to_file_path() { + self.documents.open_root(&dir_path); + } + } + self.performance.measure(mark); Ok(InitializeResult { capabilities, From f08a5bf6ecb6001005a19920cfe53c67fdf9012e Mon Sep 17 00:00:00 2001 From: David Sherret Date: Tue, 28 Feb 2023 16:04:59 -0500 Subject: [PATCH 02/11] Fix --- cli/lsp/documents.rs | 5 +++++ cli/lsp/tsc.rs | 6 +++--- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/cli/lsp/documents.rs b/cli/lsp/documents.rs index 46639cebe8a9d6..a4e1e0e3d381f0 100644 --- a/cli/lsp/documents.rs +++ b/cli/lsp/documents.rs @@ -853,6 +853,11 @@ impl Documents { } pub fn open_root(&mut self, root_dir: &Path) { + // todo(THIS PR): this is wrong. We should probably do this when + // updating the configuration and it should be based on the enabled + // paths rather than pulling in everything. Additionally, we should + // skip searching certain directories by default like node_modules + // and .git let mut fs_docs = self.file_system_docs.lock(); let resolver = self.resolver.as_graph_resolver(); diff --git a/cli/lsp/tsc.rs b/cli/lsp/tsc.rs index 2f66e2d2db6db0..9050b765a6a9f8 100644 --- a/cli/lsp/tsc.rs +++ b/cli/lsp/tsc.rs @@ -2767,16 +2767,16 @@ fn op_respond(state: &mut OpState, args: Response) -> bool { fn op_script_names(state: &mut OpState) -> Vec { let state = state.borrow_mut::(); let documents = &state.state_snapshot.documents; - let open_docs = documents.documents(true, true); + let all_docs = documents.documents(false, true); - let mut result = Vec::with_capacity(open_docs.len() + 1); + let mut result = Vec::with_capacity(all_docs.len() + 1); if documents.has_injected_types_node_package() { // ensure this is first so it resolves the node types first result.push("asset:///node_types.d.ts".to_string()); } - result.extend(open_docs.into_iter().map(|d| d.specifier().to_string())); + result.extend(all_docs.into_iter().map(|d| d.specifier().to_string())); result } From ef6475cb1b09081a682c76783f928124d461bedd Mon Sep 17 00:00:00 2001 From: David Sherret Date: Wed, 1 Mar 2023 11:10:53 -0500 Subject: [PATCH 03/11] Take into account enabled_paths --- cli/lsp/config.rs | 85 +++++++++++++++--- cli/lsp/documents.rs | 172 ++++++++++++++++++++++--------------- cli/lsp/language_server.rs | 43 ++++------ 3 files changed, 191 insertions(+), 109 deletions(-) diff --git a/cli/lsp/config.rs b/cli/lsp/config.rs index 3da9f7a0722c4c..e89db5c6be5a82 100644 --- a/cli/lsp/config.rs +++ b/cli/lsp/config.rs @@ -10,6 +10,7 @@ use deno_core::serde::Serialize; use deno_core::serde_json; use deno_core::serde_json::Value; use deno_core::ModuleSpecifier; +use lsp::Url; use std::collections::BTreeMap; use std::collections::HashMap; use std::sync::Arc; @@ -388,7 +389,7 @@ impl WorkspaceSettings { #[derive(Debug, Clone, Default)] pub struct ConfigSnapshot { pub client_capabilities: ClientCapabilities, - pub enabled_paths: HashMap>, + pub enabled_paths: HashMap>, pub settings: Settings, } @@ -396,12 +397,12 @@ impl ConfigSnapshot { /// Determine if the provided specifier is enabled or not. pub fn specifier_enabled(&self, specifier: &ModuleSpecifier) -> bool { if !self.enabled_paths.is_empty() { - let specifier_str = specifier.to_string(); + let specifier_str = specifier.as_str(); for (workspace, enabled_paths) in self.enabled_paths.iter() { - if specifier_str.starts_with(workspace) { + if specifier_str.starts_with(workspace.as_str()) { return enabled_paths .iter() - .any(|path| specifier_str.starts_with(path)); + .any(|path| specifier_str.starts_with(path.as_str())); } } } @@ -431,7 +432,7 @@ pub struct Settings { #[derive(Debug)] pub struct Config { pub client_capabilities: ClientCapabilities, - enabled_paths: HashMap>, + enabled_paths: HashMap>, pub root_uri: Option, settings: Settings, pub workspace_folders: Option>, @@ -478,12 +479,12 @@ impl Config { pub fn specifier_enabled(&self, specifier: &ModuleSpecifier) -> bool { if !self.enabled_paths.is_empty() { - let specifier_str = specifier.to_string(); + let specifier_str = specifier.as_str(); for (workspace, enabled_paths) in self.enabled_paths.iter() { - if specifier_str.starts_with(workspace) { + if specifier_str.starts_with(workspace.as_str()) { return enabled_paths .iter() - .any(|path| specifier_str.starts_with(path)); + .any(|path| specifier_str.starts_with(path.as_str())); } } } @@ -495,6 +496,24 @@ impl Config { .unwrap_or_else(|| self.settings.workspace.enable) } + pub fn root_dirs(&self) -> Vec { + let mut dirs: Vec = Vec::new(); + for (workspace, enabled_paths) in &self.enabled_paths { + if enabled_paths.is_empty() { + dirs.extend(enabled_paths.iter().cloned()); + } else { + dirs.push(workspace.clone()); + } + } + if dirs.is_empty() { + if let Some(root_dir) = &self.root_uri { + dirs.push(root_dir.clone()) + } + } + sort_and_remove_child_dirs(&mut dirs); + dirs + } + pub fn specifier_code_lens_test(&self, specifier: &ModuleSpecifier) -> bool { let value = self .settings @@ -583,7 +602,6 @@ impl Config { enabled_paths: Vec, ) -> bool { let workspace = ensure_directory_specifier(workspace); - let key = workspace.to_string(); let mut touched = false; if !enabled_paths.is_empty() { if let Ok(workspace_path) = specifier_to_file_path(&workspace) { @@ -592,7 +610,7 @@ impl Config { let fs_path = workspace_path.join(path); match ModuleSpecifier::from_file_path(fs_path) { Ok(path_uri) => { - paths.push(path_uri.to_string()); + paths.push(path_uri); } Err(_) => { lsp_log!("Unable to resolve a file path for `deno.enablePath` from \"{}\" for workspace \"{}\".", path, workspace); @@ -601,12 +619,12 @@ impl Config { } if !paths.is_empty() { touched = true; - self.enabled_paths.insert(key, paths); + self.enabled_paths.insert(workspace.clone(), paths); } } } else { touched = true; - self.enabled_paths.remove(&key); + self.enabled_paths.remove(&workspace); } touched } @@ -636,6 +654,20 @@ impl Config { } } +fn sort_and_remove_child_dirs(dirs: &mut Vec) { + if dirs.is_empty() { + return; + } + + dirs.sort(); + for i in (0..dirs.len() - 1).rev() { + let prev = &dirs[i + 1]; + if prev.as_str().starts_with(dirs[i].as_str()) { + dirs.remove(i + 1); + } + } +} + #[cfg(test)] mod tests { use super::*; @@ -678,8 +710,8 @@ mod tests { assert!(!config.specifier_enabled(&specifier_b)); let mut enabled_paths = HashMap::new(); enabled_paths.insert( - "file:///project/".to_string(), - vec!["file:///project/worker/".to_string()], + Url::parse("file:///project/").unwrap(), + vec![Url::parse("file:///project/worker/").unwrap()], ); config.enabled_paths = enabled_paths; assert!(config.specifier_enabled(&specifier_a)); @@ -800,4 +832,29 @@ mod tests { WorkspaceSettings::default() ); } + + #[test] + fn test_sort_and_remove_child_dirs() { + fn run_test(dirs: Vec<&str>, expected_output: Vec<&str>) { + let mut dirs = dirs + .into_iter() + .map(|dir| Url::parse(dir).unwrap()) + .collect(); + sort_and_remove_child_dirs(&mut dirs); + let dirs: Vec<_> = dirs.iter().map(|dir| dir.as_str()).collect(); + assert_eq!(dirs, expected_output); + } + + run_test( + vec![ + "file:///test/asdf/test/asdf/", + "file:///test/asdf/", + "file:///test/asdf/", + "file:///testing/456/893/", + "file:///testing/456/893/test/", + ], + vec!["file:///test/asdf/", "file:///testing/456/893/"], + ); + run_test(vec![], vec![]); + } } diff --git a/cli/lsp/documents.rs b/cli/lsp/documents.rs index a4e1e0e3d381f0..7952d2774f2fad 100644 --- a/cli/lsp/documents.rs +++ b/cli/lsp/documents.rs @@ -43,6 +43,7 @@ use deno_runtime::deno_node::NodeResolutionMode; use deno_runtime::deno_node::PackageJson; use deno_runtime::permissions::PermissionsContainer; use indexmap::IndexMap; +use lsp::Url; use once_cell::sync::Lazy; use std::collections::BTreeMap; use std::collections::HashMap; @@ -775,18 +776,6 @@ impl FileSystemDocuments { self.docs.insert(specifier.clone(), doc.clone()); Some(doc) } - - pub fn refresh_dependencies( - &mut self, - resolver: &dyn deno_graph::source::Resolver, - ) { - for doc in self.docs.values_mut() { - if let Some(new_doc) = doc.maybe_with_new_resolver(resolver) { - *doc = new_doc; - } - } - self.dirty = true; - } } fn get_document_path( @@ -852,53 +841,6 @@ impl Documents { } } - pub fn open_root(&mut self, root_dir: &Path) { - // todo(THIS PR): this is wrong. We should probably do this when - // updating the configuration and it should be based on the enabled - // paths rather than pulling in everything. Additionally, we should - // skip searching certain directories by default like node_modules - // and .git - let mut fs_docs = self.file_system_docs.lock(); - let resolver = self.resolver.as_graph_resolver(); - - let mut pending_dirs = VecDeque::new(); - pending_dirs.push_back(root_dir.to_path_buf()); - - while let Some(dir_path) = pending_dirs.pop_front() { - if let Ok(read_dir) = std::fs::read_dir(&dir_path) { - for entry in read_dir { - let entry = match entry { - Ok(entry) => entry, - Err(_) => continue, - }; - let file_type = match entry.file_type() { - Ok(file_type) => file_type, - Err(_) => continue, - }; - let new_path = dir_path.join(entry.file_name()); - if file_type.is_dir() { - pending_dirs.push_back(new_path); - } else if file_type.is_file() { - if let Some(ext) = new_path.extension() { - let ext = ext.to_string_lossy().to_lowercase(); - // todo: use media type here? - if matches!(ext.as_str(), "ts" | "js" | "mts" | "tsx") { - if let Ok(specifier) = ModuleSpecifier::from_file_path(new_path) - { - if !self.open_docs.contains_key(&specifier) - && !fs_docs.docs.contains_key(&specifier) - { - fs_docs.refresh_document(&self.cache, resolver, &specifier); - } - } - } - } - } - } - } - } - } - /// "Open" a document from the perspective of the editor, meaning that /// requests for information from the document will come from the in-memory /// representation received from the language server client, versus reading @@ -1209,6 +1151,7 @@ impl Documents { pub fn update_config( &mut self, + root_dirs: Vec, maybe_import_map: Option>, maybe_config_file: Option<&ConfigFile>, maybe_package_json: Option<&PackageJson>, @@ -1216,21 +1159,24 @@ impl Documents { npm_resolution: NpmResolution, ) { fn calculate_resolver_config_hash( + root_dirs: &[Url], maybe_import_map: Option<&import_map::ImportMap>, maybe_jsx_config: Option<&JsxImportSourceConfig>, maybe_package_json_deps: Option<&BTreeMap>, ) -> u64 { let mut hasher = FastInsecureHasher::default(); + hasher.write_hashable(&{ + // ensure these are sorted + let mut root_dirs = root_dirs.to_vec(); + root_dirs.sort(); + root_dirs + }); if let Some(import_map) = maybe_import_map { hasher.write_str(&import_map.to_json()); hasher.write_str(import_map.base_url().as_str()); } - if let Some(jsx_config) = maybe_jsx_config { - hasher.write_hashable(&jsx_config); - } - if let Some(deps) = maybe_package_json_deps { - hasher.write_hashable(&deps); - } + hasher.write_hashable(&maybe_jsx_config); + hasher.write_hashable(&maybe_package_json_deps); hasher.finish() } @@ -1246,6 +1192,7 @@ impl Documents { let maybe_jsx_config = maybe_config_file.and_then(|cf| cf.to_maybe_jsx_import_source_config()); let new_resolver_config_hash = calculate_resolver_config_hash( + &root_dirs, maybe_import_map.as_deref(), maybe_jsx_config.as_ref(), maybe_package_json_deps.as_ref(), @@ -1295,21 +1242,108 @@ impl Documents { // only refresh the dependencies if the underlying configuration has changed if self.resolver_config_hash != new_resolver_config_hash { - self.refresh_dependencies(); + self.refresh_dependencies(root_dirs); self.resolver_config_hash = new_resolver_config_hash; } self.dirty = true; } - fn refresh_dependencies(&mut self) { + fn refresh_dependencies(&mut self, root_dirs: Vec) { let resolver = self.resolver.as_graph_resolver(); for doc in self.open_docs.values_mut() { if let Some(new_doc) = doc.maybe_with_new_resolver(resolver) { *doc = new_doc; } } - self.file_system_docs.lock().refresh_dependencies(resolver); + + // update the file system document + let mut fs_docs = self.file_system_docs.lock(); + let mut not_found_docs = + fs_docs.docs.keys().cloned().collect::>(); + + let mut pending_dirs = VecDeque::new(); + for dir in root_dirs { + if let Ok(path) = dir.to_file_path() { + pending_dirs.push_back(path.to_path_buf()); + } + } + + while let Some(dir_path) = pending_dirs.pop_front() { + if let Some(dir_name) = dir_path.file_name() { + let dir_name = dir_name.to_string_lossy().to_lowercase(); + if matches!(dir_name.as_str(), "node_modules" | ".git") { + continue; + } + } + let read_dir = match std::fs::read_dir(&dir_path) { + Ok(entry) => entry, + Err(_) => continue, + }; + + for entry in read_dir { + let entry = match entry { + Ok(entry) => entry, + Err(_) => continue, + }; + let file_type = match entry.file_type() { + Ok(file_type) => file_type, + Err(_) => continue, + }; + let new_path = dir_path.join(entry.file_name()); + if file_type.is_dir() { + pending_dirs.push_back(new_path); + } else if file_type.is_file() { + let media_type: MediaType = (&new_path).into(); + let is_diagnosable = match media_type { + MediaType::JavaScript + | MediaType::Jsx + | MediaType::Mjs + | MediaType::Cjs + | MediaType::TypeScript + | MediaType::Mts + | MediaType::Cts + | MediaType::Dts + | MediaType::Dmts + | MediaType::Dcts + | MediaType::Tsx => true, + MediaType::Json + | MediaType::Wasm + | MediaType::SourceMap + | MediaType::TsBuildInfo + | MediaType::Unknown => false, + }; + if !is_diagnosable { + continue; + } + let specifier = match ModuleSpecifier::from_file_path(new_path) { + Ok(specifier) => specifier, + Err(_) => continue, + }; + + if !self.open_docs.contains_key(&specifier) + && !fs_docs.docs.contains_key(&specifier) + { + fs_docs.refresh_document(&self.cache, resolver, &specifier); + } else { + not_found_docs.remove(&specifier); + // update the existing entry to have the new resolver + if let Some(doc) = fs_docs.docs.get_mut(&specifier) { + if let Some(new_doc) = doc.maybe_with_new_resolver(resolver) { + *doc = new_doc; + } + } + } + } + } + } + + // clean up and remove any documents that weren't found + for uri in not_found_docs { + fs_docs.docs.remove(&uri); + } + + fs_docs.dirty = true; } /// Iterate through the documents, building a map where the key is a unique @@ -1681,6 +1715,7 @@ console.log(b, "hello deno"); .unwrap(); documents.update_config( + vec![], Some(Arc::new(import_map)), None, None, @@ -1720,6 +1755,7 @@ console.log(b, "hello deno"); .unwrap(); documents.update_config( + vec![], Some(Arc::new(import_map)), None, None, diff --git a/cli/lsp/language_server.rs b/cli/lsp/language_server.rs index 1b0d4637ac32b0..5a4663b6ce4bdd 100644 --- a/cli/lsp/language_server.rs +++ b/cli/lsp/language_server.rs @@ -985,22 +985,10 @@ impl Inner { if let Err(err) = self.update_registries().await { self.client.show_message(MessageType::WARNING, err).await; } - self.documents.update_config( - self.maybe_import_map.clone(), - self.maybe_config_file.as_ref(), - self.maybe_package_json.as_ref(), - self.npm_resolver.api().clone(), - self.npm_resolver.resolution().clone(), - ); + self.refresh_config(); self.assets.intitialize(self.snapshot()).await; - if let Some(root_uri) = &self.config.root_uri { - if let Ok(dir_path) = root_uri.to_file_path() { - self.documents.open_root(&dir_path); - } - } - self.performance.measure(mark); Ok(InitializeResult { capabilities, @@ -1009,6 +997,17 @@ impl Inner { }) } + fn refresh_config(&mut self) { + self.documents.update_config( + self.config.root_dirs(), + self.maybe_import_map.clone(), + self.maybe_config_file.as_ref(), + self.maybe_package_json.as_ref(), + self.npm_resolver.api().clone(), + self.npm_resolver.resolution().clone(), + ); + } + async fn initialized(&mut self, _: InitializedParams) { if self .config @@ -1039,6 +1038,7 @@ impl Inner { } } self.config.update_enabled_paths(self.client.clone()).await; + self.refresh_config(); if self.config.client_capabilities.testing_api { let test_server = testing::TestServer::new( @@ -1184,13 +1184,7 @@ impl Inner { self.client.show_message(MessageType::WARNING, err).await; } - self.documents.update_config( - self.maybe_import_map.clone(), - self.maybe_config_file.as_ref(), - self.maybe_package_json.as_ref(), - self.npm_resolver.api().clone(), - self.npm_resolver.resolution().clone(), - ); + self.refresh_config(); self.send_diagnostics_update(); self.send_testing_update(); @@ -1242,13 +1236,7 @@ impl Inner { } } if touched { - self.documents.update_config( - self.maybe_import_map.clone(), - self.maybe_config_file.as_ref(), - self.maybe_package_json.as_ref(), - self.npm_resolver.api().clone(), - self.npm_resolver.resolution().clone(), - ); + self.refresh_config(); self.refresh_npm_specifiers().await; self.diagnostics_server.invalidate_all(); self.restart_ts_server().await; @@ -2826,6 +2814,7 @@ impl tower_lsp::LanguageServer for LanguageServer { } let mut ls = language_server.0.write().await; if ls.config.update_enabled_paths(client).await { + ls.refresh_config(); ls.diagnostics_server.invalidate_all(); // this will be called in the inner did_change_configuration, but the // problem then becomes, if there was a change, the snapshot used From 17b1f0bb0e634456c89380fca69cb256175e9708 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Wed, 1 Mar 2023 11:17:15 -0500 Subject: [PATCH 04/11] Another one. --- cli/lsp/language_server.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/cli/lsp/language_server.rs b/cli/lsp/language_server.rs index 5a4663b6ce4bdd..375cced2322b73 100644 --- a/cli/lsp/language_server.rs +++ b/cli/lsp/language_server.rs @@ -2873,6 +2873,7 @@ impl tower_lsp::LanguageServer for LanguageServer { tokio::spawn(async move { let mut ls = language_server.0.write().await; if ls.config.update_enabled_paths(client).await { + ls.refresh_config(); ls.diagnostics_server.invalidate_all(); ls.send_diagnostics_update(); } From 67439dd8340d35fe37681de06e580884f65071d2 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Fri, 3 Mar 2023 20:50:21 -0500 Subject: [PATCH 05/11] More work. --- cli/lsp/config.rs | 61 ++++----- cli/lsp/diagnostics.rs | 13 +- cli/lsp/language_server.rs | 246 ++++++++++++++++++++----------------- cli/lsp/urls.rs | 21 ++-- 4 files changed, 175 insertions(+), 166 deletions(-) diff --git a/cli/lsp/config.rs b/cli/lsp/config.rs index e89db5c6be5a82..72588c4da83b74 100644 --- a/cli/lsp/config.rs +++ b/cli/lsp/config.rs @@ -1,6 +1,5 @@ // Copyright 2018-2023 the Deno authors. All rights reserved. MIT license. -use super::client::Client; use super::logging::lsp_log; use crate::util::path::ensure_directory_specifier; use crate::util::path::specifier_to_file_path; @@ -227,7 +226,7 @@ impl Default for ImportCompletionSettings { /// Deno language server specific settings that can be applied uniquely to a /// specifier. -#[derive(Debug, Default, Clone, Deserialize)] +#[derive(Debug, Default, Clone, Deserialize, PartialEq, Eq)] #[serde(rename_all = "camelCase")] pub struct SpecifierSettings { /// A flag that indicates if Deno is enabled for this specifier or not. @@ -406,26 +405,17 @@ impl ConfigSnapshot { } } } - if let Some((_, SpecifierSettings { enable, .. })) = - self.settings.specifiers.get(specifier) - { - *enable + if let Some(settings) = self.settings.specifiers.get(specifier) { + settings.enable } else { self.settings.workspace.enable } } } -#[derive(Debug, Clone)] -pub struct SpecifierWithClientUri { - pub specifier: ModuleSpecifier, - pub client_uri: ModuleSpecifier, -} - #[derive(Debug, Default, Clone)] pub struct Settings { - pub specifiers: - BTreeMap, + pub specifiers: BTreeMap, pub workspace: WorkspaceSettings, } @@ -492,7 +482,7 @@ impl Config { .settings .specifiers .get(specifier) - .map(|(_, s)| s.enable) + .map(|settings| settings.enable) .unwrap_or_else(|| self.settings.workspace.enable) } @@ -519,7 +509,7 @@ impl Config { .settings .specifiers .get(specifier) - .map(|(_, s)| s.code_lens.test) + .map(|settings| settings.code_lens.test) .unwrap_or_else(|| self.settings.workspace.code_lens.test); value } @@ -573,13 +563,15 @@ impl Config { /// Given the configured workspaces or root URI and the their settings, /// update and resolve any paths that should be enabled - pub async fn update_enabled_paths(&mut self, client: Client) -> bool { + pub fn update_enabled_paths(&mut self) -> bool { if let Some(workspace_folders) = self.workspace_folders.clone() { let mut touched = false; - for (workspace, folder) in workspace_folders { - if let Ok(settings) = client.specifier_configuration(&folder.uri).await - { - if self.update_enabled_paths_entry(workspace, settings.enable_paths) { + for (workspace, _) in workspace_folders { + if let Some(settings) = self.settings.specifiers.get(&workspace) { + if self.update_enabled_paths_entry( + workspace, + settings.enable_paths.clone(), + ) { touched = true; } } @@ -629,28 +621,23 @@ impl Config { touched } - pub fn get_specifiers_with_client_uris(&self) -> Vec { - self - .settings - .specifiers - .iter() - .map(|(s, (u, _))| SpecifierWithClientUri { - specifier: s.clone(), - client_uri: u.clone(), - }) - .collect() + pub fn get_specifiers(&self) -> Vec { + self.settings.specifiers.keys().cloned().collect() } pub fn set_specifier_settings( &mut self, specifier: ModuleSpecifier, - client_uri: ModuleSpecifier, settings: SpecifierSettings, - ) { - self - .settings - .specifiers - .insert(specifier, (client_uri, settings)); + ) -> bool { + if let Some(existing) = self.settings.specifiers.get(&specifier) { + if *existing == settings { + return false; + } + } + + self.settings.specifiers.insert(specifier, settings); + true } } diff --git a/cli/lsp/diagnostics.rs b/cli/lsp/diagnostics.rs index f938ba6f450967..3e59fefc224d96 100644 --- a/cli/lsp/diagnostics.rs +++ b/cli/lsp/diagnostics.rs @@ -1179,14 +1179,11 @@ let c: number = "a"; let mut disabled_config = mock_config(); disabled_config.settings.specifiers.insert( specifier.clone(), - ( - specifier.clone(), - SpecifierSettings { - enable: false, - enable_paths: Vec::new(), - code_lens: Default::default(), - }, - ), + SpecifierSettings { + enable: false, + enable_paths: Vec::new(), + code_lens: Default::default(), + }, ); let diagnostics = generate_lint_diagnostics( diff --git a/cli/lsp/language_server.rs b/cli/lsp/language_server.rs index 375cced2322b73..703243a904bdcc 100644 --- a/cli/lsp/language_server.rs +++ b/cli/lsp/language_server.rs @@ -316,6 +316,89 @@ impl LanguageServer { None => Err(LspError::invalid_params("Missing parameters")), } } + + pub async fn refresh_specifiers_from_client(&self) { + let (client, specifiers) = + { + let ls = self.0.read().await; + let specifiers = + if ls.config.client_capabilities.workspace_configuration { + let root_capacity = match &ls.config.workspace_folders { + Some(folder) => folder.len(), + None => 1, + }; + let config_specifiers = ls.config.get_specifiers(); + let mut specifiers = + HashMap::with_capacity(root_capacity + config_specifiers.len()); + match &ls.config.workspace_folders { + Some(entry) => { + for (specifier, folder) in entry { + specifiers.insert(specifier.clone(), folder.uri.clone()); + } + } + None => { + if let Some(root_uri) = &ls.config.root_uri { + specifiers.insert( + root_uri.clone(), + ls.url_map.normalize_specifier(root_uri).unwrap(), + ); + } + } + } + specifiers.extend(ls.config.get_specifiers().iter().map(|s| { + (s.clone(), ls.url_map.normalize_specifier(s).unwrap()) + })); + + Some(specifiers.into_iter().collect::>()) + } else { + None + }; + + (ls.client.clone(), specifiers) + }; + + let mut touched = false; + if let Some(specifiers) = specifiers { + let configs_result = client + .specifier_configurations( + specifiers + .iter() + .map(|(_, client_uri)| client_uri.clone()) + .collect(), + ) + .await; + + let mut ls = self.0.write().await; + if let Ok(configs) = configs_result { + for (i, value) in configs.into_iter().enumerate() { + match value { + Ok(specifier_settings) => { + let internal_uri = specifiers[i].1.clone(); + if ls + .config + .set_specifier_settings(internal_uri, specifier_settings) + { + touched = true; + } + } + Err(err) => { + error!("{}", err); + } + } + } + } + + if ls.config.update_enabled_paths() { + touched = true; + } + + if touched { + ls.refresh_documents_config(); + ls.diagnostics_server.invalidate_all(); + ls.send_diagnostics_update(); + } + } + } } fn create_lsp_npm_resolver( @@ -986,7 +1069,7 @@ impl Inner { self.client.show_message(MessageType::WARNING, err).await; } - self.refresh_config(); + // self.refresh_documents_config(); // todo(THIS PR): REMOVE self.assets.intitialize(self.snapshot()).await; self.performance.measure(mark); @@ -997,7 +1080,7 @@ impl Inner { }) } - fn refresh_config(&mut self) { + fn refresh_documents_config(&mut self) { self.documents.update_config( self.config.root_dirs(), self.maybe_import_map.clone(), @@ -1037,8 +1120,6 @@ impl Inner { warn!("Client errored on capabilities.\n{:#}", err); } } - self.config.update_enabled_paths(self.client.clone()).await; - self.refresh_config(); if self.config.client_capabilities.testing_api { let test_server = testing::TestServer::new( @@ -1048,8 +1129,6 @@ impl Inner { ); self.maybe_testing_server = Some(test_server); } - - lsp_log!("Server ready."); } async fn shutdown(&self) -> LspResult<()> { @@ -1184,7 +1263,7 @@ impl Inner { self.client.show_message(MessageType::WARNING, err).await; } - self.refresh_config(); + self.refresh_documents_config(); self.send_diagnostics_update(); self.send_testing_update(); @@ -1236,7 +1315,7 @@ impl Inner { } } if touched { - self.refresh_config(); + self.refresh_documents_config(); self.refresh_npm_specifiers().await; self.diagnostics_server.invalidate_all(); self.restart_ts_server().await; @@ -1246,13 +1325,10 @@ impl Inner { self.performance.measure(mark); } - async fn did_change_workspace_folders( + fn did_change_workspace_folders( &mut self, params: DidChangeWorkspaceFoldersParams, ) { - let mark = self - .performance - .mark("did_change_workspace_folders", Some(¶ms)); let mut workspace_folders = params .event .added @@ -1271,7 +1347,6 @@ impl Inner { } self.config.workspace_folders = Some(workspace_folders); - self.performance.measure(mark); } async fn document_symbol( @@ -2677,7 +2752,11 @@ impl tower_lsp::LanguageServer for LanguageServer { } async fn initialized(&self, params: InitializedParams) { - self.0.write().await.initialized(params).await + self.0.write().await.initialized(params).await; + + self.refresh_specifiers_from_client().await; + + lsp_log!("Server ready."); } async fn shutdown(&self) -> LspResult<()> { @@ -2692,11 +2771,11 @@ impl tower_lsp::LanguageServer for LanguageServer { return; } - let (client, uri, specifier, had_specifier_settings) = { + let (client, client_uri, specifier, had_specifier_settings) = { let mut inner = self.0.write().await; let client = inner.client.clone(); - let uri = params.text_document.uri.clone(); - let specifier = inner.url_map.normalize_url(&uri); + let client_uri = params.text_document.uri.clone(); + let specifier = inner.url_map.normalize_url(&client_uri); let document = inner.did_open(&specifier, params).await; let has_specifier_settings = inner.config.has_specifier_settings(&specifier); @@ -2710,39 +2789,34 @@ impl tower_lsp::LanguageServer for LanguageServer { inner.send_testing_update(); } } - (client, uri, specifier, has_specifier_settings) + (client, client_uri, specifier, has_specifier_settings) }; // retrieve the specifier settings outside the lock if - // they haven't been asked for yet on its own time + // they haven't been asked for yet if !had_specifier_settings { - let language_server = self.clone(); - tokio::spawn(async move { - let response = client.specifier_configuration(&uri).await; - let mut inner = language_server.0.write().await; - match response { - Ok(specifier_settings) => { - // now update the config and send a diagnostics update - inner.config.set_specifier_settings( - specifier.clone(), - uri, - specifier_settings, - ); - } - Err(err) => { - error!("{}", err); - } + let response = client.specifier_configuration(&client_uri).await; + let mut ls = self.0.write().await; + match response { + Ok(specifier_settings) => { + ls.config + .set_specifier_settings(specifier.clone(), specifier_settings); + ls.config.update_enabled_paths(); } - if inner - .documents - .get(&specifier) - .map(|d| d.is_diagnosable()) - .unwrap_or(false) - { - inner.send_diagnostics_update(); - inner.send_testing_update(); + Err(err) => { + error!("{}", err); } - }); + } + + if ls + .documents + .get(&specifier) + .map(|d| d.is_diagnosable()) + .unwrap_or(false) + { + ls.refresh_documents_config(); + ls.send_diagnostics_update(); + } } } @@ -2763,67 +2837,18 @@ impl tower_lsp::LanguageServer for LanguageServer { &self, params: DidChangeConfigurationParams, ) { - let (has_workspace_capability, client, specifiers, mark) = { - let inner = self.0.write().await; - let mark = inner - .performance - .mark("did_change_configuration", Some(¶ms)); - - let specifiers = - if inner.config.client_capabilities.workspace_configuration { - Some(inner.config.get_specifiers_with_client_uris()) - } else { - None - }; + let (mark, has_workspace_capability, client) = { + let inner = self.0.read().await; ( + inner + .performance + .mark("did_change_configuration", Some(¶ms)), inner.config.client_capabilities.workspace_configuration, inner.client.clone(), - specifiers, - mark, ) }; - // start retrieving all the specifiers' settings outside the lock on its own - // time - if let Some(specifiers) = specifiers { - let language_server = self.clone(); - let client = client.clone(); - tokio::spawn(async move { - if let Ok(configs) = client - .specifier_configurations( - specifiers.iter().map(|s| s.client_uri.clone()).collect(), - ) - .await - { - let mut inner = language_server.0.write().await; - for (i, value) in configs.into_iter().enumerate() { - match value { - Ok(specifier_settings) => { - let entry = specifiers[i].clone(); - inner.config.set_specifier_settings( - entry.specifier, - entry.client_uri, - specifier_settings, - ); - } - Err(err) => { - error!("{}", err); - } - } - } - } - let mut ls = language_server.0.write().await; - if ls.config.update_enabled_paths(client).await { - ls.refresh_config(); - ls.diagnostics_server.invalidate_all(); - // this will be called in the inner did_change_configuration, but the - // problem then becomes, if there was a change, the snapshot used - // will be an out of date one, so we will call it again here if the - // workspace folders have been touched - ls.send_diagnostics_update(); - } - }); - } + self.refresh_specifiers_from_client().await; // Get the configuration from the client outside of the lock // in order to prevent potential deadlocking scenarios where @@ -2864,20 +2889,17 @@ impl tower_lsp::LanguageServer for LanguageServer { &self, params: DidChangeWorkspaceFoldersParams, ) { - let client = { - let mut inner = self.0.write().await; - inner.did_change_workspace_folders(params).await; - inner.client.clone() + let mark = { + let mut ls = self.0.write().await; + let mark = ls + .performance + .mark("did_change_workspace_folders", Some(¶ms)); + ls.did_change_workspace_folders(params); + mark }; - let language_server = self.clone(); - tokio::spawn(async move { - let mut ls = language_server.0.write().await; - if ls.config.update_enabled_paths(client).await { - ls.refresh_config(); - ls.diagnostics_server.invalidate_all(); - ls.send_diagnostics_update(); - } - }); + + self.refresh_specifiers_from_client().await; + self.0.read().await.performance.measure(mark); } async fn document_symbol( diff --git a/cli/lsp/urls.rs b/cli/lsp/urls.rs index 4fba0c9ff3a373..2168dfed413e52 100644 --- a/cli/lsp/urls.rs +++ b/cli/lsp/urls.rs @@ -81,7 +81,7 @@ impl LspUrlMapInner { } /// 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 +/// 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>); @@ -143,15 +143,18 @@ 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.0.lock().get_specifier(url).cloned() { - return specifier; - } - if url.scheme() == "file" { - if let Ok(path) = url.to_file_path() { - return Url::from_file_path(path).unwrap(); - } + let mut inner = self.0.lock(); + if let Some(specifier) = inner.get_specifier(url).cloned() { + specifier + } else { + let specifier = if let Ok(path) = url.to_file_path() { + Url::from_file_path(path).unwrap() + } else { + url.clone() + }; + inner.put(specifier.clone(), url.clone()); + specifier } - url.clone() } } From 8c36fd7b5ffcc0bd2e8d2912260b65ca3cd75817 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Sat, 11 Mar 2023 15:52:09 -0500 Subject: [PATCH 06/11] Commit this code for backup purposes --- cli/lsp/config.rs | 57 ------------------------------ cli/lsp/documents.rs | 2 +- cli/lsp/language_server.rs | 15 ++++---- cli/tests/integration/lsp_tests.rs | 2 +- test_util/src/lsp.rs | 9 +++++ 5 files changed, 20 insertions(+), 65 deletions(-) diff --git a/cli/lsp/config.rs b/cli/lsp/config.rs index 72588c4da83b74..44eec8c3560aaf 100644 --- a/cli/lsp/config.rs +++ b/cli/lsp/config.rs @@ -486,24 +486,6 @@ impl Config { .unwrap_or_else(|| self.settings.workspace.enable) } - pub fn root_dirs(&self) -> Vec { - let mut dirs: Vec = Vec::new(); - for (workspace, enabled_paths) in &self.enabled_paths { - if enabled_paths.is_empty() { - dirs.extend(enabled_paths.iter().cloned()); - } else { - dirs.push(workspace.clone()); - } - } - if dirs.is_empty() { - if let Some(root_dir) = &self.root_uri { - dirs.push(root_dir.clone()) - } - } - sort_and_remove_child_dirs(&mut dirs); - dirs - } - pub fn specifier_code_lens_test(&self, specifier: &ModuleSpecifier) -> bool { let value = self .settings @@ -641,20 +623,6 @@ impl Config { } } -fn sort_and_remove_child_dirs(dirs: &mut Vec) { - if dirs.is_empty() { - return; - } - - dirs.sort(); - for i in (0..dirs.len() - 1).rev() { - let prev = &dirs[i + 1]; - if prev.as_str().starts_with(dirs[i].as_str()) { - dirs.remove(i + 1); - } - } -} - #[cfg(test)] mod tests { use super::*; @@ -819,29 +787,4 @@ mod tests { WorkspaceSettings::default() ); } - - #[test] - fn test_sort_and_remove_child_dirs() { - fn run_test(dirs: Vec<&str>, expected_output: Vec<&str>) { - let mut dirs = dirs - .into_iter() - .map(|dir| Url::parse(dir).unwrap()) - .collect(); - sort_and_remove_child_dirs(&mut dirs); - let dirs: Vec<_> = dirs.iter().map(|dir| dir.as_str()).collect(); - assert_eq!(dirs, expected_output); - } - - run_test( - vec![ - "file:///test/asdf/test/asdf/", - "file:///test/asdf/", - "file:///test/asdf/", - "file:///testing/456/893/", - "file:///testing/456/893/test/", - ], - vec!["file:///test/asdf/", "file:///testing/456/893/"], - ); - run_test(vec![], vec![]); - } } diff --git a/cli/lsp/documents.rs b/cli/lsp/documents.rs index 625f4728d471e0..14207ccd5dc8f5 100644 --- a/cli/lsp/documents.rs +++ b/cli/lsp/documents.rs @@ -1248,7 +1248,7 @@ impl Documents { // only refresh the dependencies if the underlying configuration has changed if self.resolver_config_hash != new_resolver_config_hash { - self.refresh_dependencies(root_dirs); + self.refresh_dependencies(); self.resolver_config_hash = new_resolver_config_hash; } diff --git a/cli/lsp/language_server.rs b/cli/lsp/language_server.rs index 856e6286bff347..49a809be5675a5 100644 --- a/cli/lsp/language_server.rs +++ b/cli/lsp/language_server.rs @@ -316,7 +316,7 @@ impl LanguageServer { } } - pub async fn refresh_specifiers_from_client(&self) { + pub async fn refresh_specifiers_from_client(&self) -> bool { let (client, specifiers) = { let ls = self.0.read().await; @@ -397,6 +397,7 @@ impl LanguageServer { ls.send_diagnostics_update(); } } + touched } } @@ -1080,7 +1081,6 @@ impl Inner { fn refresh_documents_config(&mut self) { self.documents.update_config( - self.config.root_dirs(), self.maybe_import_map.clone(), self.maybe_config_file.as_ref(), self.maybe_package_json.as_ref(), @@ -2752,7 +2752,10 @@ impl tower_lsp::LanguageServer for LanguageServer { async fn initialized(&self, params: InitializedParams) { self.0.write().await.initialized(params).await; - self.refresh_specifiers_from_client().await; + if !self.refresh_specifiers_from_client().await { + // force update config + self.0.write().await.refresh_documents_config(); + } lsp_log!("Server ready."); } @@ -2887,17 +2890,17 @@ impl tower_lsp::LanguageServer for LanguageServer { &self, params: DidChangeWorkspaceFoldersParams, ) { - let mark = { + let (performance, mark) = { let mut ls = self.0.write().await; let mark = ls .performance .mark("did_change_workspace_folders", Some(¶ms)); ls.did_change_workspace_folders(params); - mark + (ls.performance.clone(), mark) }; self.refresh_specifiers_from_client().await; - self.0.read().await.performance.measure(mark); + performance.measure(mark); } async fn document_symbol( diff --git a/cli/tests/integration/lsp_tests.rs b/cli/tests/integration/lsp_tests.rs index 981488a67eeff4..8107da1ccffbfd 100644 --- a/cli/tests/integration/lsp_tests.rs +++ b/cli/tests/integration/lsp_tests.rs @@ -5684,7 +5684,7 @@ struct PerformanceAverages { #[test] fn lsp_performance() { - let mut client = LspClientBuilder::new().build(); + let mut client = LspClientBuilder::new().print_stderr().build(); client.initialize_default(); client.did_open(json!({ "textDocument": { diff --git a/test_util/src/lsp.rs b/test_util/src/lsp.rs index 7578aedf323dc1..6adc877cca1bbc 100644 --- a/test_util/src/lsp.rs +++ b/test_util/src/lsp.rs @@ -148,6 +148,11 @@ impl LspStdoutReader { self.pending_messages.0.lock().len() } + pub fn output_pending_messages(&self) { + let messages = self.pending_messages.0.lock(); + eprintln!("{:?}", messages); + } + pub fn had_message(&self, is_match: impl Fn(&LspMessage) -> bool) -> bool { self.read_messages.iter().any(&is_match) || self.pending_messages.0.lock().iter().any(&is_match) @@ -453,6 +458,9 @@ impl LspClientBuilder { self } + // not deprecated, this is just here so you don't accidentally + // commit code with this enabled + #[deprecated] pub fn print_stderr(&mut self) -> &mut Self { self.print_stderr = true; self @@ -541,6 +549,7 @@ impl LspClient { } pub fn queue_len(&self) -> usize { + self.reader.output_pending_messages(); self.reader.pending_len() } From 8ef8a28192b7c54b850b44ed35cd79ed7202d01a Mon Sep 17 00:00:00 2001 From: David Sherret Date: Tue, 14 Mar 2023 10:49:26 -0400 Subject: [PATCH 07/11] Updates. --- cli/tests/integration/cert_tests.rs | 2 +- cli/tests/integration/lsp_tests.rs | 34 ++++++++++++++--------------- cli/tests/integration/run_tests.rs | 4 ++-- 3 files changed, 20 insertions(+), 20 deletions(-) diff --git a/cli/tests/integration/cert_tests.rs b/cli/tests/integration/cert_tests.rs index 0031367589d05f..d3da6d75af3830 100644 --- a/cli/tests/integration/cert_tests.rs +++ b/cli/tests/integration/cert_tests.rs @@ -110,7 +110,7 @@ fn cafile_fetch() { #[test] fn cafile_compile() { let context = TestContext::with_http_server(); - let temp_dir = context.deno_dir().path(); + let temp_dir = context.temp_dir().path(); let output_exe = if cfg!(windows) { temp_dir.join("cert.exe") } else { diff --git a/cli/tests/integration/lsp_tests.rs b/cli/tests/integration/lsp_tests.rs index 8107da1ccffbfd..b8cc8697cdc479 100644 --- a/cli/tests/integration/lsp_tests.rs +++ b/cli/tests/integration/lsp_tests.rs @@ -26,7 +26,7 @@ fn lsp_startup_shutdown() { #[test] fn lsp_init_tsconfig() { let context = TestContextBuilder::new().build(); - let temp_dir = context.deno_dir(); + let temp_dir = context.temp_dir(); temp_dir.write( "lib.tsconfig.json", @@ -59,7 +59,7 @@ fn lsp_init_tsconfig() { #[test] fn lsp_tsconfig_types() { let context = TestContextBuilder::new().build(); - let temp_dir = context.deno_dir(); + let temp_dir = context.temp_dir(); temp_dir.write( "types.tsconfig.json", @@ -124,7 +124,7 @@ fn lsp_tsconfig_bad_config_path() { #[test] fn lsp_triple_slash_types() { let context = TestContextBuilder::new().build(); - let temp_dir = context.deno_dir(); + let temp_dir = context.temp_dir(); let a_dts = "// deno-lint-ignore-file no-var\ndeclare var a: string;"; temp_dir.write("a.d.ts", a_dts); let mut client = context.new_lsp_command().build(); @@ -147,7 +147,7 @@ fn lsp_triple_slash_types() { #[test] fn lsp_import_map() { let context = TestContextBuilder::new().build(); - let temp_dir = context.deno_dir(); + let temp_dir = context.temp_dir(); let import_map = r#"{ "imports": { "/~/": "./lib/" @@ -231,7 +231,7 @@ fn lsp_import_map_data_url() { #[test] fn lsp_import_map_config_file() { let context = TestContextBuilder::new().build(); - let temp_dir = context.deno_dir(); + let temp_dir = context.temp_dir(); temp_dir.write( "deno.import_map.jsonc", r#"{ @@ -298,7 +298,7 @@ fn lsp_import_map_config_file() { #[test] fn lsp_import_map_embedded_in_config_file() { let context = TestContextBuilder::new().build(); - let temp_dir = context.deno_dir(); + let temp_dir = context.temp_dir(); temp_dir.write( "deno.embedded_import_map.jsonc", r#"{ @@ -359,7 +359,7 @@ fn lsp_import_map_embedded_in_config_file() { #[test] fn lsp_deno_task() { let context = TestContextBuilder::new().build(); - let temp_dir = context.deno_dir(); + let temp_dir = context.temp_dir(); temp_dir.write( "deno.jsonc", r#"{ @@ -510,7 +510,7 @@ fn lsp_import_assertions() { #[test] fn lsp_import_map_import_completions() { let context = TestContextBuilder::new().build(); - let temp_dir = context.deno_dir(); + let temp_dir = context.temp_dir(); temp_dir.write( "import-map.json", r#"{ @@ -932,7 +932,7 @@ fn lsp_workspace_enable_paths() { // we aren't actually writing anything to the tempdir in this test, but we // just need a legitimate file path on the host system so that logic that // tries to convert to and from the fs paths works on all env - let temp_dir = context.deno_dir(); + let temp_dir = context.temp_dir(); let root_specifier = temp_dir.uri(); @@ -1216,7 +1216,7 @@ fn lsp_hover_change_mbc() { #[test] fn lsp_hover_closed_document() { let context = TestContextBuilder::new().build(); - let temp_dir = context.deno_dir(); + let temp_dir = context.temp_dir(); temp_dir.write("a.ts", r#"export const a = "a";"#); temp_dir.write("b.ts", r#"export * from "./a.ts";"#); temp_dir.write("c.ts", "import { a } from \"./b.ts\";\nconsole.log(a);\n"); @@ -5188,7 +5188,7 @@ fn lsp_auto_discover_registry() { #[test] fn lsp_cache_location() { let context = TestContextBuilder::new().use_http_server().build(); - let temp_dir = context.deno_dir(); + let temp_dir = context.temp_dir(); let mut client = context.new_lsp_command().build(); client.initialize(|builder| { builder.set_cache(".cache").add_test_server_suggestions(); @@ -5826,7 +5826,7 @@ fn lsp_format_mbc() { #[test] fn lsp_format_exclude_with_config() { let context = TestContextBuilder::new().build(); - let temp_dir = context.deno_dir(); + let temp_dir = context.temp_dir(); temp_dir.write( "deno.fmt.jsonc", @@ -5879,7 +5879,7 @@ fn lsp_format_exclude_with_config() { #[test] fn lsp_format_exclude_default_config() { let context = TestContextBuilder::new().build(); - let temp_dir = context.deno_dir(); + let temp_dir = context.temp_dir(); temp_dir.write( "deno.fmt.jsonc", @@ -6071,7 +6071,7 @@ fn lsp_format_markdown() { #[test] fn lsp_format_with_config() { let context = TestContextBuilder::new().build(); - let temp_dir = context.deno_dir(); + let temp_dir = context.temp_dir(); temp_dir.write( "deno.fmt.jsonc", r#"{ @@ -6635,7 +6635,7 @@ console.log(snake_case); #[test] fn lsp_lint_with_config() { let context = TestContextBuilder::new().build(); - let temp_dir = context.deno_dir(); + let temp_dir = context.temp_dir(); temp_dir.write( "deno.lint.jsonc", @@ -6676,7 +6676,7 @@ fn lsp_lint_with_config() { #[test] fn lsp_lint_exclude_with_config() { let context = TestContextBuilder::new().build(); - let temp_dir = context.deno_dir(); + let temp_dir = context.temp_dir(); temp_dir.write( "deno.lint.jsonc", @@ -6814,7 +6814,7 @@ struct TestRunResponseParams { #[test] fn lsp_testing_api() { let context = TestContextBuilder::new().build(); - let temp_dir = context.deno_dir(); + let temp_dir = context.temp_dir(); let contents = r#" Deno.test({ diff --git a/cli/tests/integration/run_tests.rs b/cli/tests/integration/run_tests.rs index 3a19564a39463d..f57dd987673a39 100644 --- a/cli/tests/integration/run_tests.rs +++ b/cli/tests/integration/run_tests.rs @@ -2874,7 +2874,7 @@ fn package_json_no_node_modules_dir_created() { .add_npm_env_vars() .use_temp_cwd() .build(); - let temp_dir = context.deno_dir(); + let temp_dir = context.temp_dir(); temp_dir.write("deno.json", "{}"); temp_dir.write("package.json", "{}"); @@ -2892,7 +2892,7 @@ fn node_modules_dir_no_npm_specifiers_no_dir_created() { .add_npm_env_vars() .use_temp_cwd() .build(); - let temp_dir = context.deno_dir(); + let temp_dir = context.temp_dir(); temp_dir.write("deno.json", "{}"); temp_dir.write("main.ts", ""); From 52476b858369be91c74de66d081eec519e1b2a1b Mon Sep 17 00:00:00 2001 From: David Sherret Date: Tue, 14 Mar 2023 15:46:47 -0400 Subject: [PATCH 08/11] Passing tests --- cli/lsp/client.rs | 29 +++++ cli/lsp/completions.rs | 20 ++-- cli/lsp/config.rs | 2 - cli/lsp/language_server.rs | 164 +++++++++++++++++++---------- cli/lsp/urls.rs | 27 +++-- cli/tests/integration/lsp_tests.rs | 112 ++++++++++++-------- cli/util/path.rs | 27 ----- test_util/src/lsp.rs | 14 +++ 8 files changed, 248 insertions(+), 147 deletions(-) diff --git a/cli/lsp/client.rs b/cli/lsp/client.rs index cdef1cfbf5662d..db8e3f7d8262a3 100644 --- a/cli/lsp/client.rs +++ b/cli/lsp/client.rs @@ -27,6 +27,26 @@ pub enum TestingNotification { Progress(testing_lsp_custom::TestRunProgressParams), } +/// A client that provides methods that don't need to be awaited. +/// +/// This struct should be used in places where you want to ensure the +/// code doesn't accidentally call into the client while holding an +/// LSP lock, which could then call into the server causing a deadlock. +#[derive(Clone)] +pub struct NotificationOnlyClient(Client); + +impl NotificationOnlyClient { + pub fn send_registry_state_notification( + &self, + params: lsp_custom::RegistryStateNotificationParams, + ) { + let client = self.0.clone(); + tokio::task::spawn(async move { + client.send_registry_state_notification(params).await; + }); + } +} + #[derive(Clone)] pub struct Client(Arc); @@ -45,6 +65,10 @@ impl Client { Self(Arc::new(ReplClient)) } + pub fn as_notification_only(&self) -> NotificationOnlyClient { + NotificationOnlyClient(self.clone()) + } + pub async fn publish_diagnostics( &self, uri: lsp::Url, @@ -207,6 +231,10 @@ impl ClientTrait for TowerClient { uris: Vec, ) -> AsyncReturn>, AnyError>> { + eprintln!( + "URIS: {:?}", + uris.iter().map(|i| i.as_str()).collect::>() + ); let client = self.0.clone(); Box::pin(async move { let config_response = client @@ -235,6 +263,7 @@ impl ClientTrait for TowerClient { } fn workspace_configuration(&self) -> AsyncReturn> { + eprintln!("WORKSPACE CONFIG"); let client = self.0.clone(); Box::pin(async move { let config_response = client diff --git a/cli/lsp/completions.rs b/cli/lsp/completions.rs index 3651fbeec0b193..132d4d84cb0bf5 100644 --- a/cli/lsp/completions.rs +++ b/cli/lsp/completions.rs @@ -1,6 +1,6 @@ // Copyright 2018-2023 the Deno authors. All rights reserved. MIT license. -use super::client::Client; +use super::client::NotificationOnlyClient; use super::config::ConfigSnapshot; use super::documents::Documents; use super::lsp_custom; @@ -48,7 +48,7 @@ pub struct CompletionItemData { async fn check_auto_config_registry( url_str: &str, config: &ConfigSnapshot, - client: Client, + client: &NotificationOnlyClient, module_registries: &ModuleRegistry, ) { // check to see if auto discovery is enabled @@ -78,14 +78,12 @@ async fn check_auto_config_registry( // incompatible. // TODO(@kitsonk) clean up protocol when doing v2 of suggestions if suggestions { - client - .send_registry_state_notification( - lsp_custom::RegistryStateNotificationParams { - origin, - suggestions, - }, - ) - .await; + client.send_registry_state_notification( + lsp_custom::RegistryStateNotificationParams { + origin, + suggestions, + }, + ); } } } @@ -139,7 +137,7 @@ pub async fn get_import_completions( specifier: &ModuleSpecifier, position: &lsp::Position, config: &ConfigSnapshot, - client: Client, + client: &NotificationOnlyClient, module_registries: &ModuleRegistry, documents: &Documents, maybe_import_map: Option>, diff --git a/cli/lsp/config.rs b/cli/lsp/config.rs index 44eec8c3560aaf..60b975a44bc92d 100644 --- a/cli/lsp/config.rs +++ b/cli/lsp/config.rs @@ -1,7 +1,6 @@ // Copyright 2018-2023 the Deno authors. All rights reserved. MIT license. use super::logging::lsp_log; -use crate::util::path::ensure_directory_specifier; use crate::util::path::specifier_to_file_path; use deno_core::error::AnyError; use deno_core::serde::Deserialize; @@ -575,7 +574,6 @@ impl Config { workspace: ModuleSpecifier, enabled_paths: Vec, ) -> bool { - let workspace = ensure_directory_specifier(workspace); let mut touched = false; if !enabled_paths.is_empty() { if let Ok(workspace_path) = specifier_to_file_path(&workspace) { diff --git a/cli/lsp/language_server.rs b/cli/lsp/language_server.rs index 4edebb4a96d5e8..b866056f863236 100644 --- a/cli/lsp/language_server.rs +++ b/cli/lsp/language_server.rs @@ -75,6 +75,7 @@ use crate::cache::HttpCache; use crate::file_fetcher::FileFetcher; use crate::graph_util; use crate::http_util::HttpClient; +use crate::lsp::urls::LspUrlKind; use crate::npm::create_npm_fs_resolver; use crate::npm::NpmCache; use crate::npm::NpmPackageResolver; @@ -84,7 +85,6 @@ use crate::proc_state::ProcState; use crate::tools::fmt::format_file; use crate::tools::fmt::format_parsed_source; use crate::util::fs::remove_dir_all_if_exists; -use crate::util::path::ensure_directory_specifier; use crate::util::path::specifier_to_file_path; use crate::util::progress_bar::ProgressBar; use crate::util::progress_bar::ProgressBarStyle; @@ -377,10 +377,11 @@ impl LanguageServer { let mut ls = self.0.write().await; if let Ok(configs) = configs_result { - for (i, value) in configs.into_iter().enumerate() { + for (value, internal_uri) in + configs.into_iter().zip(specifiers.into_iter().map(|s| s.1)) + { match value { Ok(specifier_settings) => { - let internal_uri = specifiers[i].1.clone(); if ls .config .set_specifier_settings(internal_uri, specifier_settings) @@ -1062,8 +1063,7 @@ impl Inner { // sometimes this root uri may not have a trailing slash, so force it to self.config.root_uri = params .root_uri - .map(|s| self.url_map.normalize_url(&s)) - .map(ensure_directory_specifier); + .map(|s| self.url_map.normalize_url(&s, LspUrlKind::Folder)); if let Some(value) = params.initialization_options { self.config.set_workspace_settings(value).map_err(|err| { @@ -1074,7 +1074,12 @@ impl Inner { self.config.workspace_folders = params.workspace_folders.map(|folders| { folders .into_iter() - .map(|folder| (self.url_map.normalize_url(&folder.uri), folder)) + .map(|folder| { + ( + self.url_map.normalize_url(&folder.uri, LspUrlKind::Folder), + folder, + ) + }) .collect() }); self.config.update_capabilities(¶ms.capabilities); @@ -1215,7 +1220,9 @@ impl Inner { async fn did_change(&mut self, params: DidChangeTextDocumentParams) { let mark = self.performance.mark("did_change", Some(¶ms)); - let specifier = self.url_map.normalize_url(¶ms.text_document.uri); + let specifier = self + .url_map + .normalize_url(¶ms.text_document.uri, LspUrlKind::File); match self.documents.change( &specifier, params.text_document.version, @@ -1251,7 +1258,9 @@ impl Inner { // already managed by the language service return; } - let specifier = self.url_map.normalize_url(¶ms.text_document.uri); + let specifier = self + .url_map + .normalize_url(¶ms.text_document.uri, LspUrlKind::File); if let Err(err) = self.documents.close(&specifier) { error!("{}", err); @@ -1326,7 +1335,7 @@ impl Inner { let changes: HashSet = params .changes .iter() - .map(|f| self.url_map.normalize_url(&f.uri)) + .map(|f| self.url_map.normalize_url(&f.uri, LspUrlKind::File)) .collect(); // if the current deno.json has changed, we need to reload it @@ -1379,7 +1388,12 @@ impl Inner { .event .added .into_iter() - .map(|folder| (self.url_map.normalize_url(&folder.uri), folder)) + .map(|folder| { + ( + self.url_map.normalize_url(&folder.uri, LspUrlKind::Folder), + folder, + ) + }) .collect::>(); if let Some(current_folders) = &self.config.workspace_folders { for (specifier, folder) in current_folders { @@ -1399,7 +1413,9 @@ impl Inner { &self, params: DocumentSymbolParams, ) -> LspResult> { - let specifier = self.url_map.normalize_url(¶ms.text_document.uri); + let specifier = self + .url_map + .normalize_url(¶ms.text_document.uri, LspUrlKind::File); if !self.is_diagnosable(&specifier) || !self.config.specifier_enabled(&specifier) { @@ -1437,7 +1453,9 @@ impl Inner { &self, params: DocumentFormattingParams, ) -> LspResult>> { - let specifier = self.url_map.normalize_url(¶ms.text_document.uri); + let specifier = self + .url_map + .normalize_url(¶ms.text_document.uri, LspUrlKind::File); let document = match self.documents.get(&specifier) { Some(doc) if doc.is_open() => doc, _ => return Ok(None), @@ -1500,9 +1518,10 @@ impl Inner { } async fn hover(&self, params: HoverParams) -> LspResult> { - let specifier = self - .url_map - .normalize_url(¶ms.text_document_position_params.text_document.uri); + let specifier = self.url_map.normalize_url( + ¶ms.text_document_position_params.text_document.uri, + LspUrlKind::File, + ); if !self.is_diagnosable(&specifier) || !self.config.specifier_enabled(&specifier) { @@ -1587,7 +1606,9 @@ impl Inner { &self, params: CodeActionParams, ) -> LspResult> { - let specifier = self.url_map.normalize_url(¶ms.text_document.uri); + let specifier = self + .url_map + .normalize_url(¶ms.text_document.uri, LspUrlKind::File); if !self.is_diagnosable(&specifier) || !self.config.specifier_enabled(&specifier) { @@ -1847,7 +1868,9 @@ impl Inner { &self, params: CodeLensParams, ) -> LspResult>> { - let specifier = self.url_map.normalize_url(¶ms.text_document.uri); + let specifier = self + .url_map + .normalize_url(¶ms.text_document.uri, LspUrlKind::File); if !self.is_diagnosable(&specifier) || !self.config.specifier_enabled(&specifier) || !(self.config.get_workspace_settings().enabled_code_lens() @@ -1907,9 +1930,10 @@ impl Inner { &self, params: DocumentHighlightParams, ) -> LspResult>> { - let specifier = self - .url_map - .normalize_url(¶ms.text_document_position_params.text_document.uri); + let specifier = self.url_map.normalize_url( + ¶ms.text_document_position_params.text_document.uri, + LspUrlKind::File, + ); if !self.is_diagnosable(&specifier) || !self.config.specifier_enabled(&specifier) { @@ -1951,9 +1975,10 @@ impl Inner { &self, params: ReferenceParams, ) -> LspResult>> { - let specifier = self - .url_map - .normalize_url(¶ms.text_document_position.text_document.uri); + let specifier = self.url_map.normalize_url( + ¶ms.text_document_position.text_document.uri, + LspUrlKind::File, + ); if !self.is_diagnosable(&specifier) || !self.config.specifier_enabled(&specifier) { @@ -2007,9 +2032,10 @@ impl Inner { &self, params: GotoDefinitionParams, ) -> LspResult> { - let specifier = self - .url_map - .normalize_url(¶ms.text_document_position_params.text_document.uri); + let specifier = self.url_map.normalize_url( + ¶ms.text_document_position_params.text_document.uri, + LspUrlKind::File, + ); if !self.is_diagnosable(&specifier) || !self.config.specifier_enabled(&specifier) { @@ -2046,9 +2072,10 @@ impl Inner { &self, params: GotoTypeDefinitionParams, ) -> LspResult> { - let specifier = self - .url_map - .normalize_url(¶ms.text_document_position_params.text_document.uri); + let specifier = self.url_map.normalize_url( + ¶ms.text_document_position_params.text_document.uri, + LspUrlKind::File, + ); if !self.is_diagnosable(&specifier) || !self.config.specifier_enabled(&specifier) { @@ -2093,9 +2120,10 @@ impl Inner { &self, params: CompletionParams, ) -> LspResult> { - let specifier = self - .url_map - .normalize_url(¶ms.text_document_position.text_document.uri); + let specifier = self.url_map.normalize_url( + ¶ms.text_document_position.text_document.uri, + LspUrlKind::File, + ); if !self.is_diagnosable(&specifier) || !self.config.specifier_enabled(&specifier) { @@ -2112,7 +2140,7 @@ impl Inner { &specifier, ¶ms.text_document_position.position, &self.config.snapshot(), - self.client.clone(), + &self.client.as_notification_only(), &self.module_registries, &self.documents, self.maybe_import_map.clone(), @@ -2252,9 +2280,10 @@ impl Inner { &self, params: GotoImplementationParams, ) -> LspResult> { - let specifier = self - .url_map - .normalize_url(¶ms.text_document_position_params.text_document.uri); + let specifier = self.url_map.normalize_url( + ¶ms.text_document_position_params.text_document.uri, + LspUrlKind::File, + ); if !self.is_diagnosable(&specifier) || !self.config.specifier_enabled(&specifier) { @@ -2298,7 +2327,9 @@ impl Inner { &self, params: FoldingRangeParams, ) -> LspResult>> { - let specifier = self.url_map.normalize_url(¶ms.text_document.uri); + let specifier = self + .url_map + .normalize_url(¶ms.text_document.uri, LspUrlKind::File); if !self.is_diagnosable(&specifier) || !self.config.specifier_enabled(&specifier) { @@ -2342,7 +2373,9 @@ impl Inner { &self, params: CallHierarchyIncomingCallsParams, ) -> LspResult>> { - let specifier = self.url_map.normalize_url(¶ms.item.uri); + let specifier = self + .url_map + .normalize_url(¶ms.item.uri, LspUrlKind::File); if !self.is_diagnosable(&specifier) || !self.config.specifier_enabled(&specifier) { @@ -2388,7 +2421,9 @@ impl Inner { &self, params: CallHierarchyOutgoingCallsParams, ) -> LspResult>> { - let specifier = self.url_map.normalize_url(¶ms.item.uri); + let specifier = self + .url_map + .normalize_url(¶ms.item.uri, LspUrlKind::File); if !self.is_diagnosable(&specifier) || !self.config.specifier_enabled(&specifier) { @@ -2435,9 +2470,10 @@ impl Inner { &self, params: CallHierarchyPrepareParams, ) -> LspResult>> { - let specifier = self - .url_map - .normalize_url(¶ms.text_document_position_params.text_document.uri); + let specifier = self.url_map.normalize_url( + ¶ms.text_document_position_params.text_document.uri, + LspUrlKind::File, + ); if !self.is_diagnosable(&specifier) || !self.config.specifier_enabled(&specifier) { @@ -2503,9 +2539,10 @@ impl Inner { &self, params: RenameParams, ) -> LspResult> { - let specifier = self - .url_map - .normalize_url(¶ms.text_document_position.text_document.uri); + let specifier = self.url_map.normalize_url( + ¶ms.text_document_position.text_document.uri, + LspUrlKind::File, + ); if !self.is_diagnosable(&specifier) || !self.config.specifier_enabled(&specifier) { @@ -2555,7 +2592,9 @@ impl Inner { &self, params: SelectionRangeParams, ) -> LspResult>> { - let specifier = self.url_map.normalize_url(¶ms.text_document.uri); + let specifier = self + .url_map + .normalize_url(¶ms.text_document.uri, LspUrlKind::File); if !self.is_diagnosable(&specifier) || !self.config.specifier_enabled(&specifier) { @@ -2593,7 +2632,9 @@ impl Inner { &self, params: SemanticTokensParams, ) -> LspResult> { - let specifier = self.url_map.normalize_url(¶ms.text_document.uri); + let specifier = self + .url_map + .normalize_url(¶ms.text_document.uri, LspUrlKind::File); if !self.is_diagnosable(&specifier) || !self.config.specifier_enabled(&specifier) { @@ -2635,7 +2676,9 @@ impl Inner { &self, params: SemanticTokensRangeParams, ) -> LspResult> { - let specifier = self.url_map.normalize_url(¶ms.text_document.uri); + let specifier = self + .url_map + .normalize_url(¶ms.text_document.uri, LspUrlKind::File); if !self.is_diagnosable(&specifier) || !self.config.specifier_enabled(&specifier) { @@ -2678,9 +2721,10 @@ impl Inner { &self, params: SignatureHelpParams, ) -> LspResult> { - let specifier = self - .url_map - .normalize_url(¶ms.text_document_position_params.text_document.uri); + let specifier = self.url_map.normalize_url( + ¶ms.text_document_position_params.text_document.uri, + LspUrlKind::File, + ); if !self.is_diagnosable(&specifier) || !self.config.specifier_enabled(&specifier) { @@ -2824,7 +2868,8 @@ impl tower_lsp::LanguageServer for LanguageServer { let mut inner = self.0.write().await; let client = inner.client.clone(); let client_uri = params.text_document.uri.clone(); - let specifier = inner.url_map.normalize_url(&client_uri); + let specifier = + inner.url_map.normalize_url(&client_uri, LspUrlKind::File); let document = inner.did_open(&specifier, params).await; let has_specifier_settings = inner.config.has_specifier_settings(&specifier); @@ -2865,6 +2910,7 @@ impl tower_lsp::LanguageServer for LanguageServer { { ls.refresh_documents_config(); ls.send_diagnostics_update(); + ls.send_testing_update(); } } } @@ -3127,7 +3173,9 @@ impl Inner { &self, params: lsp_custom::CacheParams, ) -> Result, AnyError> { - let referrer = self.url_map.normalize_url(¶ms.referrer.uri); + let referrer = self + .url_map + .normalize_url(¶ms.referrer.uri, LspUrlKind::File); if !self.is_diagnosable(&referrer) { return Ok(None); } @@ -3137,7 +3185,7 @@ impl Inner { params .uris .iter() - .map(|t| self.url_map.normalize_url(&t.uri)) + .map(|t| self.url_map.normalize_url(&t.uri, LspUrlKind::File)) .collect() } else { vec![referrer] @@ -3211,7 +3259,9 @@ impl Inner { &self, params: InlayHintParams, ) -> LspResult>> { - let specifier = self.url_map.normalize_url(¶ms.text_document.uri); + let specifier = self + .url_map + .normalize_url(¶ms.text_document.uri, LspUrlKind::File); let workspace_settings = self.config.get_workspace_settings(); if !self.is_diagnosable(&specifier) || !self.config.specifier_enabled(&specifier) @@ -3272,7 +3322,9 @@ impl Inner { let mark = self .performance .mark("virtual_text_document", Some(¶ms)); - let specifier = self.url_map.normalize_url(¶ms.text_document.uri); + let specifier = self + .url_map + .normalize_url(¶ms.text_document.uri, LspUrlKind::File); let contents = if specifier.as_str() == "deno:/status.md" { let mut contents = String::new(); let mut documents_specifiers = self diff --git a/cli/lsp/urls.rs b/cli/lsp/urls.rs index 2168dfed413e52..14717d71527a6a 100644 --- a/cli/lsp/urls.rs +++ b/cli/lsp/urls.rs @@ -80,6 +80,12 @@ impl LspUrlMapInner { } } +#[derive(Debug, Clone, Copy)] +pub enum LspUrlKind { + File, + Folder, +} + /// 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. @@ -142,13 +148,20 @@ impl LspUrlMap { /// converted into proper module specifiers, as well as handle situations /// 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 { + /// + /// Note: Sometimes the url provided by the client may not have a trailing slash, + /// so we need to force it to in the mapping and nee to explicitly state whether + /// this is a file or directory url. + pub fn normalize_url(&self, url: &Url, kind: LspUrlKind) -> ModuleSpecifier { let mut inner = self.0.lock(); if let Some(specifier) = inner.get_specifier(url).cloned() { specifier } else { let specifier = if let Ok(path) = url.to_file_path() { - Url::from_file_path(path).unwrap() + match kind { + LspUrlKind::Folder => Url::from_directory_path(path).unwrap(), + LspUrlKind::File => Url::from_file_path(path).unwrap(), + } } else { url.clone() }; @@ -184,7 +197,7 @@ mod tests { Url::parse("deno:/https/deno.land/x/pkg%401.0.0/mod.ts").unwrap(); assert_eq!(actual_url, expected_url); - let actual_specifier = map.normalize_url(&actual_url); + let actual_specifier = map.normalize_url(&actual_url, LspUrlKind::File); assert_eq!(actual_specifier, fixture); } @@ -199,7 +212,7 @@ mod tests { let expected_url = Url::parse("deno:/https/cdn.skypack.dev/-/postcss%40v8.2.9-E4SktPp9c0AtxrJHp8iV/dist%3Des2020%2Cmode%3Dtypes/lib/postcss.d.ts").unwrap(); assert_eq!(actual_url, expected_url); - let actual_specifier = map.normalize_url(&actual_url); + let actual_specifier = map.normalize_url(&actual_url, LspUrlKind::File); assert_eq!(actual_specifier, fixture); } @@ -213,7 +226,7 @@ mod tests { let expected_url = Url::parse("deno:/c21c7fc382b2b0553dc0864aa81a3acacfb7b3d1285ab5ae76da6abec213fb37/data_url.ts").unwrap(); assert_eq!(actual_url, expected_url); - let actual_specifier = map.normalize_url(&actual_url); + let actual_specifier = map.normalize_url(&actual_url, LspUrlKind::File); assert_eq!(actual_specifier, fixture); } @@ -225,7 +238,7 @@ mod tests { "file:///c%3A/Users/deno/Desktop/file%20with%20spaces%20in%20name.txt", ) .unwrap(); - let actual = map.normalize_url(&fixture); + let actual = map.normalize_url(&fixture, LspUrlKind::File); let expected = Url::parse("file:///C:/Users/deno/Desktop/file with spaces in name.txt") .unwrap(); @@ -240,7 +253,7 @@ mod tests { "file:///Users/deno/Desktop/file%20with%20spaces%20in%20name.txt", ) .unwrap(); - let actual = map.normalize_url(&fixture); + let actual = map.normalize_url(&fixture, LspUrlKind::File); let expected = Url::parse("file:///Users/deno/Desktop/file with spaces in name.txt") .unwrap(); diff --git a/cli/tests/integration/lsp_tests.rs b/cli/tests/integration/lsp_tests.rs index b8cc8697cdc479..2296181ffa2482 100644 --- a/cli/tests/integration/lsp_tests.rs +++ b/cli/tests/integration/lsp_tests.rs @@ -937,21 +937,22 @@ fn lsp_workspace_enable_paths() { let root_specifier = temp_dir.uri(); let mut client = context.new_lsp_command().build(); - client.initialize(|builder| { - builder - .set_enable_paths(vec!["./worker".to_string()]) - .set_root_uri(root_specifier.clone()) - .set_workspace_folders(vec![lsp::WorkspaceFolder { - uri: root_specifier.clone(), - name: "project".to_string(), - }]) - .set_deno_enable(false); - }); - - client.handle_configuration_request(json!([{ - "enable": false, - "enablePaths": ["./worker"], - }])); + client.initialize_with_config( + |builder| { + builder + .set_enable_paths(vec!["./worker".to_string()]) + .set_root_uri(root_specifier.clone()) + .set_workspace_folders(vec![lsp::WorkspaceFolder { + uri: root_specifier.clone(), + name: "project".to_string(), + }]) + .set_deno_enable(false); + }, + json!([{ + "enable": false, + "enablePaths": ["./worker"], + }]), + ); client.did_open(json!({ "textDocument": { @@ -5668,7 +5669,7 @@ fn lsp_diagnostics_refresh_dependents() { assert_eq!(client.queue_len(), 0); } -#[derive(Deserialize)] +#[derive(Debug, Deserialize)] #[serde(rename_all = "camelCase")] pub struct PerformanceAverage { pub name: String, @@ -5676,7 +5677,7 @@ pub struct PerformanceAverage { pub average_duration: u32, } -#[derive(Deserialize)] +#[derive(Debug, Deserialize)] #[serde(rename_all = "camelCase")] struct PerformanceAverages { averages: Vec, @@ -5684,7 +5685,7 @@ struct PerformanceAverages { #[test] fn lsp_performance() { - let mut client = LspClientBuilder::new().print_stderr().build(); + let mut client = LspClientBuilder::new().build(); client.initialize_default(); client.did_open(json!({ "textDocument": { @@ -5707,7 +5708,30 @@ fn lsp_performance() { "deno/performance", json!(null), ); - assert_eq!(res.averages.len(), 13); + let mut averages = res + .averages + .iter() + .map(|a| a.name.as_str()) + .collect::>(); + averages.sort(); + assert_eq!( + averages, + vec![ + "did_open", + "hover", + "initialize", + "op_load", + "request", + "testing_update", + "update_cache", + "update_diagnostics_deps", + "update_diagnostics_lint", + "update_diagnostics_ts", + "update_import_map", + "update_registries", + "update_tsconfig", + ] + ); client.shutdown(); } @@ -6231,32 +6255,32 @@ fn lsp_configuration_did_change() { "settings": {} }), ); - let (id, method, _) = client.read_request::(); - assert_eq!(method, "workspace/configuration"); - client.write_response( - id, - json!([{ - "enable": true, - "codeLens": { - "implementations": true, - "references": true - }, - "importMap": null, - "lint": true, - "suggest": { - "autoImports": true, - "completeFunctionCalls": false, - "names": true, - "paths": true, - "imports": { - "hosts": { - "http://localhost:4545/": true - } + let request = json!([{ + "enable": true, + "codeLens": { + "implementations": true, + "references": true + }, + "importMap": null, + "lint": true, + "suggest": { + "autoImports": true, + "completeFunctionCalls": false, + "names": true, + "paths": true, + "imports": { + "hosts": { + "http://localhost:4545/": true } - }, - "unstable": false - }]), - ); + } + }, + "unstable": false + }]); + // one for the workspace + client.handle_configuration_request(request.clone()); + // one for the specifier + client.handle_configuration_request(request); + let list = client.get_completion_list( "file:///a/file.ts", (0, 46), diff --git a/cli/util/path.rs b/cli/util/path.rs index d073d80bd5b44b..69c52bf6edc94a 100644 --- a/cli/util/path.rs +++ b/cli/util/path.rs @@ -69,18 +69,6 @@ pub fn specifier_to_file_path( } } -/// Ensures a specifier that will definitely be a directory has a trailing slash. -pub fn ensure_directory_specifier( - mut specifier: ModuleSpecifier, -) -> ModuleSpecifier { - let path = specifier.path(); - if !path.ends_with('/') { - let new_path = format!("{path}/"); - specifier.set_path(&new_path); - } - specifier -} - /// Gets the parent of this module specifier. pub fn specifier_parent(specifier: &ModuleSpecifier) -> ModuleSpecifier { let mut specifier = specifier.clone(); @@ -264,21 +252,6 @@ mod test { } } - #[test] - fn test_ensure_directory_specifier() { - run_test("file:///", "file:///"); - run_test("file:///test", "file:///test/"); - run_test("file:///test/", "file:///test/"); - run_test("file:///test/other", "file:///test/other/"); - run_test("file:///test/other/", "file:///test/other/"); - - fn run_test(specifier: &str, expected: &str) { - let result = - ensure_directory_specifier(ModuleSpecifier::parse(specifier).unwrap()); - assert_eq!(result.to_string(), expected); - } - } - #[test] fn test_specifier_parent() { run_test("file:///", "file:///"); diff --git a/test_util/src/lsp.rs b/test_util/src/lsp.rs index 6adc877cca1bbc..786a2dee5decd7 100644 --- a/test_util/src/lsp.rs +++ b/test_util/src/lsp.rs @@ -560,12 +560,26 @@ impl LspClient { pub fn initialize( &mut self, do_build: impl Fn(&mut InitializeParamsBuilder), + ) { + self.initialize_with_config( + do_build, + json!([{ + "enable": true + }]), + ) + } + + pub fn initialize_with_config( + &mut self, + do_build: impl Fn(&mut InitializeParamsBuilder), + config: Value, ) { let mut builder = InitializeParamsBuilder::new(); builder.set_root_uri(self.context.deno_dir().uri()); do_build(&mut builder); self.write_request("initialize", builder.build()); self.write_notification("initialized", json!({})); + self.handle_configuration_request(config); } pub fn did_open(&mut self, params: Value) -> CollectedDiagnostics { From f8c711935603a2116b408db9b902a14115181c7b Mon Sep 17 00:00:00 2001 From: David Sherret Date: Tue, 14 Mar 2023 16:41:59 -0400 Subject: [PATCH 09/11] Updates. --- cli/lsp/client.rs | 329 +++++++++++++++++-------------------- cli/lsp/completions.rs | 6 +- cli/lsp/diagnostics.rs | 1 + cli/lsp/language_server.rs | 145 ++++++++-------- cli/lsp/testing/server.rs | 2 +- 5 files changed, 234 insertions(+), 249 deletions(-) diff --git a/cli/lsp/client.rs b/cli/lsp/client.rs index db8e3f7d8262a3..91b12983da553c 100644 --- a/cli/lsp/client.rs +++ b/cli/lsp/client.rs @@ -1,13 +1,11 @@ // Copyright 2018-2023 the Deno authors. All rights reserved. MIT license. -use std::future::Future; -use std::pin::Pin; use std::sync::Arc; +use async_trait::async_trait; use deno_core::anyhow::anyhow; use deno_core::anyhow::bail; use deno_core::error::AnyError; -use deno_core::futures::future; use deno_core::serde_json; use deno_core::serde_json::Value; use tower_lsp::lsp_types as lsp; @@ -27,26 +25,6 @@ pub enum TestingNotification { Progress(testing_lsp_custom::TestRunProgressParams), } -/// A client that provides methods that don't need to be awaited. -/// -/// This struct should be used in places where you want to ensure the -/// code doesn't accidentally call into the client while holding an -/// LSP lock, which could then call into the server causing a deadlock. -#[derive(Clone)] -pub struct NotificationOnlyClient(Client); - -impl NotificationOnlyClient { - pub fn send_registry_state_notification( - &self, - params: lsp_custom::RegistryStateNotificationParams, - ) { - let client = self.0.clone(); - tokio::task::spawn(async move { - client.send_registry_state_notification(params).await; - }); - } -} - #[derive(Clone)] pub struct Client(Arc); @@ -65,28 +43,57 @@ impl Client { Self(Arc::new(ReplClient)) } - pub fn as_notification_only(&self) -> NotificationOnlyClient { - NotificationOnlyClient(self.clone()) + /// Gets additional methods that should only be called outside + /// the LSP's lock to prevent deadlocking scenarios. + pub fn when_outside_lsp_lock(&self) -> OutsideLockClient { + OutsideLockClient(self.0.clone()) } - pub async fn publish_diagnostics( + pub fn send_registry_state_notification( &self, - uri: lsp::Url, - diags: Vec, - version: Option, + params: lsp_custom::RegistryStateNotificationParams, ) { - self.0.publish_diagnostics(uri, diags, version).await; + // do on a task in case the caller currently is in the lsp lock + let client = self.0.clone(); + tokio::task::spawn(async move { + client.send_registry_state_notification(params).await; + }); } - pub async fn send_registry_state_notification( + pub fn send_test_notification(&self, params: TestingNotification) { + // do on a task in case the caller currently is in the lsp lock + let client = self.0.clone(); + tokio::task::spawn(async move { + client.send_test_notification(params).await; + }); + } + + pub fn show_message( &self, - params: lsp_custom::RegistryStateNotificationParams, + message_type: lsp::MessageType, + message: impl std::fmt::Display, ) { - self.0.send_registry_state_notification(params).await; + // do on a task in case the caller currently is in the lsp lock + let client = self.0.clone(); + let message = message.to_string(); + tokio::task::spawn(async move { + client.show_message(message_type, message).await; + }); } +} - pub fn send_test_notification(&self, params: TestingNotification) { - self.0.send_test_notification(params); +/// DANGER: The methods on this client should only be called outside +/// the LSP's lock. The reason is you never want to call into the client +/// while holding the lock because the client might call back into the +/// server and cause a deadlock. +pub struct OutsideLockClient(Arc); + +impl OutsideLockClient { + pub async fn register_capability( + &self, + registrations: Vec, + ) -> Result<(), AnyError> { + self.0.register_capability(registrations).await } pub async fn specifier_configurations( @@ -124,216 +131,185 @@ impl Client { self.0.workspace_configuration().await } - pub async fn show_message( + pub async fn publish_diagnostics( &self, - message_type: lsp::MessageType, - message: impl std::fmt::Display, + uri: lsp::Url, + diags: Vec, + version: Option, ) { - self - .0 - .show_message(message_type, format!("{message}")) - .await - } - - pub async fn register_capability( - &self, - registrations: Vec, - ) -> Result<(), AnyError> { - self.0.register_capability(registrations).await + self.0.publish_diagnostics(uri, diags, version).await; } } -type AsyncReturn = Pin + 'static + Send>>; - +#[async_trait] trait ClientTrait: Send + Sync { - fn publish_diagnostics( + async fn publish_diagnostics( &self, uri: lsp::Url, diagnostics: Vec, version: Option, - ) -> AsyncReturn<()>; - fn send_registry_state_notification( + ); + async fn send_registry_state_notification( &self, params: lsp_custom::RegistryStateNotificationParams, - ) -> AsyncReturn<()>; - fn send_test_notification(&self, params: TestingNotification); - fn specifier_configurations( + ); + async fn send_test_notification(&self, params: TestingNotification); + async fn specifier_configurations( &self, uris: Vec, - ) -> AsyncReturn>, AnyError>>; - fn workspace_configuration(&self) -> AsyncReturn>; - fn show_message( - &self, - message_type: lsp::MessageType, - text: String, - ) -> AsyncReturn<()>; - fn register_capability( + ) -> Result>, AnyError>; + async fn workspace_configuration(&self) -> Result; + async fn show_message(&self, message_type: lsp::MessageType, text: String); + async fn register_capability( &self, registrations: Vec, - ) -> AsyncReturn>; + ) -> Result<(), AnyError>; } #[derive(Clone)] struct TowerClient(tower_lsp::Client); +#[async_trait] impl ClientTrait for TowerClient { - fn publish_diagnostics( + async fn publish_diagnostics( &self, uri: lsp::Url, diagnostics: Vec, version: Option, - ) -> AsyncReturn<()> { - let client = self.0.clone(); - Box::pin(async move { - client.publish_diagnostics(uri, diagnostics, version).await - }) + ) { + self.0.publish_diagnostics(uri, diagnostics, version).await } - fn send_registry_state_notification( + async fn send_registry_state_notification( &self, params: lsp_custom::RegistryStateNotificationParams, - ) -> AsyncReturn<()> { - let client = self.0.clone(); - Box::pin(async move { - client - .send_notification::(params) - .await - }) + ) { + self + .0 + .send_notification::(params) + .await } - fn send_test_notification(&self, notification: TestingNotification) { - let client = self.0.clone(); - tokio::task::spawn(async move { - match notification { - TestingNotification::Module(params) => { - client - .send_notification::( - params, - ) - .await - } - TestingNotification::DeleteModule(params) => client - .send_notification::( + async fn send_test_notification(&self, notification: TestingNotification) { + match notification { + TestingNotification::Module(params) => { + self + .0 + .send_notification::( params, ) - .await, - TestingNotification::Progress(params) => client + .await + } + TestingNotification::DeleteModule(params) => self + .0 + .send_notification::( + params, + ) + .await, + TestingNotification::Progress(params) => { + self + .0 .send_notification::( params, ) - .await, + .await } - }); + } } - fn specifier_configurations( + async fn specifier_configurations( &self, uris: Vec, - ) -> AsyncReturn>, AnyError>> - { - eprintln!( - "URIS: {:?}", - uris.iter().map(|i| i.as_str()).collect::>() - ); - let client = self.0.clone(); - Box::pin(async move { - let config_response = client - .configuration( - uris - .into_iter() - .map(|uri| ConfigurationItem { - scope_uri: Some(uri), - section: Some(SETTINGS_SECTION.to_string()), - }) - .collect(), - ) - .await?; - - Ok( - config_response + ) -> Result>, AnyError> { + let config_response = self + .0 + .configuration( + uris .into_iter() - .map(|value| { - serde_json::from_value::(value).map_err(|err| { - anyhow!("Error converting specifier settings: {}", err) - }) + .map(|uri| ConfigurationItem { + scope_uri: Some(uri), + section: Some(SETTINGS_SECTION.to_string()), }) .collect(), ) - }) + .await?; + + Ok( + config_response + .into_iter() + .map(|value| { + serde_json::from_value::(value).map_err(|err| { + anyhow!("Error converting specifier settings: {}", err) + }) + }) + .collect(), + ) } - fn workspace_configuration(&self) -> AsyncReturn> { - eprintln!("WORKSPACE CONFIG"); - let client = self.0.clone(); - Box::pin(async move { - let config_response = client - .configuration(vec![ConfigurationItem { - scope_uri: None, - section: Some(SETTINGS_SECTION.to_string()), - }]) - .await; - match config_response { - Ok(value_vec) => match value_vec.get(0).cloned() { - Some(value) => Ok(value), - None => bail!("Missing response workspace configuration."), - }, - Err(err) => { - bail!("Error getting workspace configuration: {}", err) - } + async fn workspace_configuration(&self) -> Result { + let config_response = self + .0 + .configuration(vec![ConfigurationItem { + scope_uri: None, + section: Some(SETTINGS_SECTION.to_string()), + }]) + .await; + match config_response { + Ok(value_vec) => match value_vec.get(0).cloned() { + Some(value) => Ok(value), + None => bail!("Missing response workspace configuration."), + }, + Err(err) => { + bail!("Error getting workspace configuration: {}", err) } - }) + } } - fn show_message( + async fn show_message( &self, message_type: lsp::MessageType, message: String, - ) -> AsyncReturn<()> { - let client = self.0.clone(); - Box::pin(async move { client.show_message(message_type, message).await }) + ) { + self.0.show_message(message_type, message).await } - fn register_capability( + async fn register_capability( &self, registrations: Vec, - ) -> AsyncReturn> { - let client = self.0.clone(); - Box::pin(async move { - client - .register_capability(registrations) - .await - .map_err(|err| anyhow!("{}", err)) - }) + ) -> Result<(), AnyError> { + self + .0 + .register_capability(registrations) + .await + .map_err(|err| anyhow!("{}", err)) } } #[derive(Clone)] struct ReplClient; +#[async_trait] impl ClientTrait for ReplClient { - fn publish_diagnostics( + async fn publish_diagnostics( &self, _uri: lsp::Url, _diagnostics: Vec, _version: Option, - ) -> AsyncReturn<()> { - Box::pin(future::ready(())) + ) { } - fn send_registry_state_notification( + async fn send_registry_state_notification( &self, _params: lsp_custom::RegistryStateNotificationParams, - ) -> AsyncReturn<()> { - Box::pin(future::ready(())) + ) { } - fn send_test_notification(&self, _params: TestingNotification) {} + async fn send_test_notification(&self, _params: TestingNotification) {} - fn specifier_configurations( + async fn specifier_configurations( &self, uris: Vec, - ) -> AsyncReturn>, AnyError>> - { + ) -> Result>, AnyError> { // all specifiers are enabled for the REPL let settings = uris .into_iter() @@ -344,27 +320,24 @@ impl ClientTrait for ReplClient { }) }) .collect(); - Box::pin(future::ready(Ok(settings))) + Ok(settings) } - fn workspace_configuration(&self) -> AsyncReturn> { - Box::pin(future::ready(Ok( - serde_json::to_value(get_repl_workspace_settings()).unwrap(), - ))) + async fn workspace_configuration(&self) -> Result { + Ok(serde_json::to_value(get_repl_workspace_settings()).unwrap()) } - fn show_message( + async fn show_message( &self, _message_type: lsp::MessageType, _message: String, - ) -> AsyncReturn<()> { - Box::pin(future::ready(())) + ) { } - fn register_capability( + async fn register_capability( &self, _registrations: Vec, - ) -> AsyncReturn> { - Box::pin(future::ready(Ok(()))) + ) -> Result<(), AnyError> { + Ok(()) } } diff --git a/cli/lsp/completions.rs b/cli/lsp/completions.rs index 132d4d84cb0bf5..a767c4d82a6192 100644 --- a/cli/lsp/completions.rs +++ b/cli/lsp/completions.rs @@ -1,6 +1,6 @@ // Copyright 2018-2023 the Deno authors. All rights reserved. MIT license. -use super::client::NotificationOnlyClient; +use super::client::Client; use super::config::ConfigSnapshot; use super::documents::Documents; use super::lsp_custom; @@ -48,7 +48,7 @@ pub struct CompletionItemData { async fn check_auto_config_registry( url_str: &str, config: &ConfigSnapshot, - client: &NotificationOnlyClient, + client: &Client, module_registries: &ModuleRegistry, ) { // check to see if auto discovery is enabled @@ -137,7 +137,7 @@ pub async fn get_import_completions( specifier: &ModuleSpecifier, position: &lsp::Position, config: &ConfigSnapshot, - client: &NotificationOnlyClient, + client: &Client, module_registries: &ModuleRegistry, documents: &Documents, maybe_import_map: Option>, diff --git a/cli/lsp/diagnostics.rs b/cli/lsp/diagnostics.rs index 9b12a022c443d3..3ac15505f46905 100644 --- a/cli/lsp/diagnostics.rs +++ b/cli/lsp/diagnostics.rs @@ -88,6 +88,7 @@ impl DiagnosticsPublisher { self .client + .when_outside_lsp_lock() .publish_diagnostics(specifier, version_diagnostics.clone(), version) .await; } diff --git a/cli/lsp/language_server.rs b/cli/lsp/language_server.rs index b866056f863236..fc87001af35b1f 100644 --- a/cli/lsp/language_server.rs +++ b/cli/lsp/language_server.rs @@ -214,8 +214,7 @@ impl LanguageServer { .read() .await .client - .show_message(MessageType::WARNING, err) - .await; + .show_message(MessageType::WARNING, err); return Err(LspError::internal_error()); } } @@ -233,8 +232,7 @@ impl LanguageServer { .read() .await .client - .show_message(MessageType::WARNING, err) - .await; + .show_message(MessageType::WARNING, err); } // do npm resolution in a write—we should have everything // cached by this point anyway @@ -367,6 +365,7 @@ impl LanguageServer { let mut touched = false; if let Some(specifiers) = specifiers { let configs_result = client + .when_outside_lsp_lock() .specifier_configurations( specifiers .iter() @@ -1008,7 +1007,7 @@ impl Inner { tsconfig.merge(&unstable_libs); } if let Err(err) = self.merge_user_tsconfig(&mut tsconfig) { - self.client.show_message(MessageType::WARNING, err).await; + self.client.show_message(MessageType::WARNING, err); } let _ok: bool = self .ts_server @@ -1088,16 +1087,16 @@ impl Inner { self.update_debug_flag(); // Check to see if we need to change the cache path if let Err(err) = self.update_cache() { - self.client.show_message(MessageType::WARNING, err).await; + self.client.show_message(MessageType::WARNING, err); } if let Err(err) = self.update_config_file() { - self.client.show_message(MessageType::WARNING, err).await; + self.client.show_message(MessageType::WARNING, err); } if let Err(err) = self.update_package_json() { - self.client.show_message(MessageType::WARNING, err).await; + self.client.show_message(MessageType::WARNING, err); } if let Err(err) = self.update_tsconfig().await { - self.client.show_message(MessageType::WARNING, err).await; + self.client.show_message(MessageType::WARNING, err); } if capabilities.code_action_provider.is_some() { @@ -1114,11 +1113,11 @@ impl Inner { // Check to see if we need to setup the import map if let Err(err) = self.update_import_map().await { - self.client.show_message(MessageType::WARNING, err).await; + self.client.show_message(MessageType::WARNING, err); } // Check to see if we need to setup any module registries if let Err(err) = self.update_registries().await { - self.client.show_message(MessageType::WARNING, err).await; + self.client.show_message(MessageType::WARNING, err); } // self.refresh_documents_config(); // todo(THIS PR): REMOVE @@ -1142,46 +1141,6 @@ impl Inner { ); } - async fn initialized(&mut self, _: InitializedParams) { - if self - .config - .client_capabilities - .workspace_did_change_watched_files - { - // we are going to watch all the JSON files in the workspace, and the - // notification handler will pick up any of the changes of those files we - // are interested in. - let watch_registration_options = - DidChangeWatchedFilesRegistrationOptions { - watchers: vec![FileSystemWatcher { - glob_pattern: "**/*.{json,jsonc}".to_string(), - kind: Some(WatchKind::Change), - }], - }; - let registration = Registration { - id: "workspace/didChangeWatchedFiles".to_string(), - method: "workspace/didChangeWatchedFiles".to_string(), - register_options: Some( - serde_json::to_value(watch_registration_options).unwrap(), - ), - }; - if let Err(err) = - self.client.register_capability(vec![registration]).await - { - warn!("Client errored on capabilities.\n{:#}", err); - } - } - - if self.config.client_capabilities.testing_api { - let test_server = testing::TestServer::new( - self.client.clone(), - self.performance.clone(), - self.config.root_uri.clone(), - ); - self.maybe_testing_server = Some(test_server); - } - } - async fn shutdown(&self) -> LspResult<()> { Ok(()) } @@ -1300,22 +1259,22 @@ impl Inner { self.update_debug_flag(); if let Err(err) = self.update_cache() { - self.client.show_message(MessageType::WARNING, err).await; + self.client.show_message(MessageType::WARNING, err); } if let Err(err) = self.update_registries().await { - self.client.show_message(MessageType::WARNING, err).await; + self.client.show_message(MessageType::WARNING, err); } if let Err(err) = self.update_config_file() { - self.client.show_message(MessageType::WARNING, err).await; + self.client.show_message(MessageType::WARNING, err); } if let Err(err) = self.update_package_json() { - self.client.show_message(MessageType::WARNING, err).await; + self.client.show_message(MessageType::WARNING, err); } if let Err(err) = self.update_import_map().await { - self.client.show_message(MessageType::WARNING, err).await; + self.client.show_message(MessageType::WARNING, err); } if let Err(err) = self.update_tsconfig().await { - self.client.show_message(MessageType::WARNING, err).await; + self.client.show_message(MessageType::WARNING, err); } self.refresh_documents_config(); @@ -1342,10 +1301,10 @@ impl Inner { if let Some(config_file) = &self.maybe_config_file { if changes.contains(&config_file.specifier) { if let Err(err) = self.update_config_file() { - self.client.show_message(MessageType::WARNING, err).await; + self.client.show_message(MessageType::WARNING, err); } if let Err(err) = self.update_tsconfig().await { - self.client.show_message(MessageType::WARNING, err).await; + self.client.show_message(MessageType::WARNING, err); } touched = true; } @@ -1354,7 +1313,7 @@ impl Inner { // always update the package json if the deno config changes if touched || changes.contains(&package_json.specifier()) { if let Err(err) = self.update_package_json() { - self.client.show_message(MessageType::WARNING, err).await; + self.client.show_message(MessageType::WARNING, err); } touched = true; } @@ -1364,7 +1323,7 @@ impl Inner { if let Some(import_map_uri) = &self.maybe_import_map_uri { if touched || changes.contains(import_map_uri) { if let Err(err) = self.update_import_map().await { - self.client.show_message(MessageType::WARNING, err).await; + self.client.show_message(MessageType::WARNING, err); } touched = true; } @@ -1512,7 +1471,7 @@ impl Inner { Ok(Some(text_edits)) } } else { - self.client.show_message(MessageType::WARNING, format!("Unable to format \"{specifier}\". Likely due to unrecoverable syntax errors in the file.")).await; + self.client.show_message(MessageType::WARNING, format!("Unable to format \"{specifier}\". Likely due to unrecoverable syntax errors in the file.")); Ok(None) } } @@ -2140,7 +2099,7 @@ impl Inner { &specifier, ¶ms.text_document_position.position, &self.config.snapshot(), - &self.client.as_notification_only(), + &self.client, &self.module_registries, &self.documents, self.maybe_import_map.clone(), @@ -2841,8 +2800,54 @@ impl tower_lsp::LanguageServer for LanguageServer { language_server.initialize(params).await } - async fn initialized(&self, params: InitializedParams) { - self.0.write().await.initialized(params).await; + async fn initialized(&self, _: InitializedParams) { + let mut maybe_registration = None; + let client = { + let mut ls = self.0.write().await; + if ls + .config + .client_capabilities + .workspace_did_change_watched_files + { + // we are going to watch all the JSON files in the workspace, and the + // notification handler will pick up any of the changes of those files we + // are interested in. + let watch_registration_options = + DidChangeWatchedFilesRegistrationOptions { + watchers: vec![FileSystemWatcher { + glob_pattern: "**/*.{json,jsonc}".to_string(), + kind: Some(WatchKind::Change), + }], + }; + maybe_registration = Some(Registration { + id: "workspace/didChangeWatchedFiles".to_string(), + method: "workspace/didChangeWatchedFiles".to_string(), + register_options: Some( + serde_json::to_value(watch_registration_options).unwrap(), + ), + }); + } + + if ls.config.client_capabilities.testing_api { + let test_server = testing::TestServer::new( + ls.client.clone(), + ls.performance.clone(), + ls.config.root_uri.clone(), + ); + ls.maybe_testing_server = Some(test_server); + } + ls.client.clone() + }; + + if let Some(registration) = maybe_registration { + if let Err(err) = client + .when_outside_lsp_lock() + .register_capability(vec![registration]) + .await + { + warn!("Client errored on capabilities.\n{:#}", err); + } + } if !self.refresh_specifiers_from_client().await { // force update config @@ -2889,7 +2894,10 @@ impl tower_lsp::LanguageServer for LanguageServer { // retrieve the specifier settings outside the lock if // they haven't been asked for yet if !had_specifier_settings { - let response = client.specifier_configuration(&client_uri).await; + let response = client + .when_outside_lsp_lock() + .specifier_configuration(&client_uri) + .await; let mut ls = self.0.write().await; match response { Ok(specifier_settings) => { @@ -2953,7 +2961,10 @@ impl tower_lsp::LanguageServer for LanguageServer { // received and acquiring the lock, but most likely there // won't be any racing here. let client_workspace_config = if has_workspace_capability { - let config_response = client.workspace_configuration().await; + let config_response = client + .when_outside_lsp_lock() + .workspace_configuration() + .await; match config_response { Ok(value) => Some(value), Err(err) => { diff --git a/cli/lsp/testing/server.rs b/cli/lsp/testing/server.rs index 66f66ed1d943cf..61db4316a28148 100644 --- a/cli/lsp/testing/server.rs +++ b/cli/lsp/testing/server.rs @@ -156,7 +156,7 @@ impl TestServer { match run.exec(&client, maybe_root_uri.as_ref()).await { Ok(_) => (), Err(err) => { - client.show_message(lsp::MessageType::ERROR, err).await; + client.show_message(lsp::MessageType::ERROR, err); } } client.send_test_notification(TestingNotification::Progress( From 83bb2cacd4e5e5c4b614891072e94784ccff6d48 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Wed, 15 Mar 2023 09:45:02 -0400 Subject: [PATCH 10/11] Use spawn_local --- cli/lsp/client.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/cli/lsp/client.rs b/cli/lsp/client.rs index 91b12983da553c..563670368716b6 100644 --- a/cli/lsp/client.rs +++ b/cli/lsp/client.rs @@ -55,7 +55,7 @@ impl Client { ) { // do on a task in case the caller currently is in the lsp lock let client = self.0.clone(); - tokio::task::spawn(async move { + tokio::task::spawn_local(async move { client.send_registry_state_notification(params).await; }); } @@ -63,7 +63,7 @@ impl Client { pub fn send_test_notification(&self, params: TestingNotification) { // do on a task in case the caller currently is in the lsp lock let client = self.0.clone(); - tokio::task::spawn(async move { + tokio::task::spawn_local(async move { client.send_test_notification(params).await; }); } @@ -76,7 +76,7 @@ impl Client { // do on a task in case the caller currently is in the lsp lock let client = self.0.clone(); let message = message.to_string(); - tokio::task::spawn(async move { + tokio::task::spawn_local(async move { client.show_message(message_type, message).await; }); } From 7a0206005b117fe0e3d220d021c67111d9c77e83 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Wed, 15 Mar 2023 09:58:23 -0400 Subject: [PATCH 11/11] Revert --- cli/lsp/client.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/cli/lsp/client.rs b/cli/lsp/client.rs index 563670368716b6..91b12983da553c 100644 --- a/cli/lsp/client.rs +++ b/cli/lsp/client.rs @@ -55,7 +55,7 @@ impl Client { ) { // do on a task in case the caller currently is in the lsp lock let client = self.0.clone(); - tokio::task::spawn_local(async move { + tokio::task::spawn(async move { client.send_registry_state_notification(params).await; }); } @@ -63,7 +63,7 @@ impl Client { pub fn send_test_notification(&self, params: TestingNotification) { // do on a task in case the caller currently is in the lsp lock let client = self.0.clone(); - tokio::task::spawn_local(async move { + tokio::task::spawn(async move { client.send_test_notification(params).await; }); } @@ -76,7 +76,7 @@ impl Client { // do on a task in case the caller currently is in the lsp lock let client = self.0.clone(); let message = message.to_string(); - tokio::task::spawn_local(async move { + tokio::task::spawn(async move { client.show_message(message_type, message).await; }); }