From 720f1c270be36aa69dd9e91a8b117d4ad23aa24b Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Mon, 1 Jun 2026 16:05:02 +0200 Subject: [PATCH 1/3] Use Salsa infrastructure for cross-file goto-definition --- Cargo.lock | 4 + crates/ark/src/lsp/config.rs | 20 +- crates/ark/src/lsp/goto_definition.rs | 1170 +----------- crates/ark/src/lsp/handlers.rs | 5 +- crates/ark/src/lsp/tests/goto_definition.rs | 172 +- crates/ark/src/lsp/tests/utils.rs | 12 + crates/oak_db/Cargo.toml | 1 + crates/oak_db/src/file.rs | 13 + crates/oak_db/src/file_resolve.rs | 103 +- crates/oak_db/src/imports.rs | 5 + crates/oak_db/src/tests/file.rs | 35 + crates/oak_db/src/tests/file_imports_at.rs | 27 + crates/oak_db/src/tests/file_resolve.rs | 18 +- crates/oak_db/src/tests/file_resolve_at.rs | 130 +- crates/oak_ide/Cargo.toml | 3 + crates/oak_ide/src/goto_definition.rs | 84 +- crates/oak_ide/src/lib.rs | 3 +- .../tests/integration/goto_definition.rs | 1685 +---------------- .../oak_ide/tests/integration/identifier.rs | 140 ++ crates/oak_ide/tests/integration/main.rs | 1 + 20 files changed, 635 insertions(+), 2996 deletions(-) create mode 100644 crates/oak_ide/tests/integration/identifier.rs diff --git a/Cargo.lock b/Cargo.lock index 5a948126d2..ae1681fe99 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2593,6 +2593,7 @@ dependencies = [ "aether_path", "air_r_parser", "air_r_syntax", + "biome_line_index", "biome_rowan", "compact_str", "log", @@ -2619,11 +2620,14 @@ dependencies = [ name = "oak_ide" version = "0.1.0" dependencies = [ + "aether_path", "air_r_parser", "air_r_syntax", "anyhow", "biome_rowan", "oak_core", + "oak_db", + "oak_scan", "oak_semantic", "url", ] diff --git a/crates/ark/src/lsp/config.rs b/crates/ark/src/lsp/config.rs index 82c1b267bc..dfba452be5 100644 --- a/crates/ark/src/lsp/config.rs +++ b/crates/ark/src/lsp/config.rs @@ -1,3 +1,5 @@ +use aether_lsp_utils::proto::PositionEncoding; +use biome_line_index::WideEncoding; use serde::Deserialize; use serde::Serialize; use serde_json::Value; @@ -81,11 +83,27 @@ pub static DOCUMENT_SETTINGS: &[Setting] = &[ ]; /// Configuration of the LSP -#[derive(Clone, Default, Debug)] +#[derive(Clone, Debug)] pub(crate) struct LspConfig { pub(crate) diagnostics: DiagnosticsConfig, pub(crate) symbols: SymbolsConfig, pub(crate) workspace_symbols: WorkspaceSymbolsConfig, + + /// Session-wide position encoding for offset <-> LSP-position conversion. + /// One value for the whole session, not per document. Hard-coded to UTF-16, + /// the encoding we advertise at `initialize`. + pub(crate) position_encoding: PositionEncoding, +} + +impl Default for LspConfig { + fn default() -> Self { + Self { + diagnostics: DiagnosticsConfig::default(), + symbols: SymbolsConfig::default(), + workspace_symbols: WorkspaceSymbolsConfig::default(), + position_encoding: PositionEncoding::Wide(WideEncoding::Utf16), + } + } } #[derive(Serialize, Deserialize, Clone, Debug, Default)] diff --git a/crates/ark/src/lsp/goto_definition.rs b/crates/ark/src/lsp/goto_definition.rs index e13fda81e0..4d1bbb34d1 100644 --- a/crates/ark/src/lsp/goto_definition.rs +++ b/crates/ark/src/lsp/goto_definition.rs @@ -1,1172 +1,62 @@ use aether_lsp_utils::proto::from_proto; use aether_lsp_utils::proto::to_proto; +use aether_lsp_utils::proto::PositionEncoding; +use aether_path::FilePath; +use oak_db::Db; use oak_ide::NavigationTarget; -use stdext::result::ResultExt; use tower_lsp::lsp_types::GotoDefinitionParams; use tower_lsp::lsp_types::GotoDefinitionResponse; use tower_lsp::lsp_types::LocationLink; -use tower_lsp::lsp_types::Position; -use url::Url; -use crate::lsp::document::Document; -use crate::lsp::indexer; -use crate::lsp::traits::node::NodeExt; -use crate::treesitter::NodeTypeExt; +use crate::lsp::state::WorldState; pub(crate) fn goto_definition( - document: &Document, params: GotoDefinitionParams, + state: &WorldState, ) -> anyhow::Result> { - let uri = params.text_document_position_params.text_document.uri; + let uri = ¶ms.text_document_position_params.text_document.uri; let position = params.text_document_position_params.position; - let offset = from_proto::offset_from_position( - position, - &document.line_index, - document.position_encoding, - )?; + let db = &state.oak; + let encoding = state.config.position_encoding; - let index = document.semantic_index(); - let root = document.syntax()?; - let pos = oak_ide::FilePosition { - file: uri.clone(), - offset, - }; - let targets = oak_ide::goto_definition(&index, &root, &pos); - - let links: Vec<_> = targets - .into_iter() - .filter_map(|target| nav_target_to_link(target, document).log_err()) - .collect(); - - if !links.is_empty() { - return Ok(Some(GotoDefinitionResponse::Link(links))); - } - - // Within-file resolution found nothing. Fall back to the workspace - // indexer for a best-effort cross-file lookup of the identifier at the - // cursor. Non-identifier cursors (operators, parens, whitespace) return - // `None`. TODO(salsa): Replace the indexer step with a proper cross-file - // (file imports) lookup. - if let Some(link) = fallback_link_at(document, position, &uri)? { - return Ok(Some(GotoDefinitionResponse::Link(vec![link]))); - } - - Ok(None) -} - -/// Look up the identifier at `position` in the workspace indexer (current -/// file first, then other files). Returns `None` when the cursor isn't on -/// an identifier or the symbol isn't in the index. -fn fallback_link_at( - document: &Document, - position: Position, - uri: &Url, -) -> anyhow::Result> { - let point = document.tree_sitter_point_from_lsp_position(position)?; - let Some(node) = document.ast.root_node().find_closest_node_to_point(point) else { - log::warn!("Failed to find the closest node to point {point}."); + let Some(file) = db.file_by_url(&FilePath::from_url(uri)) else { return Ok(None); }; - if !node.is_identifier() { + let offset = from_proto::offset_from_position(position, file.line_index(db), encoding)?; + + let targets = oak_ide::goto_definition(db, file, offset); + if targets.is_empty() { return Ok(None); } - let symbol = node.node_as_str(&document.contents)?; - let Some((file_id, entry)) = - indexer::find_in_file(symbol, uri).or_else(|| indexer::find(symbol)) - else { - return Ok(None); - }; + // An ambiguous name (e.g. defined on both arms of an `if`/`else`) resolves + // to several bindings; the client offers all of them. + let links = targets + .iter() + .map(|target| nav_target_to_link(db, encoding, target)) + .collect::>>()?; - Ok(Some(LocationLink { - origin_selection_range: None, - target_uri: file_id.as_uri().clone(), - target_range: entry.range, - target_selection_range: entry.range, - })) + Ok(Some(GotoDefinitionResponse::Link(links))) } +/// Convert a [`NavigationTarget`] into a `LocationLink`. Its ranges are byte +/// 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( - target: NavigationTarget, - document: &Document, + db: &dyn Db, + encoding: PositionEncoding, + target: &NavigationTarget, ) -> anyhow::Result { - let target_range = to_proto::range( - target.full_range, - &document.line_index, - document.position_encoding, - )?; - let target_selection_range = to_proto::range( - target.focus_range, - &document.line_index, - document.position_encoding, - )?; + 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, + target_uri: target.file.url(db).to_url(), target_range, target_selection_range, }) } - -// Parked cross-file goto-definition tests pending the Salsa Oak wiring. -// -// Snapshot of the old `LegacyDb`-based suite, trimmed to the cross-file -// cases that the within-file handler doesn't cover (collation, `source()`, -// `library()`, NAMESPACE, package scope, base/search path). Intra-file -// tests live in `crates/ark/src/lsp/tests/goto_definition.rs`. -// `#[cfg(any())]` skips the block at compile time so the references to -// deleted types (`LegacyDb`, `ExternalScope`, `ScopeLayer`, ...) stay as -// the behavioural spec to re-implement on top of `oak_db::File::resolve_at`. -#[cfg(any())] -mod parked_salsa_tests { - use std::path::PathBuf; - use std::process::Command; - - use assert_matches::assert_matches; - use oak_package_metadata::description::Description; - use oak_package_metadata::namespace::Import; - use oak_package_metadata::namespace::Namespace; - use oak_semantic::library::Library; - use oak_semantic::package::Package; - use tower_lsp::lsp_types; - - use super::*; - use crate::lsp::document::Document; - use crate::lsp::inputs::source_root::SourceRoot; - use crate::lsp::util::test_path; - - fn r_library() -> Option { - let output = Command::new("R") - .args(["--no-save", "-e", "cat(.libPaths(), sep='\\n')"]) - .output() - .ok()?; - let stdout = String::from_utf8(output.stdout).ok()?; - let paths: Vec = stdout.lines().map(PathBuf::from).collect(); - if paths.is_empty() { - return None; - } - Some(Library::new(paths, None)) - } - - fn make_params(uri: lsp_types::Url, line: u32, character: u32) -> GotoDefinitionParams { - GotoDefinitionParams { - text_document_position_params: lsp_types::TextDocumentPositionParams { - text_document: lsp_types::TextDocumentIdentifier { uri }, - position: lsp_types::Position::new(line, character), - }, - work_done_progress_params: Default::default(), - partial_result_params: Default::default(), - } - } - - fn make_state(uri: &lsp_types::Url, doc: &Document) -> WorldState { - let mut state = WorldState::default(); - state.insert_document(uri.clone(), doc.clone()); - state - } - - #[test] - fn test_package_import_from_resolves() { - // A package with `importFrom(dplyr, mutate)` should make `mutate` - // visible. But we don't have a file/range for package symbols yet, - // so the result is None. - let doc = Document::new("mutate\n", None); - let uri = test_path("R/my_file.R"); - - let ns = Namespace { - imports: vec![Import { - name: "mutate".to_string(), - package: "dplyr".to_string(), - }], - ..Default::default() - }; - let desc = Description { - name: "mypkg".to_string(), - ..Default::default() - }; - let pkg = Package::from_parts(PathBuf::from("/fake"), desc, ns); - - let mut state = make_state(&uri, &doc); - state.root = Some(SourceRoot::Package(pkg)); - - let params = make_params(uri, 0, 0); - let result = goto_definition(&doc, params, &state).unwrap(); - // Package symbols don't produce NavigationTargets yet - assert_eq!(result, None); - } - - #[test] - fn test_cross_file_via_collation() { - // Collation order: aaa.R, bbb.R, ccc.R - // bbb.R defines `helper`. ccc.R (later) can see it, - // aaa.R (earlier) cannot. - let pkg_root = std::env::temp_dir().join("test_pkg"); - let r_dir = pkg_root.join("R"); - std::fs::create_dir_all(&r_dir).unwrap(); - - std::fs::write(r_dir.join("aaa.R"), "helper\n").unwrap(); - std::fs::write(r_dir.join("bbb.R"), "helper <- function() 1\n").unwrap(); - std::fs::write(r_dir.join("ccc.R"), "helper\n").unwrap(); - - // aaa.R and ccc.R are "open" (in state.documents). - // bbb.R is only on disk, exercising the read-from-disk fallback. - let doc_aaa = Document::new("helper\n", None); - let uri_aaa = lsp_types::Url::from_file_path(r_dir.join("aaa.R")).unwrap(); - - let uri_bbb = lsp_types::Url::from_file_path(r_dir.join("bbb.R")).unwrap(); - - let doc_ccc = Document::new("helper\n", None); - let uri_ccc = lsp_types::Url::from_file_path(r_dir.join("ccc.R")).unwrap(); - - let ns = Namespace::default(); - let desc = Description { - name: "mypkg".to_string(), - ..Default::default() - }; - let pkg = Package::from_parts(pkg_root, desc, ns); - - let mut state = WorldState::default(); - 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) - let params = make_params(uri_ccc, 0, 0); - assert_matches!( - goto_definition(&doc_ccc, params, &state).unwrap(), - Some(GotoDefinitionResponse::Link(ref links)) => { - assert_eq!(links[0].target_uri, uri_bbb); - assert_eq!( - links[0].target_range, - lsp_types::Range { - start: lsp_types::Position::new(0, 0), - end: lsp_types::Position::new(0, 6), - } - ); - } - ); - - // aaa.R cannot see bbb.R's definition (earlier in collation) - let params = make_params(uri_aaa, 0, 0); - let result = goto_definition(&doc_aaa, params, &state).unwrap(); - assert_eq!(result, None); - } - - #[test] - fn test_cross_file_collate_field_reverses_order() { - // Same files, but DESCRIPTION has `Collate: ccc.R bbb.R aaa.R` - // which reverses the order. Now aaa.R is last, so it can see - // bbb.R's definition. ccc.R is first, so it cannot. - let pkg_root = std::env::temp_dir().join("test_pkg_collate"); - - let doc_aaa = Document::new("helper\n", None); - let uri_aaa = lsp_types::Url::from_file_path(pkg_root.join("R/aaa.R")).unwrap(); - - let doc_bbb = Document::new("helper <- function() 1\n", None); - let uri_bbb = lsp_types::Url::from_file_path(pkg_root.join("R/bbb.R")).unwrap(); - - let doc_ccc = Document::new("helper\n", None); - let uri_ccc = lsp_types::Url::from_file_path(pkg_root.join("R/ccc.R")).unwrap(); - - let mut dcf_fields = std::collections::HashMap::new(); - dcf_fields.insert("Collate".to_string(), "ccc.R bbb.R aaa.R".to_string()); - - let ns = Namespace::default(); - let desc = Description { - name: "mypkg".to_string(), - fields: oak_package_metadata::dcf::Dcf { fields: dcf_fields }, - ..Default::default() - }; - let pkg = Package::from_parts(pkg_root, desc, ns); - - let mut state = WorldState::default(); - 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 - let params = make_params(uri_aaa, 0, 0); - assert_matches!( - goto_definition(&doc_aaa, params, &state).unwrap(), - Some(GotoDefinitionResponse::Link(ref links)) => { - assert_eq!(links[0].target_uri, uri_bbb); - } - ); - - // ccc.R is now first in collation, so it cannot see bbb.R's definition - let params = make_params(uri_ccc, 0, 0); - let result = goto_definition(&doc_ccc, params, &state).unwrap(); - assert_eq!(result, None); - } - - #[test] - fn test_use_in_function_body_resolves_across_files() { - // aaa.R uses `helper` inside a function body. zzz.R defines - // `helper` at the top level. In alphabetical collation zzz.R - // comes after aaa.R, but the definition should still be visible - // because function bodies execute lazily — the full package - // namespace is populated before any function is called. - let pkg_root = std::env::temp_dir().join("test_pkg_lazy"); - let r_dir = pkg_root.join("R"); - std::fs::create_dir_all(&r_dir).unwrap(); - - std::fs::write(r_dir.join("aaa.R"), "f <- function() helper()\n").unwrap(); - std::fs::write(r_dir.join("zzz.R"), "helper <- function() 1\n").unwrap(); - - // aaa.R is "open" (in state.documents). - // zzz.R is only on disk, exercising the read-from-disk fallback. - let doc_aaa = Document::new("f <- function() helper()\n", None); - let uri_aaa = lsp_types::Url::from_file_path(r_dir.join("aaa.R")).unwrap(); - - let uri_zzz = lsp_types::Url::from_file_path(r_dir.join("zzz.R")).unwrap(); - - let ns = Namespace::default(); - let desc = Description { - name: "mypkg".to_string(), - ..Default::default() - }; - let pkg = Package::from_parts(pkg_root, desc, ns); - - let mut state = WorldState::default(); - 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) - let params = make_params(uri_aaa, 0, 16); - assert_matches!( - goto_definition(&doc_aaa, params, &state).unwrap(), - Some(GotoDefinitionResponse::Link(ref links)) => { - assert_eq!(links[0].target_uri, uri_zzz); - } - ); - } - - // --- Base R and search path --- - - #[test] - fn test_package_scope_includes_base() { - // `cat` is in the base INDEX, so it resolves through the real - // library. file_scope adds base at the bottom of the package chain. - let Some(library) = r_library() else { - eprintln!("skipping: R not found"); - return; - }; - - let pkg_root = std::env::temp_dir().join("test_pkg_base"); - - let doc = Document::new("cat(1)\n", None); - let uri = lsp_types::Url::from_file_path(pkg_root.join("R/foo.R")).unwrap(); - - let ns = Namespace::default(); - let desc = Description { - name: "mypkg".to_string(), - ..Default::default() - }; - let pkg = Package::from_parts(pkg_root, desc, ns); - - let mut state = make_state(&uri, &doc); - state.root = Some(SourceRoot::Package(pkg)); - state.library = library; - - // `cat` at file level — package symbol, no NavigationTarget yet - let params = make_params(uri, 0, 0); - let result = goto_definition(&doc, params, &state).unwrap(); - assert_eq!(result, None); - } - - #[test] - fn test_search_path_includes_defaults() { - // A script outside a package should see base symbols via the - // default search path built by file_scope. - let Some(library) = r_library() else { - eprintln!("skipping: R not found"); - return; - }; - - let doc = Document::new("cat(1)\n", None); - let uri = test_path("script.R"); - - let mut state = make_state(&uri, &doc); - state.library = library; - - let params = make_params(uri, 0, 0); - let result = goto_definition(&doc, params, &state).unwrap(); - // Package symbol, no NavigationTarget yet - assert_eq!(result, None); - } - - #[test] - fn test_search_path_same_for_top_level_and_function() { - // Unlike packages, scripts use the same scope chain everywhere. - let Some(library) = r_library() else { - eprintln!("skipping: R not found"); - return; - }; - - let code = "f <- function() cat(1)\ncat(2)\n"; - let doc = Document::new(code, None); - let uri = test_path("script.R"); - - let mut state = make_state(&uri, &doc); - state.library = library; - - // `cat` at top level (line 1, col 0) - let params = make_params(uri.clone(), 1, 0); - let top_result = goto_definition(&doc, params, &state).unwrap(); - - // `cat` inside function body (line 0, col 16) - let params = make_params(uri, 0, 16); - let fn_result = goto_definition(&doc, params, &state).unwrap(); - - // Both resolve the same way (both None since package symbols - // don't produce NavigationTargets yet) - assert_eq!(top_result, None); - assert_eq!(fn_result, None); - } - - #[test] - fn test_fixme_base_function_missing_from_index() { - // `is.null` is a base R function but it's not in the base INDEX - // file (it's documented under the `NULL` help page). Since base - // has no NAMESPACE, we rely on INDEX for exported symbols, which - // misses many common functions. - let Some(library) = r_library() else { - eprintln!("skipping: R not found"); - return; - }; - - let pkg_root = std::env::temp_dir().join("test_pkg_base_fixme"); - - let ns = Namespace::default(); - let desc = Description { - name: "mypkg".to_string(), - ..Default::default() - }; - let pkg = Package::from_parts(pkg_root.clone(), desc, ns); - - // `cat` IS in the INDEX — verify it resolves (no NavigationTarget - // for package symbols, but the scope chain finds it) - let doc_cat = Document::new("cat(1)\n", None); - let uri_cat = lsp_types::Url::from_file_path(pkg_root.join("R/foo.R")).unwrap(); - - let mut state = make_state(&uri_cat, &doc_cat); - state.root = Some(SourceRoot::Package(pkg)); - state.library = library; - - let params = make_params(uri_cat, 0, 0); - let result = goto_definition(&doc_cat, params, &state).unwrap(); - assert_eq!(result, None); // package symbol, no NavigationTarget yet - - // `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.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(); - // FIXME: should resolve to base::is.null but doesn't because - // `is.null` is missing from the INDEX-based export list. - assert_eq!(result, None); - } - - // --- Script library() directives --- - - #[test] - fn test_script_library_directive_resolves() { - let Some(library) = r_library() else { - eprintln!("skipping: R not found"); - return; - }; - if library.get("rlang").is_none() { - eprintln!("skipping: rlang not installed"); - return; - } - - // `inform` before the `library()` call should not resolve, - // `inform` after should (to a package symbol, no NavigationTarget yet). - let code = "inform('hi')\nlibrary(rlang)\ninform('hello')\n"; - let doc = Document::new(code, None); - let uri = test_path("script.R"); - - let mut state = make_state(&uri, &doc); - state.library = library; - - let params = make_params(uri.clone(), 0, 0); - let result = goto_definition(&doc, params, &state).unwrap(); - assert_eq!(result, None); - - let params = make_params(uri, 2, 0); - let result = goto_definition(&doc, params, &state).unwrap(); - assert_eq!(result, None); - } - - // --- source() directive in scripts --- - - #[test] - fn test_script_source_resolves_from_workspace_root() { - // source() paths are resolved relative to the workspace root, - // not the sourcing file's directory. Here script.R is in a - // subdirectory but sources "helpers.R" which lives at the root. - let dir = tempfile::tempdir().unwrap(); - let subdir = dir.path().join("subdir"); - std::fs::create_dir(&subdir).unwrap(); - - std::fs::write(dir.path().join("helpers.R"), "helper <- function() 1\n").unwrap(); - - let script_doc = Document::new("source(\"helpers.R\")\nhelper\n", None); - let script_uri = lsp_types::Url::from_file_path(subdir.join("script.R")).unwrap(); - - let helpers_uri = lsp_types::Url::from_file_path(dir.path().join("helpers.R")).unwrap(); - - let mut state = WorldState::default(); - 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); - assert_matches!( - goto_definition(&script_doc, params, &state).unwrap(), - Some(GotoDefinitionResponse::Link(ref links)) => { - assert_eq!(links[0].target_uri, helpers_uri); - } - ); - } - - #[test] - fn test_script_source_directive_resolves() { - // script.R has `source("helpers.R")` then uses `helper`. - // WorldState::file_analysis() should resolve the source() directive - // and make helpers.R's exports visible via the search path. - let script_dir = std::env::temp_dir().join("test_script_source"); - - let helpers_doc = Document::new("helper <- function() 1\n", None); - let helpers_uri = lsp_types::Url::from_file_path(script_dir.join("helpers.R")).unwrap(); - - let script_doc = Document::new("source(\"helpers.R\")\nhelper\n", None); - let script_uri = lsp_types::Url::from_file_path(script_dir.join("script.R")).unwrap(); - - let mut state = WorldState::default(); - 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); - assert_matches!( - goto_definition(&script_doc, params, &state).unwrap(), - Some(GotoDefinitionResponse::Link(ref links)) => { - assert_eq!(links[0].target_uri, helpers_uri); - assert_eq!( - links[0].target_range, - lsp_types::Range { - start: lsp_types::Position::new(0, 0), - end: lsp_types::Position::new(0, 6), - } - ); - } - ); - } - - #[test] - fn test_script_source_directive_resolves_nested_library() { - // helpers.R has `library(dplyr)` and defines `helper`. - // script.R sources helpers.R then uses `mutate` (from dplyr). - // The nested library() directive should be visible through - // the source() resolution. - let Some(library) = r_library() else { - eprintln!("skipping: R not found"); - return; - }; - - let script_dir = std::env::temp_dir().join("test_script_source_nested"); - - let helpers_doc = Document::new("library(dplyr)\nhelper <- function() 1\n", None); - let helpers_uri = lsp_types::Url::from_file_path(script_dir.join("helpers.R")).unwrap(); - - let script_doc = Document::new("source(\"helpers.R\")\nmutate\nhelper\n", None); - let script_uri = lsp_types::Url::from_file_path(script_dir.join("script.R")).unwrap(); - - let mut state = make_state(&script_uri, &script_doc); - state.library = library; - 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. - let params = make_params(script_uri.clone(), 1, 0); - let result = goto_definition(&script_doc, params, &state).unwrap(); - assert_eq!(result, None); - - // `helper` (line 2) resolves to helpers.R's definition - let params = make_params(script_uri, 2, 0); - assert_matches!( - goto_definition(&script_doc, params, &state).unwrap(), - Some(GotoDefinitionResponse::Link(ref links)) => { - assert_eq!(links[0].target_uri, helpers_uri); - assert_eq!( - links[0].target_range, - lsp_types::Range { - start: lsp_types::Position::new(1, 0), - end: lsp_types::Position::new(1, 6), - } - ); - } - ); - } - - #[test] - fn test_script_source_resolves_from_disk() { - // helpers.R exists on disk but is NOT in state.documents. - // script.R sources it. The resolver should read from disk. - let script_dir = tempfile::tempdir().unwrap(); - - let helpers_path = script_dir.path().join("helpers.R"); - std::fs::write(&helpers_path, "helper <- function() 1\n").unwrap(); - let helpers_uri = lsp_types::Url::from_file_path(&helpers_path).unwrap(); - - let script_doc = Document::new("source(\"helpers.R\")\nhelper\n", None); - let script_uri = - lsp_types::Url::from_file_path(script_dir.path().join("script.R")).unwrap(); - - let mut state = WorldState::default(); - 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); - assert_matches!( - goto_definition(&script_doc, params, &state).unwrap(), - Some(GotoDefinitionResponse::Link(ref links)) => { - assert_eq!(links[0].target_uri, helpers_uri); - assert_eq!( - links[0].target_range, - lsp_types::Range { - start: lsp_types::Position::new(0, 0), - end: lsp_types::Position::new(0, 6), - } - ); - } - ); - } - - #[test] - fn test_script_file_scope_from_disk() { - // The script itself is on disk, not in state.documents. - // file_scope should still read it and resolve its directives. - let script_dir = tempfile::tempdir().unwrap(); - - let helpers_path = script_dir.path().join("helpers.R"); - std::fs::write(&helpers_path, "helper <- function() 1\n").unwrap(); - - let script_path = script_dir.path().join("script.R"); - std::fs::write(&script_path, "source(\"helpers.R\")\nhelper\n").unwrap(); - let script_uri = lsp_types::Url::from_file_path(&script_path).unwrap(); - - let script_doc = Document::new("source(\"helpers.R\")\nhelper\n", None); - - // Neither file is in state.documents - let state = WorldState::default(); - - let params = make_params(script_uri, 1, 0); - assert_matches!( - goto_definition(&script_doc, params, &state).unwrap(), - Some(GotoDefinitionResponse::Link(ref links)) => { - assert_eq!( - links[0].target_range, - lsp_types::Range { - start: lsp_types::Position::new(0, 0), - end: lsp_types::Position::new(0, 6), - } - ); - } - ); - } - - #[test] - fn test_script_source_transitive() { - // script.R sources a.R, a.R sources b.R. - // script.R should see b.R's exports transitively. - let dir = tempfile::tempdir().unwrap(); - - std::fs::write( - dir.path().join("b.R"), - "library(dplyr)\nfrom_b <- function() 1\n", - ) - .unwrap(); - std::fs::write( - dir.path().join("a.R"), - "source(\"b.R\")\nfrom_a <- function() 2\n", - ) - .unwrap(); - - let script_doc = Document::new("source(\"a.R\")\nfrom_a\nfrom_b\nmutate\n", None); - let script_uri = lsp_types::Url::from_file_path(dir.path().join("script.R")).unwrap(); - - let Some(library) = r_library() else { - eprintln!("skipping: R not found"); - return; - }; - - let mut state = make_state(&script_uri, &script_doc); - state.library = library; - - // `from_a` (line 1) — defined in a.R - let a_uri = lsp_types::Url::from_file_path(dir.path().join("a.R")).unwrap(); - let params = make_params(script_uri.clone(), 1, 0); - assert_matches!( - goto_definition(&script_doc, params, &state).unwrap(), - Some(GotoDefinitionResponse::Link(ref links)) => { - assert_eq!(links[0].target_uri, a_uri); - } - ); - - // `from_b` (line 2) — defined in b.R, reachable transitively - let b_uri = lsp_types::Url::from_file_path(dir.path().join("b.R")).unwrap(); - let params = make_params(script_uri.clone(), 2, 0); - assert_matches!( - goto_definition(&script_doc, params, &state).unwrap(), - Some(GotoDefinitionResponse::Link(ref links)) => { - assert_eq!(links[0].target_uri, b_uri); - } - ); - - // `mutate` (line 3) — from dplyr, attached by b.R's library() call. - // Package symbol, no NavigationTarget. - let params = make_params(script_uri, 3, 0); - let result = goto_definition(&script_doc, params, &state).unwrap(); - assert_eq!(result, None); - } - - #[test] - fn test_script_source_cycle_does_not_hang() { - // a.R sources b.R, b.R sources a.R. Should not recurse infinitely. - let dir = tempfile::tempdir().unwrap(); - - std::fs::write(dir.path().join("a.R"), "source(\"b.R\")\nfrom_a <- 1\n").unwrap(); - std::fs::write(dir.path().join("b.R"), "source(\"a.R\")\nfrom_b <- 2\n").unwrap(); - - let script_doc = Document::new("source(\"a.R\")\nfrom_a\nfrom_b\n", None); - let script_uri = lsp_types::Url::from_file_path(dir.path().join("script.R")).unwrap(); - - let mut state = WorldState::default(); - 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), - // and b.R's attempt to re-source a.R is a no-op due to cycle detection. - let a_uri = lsp_types::Url::from_file_path(dir.path().join("a.R")).unwrap(); - let params = make_params(script_uri.clone(), 1, 0); - assert_matches!( - goto_definition(&script_doc, params, &state).unwrap(), - Some(GotoDefinitionResponse::Link(ref links)) => { - assert_eq!(links[0].target_uri, a_uri); - } - ); - - let b_uri = lsp_types::Url::from_file_path(dir.path().join("b.R")).unwrap(); - let params = make_params(script_uri, 2, 0); - assert_matches!( - goto_definition(&script_doc, params, &state).unwrap(), - Some(GotoDefinitionResponse::Link(ref links)) => { - assert_eq!(links[0].target_uri, b_uri); - } - ); - } - - #[test] - fn test_script_source_in_function_scoping() { - // `source(local = FALSE)` inside a function scopes directives to the - // function scope, so definitions are NOT visible at file scope. - let dir = tempfile::tempdir().unwrap(); - - std::fs::write(dir.path().join("helpers.R"), "helper <- function() 1\n").unwrap(); - - // Line 0: "f <- function() {\n" - // Line 1: " source(\"helpers.R\")\n" - // Line 2: " helper\n" <- inside f, after source() - // Line 3: " function() helper\n" <- nested scope inside f - // Line 4: "}\n" - // Line 5: "helper\n" <- outside f - let script_source = - "f <- function() {\n source(\"helpers.R\")\n helper\n function() helper\n}\nhelper\n"; - let script_doc = Document::new(script_source, None); - let script_uri = lsp_types::Url::from_file_path(dir.path().join("script.R")).unwrap(); - - let mut state = WorldState::default(); - state.insert_document(script_uri.clone(), script_doc.clone()); - - let helpers_uri = lsp_types::Url::from_file_path(dir.path().join("helpers.R")).unwrap(); - - // `helper` on line 2 (inside f, after source()) — should resolve - let params = make_params(script_uri.clone(), 2, 2); - assert_matches!( - goto_definition(&script_doc, params, &state).unwrap(), - Some(GotoDefinitionResponse::Link(ref links)) => { - assert_eq!(links[0].target_uri, helpers_uri); - } - ); - - // `helper` on line 3 (nested function inside f) — should resolve - let params = make_params(script_uri.clone(), 3, 14); - assert_matches!( - goto_definition(&script_doc, params, &state).unwrap(), - Some(GotoDefinitionResponse::Link(ref links)) => { - assert_eq!(links[0].target_uri, helpers_uri); - } - ); - - // `helper` on line 5 (outside f) — NOT visible - let params = make_params(script_uri, 5, 0); - let result = goto_definition(&script_doc, params, &state).unwrap(); - assert_eq!(result, None); - } - - #[test] - fn test_script_source_does_not_shadow_local_def() { - // A local definition should take precedence over a sourced one. - // `source()` at file scope defines `foo`, but a subsequent local - // `foo <- "local"` should shadow it. - let dir = tempfile::tempdir().unwrap(); - - std::fs::write(dir.path().join("helpers.R"), "foo <- function() 1\n").unwrap(); - - // Line 0: "source(\"helpers.R\")\n" - // Line 1: "foo <- \"local\"\n" - // Line 2: "foo\n" - let script_source = "source(\"helpers.R\")\nfoo <- \"local\"\nfoo\n"; - let script_doc = Document::new(script_source, None); - let script_uri = lsp_types::Url::from_file_path(dir.path().join("script.R")).unwrap(); - - let mut state = WorldState::default(); - 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. - let params = make_params(script_uri.clone(), 2, 0); - assert_matches!( - goto_definition(&script_doc, params, &state).unwrap(), - Some(GotoDefinitionResponse::Link(ref links)) => { - assert_eq!(links[0].target_uri, script_uri); - assert_eq!( - links[0].target_range, - lsp_types::Range { - start: lsp_types::Position::new(1, 0), - end: lsp_types::Position::new(1, 3), - } - ); - } - ); - } - - #[test] - fn test_script_source_diamond_dependency() { - // Diamond: a.R and b.R both source helpers.R. - // script.R sources both a.R and b.R. - // `helper` (from helpers.R) should be visible — the grey-set - // cycle detection must not prevent re-resolving a shared dependency. - let dir = tempfile::tempdir().unwrap(); - - std::fs::write(dir.path().join("helpers.R"), "helper <- function() 1\n").unwrap(); - std::fs::write( - dir.path().join("a.R"), - "source(\"helpers.R\")\nfrom_a <- 1\n", - ) - .unwrap(); - std::fs::write( - dir.path().join("b.R"), - "source(\"helpers.R\")\nfrom_b <- 2\n", - ) - .unwrap(); - - let script_doc = Document::new( - "source(\"a.R\")\nsource(\"b.R\")\nhelper\nfrom_a\nfrom_b\n", - None, - ); - let script_uri = lsp_types::Url::from_file_path(dir.path().join("script.R")).unwrap(); - - let mut state = WorldState::default(); - 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(); - let b_uri = lsp_types::Url::from_file_path(dir.path().join("b.R")).unwrap(); - - // `helper` (line 2) — from helpers.R, reachable through both a.R and b.R - let params = make_params(script_uri.clone(), 2, 0); - assert_matches!( - goto_definition(&script_doc, params, &state).unwrap(), - Some(GotoDefinitionResponse::Link(ref links)) => { - assert_eq!(links[0].target_uri, helpers_uri); - } - ); - - // `from_a` (line 3) — from a.R - let params = make_params(script_uri.clone(), 3, 0); - assert_matches!( - goto_definition(&script_doc, params, &state).unwrap(), - Some(GotoDefinitionResponse::Link(ref links)) => { - assert_eq!(links[0].target_uri, a_uri); - } - ); - - // `from_b` (line 4) — from b.R - let params = make_params(script_uri, 4, 0); - assert_matches!( - goto_definition(&script_doc, params, &state).unwrap(), - Some(GotoDefinitionResponse::Link(ref links)) => { - assert_eq!(links[0].target_uri, b_uri); - } - ); - } - - #[test] - fn test_script_source_self_reference() { - // script.R sources itself. The grey-set pre-seeds the current file - // so the self-reference is a no-op and doesn't create duplicate - // definitions. - let dir = tempfile::tempdir().unwrap(); - - let script_path = dir.path().join("script.R"); - std::fs::write(&script_path, "source(\"script.R\")\nmy_var <- 1\nmy_var\n").unwrap(); - let script_uri = lsp_types::Url::from_file_path(&script_path).unwrap(); - - let script_doc = Document::new("source(\"script.R\")\nmy_var <- 1\nmy_var\n", None); - - let mut state = WorldState::default(); - 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. - let params = make_params(script_uri.clone(), 2, 0); - assert_matches!( - goto_definition(&script_doc, params, &state).unwrap(), - Some(GotoDefinitionResponse::Link(ref links)) => { - assert_eq!(links[0].target_uri, script_uri); - assert_eq!( - links[0].target_range, - lsp_types::Range { - start: lsp_types::Position::new(1, 0), - end: lsp_types::Position::new(1, 6), - } - ); - } - ); - } - - #[test] - fn test_script_source_in_function_packages_scoped() { - // `source(local = FALSE)` inside a function scopes package directives - // to the function scope, so they are NOT visible at file scope. - let dir = tempfile::tempdir().unwrap(); - - std::fs::write( - dir.path().join("helpers.R"), - "library(dplyr)\nhelper <- function() 1\n", - ) - .unwrap(); - - // "mutate\n" offset 0 - // "f <- function() {\n" offset 7 - // " source(\"helpers.R\")\n" offset 25 - // " mutate\n" offset 46 - // "}\n" offset 55 - // "mutate\n" offset 57 - let script_source = - "mutate\nf <- function() {\n source(\"helpers.R\")\n mutate\n}\nmutate\n"; - let script_doc = Document::new(script_source, None); - let script_uri = lsp_types::Url::from_file_path(dir.path().join("script.R")).unwrap(); - - let mut state = WorldState::default(); - state.insert_document(script_uri.clone(), script_doc.clone()); - - let (index, file_scope) = state.file_analysis(&script_uri, &script_doc); - - let has_dplyr = |layers: &[oak_semantic::scope_layer::ScopeLayer]| -> bool { - layers.iter().any(|l| matches!(l, oak_semantic::scope_layer::ScopeLayer::PackageExports(pkg) if pkg == "dplyr")) - }; - - // Before f (offset 0, on "mutate"): dplyr is NOT visible because the - // directive's offset is the `source()` call site inside f. - let before_offset = biome_rowan::TextSize::from(0); - let before_chain = file_scope.at(&index, before_offset); - assert!(!has_dplyr(&before_chain)); - - // Inside f (offset 48, on "mutate"): dplyr should be in the scope chain - let inner_offset = biome_rowan::TextSize::from(48); - let inner_chain = file_scope.at(&index, inner_offset); - assert!(has_dplyr(&inner_chain)); - - // After f (offset 57, on "mutate"): dplyr is NOT visible - let outer_offset = biome_rowan::TextSize::from(57); - let outer_chain = file_scope.at(&index, outer_offset); - assert!(!has_dplyr(&outer_chain)); - } - - #[test] - fn test_script_source_later_shadows_earlier() { - // When two sourced files define the same name, the later - // source() call shadows the earlier one. - let dir = tempfile::tempdir().unwrap(); - - std::fs::write(dir.path().join("a.R"), "foo <- 1\n").unwrap(); - std::fs::write(dir.path().join("b.R"), "foo <- 2\n").unwrap(); - - let script_doc = Document::new("source(\"a.R\")\nsource(\"b.R\")\nfoo\n", None); - let script_uri = lsp_types::Url::from_file_path(dir.path().join("script.R")).unwrap(); - - let mut state = WorldState::default(); - state.insert_document(script_uri.clone(), script_doc.clone()); - - let b_uri = lsp_types::Url::from_file_path(dir.path().join("b.R")).unwrap(); - - // `foo` (line 2) should resolve to b.R (later source shadows earlier) - let params = make_params(script_uri, 2, 0); - assert_matches!( - goto_definition(&script_doc, params, &state).unwrap(), - Some(GotoDefinitionResponse::Link(ref links)) => { - assert_eq!(links[0].target_uri, b_uri); - } - ); - } - - #[test] - fn test_script_source_local_true_in_function_scoping() { - // `source(local = TRUE)` injects definitions into the function - // scope, so `helper` is visible inside f but not outside. - let dir = tempfile::tempdir().unwrap(); - - std::fs::write(dir.path().join("helpers.R"), "helper <- function() 1\n").unwrap(); - - let script_source = - "f <- function() {\n source(\"helpers.R\", local = TRUE)\n helper\n}\nhelper\n"; - let script_doc = Document::new(script_source, None); - let script_uri = lsp_types::Url::from_file_path(dir.path().join("script.R")).unwrap(); - - let mut state = WorldState::default(); - state.insert_document(script_uri.clone(), script_doc.clone()); - - let helpers_uri = lsp_types::Url::from_file_path(dir.path().join("helpers.R")).unwrap(); - - // `helper` on line 2 (inside f, after source(local = TRUE)) — resolves - let params = make_params(script_uri.clone(), 2, 2); - assert_matches!( - goto_definition(&script_doc, params, &state).unwrap(), - Some(GotoDefinitionResponse::Link(ref links)) => { - assert_eq!(links[0].target_uri, helpers_uri); - } - ); - - // `helper` on line 4 (outside f) — does NOT resolve - let params = make_params(script_uri, 4, 0); - let result = goto_definition(&script_doc, params, &state).unwrap(); - assert_eq!(result, None); - } - - #[test] - fn test_script_source_local_true_shadows_local_def() { - // `source(local = TRUE)` injects into the use-def map and - // shadows a prior local binding. - let dir = tempfile::tempdir().unwrap(); - - std::fs::write(dir.path().join("helpers.R"), "foo <- function() 1\n").unwrap(); - - // Line 0: "f <- function() {\n" - // Line 1: " foo <- \"local\"\n" - // Line 2: " source(\"helpers.R\", local = TRUE)\n" - // Line 3: " foo\n" - // Line 4: "}\n" - let script_source = - "f <- function() {\n foo <- \"local\"\n source(\"helpers.R\", local = TRUE)\n foo\n}\n"; - let script_doc = Document::new(script_source, None); - let script_uri = lsp_types::Url::from_file_path(dir.path().join("script.R")).unwrap(); - - let mut state = WorldState::default(); - state.insert_document(script_uri.clone(), script_doc.clone()); - - let helpers_uri = lsp_types::Url::from_file_path(dir.path().join("helpers.R")).unwrap(); - - // `foo` on line 3 resolves to helpers.R (sourced def shadows local) - let params = make_params(script_uri, 3, 2); - assert_matches!( - goto_definition(&script_doc, params, &state).unwrap(), - Some(GotoDefinitionResponse::Link(ref links)) => { - assert_eq!(links[0].target_uri, helpers_uri); - } - ); - } - - #[test] - fn test_script_source_shadows_local_def() { - // A local definition followed by source() of a file that also - // defines the same name: the use after source() should resolve - // to the sourced file's definition (later shadows earlier). - let dir = tempfile::tempdir().unwrap(); - - std::fs::write(dir.path().join("helpers.R"), "helper <- function() 1\n").unwrap(); - - // Line 0: "helper <- 'local'\n" - // Line 1: "source(\"helpers.R\")\n" - // Line 2: "helper\n" - let script_source = "helper <- 'local'\nsource(\"helpers.R\")\nhelper\n"; - let script_doc = Document::new(script_source, None); - let script_uri = lsp_types::Url::from_file_path(dir.path().join("script.R")).unwrap(); - - let helpers_uri = lsp_types::Url::from_file_path(dir.path().join("helpers.R")).unwrap(); - - let mut state = WorldState::default(); - 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); - assert_matches!( - goto_definition(&script_doc, params, &state).unwrap(), - Some(GotoDefinitionResponse::Link(ref links)) => { - assert_eq!(links[0].target_uri, helpers_uri); - assert_eq!( - links[0].target_range, - lsp_types::Range { - start: lsp_types::Position::new(0, 0), - end: lsp_types::Position::new(0, 6), - } - ); - } - ); - } - - #[test] - fn test_script_source_last_def_wins_in_target() { - // If the sourced file defines the same name twice, the LAST - // definition is the one that goto-definition navigates to. - let dir = tempfile::tempdir().unwrap(); - - // helpers.R defines `fn` twice — second def is at line 1 - std::fs::write( - dir.path().join("helpers.R"), - "fn <- function() 'first'\nfn <- function() 'second'\n", - ) - .unwrap(); - - let script_doc = Document::new("source(\"helpers.R\")\nfn\n", None); - let script_uri = lsp_types::Url::from_file_path(dir.path().join("script.R")).unwrap(); - let helpers_uri = lsp_types::Url::from_file_path(dir.path().join("helpers.R")).unwrap(); - - let mut state = WorldState::default(); - 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); - assert_matches!( - goto_definition(&script_doc, params, &state).unwrap(), - Some(GotoDefinitionResponse::Link(ref links)) => { - assert_eq!(links[0].target_uri, helpers_uri); - assert_eq!( - links[0].target_range, - lsp_types::Range { - start: lsp_types::Position::new(1, 0), - end: lsp_types::Position::new(1, 2), - } - ); - } - ); - } -} diff --git a/crates/ark/src/lsp/handlers.rs b/crates/ark/src/lsp/handlers.rs index ae0c44786d..a73e540a85 100644 --- a/crates/ark/src/lsp/handlers.rs +++ b/crates/ark/src/lsp/handlers.rs @@ -307,10 +307,7 @@ pub(crate) fn handle_goto_definition( params: GotoDefinitionParams, state: &WorldState, ) -> LspResult> { - let uri = ¶ms.text_document_position_params.text_document.uri; - let document = state.get_document(uri)?; - - Ok(goto_definition(document, params).log_err().flatten()) + Ok(goto_definition(params, state).log_err().flatten()) } #[tracing::instrument(level = "info", skip_all)] diff --git a/crates/ark/src/lsp/tests/goto_definition.rs b/crates/ark/src/lsp/tests/goto_definition.rs index 0fe97a83f2..ac93569582 100644 --- a/crates/ark/src/lsp/tests/goto_definition.rs +++ b/crates/ark/src/lsp/tests/goto_definition.rs @@ -2,10 +2,14 @@ use assert_matches::assert_matches; use tower_lsp::lsp_types; use tower_lsp::lsp_types::GotoDefinitionParams; use tower_lsp::lsp_types::GotoDefinitionResponse; +use url::Url; +use super::utils::insert_file; +use super::utils::make_state; +use super::utils::range; use crate::lsp::document::Document; use crate::lsp::goto_definition::goto_definition; -use crate::lsp::indexer; +use crate::lsp::state::WorldState; use crate::lsp::util::test_path; fn make_params(uri: lsp_types::Url, line: u32, character: u32) -> GotoDefinitionParams { @@ -19,133 +23,149 @@ fn make_params(uri: lsp_types::Url, line: u32, character: u32) -> GotoDefinition } } +/// A state with several open files, each mirrored into `oak` like `did_open` +/// does, so `source()` targets resolve through `file_by_url`. +fn make_state_with(files: &[(&Url, &str)]) -> WorldState { + let mut state = WorldState::default(); + for (uri, code) in files { + insert_file(&mut state, uri, &Document::new(code, None)); + } + state +} + #[test] fn test_goto_definition() { - let code = "foo <- 42\nprint(foo)\n"; - let doc = Document::new(code, None); let uri = test_path("test.R"); + let state = make_state(&uri, &Document::new("foo <- 42\nprint(foo)\n", None)); let params = make_params(uri, 1, 6); assert_matches!( - goto_definition(&doc, params).unwrap(), + goto_definition(params, &state).unwrap(), Some(GotoDefinitionResponse::Link(ref links)) => { - assert_eq!( - links[0].target_range, - lsp_types::Range { - start: lsp_types::Position::new(0, 0), - end: lsp_types::Position::new(0, 3), - } - ); + assert_eq!(links[0].target_range, range((0, 0), (0, 3))); } ); } #[test] fn test_goto_definition_prefers_local_symbol() { - let code = "foo <- 1\nfoo\n"; - let doc = Document::new(code, None); let uri = test_path("file.R"); + let state = make_state(&uri, &Document::new("foo <- 1\nfoo\n", None)); let params = make_params(uri.clone(), 1, 0); assert_matches!( - goto_definition(&doc, params).unwrap(), + goto_definition(params, &state).unwrap(), Some(GotoDefinitionResponse::Link(ref links)) => { assert_eq!(links[0].target_uri, uri); - assert_eq!( - links[0].target_range, - lsp_types::Range { - start: lsp_types::Position::new(0, 0), - end: lsp_types::Position::new(0, 3), - } - ); + assert_eq!(links[0].target_range, range((0, 0), (0, 3))); } ); } #[test] fn test_unbound_identifier_returns_none() { - // A free identifier with no local def and no indexer entry returns - // `None`, matching how rust-analyzer and ty handle the same case. - let _guard = indexer::ResetIndexerGuard; - - let doc = Document::new("foo\n", None); + // A free identifier with no reachable binding returns `None`, matching how + // rust-analyzer and ty handle the same case. let uri = test_path("file.R"); + let state = make_state(&uri, &Document::new("foo\n", None)); let params = make_params(uri, 0, 0); - assert_eq!(goto_definition(&doc, params).unwrap(), None); + assert_eq!(goto_definition(params, &state).unwrap(), None); } #[test] fn test_cursor_on_operator_returns_none() { - // Cursor on `<-` (a non-identifier token): even though the file has - // an identifier nearby, the operator itself doesn't resolve to - // anything, so the response is `None`. - let _guard = indexer::ResetIndexerGuard; - - let other = Document::new("foo <- function() 'other'\n", None); - let other_uri = test_path("other.R"); - indexer::update(&other, &other_uri).unwrap(); - - let doc = Document::new("foo <- 1\n", None); + // Cursor on `<-`, not on an identifier use: nothing to resolve. let uri = test_path("file.R"); + let state = make_state(&uri, &Document::new("foo <- 1\n", None)); // Cursor on the `<` of `<-` at column 4. let params = make_params(uri, 0, 4); - assert_eq!(goto_definition(&doc, params).unwrap(), None); + assert_eq!(goto_definition(params, &state).unwrap(), None); } #[test] -fn test_fallback_resolves_cross_file() { - // A free variable that the intra-file resolver can't bind falls back - // to the workspace indexer, which finds the definition in another - // indexed file. - let _guard = indexer::ResetIndexerGuard; - - let doc1 = Document::new("foo <- function() 1\n", None); - let uri1 = test_path("defs.R"); - indexer::update(&doc1, &uri1).unwrap(); - - let doc2 = Document::new("foo\n", None); - let uri2 = test_path("uses.R"); +fn test_unlinked_cross_file_returns_none() { + // `foo` is defined in another open file, but this file doesn't `source()` + // it, so R semantics can't reach it. goto-def is precise: it returns `None` + // rather than guessing by name across the workspace like the legacy ark handler. + let uses_uri = test_path("uses.R"); + let defs_uri = test_path("defs.R"); + let state = make_state_with(&[(&uses_uri, "foo\n"), (&defs_uri, "foo <- function() 1\n")]); + + let params = make_params(uses_uri, 0, 0); + assert_eq!(goto_definition(params, &state).unwrap(), None); +} - let params = make_params(uri2, 0, 0); +#[test] +fn test_resolves_across_source_directive() { + // `script.R` sources `helpers.R`; goto-def on the forwarded `helper` use + // lands in `helpers.R`. Exercises the cross-file branch of + // `definition_to_link` (the target file's own line index + URL). The + // resolution itself is covered exhaustively by `oak_db`'s `file_resolve_at` + // tests; this checks the goto-def wiring on top of it. + let script_uri = test_path("script.R"); + let helpers_uri = test_path("helpers.R"); + let state = make_state_with(&[ + (&script_uri, "source(\"helpers.R\")\nhelper\n"), + (&helpers_uri, "helper <- function() 1\n"), + ]); + + let params = make_params(script_uri, 1, 0); assert_matches!( - goto_definition(&doc2, params).unwrap(), + goto_definition(params, &state).unwrap(), Some(GotoDefinitionResponse::Link(ref links)) => { - assert_eq!(links.len(), 1); - assert_eq!(links[0].target_uri, uri1); + assert_eq!(links[0].target_uri, helpers_uri); + assert_eq!(links[0].target_range, range((0, 0), (0, 6))); } ); } #[test] -fn test_fallback_skipped_when_local_def_wins() { - // When the symbol has a binding within the file, the within-file - // result is returned and the indexer fallback isn't consulted. - let _guard = indexer::ResetIndexerGuard; - - let other = Document::new("foo <- function() 'other'\n", None); - let other_uri = test_path("other.R"); - indexer::update(&other, &other_uri).unwrap(); - - let code = "foo <- 1\nfoo\n"; - let doc = Document::new(code, None); - let uri = test_path("main.R"); +fn test_local_def_shadows_sourced() { + // A local `<-` after a `source()` shadows the sourced binding, so the use + // resolves to the local def (in this file), not the sourced one. The link + // range must point at the local def. + let script_uri = test_path("script.R"); + let helpers_uri = test_path("helpers.R"); + let state = make_state_with(&[ + (&script_uri, "source(\"helpers.R\")\nfoo <- 1\nfoo\n"), + (&helpers_uri, "foo <- function() 2\n"), + ]); + + let params = make_params(script_uri.clone(), 2, 0); + assert_matches!( + goto_definition(params, &state).unwrap(), + Some(GotoDefinitionResponse::Link(ref links)) => { + assert_eq!(links[0].target_uri, script_uri); + assert_eq!(links[0].target_range, range((1, 0), (1, 3))); + } + ); +} - let params = make_params(uri.clone(), 1, 0); +#[test] +fn test_last_def_in_sourced_file_wins() { + // When the sourced file binds the same name twice, the use navigates to + // the last definition. The link range must point at that second def, in + // the target file's coordinates. + let script_uri = test_path("script.R"); + let helpers_uri = test_path("helpers.R"); + let state = make_state_with(&[ + (&script_uri, "source(\"helpers.R\")\nfn\n"), + ( + &helpers_uri, + "fn <- function() 'first'\nfn <- function() 'second'\n", + ), + ]); + + let params = make_params(script_uri, 1, 0); assert_matches!( - goto_definition(&doc, params).unwrap(), + goto_definition(params, &state).unwrap(), Some(GotoDefinitionResponse::Link(ref links)) => { - assert_eq!(links[0].target_uri, uri); - assert_eq!( - links[0].target_range, - lsp_types::Range { - start: lsp_types::Position::new(0, 0), - end: lsp_types::Position::new(0, 3), - } - ); + assert_eq!(links[0].target_uri, helpers_uri); + assert_eq!(links[0].target_range, range((1, 0), (1, 2))); } ); } diff --git a/crates/ark/src/lsp/tests/utils.rs b/crates/ark/src/lsp/tests/utils.rs index 615188219e..aee5942cf3 100644 --- a/crates/ark/src/lsp/tests/utils.rs +++ b/crates/ark/src/lsp/tests/utils.rs @@ -1,3 +1,5 @@ +use aether_path::FilePath; +use oak_scan::DbExt; use tower_lsp::lsp_types; use crate::lsp::document::Document; @@ -5,8 +7,18 @@ use crate::lsp::state::WorldState; pub(super) fn make_state(uri: &lsp_types::Url, doc: &Document) -> WorldState { let mut state = WorldState::default(); + insert_file(&mut state, uri, doc); + state +} + +/// Insert a document and mirror its contents into `oak`, the same pair +/// `did_open` performs, so handlers reading either `state.documents` or +/// `state.oak` (via `file_by_url`) see a consistent file. +pub(super) fn insert_file(state: &mut WorldState, uri: &lsp_types::Url, doc: &Document) { state.insert_document(uri.clone(), doc.clone()); state + .oak + .upsert_editor(FilePath::from_url(uri), doc.contents.clone()); } pub(super) fn range(start: (u32, u32), end: (u32, u32)) -> lsp_types::Range { diff --git a/crates/oak_db/Cargo.toml b/crates/oak_db/Cargo.toml index a468b247b1..4cdd71a0c8 100644 --- a/crates/oak_db/Cargo.toml +++ b/crates/oak_db/Cargo.toml @@ -17,6 +17,7 @@ workspace = true aether_parser.workspace = true aether_syntax.workspace = true aether_path.workspace = true +biome_line_index.workspace = true biome_rowan.workspace = true compact_str.workspace = true log.workspace = true diff --git a/crates/oak_db/src/file.rs b/crates/oak_db/src/file.rs index c5424bfc6c..928916d02a 100644 --- a/crates/oak_db/src/file.rs +++ b/crates/oak_db/src/file.rs @@ -75,6 +75,19 @@ impl File { )) } + /// Line index for this file, mapping byte offsets to `(line, column)`. + /// + /// Computed straight from `contents`, so it doesn't depend on a syntax + /// tree. The LSP needs it for every offset <-> position translation. No + /// `lru`: it's small and almost every request needs it, so it stays + /// resident for the file's lifetime. Follows ty and rust-analyzer, which + /// both compute the line index as a query off the source text rather than + /// storing it on the file input. + #[salsa::tracked(returns(ref))] + pub fn line_index(self, db: &dyn Db) -> biome_line_index::LineIndex { + biome_line_index::LineIndex::new(self.contents(db)) + } + /// Build this file's `SemanticIndex` from the parse tree. /// /// This is a coarse query that invalidates downstream on every edit diff --git a/crates/oak_db/src/file_resolve.rs b/crates/oak_db/src/file_resolve.rs index 81c321349d..7550f6771d 100644 --- a/crates/oak_db/src/file_resolve.rs +++ b/crates/oak_db/src/file_resolve.rs @@ -1,7 +1,10 @@ use std::collections::HashSet; use std::rc::Rc; +use aether_path::FilePath; use biome_rowan::TextSize; +use oak_semantic::semantic_index::DefinitionKind; +use oak_semantic::DefinitionId; use oak_semantic::ScopeId; use crate::Db; @@ -75,72 +78,86 @@ impl<'db> File { None } - /// Resolve the name at `offset` to its definition. + /// Resolve the name at `offset` to its definition(s). /// - /// Implements R's full lookup chain at the use site, with offset-aware - /// sequential semantics for top-level code: - /// - /// 1. **Lexical scopes**: walk ancestor scopes from the use, returning - /// the first function-scope binding (parameter, local `<-`, etc.). - /// 2. **In a function body (lazy context)**: defer to `resolve`, which - /// uses the EOF state (full imports, source forwards). The function - /// will run after the whole script/package is loaded, so we - /// over-approximate accordingly. - /// 3. **At top-level (sequential context)**: walk the exports chain - /// (handles `<-` and `source()` shadowing via `exports`'s - /// last-wins ordering), then walk `imports_at(offset)` for the - /// visible subset of attaches / collation predecessors. + /// Returns every binding that could reach the use, so a name defined on + /// both arms of an `if`/`else` yields two. A cursor on a binding's own name + /// resolves to that binding. Empty means nothing reachable binds the name + /// here. /// /// Not `#[salsa::tracked]` because keying on `(self, offset)` would /// balloon the cache. The `Definition` entities it returns are minted by - /// the tracked [`File::definitions()`], so they stay cached and - /// identity-stable even though this lookup isn't. - pub fn resolve_at(self, db: &'db dyn Db, offset: TextSize) -> Option> { + /// the tracked [`File::definitions()`] and looked up via + /// [`File::definition()`], so they stay cached and identity-stable even + /// though this lookup isn't. + pub fn resolve_at(self, db: &'db dyn Db, offset: TextSize) -> Vec> { let index = self.semantic_index(db); - let (use_scope, _use_id, use_site) = index.use_at(offset)?; + let Some((use_scope, use_id, use_site)) = index.use_at(offset) else { + // Cursor on a binding's own name (a def site, not a use): jump to + // it, like rust-analyzer / ty. + return match index.definition_at(offset) { + Some((scope, def_id, _)) => { + self.definition(db, scope, def_id).into_iter().collect() + }, + None => Vec::new(), + }; + }; let name = index .symbols(use_scope) .symbol(use_site.symbol()) .name() .to_string(); - - // Step 1, lexical. Function-scope hits return directly. File-scope - // hits fall through. `resolve_symbol()` only tracks IS_BOUND, but - // `exports()` orders bindings in source order so steps 2/3 pick the - // last winner between `<-` and a same-name `source()`. - let file_scope = ScopeId::from(0); - if let Some((binding_scope, def_id, _def)) = index.resolve(&name, use_scope) { - if binding_scope != file_scope { - return self.definition(db, binding_scope, def_id); - } - } - - let in_function = use_scope != file_scope; let interned = Name::new(db, name.as_str()); - // 2. Function body: In lazy contexts we over-approximate by resolving - // as if cursor was at EOF. Defer to `resolve`. - if in_function { - return self.resolve(db, interned); + // Get local definitions for that use + let reaching: Vec<(ScopeId, DefinitionId)> = + index.reaching_definitions(use_scope, use_id).collect(); + + if !reaching.is_empty() { + return reaching + .into_iter() + .filter_map(|(scope, def_id)| self.resolve_definition(db, scope, def_id)) + .collect(); } - // 3. Top-level: exports chain (here) offset-narrowed imports (below). - if let Some(def) = self.resolve_export(db, interned) { - return Some(def); + // Nothing local reaches the use, so resolve across files. + let file_scope = ScopeId::from(0); + if use_scope != file_scope { + // Function body: the lazy / end-of-file view the body sees at run time. + return self.resolve(db, interned).into_iter().collect(); } - // Same exports-only chase as `resolve`'s imports walk: avoids the - // sibling cycle, matches R's namespace semantics. TODO: Package-level - // layers. + // Top level: collation predecessors / other visible files (exports-only + // chase, same as `resolve`'s imports walk). Avoids the sibling cycle and + // matches R's namespace semantics. TODO: Package-level layers. for layer in self.imports_at(db, offset) { if let ImportLayer::File(target) = layer { if let Some(def) = target.resolve_export(db, interned) { - return Some(def); + return vec![def]; } } } - None + Vec::new() + } + + fn resolve_definition( + self, + db: &'db dyn Db, + scope: ScopeId, + def_id: DefinitionId, + ) -> Option> { + let index = self.semantic_index(db); + if let DefinitionKind::Import { + file: target_url, + name: forwarded, + .. + } = index.definitions(scope)[def_id].kind() + { + let target = db.file_by_url(&FilePath::from_url(target_url))?; + return target.resolve_export(db, Name::new(db, forwarded.as_str())); + } + self.definition(db, scope, def_id) } /// Walk this file's exports chain for `name`, chasing `source()`-forwarded diff --git a/crates/oak_db/src/imports.rs b/crates/oak_db/src/imports.rs index 0f09f06753..c29a5c5a95 100644 --- a/crates/oak_db/src/imports.rs +++ b/crates/oak_db/src/imports.rs @@ -53,6 +53,11 @@ impl<'db> ImportsResolver for SalsaImportsResolver<'db> { fn resolve_source(&mut self, path: &str) -> Option { let anchor = anchor_dir(self.db, self.calling_file)?; let url = resolve_relative_to(&anchor, path)?; + // TODO(diagnostics): a `source()` target outside the workspace never + // becomes a `File` (nothing watches or scans it), so `file_by_url` + // misses it and the names it injects are invisible. Picking these up + // (to resolve and to lint) needs a non-LSP file watcher we don't have + // yet. let file = self.db.file_by_url(&url)?; let names: Vec = file diff --git a/crates/oak_db/src/tests/file.rs b/crates/oak_db/src/tests/file.rs index f2cf9c4610..936ac082ac 100644 --- a/crates/oak_db/src/tests/file.rs +++ b/crates/oak_db/src/tests/file.rs @@ -1,3 +1,4 @@ +use biome_rowan::TextSize; use salsa::Setter; use crate::tests::test_db::file_url; @@ -24,6 +25,40 @@ fn test_parse_is_cached_across_calls() { assert_eq!(db.executions("parse"), 1); } +#[test] +fn test_line_index_is_cached_across_calls() { + let mut db = TestDb::new(); + let file = new_file(&mut db, "a.R", "x <- 1\n"); + + let _ = file.line_index(&db); + assert_eq!(db.executions("line_index"), 1); + + let _ = file.line_index(&db); + assert_eq!(db.executions("line_index"), 1); +} + +#[test] +fn test_line_index_recomputes_on_content_change() { + let mut db = TestDb::new(); + let file = new_file(&mut db, "a.R", "x <- 1\n"); + + // One offset per line start: byte 0, then just past the `\n` at byte 6. + assert_eq!(file.line_index(&db).newlines, vec![ + TextSize::from(0u32), + TextSize::from(7u32) + ]); + assert_eq!(db.executions("line_index"), 1); + + file.set_contents(&mut db).to("x\ny\nz\n".to_string()); + assert_eq!(file.line_index(&db).newlines, vec![ + TextSize::from(0u32), + TextSize::from(2u32), + TextSize::from(4u32), + TextSize::from(6u32), + ]); + assert_eq!(db.executions("line_index"), 2); +} + #[test] fn test_semantic_index_is_cached_across_calls() { let mut db = TestDb::new(); diff --git a/crates/oak_db/src/tests/file_imports_at.rs b/crates/oak_db/src/tests/file_imports_at.rs index 0592d7aa93..e31968d3d3 100644 --- a/crates/oak_db/src/tests/file_imports_at.rs +++ b/crates/oak_db/src/tests/file_imports_at.rs @@ -217,3 +217,30 @@ fn test_package_namespace_and_base_layers_always_visible() { let layers = file.imports_at(&db, TextSize::from(0)); assert!(attached_packages(&layers).contains(&base)); } + +#[test] +fn test_library_in_function_scoped_source_is_visible_only_in_that_function() { + // A `library()` inside a file that's `source()`d from a function body is + // forwarded by the builder as an `Attach` scoped to that `source()` call, + // so its `Package` layer is visible inside the function (the lazy / EOF + // view) but not at file scope before or after it. + let mut db = TestDb::new(); + let dplyr = install_packages(&mut db, &["dplyr"])[0]; + + let helpers = make_file(&mut db, "w/helpers.R", "library(dplyr)\n"); + let script_src = "before\nf <- function() {\n source(\"helpers.R\")\n}\nafter\n"; + let script = make_file(&mut db, "w/script.R", script_src); + + let root = workspace_root(&db, "w"); + root.set_scripts(&mut db).to(vec![helpers, script]); + db.workspace_roots().set_roots(&mut db).to(vec![root]); + + let at = |needle: &str| { + let offset = TextSize::from(script_src.find(needle).unwrap() as u32); + attached_packages(&script.imports_at(&db, offset)) + }; + + assert!(!at("before").contains(&dplyr)); + assert!(at("source").contains(&dplyr)); + assert!(!at("after").contains(&dplyr)); +} diff --git a/crates/oak_db/src/tests/file_resolve.rs b/crates/oak_db/src/tests/file_resolve.rs index 2b627146c6..49216173ca 100644 --- a/crates/oak_db/src/tests/file_resolve.rs +++ b/crates/oak_db/src/tests/file_resolve.rs @@ -290,10 +290,11 @@ fn test_definition_id_stable_across_def_id_renumber_local_path() { let files = setup_workspace(&mut db, &[("w/a.R", content1)]); let file = files[0]; - let id1 = file - .resolve_at(&db, TextSize::from(use1 as u32)) - .expect("use of x resolves to its function-scope binding") - .as_id(); + let id1 = { + let defs = file.resolve_at(&db, TextSize::from(use1 as u32)); + assert_eq!(defs.len(), 1); + defs[0].as_id() + }; // Prepend an unrelated binding inside the function so x's DefinitionId // shifts 0 -> 1 within the function scope. @@ -301,10 +302,11 @@ fn test_definition_id_stable_across_def_id_renumber_local_path() { let use2 = content2.find("\nx\n").expect("standalone use of x") + 1; file.set_contents(&mut db).to(content2.to_string()); - let id2 = file - .resolve_at(&db, TextSize::from(use2 as u32)) - .expect("use of x still resolves") - .as_id(); + let id2 = { + let defs = file.resolve_at(&db, TextSize::from(use2 as u32)); + assert_eq!(defs.len(), 1); + defs[0].as_id() + }; assert_eq!(id1, id2); } diff --git a/crates/oak_db/src/tests/file_resolve_at.rs b/crates/oak_db/src/tests/file_resolve_at.rs index 4d568db32d..1e3adec343 100644 --- a/crates/oak_db/src/tests/file_resolve_at.rs +++ b/crates/oak_db/src/tests/file_resolve_at.rs @@ -6,6 +6,7 @@ use crate::tests::test_db::file_url; use crate::tests::test_db::workspace_root; use crate::tests::test_db::TestDb; use crate::DbInputs; +use crate::Definition; use crate::File; use crate::Package; use crate::Root; @@ -14,6 +15,14 @@ fn make_file(db: &mut TestDb, path: &str, contents: &str) -> File { File::new(db, file_url(path), contents.to_string(), None) } +/// Resolve at `offset`, asserting exactly one definition. Most cases are +/// unambiguous; the ambiguous ones (e.g. `if`/`else`) have their own test. +fn resolve_one(db: &TestDb, file: File, offset: TextSize) -> Definition<'_> { + let defs = file.resolve_at(db, offset); + assert_eq!(defs.len(), 1); + defs[0] +} + fn make_package_file(db: &mut TestDb, path: &str, contents: &str, package: Package) -> File { File::new(db, file_url(path), contents.to_string(), Some(package)) } @@ -61,7 +70,7 @@ fn test_resolves_function_parameter_at_use_site() { // Cursor on the second `x` (the use inside the function body). let offset = TextSize::from(source.rfind('x').unwrap() as u32); - let def = file.resolve_at(&db, offset).expect("should resolve"); + let def = resolve_one(&db, file, offset); assert_eq!(def.file(&db), file); assert_eq!(def.name(&db).text(&db).as_str(), "x"); @@ -79,7 +88,7 @@ fn test_resolves_local_let_inside_function() { // Cursor on the second `y` (the use after the local binding). let offset = TextSize::from(source.rfind('y').unwrap() as u32); - let def = file.resolve_at(&db, offset).expect("should resolve"); + let def = resolve_one(&db, file, offset); assert_eq!(def.file(&db), file); assert_eq!(def.name(&db).text(&db).as_str(), "y"); @@ -96,7 +105,7 @@ fn test_resolves_file_top_level_binding() { let file = make_file(&mut db, "a.R", source); let offset = TextSize::from(source.rfind('x').unwrap() as u32); - let def = file.resolve_at(&db, offset).expect("should resolve"); + let def = resolve_one(&db, file, offset); assert_eq!(def.file(&db), file); assert_eq!(def.name(&db).text(&db).as_str(), "x"); @@ -114,7 +123,7 @@ fn test_function_body_falls_through_to_file_top_level() { let file = make_file(&mut db, "a.R", source); let offset = TextSize::from(source.rfind('x').unwrap() as u32); - let def = file.resolve_at(&db, offset).expect("should resolve"); + let def = resolve_one(&db, file, offset); assert_eq!(def.file(&db), file); assert_eq!(def.name(&db).text(&db).as_str(), "x"); @@ -134,9 +143,7 @@ fn test_resolves_source_forwarded_name_to_origin_file() { let analysis_source = analysis.contents(&db).clone(); let offset = TextSize::from(analysis_source.find("helper()").unwrap() as u32); - let def = analysis - .resolve_at(&db, offset) - .expect("should resolve via source() forwarding"); + let def = resolve_one(&db, analysis, offset); assert_eq!(def.file(&db), helpers); assert_eq!(def.name(&db).text(&db).as_str(), "helper"); @@ -157,7 +164,7 @@ fn test_resolves_package_sibling_predecessor() { // in `b`'s own exports, then walks visible imports and reaches `a` // (a predecessor sibling). let offset = TextSize::from(b_source.rfind("shared").unwrap() as u32); - let def = b.resolve_at(&db, offset).expect("should resolve"); + let def = resolve_one(&db, b, offset); assert_eq!(def.file(&db), a); assert_eq!(def.name(&db).text(&db).as_str(), "shared"); @@ -179,7 +186,7 @@ fn test_local_after_source_shadows_forwarded_entry() { let analysis_source = analysis.contents(&db).clone(); let offset = TextSize::from(analysis_source.rfind("foo").unwrap() as u32); - let def = analysis.resolve_at(&db, offset).expect("should resolve"); + let def = resolve_one(&db, analysis, offset); assert_eq!(def.file(&db), analysis); assert_eq!(def.name(&db).text(&db).as_str(), "foo"); @@ -206,7 +213,7 @@ fn test_source_after_local_overrides_local() { let analysis_source = analysis.contents(&db).clone(); let offset = TextSize::from(analysis_source.rfind("foo").unwrap() as u32); - let def = analysis.resolve_at(&db, offset).expect("should resolve"); + let def = resolve_one(&db, analysis, offset); assert_eq!(def.file(&db), helpers); assert_eq!(def.name(&db).text(&db).as_str(), "foo"); @@ -219,7 +226,7 @@ fn test_unbound_name_returns_none() { let file = make_file(&mut db, "a.R", source); let offset = TextSize::from(0); - assert!(file.resolve_at(&db, offset).is_none()); + assert!(file.resolve_at(&db, offset).is_empty()); } #[test] @@ -230,5 +237,104 @@ fn test_offset_not_on_any_use_returns_none() { // Cursor on the `<-` operator, not on any identifier use. let offset = TextSize::from(source.find("<-").unwrap() as u32); - assert!(file.resolve_at(&db, offset).is_none()); + assert!(file.resolve_at(&db, offset).is_empty()); +} + +#[test] +fn test_top_level_use_between_defs_binds_reaching_def() { + // A use sitting between two top-level defs of the same name binds to the + // earlier (reaching) def, not the later one. The EOF `exports()` view + // would wrongly pick `foo <- 2`. + let mut db = TestDb::new(); + let source = "foo <- 1\nfoo\nfoo <- 2\n"; + let file = make_file(&mut db, "a.R", source); + + let offset = TextSize::from(source.find("\nfoo\n").unwrap() as u32 + 1); + let def = resolve_one(&db, file, offset); + + assert_eq!(def.file(&db), file); + let range = def.name_range(&db).expect("local has a name range"); + assert_eq!(usize::from(range.start()), source.find("foo <- 1").unwrap()); +} + +#[test] +fn test_top_level_use_after_redefinition_binds_latest_def() { + // A use after both defs binds to the latest one, the same answer the EOF + // view gives. Guards against over-correcting the reaching-def fix. + let mut db = TestDb::new(); + let source = "foo <- 1\nfoo <- 2\nfoo\n"; + let file = make_file(&mut db, "a.R", source); + + let offset = TextSize::from(source.rfind("foo").unwrap() as u32); + let def = resolve_one(&db, file, offset); + + let range = def.name_range(&db).expect("local has a name range"); + assert_eq!(usize::from(range.start()), source.find("foo <- 2").unwrap()); +} + +#[test] +fn test_cursor_on_assignment_target_resolves_to_itself() { + // Cursor on the `foo` being bound, not a use of it: navigate to self. + let mut db = TestDb::new(); + let source = "foo <- 1\n"; + let file = make_file(&mut db, "a.R", source); + + let def = resolve_one(&db, file, TextSize::from(0)); + + assert_eq!(def.file(&db), file); + assert_eq!(def.name(&db).text(&db).as_str(), "foo"); + let range = def.name_range(&db).expect("local has a name range"); + assert_eq!(usize::from(range.start()), 0); +} + +#[test] +fn test_cursor_on_parameter_declaration_resolves_to_itself() { + let mut db = TestDb::new(); + let source = "f <- function(x) 1\n"; + let file = make_file(&mut db, "a.R", source); + + // Cursor on the `x` parameter declaration. + let offset = TextSize::from(source.find("(x)").unwrap() as u32 + 1); + let def = resolve_one(&db, file, offset); + + let range = def.name_range(&db).expect("local has a name range"); + assert_eq!(range.start(), offset); +} + +#[test] +fn test_top_level_conditional_reports_both_arm_defs() { + // A name defined on both arms of an `if`/`else` is ambiguous at the use: + // either arm could have run, so both defs are reported. + let mut db = TestDb::new(); + let source = "if (cond) foo <- 1 else foo <- 2\nfoo\n"; + let file = make_file(&mut db, "a.R", source); + + let offset = TextSize::from(source.rfind("foo").unwrap() as u32); + let defs = file.resolve_at(&db, offset); + + let starts: Vec = defs + .iter() + .map(|d| usize::from(d.name_range(&db).expect("local has a name range").start())) + .collect(); + assert_eq!(defs.len(), 2); + assert!(starts.contains(&source.find("foo <- 1").unwrap())); + assert!(starts.contains(&source.find("foo <- 2").unwrap())); } + +// Package-layer resolution, pending. `resolve` / `resolve_at` walk only +// `ImportLayer::File`; the `From` / `Package` layers (`library()`, NAMESPACE +// `importFrom`, the base search path) are deferred, see the TODOs in +// `file_resolve.rs`. These scenarios came from the old goto-def `LegacyDb` +// suite, which only asserted them as unsupported (resolved to `None`). When +// package layers land, add them here on `resolve_at`: +// +// - `library(pkg)` in a script makes a `pkg` export resolve at a later use. +// - `importFrom(dplyr, mutate)` in a package's NAMESPACE makes `mutate` resolve. +// - a package file resolves base symbols (e.g. `cat`). +// - a standalone script resolves base / default-attached symbols. +// - a script's search path is identical at top level and in a function body. +// - `library()` attached inside a sourced file is visible to a function body +// that runs after the `source()`. +// +// Resolving package symbols also needs a navigable location for installed +// package files, which aren't `oak_db::File` entities yet. diff --git a/crates/oak_ide/Cargo.toml b/crates/oak_ide/Cargo.toml index 05820636e8..1e523dea02 100644 --- a/crates/oak_ide/Cargo.toml +++ b/crates/oak_ide/Cargo.toml @@ -18,9 +18,12 @@ aether_syntax.workspace = true anyhow.workspace = true biome_rowan.workspace = true oak_core.workspace = true +oak_db.workspace = true oak_semantic.workspace = true url.workspace = true [dev-dependencies] aether_parser.workspace = true +aether_path.workspace = true +oak_scan.workspace = true oak_semantic = { workspace = true, features = ["testing"] } diff --git a/crates/oak_ide/src/goto_definition.rs b/crates/oak_ide/src/goto_definition.rs index 0d8bd38eb2..293a5cd0eb 100644 --- a/crates/oak_ide/src/goto_definition.rs +++ b/crates/oak_ide/src/goto_definition.rs @@ -1,70 +1,28 @@ -use aether_syntax::RSyntaxNode; -use oak_semantic::semantic_index::SemanticIndex; -use oak_semantic::ScopeId; -use oak_semantic::UseId; -use url::Url; +use biome_rowan::TextSize; +use oak_db::Db; +use oak_db::File; -use crate::FilePosition; -use crate::Identifier; use crate::NavigationTarget; -/// Resolve the symbol at `offset` in a file. +/// Resolve the name at `offset` in `file` and describe where to jump. /// -/// TODO(salsa) Within-file only. `pkg::sym` and any unresolved free variable -/// currently return an empty list. Cross-file resolution lives in the -/// salsa-backed path and will be wired in later. -pub fn goto_definition( - index: &SemanticIndex, - root: &RSyntaxNode, - position: &FilePosition, -) -> Vec { - let Some(ident) = Identifier::classify(index, root, position.offset) else { - return Vec::new(); - }; - - match ident { - Identifier::Definition { def, name, .. } => { - vec![NavigationTarget { - file: position.file.clone(), - name: name.to_string(), - full_range: def.range(), - focus_range: def.range(), - }] - }, - Identifier::Use { - scope_id, - use_id, - name, - .. - } => resolve_use(index, &position.file, scope_id, use_id, name), - Identifier::NamespaceAccess { .. } => Vec::new(), - } -} - -fn resolve_use( - index: &SemanticIndex, - file: &Url, - scope_id: ScopeId, - use_id: UseId, - name: &str, -) -> Vec { - // `reaching_definitions` unions the local use-def map with the - // enclosing-scope snapshot when `may_be_unbound` is true. That - // covers the conditional-local-plus-outer-binding case where both - // the inner conditional def and the outer def can reach the use. - // - // TODO(salsa): when the result is empty, fall through to cross-file - // resolution (file imports / package exports). - index - .reaching_definitions(scope_id, use_id) - .map(|(scope, def_id)| { - let def = &index.definitions(scope)[def_id]; - NavigationTarget { - file: file.clone(), - name: name.to_string(), - full_range: def.range(), - focus_range: def.range(), - } +/// Thin wrapper over [`File::resolve_at`], which runs R's whole lookup chain +/// (lexical scopes, function-body resolution, top-level reaching defs, file +/// imports). Each binding that could reach the use becomes a +/// [`NavigationTarget`] in its own file's coordinates, so an ambiguous name +/// (e.g. defined on both arms of an `if`/`else`) yields several. Empty means +/// the name isn't reachable, rather than guessing by name across the workspace. +pub fn goto_definition(db: &dyn Db, file: File, offset: TextSize) -> Vec { + file.resolve_at(db, offset) + .into_iter() + .filter_map(|def| { + let range = def.name_range(db)?; + Some(NavigationTarget { + file: def.file(db), + name: def.name(db).text(db).to_string(), + full_range: range, + focus_range: range, + }) }) .collect() } diff --git a/crates/oak_ide/src/lib.rs b/crates/oak_ide/src/lib.rs index eefc25761b..dc6a12a185 100644 --- a/crates/oak_ide/src/lib.rs +++ b/crates/oak_ide/src/lib.rs @@ -9,6 +9,7 @@ pub use find_references::find_references; pub use find_references::References; pub use goto_definition::goto_definition; pub use identifier::Identifier; +use oak_db::File; pub use rename::prepare_rename; pub use rename::rename; pub use rename::RenameTargets; @@ -37,7 +38,7 @@ pub struct FileRange { /// document-highlight), use `FileRange` directly. #[derive(Debug, Clone, PartialEq, Eq)] pub struct NavigationTarget { - pub file: Url, + pub file: File, pub name: String, pub full_range: TextRange, pub focus_range: TextRange, diff --git a/crates/oak_ide/tests/integration/goto_definition.rs b/crates/oak_ide/tests/integration/goto_definition.rs index a6742f7a47..481143bb15 100644 --- a/crates/oak_ide/tests/integration/goto_definition.rs +++ b/crates/oak_ide/tests/integration/goto_definition.rs @@ -1,1673 +1,62 @@ -use aether_parser::parse; -use aether_parser::RParserOptions; -use aether_syntax::RSyntaxNode; +//! Goto-definition at the ide layer. +//! +//! These only check that `oak_ide::goto_definition` assembles a +//! `NavigationTarget` from a resolved binding, for a local def and a +//! cross-file `source()` jump. The resolution itself is covered exhaustively +//! by `oak_db`'s `file_resolve_at` / `file_resolve` tests, and the use-def +//! logic by `oak_semantic`; we don't re-test it here. + +use aether_path::FilePath; use biome_rowan::TextRange; use biome_rowan::TextSize; +use oak_db::File; +use oak_db::OakDatabase; use oak_ide::goto_definition; -use oak_ide::FilePosition; -use oak_ide::NavigationTarget; -use oak_semantic::build_index; -use oak_semantic::semantic_index::SemanticIndex; -use oak_semantic::NoopImportsResolver; +use oak_scan::DbExt; use url::Url; -fn parse_source(source: &str) -> (RSyntaxNode, SemanticIndex) { - let parsed = parse(source, RParserOptions::default()); - let root = parsed.syntax(); - let index = build_index(&parsed.tree(), NoopImportsResolver); - (root, index) -} - -fn text_range(start: u32, end: u32) -> TextRange { - TextRange::new(TextSize::from(start), TextSize::from(end)) -} - fn file_url(name: &str) -> Url { Url::parse(&format!("file:///project/R/{name}")).unwrap() } -fn offset(n: u32) -> TextSize { - TextSize::from(n) -} - -fn pos(file: &Url, n: u32) -> FilePosition { - FilePosition { - file: file.clone(), - offset: offset(n), - } -} - -// --- Local resolution --- - -#[test] -fn test_local_simple() { - // "x <- 1\nx\n" - // 0123456 78 - let source = "x <- 1\nx\n"; - let file = file_url("test.R"); - let (root, idx) = parse_source(source); - - let targets = goto_definition(&idx, &root, &pos(&file, 7)); - assert_eq!(targets, vec![NavigationTarget { - file, - name: "x".to_string(), - full_range: text_range(0, 1), - focus_range: text_range(0, 1), - }]); -} - -#[test] -fn test_local_reassignment_shadows() { - // Straight-line reassignment: second def kills the first - let source = "x <- 1\nx <- 2\nx\n"; - let file = file_url("test.R"); - let (root, idx) = parse_source(source); - - let targets = goto_definition(&idx, &root, &pos(&file, 14)); - assert_eq!(targets, vec![NavigationTarget { - file, - name: "x".to_string(), - full_range: text_range(7, 8), - focus_range: text_range(7, 8), - }]); -} - -#[test] -fn test_local_conditional_returns_both() { - let source = "if (TRUE) x <- 1 else x <- 2\nx\n"; - let file = file_url("test.R"); - let (root, idx) = parse_source(source); - - let use_offset = source.rfind('x').unwrap() as u32; - - let targets = goto_definition(&idx, &root, &pos(&file, use_offset)); - assert_eq!(targets, vec![ - NavigationTarget { - file: file.clone(), - name: "x".to_string(), - full_range: text_range(10, 11), - focus_range: text_range(10, 11), - }, - NavigationTarget { - file, - name: "x".to_string(), - full_range: text_range(22, 23), - focus_range: text_range(22, 23), - }, - ]); -} - -#[test] -fn test_local_in_function() { - let source = "f <- function() {\n x <- 1\n x\n}\n"; - let file = file_url("test.R"); - let (root, idx) = parse_source(source); - - let use_offset = source.rfind('x').unwrap() as u32; - let targets = goto_definition(&idx, &root, &pos(&file, use_offset)); - assert_eq!(targets, vec![NavigationTarget { - file, - name: "x".to_string(), - full_range: text_range(20, 21), - focus_range: text_range(20, 21), - }]); -} - -#[test] -fn test_local_parameter() { - let source = "f <- function(x) {\n x\n}\n"; - let file = file_url("test.R"); - let (root, idx) = parse_source(source); - - let use_offset = source.rfind('x').unwrap() as u32; - let targets = goto_definition(&idx, &root, &pos(&file, use_offset)); - assert_eq!(targets, vec![NavigationTarget { - file, - name: "x".to_string(), - full_range: text_range(14, 15), - focus_range: text_range(14, 15), - }]); -} - -// --- Enclosing scope resolution --- - -#[test] -fn test_enclosing_scope() { - let source = "x <- 1\nf <- function() {\n x\n}\n"; - let file = file_url("test.R"); - let (root, idx) = parse_source(source); - - let use_offset = source.rfind('x').unwrap() as u32; - let targets = goto_definition(&idx, &root, &pos(&file, use_offset)); - assert_eq!(targets, vec![NavigationTarget { - file, - name: "x".to_string(), - full_range: text_range(0, 1), - focus_range: text_range(0, 1), - }]); -} - -#[test] -fn test_conditional_local_falls_through_to_enclosing() { - // Inside `f`, `x` is conditionally defined: bindings = {inner def} - // with `may_be_unbound = true`. At runtime the use of `x` reaches - // either the inner def (when the branch ran) or the outer def (when - // it didn't). Goto-def should return both. - // - // "x <- 1\nf <- function(cond) {\n if (cond) x <- 2\n x\n}\n" - // 0 7 30 42 49 - let source = "x <- 1\nf <- function(cond) {\n if (cond) x <- 2\n x\n}\n"; - let file = file_url("test.R"); - let (root, idx) = parse_source(source); - - let outer_def = source.find("x <- 1").unwrap() as u32; - let inner_def = source.find("x <- 2").unwrap() as u32; - let use_offset = source.rfind('x').unwrap() as u32; - - let targets = goto_definition(&idx, &root, &pos(&file, use_offset)); - let ranges: Vec<_> = targets.into_iter().map(|t| t.full_range).collect(); - assert!(ranges.contains(&text_range(outer_def, outer_def + 1))); - assert!(ranges.contains(&text_range(inner_def, inner_def + 1))); - assert_eq!(ranges.len(), 2); -} - -// --- Member access (`$`) --- - -#[test] -fn test_dollar_lhs_resolves() { - // Cursor on `foo` in `foo$bar` resolves to the definition of `foo` - let source = "foo <- list()\nfoo$bar\n"; - let file = file_url("test.R"); - let (root, idx) = parse_source(source); - - // `foo` in `foo$bar` starts at offset 14 - let targets = goto_definition(&idx, &root, &pos(&file, 14)); - assert_eq!(targets, vec![NavigationTarget { - file, - name: "foo".to_string(), - full_range: text_range(0, 3), - focus_range: text_range(0, 3), - }]); -} - -#[test] -fn test_dollar_rhs_no_resolution() { - // Cursor on `bar` in `foo$bar`: member names are not tracked by the index - let source = "foo <- list()\nfoo$bar\n"; - let file = file_url("test.R"); - let (root, idx) = parse_source(source); - - // `bar` starts at offset 18 - let targets = goto_definition(&idx, &root, &pos(&file, 18)); - assert!(targets.is_empty()); -} - -// --- No resolution --- - -#[test] -fn test_no_use_at_offset() { - let source = "x <- 1\n"; - let file = file_url("test.R"); - let (root, idx) = parse_source(source); - - let targets = goto_definition(&idx, &root, &pos(&file, 3)); - assert!(targets.is_empty()); -} - -#[test] -fn test_unresolved_symbol() { - let source = "foo\n"; - let file = file_url("test.R"); - let (root, idx) = parse_source(source); - - let targets = goto_definition(&idx, &root, &pos(&file, 0)); - assert!(targets.is_empty()); -} - -// --- Definition site navigation --- - -#[test] -fn test_definition_site_assignment() { - // Cursor on the `foo` in `foo <- 1` should navigate to itself - let source = "foo <- 1\n"; - let file = file_url("test.R"); - let (root, idx) = parse_source(source); - - let targets = goto_definition(&idx, &root, &pos(&file, 0)); - assert_eq!(targets, vec![NavigationTarget { - file, - name: "foo".to_string(), - full_range: text_range(0, 3), - focus_range: text_range(0, 3), - }]); -} - -#[test] -fn test_definition_site_parameter() { - let source = "f <- function(x) { x }\n"; - let file = file_url("test.R"); - let (root, idx) = parse_source(source); - - // Cursor on the `x` parameter name (offset 14) - let targets = goto_definition(&idx, &root, &pos(&file, 14)); - assert_eq!(targets, vec![NavigationTarget { - file, - name: "x".to_string(), - full_range: text_range(14, 15), - focus_range: text_range(14, 15), - }]); -} - -#[test] -fn test_definition_site_for_variable() { - let source = "for (i in 1:10) i\n"; - let file = file_url("test.R"); - let (root, idx) = parse_source(source); - - // Cursor on the `i` in `for (i in ...)` - let targets = goto_definition(&idx, &root, &pos(&file, 5)); - assert_eq!(targets, vec![NavigationTarget { - file, - name: "i".to_string(), - full_range: text_range(5, 6), - focus_range: text_range(5, 6), - }]); -} - -// --- Right assignment --- - -#[test] -fn test_right_assignment_definition_site() { - // `1 -> x`: cursor on `x` (the definition target) - let source = "1 -> x\nx\n"; - let file = file_url("test.R"); - let (root, idx) = parse_source(source); - - let targets = goto_definition(&idx, &root, &pos(&file, 5)); - assert_eq!(targets, vec![NavigationTarget { - file: file.clone(), - name: "x".to_string(), - full_range: text_range(5, 6), - focus_range: text_range(5, 6), - }]); -} - -#[test] -fn test_right_assignment_use_resolves() { - // `1 -> x` then use `x` - let source = "1 -> x\nx\n"; - let file = file_url("test.R"); - let (root, idx) = parse_source(source); - - let targets = goto_definition(&idx, &root, &pos(&file, 7)); - assert_eq!(targets, vec![NavigationTarget { - file, - name: "x".to_string(), - full_range: text_range(5, 6), - focus_range: text_range(5, 6), - }]); -} - -// --- Super assignment --- - -#[test] -fn test_super_assignment_resolves_in_enclosing() { - // `x <<- 1` inside a function creates a definition in the file scope. - // A use of `x` in another function should resolve to it. - let source = "f <- function() x <<- 1\ng <- function() x\n"; - let file = file_url("test.R"); - let (root, idx) = parse_source(source); - - // `x` use in `g` body - let use_offset = source.rfind('x').unwrap() as u32; - let targets = goto_definition(&idx, &root, &pos(&file, use_offset)); - assert_eq!(targets.len(), 1); - assert_eq!(targets[0].name, "x"); - assert_eq!(targets[0].file, file); -} - -#[test] -fn test_super_assignment_definition_site() { - // Cursor on `x` in `x <<- 1` - let source = "f <- function() {\n x <<- 1\n}\n"; - let file = file_url("test.R"); - let (root, idx) = parse_source(source); - - // `x` at offset 20 - let def_offset = source.find("x <<-").unwrap() as u32; - let targets = goto_definition(&idx, &root, &pos(&file, def_offset)); - assert_eq!(targets.len(), 1); - assert_eq!(targets[0].name, "x"); -} - -// --- String definitions --- - -#[test] -fn test_string_definition() { - // `"foo" <- 1` is equivalent to `foo <- 1` in R - let source = "\"foo\" <- 1\nfoo\n"; - let file = file_url("test.R"); - let (root, idx) = parse_source(source); - - // Use of `foo` at offset 11 - let targets = goto_definition(&idx, &root, &pos(&file, 11)); - assert_eq!(targets, vec![NavigationTarget { - file, - name: "foo".to_string(), - // The definition range covers the string literal `"foo"` - full_range: text_range(0, 5), - focus_range: text_range(0, 5), - }]); -} - -// --- Nested functions --- - -#[test] -fn test_deeply_nested_function() { - // Free variable `z` resolves through two function scopes to file scope - let source = "z <- 1\nf <- function() {\n g <- function() {\n z\n }\n}\n"; - let file = file_url("test.R"); - let (root, idx) = parse_source(source); - - let use_offset = source.rfind('z').unwrap() as u32; - let targets = goto_definition(&idx, &root, &pos(&file, use_offset)); - assert_eq!(targets, vec![NavigationTarget { - file, - name: "z".to_string(), - full_range: text_range(0, 1), - focus_range: text_range(0, 1), - }]); -} - -// --- Use on RHS of assignment --- - -#[test] -fn test_use_on_rhs_of_assignment() { - // `x <- x + 1`: the `x` on the RHS refers to the previous binding - let source = "x <- 1\nx <- x + 1\n"; - let file = file_url("test.R"); - let (root, idx) = parse_source(source); - - // The `x` on the RHS of the second assignment. `x <- x + 1` starts at - // offset 7, the RHS `x` is at offset 12. - let rhs_offset = 7 + "x <- ".len() as u32; - let targets = goto_definition(&idx, &root, &pos(&file, rhs_offset)); - assert_eq!(targets, vec![NavigationTarget { - file, - name: "x".to_string(), - // Resolves to the first definition - full_range: text_range(0, 1), - focus_range: text_range(0, 1), - }]); -} - -// --- Namespace access (`::`, `:::`) returns empty in the within-file handler --- - -#[test] -fn test_namespace_access_returns_empty() { - let source = "dplyr::mutate\n"; - let file = file_url("test.R"); - let (root, idx) = parse_source(source); - - // Cursor on `mutate` (offset 7) - let targets = goto_definition(&idx, &root, &pos(&file, 7)); - assert!(targets.is_empty()); - - // Cursor on `dplyr` (offset 0) - let targets = goto_definition(&idx, &root, &pos(&file, 0)); - assert!(targets.is_empty()); -} - -// --- Identifier::classify keeps recognizing `pkg::sym` --- - -#[test] -fn test_namespace_classify() { - use oak_ide::Identifier; - - let source = "dplyr::mutate\n"; - let parsed = parse(source, RParserOptions::default()); - let root = parsed.syntax(); - let idx = build_index(&parsed.tree(), NoopImportsResolver); - - // Cursor on `mutate` (offset 7) - let ident = Identifier::classify(&idx, &root, offset(7)); - assert_eq!( - ident, - Some(Identifier::NamespaceAccess { - package: "dplyr".to_string(), - symbol: "mutate".to_string(), - internal: false, - package_range: text_range(0, 5), - symbol_range: text_range(7, 13), - }) - ); - - // Cursor on `dplyr` (offset 2) - let ident = Identifier::classify(&idx, &root, offset(2)); - assert_eq!( - ident, - Some(Identifier::NamespaceAccess { - package: "dplyr".to_string(), - symbol: "mutate".to_string(), - internal: false, - package_range: text_range(0, 5), - symbol_range: text_range(7, 13), - }) - ); -} - -#[test] -fn test_namespace_classify_triple_colon() { - use oak_ide::Identifier; - - let source = "pkg:::sym\n"; - let parsed = parse(source, RParserOptions::default()); - let root = parsed.syntax(); - let idx = build_index(&parsed.tree(), NoopImportsResolver); - - let ident = Identifier::classify(&idx, &root, offset(6)); - assert_eq!( - ident, - Some(Identifier::NamespaceAccess { - package: "pkg".to_string(), - symbol: "sym".to_string(), - internal: true, - package_range: text_range(0, 3), - symbol_range: text_range(6, 9), - }) - ); -} - -#[test] -fn test_namespace_classify_in_call() { - use oak_ide::Identifier; - - // foo::bar() - // 0123456789 - let source = "foo::bar()\n"; - let parsed = parse(source, RParserOptions::default()); - let root = parsed.syntax(); - let idx = build_index(&parsed.tree(), NoopImportsResolver); - - let ident = Identifier::classify(&idx, &root, offset(5)); - assert_eq!( - ident, - Some(Identifier::NamespaceAccess { - package: "foo".to_string(), - symbol: "bar".to_string(), - internal: false, - package_range: text_range(0, 3), - symbol_range: text_range(5, 8), - }) - ); -} - -#[test] -fn test_namespace_classify_in_extract() { - use oak_ide::Identifier; - - // foo::bar$baz - // 0123456789... - let source = "foo::bar$baz\n"; - let parsed = parse(source, RParserOptions::default()); - let root = parsed.syntax(); - let idx = build_index(&parsed.tree(), NoopImportsResolver); - - // Cursor on `bar` (offset 5) -- inside the RNamespaceExpression - let ident = Identifier::classify(&idx, &root, offset(5)); - assert_eq!( - ident, - Some(Identifier::NamespaceAccess { - package: "foo".to_string(), - symbol: "bar".to_string(), - internal: false, - package_range: text_range(0, 3), - symbol_range: text_range(5, 8), - }) - ); - - // Cursor on `baz` (offset 9) -- RHS of $, not a namespace access - let ident = Identifier::classify(&idx, &root, offset(9)); - assert_eq!(ident, None); -} - -#[test] -fn test_namespace_classify_string_selectors() { - use oak_ide::Identifier; - - // "foo"::"bar" - // 0123456789... - let source = "\"foo\"::\"bar\"\n"; - let parsed = parse(source, RParserOptions::default()); - let root = parsed.syntax(); - let idx = build_index(&parsed.tree(), NoopImportsResolver); - - let ident = Identifier::classify(&idx, &root, offset(7)); - assert_eq!( - ident, - Some(Identifier::NamespaceAccess { - package: "foo".to_string(), - symbol: "bar".to_string(), - internal: false, - package_range: text_range(0, 5), - symbol_range: text_range(7, 12), - }) - ); -} - -// Parked cross-file goto-definition tests pending the Salsa Oak wiring. -// -// Snapshot of the old `LegacyDb`-based suite, trimmed to the cross-file cases -// that the within-file handler doesn't cover (`source()`, package collation, -// `library()`, NAMESPACE imports, `pkg::sym` resolution). Intra-file tests live -// in the active section above. `#[cfg(any())]` skips this block at compile time -// so the references to deleted types (`LegacyDb`, `ExternalScope`, -// `ScopeLayer`, ...) stay as the behavioural spec to re-implement on top of -// `oak_db::File::resolve_at`. -#[cfg(any())] -#[rustfmt::skip] -mod parked_salsa_tests { -use std::collections::HashMap; -use std::path::PathBuf; -use std::sync::Arc; - -use aether_parser::parse; -use aether_parser::RParserOptions; -use aether_syntax::RSyntaxNode; -use biome_rowan::TextRange; -use biome_rowan::TextSize; -use oak_db::LegacyDb; -use oak_ide::goto_definition; -use oak_ide::ExternalScope; -use oak_ide::NavigationTarget; -use oak_package_metadata::description::Description; -use oak_package_metadata::namespace::Namespace; -use oak_semantic::build_index; -use oak_semantic::library::Library; -use oak_semantic::package::Package; -use oak_semantic::scope_layer::file_layers; -use oak_semantic::scope_layer::ScopeLayer; -use oak_semantic::semantic_index::SemanticCallKind; -use oak_semantic::semantic_index::SemanticIndex; -use oak_semantic::ImportsResolver; -use oak_semantic::NoopImportsResolver; -use oak_semantic::ScopeId; -use oak_semantic::SourceResolution; -use oak_sources::test::TestPackageCache; -use stdext::SortedVec; -use url::Url; - -fn parse_source(source: &str) -> (RSyntaxNode, SemanticIndex) { - let parsed = parse(source, RParserOptions::default()); - let root = parsed.syntax(); - let index = build_index(&parsed.tree(), NoopImportsResolver); - (root, index) -} - -/// Cross-file resolver that returns the same resolution for any path. Used -/// by the source()-resolution tests below; the path argument is irrelevant -/// because each test wires a single sourced file. -struct ConstResolver(SourceResolution); - -impl ImportsResolver for ConstResolver { - fn resolve_source(&mut self, _path: &str) -> Option { - Some(self.0.clone()) - } -} - -struct TestDb { - library: Library, - sources: HashMap, -} - -impl LegacyDb for TestDb { - fn semantic_index(&self, file: &Url) -> Option { - // Rebuild from source for tests. We store the source instead. - self.sources.get(file).map(|source| { - let parsed = parse(source, RParserOptions::default()); - build_index(&parsed.tree(), NoopImportsResolver) - }) - } - fn library(&self) -> &Library { - &self.library - } -} - -fn empty_library() -> Library { - Library::new(vec![], None) -} - -struct TestPackage { - name: String, - file: PathBuf, - exports: Vec, - internals: Vec, -} - -impl TestPackage { - // Most convenient input types for test construction - fn new(name: &str, file: &str, exports: Vec<&str>, internals: Vec<&str>) -> Self { - Self { - name: String::from(name), - file: PathBuf::from(file), - exports: exports - .into_iter() - .map(|export| export.to_string()) - .collect(), - internals: internals - .into_iter() - .map(|export| export.to_string()) - .collect(), - } - } -} - -fn test_library(packages: Vec) -> Library { - let cache = TestPackageCache::new().unwrap(); - - // Both exports and internals are in the test file - for package in &packages { - let content = test_file(&package.exports, &package.internals); - cache - .add(&package.name, vec![(package.file.as_path(), &content)]) - .expect("Can write to cache"); - } - - let cache = Arc::new(cache); - - let mut library = Library::new(vec![], Some(cache)); - - // Only exports are included in `Namespace` - for package in &packages { - let ns = Namespace { - exports: SortedVec::from_vec(package.exports.clone()), - ..Default::default() - }; - let desc = Description { - name: package.name.clone(), - ..Default::default() - }; - let pkg = Package::from_parts(PathBuf::from("/fake"), desc, ns); - library = library.insert(&package.name, pkg); - } - - library -} - -// Create a file worth of function definitions -fn test_file(exports: &Vec, internals: &Vec) -> String { - let mut out = String::new(); - - for export in exports { - out.push_str(&format!("{export} <- function() {{}}\n\n")); - } - for internal in internals { - out.push_str(&format!("{internal} <- function() {{}}\n\n")); - } - - out +fn upsert(db: &mut OakDatabase, name: &str, contents: &str) -> File { + db.upsert_editor(FilePath::from_url(&file_url(name)), contents.to_string()) } -fn text_range(start: u32, end: u32) -> TextRange { +fn range(start: u32, end: u32) -> TextRange { TextRange::new(TextSize::from(start), TextSize::from(end)) } -fn file_url(name: &str) -> Url { - Url::parse(&format!("file:///project/R/{name}")).unwrap() -} - -fn offset(n: u32) -> TextSize { - TextSize::from(n) -} - -// --- External resolution: project file --- - -#[test] -fn test_external_project_file() { - let source = "foo\n"; - let file = file_url("current.R"); - let (root, idx) = parse_source(source); - let db = TestDb { - library: empty_library(), - sources: HashMap::new(), - }; - - let other_url = file_url("other.R"); - let other_source = "foo <- 42\n"; - let (_other_root, other_idx) = parse_source(other_source); - let scope_chain = file_layers(other_url.clone(), &other_idx); - - let targets = goto_definition( - &db, - offset(0), - &file, - &root, - &idx, - &ExternalScope::package(scope_chain.clone(), scope_chain), - ); - assert_eq!(targets, vec![NavigationTarget { - file: other_url, - name: "foo".to_string(), - full_range: text_range(0, 3), - focus_range: text_range(0, 3), - }]); -} - -// --- External resolution: package --- - #[test] -fn test_external_package() { - let source = "mutate\n"; - let file = file_url("test.R"); - let (root, idx) = parse_source(source); - let library = test_library(vec![TestPackage::new( - "dplyr", - "dplyr.R", - vec!["filter", "mutate", "select"], - vec![], - )]); - let db = TestDb { - library, - sources: HashMap::new(), - }; - - let scope_chain = vec![ScopeLayer::PackageExports("dplyr".to_string())]; - - let targets = goto_definition( - &db, - offset(0), - &file, - &root, - &idx, - &ExternalScope::package(scope_chain.clone(), scope_chain), - ); +fn test_local_definition_navigates_to_binding() { + let mut db = OakDatabase::new(); + let file = upsert(&mut db, "a.R", "x <- 1\nx\n"); + // Cursor on the use `x` on the second line (offset 7). + let targets = goto_definition(&db, file, TextSize::from(7u32)); assert_eq!(targets.len(), 1); + let target = &targets[0]; - let target = targets.first().unwrap(); - assert!(target.file.path().ends_with("dplyr.R")); - assert_eq!(target.name, "mutate".to_string()); + assert_eq!(target.file, file); + assert_eq!(target.name, "x"); + assert_eq!(target.full_range, range(0, 1)); + assert_eq!(target.focus_range, range(0, 1)); } -// --- External resolution: importFrom --- - #[test] -fn test_external_import_from() { - let source = "tibble\n"; - let file = file_url("test.R"); - let (root, idx) = parse_source(source); - let db = TestDb { - library: empty_library(), - sources: HashMap::new(), - }; - - let mut imports = HashMap::new(); - imports.insert("tibble".to_string(), "tibble".to_string()); - let scope_chain = vec![ScopeLayer::PackageImports(imports)]; - - let targets = goto_definition( - &db, - offset(0), - &file, - &root, - &idx, - &ExternalScope::package(scope_chain.clone(), scope_chain), - ); - // importFrom resolves to a package, no file/range to navigate to - assert!(targets.is_empty()); -} - -// --- Use inside function body with cross-file definition --- - -#[test] -fn test_use_in_function_body_resolves_via_external() { - // Reproduces: `is_null` used inside a function body, defined in another - // file. The use is free in the function scope, so resolution should fall - // through enclosing scopes to the external scope chain. - let source = "f <- function(x) {\n if (is_null(x)) NULL\n}\n"; - let file = file_url("R/cnd-last.R"); - let (root, idx) = parse_source(source); - - let other_source = "is_null <- is.null\n"; - let (_other_root, other_idx) = parse_source(other_source); - let other_url = file_url("R/types.R"); - let scope_chain = file_layers(other_url.clone(), &other_idx); - - let db = TestDb { - library: empty_library(), - sources: HashMap::new(), - }; - - // `is_null` starts at offset 24 - let is_null_offset = source.find("is_null").unwrap(); - assert_eq!(is_null_offset, 25); - - let scope = ExternalScope::package(Vec::new(), scope_chain); - - let targets = goto_definition( - &db, - offset(is_null_offset as u32), - &file, - &root, - &idx, - &scope, - ); - assert_eq!(targets, vec![NavigationTarget { - file: other_url, - name: "is_null".to_string(), - full_range: text_range(0, 7), - focus_range: text_range(0, 7), - }]); -} - -// --- Local takes precedence over external --- - -#[test] -fn test_local_shadows_external() { - let source = "foo <- 1\nfoo\n"; - let file = file_url("test.R"); - let (root, idx) = parse_source(source); - let library = test_library(vec![TestPackage::new("pkg", "pkg.R", vec!["foo"], vec![])]); - let db = TestDb { - library, - sources: HashMap::new(), - }; - - let scope_chain = vec![ScopeLayer::PackageExports("pkg".to_string())]; - - let use_offset = source.rfind("foo").unwrap() as u32; - let targets = goto_definition( - &db, - offset(use_offset), - &file, - &root, - &idx, - &ExternalScope::package(scope_chain.clone(), scope_chain), - ); - assert_eq!(targets, vec![NavigationTarget { - file, - name: "foo".to_string(), - full_range: text_range(0, 3), - focus_range: text_range(0, 3), - }]); -} - -// --- Conditional definition (may_be_unbound but has local defs) --- - -#[test] -fn test_conditional_definition_includes_external() { - let source = "if (TRUE) x <- 1\nx\n"; - let file = file_url("test.R"); - let (root, idx) = parse_source(source); - - let other_url = file_url("other.R"); - let other_source = "x <- 99\n"; - let (_other_root, other_idx) = parse_source(other_source); - let scope_chain = file_layers(other_url.clone(), &other_idx); - - let db = TestDb { - library: empty_library(), - sources: HashMap::new(), - }; - - let use_offset = source.rfind('x').unwrap() as u32; - let targets = goto_definition( - &db, - offset(use_offset), - &file, - &root, - &idx, - &ExternalScope::package(scope_chain.clone(), scope_chain), - ); - assert_eq!(targets, vec![ - NavigationTarget { - file, - name: "x".to_string(), - full_range: text_range(10, 11), - focus_range: text_range(10, 11), - }, - NavigationTarget { - file: other_url, - name: "x".to_string(), - full_range: text_range(0, 1), - focus_range: text_range(0, 1), - }, - ]); -} - -// --- library() directive in predecessor file --- - -#[test] -fn test_library_directive_in_predecessor() { - // aaa.R has `library(dplyr)`, bbb.R uses `mutate`. - // The library() directive in aaa.R should make dplyr exports visible. - let aaa_source = "library(dplyr)\n"; - let (_aaa_root, aaa_idx) = parse_source(aaa_source); - let aaa_url = file_url("R/aaa.R"); - - let bbb_source = "mutate\n"; - let bbb_url = file_url("R/bbb.R"); - let (bbb_root, bbb_idx) = parse_source(bbb_source); - - let aaa_layers = file_layers(aaa_url, &aaa_idx); - let library = test_library(vec![TestPackage::new( - "dplyr", - "dplyr.R", - vec!["filter", "mutate", "select"], - vec![], - )]); - let db = TestDb { - library, - sources: HashMap::new(), - }; - - let scope = ExternalScope::package(aaa_layers.clone(), aaa_layers); - - let targets = goto_definition(&db, offset(0), &bbb_url, &bbb_root, &bbb_idx, &scope); - - assert_eq!(targets.len(), 1); - - let target = targets.first().unwrap(); - assert!(target.file.path().ends_with("dplyr.R")); - assert_eq!(target.name, "mutate".to_string()); -} - -// --- Namespace access (:: and :::) --- - -#[test] -fn test_namespace_access_exported_symbol() { - // "dplyr::mutate\n" - // 0123456789... - // Cursor on `mutate` (offset 7) - let source = "dplyr::mutate\n"; - let file = file_url("test.R"); - let (root, idx) = parse_source(source); - let library = test_library(vec![TestPackage::new( - "dplyr", - "dplyr.R", - vec!["filter", "mutate", "select"], - vec![], - )]); - let db = TestDb { - library, - sources: HashMap::new(), - }; - - let targets = goto_definition( - &db, - offset(7), - &file, - &root, - &idx, - &ExternalScope::default(), - ); +fn test_navigates_across_source_directive() { + let mut db = OakDatabase::new(); + let helpers = upsert(&mut db, "helpers.R", "helper <- function() 1\n"); + let script = upsert(&mut db, "script.R", "source(\"helpers.R\")\nhelper\n"); + // Cursor on the forwarded `helper` use, on the line after the source(). + let offset = TextSize::from("source(\"helpers.R\")\n".len() as u32); + let targets = goto_definition(&db, script, offset); assert_eq!(targets.len(), 1); + let target = &targets[0]; - let target = targets.first().unwrap(); - assert!(target.file.path().ends_with("dplyr.R")); - assert_eq!(target.name, "mutate".to_string()); -} - -#[test] -fn test_namespace_access_unknown_symbol() { - // Symbol not in exports - let source = "dplyr::nonexistent\n"; - let file = file_url("test.R"); - let (root, idx) = parse_source(source); - let library = test_library(vec![TestPackage::new( - "dplyr", - "dplyr.R", - vec!["filter", "mutate", "select"], - vec![], - )]); - let db = TestDb { - library, - sources: HashMap::new(), - }; - - let targets = goto_definition( - &db, - offset(7), - &file, - &root, - &idx, - &ExternalScope::default(), - ); - assert!(targets.is_empty()); -} - -#[test] -fn test_namespace_access_unknown_package() { - let source = "bogus::foo\n"; - let file = file_url("test.R"); - let (root, idx) = parse_source(source); - let library = test_library(vec![]); - let db = TestDb { - library, - sources: HashMap::new(), - }; - - let targets = goto_definition( - &db, - offset(7), - &file, - &root, - &idx, - &ExternalScope::default(), - ); - assert!(targets.is_empty()); -} - -#[test] -fn test_namespace_access_triple_colon() { - let library = test_library(vec![TestPackage::new( - "pkg", - "pkg.R", - vec!["external_fn"], - vec!["internal_fn"], - )]); - let db = TestDb { - library, - sources: HashMap::new(), - }; - - // "pkg:::internal_fn\n" - // 01234567890... - // Cursor on `internal_fn` (offset 6) - let source = "pkg:::internal_fn\n"; - let file = file_url("test.R"); - let (root, idx) = parse_source(source); - let targets = goto_definition( - &db, - offset(6), - &file, - &root, - &idx, - &ExternalScope::default(), - ); - assert_eq!(targets.len(), 1); - let target = targets.first().unwrap(); - assert!(target.file.path().ends_with("pkg.R")); - assert_eq!(target.name, "internal_fn".to_string()); - - // "pkg:::external_fn\n" - // 01234567890... - // Cursor on `external_fn` (offset 6) - let source = "pkg:::external_fn\n"; - let file = file_url("test.R"); - let (root, idx) = parse_source(source); - let targets = goto_definition( - &db, - offset(6), - &file, - &root, - &idx, - &ExternalScope::default(), - ); - assert_eq!(targets.len(), 1); - let target = targets.first().unwrap(); - assert!(target.file.path().ends_with("pkg.R")); - assert_eq!(target.name, "external_fn".to_string()); -} - -#[test] -fn test_fixme_namespace_access_cursor_on_package_name() { - // Cursor on `dplyr` (offset 0) — the LHS of :: - let source = "dplyr::mutate\n"; - let file = file_url("test.R"); - let (root, idx) = parse_source(source); - let library = test_library(vec![TestPackage::new( - "dplyr", - "dplyr.R", - vec!["filter", "mutate", "select"], - vec![], - )]); - let db = TestDb { - library, - sources: HashMap::new(), - }; - - let targets = goto_definition( - &db, - offset(0), - &file, - &root, - &idx, - &ExternalScope::default(), - ); - // FIXME: Cursor on the package name still classifies as NamespaceAccess - // and resolves `mutate` in `dplyr`. - assert_eq!(targets.len(), 1); - let target = targets.first().unwrap(); - assert!(target.file.path().ends_with("dplyr.R")); - assert_eq!(target.name, "mutate".to_string()); -} - -#[test] -fn test_fixme_namespace_access_cursor_on_operator() { - // Cursor on `::` (offset 5) - let source = "dplyr::mutate\n"; - let file = file_url("test.R"); - let (root, idx) = parse_source(source); - let library = test_library(vec![TestPackage::new( - "dplyr", - "dplyr.R", - vec!["filter", "mutate", "select"], - vec![], - )]); - let db = TestDb { - library, - sources: HashMap::new(), - }; - - let targets = goto_definition( - &db, - offset(5), - &file, - &root, - &idx, - &ExternalScope::default(), - ); - // FIXME: Operator token is inside the RNamespaceExpression, still resolves. - assert_eq!(targets.len(), 1); - let target = targets.first().unwrap(); - assert!(target.file.path().ends_with("dplyr.R")); - assert_eq!(target.name, "mutate".to_string()); -} - -#[test] -fn test_namespace_access_in_call() { - // foo::bar() — cursor on `bar` - let source = "foo::bar()\n"; - let file = file_url("test.R"); - let (root, idx) = parse_source(source); - let library = test_library(vec![TestPackage::new("foo", "foo.R", vec!["bar"], vec![])]); - let db = TestDb { - library, - sources: HashMap::new(), - }; - - let targets = goto_definition( - &db, - offset(5), - &file, - &root, - &idx, - &ExternalScope::default(), - ); - - assert_eq!(targets.len(), 1); - - let target = targets.first().unwrap(); - assert!(target.file.path().ends_with("foo.R")); - assert_eq!(target.name, "bar".to_string()); -} - -#[test] -fn test_namespace_access_in_extract() { - // foo::bar$baz — cursor on `bar` - let source = "foo::bar$baz\n"; - let file = file_url("test.R"); - let (root, idx) = parse_source(source); - let library = test_library(vec![TestPackage::new("foo", "foo.R", vec!["bar"], vec![])]); - let db = TestDb { - library, - sources: HashMap::new(), - }; - - let targets = goto_definition( - &db, - offset(5), - &file, - &root, - &idx, - &ExternalScope::default(), - ); - - assert_eq!(targets.len(), 1); - - let target = targets.first().unwrap(); - assert!(target.file.path().ends_with("foo.R")); - assert_eq!(target.name, "bar".to_string()); -} - -// --- source() directive --- - -#[test] -fn test_source_directive_resolves_to_sourced_file() { - // script.R has `source("helpers.R")` then uses `helper`. - // The builder resolves source() via the callback and injects the - // sourced file's exports, enabling goto-definition. - let helpers_source = "helper <- function() 1\n"; - let helpers_url = file_url("helpers.R"); - let (_helpers_root, helpers_idx) = parse_source(helpers_source); - - let script_source = "source(\"helpers.R\")\nhelper\n"; - let script_url = file_url("script.R"); - - let helpers_names: Vec = helpers_idx - .exports() - .keys() - .map(|name| name.to_string()) - .collect(); - - let parsed = parse(script_source, RParserOptions::default()); - let script_root = parsed.syntax(); - let resolver = ConstResolver(SourceResolution { - url: helpers_url.clone(), - names: helpers_names, - packages: Vec::new(), - }); - let script_idx = build_index(&parsed.tree(), resolver); - - let dir_layers = script_idx.semantic_calls().to_vec(); - let scope = ExternalScope::search_path(dir_layers, Vec::new()); - - let mut sources = HashMap::new(); - sources.insert(helpers_url.clone(), helpers_source.to_string()); - let db = TestDb { - library: empty_library(), - sources, - }; - - let use_offset = script_source.rfind("helper").unwrap() as u32; - let targets = goto_definition( - &db, - offset(use_offset), - &script_url, - &script_root, - &script_idx, - &scope, - ); - assert_eq!(targets, vec![NavigationTarget { - file: helpers_url, - name: "helper".to_string(), - full_range: text_range(0, 6), - focus_range: text_range(0, 6), - }]); -} - -#[test] -fn test_source_directive_resolves_nested_library() { - // helpers.R has `library(dplyr)` and defines `helper`. - // script.R sources helpers.R then uses `mutate` (from dplyr). - // The nested library() directive should be visible via the resolver. - let helpers_source = "library(dplyr)\nhelper <- function() 1\n"; - let helpers_url = file_url("helpers.R"); - let (_helpers_root, helpers_idx) = parse_source(helpers_source); - - let helpers_names: Vec = helpers_idx - .exports() - .keys() - .map(|name| name.to_string()) - .collect(); - let helpers_packages: Vec<_> = helpers_idx - .semantic_calls() - .iter() - .filter_map(|c| match c.kind() { - SemanticCallKind::Attach { package } => Some(package.clone()), - SemanticCallKind::Source { .. } => None, - }) - .collect(); - - let script_source = "source(\"helpers.R\")\nmutate\n"; - let script_url = file_url("script.R"); - - let library = test_library(vec![TestPackage::new( - "dplyr", - "dplyr.R", - vec!["filter", "mutate", "select"], - vec![], - )]); - - let parsed = parse(script_source, RParserOptions::default()); - let script_root = parsed.syntax(); - let resolver = ConstResolver(SourceResolution { - url: helpers_url.clone(), - names: helpers_names.clone(), - packages: helpers_packages.clone(), - }); - let script_idx = build_index(&parsed.tree(), resolver); - - let dir_layers = script_idx.semantic_calls().to_vec(); - let scope = ExternalScope::search_path(dir_layers, Vec::new()); - - let mut sources = HashMap::new(); - sources.insert(helpers_url.clone(), helpers_source.to_string()); - let db = TestDb { library, sources }; - - // `mutate` resolves via dplyr (attached by helpers.R's library() call) - let use_offset = script_source.rfind("mutate").unwrap() as u32; - let targets = goto_definition( - &db, - offset(use_offset), - &script_url, - &script_root, - &script_idx, - &scope, - ); - // `mutate` resolves via dplyr (attached by helpers.R's library() call) - assert!(!targets.is_empty()); - let source_with_helper = "source(\"helpers.R\")\nhelper\n"; - - let parsed2 = parse(source_with_helper, RParserOptions::default()); - let script_root2 = parsed2.syntax(); - let resolver2 = ConstResolver(SourceResolution { - url: helpers_url.clone(), - names: helpers_names, - packages: helpers_packages, - }); - let script_idx2 = build_index(&parsed2.tree(), resolver2); - - let dir_layers = script_idx2.semantic_calls().to_vec(); - let scope = ExternalScope::search_path(dir_layers, Vec::new()); - - let use_offset = source_with_helper.rfind("helper").unwrap() as u32; - let targets = goto_definition( - &db, - offset(use_offset), - &script_url, - &script_root2, - &script_idx2, - &scope, - ); - assert_eq!(targets, vec![NavigationTarget { - file: helpers_url, - name: "helper".to_string(), - full_range: text_range(15, 21), - focus_range: text_range(15, 21), - }]); -} - -#[test] -fn test_directive_not_visible_before_call_site() { - // Directives are position-stamped: only code AFTER a `source()` or - // `library()` call sees its effects. - // - // "mutate\n" offset 0..6 - // "helper\n" offset 7..13 - // "library(dplyr)\n" offset 14..28 - // "source(\"helpers.R\")\n" offset 29..48 - // "mutate\n" offset 49..55 - // "helper\n" offset 56..62 - let helpers_source = "helper <- function() 1\n"; - let helpers_url = file_url("helpers.R"); - let (_helpers_root, helpers_idx) = parse_source(helpers_source); - - let script_source = "mutate\nhelper\nlibrary(dplyr)\nsource(\"helpers.R\")\nmutate\nhelper\n"; - let script_url = file_url("script.R"); - - let library = test_library(vec![TestPackage::new( - "dplyr", - "dplyr.R", - vec!["filter", "mutate", "select"], - vec![], - )]); - - let helpers_names: Vec = helpers_idx - .exports() - .keys() - .map(|name| name.to_string()) - .collect(); - - let parsed = parse(script_source, RParserOptions::default()); - let script_root = parsed.syntax(); - let resolver = ConstResolver(SourceResolution { - url: helpers_url.clone(), - names: helpers_names, - packages: Vec::new(), - }); - let script_idx = build_index(&parsed.tree(), resolver); - - let dir_layers = script_idx.semantic_calls().to_vec(); - let scope = ExternalScope::search_path(dir_layers, Vec::new()); - - let mut sources = HashMap::new(); - sources.insert(helpers_url.clone(), helpers_source.to_string()); - let db = TestDb { library, sources }; - - // `mutate` before library(dplyr) (offset 0) — should NOT resolve - let targets = goto_definition( - &db, - offset(0), - &script_url, - &script_root, - &script_idx, - &scope, - ); - assert!(targets.is_empty()); - - // `helper` before source() (offset 7) — should NOT resolve - let targets = goto_definition( - &db, - offset(7), - &script_url, - &script_root, - &script_idx, - &scope, - ); - assert!(targets.is_empty()); - - // `mutate` after library(dplyr) (offset 49) — resolves via dplyr - let targets = goto_definition( - &db, - offset(49), - &script_url, - &script_root, - &script_idx, - &scope, - ); - assert!(!targets.is_empty()); - - // `helper` after source() (offset 56) — should resolve to helpers.R - let targets = goto_definition( - &db, - offset(56), - &script_url, - &script_root, - &script_idx, - &scope, - ); - assert_eq!(targets, vec![NavigationTarget { - file: helpers_url, - name: "helper".to_string(), - full_range: text_range(0, 6), - focus_range: text_range(0, 6), - }]); -} - -#[test] -fn test_directives_in_function_body_are_scoped() { - // `library()` inside a function body produces a scoped Attach - // semantic call: visible inside the function but not at file scope. - // The `source()` call is recorded as a Source semantic call, - // independent of the legacy resolver path. - let script_source = - "f <- function() {\n source(\"helpers.R\")\n library(dplyr)\n mutate\n}\nhelper\nmutate\n"; - let script_url = file_url("script.R"); - let (script_root, script_idx) = parse_source(script_source); - - let library = test_library(vec![TestPackage::new( - "dplyr", - "dplyr.R", - vec!["filter", "mutate", "select"], - vec![], - )]); - let db = TestDb { - library, - sources: HashMap::new(), - }; - - // Both source() and library() inside f are recorded as scoped - // semantic calls, in source order. - let semantic_calls = script_idx.semantic_calls(); - assert_eq!(semantic_calls.len(), 2); - assert_eq!(semantic_calls[0].kind(), &SemanticCallKind::Source { - path: "helpers.R".into(), - resolved: None, - }); - assert_eq!(semantic_calls[1].kind(), &SemanticCallKind::Attach { - package: "dplyr".into() - }); - assert_ne!(semantic_calls[0].scope(), ScopeId::from(0)); - assert_ne!(semantic_calls[1].scope(), ScopeId::from(0)); - - let dir_layers = script_idx.semantic_calls().to_vec(); - let scope = ExternalScope::search_path(dir_layers, Vec::new()); - - // `mutate` inside f (after library()) — resolves via scoped dplyr - let use_offset = script_source.find(" mutate").unwrap() as u32 + 2; - let targets = goto_definition( - &db, - offset(use_offset), - &script_url, - &script_root, - &script_idx, - &scope, - ); - // `mutate` inside f (after library()) — resolves via scoped dplyr - assert!(!targets.is_empty()); - - // `helper` at file scope — not resolved (source() had no resolver) - let use_offset = script_source.find("\nhelper").unwrap() as u32 + 1; - let targets = goto_definition( - &db, - offset(use_offset), - &script_url, - &script_root, - &script_idx, - &scope, - ); - assert!(targets.is_empty()); - - // `mutate` at file scope — not resolved (library() directive is - // scoped to f, not visible here) - let use_offset = script_source.rfind("mutate").unwrap() as u32; - let targets = goto_definition( - &db, - offset(use_offset), - &script_url, - &script_root, - &script_idx, - &scope, - ); - assert!(targets.is_empty()); -} - -#[test] -fn test_source_in_function_body_scoping() { - // `source(local = FALSE)` inside a function body scopes directives to the - // function scope, so sourced definitions are NOT visible at file scope. - let helpers_source = "helper <- function() 1\n"; - let helpers_url = file_url("helpers.R"); - let (_helpers_root, helpers_idx) = parse_source(helpers_source); - - let script_source = "f <- function() {\n source(\"helpers.R\")\n helper\n}\nhelper\n"; - let script_url = file_url("script.R"); - - let helpers_names: Vec = helpers_idx - .exports() - .keys() - .map(|name| name.to_string()) - .collect(); - - let parsed = parse(script_source, RParserOptions::default()); - let script_root = parsed.syntax(); - let resolver = ConstResolver(SourceResolution { - url: helpers_url.clone(), - names: helpers_names, - packages: Vec::new(), - }); - let script_idx = build_index(&parsed.tree(), resolver); - - let dir_layers = script_idx.semantic_calls().to_vec(); - let scope = ExternalScope::search_path(dir_layers, Vec::new()); - - let mut sources = HashMap::new(); - sources.insert(helpers_url.clone(), helpers_source.to_string()); - let db = TestDb { - library: empty_library(), - sources, - }; - - // `helper` inside the function body — should resolve to helpers.R - let inner_offset = script_source.find(" helper\n}").unwrap() as u32 + 2; - let targets = goto_definition( - &db, - offset(inner_offset), - &script_url, - &script_root, - &script_idx, - &scope, - ); - assert_eq!(targets, vec![NavigationTarget { - file: helpers_url, - name: "helper".to_string(), - full_range: text_range(0, 6), - focus_range: text_range(0, 6), - }]); - - // `helper` outside the function — NOT visible - let outer_offset = script_source.rfind("\nhelper\n").unwrap() as u32 + 1; - let targets = goto_definition( - &db, - offset(outer_offset), - &script_url, - &script_root, - &script_idx, - &scope, - ); - assert!(targets.is_empty()); -} - -// TODO(salsa): This tests `resolve_import` which is slated to move to -// `oak_semantic`. Move this test alongside it and call `resolve_import` directly -// once it becomes pub. -#[test] -fn test_resolve_import_last_def_wins() { - // If the target file defines the same name twice, resolve_import - // should navigate to the last definition. - let helpers_url = file_url("helpers.R"); - let script_url = file_url("script.R"); - - let db = TestDb { - library: empty_library(), - sources: HashMap::from([( - helpers_url.clone(), - "foo <- function() 'first'\nfoo <- function() 'second'\n".to_string(), - )]), - }; - - let script_source = "source(\"helpers.R\")\nfoo\n"; - let parsed = parse(script_source, RParserOptions::default()); - let script_root = parsed.syntax(); - let resolver = ConstResolver(SourceResolution { - url: helpers_url.clone(), - names: vec!["foo".to_string()], - packages: Vec::new(), - }); - let script_idx = build_index(&parsed.tree(), resolver); - - let dir_layers = script_idx.semantic_calls().to_vec(); - let scope = ExternalScope::search_path(dir_layers, Vec::new()); - - let use_offset = script_source.rfind("foo").unwrap() as u32; - let targets = goto_definition( - &db, - offset(use_offset), - &script_url, - &script_root, - &script_idx, - &scope, - ); - - // Should resolve to the SECOND definition of `foo` in helpers.R (line 1) - assert_eq!(targets, vec![NavigationTarget { - file: helpers_url, - name: "foo".to_string(), - full_range: text_range(26, 29), - focus_range: text_range(26, 29), - }]); -} + // The target lives in the sourced file, in that file's coordinates. + assert_eq!(target.file, helpers); + assert_eq!(target.name, "helper"); + assert_eq!(target.full_range, range(0, 6)); } diff --git a/crates/oak_ide/tests/integration/identifier.rs b/crates/oak_ide/tests/integration/identifier.rs new file mode 100644 index 0000000000..3f1e01ffa3 --- /dev/null +++ b/crates/oak_ide/tests/integration/identifier.rs @@ -0,0 +1,140 @@ +use aether_parser::parse; +use aether_parser::RParserOptions; +use biome_rowan::TextRange; +use biome_rowan::TextSize; +use oak_ide::Identifier; +use oak_semantic::build_index; +use oak_semantic::NoopImportsResolver; + +fn text_range(start: u32, end: u32) -> TextRange { + TextRange::new(TextSize::from(start), TextSize::from(end)) +} + +fn offset(n: u32) -> TextSize { + TextSize::from(n) +} + +#[test] +fn test_namespace_classify() { + let source = "dplyr::mutate\n"; + let parsed = parse(source, RParserOptions::default()); + let root = parsed.syntax(); + let idx = build_index(&parsed.tree(), NoopImportsResolver); + + // Cursor on `mutate` (offset 7) + let ident = Identifier::classify(&idx, &root, offset(7)); + assert_eq!( + ident, + Some(Identifier::NamespaceAccess { + package: "dplyr".to_string(), + symbol: "mutate".to_string(), + internal: false, + package_range: text_range(0, 5), + symbol_range: text_range(7, 13), + }) + ); + + // Cursor on `dplyr` (offset 2) + let ident = Identifier::classify(&idx, &root, offset(2)); + assert_eq!( + ident, + Some(Identifier::NamespaceAccess { + package: "dplyr".to_string(), + symbol: "mutate".to_string(), + internal: false, + package_range: text_range(0, 5), + symbol_range: text_range(7, 13), + }) + ); +} + +#[test] +fn test_namespace_classify_triple_colon() { + let source = "pkg:::sym\n"; + let parsed = parse(source, RParserOptions::default()); + let root = parsed.syntax(); + let idx = build_index(&parsed.tree(), NoopImportsResolver); + + let ident = Identifier::classify(&idx, &root, offset(6)); + assert_eq!( + ident, + Some(Identifier::NamespaceAccess { + package: "pkg".to_string(), + symbol: "sym".to_string(), + internal: true, + package_range: text_range(0, 3), + symbol_range: text_range(6, 9), + }) + ); +} + +#[test] +fn test_namespace_classify_in_call() { + // foo::bar() + // 0123456789 + let source = "foo::bar()\n"; + let parsed = parse(source, RParserOptions::default()); + let root = parsed.syntax(); + let idx = build_index(&parsed.tree(), NoopImportsResolver); + + let ident = Identifier::classify(&idx, &root, offset(5)); + assert_eq!( + ident, + Some(Identifier::NamespaceAccess { + package: "foo".to_string(), + symbol: "bar".to_string(), + internal: false, + package_range: text_range(0, 3), + symbol_range: text_range(5, 8), + }) + ); +} + +#[test] +fn test_namespace_classify_in_extract() { + // foo::bar$baz + // 0123456789... + let source = "foo::bar$baz\n"; + let parsed = parse(source, RParserOptions::default()); + let root = parsed.syntax(); + let idx = build_index(&parsed.tree(), NoopImportsResolver); + + // Cursor on `bar` (offset 5) -- inside the RNamespaceExpression + let ident = Identifier::classify(&idx, &root, offset(5)); + assert_eq!( + ident, + Some(Identifier::NamespaceAccess { + package: "foo".to_string(), + symbol: "bar".to_string(), + internal: false, + package_range: text_range(0, 3), + symbol_range: text_range(5, 8), + }) + ); + + // Cursor on `baz` (offset 9) -- RHS of $, not a namespace access + let ident = Identifier::classify(&idx, &root, offset(9)); + assert_eq!(ident, None); +} + +#[test] +fn test_namespace_classify_string_selectors() { + // "foo"::"bar" + // 0123456789... + let source = "\"foo\"::\"bar\"\n"; + let parsed = parse(source, RParserOptions::default()); + let root = parsed.syntax(); + let idx = build_index(&parsed.tree(), NoopImportsResolver); + + let ident = Identifier::classify(&idx, &root, offset(7)); + assert_eq!( + ident, + Some(Identifier::NamespaceAccess { + package: "foo".to_string(), + symbol: "bar".to_string(), + internal: false, + package_range: text_range(0, 5), + symbol_range: text_range(7, 12), + }) + ); +} diff --git a/crates/oak_ide/tests/integration/main.rs b/crates/oak_ide/tests/integration/main.rs index 03fa8ada7a..7d6e7952c2 100644 --- a/crates/oak_ide/tests/integration/main.rs +++ b/crates/oak_ide/tests/integration/main.rs @@ -1,3 +1,4 @@ mod find_references; mod goto_definition; +mod identifier; mod rename; From 644b1b5c75e40f22fe12ad54856314edfc56b18d Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Wed, 3 Jun 2026 15:14:52 +0200 Subject: [PATCH 2/3] Update TODO about out-of-workspace `source()` --- crates/oak_db/src/imports.rs | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/crates/oak_db/src/imports.rs b/crates/oak_db/src/imports.rs index c29a5c5a95..3719765abf 100644 --- a/crates/oak_db/src/imports.rs +++ b/crates/oak_db/src/imports.rs @@ -53,11 +53,18 @@ impl<'db> ImportsResolver for SalsaImportsResolver<'db> { fn resolve_source(&mut self, path: &str) -> Option { let anchor = anchor_dir(self.db, self.calling_file)?; let url = resolve_relative_to(&anchor, path)?; - // TODO(diagnostics): a `source()` target outside the workspace never - // becomes a `File` (nothing watches or scans it), so `file_by_url` - // misses it and the names it injects are invisible. Picking these up - // (to resolve and to lint) needs a non-LSP file watcher we don't have - // yet. + // TODO: a `source()` target outside every workspace root never becomes + // a `File`, so `file_by_url()` misses it and the names it injects stay + // invisible. Minting can't happen here, so the work belongs on the + // write side in `oak_scan`. We should carry the resolved URL on the + // directive even when no `File` exists (today the miss returns `None` + // and drops it), then have `oak_scan` enumerate source directives after + // a scan, mint an `OrphanRoot` `File` from disk for each + // out-of-workspace target, and iterate for `source()` chains. A file + // watcher is only needed for freshness (re-reading after an external + // edit), plus GC to drop the orphan once the directive goes away. + // TODO(diagnostics): Until we support out-of-workspace sourced files, + // should we at least lint so user knows that we can't analyse the file? let file = self.db.file_by_url(&url)?; let names: Vec = file From 399f3f3dbfb075e98c6033b7c2e1b7584dc3e5cf Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Wed, 3 Jun 2026 15:40:10 +0200 Subject: [PATCH 3/3] Fix test on Windows --- crates/oak_ide/tests/integration/goto_definition.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/crates/oak_ide/tests/integration/goto_definition.rs b/crates/oak_ide/tests/integration/goto_definition.rs index 481143bb15..fbf079fddd 100644 --- a/crates/oak_ide/tests/integration/goto_definition.rs +++ b/crates/oak_ide/tests/integration/goto_definition.rs @@ -16,7 +16,13 @@ use oak_scan::DbExt; use url::Url; fn file_url(name: &str) -> Url { - Url::parse(&format!("file:///project/R/{name}")).unwrap() + // `Url::to_file_path` on Windows requires a drive-letter prefix, so + // synthesize one for tests. Linux is happy with rootless paths. + if cfg!(windows) { + Url::parse(&format!("file:///C:/project/R/{name}")).unwrap() + } else { + Url::parse(&format!("file:///project/R/{name}")).unwrap() + } } fn upsert(db: &mut OakDatabase, name: &str, contents: &str) -> File {