From 7070b8ed50f13d95d926b19ed7d7ce9fc0d6d4f3 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Wed, 15 Mar 2023 10:34:23 -0400 Subject: [PATCH] fix(lsp): avoid calling client while holding lock (#18197) --- cli/lsp/client.rs | 304 +++++++-------- cli/lsp/completions.rs | 18 +- cli/lsp/config.rs | 91 ++--- cli/lsp/diagnostics.rs | 14 +- cli/lsp/documents.rs | 8 +- cli/lsp/language_server.rs | 566 ++++++++++++++++------------ cli/lsp/testing/server.rs | 2 +- cli/lsp/urls.rs | 46 ++- cli/tests/integration/cert_tests.rs | 2 +- cli/tests/integration/lsp_tests.rs | 144 ++++--- cli/tests/integration/run_tests.rs | 4 +- cli/util/path.rs | 27 -- test_util/src/lsp.rs | 23 ++ 13 files changed, 674 insertions(+), 575 deletions(-) diff --git a/cli/lsp/client.rs b/cli/lsp/client.rs index cdef1cfbf5..91b12983da 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; @@ -45,24 +43,57 @@ impl Client { Self(Arc::new(ReplClient)) } - pub async fn publish_diagnostics( - &self, - uri: lsp::Url, - diags: Vec, - version: Option, - ) { - self.0.publish_diagnostics(uri, diags, version).await; + /// 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 send_registry_state_notification( + pub fn send_registry_state_notification( &self, params: lsp_custom::RegistryStateNotificationParams, ) { - 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(); + tokio::task::spawn(async move { + client.send_registry_state_notification(params).await; + }); } pub fn send_test_notification(&self, params: TestingNotification) { - self.0.send_test_notification(params); + // 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, + message_type: lsp::MessageType, + message: impl std::fmt::Display, + ) { + // 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; + }); + } +} + +/// 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( @@ -100,211 +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>> - { - 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> { - 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() @@ -315,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 3651fbeec0..a767c4d82a 100644 --- a/cli/lsp/completions.rs +++ b/cli/lsp/completions.rs @@ -48,7 +48,7 @@ pub struct CompletionItemData { async fn check_auto_config_registry( url_str: &str, config: &ConfigSnapshot, - client: Client, + client: &Client, 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: &Client, module_registries: &ModuleRegistry, documents: &Documents, maybe_import_map: Option>, diff --git a/cli/lsp/config.rs b/cli/lsp/config.rs index 3da9f7a072..60b975a44b 100644 --- a/cli/lsp/config.rs +++ b/cli/lsp/config.rs @@ -1,8 +1,6 @@ // 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; use deno_core::error::AnyError; use deno_core::serde::Deserialize; @@ -10,6 +8,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; @@ -226,7 +225,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. @@ -388,7 +387,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,42 +395,33 @@ 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())); } } } - 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, } #[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 +468,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())); } } } @@ -491,7 +481,7 @@ impl Config { .settings .specifiers .get(specifier) - .map(|(_, s)| s.enable) + .map(|settings| settings.enable) .unwrap_or_else(|| self.settings.workspace.enable) } @@ -500,7 +490,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 } @@ -554,13 +544,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; } } @@ -582,8 +574,6 @@ impl Config { workspace: ModuleSpecifier, 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 +582,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,38 +591,33 @@ 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 } - 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 } } @@ -678,8 +663,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)); diff --git a/cli/lsp/diagnostics.rs b/cli/lsp/diagnostics.rs index 8c1c91da05..3ac15505f4 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; } @@ -1177,14 +1178,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/documents.rs b/cli/lsp/documents.rs index ff384bbf1a..9426378d50 100644 --- a/cli/lsp/documents.rs +++ b/cli/lsp/documents.rs @@ -1184,12 +1184,8 @@ impl Documents { 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() } diff --git a/cli/lsp/language_server.rs b/cli/lsp/language_server.rs index 50cc0326e3..fc87001af3 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; @@ -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 @@ -323,6 +321,92 @@ impl LanguageServer { None => Err(LspError::invalid_params("Missing parameters")), } } + + pub async fn refresh_specifiers_from_client(&self) -> bool { + 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 + .when_outside_lsp_lock() + .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 (value, internal_uri) in + configs.into_iter().zip(specifiers.into_iter().map(|s| s.1)) + { + match value { + Ok(specifier_settings) => { + 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(); + } + } + touched + } } fn create_lsp_structs( @@ -923,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 @@ -978,8 +1062,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| { @@ -990,7 +1073,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); @@ -999,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() { @@ -1025,20 +1113,14 @@ 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.documents.update_config( - self.maybe_import_map.clone(), - self.maybe_config_file.as_ref(), - self.maybe_package_json.as_ref(), - self.npm_api.clone(), - self.npm_resolution.clone(), - ); + // self.refresh_documents_config(); // todo(THIS PR): REMOVE self.assets.intitialize(self.snapshot()).await; self.performance.measure(mark); @@ -1049,47 +1131,14 @@ 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); - } - } - self.config.update_enabled_paths(self.client.clone()).await; - - 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); - } - - lsp_log!("Server ready."); + fn refresh_documents_config(&mut self) { + self.documents.update_config( + self.maybe_import_map.clone(), + self.maybe_config_file.as_ref(), + self.maybe_package_json.as_ref(), + self.npm_api.clone(), + self.npm_resolution.clone(), + ); } async fn shutdown(&self) -> LspResult<()> { @@ -1130,7 +1179,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, @@ -1166,7 +1217,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); @@ -1206,31 +1259,25 @@ 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.documents.update_config( - self.maybe_import_map.clone(), - self.maybe_config_file.as_ref(), - self.maybe_package_json.as_ref(), - self.npm_api.clone(), - self.npm_resolution.clone(), - ); + self.refresh_documents_config(); self.send_diagnostics_update(); self.send_testing_update(); @@ -1247,17 +1294,17 @@ 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 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; } @@ -1266,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; } @@ -1276,19 +1323,13 @@ 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; } } 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_api.clone(), - self.npm_resolution.clone(), - ); + self.refresh_documents_config(); self.refresh_npm_specifiers().await; self.diagnostics_server.invalidate_all(); self.restart_ts_server().await; @@ -1298,18 +1339,20 @@ 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 .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 { @@ -1323,14 +1366,15 @@ impl Inner { } self.config.workspace_folders = Some(workspace_folders); - self.performance.measure(mark); } async fn document_symbol( &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) { @@ -1368,7 +1412,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), @@ -1425,15 +1471,16 @@ 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) } } 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) { @@ -1518,7 +1565,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) { @@ -1778,7 +1827,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() @@ -1838,9 +1889,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) { @@ -1882,9 +1934,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) { @@ -1938,9 +1991,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) { @@ -1977,9 +2031,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) { @@ -2024,9 +2079,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) { @@ -2043,7 +2099,7 @@ impl Inner { &specifier, ¶ms.text_document_position.position, &self.config.snapshot(), - self.client.clone(), + &self.client, &self.module_registries, &self.documents, self.maybe_import_map.clone(), @@ -2183,9 +2239,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) { @@ -2229,7 +2286,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) { @@ -2273,7 +2332,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) { @@ -2319,7 +2380,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) { @@ -2366,9 +2429,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) { @@ -2434,9 +2498,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) { @@ -2486,7 +2551,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) { @@ -2524,7 +2591,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) { @@ -2566,7 +2635,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) { @@ -2609,9 +2680,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) { @@ -2728,8 +2800,61 @@ 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 + self.0.write().await.refresh_documents_config(); + } + + lsp_log!("Server ready."); } async fn shutdown(&self) -> LspResult<()> { @@ -2744,11 +2869,12 @@ 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, LspUrlKind::File); let document = inner.did_open(&specifier, params).await; let has_specifier_settings = inner.config.has_specifier_settings(&specifier); @@ -2762,39 +2888,38 @@ 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 + .when_outside_lsp_lock() + .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(); + ls.send_testing_update(); + } } } @@ -2815,66 +2940,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.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 @@ -2884,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) => { @@ -2915,19 +2995,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 (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); + (ls.performance.clone(), 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.diagnostics_server.invalidate_all(); - ls.send_diagnostics_update(); - } - }); + + self.refresh_specifiers_from_client().await; + performance.measure(mark); } async fn document_symbol( @@ -3106,7 +3184,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); } @@ -3116,7 +3196,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] @@ -3190,7 +3270,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) @@ -3251,7 +3333,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/testing/server.rs b/cli/lsp/testing/server.rs index 66f66ed1d9..61db4316a2 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( diff --git a/cli/lsp/urls.rs b/cli/lsp/urls.rs index 4fba0c9ff3..14717d7152 100644 --- a/cli/lsp/urls.rs +++ b/cli/lsp/urls.rs @@ -80,8 +80,14 @@ 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 +/// 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>); @@ -142,16 +148,26 @@ 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 { - if let Some(specifier) = self.0.lock().get_specifier(url).cloned() { - return specifier; + /// + /// 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() { + match kind { + LspUrlKind::Folder => Url::from_directory_path(path).unwrap(), + LspUrlKind::File => Url::from_file_path(path).unwrap(), + } + } else { + url.clone() + }; + inner.put(specifier.clone(), url.clone()); + specifier } - if url.scheme() == "file" { - if let Ok(path) = url.to_file_path() { - return Url::from_file_path(path).unwrap(); - } - } - url.clone() } } @@ -181,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); } @@ -196,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); } @@ -210,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); } @@ -222,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(); @@ -237,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/cert_tests.rs b/cli/tests/integration/cert_tests.rs index 0031367589..d3da6d75af 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 981488a67e..2296181ffa 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,26 +932,27 @@ 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(); 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": { @@ -1216,7 +1217,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 +5189,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(); @@ -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, @@ -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(); } @@ -5826,7 +5850,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 +5903,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 +6095,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#"{ @@ -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), @@ -6635,7 +6659,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 +6700,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 +6838,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 3a19564a39..f57dd98767 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", ""); diff --git a/cli/util/path.rs b/cli/util/path.rs index d073d80bd5..69c52bf6ed 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 7578aedf32..786a2dee5d 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() } @@ -551,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 {