From 6e5a631fe094fa77209d51b65adfcc52d738e197 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Thu, 23 Mar 2023 10:23:04 -0400 Subject: [PATCH] refactor(lsp): add `LspClientUrl` (#18376) This will make it a bit harder to accidentally use a client url in the wrong place. I don't fully understand why we do this mapping, but this will help prevent bugs like #18373 Closes #18374 --- cli/lsp/client.rs | 14 ++++++--- cli/lsp/language_server.rs | 13 +++++--- cli/lsp/tsc.rs | 40 ++++++++++++++---------- cli/lsp/urls.rs | 64 +++++++++++++++++++++++++++++--------- 4 files changed, 92 insertions(+), 39 deletions(-) diff --git a/cli/lsp/client.rs b/cli/lsp/client.rs index 91b12983da..d24d4c2a9e 100644 --- a/cli/lsp/client.rs +++ b/cli/lsp/client.rs @@ -17,6 +17,7 @@ use super::config::SpecifierSettings; 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 { @@ -98,18 +99,23 @@ impl OutsideLockClient { pub async fn specifier_configurations( &self, - specifiers: Vec, + specifiers: Vec, ) -> Result>, AnyError> { - self.0.specifier_configurations(specifiers).await + self + .0 + .specifier_configurations( + specifiers.into_iter().map(|s| s.into_url()).collect(), + ) + .await } pub async fn specifier_configuration( &self, - specifier: &lsp::Url, + specifier: &LspClientUrl, ) -> Result { let values = self .0 - .specifier_configurations(vec![specifier.clone()]) + .specifier_configurations(vec![specifier.as_url().clone()]) .await?; if let Some(value) = values.into_iter().next() { value.map_err(|err| { diff --git a/cli/lsp/language_server.rs b/cli/lsp/language_server.rs index 519d1c94fe..72beb04bb1 100644 --- a/cli/lsp/language_server.rs +++ b/cli/lsp/language_server.rs @@ -59,6 +59,7 @@ use super::tsc::Assets; use super::tsc::AssetsSnapshot; use super::tsc::TsServer; use super::urls; +use super::urls::LspClientUrl; use crate::args::get_root_cert_store; use crate::args::package_json; use crate::args::resolve_import_map_from_specifier; @@ -338,7 +339,10 @@ impl LanguageServer { match &ls.config.workspace_folders { Some(entry) => { for (specifier, folder) in entry { - specifiers.insert(specifier.clone(), folder.uri.clone()); + specifiers.insert( + specifier.clone(), + LspClientUrl::new(folder.uri.clone()), + ); } } None => { @@ -2868,9 +2872,10 @@ impl tower_lsp::LanguageServer for LanguageServer { let (client, client_uri, specifier, had_specifier_settings) = { let mut inner = self.0.write().await; let client = inner.client.clone(); - let client_uri = params.text_document.uri.clone(); - let specifier = - inner.url_map.normalize_url(&client_uri, LspUrlKind::File); + let client_uri = LspClientUrl::new(params.text_document.uri.clone()); + let specifier = inner + .url_map + .normalize_url(client_uri.as_url(), LspUrlKind::File); let document = inner.did_open(&specifier, params).await; let has_specifier_settings = inner.config.has_specifier_settings(&specifier); diff --git a/cli/lsp/tsc.rs b/cli/lsp/tsc.rs index fedaae5884..3c70e40296 100644 --- a/cli/lsp/tsc.rs +++ b/cli/lsp/tsc.rs @@ -14,6 +14,7 @@ use super::refactor::EXTRACT_TYPE; use super::semantic_tokens; use super::semantic_tokens::SemanticTokensBuilder; use super::text::LineIndex; +use super::urls::LspClientUrl; use super::urls::LspUrlMap; use super::urls::INVALID_SPECIFIER; @@ -36,7 +37,6 @@ use deno_core::serde::Serialize; use deno_core::serde_json; use deno_core::serde_json::json; use deno_core::serde_json::Value; -use deno_core::url::Url; use deno_core::JsRuntime; use deno_core::ModuleSpecifier; use deno_core::OpState; @@ -842,7 +842,7 @@ impl DocumentSpan { }; let link = lsp::LocationLink { origin_selection_range, - target_uri, + target_uri: target_uri.into_url(), target_range, target_selection_range, }; @@ -864,7 +864,8 @@ impl DocumentSpan { let mut target = language_server .url_map .normalize_specifier(&specifier) - .ok()?; + .ok()? + .into_url(); target.set_fragment(Some(&format!( "L{},{}", range.start.line + 1, @@ -915,7 +916,10 @@ impl NavigateToItem { .normalize_specifier(&specifier) .ok()?; let range = self.text_span.to_range(line_index); - let location = lsp::Location { uri, range }; + let location = lsp::Location { + uri: uri.into_url(), + range, + }; let mut tags: Option> = None; let kind_modifiers = parse_kind_modifier(&self.kind_modifiers); @@ -1160,9 +1164,11 @@ impl ImplementationLocation { let uri = language_server .url_map .normalize_specifier(&specifier) - .unwrap_or_else(|_| ModuleSpecifier::parse("deno://invalid").unwrap()); + .unwrap_or_else(|_| { + LspClientUrl::new(ModuleSpecifier::parse("deno://invalid").unwrap()) + }); lsp::Location { - uri, + uri: uri.into_url(), range: self.document_span.text_span.to_range(line_index), } } @@ -1196,8 +1202,10 @@ impl RenameLocations { new_name: &str, language_server: &language_server::Inner, ) -> Result { - let mut text_document_edit_map: HashMap = - HashMap::new(); + let mut text_document_edit_map: HashMap< + LspClientUrl, + lsp::TextDocumentEdit, + > = HashMap::new(); for location in self.locations.iter() { let specifier = normalize_specifier(&location.document_span.file_name)?; let uri = language_server.url_map.normalize_specifier(&specifier)?; @@ -1209,7 +1217,7 @@ impl RenameLocations { uri.clone(), lsp::TextDocumentEdit { text_document: lsp::OptionalVersionedTextDocumentIdentifier { - uri: uri.clone(), + uri: uri.as_url().clone(), version: asset_or_doc.document_lsp_version(), }, edits: @@ -1698,9 +1706,9 @@ impl ReferenceEntry { .unwrap_or_else(|_| INVALID_SPECIFIER.clone()); let uri = url_map .normalize_specifier(&specifier) - .unwrap_or_else(|_| INVALID_SPECIFIER.clone()); + .unwrap_or_else(|_| LspClientUrl::new(INVALID_SPECIFIER.clone())); lsp::Location { - uri, + uri: uri.into_url(), range: self.document_span.text_span.to_range(line_index), } } @@ -1748,11 +1756,11 @@ impl CallHierarchyItem { let uri = language_server .url_map .normalize_specifier(&target_specifier) - .unwrap_or_else(|_| INVALID_SPECIFIER.clone()); + .unwrap_or_else(|_| LspClientUrl::new(INVALID_SPECIFIER.clone())); let use_file_name = self.is_source_file_item(); - let maybe_file_path = if uri.scheme() == "file" { - specifier_to_file_path(&uri).ok() + let maybe_file_path = if uri.as_url().scheme() == "file" { + specifier_to_file_path(uri.as_url()).ok() } else { None }; @@ -1760,7 +1768,7 @@ impl CallHierarchyItem { if let Some(file_path) = maybe_file_path.as_ref() { file_path.file_name().unwrap().to_string_lossy().to_string() } else { - uri.to_string() + uri.as_str().to_string() } } else { self.name.clone() @@ -1796,7 +1804,7 @@ impl CallHierarchyItem { lsp::CallHierarchyItem { name, tags, - uri, + uri: uri.into_url(), 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 14717d7152..905803da88 100644 --- a/cli/lsp/urls.rs +++ b/cli/lsp/urls.rs @@ -59,19 +59,50 @@ fn hash_data_specifier(specifier: &ModuleSpecifier) -> String { crate::util::checksum::gen(&[file_name_str.as_bytes()]) } +/// 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 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, + specifier_to_url: HashMap, url_to_specifier: HashMap, } impl LspUrlMapInner { - fn put(&mut self, specifier: ModuleSpecifier, url: Url) { - self.specifier_to_url.insert(specifier.clone(), url.clone()); - self.url_to_specifier.insert(url, specifier); + 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 get_url(&self, specifier: &ModuleSpecifier) -> Option<&Url> { + fn get_url(&self, specifier: &ModuleSpecifier) -> Option<&LspClientUrl> { self.specifier_to_url.get(specifier) } @@ -98,13 +129,13 @@ impl LspUrlMap { pub fn normalize_specifier( &self, specifier: &ModuleSpecifier, - ) -> Result { + ) -> Result { let mut inner = self.0.lock(); if let Some(url) = inner.get_url(specifier).cloned() { Ok(url) } else { let url = if specifier.scheme() == "file" { - specifier.clone() + LspClientUrl(specifier.clone()) } else { let specifier_str = if specifier.scheme() == "asset" { format!("deno:/asset{}", specifier.path()) @@ -136,7 +167,7 @@ impl LspUrlMap { path.push_str(&parts.join("/")); format!("deno:/{path}") }; - let url = Url::parse(&specifier_str)?; + let url = LspClientUrl(Url::parse(&specifier_str)?); inner.put(specifier.clone(), url.clone()); url }; @@ -165,7 +196,7 @@ impl LspUrlMap { } else { url.clone() }; - inner.put(specifier.clone(), url.clone()); + inner.put(specifier.clone(), LspClientUrl(url.clone())); specifier } } @@ -195,9 +226,10 @@ mod tests { .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, expected_url); + assert_eq!(actual_url.as_url(), &expected_url); - let actual_specifier = map.normalize_url(&actual_url, LspUrlKind::File); + let actual_specifier = + map.normalize_url(actual_url.as_url(), LspUrlKind::File); assert_eq!(actual_specifier, fixture); } @@ -210,9 +242,10 @@ mod tests { .normalize_specifier(&fixture) .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, expected_url); + assert_eq!(actual_url.as_url(), &expected_url); - let actual_specifier = map.normalize_url(&actual_url, LspUrlKind::File); + let actual_specifier = + map.normalize_url(actual_url.as_url(), LspUrlKind::File); assert_eq!(actual_specifier, fixture); } @@ -224,9 +257,10 @@ mod tests { .normalize_specifier(&fixture) .expect("could not handle specifier"); let expected_url = Url::parse("deno:/c21c7fc382b2b0553dc0864aa81a3acacfb7b3d1285ab5ae76da6abec213fb37/data_url.ts").unwrap(); - assert_eq!(actual_url, expected_url); + assert_eq!(actual_url.as_url(), &expected_url); - let actual_specifier = map.normalize_url(&actual_url, LspUrlKind::File); + let actual_specifier = + map.normalize_url(actual_url.as_url(), LspUrlKind::File); assert_eq!(actual_specifier, fixture); }