Repository: denoland/deno
Pull request: fix(lsp): url_to_uri() encoding on windows

Fixes #28736.
Fixes #28743.

Changed files:
filename: cli/lsp/diagnostics.rs
patch: @@ -1042,16 +1042,18 @@ async fn generate_ts_diagnostics(
   let mut enabled_modules_by_scope = BTreeMap::<_, Vec<_>>::new();
   let mut disabled_documents = Vec::new();
   for document in snapshot.document_modules.documents.open_docs() {
-    if let Some(module) = snapshot
-      .document_modules
-      .primary_module(&Document::Open(document.clone()))
-    {
-      if config.specifier_enabled(&module.specifier) {
-        enabled_modules_by_scope
-          .entry(module.scope.clone())
-          .or_default()
-          .push(module);
-        continue;
+    if document.is_diagnosable() {
+      if let Some(module) = snapshot
+        .document_modules
+        .primary_module(&Document::Open(document.clone()))
+      {
+        if config.specifier_enabled(&module.specifier) {
+          enabled_modules_by_scope
+            .entry(module.scope.clone())
+            .or_default()
+            .push(module);
+          continue;
+        }
       }
     }
     disabled_documents.push(document.clone());

filename: cli/lsp/urls.rs
patch: @@ -1,5 +1,7 @@
 // Copyright 2018-2025 the Deno authors. MIT license.
 
+use std::path::Component;
+use std::path::Path;
 use std::path::PathBuf;
 use std::str::FromStr;
 
@@ -82,29 +84,21 @@ pub fn url_to_uri(url: &Url) -> Result<Uri, AnyError> {
   let components = deno_core::url::quirks::internal_components(url);
   let mut input = String::with_capacity(url.as_str().len());
   input.push_str(&url.as_str()[..components.path_start as usize]);
-  if cfg!(windows) && url.scheme() == "file" {
-    let path = url.path();
-    let mut chars = path.chars();
-    let has_drive_letter = chars.next().is_some_and(|c| c == '/')
-      && chars.next().is_some_and(|c| c.is_ascii_alphabetic())
-      && chars.next().is_some_and(|c| c == ':')
-      && chars.next().is_none_or(|c| c == '/');
-    if has_drive_letter {
-      input.push_str(&path[..3]);
-      input.push_str(
-        &percent_encoding::utf8_percent_encode(&path[3..], URL_TO_URI_PATH)
-          .to_string(),
-      );
-    } else {
-      input.push_str(
-        &percent_encoding::utf8_percent_encode(path, URL_TO_URI_PATH)
-          .to_string(),
-      );
-    }
+  let path = url.path();
+  let mut chars = path.chars();
+  let has_drive_letter = chars.next().is_some_and(|c| c == '/')
+    && chars.next().is_some_and(|c| c.is_ascii_alphabetic())
+    && chars.next().is_some_and(|c| c == ':')
+    && chars.next().is_none_or(|c| c == '/');
+  if has_drive_letter {
+    let (dl_part, rest) = path.split_at(2);
+    input.push_str(&dl_part.to_ascii_lowercase());
+    input.push_str(
+      &percent_encoding::utf8_percent_encode(rest, URL_TO_URI_PATH).to_string(),
+    );
   } else {
     input.push_str(
-      &percent_encoding::utf8_percent_encode(url.path(), URL_TO_URI_PATH)
-        .to_string(),
+      &percent_encoding::utf8_percent_encode(path, URL_TO_URI_PATH).to_string(),
     );
   }
   if let Some(query) = url.query() {
@@ -142,8 +136,9 @@ pub fn uri_to_url(uri: &Uri) -> Url {
         .trim_start_matches('/'),
     ))
     .ok()
+    .map(normalize_url)
   })()
-  .unwrap_or_else(|| Url::parse(uri.as_str()).unwrap())
+  .unwrap_or_else(|| normalize_url(Url::parse(uri.as_str()).unwrap()))
 }
 
 pub fn uri_to_file_path(uri: &Uri) -> Result<PathBuf, UrlToFilePathError> {
@@ -160,3 +155,53 @@ pub fn uri_is_file_like(uri: &Uri) -> bool {
     || scheme.eq_lowercase("deno-notebook-cell")
     || scheme.eq_lowercase("vscode-userdata")
 }
+
+fn normalize_url(url: Url) -> Url {
+  let Ok(path) = url_to_file_path(&url) else {
+    return url;
+  };
+  let normalized_path = normalize_path(&path);
+  let Ok(normalized_url) = Url::from_file_path(&normalized_path) else {
+    return url;
+  };
+  normalized_url
+}
+
+// TODO(nayeemrmn): Change the version of this in deno_path_util to force
+// uppercase on drive letters. Then remove this.
+fn normalize_path<P: AsRef<Path>>(path: P) -> PathBuf {
+  fn inner(path: &Path) -> PathBuf {
+    let mut components = path.components().peekable();
+    let mut ret =
+      if let Some(c @ Component::Prefix(..)) = components.peek().cloned() {
+        components.next();
+        let s = c.as_os_str();
+        if s.len() == 2 {
+          PathBuf::from(s.to_ascii_uppercase())
+        } else {
+          PathBuf::from(s)
+        }
+      } else {
+        PathBuf::new()
+      };
+
+    for component in components {
+      match component {
+        Component::Prefix(..) => unreachable!(),
+        Component::RootDir => {
+          ret.push(component.as_os_str());
+        }
+        Component::CurDir => {}
+        Component::ParentDir => {
+          ret.pop();
+        }
+        Component::Normal(c) => {
+          ret.push(c);
+        }
+      }
+    }
+    ret
+  }
+
+  inner(path.as_ref())
+}

filename: tests/util/server/src/fs.rs
patch: @@ -22,6 +22,79 @@ use url::Url;
 use crate::assertions::assert_wildcard_match;
 use crate::testdata_path;
 
+/// Characters that are left unencoded in a `Url` path but will be encoded in a
+/// VSCode URI.
+const URL_TO_URI_PATH: &percent_encoding::AsciiSet =
+  &percent_encoding::CONTROLS
+    .add(b' ')
+    .add(b'!')
+    .add(b'$')
+    .add(b'&')
+    .add(b'\'')
+    .add(b'(')
+    .add(b')')
+    .add(b'*')
+    .add(b'+')
+    .add(b',')
+    .add(b':')
+    .add(b';')
+    .add(b'=')
+    .add(b'@')
+    .add(b'[')
+    .add(b']')
+    .add(b'^')
+    .add(b'|');
+
+/// Characters that may be left unencoded in a `Url` query but not valid in a
+/// `Uri` query.
+const URL_TO_URI_QUERY: &percent_encoding::AsciiSet =
+  &URL_TO_URI_PATH.add(b'\\').add(b'`').add(b'{').add(b'}');
+
+/// Characters that may be left unencoded in a `Url` fragment but not valid in
+/// a `Uri` fragment.
+const URL_TO_URI_FRAGMENT: &percent_encoding::AsciiSet =
+  &URL_TO_URI_PATH.add(b'#').add(b'\\').add(b'{').add(b'}');
+
+pub fn url_to_uri(url: &Url) -> Result<Uri, anyhow::Error> {
+  let components = url::quirks::internal_components(url);
+  let mut input = String::with_capacity(url.as_str().len());
+  input.push_str(&url.as_str()[..components.path_start as usize]);
+  let path = url.path();
+  let mut chars = path.chars();
+  let has_drive_letter = chars.next().is_some_and(|c| c == '/')
+    && chars.next().is_some_and(|c| c.is_ascii_alphabetic())
+    && chars.next().is_some_and(|c| c == ':')
+    && chars.next().is_none_or(|c| c == '/');
+  if has_drive_letter {
+    let (dl_part, rest) = path.split_at(2);
+    input.push_str(&dl_part.to_ascii_lowercase());
+    input.push_str(
+      &percent_encoding::utf8_percent_encode(rest, URL_TO_URI_PATH).to_string(),
+    );
+  } else {
+    input.push_str(
+      &percent_encoding::utf8_percent_encode(path, URL_TO_URI_PATH).to_string(),
+    );
+  }
+  if let Some(query) = url.query() {
+    input.push('?');
+    input.push_str(
+      &percent_encoding::utf8_percent_encode(query, URL_TO_URI_QUERY)
+        .to_string(),
+    );
+  }
+  if let Some(fragment) = url.fragment() {
+    input.push('#');
+    input.push_str(
+      &percent_encoding::utf8_percent_encode(fragment, URL_TO_URI_FRAGMENT)
+        .to_string(),
+    );
+  }
+  Uri::from_str(&input).map_err(|err| {
+    anyhow::anyhow!("Could not convert URL \"{url}\" to URI: {err}")
+  })
+}
+
 /// Represents a path on the file system, which can be used
 /// to perform specific actions.
 #[derive(Clone, Debug, Default, PartialEq, Eq, Hash)]
@@ -63,11 +136,11 @@ impl PathRef {
   }
 
   pub fn uri_dir(&self) -> Uri {
-    Uri::from_str(self.url_dir().as_str()).unwrap()
+    url_to_uri(&self.url_dir()).unwrap()
   }
 
   pub fn uri_file(&self) -> Uri {
-    Uri::from_str(self.url_file().as_str()).unwrap()
+    url_to_uri(&self.url_file()).unwrap()
   }
 
   pub fn as_path(&self) -> &Path {
@@ -475,7 +548,7 @@ impl TempDir {
   }
 
   pub fn uri(&self) -> Uri {
-    Uri::from_str(self.url().as_str()).unwrap()
+    url_to_uri(&self.url()).unwrap()
   }
 
   pub fn path(&self) -> &PathRef {

filename: tests/util/server/src/lib.rs
patch: @@ -40,6 +40,7 @@ pub use builders::TestCommandBuilder;
 pub use builders::TestCommandOutput;
 pub use builders::TestContext;
 pub use builders::TestContextBuilder;
+pub use fs::url_to_uri;
 pub use fs::PathRef;
 pub use fs::TempDir;
 

filename: tests/util/server/src/lsp.rs
patch: @@ -14,7 +14,6 @@ use std::process::ChildStdin;
 use std::process::ChildStdout;
 use std::process::Command;
 use std::process::Stdio;
-use std::str::FromStr;
 use std::sync::mpsc;
 use std::sync::Arc;
 use std::time::Duration;
@@ -34,6 +33,7 @@ use lsp_types::FoldingRangeClientCapabilities;
 use lsp_types::InitializeParams;
 use lsp_types::TextDocumentClientCapabilities;
 use lsp_types::TextDocumentSyncClientCapabilities;
+use lsp_types::Uri;
 use lsp_types::WorkspaceClientCapabilities;
 use once_cell::sync::Lazy;
 use parking_lot::Condvar;
@@ -291,13 +291,12 @@ impl InitializeParamsBuilder {
   }
 
   #[allow(deprecated)]
-  pub fn set_maybe_root_uri(&mut self, value: Option<Url>) -> &mut Self {
-    self.params.root_uri =
-      value.map(|v| lsp::Uri::from_str(v.as_str()).unwrap());
+  pub fn set_maybe_root_uri(&mut self, value: Option<Uri>) -> &mut Self {
+    self.params.root_uri = value;
     self
   }
 
-  pub fn set_root_uri(&mut self, value: Url) -> &mut Self {
+  pub fn set_root_uri(&mut self, value: Uri) -> &mut Self {
     self.set_maybe_root_uri(Some(value))
   }
 
@@ -854,7 +853,7 @@ impl LspClient {
     mut config: Value,
   ) {
     let mut builder = InitializeParamsBuilder::new(config.clone());
-    builder.set_root_uri(self.root_dir.url_dir());
+    builder.set_root_uri(self.root_dir.uri_dir());
     do_build(&mut builder);
     let params: InitializeParams = builder.build();
     // `config` must be updated to account for the builder changes.
@@ -1228,11 +1227,11 @@ impl CollectedDiagnostics {
       .collect()
   }
 
-  pub fn for_file(&self, specifier: &Url) -> Vec<lsp::Diagnostic> {
+  pub fn for_file(&self, uri: &Uri) -> Vec<lsp::Diagnostic> {
     self
       .all_messages()
       .iter()
-      .filter(|p| p.uri.as_str() == specifier.as_str())
+      .filter(|p| p.uri.as_str() == uri.as_str())
       .flat_map(|p| p.diagnostics.iter())
       .cloned()
       .collect()