From 97d1635343dc6e93c8dcf4b116922de5b9c57af3 Mon Sep 17 00:00:00 2001 From: Nayeem Rahman Date: Wed, 28 Aug 2024 05:15:48 +0100 Subject: [PATCH] fix(lsp): panic on url_to_uri() (#25238) --- cli/lsp/analysis.rs | 4 +- cli/lsp/client.rs | 9 +- cli/lsp/config.rs | 2 +- cli/lsp/diagnostics.rs | 39 ++++---- cli/lsp/language_server.rs | 115 +++++++++++++---------- cli/lsp/repl.rs | 2 +- cli/lsp/testing/definitions.rs | 29 +++--- cli/lsp/testing/execution.rs | 30 +++--- cli/lsp/testing/server.rs | 30 +++--- cli/lsp/tsc.rs | 57 ++++++----- cli/lsp/urls.rs | 166 +++++++++++++-------------------- 11 files changed, 232 insertions(+), 251 deletions(-) diff --git a/cli/lsp/analysis.rs b/cli/lsp/analysis.rs index bea6268419..eeab796bca 100644 --- a/cli/lsp/analysis.rs +++ b/cli/lsp/analysis.rs @@ -751,7 +751,7 @@ impl CodeActionCollection { .as_ref() .and_then(|d| serde_json::from_value::>(d.clone()).ok()) { - let uri = url_to_uri(specifier); + let uri = url_to_uri(specifier)?; for quick_fix in data_quick_fixes { let mut changes = HashMap::new(); changes.insert( @@ -797,7 +797,7 @@ impl CodeActionCollection { maybe_text_info: Option<&SourceTextInfo>, maybe_parsed_source: Option<&deno_ast::ParsedSource>, ) -> Result<(), AnyError> { - let uri = url_to_uri(specifier); + let uri = url_to_uri(specifier)?; let code = diagnostic .code .as_ref() diff --git a/cli/lsp/client.rs b/cli/lsp/client.rs index 6ed0c04301..b3f0d64fa6 100644 --- a/cli/lsp/client.rs +++ b/cli/lsp/client.rs @@ -8,6 +8,7 @@ use deno_core::anyhow::bail; use deno_core::error::AnyError; use deno_core::serde_json::json; use deno_core::unsync::spawn; +use lsp_types::Uri; use tower_lsp::lsp_types as lsp; use tower_lsp::lsp_types::ConfigurationItem; @@ -17,7 +18,6 @@ use super::config::WorkspaceSettings; use super::config::SETTINGS_SECTION; use super::lsp_custom; use super::testing::lsp_custom as testing_lsp_custom; -use super::urls::LspClientUrl; #[derive(Debug)] pub enum TestingNotification { @@ -52,14 +52,11 @@ impl Client { pub async fn publish_diagnostics( &self, - uri: LspClientUrl, + uri: Uri, diags: Vec, version: Option, ) { - self - .0 - .publish_diagnostics(uri.to_uri(), diags, version) - .await; + self.0.publish_diagnostics(uri, diags, version).await; } pub fn send_registry_state_notification( diff --git a/cli/lsp/config.rs b/cli/lsp/config.rs index 4a279a7c41..ba5cc3ac44 100644 --- a/cli/lsp/config.rs +++ b/cli/lsp/config.rs @@ -850,7 +850,7 @@ impl Config { let mut config = Self::default(); let mut folders = vec![]; for root_url in root_urls { - let root_uri = url_to_uri(&root_url); + let root_uri = url_to_uri(&root_url).unwrap(); let name = root_url.path_segments().and_then(|s| s.last()); let name = name.unwrap_or_default().to_string(); folders.push(( diff --git a/cli/lsp/diagnostics.rs b/cli/lsp/diagnostics.rs index d871ffbe46..9d9a0ae458 100644 --- a/cli/lsp/diagnostics.rs +++ b/cli/lsp/diagnostics.rs @@ -13,7 +13,6 @@ use super::performance::Performance; use super::tsc; use super::tsc::TsServer; use super::urls::url_to_uri; -use super::urls::LspClientUrl; use super::urls::LspUrlMap; use crate::graph_util; @@ -164,15 +163,14 @@ impl DiagnosticsPublisher { .state .update(&record.specifier, version, &all_specifier_diagnostics); let file_referrer = documents.get_file_referrer(&record.specifier); + let Ok(uri) = + url_map.specifier_to_uri(&record.specifier, file_referrer.as_deref()) + else { + continue; + }; self .client - .publish_diagnostics( - url_map - .normalize_specifier(&record.specifier, file_referrer.as_deref()) - .unwrap_or(LspClientUrl::new(record.specifier)), - all_specifier_diagnostics, - version, - ) + .publish_diagnostics(uri, all_specifier_diagnostics, version) .await; messages_sent += 1; } @@ -195,15 +193,14 @@ impl DiagnosticsPublisher { // clear out any diagnostics for this specifier self.state.update(specifier, removed_value.version, &[]); let file_referrer = documents.get_file_referrer(specifier); + let Ok(uri) = + url_map.specifier_to_uri(specifier, file_referrer.as_deref()) + else { + continue; + }; self .client - .publish_diagnostics( - url_map - .normalize_specifier(specifier, file_referrer.as_deref()) - .unwrap_or_else(|_| LspClientUrl::new(specifier.clone())), - Vec::new(), - removed_value.version, - ) + .publish_diagnostics(uri, Vec::new(), removed_value.version) .await; messages_sent += 1; } @@ -1074,7 +1071,7 @@ impl DenoDiagnostic { diagnostics: Some(vec![diagnostic.clone()]), edit: Some(lsp::WorkspaceEdit { changes: Some(HashMap::from([( - url_to_uri(specifier), + url_to_uri(specifier)?, vec![lsp::TextEdit { new_text: format!("\"{to}\""), range: diagnostic.range, @@ -1091,7 +1088,7 @@ impl DenoDiagnostic { diagnostics: Some(vec![diagnostic.clone()]), edit: Some(lsp::WorkspaceEdit { changes: Some(HashMap::from([( - url_to_uri(specifier), + url_to_uri(specifier)?, vec![lsp::TextEdit { new_text: " with { type: \"json\" }".to_string(), range: lsp::Range { @@ -1142,7 +1139,7 @@ impl DenoDiagnostic { diagnostics: Some(vec![diagnostic.clone()]), edit: Some(lsp::WorkspaceEdit { changes: Some(HashMap::from([( - url_to_uri(specifier), + url_to_uri(specifier)?, vec![lsp::TextEdit { new_text: format!( "\"{}\"", @@ -1168,7 +1165,7 @@ impl DenoDiagnostic { diagnostics: Some(vec![diagnostic.clone()]), edit: Some(lsp::WorkspaceEdit { changes: Some(HashMap::from([( - url_to_uri(specifier), + url_to_uri(specifier)?, vec![lsp::TextEdit { new_text: format!( "\"{}\"", @@ -1194,7 +1191,7 @@ impl DenoDiagnostic { diagnostics: Some(vec![diagnostic.clone()]), edit: Some(lsp::WorkspaceEdit { changes: Some(HashMap::from([( - url_to_uri(specifier), + url_to_uri(specifier)?, vec![lsp::TextEdit { new_text: format!("\"node:{}\"", data.specifier), range: diagnostic.range, @@ -1642,7 +1639,7 @@ mod tests { fn mock_config() -> Config { let root_url = resolve_url("file:///").unwrap(); - let root_uri = url_to_uri(&root_url); + let root_uri = url_to_uri(&root_url).unwrap(); Config { settings: Arc::new(Settings { unscoped: Arc::new(WorkspaceSettings { diff --git a/cli/lsp/language_server.rs b/cli/lsp/language_server.rs index 86277ab416..37e1aa0be2 100644 --- a/cli/lsp/language_server.rs +++ b/cli/lsp/language_server.rs @@ -32,6 +32,7 @@ use std::collections::VecDeque; use std::env; use std::fmt::Write as _; use std::path::PathBuf; +use std::str::FromStr; use std::sync::Arc; use tokio::sync::mpsc::unbounded_channel; use tokio::sync::mpsc::UnboundedReceiver; @@ -723,7 +724,9 @@ impl Inner { .into_iter() .map(|folder| { ( - self.url_map.normalize_url(&folder.uri, LspUrlKind::Folder), + self + .url_map + .uri_to_specifier(&folder.uri, LspUrlKind::Folder), folder, ) }) @@ -735,7 +738,7 @@ impl Inner { if let Some(root_uri) = params.root_uri { if !workspace_folders.iter().any(|(_, f)| f.uri == root_uri) { let root_url = - self.url_map.normalize_url(&root_uri, LspUrlKind::Folder); + self.url_map.uri_to_specifier(&root_uri, LspUrlKind::Folder); let name = root_url.path_segments().and_then(|s| s.last()); let name = name.unwrap_or_default().to_string(); workspace_folders.insert( @@ -1043,7 +1046,7 @@ impl Inner { .filter(|s| self.documents.is_valid_file_referrer(s)); let specifier = self .url_map - .normalize_url(¶ms.text_document.uri, LspUrlKind::File); + .uri_to_specifier(¶ms.text_document.uri, LspUrlKind::File); let document = self.documents.open( specifier.clone(), params.text_document.version, @@ -1065,7 +1068,7 @@ impl Inner { let mark = self.performance.mark_with_args("lsp.did_change", ¶ms); let specifier = self .url_map - .normalize_url(¶ms.text_document.uri, LspUrlKind::File); + .uri_to_specifier(¶ms.text_document.uri, LspUrlKind::File); match self.documents.change( &specifier, params.text_document.version, @@ -1102,7 +1105,7 @@ impl Inner { let _mark = self.performance.measure_scope("lsp.did_save"); let specifier = self .url_map - .normalize_url(¶ms.text_document.uri, LspUrlKind::File); + .uri_to_specifier(¶ms.text_document.uri, LspUrlKind::File); self.documents.save(&specifier); if !self .config @@ -1148,7 +1151,7 @@ impl Inner { } let specifier = self .url_map - .normalize_url(¶ms.text_document.uri, LspUrlKind::File); + .uri_to_specifier(¶ms.text_document.uri, LspUrlKind::File); self.diagnostics_state.clear(&specifier); if self.is_diagnosable(&specifier) { self.refresh_npm_specifiers().await; @@ -1202,7 +1205,7 @@ impl Inner { let changes = params .changes .into_iter() - .map(|e| (self.url_map.normalize_url(&e.uri, LspUrlKind::File), e)) + .map(|e| (self.url_map.uri_to_specifier(&e.uri, LspUrlKind::File), e)) .collect::>(); if changes .iter() @@ -1221,7 +1224,7 @@ impl Inner { _ => return None, }; Some(lsp_custom::DenoConfigurationChangeEvent { - scope_uri: url_to_uri(t.0), + scope_uri: url_to_uri(t.0).ok()?, file_uri: e.uri.clone(), typ: lsp_custom::DenoConfigurationChangeType::from_file_change_type( e.typ, @@ -1256,7 +1259,7 @@ impl Inner { _ => return None, }; Some(lsp_custom::DenoConfigurationChangeEvent { - scope_uri: url_to_uri(t.0), + scope_uri: url_to_uri(t.0).ok()?, file_uri: e.uri.clone(), typ: lsp_custom::DenoConfigurationChangeType::from_file_change_type( e.typ, @@ -1282,7 +1285,7 @@ impl Inner { ) -> LspResult> { let specifier = self .url_map - .normalize_url(¶ms.text_document.uri, LspUrlKind::File); + .uri_to_specifier(¶ms.text_document.uri, LspUrlKind::File); if !self.is_diagnosable(&specifier) || !self.config.specifier_enabled(&specifier) { @@ -1326,7 +1329,7 @@ impl Inner { .filter(|s| self.documents.is_valid_file_referrer(s)); let mut specifier = self .url_map - .normalize_url(¶ms.text_document.uri, LspUrlKind::File); + .uri_to_specifier(¶ms.text_document.uri, LspUrlKind::File); // skip formatting any files ignored by the config file if !self .config @@ -1441,7 +1444,7 @@ impl Inner { } async fn hover(&self, params: HoverParams) -> LspResult> { - let specifier = self.url_map.normalize_url( + let specifier = self.url_map.uri_to_specifier( ¶ms.text_document_position_params.text_document.uri, LspUrlKind::File, ); @@ -1574,7 +1577,7 @@ impl Inner { ) -> LspResult> { let specifier = self .url_map - .normalize_url(¶ms.text_document.uri, LspUrlKind::File); + .uri_to_specifier(¶ms.text_document.uri, LspUrlKind::File); if !self.is_diagnosable(&specifier) || !self.config.specifier_enabled(&specifier) { @@ -1918,7 +1921,7 @@ impl Inner { ) -> LspResult>> { let specifier = self .url_map - .normalize_url(¶ms.text_document.uri, LspUrlKind::File); + .uri_to_specifier(¶ms.text_document.uri, LspUrlKind::File); if !self.is_diagnosable(&specifier) || !self.config.specifier_enabled(&specifier) { @@ -2004,7 +2007,7 @@ impl Inner { &self, params: DocumentHighlightParams, ) -> LspResult>> { - let specifier = self.url_map.normalize_url( + let specifier = self.url_map.uri_to_specifier( ¶ms.text_document_position_params.text_document.uri, LspUrlKind::File, ); @@ -2048,7 +2051,7 @@ impl Inner { &self, params: ReferenceParams, ) -> LspResult>> { - let specifier = self.url_map.normalize_url( + let specifier = self.url_map.uri_to_specifier( ¶ms.text_document_position.text_document.uri, LspUrlKind::File, ); @@ -2104,7 +2107,7 @@ impl Inner { &self, params: GotoDefinitionParams, ) -> LspResult> { - let specifier = self.url_map.normalize_url( + let specifier = self.url_map.uri_to_specifier( ¶ms.text_document_position_params.text_document.uri, LspUrlKind::File, ); @@ -2143,7 +2146,7 @@ impl Inner { &self, params: GotoTypeDefinitionParams, ) -> LspResult> { - let specifier = self.url_map.normalize_url( + let specifier = self.url_map.uri_to_specifier( ¶ms.text_document_position_params.text_document.uri, LspUrlKind::File, ); @@ -2189,7 +2192,7 @@ impl Inner { &self, params: CompletionParams, ) -> LspResult> { - let specifier = self.url_map.normalize_url( + let specifier = self.url_map.uri_to_specifier( ¶ms.text_document_position.text_document.uri, LspUrlKind::File, ); @@ -2378,7 +2381,7 @@ impl Inner { &self, params: GotoImplementationParams, ) -> LspResult> { - let specifier = self.url_map.normalize_url( + let specifier = self.url_map.uri_to_specifier( ¶ms.text_document_position_params.text_document.uri, LspUrlKind::File, ); @@ -2429,7 +2432,7 @@ impl Inner { ) -> LspResult>> { let specifier = self .url_map - .normalize_url(¶ms.text_document.uri, LspUrlKind::File); + .uri_to_specifier(¶ms.text_document.uri, LspUrlKind::File); if !self.is_diagnosable(&specifier) || !self.config.specifier_enabled(&specifier) { @@ -2476,7 +2479,7 @@ impl Inner { ) -> LspResult>> { let specifier = self .url_map - .normalize_url(¶ms.item.uri, LspUrlKind::File); + .uri_to_specifier(¶ms.item.uri, LspUrlKind::File); if !self.is_diagnosable(&specifier) || !self.config.specifier_enabled(&specifier) { @@ -2525,7 +2528,7 @@ impl Inner { ) -> LspResult>> { let specifier = self .url_map - .normalize_url(¶ms.item.uri, LspUrlKind::File); + .uri_to_specifier(¶ms.item.uri, LspUrlKind::File); if !self.is_diagnosable(&specifier) || !self.config.specifier_enabled(&specifier) { @@ -2570,7 +2573,7 @@ impl Inner { &self, params: CallHierarchyPrepareParams, ) -> LspResult>> { - let specifier = self.url_map.normalize_url( + let specifier = self.url_map.uri_to_specifier( ¶ms.text_document_position_params.text_document.uri, LspUrlKind::File, ); @@ -2634,7 +2637,7 @@ impl Inner { &self, params: RenameParams, ) -> LspResult> { - let specifier = self.url_map.normalize_url( + let specifier = self.url_map.uri_to_specifier( ¶ms.text_document_position.text_document.uri, LspUrlKind::File, ); @@ -2683,7 +2686,7 @@ impl Inner { ) -> LspResult>> { let specifier = self .url_map - .normalize_url(¶ms.text_document.uri, LspUrlKind::File); + .uri_to_specifier(¶ms.text_document.uri, LspUrlKind::File); if !self.is_diagnosable(&specifier) || !self.config.specifier_enabled(&specifier) { @@ -2721,7 +2724,7 @@ impl Inner { ) -> LspResult> { let specifier = self .url_map - .normalize_url(¶ms.text_document.uri, LspUrlKind::File); + .uri_to_specifier(¶ms.text_document.uri, LspUrlKind::File); if !self.is_diagnosable(&specifier) { return Ok(None); } @@ -2774,7 +2777,7 @@ impl Inner { ) -> LspResult> { let specifier = self .url_map - .normalize_url(¶ms.text_document.uri, LspUrlKind::File); + .uri_to_specifier(¶ms.text_document.uri, LspUrlKind::File); if !self.is_diagnosable(&specifier) { return Ok(None); } @@ -2823,7 +2826,7 @@ impl Inner { &self, params: SignatureHelpParams, ) -> LspResult> { - let specifier = self.url_map.normalize_url( + let specifier = self.url_map.uri_to_specifier( ¶ms.text_document_position_params.text_document.uri, LspUrlKind::File, ); @@ -2877,8 +2880,8 @@ impl Inner { ) -> LspResult> { let mut changes = vec![]; for rename in params.files { - let old_specifier = self.url_map.normalize_url( - &url_to_uri(&resolve_url(&rename.old_uri).unwrap()), + let old_specifier = self.url_map.uri_to_specifier( + &Uri::from_str(&rename.old_uri).unwrap(), LspUrlKind::File, ); let options = self @@ -2903,8 +2906,8 @@ impl Inner { .get_edits_for_file_rename( self.snapshot(), old_specifier, - self.url_map.normalize_url( - &url_to_uri(&resolve_url(&rename.new_uri).unwrap()), + self.url_map.uri_to_specifier( + &Uri::from_str(&rename.new_uri).unwrap(), LspUrlKind::File, ), format_code_settings, @@ -3503,22 +3506,28 @@ impl Inner { let mut config_events = vec![]; for (scope_url, config_data) in self.config.tree.data_by_scope().iter() { - let scope_uri = url_to_uri(scope_url); + let Ok(scope_uri) = url_to_uri(scope_url) else { + continue; + }; if let Some(config_file) = config_data.maybe_deno_json() { - config_events.push(lsp_custom::DenoConfigurationChangeEvent { - scope_uri: scope_uri.clone(), - file_uri: url_to_uri(&config_file.specifier), - typ: lsp_custom::DenoConfigurationChangeType::Added, - configuration_type: lsp_custom::DenoConfigurationType::DenoJson, - }); + if let Ok(file_uri) = url_to_uri(&config_file.specifier) { + config_events.push(lsp_custom::DenoConfigurationChangeEvent { + scope_uri: scope_uri.clone(), + file_uri, + typ: lsp_custom::DenoConfigurationChangeType::Added, + configuration_type: lsp_custom::DenoConfigurationType::DenoJson, + }); + } } if let Some(package_json) = config_data.maybe_pkg_json() { - config_events.push(lsp_custom::DenoConfigurationChangeEvent { - scope_uri, - file_uri: url_to_uri(&package_json.specifier()), - typ: lsp_custom::DenoConfigurationChangeType::Added, - configuration_type: lsp_custom::DenoConfigurationType::PackageJson, - }); + if let Ok(file_uri) = url_to_uri(&package_json.specifier()) { + config_events.push(lsp_custom::DenoConfigurationChangeEvent { + scope_uri, + file_uri, + typ: lsp_custom::DenoConfigurationChangeType::Added, + configuration_type: lsp_custom::DenoConfigurationType::PackageJson, + }); + } } } if !config_events.is_empty() { @@ -3648,7 +3657,9 @@ impl Inner { .into_iter() .map(|folder| { ( - self.url_map.normalize_url(&folder.uri, LspUrlKind::Folder), + self + .url_map + .uri_to_specifier(&folder.uri, LspUrlKind::Folder), folder, ) }) @@ -3724,7 +3735,8 @@ impl Inner { result.push(TaskDefinition { name: name.clone(), command: command.to_string(), - source_uri: url_to_uri(&config_file.specifier), + source_uri: url_to_uri(&config_file.specifier) + .map_err(|_| LspError::internal_error())?, }); } }; @@ -3735,7 +3747,8 @@ impl Inner { result.push(TaskDefinition { name: name.clone(), command: command.clone(), - source_uri: url_to_uri(&package_json.specifier()), + source_uri: url_to_uri(&package_json.specifier()) + .map_err(|_| LspError::internal_error())?, }); } } @@ -3750,7 +3763,7 @@ impl Inner { ) -> LspResult>> { let specifier = self .url_map - .normalize_url(¶ms.text_document.uri, LspUrlKind::File); + .uri_to_specifier(¶ms.text_document.uri, LspUrlKind::File); if !self.is_diagnosable(&specifier) || !self.config.specifier_enabled(&specifier) || !self.config.enabled_inlay_hints_for_specifier(&specifier) @@ -3813,7 +3826,7 @@ impl Inner { .mark_with_args("lsp.virtual_text_document", ¶ms); let specifier = self .url_map - .normalize_url(¶ms.text_document.uri, LspUrlKind::File); + .uri_to_specifier(¶ms.text_document.uri, LspUrlKind::File); let contents = if specifier.scheme() == "deno" && specifier.path() == "/status.md" { diff --git a/cli/lsp/repl.rs b/cli/lsp/repl.rs index ada30f8374..85d3a022e6 100644 --- a/cli/lsp/repl.rs +++ b/cli/lsp/repl.rs @@ -76,7 +76,7 @@ impl ReplLanguageServer { .initialize(InitializeParams { process_id: None, root_path: None, - root_uri: Some(url_to_uri(&cwd_uri)), + root_uri: Some(url_to_uri(&cwd_uri).unwrap()), initialization_options: Some( serde_json::to_value(get_repl_workspace_settings()).unwrap(), ), diff --git a/cli/lsp/testing/definitions.rs b/cli/lsp/testing/definitions.rs index 69baf053e2..f23411852f 100644 --- a/cli/lsp/testing/definitions.rs +++ b/cli/lsp/testing/definitions.rs @@ -10,6 +10,7 @@ use crate::tools::test::TestDescription; use crate::tools::test::TestStepDescription; use crate::util::checksum; +use deno_core::error::AnyError; use deno_core::ModuleSpecifier; use lsp::Range; use std::collections::HashMap; @@ -144,21 +145,23 @@ impl TestModule { pub fn as_replace_notification( &self, maybe_root_uri: Option<&ModuleSpecifier>, - ) -> TestingNotification { + ) -> Result { let label = self.label(maybe_root_uri); - TestingNotification::Module(lsp_custom::TestModuleNotificationParams { - text_document: lsp::TextDocumentIdentifier { - uri: url_to_uri(&self.specifier), + Ok(TestingNotification::Module( + lsp_custom::TestModuleNotificationParams { + text_document: lsp::TextDocumentIdentifier { + uri: url_to_uri(&self.specifier)?, + }, + kind: lsp_custom::TestModuleNotificationKind::Replace, + label, + tests: self + .defs + .iter() + .filter(|(_, def)| def.parent_id.is_none()) + .map(|(id, _)| self.get_test_data(id)) + .collect(), }, - kind: lsp_custom::TestModuleNotificationKind::Replace, - label, - tests: self - .defs - .iter() - .filter(|(_, def)| def.parent_id.is_none()) - .map(|(id, _)| self.get_test_data(id)) - .collect(), - }) + )) } pub fn label(&self, maybe_root_uri: Option<&ModuleSpecifier>) -> String { diff --git a/cli/lsp/testing/execution.rs b/cli/lsp/testing/execution.rs index aec91b3e7d..4ac565aa0b 100644 --- a/cli/lsp/testing/execution.rs +++ b/cli/lsp/testing/execution.rs @@ -186,7 +186,7 @@ impl TestRun { self .queue .iter() - .map(|s| { + .filter_map(|s| { let ids = if let Some((test_module, _)) = tests.get(s) { if let Some(filter) = self.filters.get(s) { filter.as_ids(test_module) @@ -196,10 +196,12 @@ impl TestRun { } else { Vec::new() }; - lsp_custom::EnqueuedTestModule { - text_document: lsp::TextDocumentIdentifier { uri: url_to_uri(s) }, + Some(lsp_custom::EnqueuedTestModule { + text_document: lsp::TextDocumentIdentifier { + uri: url_to_uri(s).ok()?, + }, ids, - } + }) }) .collect() } @@ -591,6 +593,9 @@ impl LspTestReporter { let (test_module, _) = files .entry(specifier.clone()) .or_insert_with(|| (TestModule::new(specifier), "1".to_string())); + let Ok(uri) = url_to_uri(&test_module.specifier) else { + return; + }; let (static_id, is_new) = test_module.register_dynamic(desc); self.tests.insert( desc.id, @@ -601,9 +606,7 @@ impl LspTestReporter { .client .send_test_notification(TestingNotification::Module( lsp_custom::TestModuleNotificationParams { - text_document: lsp::TextDocumentIdentifier { - uri: url_to_uri(&test_module.specifier), - }, + text_document: lsp::TextDocumentIdentifier { uri }, kind: lsp_custom::TestModuleNotificationKind::Insert, label: test_module.label(self.maybe_root_uri.as_ref()), tests: vec![test_module.get_test_data(&static_id)], @@ -701,6 +704,9 @@ impl LspTestReporter { let (test_module, _) = files .entry(specifier.clone()) .or_insert_with(|| (TestModule::new(specifier), "1".to_string())); + let Ok(uri) = url_to_uri(&test_module.specifier) else { + return; + }; let (static_id, is_new) = test_module.register_step_dynamic( desc, self.tests.get(&desc.parent_id).unwrap().static_id(), @@ -714,9 +720,7 @@ impl LspTestReporter { .client .send_test_notification(TestingNotification::Module( lsp_custom::TestModuleNotificationParams { - text_document: lsp::TextDocumentIdentifier { - uri: url_to_uri(&test_module.specifier), - }, + text_document: lsp::TextDocumentIdentifier { uri }, kind: lsp_custom::TestModuleNotificationKind::Insert, label: test_module.label(self.maybe_root_uri.as_ref()), tests: vec![test_module.get_test_data(&static_id)], @@ -800,14 +804,14 @@ mod tests { include: Some(vec![ lsp_custom::TestIdentifier { text_document: lsp::TextDocumentIdentifier { - uri: url_to_uri(&specifier), + uri: url_to_uri(&specifier).unwrap(), }, id: None, step_id: None, }, lsp_custom::TestIdentifier { text_document: lsp::TextDocumentIdentifier { - uri: url_to_uri(&non_test_specifier), + uri: url_to_uri(&non_test_specifier).unwrap(), }, id: None, step_id: None, @@ -815,7 +819,7 @@ mod tests { ]), exclude: vec![lsp_custom::TestIdentifier { text_document: lsp::TextDocumentIdentifier { - uri: url_to_uri(&specifier), + uri: url_to_uri(&specifier).unwrap(), }, id: Some( "69d9fe87f64f5b66cb8b631d4fd2064e8224b8715a049be54276c42189ff8f9f" diff --git a/cli/lsp/testing/server.rs b/cli/lsp/testing/server.rs index 0c34ea7bc2..c9c39d9ffc 100644 --- a/cli/lsp/testing/server.rs +++ b/cli/lsp/testing/server.rs @@ -27,14 +27,16 @@ use tower_lsp::jsonrpc::Error as LspError; use tower_lsp::jsonrpc::Result as LspResult; use tower_lsp::lsp_types as lsp; -fn as_delete_notification(url: ModuleSpecifier) -> TestingNotification { - TestingNotification::DeleteModule( +fn as_delete_notification( + url: &ModuleSpecifier, +) -> Result { + Ok(TestingNotification::DeleteModule( lsp_custom::TestModuleDeleteNotificationParams { text_document: lsp::TextDocumentIdentifier { - uri: url_to_uri(&url), + uri: url_to_uri(url)?, }, }, - ) + )) } pub type TestServerTests = @@ -126,20 +128,24 @@ impl TestServer { .map(|tm| tm.as_ref().clone()) .unwrap_or_else(|| TestModule::new(specifier.clone())); if !test_module.is_empty() { - client.send_test_notification( - test_module.as_replace_notification(mru.as_ref()), - ); + if let Ok(params) = + test_module.as_replace_notification(mru.as_ref()) + { + client.send_test_notification(params); + } } else if !was_empty { - client.send_test_notification(as_delete_notification( - specifier.clone(), - )); + if let Ok(params) = as_delete_notification(specifier) { + client.send_test_notification(params); + } } tests .insert(specifier.clone(), (test_module, script_version)); } } - for key in keys { - client.send_test_notification(as_delete_notification(key)); + for key in &keys { + if let Ok(params) = as_delete_notification(key) { + client.send_test_notification(params); + } } performance.measure(mark); } diff --git a/cli/lsp/tsc.rs b/cli/lsp/tsc.rs index 566c619779..28b1cd0b7a 100644 --- a/cli/lsp/tsc.rs +++ b/cli/lsp/tsc.rs @@ -19,9 +19,10 @@ use super::refactor::EXTRACT_TYPE; use super::semantic_tokens; use super::semantic_tokens::SemanticTokensBuilder; use super::text::LineIndex; +use super::urls::uri_to_url; use super::urls::url_to_uri; -use super::urls::LspClientUrl; use super::urls::INVALID_SPECIFIER; +use super::urls::INVALID_URI; use crate::args::jsr_url; use crate::args::FmtOptionsConfig; @@ -2047,7 +2048,7 @@ impl DocumentSpan { let file_referrer = target_asset_or_doc.file_referrer(); let target_uri = language_server .url_map - .normalize_specifier(&target_specifier, file_referrer) + .specifier_to_uri(&target_specifier, file_referrer) .ok()?; let (target_range, target_selection_range) = if let Some(context_span) = &self.context_span { @@ -2072,7 +2073,7 @@ impl DocumentSpan { }; let link = lsp::LocationLink { origin_selection_range, - target_uri: target_uri.to_uri(), + target_uri, target_range, target_selection_range, }; @@ -2092,11 +2093,11 @@ impl DocumentSpan { let line_index = asset_or_doc.line_index(); let range = self.text_span.to_range(line_index); let file_referrer = asset_or_doc.file_referrer(); - let mut target = language_server + let target_uri = language_server .url_map - .normalize_specifier(&specifier, file_referrer) - .ok()? - .into_url(); + .specifier_to_uri(&specifier, file_referrer) + .ok()?; + let mut target = uri_to_url(&target_uri); target.set_fragment(Some(&format!( "L{},{}", range.start.line + 1, @@ -2155,13 +2156,10 @@ impl NavigateToItem { let file_referrer = asset_or_doc.file_referrer(); let uri = language_server .url_map - .normalize_specifier(&specifier, file_referrer) + .specifier_to_uri(&specifier, file_referrer) .ok()?; let range = self.text_span.to_range(line_index); - let location = lsp::Location { - uri: uri.to_uri(), - range, - }; + let location = lsp::Location { uri, range }; let mut tags: Option> = None; let kind_modifiers = parse_kind_modifier(&self.kind_modifiers); @@ -2414,12 +2412,10 @@ impl ImplementationLocation { let file_referrer = language_server.documents.get_file_referrer(&specifier); let uri = language_server .url_map - .normalize_specifier(&specifier, file_referrer.as_deref()) - .unwrap_or_else(|_| { - LspClientUrl::new(ModuleSpecifier::parse("deno://invalid").unwrap()) - }); + .specifier_to_uri(&specifier, file_referrer.as_deref()) + .unwrap_or_else(|_| INVALID_URI.clone()); lsp::Location { - uri: uri.to_uri(), + uri, range: self.document_span.text_span.to_range(line_index), } } @@ -2475,7 +2471,7 @@ impl RenameLocations { language_server.documents.get_file_referrer(&specifier); let uri = language_server .url_map - .normalize_specifier(&specifier, file_referrer.as_deref())?; + .specifier_to_uri(&specifier, file_referrer.as_deref())?; let asset_or_doc = language_server.get_asset_or_document(&specifier)?; // ensure TextDocumentEdit for `location.file_name`. @@ -2484,7 +2480,7 @@ impl RenameLocations { uri.clone(), lsp::TextDocumentEdit { text_document: lsp::OptionalVersionedTextDocumentIdentifier { - uri: uri.to_uri(), + uri: uri.clone(), version: asset_or_doc.document_lsp_version(), }, edits: @@ -2686,7 +2682,7 @@ impl FileTextChanges { .collect(); Ok(lsp::TextDocumentEdit { text_document: lsp::OptionalVersionedTextDocumentIdentifier { - uri: url_to_uri(&specifier), + uri: url_to_uri(&specifier)?, version: asset_or_doc.document_lsp_version(), }, edits, @@ -2713,7 +2709,7 @@ impl FileTextChanges { if self.is_new_file.unwrap_or(false) { ops.push(lsp::DocumentChangeOperation::Op(lsp::ResourceOp::Create( lsp::CreateFile { - uri: url_to_uri(&specifier), + uri: url_to_uri(&specifier)?, options: Some(lsp::CreateFileOptions { ignore_if_exists: Some(true), overwrite: None, @@ -2730,7 +2726,7 @@ impl FileTextChanges { .collect(); ops.push(lsp::DocumentChangeOperation::Edit(lsp::TextDocumentEdit { text_document: lsp::OptionalVersionedTextDocumentIdentifier { - uri: url_to_uri(&specifier), + uri: url_to_uri(&specifier)?, version: maybe_asset_or_document.and_then(|d| d.document_lsp_version()), }, edits, @@ -3128,10 +3124,10 @@ impl ReferenceEntry { let file_referrer = language_server.documents.get_file_referrer(&specifier); let uri = language_server .url_map - .normalize_specifier(&specifier, file_referrer.as_deref()) - .unwrap_or_else(|_| LspClientUrl::new(INVALID_SPECIFIER.clone())); + .specifier_to_uri(&specifier, file_referrer.as_deref()) + .unwrap_or_else(|_| INVALID_URI.clone()); lsp::Location { - uri: uri.to_uri(), + uri, range: self.document_span.text_span.to_range(line_index), } } @@ -3189,12 +3185,13 @@ impl CallHierarchyItem { .get_file_referrer(&target_specifier); let uri = language_server .url_map - .normalize_specifier(&target_specifier, file_referrer.as_deref()) - .unwrap_or_else(|_| LspClientUrl::new(INVALID_SPECIFIER.clone())); + .specifier_to_uri(&target_specifier, file_referrer.as_deref()) + .unwrap_or_else(|_| INVALID_URI.clone()); let use_file_name = self.is_source_file_item(); - let maybe_file_path = if uri.as_url().scheme() == "file" { - specifier_to_file_path(uri.as_url()).ok() + let maybe_file_path = if uri.scheme().is_some_and(|s| s.as_str() == "file") + { + specifier_to_file_path(&uri_to_url(&uri)).ok() } else { None }; @@ -3238,7 +3235,7 @@ impl CallHierarchyItem { lsp::CallHierarchyItem { name, tags, - uri: uri.to_uri(), + uri, detail: Some(detail), kind: self.kind.clone().into(), range: self.span.to_range(line_index.clone()), diff --git a/cli/lsp/urls.rs b/cli/lsp/urls.rs index 4a960d3667..13e66dfe99 100644 --- a/cli/lsp/urls.rs +++ b/cli/lsp/urls.rs @@ -13,12 +13,18 @@ use std::str::FromStr; use std::sync::Arc; use super::cache::LspCache; +use super::logging::lsp_warn; /// Used in situations where a default URL needs to be used where otherwise a /// panic is undesired. pub static INVALID_SPECIFIER: Lazy = Lazy::new(|| ModuleSpecifier::parse("deno://invalid").unwrap()); +/// Used in situations where a default URL needs to be used where otherwise a +/// panic is undesired. +pub static INVALID_URI: Lazy = + Lazy::new(|| Uri::from_str("deno://invalid").unwrap()); + /// Matches the `encodeURIComponent()` encoding from JavaScript, which matches /// the component percent encoding set. /// @@ -58,7 +64,7 @@ fn hash_data_specifier(specifier: &ModuleSpecifier) -> String { crate::util::checksum::gen(&[file_name_str.as_bytes()]) } -fn to_deno_url(specifier: &Url) -> String { +fn to_deno_uri(specifier: &Url) -> String { let mut string = String::with_capacity(specifier.as_str().len() + 6); string.push_str("deno:/"); string.push_str(specifier.scheme()); @@ -95,64 +101,31 @@ fn from_deno_url(url: &Url) -> Option { Url::parse(&string).ok() } -/// This exists to make it a little bit harder to accidentally use a `Url` -/// in the wrong place where a client url should be used. -#[derive(Debug, Clone, Hash, PartialEq, Eq, Ord, PartialOrd)] -pub struct LspClientUrl(Url); - -impl LspClientUrl { - pub fn new(url: Url) -> Self { - Self(url) - } - - pub fn as_url(&self) -> &Url { - &self.0 - } - - pub fn into_url(self) -> Url { - self.0 - } - - pub fn to_uri(&self) -> Uri { - url_to_uri(&self.0) - } - - pub fn as_str(&self) -> &str { - self.0.as_str() - } -} - -impl std::fmt::Display for LspClientUrl { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - self.0.fmt(f) - } -} - #[derive(Debug, Default)] struct LspUrlMapInner { - specifier_to_url: HashMap, - url_to_specifier: HashMap, + specifier_to_uri: HashMap, + uri_to_specifier: HashMap, } impl LspUrlMapInner { - fn put(&mut self, specifier: ModuleSpecifier, url: LspClientUrl) { - self - .url_to_specifier - .insert(url.as_url().clone(), specifier.clone()); - self.specifier_to_url.insert(specifier, url); + fn put(&mut self, specifier: ModuleSpecifier, uri: Uri) { + self.uri_to_specifier.insert(uri.clone(), specifier.clone()); + self.specifier_to_uri.insert(specifier, uri); } - fn get_url(&self, specifier: &ModuleSpecifier) -> Option<&LspClientUrl> { - self.specifier_to_url.get(specifier) + fn get_uri(&self, specifier: &ModuleSpecifier) -> Option<&Uri> { + self.specifier_to_uri.get(specifier) } - fn get_specifier(&self, url: &Url) -> Option<&ModuleSpecifier> { - self.url_to_specifier.get(url) + fn get_specifier(&self, uri: &Uri) -> Option<&ModuleSpecifier> { + self.uri_to_specifier.get(uri) } } -pub fn url_to_uri(url: &Url) -> Uri { - Uri::from_str(url.as_str()).unwrap() +pub fn url_to_uri(url: &Url) -> Result { + Ok(Uri::from_str(url.as_str()).inspect_err(|err| { + lsp_warn!("Could not convert URL \"{url}\" to URI: {err}") + })?) } pub fn uri_to_url(uri: &Uri) -> Url { @@ -181,24 +154,24 @@ impl LspUrlMap { /// Normalize a specifier that is used internally within Deno (or tsc) to a /// URL that can be handled as a "virtual" document by an LSP client. - pub fn normalize_specifier( + pub fn specifier_to_uri( &self, specifier: &ModuleSpecifier, file_referrer: Option<&ModuleSpecifier>, - ) -> Result { + ) -> Result { if let Some(file_url) = self.cache.vendored_specifier(specifier, file_referrer) { - return Ok(LspClientUrl(file_url)); + return url_to_uri(&file_url); } let mut inner = self.inner.lock(); - if let Some(url) = inner.get_url(specifier).cloned() { - Ok(url) + if let Some(uri) = inner.get_uri(specifier).cloned() { + Ok(uri) } else { - let url = if specifier.scheme() == "file" { - LspClientUrl(specifier.clone()) + let uri = if specifier.scheme() == "file" { + url_to_uri(specifier)? } else { - let specifier_str = if specifier.scheme() == "asset" { + let uri_str = if specifier.scheme() == "asset" { format!("deno:/asset{}", specifier.path()) } else if specifier.scheme() == "data" { let data_url = deno_graph::source::RawDataUrl::parse(specifier)?; @@ -214,13 +187,13 @@ impl LspUrlMap { extension ) } else { - to_deno_url(specifier) + to_deno_uri(specifier) }; - let url = LspClientUrl(Url::parse(&specifier_str)?); - inner.put(specifier.clone(), url.clone()); - url + let uri = Uri::from_str(&uri_str)?; + inner.put(specifier.clone(), uri.clone()); + uri }; - Ok(url) + Ok(uri) } } @@ -232,13 +205,17 @@ impl LspUrlMap { /// 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, uri: &Uri, kind: LspUrlKind) -> ModuleSpecifier { + pub fn uri_to_specifier( + &self, + uri: &Uri, + kind: LspUrlKind, + ) -> ModuleSpecifier { let url = uri_to_url(uri); if let Some(remote_url) = self.cache.unvendored_specifier(&url) { return remote_url; } let mut inner = self.inner.lock(); - if let Some(specifier) = inner.get_specifier(&url).cloned() { + if let Some(specifier) = inner.get_specifier(uri).cloned() { return specifier; } let mut specifier = None; @@ -255,7 +232,7 @@ impl LspUrlMap { specifier = Some(s); } let specifier = specifier.unwrap_or_else(|| url.clone()); - inner.put(specifier.clone(), LspClientUrl(url)); + inner.put(specifier.clone(), uri.clone()); specifier } } @@ -303,15 +280,14 @@ mod tests { fn test_lsp_url_map() { let map = LspUrlMap::default(); let fixture = resolve_url("https://deno.land/x/pkg@1.0.0/mod.ts").unwrap(); - let actual_url = map - .normalize_specifier(&fixture, None) + let actual_uri = map + .specifier_to_uri(&fixture, None) .expect("could not handle specifier"); - let expected_url = - Url::parse("deno:/https/deno.land/x/pkg%401.0.0/mod.ts").unwrap(); - assert_eq!(actual_url.as_url(), &expected_url); - - let actual_specifier = - map.normalize_url(&actual_url.to_uri(), LspUrlKind::File); + assert_eq!( + actual_uri.as_str(), + "deno:/https/deno.land/x/pkg%401.0.0/mod.ts" + ); + let actual_specifier = map.uri_to_specifier(&actual_uri, LspUrlKind::File); assert_eq!(actual_specifier, fixture); } @@ -320,17 +296,13 @@ mod tests { let map = LspUrlMap::default(); let fixture = Uri::from_str("deno:/https/deno.land/x/pkg%401.0.0/mod.ts").unwrap(); - let actual_specifier = map.normalize_url(&fixture, LspUrlKind::File); + let actual_specifier = map.uri_to_specifier(&fixture, LspUrlKind::File); let expected_specifier = Url::parse("https://deno.land/x/pkg@1.0.0/mod.ts").unwrap(); assert_eq!(&actual_specifier, &expected_specifier); - let actual_url = map - .normalize_specifier(&actual_specifier, None) - .unwrap() - .to_uri() - .clone(); - assert_eq!(actual_url, fixture); + let actual_uri = map.specifier_to_uri(&actual_specifier, None).unwrap(); + assert_eq!(actual_uri, fixture); } #[test] @@ -338,14 +310,11 @@ mod tests { // Test fix for #9741 - not properly encoding certain URLs let map = LspUrlMap::default(); let fixture = resolve_url("https://cdn.skypack.dev/-/postcss@v8.2.9-E4SktPp9c0AtxrJHp8iV/dist=es2020,mode=types/lib/postcss.d.ts").unwrap(); - let actual_url = map - .normalize_specifier(&fixture, None) + let actual_uri = map + .specifier_to_uri(&fixture, None) .expect("could not handle specifier"); - 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.as_url(), &expected_url); - - let actual_specifier = - map.normalize_url(&actual_url.to_uri(), LspUrlKind::File); + assert_eq!(actual_uri.as_str(), "deno:/https/cdn.skypack.dev/-/postcss%40v8.2.9-E4SktPp9c0AtxrJHp8iV/dist%3Des2020%2Cmode%3Dtypes/lib/postcss.d.ts"); + let actual_specifier = map.uri_to_specifier(&actual_uri, LspUrlKind::File); assert_eq!(actual_specifier, fixture); } @@ -353,14 +322,13 @@ mod tests { fn test_lsp_url_map_data() { let map = LspUrlMap::default(); let fixture = resolve_url("data:application/typescript;base64,ZXhwb3J0IGNvbnN0IGEgPSAiYSI7CgpleHBvcnQgZW51bSBBIHsKICBBLAogIEIsCiAgQywKfQo=").unwrap(); - let actual_url = map - .normalize_specifier(&fixture, None) + let actual_uri = map + .specifier_to_uri(&fixture, None) .expect("could not handle specifier"); let expected_url = Url::parse("deno:/c21c7fc382b2b0553dc0864aa81a3acacfb7b3d1285ab5ae76da6abec213fb37/data_url.ts").unwrap(); - assert_eq!(actual_url.as_url(), &expected_url); + assert_eq!(&uri_to_url(&actual_uri), &expected_url); - let actual_specifier = - map.normalize_url(&actual_url.to_uri(), LspUrlKind::File); + let actual_specifier = map.uri_to_specifier(&actual_uri, LspUrlKind::File); assert_eq!(actual_specifier, fixture); } @@ -368,15 +336,11 @@ mod tests { fn test_lsp_url_map_host_with_port() { let map = LspUrlMap::default(); let fixture = resolve_url("http://localhost:8000/mod.ts").unwrap(); - let actual_url = map - .normalize_specifier(&fixture, None) + let actual_uri = map + .specifier_to_uri(&fixture, None) .expect("could not handle specifier"); - let expected_url = - Url::parse("deno:/http/localhost%3A8000/mod.ts").unwrap(); - assert_eq!(actual_url.as_url(), &expected_url); - - let actual_specifier = - map.normalize_url(&actual_url.to_uri(), LspUrlKind::File); + assert_eq!(actual_uri.as_str(), "deno:/http/localhost%3A8000/mod.ts"); + let actual_specifier = map.uri_to_specifier(&actual_uri, LspUrlKind::File); assert_eq!(actual_specifier, fixture); } @@ -388,7 +352,7 @@ mod tests { "file:///c%3A/Users/deno/Desktop/file%20with%20spaces%20in%20name.txt", ) .unwrap(); - let actual = map.normalize_url(&fixture, LspUrlKind::File); + let actual = map.uri_to_specifier(&fixture, LspUrlKind::File); let expected = Url::parse("file:///C:/Users/deno/Desktop/file with spaces in name.txt") .unwrap(); @@ -403,7 +367,7 @@ mod tests { "file:///Users/deno/Desktop/file%20with%20spaces%20in%20name.txt", ) .unwrap(); - let actual = map.normalize_url(&fixture, LspUrlKind::File); + let actual = map.uri_to_specifier(&fixture, LspUrlKind::File); let expected = Url::parse("file:///Users/deno/Desktop/file with spaces in name.txt") .unwrap(); @@ -414,7 +378,7 @@ mod tests { fn test_normalize_deno_status() { let map = LspUrlMap::default(); let fixture = Uri::from_str("deno:/status.md").unwrap(); - let actual = map.normalize_url(&fixture, LspUrlKind::File); + let actual = map.uri_to_specifier(&fixture, LspUrlKind::File); assert_eq!(actual.as_str(), fixture.as_str()); }