Skip to content

Map LSP documents by normalised FilePath #1252

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
lionel- wants to merge 1 commit into
base: oak/salsa-15-file-path-refactor
Choose a base branch
from
Open

Map LSP documents by normalised FilePath #1252

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions crates/ark/src/lsp/document.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,26 @@ use aether_lsp_utils::proto::PositionEncoding;
use anyhow::anyhow;
use oak_semantic::semantic_index::SemanticIndex;
use tower_lsp::lsp_types;
use tower_lsp::lsp_types::Url;
use tree_sitter::Parser;
use tree_sitter::Tree;

use crate::lsp::config::DocumentConfig;

/// Placeholder URL used by `Document::new`. Production paths replace
/// this with the editor's verbatim URL via `WorldState::insert_document`.
/// Tests that don't go through `WorldState` leave the placeholder.
const PLACEHOLDER_URL: &str = "ark://internal/document-url-unset";

#[derive(Clone)]
pub struct Document {
/// The editor's URL for this document, byte-for-byte as received in
/// `did_open`. Used for wire output (diagnostics, etc.) so the
/// frontend always sees the URI it sent us, never a normalised
/// round-trip. `WorldState::insert_document` populates this; the
/// default constructor uses a placeholder.
pub url: Url,

/// The document's textual contents.
pub contents: String,

Expand Down Expand Up @@ -76,6 +89,7 @@ impl Document {
let line_index = biome_line_index::LineIndex::new(&contents);

Self {
url: Url::parse(PLACEHOLDER_URL).expect("placeholder URL parses"),
contents,
version,
ast,
Expand Down
76 changes: 24 additions & 52 deletions crates/ark/src/lsp/goto_definition.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ mod parked_salsa_tests {

fn make_state(uri: &lsp_types::Url, doc: &Document) -> WorldState {
let mut state = WorldState::default();
state.documents.insert(uri.clone(), doc.clone());
state.insert_document(uri.clone(), doc.clone());
state
}

Expand Down Expand Up @@ -229,8 +229,8 @@ mod parked_salsa_tests {
let pkg = Package::from_parts(pkg_root, desc, ns);

let mut state = WorldState::default();
state.documents.insert(uri_aaa.clone(), doc_aaa.clone());
state.documents.insert(uri_ccc.clone(), doc_ccc.clone());
state.insert_document(uri_aaa.clone(), doc_aaa.clone());
state.insert_document(uri_ccc.clone(), doc_ccc.clone());
state.root = Some(SourceRoot::Package(pkg));

// ccc.R sees bbb.R's definition (later in collation)
Expand Down Expand Up @@ -283,9 +283,9 @@ mod parked_salsa_tests {
let pkg = Package::from_parts(pkg_root, desc, ns);

let mut state = WorldState::default();
state.documents.insert(uri_aaa.clone(), doc_aaa.clone());
state.documents.insert(uri_bbb.clone(), doc_bbb);
state.documents.insert(uri_ccc.clone(), doc_ccc.clone());
state.insert_document(uri_aaa.clone(), doc_aaa.clone());
state.insert_document(uri_bbb.clone(), doc_bbb);
state.insert_document(uri_ccc.clone(), doc_ccc.clone());
state.root = Some(SourceRoot::Package(pkg));

// aaa.R is now last in collation, so it can see bbb.R's definition
Expand Down Expand Up @@ -332,7 +332,7 @@ mod parked_salsa_tests {
let pkg = Package::from_parts(pkg_root, desc, ns);

let mut state = WorldState::default();
state.documents.insert(uri_aaa.clone(), doc_aaa.clone());
state.insert_document(uri_aaa.clone(), doc_aaa.clone());
state.root = Some(SourceRoot::Package(pkg));

// Cursor on `helper` inside the function body (line 0, col 16)
Expand Down Expand Up @@ -464,7 +464,7 @@ mod parked_salsa_tests {
// `is.null` is NOT in the INDEX
let doc_null = Document::new("is.null(1)\n", None);
let uri_null = lsp_types::Url::from_file_path(pkg_root.join("R/bar.R")).unwrap();
state.documents.insert(uri_null.clone(), doc_null.clone());
state.insert_document(uri_null.clone(), doc_null.clone());

let params = make_params(uri_null, 0, 0);
let result = goto_definition(&doc_null, params, &state).unwrap();
Expand Down Expand Up @@ -523,9 +523,7 @@ mod parked_salsa_tests {
let helpers_uri = lsp_types::Url::from_file_path(dir.path().join("helpers.R")).unwrap();

let mut state = WorldState::default();
state
.documents
.insert(script_uri.clone(), script_doc.clone());
state.insert_document(script_uri.clone(), script_doc.clone());
state.workspace.folders = vec![lsp_types::Url::from_directory_path(dir.path()).unwrap()];

let params = make_params(script_uri, 1, 0);
Expand All @@ -551,10 +549,8 @@ mod parked_salsa_tests {
let script_uri = lsp_types::Url::from_file_path(script_dir.join("script.R")).unwrap();

let mut state = WorldState::default();
state.documents.insert(helpers_uri.clone(), helpers_doc);
state
.documents
.insert(script_uri.clone(), script_doc.clone());
state.insert_document(helpers_uri.clone(), helpers_doc);
state.insert_document(script_uri.clone(), script_doc.clone());

// Cursor on `helper` (line 1, col 0)
let params = make_params(script_uri, 1, 0);
Expand Down Expand Up @@ -594,7 +590,7 @@ mod parked_salsa_tests {

let mut state = make_state(&script_uri, &script_doc);
state.library = library;
state.documents.insert(helpers_uri.clone(), helpers_doc);
state.insert_document(helpers_uri.clone(), helpers_doc);

// `mutate` (line 1) resolves via dplyr, attached by helpers.R's library() call.
// Package symbol, no NavigationTarget.
Expand Down Expand Up @@ -634,9 +630,7 @@ mod parked_salsa_tests {
lsp_types::Url::from_file_path(script_dir.path().join("script.R")).unwrap();

let mut state = WorldState::default();
state
.documents
.insert(script_uri.clone(), script_doc.clone());
state.insert_document(script_uri.clone(), script_doc.clone());
// helpers.R is intentionally NOT inserted into state.documents

let params = make_params(script_uri, 1, 0);
Expand Down Expand Up @@ -755,9 +749,7 @@ mod parked_salsa_tests {
let script_uri = lsp_types::Url::from_file_path(dir.path().join("script.R")).unwrap();

let mut state = WorldState::default();
state
.documents
.insert(script_uri.clone(), script_doc.clone());
state.insert_document(script_uri.clone(), script_doc.clone());

// Should resolve without hanging. Both symbols are reachable
// because a.R is visited first (gets its exports + b.R's exports),
Expand Down Expand Up @@ -801,9 +793,7 @@ mod parked_salsa_tests {
let script_uri = lsp_types::Url::from_file_path(dir.path().join("script.R")).unwrap();

let mut state = WorldState::default();
state
.documents
.insert(script_uri.clone(), script_doc.clone());
state.insert_document(script_uri.clone(), script_doc.clone());

let helpers_uri = lsp_types::Url::from_file_path(dir.path().join("helpers.R")).unwrap();

Expand Down Expand Up @@ -848,9 +838,7 @@ mod parked_salsa_tests {
let script_uri = lsp_types::Url::from_file_path(dir.path().join("script.R")).unwrap();

let mut state = WorldState::default();
state
.documents
.insert(script_uri.clone(), script_doc.clone());
state.insert_document(script_uri.clone(), script_doc.clone());

// `foo` on line 2 should resolve to the LOCAL definition on line 1,
// not to the sourced one from helpers.R.
Expand Down Expand Up @@ -897,9 +885,7 @@ mod parked_salsa_tests {
let script_uri = lsp_types::Url::from_file_path(dir.path().join("script.R")).unwrap();

let mut state = WorldState::default();
state
.documents
.insert(script_uri.clone(), script_doc.clone());
state.insert_document(script_uri.clone(), script_doc.clone());

let helpers_uri = lsp_types::Url::from_file_path(dir.path().join("helpers.R")).unwrap();
let a_uri = lsp_types::Url::from_file_path(dir.path().join("a.R")).unwrap();
Expand Down Expand Up @@ -947,9 +933,7 @@ mod parked_salsa_tests {
let script_doc = Document::new("source(\"script.R\")\nmy_var <- 1\nmy_var\n", None);

let mut state = WorldState::default();
state
.documents
.insert(script_uri.clone(), script_doc.clone());
state.insert_document(script_uri.clone(), script_doc.clone());

// `my_var` (line 2) should resolve to its own definition on line 1,
// not to a Sourced duplicate.
Expand Down Expand Up @@ -993,9 +977,7 @@ mod parked_salsa_tests {
let script_uri = lsp_types::Url::from_file_path(dir.path().join("script.R")).unwrap();

let mut state = WorldState::default();
state
.documents
.insert(script_uri.clone(), script_doc.clone());
state.insert_document(script_uri.clone(), script_doc.clone());

let (index, file_scope) = state.file_analysis(&script_uri, &script_doc);

Expand Down Expand Up @@ -1033,9 +1015,7 @@ mod parked_salsa_tests {
let script_uri = lsp_types::Url::from_file_path(dir.path().join("script.R")).unwrap();

let mut state = WorldState::default();
state
.documents
.insert(script_uri.clone(), script_doc.clone());
state.insert_document(script_uri.clone(), script_doc.clone());

let b_uri = lsp_types::Url::from_file_path(dir.path().join("b.R")).unwrap();

Expand Down Expand Up @@ -1063,9 +1043,7 @@ mod parked_salsa_tests {
let script_uri = lsp_types::Url::from_file_path(dir.path().join("script.R")).unwrap();

let mut state = WorldState::default();
state
.documents
.insert(script_uri.clone(), script_doc.clone());
state.insert_document(script_uri.clone(), script_doc.clone());

let helpers_uri = lsp_types::Url::from_file_path(dir.path().join("helpers.R")).unwrap();

Expand Down Expand Up @@ -1103,9 +1081,7 @@ mod parked_salsa_tests {
let script_uri = lsp_types::Url::from_file_path(dir.path().join("script.R")).unwrap();

let mut state = WorldState::default();
state
.documents
.insert(script_uri.clone(), script_doc.clone());
state.insert_document(script_uri.clone(), script_doc.clone());

let helpers_uri = lsp_types::Url::from_file_path(dir.path().join("helpers.R")).unwrap();

Expand Down Expand Up @@ -1138,9 +1114,7 @@ mod parked_salsa_tests {
let helpers_uri = lsp_types::Url::from_file_path(dir.path().join("helpers.R")).unwrap();

let mut state = WorldState::default();
state
.documents
.insert(script_uri.clone(), script_doc.clone());
state.insert_document(script_uri.clone(), script_doc.clone());

// `helper` on line 2 should resolve to helpers.R (source() shadows local)
let params = make_params(script_uri, 2, 0);
Expand Down Expand Up @@ -1177,9 +1151,7 @@ mod parked_salsa_tests {
let helpers_uri = lsp_types::Url::from_file_path(dir.path().join("helpers.R")).unwrap();

let mut state = WorldState::default();
state
.documents
.insert(script_uri.clone(), script_doc.clone());
state.insert_document(script_uri.clone(), script_doc.clone());

// `fn` on line 1 should resolve to the SECOND def in helpers.R (line 1)
let params = make_params(script_uri, 1, 0);
Expand Down
15 changes: 5 additions & 10 deletions crates/ark/src/lsp/main_loop.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -455,12 +455,7 @@ impl GlobalState {
// time: a buffer may have opened or closed since the scan
// kicked off. The buffer-drain inside `apply_scan_completed` uses
// this set as its watcher-event `skip` argument.
let editor_owned: HashSet<FilePath> = self.world
.documents
.keys()
.map(FilePath::from_url)
.collect();

let editor_owned: HashSet<FilePath> = self.world.documents.keys().cloned().collect();
let followups = self.lsp_state.oak_scheduler.apply_scan_completed(
&mut self.world.db,
scan,
Expand Down Expand Up @@ -1000,7 +995,7 @@ async fn process_diagnostics_batch(batch: Vec<RefreshDiagnosticsTask>) {
futures.push(task::spawn_blocking(move || {
let _span = tracing::info_span!("diagnostics_refresh", uri = %uri).entered();

if let Some(document) = state.documents.get(&uri) {
if let Some(document) = state.documents.get(&FilePath::from_url(&uri)) {
// Special case testthat-specific behaviour. This is a simple
// stopgap approach that has some false positives (e.g. when we
// work on testthat itself the flag will always be true), but
Expand Down Expand Up @@ -1136,8 +1131,8 @@ pub(crate) fn diagnostics_refresh_all(state: WorldState) {
n = state.documents.len()
);

for (uri, _document) in state.documents.iter() {
if !ExtUrl::should_diagnose(uri) {
for document in state.documents.values() {
if !ExtUrl::should_diagnose(&document.url) {
continue;
}

Expand All @@ -1147,7 +1142,7 @@ pub(crate) fn diagnostics_refresh_all(state: WorldState) {
// non-oak state).
INDEXER_QUEUE
.send(IndexerQueueTask::Diagnostics(RefreshDiagnosticsTask {
uri: uri.clone(),
uri: document.url.clone(),
state: state.legacy_snapshot(),
}))
.unwrap_or_else(|err| lsp::log_error!("Failed to queue diagnostics refresh: {err}"));
Expand Down
29 changes: 23 additions & 6 deletions crates/ark/src/lsp/state.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use std::collections::HashMap;
use std::path::Path;

use aether_path::FilePath;
use anyhow::anyhow;
use oak_db::OakDatabase;
use oak_semantic::library::Library;
Expand All @@ -18,8 +19,10 @@ pub(crate) struct WorldState {
/// Salsa input tree for Oak queries.
pub(crate) db: OakDatabase,

/// Watched documents
pub(crate) documents: HashMap<Url, Document>,
/// Watched documents, keyed on the normalised [`FilePath`] form.
/// The verbatim editor URL is preserved on each [`Document::url`]
/// for wire output.
pub(crate) documents: HashMap<FilePath, Document>,
Comment on lines +22 to +25

@DavisVaughan DavisVaughan Jun 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to advocate for the following strategy

pub(crate) documents: HashMap<FilePath, (Url, Document)>,
  • Document no longer knows about url. I think this is correct, as you should be able to create a Document without a corresponding url.
  • Document no longer has that weird PLACEHOLDER_URL hack, which i really do not like
  • insert_document() would be removed since we only .insert() in 1 place in production code, and then in a few tests, so the abstraction doesn't feel worth it

WorldState::get_document() and WorldState::get_document_mut() would then:

  • Take a path: FilePath as I mentioned elsewhere
  • Abstract over the (Url, Document) enum and just pull out the Document and return it

We would need to go update 2 direct usages of state.document.get() to be state.get_document(), but I think we should do that anyways.

This feels much much better to me, and I looked at all call sites of state.documents. and think it would work quite well

I am quite strongly against PLACEHOLDER_URL, so am going to block this pr until that is resolved

@lionel- lionel- Jun 11, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops sorry, my fault. I asked Claude to get rid of Option and didn't closely review that one. The placeholder would leak to the user if we made a mistake somewhere.

I did consider hoisting url out of there but it conflicted with existing code IIRC, and I wanted to avoid making too many changes to legacy Ark handlers. Let me take another look.

@lionel- lionel- Jun 11, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh wait I forgot it's already gone in an ulterior PR!


/// Watched folders
pub(crate) workspace: Workspace,
Expand Down Expand Up @@ -79,15 +82,17 @@ impl WorldState {
}

pub(crate) fn get_document(&self, uri: &Url) -> anyhow::Result<&Document> {
if let Some(doc) = self.documents.get(uri) {
let key = FilePath::from_url(uri);
if let Some(doc) = self.documents.get(&key) {
Ok(doc)
} else {
Err(anyhow!("Can't find document for URI {uri}"))
}
}

pub(crate) fn get_document_mut(&mut self, uri: &Url) -> anyhow::Result<&mut Document> {
Comment on lines 84 to 93

@DavisVaughan DavisVaughan Jun 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like these should take path: &FilePath. It would encourage conversion to FilePath at the LSP boundary

if let Some(doc) = self.documents.get_mut(uri) {
let key = FilePath::from_url(uri);
if let Some(doc) = self.documents.get_mut(&key) {
Ok(doc)
} else {
Err(anyhow!("Can't find document for URI {uri}"))
Expand Down Expand Up @@ -115,6 +120,15 @@ impl WorldState {
..self.clone()
}
}

/// Insert a document, keying on the normalised [`FilePath`] and
/// stashing the verbatim editor URL on [`Document::url`] for wire
/// output.
pub(crate) fn insert_document(&mut self, uri: Url, mut doc: Document) {
let key = FilePath::from_url(&uri);
doc.url = uri;
self.documents.insert(key, doc);
}
}

pub(crate) fn with_document<T, F>(
Expand Down Expand Up @@ -151,8 +165,11 @@ where
}

pub(crate) fn workspace_uris(state: &WorldState) -> Vec<Url> {
let uris: Vec<Url> = state.documents.iter().map(|elt| elt.0.clone()).collect();
uris
state
.documents
.values()
.map(|doc| doc.url.clone())
.collect()
}

#[cfg(test)]
Expand Down
Loading
Loading