Merge #4867

4867: Cleanup URL handling r=matklad a=matklad



bors r+
🤖

Co-authored-by: Aleksey Kladov <aleksey.kladov@gmail.com>

Author: bors[bot]

crates/rust-analyzer/src/diagnostics/to_proto.rs

@@ -1,10 +1,6 @@
 //! This module provides the functionality needed to convert diagnostics from
 //! `cargo check` json format to the LSP diagnostic format.
-use std::{
-    collections::HashMap,
-    path::{Component, Path, Prefix},
-    str::FromStr,
-};
+use std::{collections::HashMap, path::Path};
 
 use lsp_types::{
     Diagnostic, DiagnosticRelatedInformation, DiagnosticSeverity, DiagnosticTag, Location,
@@ -13,7 +9,7 @@ use lsp_types::{
 use ra_flycheck::{Applicability, DiagnosticLevel, DiagnosticSpan, DiagnosticSpanMacroExpansion};
 use stdx::format_to;
 
-use crate::{lsp_ext, Result};
+use crate::{lsp_ext, to_proto::url_from_abs_path};
 
 /// Converts a Rust level string to a LSP severity
 fn map_level_to_severity(val: DiagnosticLevel) -> Option<DiagnosticSeverity> {
@@ -65,7 +61,7 @@ fn map_span_to_location(span: &DiagnosticSpan, workspace_root: &Path) -> Locatio
 fn map_span_to_location_naive(span: &DiagnosticSpan, workspace_root: &Path) -> Location {
     let mut file_name = workspace_root.to_path_buf();
     file_name.push(&span.file_name);
-    let uri = url_from_path_with_drive_lowercasing(file_name).unwrap();
+    let uri = url_from_abs_path(&file_name);
 
     // FIXME: this doesn't handle UTF16 offsets correctly
     let range = Range::new(
@@ -274,70 +270,16 @@ pub(crate) fn map_rust_diagnostic_to_lsp(
         .collect()
 }
 
-/// Returns a `Url` object from a given path, will lowercase drive letters if present.
-/// This will only happen when processing windows paths.
-///
-/// When processing non-windows path, this is essentially the same as `Url::from_file_path`.
-pub fn url_from_path_with_drive_lowercasing(path: impl AsRef<Path>) -> Result<Url> {
-    let component_has_windows_drive = path.as_ref().components().any(|comp| {
-        if let Component::Prefix(c) = comp {
-            return matches!(c.kind(), Prefix::Disk(_) | Prefix::VerbatimDisk(_));
-        }
-        false
-    });
-
-    // VSCode expects drive letters to be lowercased, where rust will uppercase the drive letters.
-    let res = if component_has_windows_drive {
-        let url_original = Url::from_file_path(&path)
-            .map_err(|_| format!("can't convert path to url: {}", path.as_ref().display()))?;
-
-        let drive_partition: Vec<&str> = url_original.as_str().rsplitn(2, ':').collect();
-
-        // There is a drive partition, but we never found a colon.
-        // This should not happen, but in this case we just pass it through.
-        if drive_partition.len() == 1 {
-            return Ok(url_original);
-        }
-
-        let joined = drive_partition[1].to_ascii_lowercase() + ":" + drive_partition[0];
-        let url = Url::from_str(&joined).expect("This came from a valid `Url`");
-
-        url
-    } else {
-        Url::from_file_path(&path)
-            .map_err(|_| format!("can't convert path to url: {}", path.as_ref().display()))?
-    };
-    Ok(res)
-}
-
 #[cfg(test)]
+#[cfg(not(windows))]
 mod tests {
     use super::*;
 
-    // `Url` is not able to parse windows paths on unix machines.
-    #[test]
-    #[cfg(target_os = "windows")]
-    fn test_lowercase_drive_letter_with_drive() {
-        let url = url_from_path_with_drive_lowercasing("C:\\Test").unwrap();
-
-        assert_eq!(url.to_string(), "file:///c:/Test");
-    }
-
-    #[test]
-    #[cfg(target_os = "windows")]
-    fn test_drive_without_colon_passthrough() {
-        let url = url_from_path_with_drive_lowercasing(r#"\\localhost\C$\my_dir"#).unwrap();
-
-        assert_eq!(url.to_string(), "file://localhost/C$/my_dir");
-    }
-
-    #[cfg(not(windows))]
     fn parse_diagnostic(val: &str) -> ra_flycheck::Diagnostic {
         serde_json::from_str::<ra_flycheck::Diagnostic>(val).unwrap()
     }
 
     #[test]
-    #[cfg(not(windows))]
     fn snap_rustc_incompatible_type_for_trait() {
         let diag = parse_diagnostic(
             r##"{
@@ -391,7 +333,6 @@ mod tests {
     }
 
     #[test]
-    #[cfg(not(windows))]
     fn snap_rustc_unused_variable() {
         let diag = parse_diagnostic(
             r##"{
@@ -474,7 +415,6 @@ mod tests {
     }
 
     #[test]
-    #[cfg(not(windows))]
     fn snap_rustc_wrong_number_of_parameters() {
         let diag = parse_diagnostic(
             r##"{
@@ -599,7 +539,6 @@ mod tests {
     }
 
     #[test]
-    #[cfg(not(windows))]
     fn snap_clippy_pass_by_ref() {
         let diag = parse_diagnostic(
             r##"{
@@ -720,7 +659,6 @@ mod tests {
     }
 
     #[test]
-    #[cfg(not(windows))]
     fn snap_rustc_mismatched_type() {
         let diag = parse_diagnostic(
             r##"{
@@ -764,7 +702,6 @@ mod tests {
     }
 
     #[test]
-    #[cfg(not(windows))]
     fn snap_handles_macro_location() {
         let diag = parse_diagnostic(
             r##"{
@@ -1036,7 +973,6 @@ mod tests {
     }
 
     #[test]
-    #[cfg(not(windows))]
     fn snap_macro_compiler_error() {
         let diag = parse_diagnostic(
             r##"{
@@ -1266,7 +1202,6 @@ mod tests {
     }
 
     #[test]
-    #[cfg(not(windows))]
     fn snap_multi_line_fix() {
         let diag = parse_diagnostic(
             r##"{

crates/rust-analyzer/src/from_proto.rs

@@ -17,7 +17,7 @@ pub(crate) fn text_range(line_index: &LineIndex, range: lsp_types::Range) -> Tex
 }
 
 pub(crate) fn file_id(world: &GlobalStateSnapshot, url: &lsp_types::Url) -> Result<FileId> {
-    world.uri_to_file_id(url)
+    world.url_to_file_id(url)
 }
 
 pub(crate) fn file_position(

crates/rust-analyzer/src/global_state.rs

@@ -22,10 +22,9 @@ use stdx::format_to;
 
 use crate::{
     config::Config,
-    diagnostics::{
-        to_proto::url_from_path_with_drive_lowercasing, CheckFixes, DiagnosticCollection,
-    },
+    diagnostics::{CheckFixes, DiagnosticCollection},
     main_loop::pending_requests::{CompletedRequest, LatestRequests},
+    to_proto::url_from_abs_path,
     vfs_glob::{Glob, RustPackageFilterBuilder},
     LspError, Result,
 };
@@ -274,8 +273,8 @@ impl GlobalStateSnapshot {
         &self.analysis
     }
 
-    pub fn uri_to_file_id(&self, uri: &Url) -> Result<FileId> {
-        let path = uri.to_file_path().map_err(|()| format!("invalid uri: {}", uri))?;
+    pub fn url_to_file_id(&self, url: &Url) -> Result<FileId> {
+        let path = url.to_file_path().map_err(|()| format!("invalid uri: {}", url))?;
         let file = self.vfs.read().path2file(&path).ok_or_else(|| {
             // Show warning as this file is outside current workspace
             // FIXME: just handle such files, and remove `LspError::UNKNOWN_FILE`.
@@ -287,11 +286,8 @@ impl GlobalStateSnapshot {
         Ok(FileId(file.0))
     }
 
-    pub fn file_id_to_uri(&self, id: FileId) -> Result<Url> {
-        let path = self.vfs.read().file2path(VfsFile(id.0));
-        let url = url_from_path_with_drive_lowercasing(path)?;
-
-        Ok(url)
+    pub fn file_id_to_url(&self, id: FileId) -> Url {
+        file_id_to_url(&self.vfs.read(), id)
     }
 
     pub fn file_id_to_path(&self, id: FileId) -> PathBuf {
@@ -302,12 +298,10 @@ impl GlobalStateSnapshot {
         self.vfs.read().file_line_endings(VfsFile(id.0))
     }
 
-    pub fn path_to_uri(&self, root: SourceRootId, path: &RelativePathBuf) -> Result<Url> {
+    pub fn path_to_url(&self, root: SourceRootId, path: &RelativePathBuf) -> Url {
         let base = self.vfs.read().root2path(VfsRoot(root.0));
         let path = path.to_path(base);
-        let url = Url::from_file_path(&path)
-            .map_err(|_| format!("can't convert path to url: {}", path.display()))?;
-        Ok(url)
+        url_from_abs_path(&path)
     }
 
     pub fn status(&self) -> String {
@@ -335,3 +329,8 @@ impl GlobalStateSnapshot {
         self.workspaces.iter().find_map(|ws| ws.workspace_root_for(&path))
     }
 }
+
+pub(crate) fn file_id_to_url(vfs: &Vfs, id: FileId) -> Url {
+    let path = vfs.file2path(VfsFile(id.0));
+    url_from_abs_path(&path)
+}

crates/rust-analyzer/src/main_loop.rs

@@ -27,17 +27,17 @@ use ra_flycheck::{CheckTask, Status};
 use ra_ide::{Canceled, FileId, LibraryData, LineIndex, SourceRootId};
 use ra_prof::profile;
 use ra_project_model::{PackageRoot, ProjectWorkspace};
-use ra_vfs::{VfsFile, VfsTask, Watch};
+use ra_vfs::{VfsTask, Watch};
 use relative_path::RelativePathBuf;
 use rustc_hash::FxHashSet;
 use serde::{de::DeserializeOwned, Serialize};
 use threadpool::ThreadPool;
 
 use crate::{
     config::{Config, FilesWatcher, LinkedProject},
-    diagnostics::{to_proto::url_from_path_with_drive_lowercasing, DiagnosticTask},
+    diagnostics::DiagnosticTask,
     from_proto,
-    global_state::{GlobalState, GlobalStateSnapshot},
+    global_state::{file_id_to_url, GlobalState, GlobalStateSnapshot},
     lsp_ext,
     main_loop::{
         pending_requests::{PendingRequest, PendingRequests},
@@ -801,17 +801,9 @@ fn on_diagnostic_task(task: DiagnosticTask, msg_sender: &Sender<Message>, state:
     let subscriptions = state.diagnostics.handle_task(task);
 
     for file_id in subscriptions {
-        let path = state.vfs.read().file2path(VfsFile(file_id.0));
-        let uri = match url_from_path_with_drive_lowercasing(&path) {
-            Ok(uri) => uri,
-            Err(err) => {
-                log::error!("Couldn't convert path to url ({}): {}", err, path.display());
-                continue;
-            }
-        };
-
+        let url = file_id_to_url(&state.vfs.read(), file_id);
         let diagnostics = state.diagnostics.diagnostics_for(file_id).cloned().collect();
-        let params = lsp_types::PublishDiagnosticsParams { uri, diagnostics, version: None };
+        let params = lsp_types::PublishDiagnosticsParams { uri: url, diagnostics, version: None };
         let not = notification_new::<lsp_types::notification::PublishDiagnostics>(params);
         msg_sender.send(not.into()).unwrap();
     }

crates/rust-analyzer/src/main_loop/handlers.rs

@@ -258,7 +258,7 @@ pub fn handle_document_symbol(
     let res = if snap.config.client_caps.hierarchical_symbols {
         document_symbols.into()
     } else {
-        let url = to_proto::url(&snap, file_id)?;
+        let url = to_proto::url(&snap, file_id);
         let mut symbol_information = Vec::<SymbolInformation>::new();
         for symbol in document_symbols {
             flatten_document_symbol(&symbol, None, &url, &mut symbol_information);
@@ -1160,7 +1160,7 @@ fn show_impl_command_link(
 ) -> Option<lsp_ext::CommandLinkGroup> {
     if snap.config.hover.implementations {
         if let Some(nav_data) = snap.analysis().goto_implementation(*position).unwrap_or(None) {
-            let uri = to_proto::url(snap, position.file_id).ok()?;
+            let uri = to_proto::url(snap, position.file_id);
             let line_index = snap.analysis().file_line_index(position.file_id).ok()?;
             let position = to_proto::position(&line_index, position.offset);
             let locations: Vec<_> = nav_data