Consistently send verbatim URLs back to frontend (#1268)

Branched from #1267
Progress towards #1212

This PR straightens out the handing of verbatim wire URLs back to the
frontend, so that our lexically normalised paths don't leak out (see
#1250).

- New `WorldState::wire_url()` method that resolves verbatim URLs for
open files. The LSP layer calls it when communicating URLs back to the
frontend, instead of the lexically-normalised paths that travel through
the analysis layers.

- The goto-definition, find-references, and rename handlers now use that
method to create wire URLs.

- `indexer::map()` now hands `File` instead of URLs, and callers that
need a URL call `wire_url()`.

Note that `ArkFile` still contains a copy of the verbatim URLs because
Ark's diagnostics and roxygen handlers still use them. This is a
temporary situation until these handlers are updated to the modern
layering.

Author: lionel-

crates/ark/src/lsp/completions/sources/composite/workspace.rs

@@ -69,13 +69,15 @@ fn completions_from_workspace(
     let token = token.as_str();
 
     // get entries from the index
-    indexer::map(&state.db, |uri, symbol, entry| {
+    indexer::map(&state.db, |file, symbol, entry| {
         if !symbol.fuzzy_matches(token) {
             return;
         }
 
         match &entry.data {
             indexer::IndexEntryData::Function { name, .. } => {
+                let uri = state.wire_url(file);
+
                 let fun_context = match completion_context.function_context() {
                     Ok(fun_context) => fun_context,
                     Err(err) => {

crates/ark/src/lsp/diagnostics.rs

@@ -171,7 +171,7 @@ pub(crate) fn generate_diagnostics(
     context.document_symbols.push(HashMap::new());
 
     // Add the current workspace symbols.
-    indexer::map(db, |_uri, _symbol, entry| match &entry.data {
+    indexer::map(db, |_file, _symbol, entry| match &entry.data {
         indexer::IndexEntryData::Function { name, arguments: _ } => {
             context.workspace_symbols.insert(name.to_string());
         },

crates/ark/src/lsp/find_references.rs

@@ -30,7 +30,7 @@ pub(crate) fn find_references(
         .iter()
         .filter_map(|fr| {
             let range = to_proto::range(fr.range, fr.file.line_index(db), encoding).log_err()?;
-            Some(Location::new(fr.file.path(db).to_url(), range))
+            Some(Location::new(state.wire_url(fr.file), range))
         })
         .collect();
 

crates/ark/src/lsp/goto_definition.rs

@@ -35,7 +35,7 @@ pub(crate) fn goto_definition(
     // to several bindings; the client offers all of them.
     let links = targets
         .iter()
-        .map(|target| nav_target_to_link(db, encoding, target))
+        .map(|target| nav_target_to_link(state, encoding, target))
         .collect::<anyhow::Result<Vec<_>>>()?;
 
     Ok(Some(GotoDefinitionResponse::Link(links)))
@@ -45,17 +45,18 @@ pub(crate) fn goto_definition(
 /// offsets in the target file, so we translate them through that file's own
 /// line index, not the file the request came from.
 fn nav_target_to_link(
-    db: &dyn Db,
+    state: &WorldState,
     encoding: PositionEncoding,
     target: &NavigationTarget,
 ) -> anyhow::Result<LocationLink> {
+    let db = &state.db;
     let line_index = target.file.line_index(db);
     let target_range = to_proto::range(target.full_range, line_index, encoding)?;
     let target_selection_range = to_proto::range(target.focus_range, line_index, encoding)?;
 
     Ok(LocationLink {
         origin_selection_range: None,
-        target_uri: target.file.path(db).to_url(),
+        target_uri: state.wire_url(target.file),
         target_range,
         target_selection_range,
     })

crates/ark/src/lsp/indexer.rs

@@ -10,7 +10,6 @@ use std::sync::LazyLock;
 
 use aether_lsp_utils::proto::to_proto;
 use aether_lsp_utils::proto::PositionEncoding;
-use aether_path::FilePath;
 use oak_db::File;
 use regex::Regex;
 use stdext::result::ResultExt;
@@ -19,7 +18,6 @@ use stdext::unwrap::IntoResult;
 use tower_lsp::lsp_types::Range;
 use tree_sitter::Node;
 use tree_sitter::Query;
-use url::Url;
 
 use crate::lsp;
 use crate::lsp::db::ArkDb;
@@ -82,16 +80,14 @@ pub struct IndexEntry {
 }
 
 /// Convert an index entry's tree-sitter point range to an LSP range, resolving
-/// the file's line index from the db. Returns `None` if the file is no longer
-/// in the db or the points fall outside it, in which case the symbol is
-/// dropped from the results.
+/// the file's line index from the db. Returns `None` if the points fall outside
+/// the line index, in which case the symbol is dropped from the results.
 pub(crate) fn index_range_to_lsp_range(
     db: &dyn ArkDb,
-    uri: &Url,
+    file: File,
     range: IndexRange,
     encoding: PositionEncoding,
 ) -> Option<Range> {
-    let file = db.file_by_path(&FilePath::from_url(uri))?;
     let line_index = file.line_index(db);
 
     let to_position = |point: IndexPoint| {
@@ -160,15 +156,16 @@ fn file_index(db: &dyn ArkDb, file: File) -> FileIndex {
     FileIndex { symbols }
 }
 
-/// Visit every workspace symbol across all indexable files.
-pub(crate) fn map(db: &dyn ArkDb, mut callback: impl FnMut(&Url, &str, &IndexEntry)) {
+/// Visit every workspace symbol across all indexable files. Callers that need a
+/// URL for messages to the frontend can resolve it from `File` via
+/// `WorldState::wire_url`.
+pub(crate) fn map(db: &dyn ArkDb, mut callback: impl FnMut(File, &str, &IndexEntry)) {
     for &file in oak_db::workspace_files(db) {
         if !is_indexable(db, file) {
             continue;
         }
-        let url = file.path(db).to_url();
         for (symbol, entry) in file_index(db, file).symbols.iter() {
-            callback(&url, symbol, entry);
+            callback(file, symbol, entry);
         }
     }
 }
@@ -380,6 +377,7 @@ mod tests {
     use assert_matches::assert_matches;
     use insta::assert_debug_snapshot;
     use oak_scan::DbScan;
+    use url::Url;
 
     use super::*;
     use crate::lsp::ark_file::test_ark_file;

crates/ark/src/lsp/rename.rs

@@ -66,7 +66,7 @@ pub(crate) fn rename(
     let mut changes: HashMap<lsp_types::Url, Vec<TextEdit>> = HashMap::new();
     for range in ranges {
         let line_index = range.file.line_index(db);
-        let target_url = range.file.path(db).to_url();
+        let target_url = state.wire_url(range.file);
         let range = to_proto::range(range.range, line_index, encoding)?;
         changes.entry(target_url).or_default().push(TextEdit {
             range,

crates/ark/src/lsp/state.rs

@@ -120,6 +120,18 @@ impl WorldState {
         Ok(ark_file.clone())
     }
 
+    /// URL to put on the wire for `file`. Open buffers keep the editor's
+    /// verbatim URL so the frontend sees the URI it sent us. Files that were
+    /// never opened in the editor (disk-scanned files, resolution targets) have
+    /// no verbatim URL, so synthesise one from the normalised path.
+    pub(crate) fn wire_url(&self, file: File) -> Url {
+        let path = file.path(&self.db);
+        self.open_files
+            .get(path)
+            .map(|ark_file| ark_file.url.clone())
+            .unwrap_or_else(|| path.to_url())
+    }
+
     /// Register an editor buffer in `open_files`, keying on the normalised
     /// [`FilePath`] and stashing the verbatim editor URL on [`ArkFile::url`] for
     /// wire output.

crates/ark/src/lsp/symbols.rs

@@ -64,24 +64,23 @@ pub(crate) fn symbols(
     let encoding = state.config.position_encoding;
     let mut info: Vec<SymbolInformation> = Vec::new();
 
-    indexer::map(db, |uri, symbol, entry| {
+    indexer::map(db, |file, symbol, entry| {
         if !symbol.fuzzy_matches(query) {
             return;
         }
 
-        let Some(range) = indexer::index_range_to_lsp_range(db, uri, entry.range, encoding) else {
+        let Some(range) = indexer::index_range_to_lsp_range(db, file, entry.range, encoding) else {
             return;
         };
 
+        let uri = state.wire_url(file);
+
         match &entry.data {
             IndexEntryData::Function { name, arguments: _ } => {
                 info.push(SymbolInformation {
                     name: name.to_string(),
                     kind: SymbolKind::FUNCTION,
-                    location: Location {
-                        uri: uri.clone(),
-                        range,
-                    },
+                    location: Location { uri, range },
                     tags: None,
                     deprecated: None,
                     container_name: None,
@@ -93,10 +92,7 @@ pub(crate) fn symbols(
                     info.push(SymbolInformation {
                         name: title.to_string(),
                         kind: SymbolKind::STRING,
-                        location: Location {
-                            uri: uri.clone(),
-                            range,
-                        },
+                        location: Location { uri, range },
                         tags: None,
                         deprecated: None,
                         container_name: None,
@@ -108,10 +104,7 @@ pub(crate) fn symbols(
                 info.push(SymbolInformation {
                     name: name.clone(),
                     kind: SymbolKind::VARIABLE,
-                    location: Location {
-                        uri: uri.clone(),
-                        range,
-                    },
+                    location: Location { uri, range },
                     tags: None,
                     deprecated: None,
                     container_name: None,
@@ -122,10 +115,7 @@ pub(crate) fn symbols(
                 info.push(SymbolInformation {
                     name: name.clone(),
                     kind: SymbolKind::METHOD,
-                    location: Location {
-                        uri: uri.clone(),
-                        range,
-                    },
+                    location: Location { uri, range },
                     tags: None,
                     deprecated: None,
                     container_name: None,

crates/ark/src/lsp/tests.rs

@@ -4,5 +4,6 @@ mod find_references;
 mod goto_definition;
 mod main_loop;
 mod rename;
+mod state;
 mod state_handlers;
 mod utils;

crates/ark/src/lsp/tests/find_references.rs

@@ -1,3 +1,4 @@
+use aether_path::FilePath;
 use tower_lsp::lsp_types;
 use tower_lsp::lsp_types::Location;
 use tower_lsp::lsp_types::ReferenceContext;
@@ -170,6 +171,24 @@ fn test_cross_file_dollar_kind() {
         .any(|l| l.uri == uri2 && l.range == range((2, 0), (2, 3))));
 }
 
+#[test]
+fn test_locations_use_verbatim_url() {
+    // The doubled slash survives in the editor's URL but `FilePath` normalises
+    // it away, so a path round-trip would change it. Locations must carry the
+    // exact URI the editor opened the buffer with.
+    let code = "foo <- 1\nfoo\n";
+    let uri = lsp_types::Url::parse("file:///C:/proj//foo.R").unwrap();
+    let state = make_state(&uri, code);
+
+    let params = make_params(uri.clone(), 1, 0, true);
+    let locs = find_references(params, &state).unwrap();
+
+    assert!(!locs.is_empty());
+    assert!(locs.iter().all(|loc| loc.uri == uri));
+    // Confirm the round-trip really would have differed, so the check above bites.
+    assert_ne!(FilePath::from_url(&uri).to_url(), uri);
+}
+
 #[test]
 fn test_cross_file_namespace_access() {
     // Cursor on `mutate` in `dplyr::mutate` -- structural scan matches every